sql: allow BEGIN READ WRITE#16348
sql: allow BEGIN READ WRITE#16348madelynnblue merged 1 commit intocockroachdb:release-1.0from madelynnblue:begin-read-write
Conversation
|
This change is |
|
Can you bump lib/pq and add a test that exercises this through txn options? Reviewed 4 of 4 files at r1. Comments from Reviewable |
all files reviewed at latest revision, all discussions resolved, all commit checks successful.|
Yes a test with the new libpq which motivated this change would be nice. LGTM otherwise. Reviewed 4 of 4 files at r1. pkg/sql/parser/sql.y, line 2311 at r1 (raw file):
Does "READ ONLY" require the server to respond an error when writing? If not (it is only advisory), we could support it too. Comments from Reviewable |
|
I think adding support for READ ONLY in a patch release is a bad idea because it's more than just a syntax change. I don't know how it behaves, but don't want to investigate for this. The only thing this PR is attempting to address is Go users using the master branch of lib/pq and trying to connect to cockroach 1.0 and failing because of our lack of READ WRITE syntax support. Anything beyond that will go into our master branch as a more general fix. I'll update lib/pq, but that means making a vendored/release-1.0 branch in the vendored repo so the ref doesn't get GCd. We don't have a process around that so I guess I'll just go for it, agreed? |
|
Aw I hadn't seen this was a 1.0.x fix. Yes, this should merge as-is. |
|
Do you think it should merge as-is without the associated lib/pq update? I do. I think a lib/pq update should go into the master branch when this issue is fixed there. Thoughts @tamird? |
|
I am slightly against updating the pq dep in 1.0.x. The only part of cockroachdb that uses this is the CLI shell, and this does not use these options. |
|
Wait, why is this going to 1.0 before it goes to master?
…On Jun 6, 2017 10:26, "kena" ***@***.***> wrote:
I am slightly against updating the pq dep in 1.0.x. The only part of
cockroachdb that uses this is the CLI shell, and this does not use these
options.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16348 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPMsuQfYaRUGGypkUnsoqoNZscFw4ks5sBYvQgaJpZM4Nwz1Y>
.
|
|
Tamir, asking the right questions :-) |
|
Because the full fix is much larger: https://github.com/mjibson/cockroach/commit/31d27670854c76e8dedace725f9ab9f160c8de63 and make take a few days to merge, and I want this fix in 1.0.2. This is similar to a PR merged yesterday (#16335) where the 1.0 PR was a 3 line change, and the full fix was much larger. |
|
@mjibson I am ok with this PR as a reduced form of the larger commit, however I do think the larger commit should already have a PR open and that you refer to that other PR here (or in the commit message). This will help tracking the relationship down the line. Also I see you do support the syntax READ ONLY in the larger commit, if only to error out as being not supported. I think you can do the same here using a grammar rule |
|
LGTM. I agree that we shouldn't mess with deps in the release branch and shouldn't add |
lib/pq recently added support for the Go 1.8 txn options, which include a field for read only/write transactions. In the default (read write) case, the string "READ WRITE" is added to the BEGIN statement. Add workaround support for this syntax by ignoring it, since it doesn't change anything. A complete fix for this will be added to the master branch later. See #16309
|
Updated to return unimplemented on READ ONLY. |
|
Reviewed 3 of 3 files at r2. Comments from Reviewable |
lib/pq recently added support for the Go 1.8 txn options, which
include a field for read only/write transactions. In the default
(read write) case, the string "READ WRITE" is added to the BEGIN
statement. Add workaround support for this syntax by ignoring it,
since it doesn't change anything. A complete fix for this will be
added to the master branch later.
See #16309
cc @cockroachdb/release