Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql/pgwire: send empty query response instead of command complete on empty query #3852

Closed
maddyblue opened this issue Jan 14, 2016 · 13 comments
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Milestone

Comments

@maddyblue
Copy link
Contributor

When sending an empty query, we respond with command complete (C), real Postgres responds with empty query (I).

@dt
Copy link
Member

dt commented Jan 15, 2016

Related to #3849 so I'll combine and do both in #3853

@dt
Copy link
Member

dt commented Jan 15, 2016

I guess one question is whether this is pgwire-specific or not: should we detect empty queries in pgwire and kill them early, or leave it up to the sql executor and add an "empty" value to the driver response?

@tamird
Copy link
Contributor

tamird commented Jan 15, 2016

Fixing this as pg-specific seems fine to me.

@dt
Copy link
Member

dt commented Jan 15, 2016

Hm, this example has me second guessing that:

irb(main):025:0> p.exec(" ; ; ;")
=> #<PG::Result:0x007f7fe18e2ca0 status=PGRES_EMPTY_QUERY ntuples=0 nfields=0 cmd_tuples=0>

Some combination of strings.Trim might work here, but that feels dirty to me? Maybe this is better left to a real parser?

@tamird
Copy link
Contributor

tamird commented Jan 15, 2016

That's interesting; what if you do p.exec("select 1; ; ;")?

@dt
Copy link
Member

dt commented Jan 15, 2016

That one is TUPLES_OK and my other test was p.exec("; ; ; BEGIN;") which is COMMAND_OK.

@tamird
Copy link
Contributor

tamird commented Jan 15, 2016

Weird. Off-topic now but what does p.exec("SELECT 1; ; ; BEGIN") return?

Anyway, yes, leave it to the parser.

@dt
Copy link
Member

dt commented Jan 15, 2016

OK. Seems like anything other than all empty queries gets an ok.
conn2.exec("; ; ; BEGIN; END; ; ;") => #<PG::Result:0x007f7fe5093640 status=PGRES_COMMAND_OK ntuples=0 nfields=0 cmd_tuples=0>

@petermattis
Copy link
Collaborator

I'm pretty sure the parser already handles stripping out empty statements.

@dt
Copy link
Member

dt commented Jan 15, 2016

Right, I think the missing piece is communicating back to the pgwire code, that after parsing, all queries were empty.

@petermattis
Copy link
Collaborator

Would be easier to do that if we weren't going through the sql/driver protos.

dt added a commit to dt/cockroach that referenced this issue Jan 15, 2016
also explicitly send EmptyQuery than execute empty query strings.

Fixes cockroachdb#3849

Starts to address cockroachdb#3852 but, as noted in TODO and on the issue,
correctly handling more complex empty queries (e.g. "; ; ;") is
better left to the actual parser, which will require plumbing that
through the driver too.
dt added a commit to dt/cockroach that referenced this issue Jan 16, 2016
also explicitly send EmptyQuery than execute empty query strings.

Fixes cockroachdb#3849

Starts to address cockroachdb#3852 but, as noted in TODO and on the issue,
correctly handling more complex empty queries (e.g. "; ; ;") is
better left to the actual parser, which will require plumbing that
through the driver too.
dt added a commit to dt/cockroach that referenced this issue Jan 21, 2016
also explicitly send EmptyQuery than execute empty query strings.

Fixes cockroachdb#3849

Starts to address cockroachdb#3852 but, as noted in TODO and on the issue,
correctly handling more complex empty queries (e.g. "; ; ;") is
better left to the actual parser, which will require plumbing that
through the driver too.
dt added a commit to dt/cockroach that referenced this issue Jan 21, 2016
also explicitly send EmptyQuery than execute empty query strings.

Fixes cockroachdb#3849

Starts to address cockroachdb#3852 but, as noted in TODO and on the issue,
correctly handling more complex empty queries (e.g. "; ; ;") is
better left to the actual parser, which will require plumbing that
through the driver too.
@knz
Copy link
Contributor

knz commented Feb 7, 2016

This is likely addressed by #4081.

@petermattis petermattis added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Feb 14, 2016
@petermattis petermattis modified the milestone: Beta Feb 14, 2016
@dt
Copy link
Member

dt commented Feb 17, 2016

@knz #4081 is our lib/pq-backed client shell. This issue is about the actual server response -- when postgres gets multiple statements that are all empty after parsing, it replies with EMPTY_QUERY, whereas we're only sending that if we have a single, trivially empty statement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

No branches or pull requests

5 participants