psql FETCH_COUNT feature does not work with combined queries
Hello devs,
As pointed out by Kyotaro Horiguchi in
/messages/by-id/20190726.131704.86173346.horikyota.ntt@gmail.com
FETCH_COUNT does not work with combined queries, and probably has never
worked since 2006.
What seems to happen is that ExecQueryUsingCursor is hardcoded to handle
one simple query. It simply inserts the cursor generation in front of the
query believed to be a select:
DECLARE ... <query>
For combined queries, say two selects, it results in:
DECLARE ... <first select>; <second select>
Then PQexec returns the result of the second one, and nothing is printed.
However, if the second query is not a select, eg: "select ... \; update
... ;", the result of the *first* query is shown.
How fun!
This is because PQexec returns the second result. The cursor declaration
expects a PGRES_COMMAND_OK before proceeding. With a select it gets
PGRES_TUPLES_OK so decides it is an error and silently skips to the end.
With the update it indeed obtains the expected PGRES_COMMAND_OK, not
really for the command it sent but who cares, and proceeds to show the
cursor results.
Basically, the whole logic is broken.
The minimum is to document that it does not work properly with combined
queries. Attached patch does that, so that the bug becomes a documented
bug, aka a feature:-)
Otherwise, probably psql lexer could detect, with some efforts, that it is
a combined query (detect embedded ; and check that they are not empty
queries), so that it could skip the feature if it is the case.
Another approach would be to try to detect if the returned result does not
correspond to the cursor one reliably. Maybe some result counting could be
added somewhere so that the number of results under PQexec is accessible
to the user, i.e. result struct would contain its own number. Hmmm.
A more complex approach would be to keep the position of embedded queries,
and to insert cursor declarations where needed, currently the last one if
it is a SELECT. However, for the previous ones the allocation and such
could be prohibitive as no cursor would be used. Not sure it is worth the
effort as the bug has not been detected for 13 years.
--
Fabien.
Attachments:
psql-fetch-count-doc-1.patchtext/x-diff; name=psql-fetch-count-doc-1.patchDownload
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 7789fc6177..d217d82f57 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3684,6 +3684,12 @@ bar
fail after having already displayed some rows.
</para>
+ <note>
+ <para>
+ This feature does not work properly with combined queries.
+ </para>
+ </note>
+
<tip>
<para>
Although you can use any output format with this feature,
FETCH_COUNT does not work with combined queries, and probably has never
worked since 2006.
For the record, this bug has already been reported & discussed by Daniel
Vￜritￜ a few months back, see:
/messages/by-id/a0a854b6-563c-4a11-bf1c-d6c6f924004d@manitou-mail.org
Given the points of view expressed on this thread, especially Tom's view
against improving the lexer used by psql to known where query ends, it
looks that my documentation patch is the way to go in the short term, and
that if the "always show query results" patch is committed, it might
simplify a bit solving this issue as the PQexec call is turned into
PQsendQuery.
--
Fabien.
On Fri, Jul 26, 2019 at 04:13:23PM +0000, Fabien COELHO wrote:
FETCH_COUNT does not work with combined queries, and probably has
never worked since 2006.For the record, this bug has already been reported & discussed by
Daniel V�rit� a few months back, see:/messages/by-id/a0a854b6-563c-4a11-bf1c-d6c6f924004d@manitou-mail.org
Given the points of view expressed on this thread, especially Tom's
view against improving the lexer used by psql to known where query
ends, it looks that my documentation patch is the way to go in the
short term, and that if the "always show query results" patch is
committed, it might simplify a bit solving this issue as the PQexec
call is turned into PQsendQuery.
Assuming there's no one willing to fix the behavior (and that seems
unlikely given we're in the middle of a 2020-01 commitfest) it makes
sense to at least document the behavior.
That being said, I think the proposed patch may be a tad too brief. I
don't think we need to describe the exact behavior, of course, but maybe
it'd be good to mention that the behavior depends on the type of the
last command etc. Also, I don't think "combined query" is a term used
anywhere in the code/docs - I think the proper term is "multi-query
string".
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi Fabien,
On 1/6/20 5:20 PM, Tomas Vondra wrote:
On Fri, Jul 26, 2019 at 04:13:23PM +0000, Fabien COELHO wrote:
FETCH_COUNT does not work with combined queries, and probably has
never worked since 2006.For the record, this bug has already been reported & discussed by
Daniel Vérité a few months back, see:/messages/by-id/a0a854b6-563c-4a11-bf1c-d6c6f924004d@manitou-mail.org
Given the points of view expressed on this thread, especially Tom's
view against improving the lexer used by psql to known where query
ends, it looks that my documentation patch is the way to go in the
short term, and that if the "always show query results" patch is
committed, it might simplify a bit solving this issue as the PQexec
call is turned into PQsendQuery.Assuming there's no one willing to fix the behavior (and that seems
unlikely given we're in the middle of a 2020-01 commitfest) it makes
sense to at least document the behavior.That being said, I think the proposed patch may be a tad too brief. I
don't think we need to describe the exact behavior, of course, but maybe
it'd be good to mention that the behavior depends on the type of the
last command etc. Also, I don't think "combined query" is a term used
anywhere in the code/docs - I think the proper term is "multi-query
string".
Any thoughts on Tomas' comments?
I'll mark this Waiting on Author because I feel like some response
and/or a new patch is needed.
Regards,
--
-David
david@pgmasters.net
Assuming there's no one willing to fix the behavior (and that seems
unlikely given we're in the middle of a 2020-01 commitfest) it makes
sense to at least document the behavior.That being said, I think the proposed patch may be a tad too brief. I
don't think we need to describe the exact behavior, of course, but maybe
it'd be good to mention that the behavior depends on the type of the
last command etc. Also, I don't think "combined query" is a term used
anywhere in the code/docs - I think the proper term is "multi-query
string".Any thoughts on Tomas' comments?
Sorry, I did not notice them.
I tried "multi-command", matching some wording use elsewhere in the file.
I'm at lost about how to describe the insane behavior the feature has when
they are used, which is really just an unfixed bug, so I expanded a minima
in the attached v2.
--
Fabien.
Attachments:
psql-fetch-count-doc-2.patchtext/x-diff; name=psql-fetch-count-doc-2.patchDownload
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 402c4b1b26..b4a9fc3b60 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3781,6 +3781,14 @@ bar
fail after having already displayed some rows.
</para>
+ <note>
+ <para>
+ This feature does not works properly with multi-command
+ (<literal>\;</literal>) queries as it depends on the command
+ types encountered in the string.
+ </para>
+ </note>
+
<tip>
<para>
Although you can use any output format with this feature,
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: tested, passed
Fabien,
There is a minor typo (works -> work) but overall I'm not a fan of the wording.
Instead of a note maybe add a paragraph stating:
"This setting is ignored when multiple statements are sent to the server as a single command (i.e., via the -c command line option or the \; meta-command). Additionally, it is possible for certain combinations of statements sent in that manner to instead return no results, or even be ignored, instead of returning the result set of the last query. In short, when FETCH_COUNT is non-zero, and you send a multi-statement command to the server, the results are undefined. This combination in presently allowed for backward compatibility."
I would suggest, however, adding some tests in src/test/psql.sql demonstrating the broken behavior. A potentially useful test setup would be:
create temp table testtbl (idx int, div int);
insert into testtbl (1,1), (2,1),(3,1),(4,0),(5,1);
and combine that with FETCH_COUNT 3
Some other things I tried, with and without FETCH_COUNT:
begin \; select 2 \; commit \; select 1 / div from (select div from testtbl order by idx) as src;
select 1/0 \; select 1 / div from (select div from testtbl where div <> 0 order by idx) as src;
begin \; select 2 \; select 1 \; commit;
begin \; select 2 \; select 1;
commit;
I'm not sure how to go about getting a equivalent result of "select pg_sleep(2) \; select 1;" not sleeping. If I put select 1/0 instead of pg_sleep I get an error and any DML seems to end up working just fine (minus actual batch fetching)
update testtbl set val = 2 \; select 1; --result (1)
David J.
The new status of this patch is: Waiting on Author
On 2020-Jul-16, David Johnston wrote:
Instead of a note maybe add a paragraph stating:
"This setting is ignored when multiple statements are sent to the server as a single command (i.e., via the -c command line option or the \; meta-command). Additionally, it is possible for certain combinations of statements sent in that manner to instead return no results, or even be ignored, instead of returning the result set of the last query. In short, when FETCH_COUNT is non-zero, and you send a multi-statement command to the server, the results are undefined. This combination in presently allowed for backward compatibility."
I personally dislike documenting things that don't work, if worded in a
way that don't leave open the possibility of fixing it in the future.
So I didn't like Fabien's original wording either, though I was thinking
that just adding "This may change in a future release of Postgres" might
have been enough. That seems more difficult with your proposed wording,
but maybe you see a good way to do it?
After rereading it a few times, I think it's too specific about how it
fails. Maybe it's possible to reduce to just the last two phrases,
along the lines of
When FETCH_COUNT is non-zero, and you send a multi-statement command
to the server (for example via -c or the \; meta-command), the results
are undefined. This combination in presently allowed for backward
compatibility and may be changed in a future release.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Jul 16, 2020 at 4:24 PM Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:
On 2020-Jul-16, David Johnston wrote:
Instead of a note maybe add a paragraph stating:
"This setting is ignored when multiple statements are sent to the server
as a single command (i.e., via the -c command line option or the \;
meta-command). Additionally, it is possible for certain combinations of
statements sent in that manner to instead return no results, or even be
ignored, instead of returning the result set of the last query. In short,
when FETCH_COUNT is non-zero, and you send a multi-statement command to the
server, the results are undefined. This combination in presently allowed
for backward compatibility."I personally dislike documenting things that don't work, if worded in a
way that don't leave open the possibility of fixing it in the future.
So I didn't like Fabien's original wording either, though I was thinking
that just adding "This may change in a future release of Postgres" might
have been enough. That seems more difficult with your proposed wording,
but maybe you see a good way to do it?After rereading it a few times, I think it's too specific about how it
fails. Maybe it's possible to reduce to just the last two phrases,
along the lines ofWhen FETCH_COUNT is non-zero, and you send a multi-statement command
to the server (for example via -c or the \; meta-command), the results
are undefined. This combination in presently allowed for backward
compatibility and may be changed in a future release.
Everything may change in a future release so I am generally opposed to
including that. But I'm ok with being less descriptive on the observed
failure modes and sweeping it all under "undefined". Need to fix my typo,
"This combination is presently allowed".
When FETCH_COUNT is non-zero, and you send a multi-statement command
to the server (for example via -c or the \; meta-command), the results
are undefined. This combination is presently allowed for backward
compatibility.
David J.
On Thu, Jul 16, 2020 at 04:44:47PM -0700, David G. Johnston wrote:
When FETCH_COUNT is non-zero, and you send a multi-statement command
to the server (for example via -c or the \; meta-command), the results
are undefined. This combination is presently allowed for backward
compatibility.
This has been waiting on author for two months now, so I have marked
the patch as RwF in the CF app.
--
Michael