Thread: psql FETCH_COUNT feature does not work with combined queries

Hello devs,

As pointed out by Kyotaro Horiguchi in

https://www.postgresql.org/message-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.

> 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:

https://www.postgresql.org/message-id/flat/a0a854b6-563c-4a11-bf1c-d6c6f924004d%40manitou-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:
>
>https://www.postgresql.org/message-id/flat/a0a854b6-563c-4a11-bf1c-d6c6f924004d%40manitou-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:
>>
>> https://www.postgresql.org/message-id/flat/a0a854b6-563c-4a11-bf1c-d6c6f924004d%40manitou-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.

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
lineoption or the \; meta-command).  Additionally, it is possible for certain combinations of statements sent in that
mannerto 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.
Thiscombination in presently allowed for backward compatibility."
 

I would suggest, however, adding some tests in src/test/psql.sql demonstrating the broken behavior.  A potentially
usefultest 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
select1/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
commandline option or the \; meta-command).  Additionally, it is possible for certain combinations of statements sent
inthat manner to instead return no results, or even be ignored, instead of returning the result set of the last query.
Inshort, when FETCH_COUNT is non-zero, and you send a multi-statement command to the server, the results are undefined.
Thiscombination 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 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