Alternative to \copy in psql modelled after \g

On Fri, Nov 9, 2018 at 4:19 AM Daniel Verite <daniel@manitou-mail.org> wrote:

Examples of use

1. $ psql -v filename="/path/data-$(date -I).csv"
COPY (SELECT *
FROM table
WHERE ...)
TO STDOUT (FORMAT csv) \copyto :filename

Do I understand correctly that you are proposing a slightly less
verbose alternative of:

\o :filename
COPY TO STDOUT
\o

David J.

David G. Johnston wrote:

Do I understand correctly that you are proposing a slightly less
verbose alternative of:

\o :filename
COPY TO STDOUT
\o

Not strictly the same because of this:

\o or \out [ filename ]
....
“Query results” includes all tables, command responses, and notices
obtained from the database server, as well as output of various
backslash commands that query the database (such as \d); but not
error messages.

If for instance psql received a notification between the two \o it would
end up in the file, which it wouldn't with \copyto.
The same is true for SELECT... \g file as opposed to \o file

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

On Fri, Nov 9, 2018 at 9:35 AM Daniel Verite <daniel@manitou-mail.org> wrote:

If for instance psql received a notification between the two \o it would
end up in the file, which it wouldn't with \copyto.
The same is true for SELECT... \g file as opposed to \o file

Is that something we could change instead of adding a new command?
POLA, for me at least, would be for \g [filename] to do exactly what
you are describing with the \copyto feature. That is doesn't and there
haven't been complaints I would chalk up to notices and command
responses not being prevalent in situations where people would use \g
[filename] or paired \o [filename].

David J.

David G. Johnston wrote:

POLA, for me at least, would be for \g [filename] to do exactly what
you are describing with the \copyto feature.

I admit that if we could improve \g to handle COPY, it would be more
elegant than the current proposal adding two meta-commands.

But the copy-workflow and non-copy-workflow are different, and in
order to know which one to start, \g would need to analyze the query
to determine whether it's a COPY FROM, COPY TO or something else.
psql parses queries syntactically, but not semantically AFAIK, and I
suspect we don't want to start doing that, as it breaks a separation
of concerns.

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

I wrote:

I admit that if we could improve \g to handle COPY, it would be more
elegant than the current proposal adding two meta-commands.

But the copy-workflow and non-copy-workflow are different, and in
order to know which one to start, \g would need to analyze the query

It turns out I was wrong on this. The workflows are different but when
psql receives the first PGresult, it's still time to handle the I/O
redirection. It just differs from \copy in the case of an error:
\copy won't even send a command to the server if the local output
stream can't be opened, whereas COPY \g would send the command, and
will have to deal with the client-side error afterwards.

Well, except that up to now, COPY ... TO STDOUT \g file or \g |command
has been ignoring "file" or "|command", which is arguably a bug as Tom
puts it in another discussion in [1]bug #15535 /messages/by-id/6309.1544031175@sss.pgh.pa.us.

So as a replacement for the \copyto I was proposing earlier, PFA a patch
for COPY TO STDOUT to make use of the argument to \g.

[1]: bug #15535 /messages/by-id/6309.1544031175@sss.pgh.pa.us
/messages/by-id/6309.1544031175@sss.pgh.pa.us

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

"Daniel Verite" <daniel@manitou-mail.org> writes:

So as a replacement for the \copyto I was proposing earlier, PFA a patch
for COPY TO STDOUT to make use of the argument to \g.

Sounds plausible, please add to next commitfest so we don't forget it.

regards, tom lane

Bonjour Daniel,

But the copy-workflow and non-copy-workflow are different, and in
order to know which one to start, \g would need to analyze the query

It turns out I was wrong on this. The workflows are different but when
psql receives the first PGresult, it's still time to handle the I/O
redirection. It just differs from \copy in the case of an error:
\copy won't even send a command to the server if the local output
stream can't be opened, whereas COPY \g would send the command, and
will have to deal with the client-side error afterwards.

