Thread: [patch] \g with multiple result sets and \watch with copy queries
Hi, The psql improvement in v15 to output multiple result sets does not behave as one might expect with \g: the output file or program to pipe into is opened/closed on each result set, overwriting the previous ones in the case of \g file. Example: psql -At <<EOF -- good (two results output) select 1\; select 2; -- bad: ends up with only "2" in the file select 1\; select 2 \g file EOF That problem with \g is due to PrintQueryTuples() an HandleCopyResult() still having the responsibility to open/close the output stream. I think this code should be moved upper in the call stack, in ExecQueryAndProcessResults(). The first attached patch implements a fix that way. When testing this I've stumbled on another issue nearby: COPY TO STDOUT followed by \watch should normally produce the error message "\watch cannot be used with COPY", but the execution goes into a infinite busy loop instead. This is because ClearOrSaveAllResults() loops over PQgetResult() until it returns NULL, but as it turns out, that never happens: it seems stuck on a PGRES_COPY_OUT result. While looking to fix that, it occurred to me that it would be simpler to allow \watch to deal with COPY results rather than continuing to disallow it. ISTM that before v15, the reason why PSQLexecWatch() did not want to deal with COPY was to not bother with a niche use case, rather than because of some specific impossibility with it. Now that it calls the generic ExecQueryAndProcessResults() code that can handle COPY transfers, \watch on a COPY query seems to work fine if not disallowed. Besides, v15 adds the possibility to feed \watch output into a program through PSQL_WATCH_PAGER, and since the copy format is the best format to be consumed by programs, this seems like a good reason to allow COPY TO STDOUT with it. \watch on a COPY FROM STDIN query doesn't make much sense, but it can be interrupted with ^C if run by mistake, so I don't see a need to disallow it specifically. So the second patch fixes the infinite loop problem like that, on top of the first patch. Best regards, -- Daniel Vérité https://postgresql.verite.pro/ Twitter: @DanielVerite
Attachment
"Daniel Verite" <daniel@manitou-mail.org> writes: > The psql improvement in v15 to output multiple result sets does not > behave as one might expect with \g: the output file or program > to pipe into is opened/closed on each result set, overwriting the > previous ones in the case of \g file. Ugh. I think we'd better fix that before 15.0, else somebody may think this is the new intended behavior and raise compatibility concerns when we fix it. I will see if I can squeeze it in before this afternoon's 15rc2 wrap. regards, tom lane
I wrote: > Ugh. I think we'd better fix that before 15.0, else somebody may > think this is the new intended behavior and raise compatibility > concerns when we fix it. I will see if I can squeeze it in before > this afternoon's 15rc2 wrap. Pushed after making some corrections. Given the time pressure, I did not worry about installing regression test coverage for this stuff, but I wonder if we shouldn't add some. regards, tom lane
Tom Lane wrote: > Pushed after making some corrections. Thanks! > Given the time pressure, I did not worry about installing regression > test coverage for this stuff, but I wonder if we shouldn't add some. Currently, test/regress/sql/psql.sql doesn't AFAICS write anything outside of stdout, but \g, \o, \copy need to write to external files to be tested properly. Looking at nearby tests, I see that commit d1029bb5a26 brings interesting additions in test/regress/sql/misc.sql that could be used as a model to handle output files. psql.sql could write into PG_ABS_BUILDDIR, then read the files back with \copy I guess, then output that again to stdout for comparison. I'll see if I can get that to work. Best regards, -- Daniel Vérité https://postgresql.verite.pro/ Twitter: @DanielVerite
"Daniel Verite" <daniel@manitou-mail.org> writes: > Tom Lane wrote: >> Given the time pressure, I did not worry about installing regression >> test coverage for this stuff, but I wonder if we shouldn't add some. > Currently, test/regress/sql/psql.sql doesn't AFAICS write anything > outside of stdout, but \g, \o, \copy need to write to external > files to be tested properly. Yeah, I don't think we can usefully test these in psql.sql, because file-system side effects are bad in that context. But maybe a TAP test could cope? regards, tom lane
Tom Lane wrote: > > Currently, test/regress/sql/psql.sql doesn't AFAICS write anything > > outside of stdout, but \g, \o, \copy need to write to external > > files to be tested properly. > > Yeah, I don't think we can usefully test these in psql.sql, because > file-system side effects are bad in that context. But maybe a TAP > test could cope? I've came up with the attached using psql.sql only, at least for \g and \o writing to files. This is a bit more complicated than the usual tests, but not that much. Any opinions on this? Best regards, -- Daniel Vérité https://postgresql.verite.pro/ Twitter: @DanielVerite
Attachment
This is a bit more complicated than the usual tests, but not
that much.
Any opinions on this?
+1
I think that because it is more complicated than usual psql, we may want to comment on the intention of the tests and some of the less-than-common psql elements (\set concatenation, resetting \o, etc). If you see value in that I can amend the patch.
Are there any options on COPY (header, formats) that we think we should test as well?
I think that because it is more complicated than usual psql, we may want to comment on the intention of the tests and some of the less-than-common psql elements (\set concatenation, resetting \o, etc). If you see value in that I can amend the patch.
Are there any options on COPY (header, formats) that we think we should test as well?
Bonjour Daniel, Good catch! Thanks for the quick fix! As usual, what is not tested does not:-( Attached a tap test to check for the expected behavior with multiple command \g. -- Fabien.
Attachment
Corey Huinker wrote: > I think that because it is more complicated than usual psql, we may want to > comment on the intention of the tests and some of the less-than-common psql > elements (\set concatenation, resetting \o, etc). If you see value in that > I can amend the patch. If the intentions of some tests appear to be unclear, then yes, sure. I don't feel the need to explain the "how" though. The other comments in these files say why we're testing such or such case, but don't go beyond that. > Are there any options on COPY (header, formats) that we think we should > test as well? There are COPY tests already in src/test/regress/sql/copy*.sql, which hopefully cover the many combination of options. For \g and \o the intention behind the tests is to check that the query output goes where it should in all cases. The options that can't affect where the results go are not really in scope. FTR I started a followup thread on this at [1], to be associated to a new CF entry [2] [1] https://www.postgresql.org/message-id/flat/25c2bb5b-9012-40f8-8088-774cb764046d%40manitou-mail.org [2] https://commitfest.postgresql.org/40/4000/ Best regards, -- Daniel Vérité https://postgresql.verite.pro/ Twitter: @DanielVerite