{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/3958","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/3958/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/3958/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/3958/events","html_url":"https://github.com/cockroachdb/cockroach/pull/3958","id":128584159,"number":3958,"title":"sql: support prepare INSERT statements","user":{"login":"mjibson","id":41181,"avatar_url":"https://avatars.githubusercontent.com/u/41181?v=3","gravatar_id":"","url":"https://api.github.com/users/mjibson","html_url":"https://github.com/mjibson","followers_url":"https://api.github.com/users/mjibson/followers","following_url":"https://api.github.com/users/mjibson/following{/other_user}","gists_url":"https://api.github.com/users/mjibson/gists{/gist_id}","starred_url":"https://api.github.com/users/mjibson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mjibson/subscriptions","organizations_url":"https://api.github.com/users/mjibson/orgs","repos_url":"https://api.github.com/users/mjibson/repos","events_url":"https://api.github.com/users/mjibson/events{/privacy}","received_events_url":"https://api.github.com/users/mjibson/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2016-01-25T16:49:20Z","updated_at":"2016-01-25T18:23:18Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/3958","html_url":"https://github.com/cockroachdb/cockroach/pull/3958","diff_url":"https://github.com/cockroachdb/cockroach/pull/3958.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/3958.patch"},"body":"Make sure prepare statements are run in a transaction.\r\n\r\nUse a new boolean flag on the planner to indicate prepare. Statement\r\nimplementations will gain knowledge of work to omit during prepare. An\r\nalternative implementation would split up the prepare and exec portions of\r\nthe work. However, this would require passing around significant amounts\r\nof state and I think the best implementation is the one here.\r\n\r\nKeep a whitelist of statement types that support preparation instead of\r\nattempting to exec them during prepare.\r\n\r\nFixes #3819\n\n<!-- Reviewable:start -->\n[<img src=\"https://reviewable.io/review_button.png\" height=40 alt=\"Review on Reviewable\"/>](https://reviewable.io/reviews/cockroachdb/cockroach/3958)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/174610169","html_url":"https://github.com/cockroachdb/cockroach/pull/3958#issuecomment-174610169","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/3958","id":174610169,"user":{"login":"petermattis","id":6201335,"avatar_url":"https://avatars.githubusercontent.com/u/6201335?v=3","gravatar_id":"","url":"https://api.github.com/users/petermattis","html_url":"https://github.com/petermattis","followers_url":"https://api.github.com/users/petermattis/followers","following_url":"https://api.github.com/users/petermattis/following{/other_user}","gists_url":"https://api.github.com/users/petermattis/gists{/gist_id}","starred_url":"https://api.github.com/users/petermattis/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/petermattis/subscriptions","organizations_url":"https://api.github.com/users/petermattis/orgs","repos_url":"https://api.github.com/users/petermattis/repos","events_url":"https://api.github.com/users/petermattis/events{/privacy}","received_events_url":"https://api.github.com/users/petermattis/received_events","type":"User","site_admin":false},"created_at":"2016-01-25T18:23:18Z","updated_at":"2016-01-25T18:23:18Z","body":"I'm not enthused by the `prepareOnly` approach. Feels fragile to have to sprinkle `if p.prepareOnly` in the code rather than have a distinct `prepare` phase (performed by `makePlan`) and then have all of the work done in the first call to `plan.Next`. Perhaps I'm worrying unnecessarily. \n\nIt's unfortunate that we have to replicate so much work for prepared statements that are subsequently executed. We'll likely have to fix that at some point. \n\n---\n\nReview status: 0 of 11 files reviewed at latest revision, 3 unresolved discussions.\n\n---\n\n*[sql/executor.go, line 163 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/3958#-K8tzQdi4vOYpMGIrd6L:-K8tzQdi4vOYpMGIrd6M:434047532) ([raw file](https://github.com/cockroachdb/cockroach/blob/613f6d2b2a2128310ade9feacb19e0585d4fca53/sql/executor.go#L163)):*\nStatement preparation should never mutate the database. Perhaps abort the transaction here. Even better, rather than using `DB.Txn` (which commits) you should manually construct a `client.Txn` here and never commit it. \n\n---\n\n*[sql/pgwire_test.go, line 340 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/3958#-K8txk9vMPVzAQqoP49g:-K8txk9vMPVzAQqoP49h:-721381561) ([raw file](https://github.com/cockroachdb/cockroach/blob/613f6d2b2a2128310ade9feacb19e0585d4fca53/sql/pgwire_test.go#L340)):*\nI think it would be good to be more exhaustive in the statements that are tested here.\n\n---\n\n*[sql/plan.go, line 160 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/3958#-K8tyn2xcEPJHSw5OEtA:-K8tyn2xcEPJHSw5OEtB:-1870851779) ([raw file](https://github.com/cockroachdb/cockroach/blob/613f6d2b2a2128310ade9feacb19e0585d4fca53/sql/plan.go#L160)):*\nIs the plan to add support for `prepareOnly` for all statements?\n\n---\n\n\n*Comments from the [review on Reviewable.io](https://reviewable.io:443/reviews/cockroachdb/cockroach/3958#-:-K8u-Y5Py2spZ0_7M4Up:1523888340)*\n<!-- Sent from Reviewable.io -->\n"}}