Well, except that up to now, COPY ... TO STDOUT \g file or \g |command
has been ignoring "file" or "|command", which is arguably a bug as Tom
puts it in another discussion in [1].

So as a replacement for the \copyto I was proposing earlier, PFA a patch
for COPY TO STDOUT to make use of the argument to \g.

Patch applies cleanly, compiles, make check is ok.

However, it does not contain any tests (bad:-) nor documentation (hmmm...
maybe none needed, though).

Is this just kind of a bug fix? Beforehand the documentation says "\g fn"
sends to file, but that was not happening with COPY, and now it does as it
says?

A question: if opening the output file fails, should not the query be
cancelled and an error be reported? Maybe it is too late for that. It
seems that "SELECT pg_sleep(10) \g /bad/file" fails but executes the query
nevertheless.

ISTM that overriding copystream on open failures is not necessary, because
its value is already NULL in this case.

I'd suggest that is_pipe should be initialized to false, otherwise it is
unclear from the code when it is set before use, and some compilers may
complain.

--
Fabien.

Fabien COELHO wrote:

Is this just kind of a bug fix? Beforehand the documentation says "\g fn"
sends to file, but that was not happening with COPY, and now it does as it
says?

Yes. The doc says about \g:

Sends the current query buffer to the server for execution. If an
argument is given, the query's output is written to the named file
or piped to the given shell command, instead of displaying it as
usual.

It does not add "unless the query is a COPY", so it seems right
to make that just work, and call it a bug fix.

A question: if opening the output file fails, should not the query
be cancelled and an error be reported? Maybe it is too late for
that. It seems that "SELECT pg_sleep(10) \g /bad/file" fails but
executes the query nevertheless.

Yes. This part works as documented:

"The file or command is written to only if the query successfully
returns zero or more tuples, not if the query fails or is a
non-data-returning SQL command."

However, it does not contain any tests (bad:-)

Testing this requires at least some interaction with the OS which I'm
uncomfortable to add. There is a precedent in
regress/sql/hs_standby_allowed.sql doing:

COPY hs1 TO '/tmp/copy_test'
\! cat /tmp/copy_test

We could add something like that in psql.sql, but I'm not fond of it
because it assumes a Unix-ish environment, and that it's OK to clobber
the hardcoded /tmp/copy_test should it preexist. I'd rather work with
a dedicated temporary directory. On Linux mktemp -d could be used, but
I don't know about a portable solution, plus POSIX has deprecated
mktemp.
I'm open to ideas on a portable way for psql.sql to test \g writing to a
file or a program, but ATM my inclination is to not add that test.

nor documentation (hmmm... maybe none needed, though).

\copy has this paragraph:

"The syntax of this command is similar to that of the SQL COPY
command. All options other than the data source/destination are as
specified for COPY. Because of this, special parsing rules apply to
the \copy meta-command. Unlike most other meta-commands, the entire
remainder of the line is always taken to be the arguments of \copy,
and neither variable interpolation nor backquote expansion are
performed in the arguments".

We could add that COPY TO STDOUT with a redirection might be used as an
alternative. The downside is that with four paragraphs plus one tip,
the explanations on \copy give already a lot to chew on, so is it
worth to add more?

ISTM that overriding copystream on open failures is not necessary, because
its value is already NULL in this case.

I'd suggest that is_pipe should be initialized to false, otherwise it is
unclear from the code when it is set before use, and some compilers may
complain.

OK, will consider these in the next revision.

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

Hello,

Is this just kind of a bug fix? Beforehand the documentation says "\g fn"
sends to file, but that was not happening with COPY, and now it does as it
says?

Yes. [...]

It does not add "unless the query is a COPY", so it seems right
to make that just work, and call it a bug fix.

Does this suggest backpatching?

However, it does not contain any tests (bad:-)

Testing this requires at least some interaction with the OS which I'm
uncomfortable to add.

