Re: COPY table FROM STDIN doesn't show count tag

Lists: pgsql-hackers
From: Rajeev rastogi <rajeev(dot)rastogi(at)huawei(dot)com>
To: Amit Khandekar <amit(dot)khandekar(at)enterprisedb(dot)com>
Cc: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: COPY table FROM STDIN doesn't show count tag
Date: 2013-12-12 11:29:31
Message-ID: BF2827DCCE55594C8D7A8F7FFD3AB7713DDB15F8@SZXEML508-MBX.china.huawei.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 12th December 2013, Rajeev Rastogi Wrote:
>On 9th December, Amit Khandelkar wrote:

>>1. slashcopyissuev1.patch :- This patch fixes the \COPY issue.
>>You have removed the if condition in this statement, mentioning that it is always true now:
>>- if (copystream == pset.cur_cmd_source)
>>- pset.lineno++;
>>+ pset.lineno++;
>>
> >But copystream can be different than pset.cur_cmd_source , right ?

>As per the earlier code, condition result was always true. So pset.lineno was always incremented.
>In the earlier code pset.cur_cmd_source was sent as parameter to function and inside the function same parameter was used with the name copystream. So on entry of this function both will be one and same.
>I checked inside the function handleCopyIn, both of these parameters are not changing before above check. Also since pset is specific to single session, so it cannot change concurrently.
>Please let me know, if I am missing something.

>>+ FILE *copyStream; /* Stream to read/write for copy command */
>>
>>There is no tab between FILE and *copystream, hence it is not aligned.

>OK. I shall change accodingly.

I ran pgindent on settings.h file but found no issue reported. Seems tab is not mandatory in between variable declaration.
Even for other parameters also in psqlSetting structure space instead of tab is being used.
So seems no change required for this. Please confirm.

>>2. initialcopyissuev1_ontopofslashcopy.patch : Fix for "COPY table FROM STDIN/STDOUT doesn't show count tag".
>>The following header comments of ProcessResult() need to be modified:
>>* Changes its argument to point to the last PGresult of the command string,
>>* or NULL if that result was for a COPY FROM STDIN or COPY TO STDOUT.

>OK. I shall change accodingly.
I have changed it in the latest patch.

>>Regression results show all passed.
>>Other than this, the patch needs a new regression test.

>I had checked the existing regression test cases and observed that it has already got all kind of test cases. Like
>copy....stdin,
>copy....stdout,
>\copy.....stdin
>\copy.....stdout.

>But since as regression framework runs in "quite i.e. -q" mode, so it does not show any message except query output.
>So our new code change does not impact regression framework.

>Please let me know if you were expecting any other test cases?

Summary of two patches:

1. slashcopyissuev1.patch :- No change in this patch (same as earlier).

2. Initialcopyissuev2_ontopofslashcopy.patch : This patch is modified to change comment as per above review comments.

Please provide your opinion or let me know if any other changes are required.

Thanks and Regards,
Kumar Rajeev Rastogi