Hmmm. This means that "\g filename" goes fully untested:-( A casual grep
seems to confirm this. Sigh:-(

There is a precedent in regress/sql/hs_standby_allowed.sql doing:

COPY hs1 TO '/tmp/copy_test'
\! cat /tmp/copy_test

Indeed. I'm unsure windows has cat or /tmp, so I do not understand how it
works on such platform. Maybe I'm missing something.

We could add something like that in psql.sql, but I'm not fond of it
because it assumes a Unix-ish environment,

Yep.

I'm open to ideas on a portable way for psql.sql to test \g writing to a
file or a program, but ATM my inclination is to not add that test.

A relative file could be ok? After some testing, the standard regression
tests do not cd to some special place, so it may fail?

However TAP tests do that, and I have used this extensively with pgbench,
so a psql TAP test could do that and other things, such as importing a csv
file or whatever.

nor documentation (hmmm... maybe none needed, though).

\copy has this paragraph:

"The syntax of this command is similar to that of the SQL COPY
command. All options other than the data source/destination are as
specified for COPY. Because of this, special parsing rules apply to
the \copy meta-command. Unlike most other meta-commands, the entire
remainder of the line is always taken to be the arguments of \copy,
and neither variable interpolation nor backquote expansion are
performed in the arguments".

We could add that COPY TO STDOUT with a redirection might be used as an
alternative. The downside is that with four paragraphs plus one tip,
the explanations on \copy give already a lot to chew on, so is it
worth to add more?

I'd say that a small paragraph with the tip would be ok.

--
Fabien.

Fabien COELHO wrote:

It does not add "unless the query is a COPY", so it seems right
to make that just work, and call it a bug fix.

Does this suggest backpatching?

Yes, I think it's a candidate for back-patching.

There is a precedent in regress/sql/hs_standby_allowed.sql doing:

COPY hs1 TO '/tmp/copy_test'
\! cat /tmp/copy_test

Indeed. I'm unsure windows has cat or /tmp, so I do not understand how it
works on such platform. Maybe I'm missing something.

It's exercised only on a standby. Possibly few machines run this test,
among which none powered by Windows? And maybe it even works
on Windows in some cases: the reference to /tmp would work
in an MSYS/MingW environment and "cat" might too if \! gets
to the /bin/sh of that environment.

However TAP tests do that, and I have used this extensively with pgbench,
so a psql TAP test could do that and other things, such as importing a csv
file or whatever.

It looks a significant step forward, to be brought by a patch on its own
without prospect of being back-patched.

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

It looks a significant step forward, to be brought by a patch on its own
without prospect of being back-patched.

Dunno. Even if an additional tap test would not be backpatched, it could
be added on master. I'm basically sadden by pg test coverage, especially
psql which is under 40%, so I try to grasp any improvement opportunity…

See https://coverage.postgresql.org/

--
Fabien.

Fabien COELHO wrote:

Dunno. Even if an additional tap test would not be backpatched, it could
be added on master. I'm basically sadden by pg test coverage, especially
psql which is under 40%, so I try to grasp any improvement opportunity…

See https://coverage.postgresql.org/

Maybe I misunderstand something, as I'm not familiar with TAP tests,
but hasn't psql no such test to begin with, as opposed to the
other programs in src/bin that have a t/ directory?

$ find . -name t
./pg_resetwal/t
./scripts/t
./pg_archivecleanup/t
./pg_verify_checksums/t
./pg_config/t
./pg_controldata/t
./pgbench/t
./pg_rewind/t
./pg_basebackup/t
./pg_dump/t
./initdb/t
./pg_ctl/t

In that case, the first thing we'd need is to add check and installcheck
targets in .../psql/Makefile, and a t/ directory with at least one Perl
script.
If that's the way to go forward, let's just do that in a patch
with a specific entry in the next CF like "Add TAP tests to psql".
Personally I'll be willing to submit and review new tests in t
independently of the patch discussed in $subject.

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

On 2018-Dec-27, Daniel Verite wrote:

Maybe I misunderstand something, as I'm not familiar with TAP tests,
but hasn't psql no such test to begin with, as opposed to the
other programs in src/bin that have a t/ directory?

That's correct. psql does have some tests though, in
src/test/regress/sql/psql.sql and psql_crosstab.sql. It's also tested
indirectly because it's used to run all the src/test/regress files.

If you want to add more tests and increase coverage, that's a good goal,
but keep in mind those other files that can also be used. It doesn't
all have to be TAP.

Some things such as help.c, sql_help.c are hard to test. describe.c
could use more coverage for sure, but lots of it is version-specific,
which makes things harder.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

I took a quick look at this patch. Some thoughts:

* I think the right way to proceed is to commit (and back-patch)
this without any regression test case, and maybe later look into
adding a TAP test that covers "\g file". We have no test cases
for any variant of "\g file", and because of platform variability
and suchlike considerations, adding that seems like a project of
its own --- one I'd not want to back-patch.

* I do agree with documenting this by adding some small note to the
discussion of \copy.

* I think you've made the wrong choice about how this interacts with
the pset.copyStream option. If copyStream is set, that should
continue to take priority, IMO, as anything else would break do_copy's
assumptions about what will happen. One example of why it would be
problematic is that both levels of code would think they control what
to do with the sigpipe trap. Now, it's likely that \copy's syntax
makes it impossible for both copyStream and gfname to be set at the
same time anyway, because \copy extends to the end of the line. But
if it were to happen, we don't want it to be something that do_copy
has to take into account; better therefore just to leave gfname alone.
(Note also the comment just above where you patched, which you failed
to update.)

* You need to be more careful about error cases. I note that
openQueryOutputFile is capable of returning failure with is_pipe
set true, which would lead this patch to disable the sigpipe trap
and never re-enable it.

regards, tom lane

Tom Lane wrote:

I took a quick look at this patch.

PFA an updated patch addressing your comments and Fabien's.

I've also changed handleCopyOut() to return success if it
could pump the data without writing it out locally for lack of
an output stream. It seems to make more sense like that.

While adding the note to the doc I've noticed that the other \copy
tip says:

"This operation is not as efficient as the SQL COPY command because
all data must pass through the client/server connection. For large
amounts of data the SQL command might be preferable.

It doesn't specify that it's for COPY TO/FROM file, not COPY TO
STDOUT/FROM STDIN. Of course the latter would rank the same as \copy
with respect to client/server throughput. Should this tip be more
specific?

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

Bonjour Daniel,

I took a quick look at this patch.

PFA an updated patch addressing your comments and Fabien's.

About this v2.

Applies cleanly, compiles cleanly, "make check" ok.

No tests, but Tom suggests this does not belong here: the committer is
right:-)

I tested the feature manually, and I noticed that with your patch
ROW_COUNT is set more consistently:

sql> COPY pgbench_branches TO STDOUT \g /dev/null # COPY 10
sql> \echo :ROW_COUNT # 10

But previously we had:

sql> \echo :ROW_COUNT # 0

This is an improvement, although I'm not sure where it comes from.

I've also changed handleCopyOut() to return success if it
could pump the data without writing it out locally for lack of
an output stream. It seems to make more sense like that.

I'm hesitating on this one.

On the one hand the SQL query is executed, on the other hand the \g
command partially failed. There is a debatable side effect: If there is an
error, eg:

sql> COPY pgbench_branches TO STDOUT \g /NIET
/NIET: Permission denied
sql> \echo :ERROR :ROW_COUNT # true 0

I understand from the code that the COPY is really executed, so the ERROR
and so ROW_COUNT about the SQL should reflect that. Basically the change
makes the client believe that there is an SQL error whereas the error is
on the client.

Does anyone else have an opinion?

About the code:

I'm unclear whether the project policy accepts "foo" for "foo != NULL",
whether the later is prefered, or whether it does not care about it.

/*
* COPY TO STDOUT \g [|]file may be used as an alternative
* to \copy
*/

The later part of the comment is already stated in the documentation, I'm
not sure it is worth repeating it here. I'd suggest a simpler /* handle
"COPY TO STDOUT \g ..." */

While adding the note to the doc I've noticed that the other \copy
tip says:

"This operation is not as efficient as the SQL COPY command because
all data must pass through the client/server connection. For large
amounts of data the SQL command might be preferable.

It doesn't specify that it's for COPY TO/FROM file, not COPY TO
STDOUT/FROM STDIN. Of course the latter would rank the same as \copy
with respect to client/server throughput. Should this tip be more
specific?

Yep.

This tip also overlooks that the client and server are not necessary on
the same host with the same permissions: it seems to say "prefer COPY to
\copy", while the alternative may work only under special conditions which
are not hinted about in any way.

--
Fabien.

Fabien COELHO wrote:

I've also changed handleCopyOut() to return success if it
could pump the data without writing it out locally for lack of
an output stream. It seems to make more sense like that.

I'm hesitating on this one.

On the one hand the SQL query is executed, on the other hand the \g
command partially failed. There is a debatable side effect: If there is an
error, eg:

The change mentioned above is not supposed to have any user-visible
effect, compared to the previous version. The success of the command
as a whole, that is reflected by :ERROR, is the result of AND'ing the
successes of different steps of the command. Having handleCopyOut()
succeed does not change the falseness of the aggregated result,
which is still false when it fails to open the output stream.

But previously we had:

sql> \echo :ROW_COUNT # 0

Previously "COPY... \g file" behaved as "COPY... \g", the argument being
ignored. In this case, and the patch doesn't change that, the code does:

/*
* Suppress status printing if the report would go to the same
* place as the COPY data just went. Note this doesn't
* prevent error reporting, since handleCopyOut did that.
*/
if (copystream == pset.queryFout)
{
PQclear(copy_result);
copy_result = NULL;
}

One effect of clearing that result is that :ROW_COUNT is going to
be set to zero by SetResultVariables(), which explains the above
output.

:ROW_COUNT is incorrect after COPY TO STDOUT but this is a PG11-only
bug, :ROW_COUNT being a recent addition, so maybe we should deal with
this separately, since the current patch is supposed to address all versions?

I understand from the code that the COPY is really executed, so the ERROR
and so ROW_COUNT about the SQL should reflect that. Basically the change
makes the client believe that there is an SQL error whereas the error is
on the client.

Right, but wether COPY fails because psql can't write the output,
possibly half-way because of a disk full condition, or because the
query was cancelled or the server went down, are these distinctions
meaningful for a script?
If we say yes, how can the user know that the data fetched is
empty or incomplete? We don't have a CLIENT_SIDE_ERROR variable.
Also, the fact that psql retrieves the results when it doesn't have
a valid destination for them is an implementation detail.
I think we could also cancel the query in a way that would be
technically an SQL error, as Ctrl+C would do.
Hiding these details under a generic ERROR=true seems reasonable,
especially since we expose SQLSTATE for fine-grained error checking,
should that be needed.
ERROR=true and SQLSTATE empty is already mentioned as plausible
in SetResultVariables():

SetVariable(pset.vars, "ERROR", "true");

/*
* If there is no SQLSTATE code, use an empty string. This can
happen
* for libpq-detected errors (e.g., lost connection, ENOMEM).
*/
if (code == NULL)
code = "";
SetVariable(pset.vars, "SQLSTATE", code);

The later part of the comment is already stated in the documentation, I'm
not sure it is worth repeating it here. I'd suggest a simpler /* handle
"COPY TO STDOUT \g ..." */

The bug we're fixing here is due to missing the point the comment is
making, so being thick seems fair.

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

Bonjour Daniel,

:ROW_COUNT is incorrect after COPY TO STDOUT but this is a PG11-only
bug, :ROW_COUNT being a recent addition, so maybe we should deal with
this separately, since the current patch is supposed to address all versions?

ISTM that the patch is considered a bug fix, so it shoud be applied to
pg11 and fix it?

I understand from the code that the COPY is really executed, so the ERROR
and so ROW_COUNT about the SQL should reflect that. Basically the change
makes the client believe that there is an SQL error whereas the error is
on the client.

Right, but wether COPY fails because psql can't write the output,
possibly half-way because of a disk full condition, or because the
query was cancelled or the server went down, are these distinctions
meaningful for a script?

It could if the SQL command has side effects, but probably this does not
apply to COPY TO which cannot have.

If we say yes, how can the user know that the data fetched is
empty or incomplete? We don't have a CLIENT_SIDE_ERROR variable.

Yep. Maybe we should.

The documentation states that ERROR is about SQL, not psql internal stuff:

ERROR true if the last SQL query failed, false if it succeeded.
See also SQLSTATE.

And this is somehow the behavior on all other SQL commands, with or
without your patch:

sql> SELECT 1 \g /BAD
/BAD: Permission denied

sql> \echo :ERROR
false

Basically, with your patch, the behavior becomes inconsistent between COPY
and other SQL commands, so there is something to fix.

Given that, I see two options:

(1) document ERROR as being muddy, i.e. there has been some error which
may be SQL or possibly client side. Although SQLSTATE would still allow to
know whether an SQL error occured, there is still no client side
expressions, and even if I had moved pgbench expressions to psql they
would still need to be extended to handle strings. This suggest that maybe
there could be an SQL_ERROR boolean which does store whether SQL succeeded
or not, and possibly a CLIENT_ERROR on the side, and ERROR = SQL_ERROR OR
CLIENT_ERROR.

(2) keep ERROR as is, i.e. about SQL, and add some CLIENT_ERROR, but then
I see another issue, if it is *only* the client error, then it should be
true if there is an SQL error, thus checking if there is any error becomes
ERROR OR CLIENT_ERROR, which is muddy as well especially as there are no
client-side expressions in psql.

Also, the fact that psql retrieves the results when it doesn't have
a valid destination for them is an implementation detail.

Yep, but if there are side effects, a script could want to know if they
occured?

I think we could also cancel the query in a way that would be
technically an SQL error, as Ctrl+C would do.

Hmmm.

Hiding these details under a generic ERROR=true seems reasonable,
especially since we expose SQLSTATE for fine-grained error checking,
should that be needed.

Yep, but no expression.

ERROR=true and SQLSTATE empty is already mentioned as plausible
in SetResultVariables():

Indeed. This suggest that ERROR is already muddy, contrary to the
documentation.

SetVariable(pset.vars, "ERROR", "true");
if (code == NULL)
code = "";
SetVariable(pset.vars, "SQLSTATE", code);

Overall, ISTM that it should point to solution (1).

- document that ERROR is muddy, "some SQL or client error occured"
- add SQL_ERROR, which is always about SQL error and nothing else
- add CLIENT_ERROR, same
- make the behavior consistent for all SQL command, COPY & others

The later part of the comment is already stated in the documentation, I'm
not sure it is worth repeating it here. I'd suggest a simpler /* handle
"COPY TO STDOUT \g ..." */

The bug we're fixing here is due to missing the point the comment is
making, so being thick seems fair.

I would not be, because ISTM that mentionning that COPY TO STDOUT is
specially handled already points clearly to the previous issue. No big
deal.

--
Fabien.

I understand from the code that the COPY is really executed, so the ERROR
and so ROW_COUNT about the SQL should reflect that. Basically the change
makes the client believe that there is an SQL error whereas the error is
on the client.

Right, but wether COPY fails because psql can't write the output,
possibly half-way because of a disk full condition, or because the
query was cancelled or the server went down, are these distinctions
meaningful for a script?

It could if the SQL command has side effects, but probably this does not
apply to COPY TO which cannot have.

Yes it can:

COPY (
UPDATE pgbench_branches
SET bbalance = bbalance + 1
WHERE bid <= 5
RETURNING *) TO STDOUT \g /BAD

The SQL command is executed but the backslash command fails.

--
Fabien.