Attachment Content-Type Size
slashcopyissuev1.patch application/octet-stream 3.8 KB
initialcopyissuev2_ontopofslashcopy.patch application/octet-stream 6.3 KB

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Rajeev rastogi <rajeev(dot)rastogi(at)huawei(dot)com>
Cc: Amit Khandekar <amit(dot)khandekar(at)enterprisedb(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: COPY table FROM STDIN doesn't show count tag
Date: 2014-03-10 18:14:19
Message-ID: 14537.1394475259@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Rajeev rastogi <rajeev(dot)rastogi(at)huawei(dot)com> writes:
> On 12th December 2013, Rajeev Rastogi Wrote:
>> On 9th December, Amit Khandelkar wrote:
>>> But copystream can be different than pset.cur_cmd_source , right ?

>> As per the earlier code, condition result was always true. So pset.lineno was always incremented.
>> In the earlier code pset.cur_cmd_source was sent as parameter to function and inside the function same parameter was used with the name copystream. So on entry of this function both will be one and same.

The problem with that argument is you're assuming that the previous
behavior was correct :-(. It isn't. If you try a case like this:

$ cat int8data
123 456
123 4567890123456789
4567890123456789 123
4567890123456789 4567890123456789
4567890123456789 -4567890123456789
$ cat testcase.sql
select 1+1;

\copy int8_tbl from 'int8data'

select 1/0;

select 2/0;
$ psql -f testcase.sql regression
?column?
----------
2
(1 row)

psql:testcase.sql:11: ERROR: division by zero
psql:testcase.sql:13: ERROR: division by zero

the script line numbers shown in the error messages are *wrong*,
because handleCopyIn has incorrectly incremented pset.lineno because
it thought it was reading from the current script file. So the
override_file business is wrong, and getting rid of it with a separate
copyStream variable is a good thing.

However, there wasn't much else that I liked about the patch :-(.
It seemed bizarre to me that the copy source/sink selection logic was
partially in ProcessResult and partially in handleCopyOut/handleCopyIn.
Also you'd created a memory leak because ProcessResult now failed to
PQclear the original PGRES_COPY_OUT/IN PGresult. I did a bit of work
to clean that up, and the attached updated patch is the result.

Unfortunately, while testing it I noticed that there's a potentially
fatal backwards-compatibility problem, namely that the "COPY n" status
gets printed on stdout, which is the same place that COPY OUT data is
going. While this isn't such a big problem for interactive use,
usages like this one are pretty popular:

psql -c 'copy mytable to stdout' mydatabase | some-program

With the patch, "COPY n" gets included in the data sent to some-program,
which never happened before and is surely not what the user wants.
The same if the -c string uses \copy.

There are several things we could do about this:

1. Treat this as a non-backwards-compatible change, and document that
people have to use -q if they don't want the COPY tag in the output.
I'm not sure this is acceptable.

2. Kluge ProcessResult so that it continues to not pass back a PGresult
for the COPY TO STDOUT case, or does so only in limited circumstances
(perhaps only if isatty(stdout), for instance).

3. Modify PrintQueryStatus so that command status goes to stderr not
stdout. While this is probably how it should've been done in the first
place, this would be a far more severe compatibility break than #1.
(For one thing, there are probably scripts out there that think that any
output to stderr is an error message.) I'm afraid this one is definitely
not acceptable, though it would be by far the cleanest solution were it
not for compatibility concerns.

4. As #3, but print the command status to stderr only if it's "COPY n",
otherwise to stdout. This is a smaller compatibility break than #3,
but still a break since COPY status was formerly issued to stdout
in non TO STDOUT/FROM STDIN cases. (Note that PrintQueryStatus can't
tell whether it was COPY TO STDOUT rather than any other kind of COPY;
if we want that to factor into the behavior, we need ProcessResult to
do it.)

5. Give up on the print-the-tag aspect of the change, and just fix the
wrong-line-number issue (so we'd still introduce the copyStream variable,
but not change how PGresults are passed around).

I'm inclined to think #2 is the best answer if we can't stomach #1.
But the exact rule for when to print a COPY OUT result probably
still requires some debate. Or maybe someone has another idea?

Also, I'm thinking we should back-patch the aspects of the patch
needed to fix the wrong-line-number issue. That appears to have been
introduced in 9.2; older versions of PG get the above example right.

Comments?

regards, tom lane

Attachment Content-Type Size
psql-copy-count-tag-20140310.patch text/x-diff 11.9 KB

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Rajeev rastogi <rajeev(dot)rastogi(at)huawei(dot)com>
Cc: Amit Khandekar <amit(dot)khandekar(at)enterprisedb(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: COPY table FROM STDIN doesn't show count tag
Date: 2014-03-10 19:50:08
Message-ID: 20099.1394481008@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

I wrote:
> Also, I'm thinking we should back-patch the aspects of the patch
> needed to fix the wrong-line-number issue. That appears to have been
> introduced in 9.2; older versions of PG get the above example right.

I've done that. For reference' sake, here's an updated patch against
HEAD with just the uncommitted changes.

regards, tom lane

Attachment Content-Type Size
psql-copy-count-tag-20140310-2.patch text/x-diff 8.6 KB

From: Rajeev rastogi <rajeev(dot)rastogi(at)huawei(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Amit Khandekar <amit(dot)khandekar(at)enterprisedb(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: COPY table FROM STDIN doesn't show count tag
Date: 2014-03-11 10:28:10
Message-ID: BF2827DCCE55594C8D7A8F7FFD3AB7713DDD6979@SZXEML508-MBX.china.huawei.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 10 March 2014 23:44, Tom Lane wrote:

> Unfortunately, while testing it I noticed that there's a potentially
> fatal backwards-compatibility problem, namely that the "COPY n" status
> gets printed on stdout, which is the same place that COPY OUT data is
> going. While this isn't such a big problem for interactive use, usages
> like this one are pretty popular:
>
> psql -c 'copy mytable to stdout' mydatabase | some-program
>
> With the patch, "COPY n" gets included in the data sent to some-program,
> which never happened before and is surely not what the user wants.
> The same if the -c string uses \copy.
>
> There are several things we could do about this:
>
> 1. Treat this as a non-backwards-compatible change, and document that
> people have to use -q if they don't want the COPY tag in the output.
> I'm not sure this is acceptable.
>
> 2. Kluge ProcessResult so that it continues to not pass back a PGresult
> for the COPY TO STDOUT case, or does so only in limited circumstances
> (perhaps only if isatty(stdout), for instance).

> I'm inclined to think #2 is the best answer if we can't stomach #1.

Is it OK to have different status output for different flavor of COPY command?
I am afraid that It will become kind of inconsistent result.

Also not providing the command result status may be inconsistent from
behavior of any other SQL commands.

I agree that it breaks the backward compatibility but I am not sure if anyone
is so tightly coupled with this ( or whether they will be effected with additional status result).

To me option #1 seems to be more suitable specially since there is an option to disable
the status output by giving -q.

Please provide your opinion or let me know If I have missed something.

Thanks and Regards,
Kumar Rajeev Rastogi


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Rajeev rastogi <rajeev(dot)rastogi(at)huawei(dot)com>
Cc: Amit Khandekar <amit(dot)khandekar(at)enterprisedb(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: COPY table FROM STDIN doesn't show count tag
Date: 2014-03-11 14:21:36
Message-ID: 12040.1394547696@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Rajeev rastogi <rajeev(dot)rastogi(at)huawei(dot)com> writes:
> On 10 March 2014 23:44, Tom Lane wrote:
>> Unfortunately, while testing it I noticed that there's a potentially
>> fatal backwards-compatibility problem, namely that the "COPY n" status
>> gets printed on stdout, which is the same place that COPY OUT data is
>> going. While this isn't such a big problem for interactive use, usages
>> like this one are pretty popular:
>>
>> psql -c 'copy mytable to stdout' mydatabase | some-program
>>
>> With the patch, "COPY n" gets included in the data sent to some-program,
>> which never happened before and is surely not what the user wants.
>> The same if the -c string uses \copy.

> Is it OK to have different status output for different flavor of COPY command?
> I am afraid that It will become kind of inconsistent result.

Well, that's the big question here.

We already do have different status output for different kinds of COPY,
ie we don't report it for COPY FROM STDIN/TO STDOUT. What now emerges
is that there's a good reason for the omission in the case of TO STDOUT.
I certainly hadn't remembered that, and there's no documentation of it
in either code comments or the SGML docs.

After sleeping on it, I'm inclined to think we should continue to not
print status for COPY TO STDOUT. Aside from the risk of breaking scripts,
there's a decent analogy to be made to SELECT: we don't print a status
tag for that either.

That leaves the question of whether we want to start printing a tag for
the COPY FROM STDIN case. I don't think that'd create much risk of
breaking anything, and the analogy to SELECT doesn't hold either.
OTOH, Robert opined upthread that FROM STDIN and TO STDOUT shouldn't
behave differently; does that argument still impress anyone? And given
that different COPY cases are still going to behave differently, maybe
we should just stick with the status quo. It's been like this for a
mighty long time with few complaints.

In any case, some documentation and code comment changes would be
appropriate ...

regards, tom lane


From: Rajeev rastogi <rajeev(dot)rastogi(at)huawei(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Amit Khandekar <amit(dot)khandekar(at)enterprisedb(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: COPY table FROM STDIN doesn't show count tag
Date: 2014-03-12 03:44:09
Message-ID: BF2827DCCE55594C8D7A8F7FFD3AB7713DDD7D45@SZXEML508-MBX.china.huawei.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 11 March 2014 19:52, Tom Lane wrote:

> After sleeping on it, I'm inclined to think we should continue to not
> print status for COPY TO STDOUT. Aside from the risk of breaking
> scripts, there's a decent analogy to be made to SELECT: we don't print
> a status tag for that either.

It is correct that SELECT does not print conventional way of status tag but still it prints the number
of rows selected (e.g. (2 rows)) along with rows actual value, which can be very well considered
as kind of status. User can make out with this result, that how many rows have been selected.

But in-case of COPY TO STDOUT, if we don't print anything, then user does not have any direct way of finding
that how many rows were copied from table to STDOUT, which might have been very useful.

Please let me know your opinion or if I have missed something.

Thanks and Regards,
Kumar Rajeev Rastogi


From: David Johnston <polobo(at)yahoo(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Subject: Re: COPY table FROM STDIN doesn't show count tag
Date: 2014-03-12 06:10:59
Message-ID: 1394604659850-5795611.post@n5.nabble.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Tom Lane-2 wrote
> Unfortunately, while testing it I noticed that there's a potentially
> fatal backwards-compatibility problem, namely that the "COPY n" status
> gets printed on stdout, which is the same place that COPY OUT data is
> going. While this isn't such a big problem for interactive use,
> usages like this one are pretty popular:
>
> psql -c 'copy mytable to stdout' mydatabase | some-program
>
> With the patch, "COPY n" gets included in the data sent to some-program,
> which never happened before and is surely not what the user wants.
> The same if the -c string uses \copy.
>
> There are several things we could do about this:
>
> 1. Treat this as a non-backwards-compatible change, and document that
> people have to use -q if they don't want the COPY tag in the output.
> I'm not sure this is acceptable.

I've mostly used copy to with files and so wouldn't mind if STDOUT had the
COPY n sent to it as long as the target file is just the copy contents.

> 2. Kluge ProcessResult so that it continues to not pass back a PGresult
> for the COPY TO STDOUT case, or does so only in limited circumstances
> (perhaps only if isatty(stdout), for instance).

The main problem with this is that people will test by sending output to a
TTY and see the COPY n. Although if it can be done consistently then you
minimize backward incompatibility and encourage people to enforce quiet mode
while the command runs...

> 3. Modify PrintQueryStatus so that command status goes to stderr not
> stdout. While this is probably how it should've been done in the first
> place, this would be a far more severe compatibility break than #1.
> (For one thing, there are probably scripts out there that think that any
> output to stderr is an error message.) I'm afraid this one is definitely
> not acceptable, though it would be by far the cleanest solution were it
> not for compatibility concerns.

Yes, it's a moot point but I'm not sure it would be best anyway.

> 4. As #3, but print the command status to stderr only if it's "COPY n",
> otherwise to stdout. This is a smaller compatibility break than #3,
> but still a break since COPY status was formerly issued to stdout
> in non TO STDOUT/FROM STDIN cases. (Note that PrintQueryStatus can't
> tell whether it was COPY TO STDOUT rather than any other kind of COPY;
> if we want that to factor into the behavior, we need ProcessResult to
> do it.)

Since we are considering stderr my (inexperienced admittedly) gut says that
using stderr for this is generally undesirable and especially given our
existing precedence. stdout is the seemingly correct target, typically, and
the existing quiet-mode toggle provides sufficient control for typical
needs.

> 5. Give up on the print-the-tag aspect of the change, and just fix the
> wrong-line-number issue (so we'd still introduce the copyStream variable,
> but not change how PGresults are passed around).
>
> I'm inclined to think #2 is the best answer if we can't stomach #1.
> But the exact rule for when to print a COPY OUT result probably
> still requires some debate. Or maybe someone has another idea?
>
> Also, I'm thinking we should back-patch the aspects of the patch
> needed to fix the wrong-line-number issue. That appears to have been
> introduced in 9.2; older versions of PG get the above example right.
>
> Comments?

I'd like COPY TO to anything but STDOUT to emit a "COPY n" on STDOUT -
unless suppressed by -q(uiet)

Document that COPY TO STDOUT does not emit "COPY n" because STDOUT is
already assigned for data and so is not available for notifications. Since
COPY is more typically used for ETL than a bare-select, in addition to
back-compatibility concerns, this default behavior seems reasonable.

Would it be possible to store the "n" somewhere and provide a command - like
GET DIAGNOSTICS in pl/pgsql - if the user really wants to know how many rows
were sent to STDOUT? I'm doubt this is even useful in the typical use-case
for COPY TO STDOUT but figured I'd toss the idea out there.

Is there anything besides a desire for consistency that anyone has or can
put forth as a use-case for COPY TO STDOUT emitting "COPY n" on STDOUT as
well? If you are going to view the content inline, and also want a quick
count, ISTM you would be more likely to use SELECT to take advantage of all
its pretty-print features.

If we really need to cater to this use then maybe a "--loud-copy-to-stdout"
switch can be provided to override its default quiet-mode.

David J.

--
View this message in context: http://postgresql.1045698.n5.nabble.com/COPY-table-FROM-STDIN-doesn-t-show-count-tag-tp5775018p5795611.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.


From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
To: David Johnston <polobo(at)yahoo(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: COPY table FROM STDIN doesn't show count tag
Date: 2014-03-12 06:23:17
Message-ID: CAFj8pRBSfwn8f_KhWeQMXwr_PhHAm2vY0oRxcwHNnhWVD6s3-Q@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

2014-03-12 7:10 GMT+01:00 David Johnston <polobo(at)yahoo(dot)com>:

> Tom Lane-2 wrote
> > Unfortunately, while testing it I noticed that there's a potentially
> > fatal backwards-compatibility problem, namely that the "COPY n" status
> > gets printed on stdout, which is the same place that COPY OUT data is
> > going. While this isn't such a big problem for interactive use,
> > usages like this one are pretty popular:
> >
> > psql -c 'copy mytable to stdout' mydatabase | some-program
> >
> > With the patch, "COPY n" gets included in the data sent to some-program,
> > which never happened before and is surely not what the user wants.
> > The same if the -c string uses \copy.
> >
> > There are several things we could do about this:
> >
> > 1. Treat this as a non-backwards-compatible change, and document that
> > people have to use -q if they don't want the COPY tag in the output.
> > I'm not sure this is acceptable.
>
> I've mostly used copy to with files and so wouldn't mind if STDOUT had the
> COPY n sent to it as long as the target file is just the copy contents.
>
>
> > 2. Kluge ProcessResult so that it continues to not pass back a PGresult
> > for the COPY TO STDOUT case, or does so only in limited circumstances
> > (perhaps only if isatty(stdout), for instance).
>
> The main problem with this is that people will test by sending output to a
> TTY and see the COPY n. Although if it can be done consistently then you
> minimize backward incompatibility and encourage people to enforce quiet
> mode
> while the command runs...
>
>
> > 3. Modify PrintQueryStatus so that command status goes to stderr not
> > stdout. While this is probably how it should've been done in the first
> > place, this would be a far more severe compatibility break than #1.
> > (For one thing, there are probably scripts out there that think that any
> > output to stderr is an error message.) I'm afraid this one is definitely
> > not acceptable, though it would be by far the cleanest solution were it
> > not for compatibility concerns.
>
> Yes, it's a moot point but I'm not sure it would be best anyway.
>
>
> > 4. As #3, but print the command status to stderr only if it's "COPY n",
> > otherwise to stdout. This is a smaller compatibility break than #3,
> > but still a break since COPY status was formerly issued to stdout
> > in non TO STDOUT/FROM STDIN cases. (Note that PrintQueryStatus can't
> > tell whether it was COPY TO STDOUT rather than any other kind of COPY;
> > if we want that to factor into the behavior, we need ProcessResult to
> > do it.)
>
> Since we are considering stderr my (inexperienced admittedly) gut says that
> using stderr for this is generally undesirable and especially given our
> existing precedence. stdout is the seemingly correct target, typically,
> and
> the existing quiet-mode toggle provides sufficient control for typical
> needs.
>
>
> > 5. Give up on the print-the-tag aspect of the change, and just fix the
> > wrong-line-number issue (so we'd still introduce the copyStream variable,
> > but not change how PGresults are passed around).
> >
> > I'm inclined to think #2 is the best answer if we can't stomach #1.
> > But the exact rule for when to print a COPY OUT result probably
> > still requires some debate. Or maybe someone has another idea?
> >
> > Also, I'm thinking we should back-patch the aspects of the patch
> > needed to fix the wrong-line-number issue. That appears to have been
> > introduced in 9.2; older versions of PG get the above example right.
> >
> > Comments?
>
> I'd like COPY TO to anything but STDOUT to emit a "COPY n" on STDOUT -
> unless suppressed by -q(uiet)
>

+1

This information can be really interesting and sometimes important, when
people has no idea, how they tables are long

Regards

Pavel

>
> Document that COPY TO STDOUT does not emit "COPY n" because STDOUT is
> already assigned for data and so is not available for notifications. Since
> COPY is more typically used for ETL than a bare-select, in addition to
> back-compatibility concerns, this default behavior seems reasonable.
>
> Would it be possible to store the "n" somewhere and provide a command -
> like
> GET DIAGNOSTICS in pl/pgsql - if the user really wants to know how many
> rows
> were sent to STDOUT? I'm doubt this is even useful in the typical use-case
> for COPY TO STDOUT but figured I'd toss the idea out there.
>
> Is there anything besides a desire for consistency that anyone has or can
> put forth as a use-case for COPY TO STDOUT emitting "COPY n" on STDOUT as
> well? If you are going to view the content inline, and also want a quick
> count, ISTM you would be more likely to use SELECT to take advantage of all
> its pretty-print features.
>
> If we really need to cater to this use then maybe a "--loud-copy-to-stdout"
> switch can be provided to override its default quiet-mode.
>
> David J.
>
>
>
>
>
> --
> View this message in context:
> http://postgresql.1045698.n5.nabble.com/COPY-table-FROM-STDIN-doesn-t-show-count-tag-tp5775018p5795611.html
> Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Rajeev rastogi <rajeev(dot)rastogi(at)huawei(dot)com>
Cc: Amit Khandekar <amit(dot)khandekar(at)enterprisedb(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: COPY table FROM STDIN doesn't show count tag
Date: 2014-03-12 15:57:04
Message-ID: 13673.1394639824@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Rajeev rastogi <rajeev(dot)rastogi(at)huawei(dot)com> writes:
> On 11 March 2014 19:52, Tom Lane wrote:
>> After sleeping on it, I'm inclined to think we should continue to not
>> print status for COPY TO STDOUT. Aside from the risk of breaking
>> scripts, there's a decent analogy to be made to SELECT: we don't print
>> a status tag for that either.

> It is correct that SELECT does not print conventional way of status tag but still it prints the number
> of rows selected (e.g. (2 rows)) along with rows actual value, which can be very well considered
> as kind of status. User can make out with this result, that how many rows have been selected.

> But in-case of COPY TO STDOUT, if we don't print anything, then user does not have any direct way of finding
> that how many rows were copied from table to STDOUT, which might have been very useful.

Uh, you mean other than the data rows that were just printed? I fail
to see how this is much different from the SELECT case:

regression=# \copy int8_tbl to stdout
123 456
123 4567890123456789
4567890123456789 123
4567890123456789 4567890123456789
4567890123456789 -4567890123456789
regression=#

(Note that I'm defining TO STDOUT from psql's perspective, ie the rows are
going to the queryFout file, which is the same place the COPY status would
get printed to.)

regards, tom lane


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: David Johnston <polobo(at)yahoo(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: COPY table FROM STDIN doesn't show count tag
Date: 2014-03-12 16:09:20
Message-ID: 13998.1394640560@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

David Johnston <polobo(at)yahoo(dot)com> writes:
> Tom Lane-2 wrote
>> 1. Treat this as a non-backwards-compatible change, and document that
>> people have to use -q if they don't want the COPY tag in the output.
>> I'm not sure this is acceptable.

> I've mostly used copy to with files and so wouldn't mind if STDOUT had the
> COPY n sent to it as long as the target file is just the copy contents.

I think you're missing the point: the case I'm concerned about is exactly
that the target file is psql's stdout, or more specifically the same place
that the COPY status would get printed to.

>> 2. Kluge ProcessResult so that it continues to not pass back a PGresult
>> for the COPY TO STDOUT case, or does so only in limited circumstances
>> (perhaps only if isatty(stdout), for instance).

> The main problem with this is that people will test by sending output to a
> TTY and see the COPY n. Although if it can be done consistently then you
> minimize backward incompatibility and encourage people to enforce quiet mode
> while the command runs...

Yeah, the inconsistency of behavior that this solution would cause is not
a good thing. My inclination now (see later traffic) is to suppress the
status report when the COPY destination is the same as pset.queryFout
(ie, a simple test whether the FILE pointers are equal). This would
suppress the status report for "\copy to stdout" and "COPY TO STDOUT"
cases, and also for "\copy to pstdout" if you'd not redirected queryFout
with \o.

regards, tom lane


From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: David Johnston <polobo(at)yahoo(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: COPY table FROM STDIN doesn't show count tag
Date: 2014-03-12 17:44:29
Message-ID: CA+TgmoaYAYHXuHVrcnv4GMBMUFNoHxWuNWMuzaGCxfjU0iYC4A@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, Mar 12, 2014 at 12:09 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> David Johnston <polobo(at)yahoo(dot)com> writes:
>> Tom Lane-2 wrote
>>> 1. Treat this as a non-backwards-compatible change, and document that
>>> people have to use -q if they don't want the COPY tag in the output.
>>> I'm not sure this is acceptable.
>
>> I've mostly used copy to with files and so wouldn't mind if STDOUT had the
>> COPY n sent to it as long as the target file is just the copy contents.
>
> I think you're missing the point: the case I'm concerned about is exactly
> that the target file is psql's stdout, or more specifically the same place
> that the COPY status would get printed to.
>
>>> 2. Kluge ProcessResult so that it continues to not pass back a PGresult
>>> for the COPY TO STDOUT case, or does so only in limited circumstances
>>> (perhaps only if isatty(stdout), for instance).
>
>> The main problem with this is that people will test by sending output to a
>> TTY and see the COPY n. Although if it can be done consistently then you
>> minimize backward incompatibility and encourage people to enforce quiet mode
>> while the command runs...
>
> Yeah, the inconsistency of behavior that this solution would cause is not
> a good thing. My inclination now (see later traffic) is to suppress the
> status report when the COPY destination is the same as pset.queryFout
> (ie, a simple test whether the FILE pointers are equal). This would
> suppress the status report for "\copy to stdout" and "COPY TO STDOUT"
> cases, and also for "\copy to pstdout" if you'd not redirected queryFout
> with \o.

This is reasonably similar to what we already do for SELECT, isn't it?
I mean, the server always sends back a command tag, but psql
sometimes opts not to print it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: David Johnston <polobo(at)yahoo(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: COPY table FROM STDIN doesn't show count tag
Date: 2014-03-12 18:26:39
Message-ID: 17600.1394648799@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Mar 12, 2014 at 12:09 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> My inclination now (see later traffic) is to suppress the
>> status report when the COPY destination is the same as pset.queryFout
>> (ie, a simple test whether the FILE pointers are equal). This would
>> suppress the status report for "\copy to stdout" and "COPY TO STDOUT"
>> cases, and also for "\copy to pstdout" if you'd not redirected queryFout
>> with \o.

> This is reasonably similar to what we already do for SELECT, isn't it?
> I mean, the server always sends back a command tag, but psql
> sometimes opts not to print it.

Right, the analogy to SELECT gives some comfort that this is reasonable.

regards, tom lane


From: Rajeev rastogi <rajeev(dot)rastogi(at)huawei(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: David Johnston <polobo(at)yahoo(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: COPY table FROM STDIN doesn't show count tag
Date: 2014-03-13 11:40:19
Message-ID: BF2827DCCE55594C8D7A8F7FFD3AB7713DDD8520@SZXEML508-MBX.china.huawei.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 12 March 2014 23:57, Tom Lane Wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Wed, Mar 12, 2014 at 12:09 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> My inclination now (see later traffic) is to suppress the status
> >> report when the COPY destination is the same as pset.queryFout (ie,
> a
> >> simple test whether the FILE pointers are equal). This would
> >> suppress the status report for "\copy to stdout" and "COPY TO
> STDOUT"
> >> cases, and also for "\copy to pstdout" if you'd not redirected
> >> queryFout with \o.

Based on my analysis, I observed that just file pointer comparison may not be sufficient
to decide whether to display command tag or not. E.g. imagine below scenario:

psql.exe -d postgres -o 'file.dat' -c " \copy tbl to 'file.dat';"

Though both destination files are same but file pointer will be different and hence
printing status in file 'file.dat' will overwrite some part of data copied to file.
Also we don't have any direct way of comparison of file name itself.
As I see \copy ... TO.. will print status only in-case of "\copy to pstdout" if -o option is given.

So instead of having so much of confusion and inconsistency that too for one very specific case,
I though not to print status for all case Of STDOUT and \COPY ... TO ...

> > This is reasonably similar to what we already do for SELECT, isn't it?
> > I mean, the server always sends back a command tag, but psql
> > sometimes opts not to print it.
>
> Right, the analogy to SELECT gives some comfort that this is reasonable.

I have modified the patch based on above analysis as:
1. In-case of COPY ... TO STDOUT, command tag will not be displayed.
2. In-case of \COPY ... TO ..., command tag will not be displayed.
3. In all other cases, command tag will be displayed similar as were getting displayed earlier.

I have modified the corresponding documentation.

Please find the attached revised patch.

Thanks and Regards,
Kumar Rajeev Rastogi

Attachment Content-Type Size
psql-copy-count-tag-20140313.patch application/octet-stream 10.1 KB

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Rajeev rastogi <rajeev(dot)rastogi(at)huawei(dot)com>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, David Johnston <polobo(at)yahoo(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: COPY table FROM STDIN doesn't show count tag
Date: 2014-03-13 17:55:07
Message-ID: 5007.1394733307@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Rajeev rastogi <rajeev(dot)rastogi(at)huawei(dot)com> writes:
> [ updated patch ]

I've committed this patch with additional revisions.

> Based on my analysis, I observed that just file pointer comparison may not be sufficient
> to decide whether to display command tag or not. E.g. imagine below scenario:

> psql.exe -d postgres -o 'file.dat' -c " \copy tbl to 'file.dat';"

I don't think it's our responsibility to avoid printing both data and
status to the same place in such cases; arguably, in fact, that's exactly
what the user told us to do. The important thing is to avoid printing
both for the straightforward case of COPY TO STDOUT. For that, file
pointer comparison is the right thing, since the option-parsing code will
set copysource to match queryFout in exactly the relevant cases.

In any case, this revised patch suppressed the status print in *all*
COPY_OUT cases, which surely seems like throwing the baby out with the
bathwater.

regards, tom lane