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

[Tom Lane]

Rajeev rastogi  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Rajeev rastogi]

On 12 March 2014 23:57, Tom Lane Wrote: 
> Robert Haas  writes:
> > On Wed, Mar 12, 2014 at 12:09 PM, Tom Lane  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




psql-copy-count-tag-20140313.patch
Description: psql-copy-count-tag-20140313.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Tom Lane]

Robert Haas  writes:
> On Wed, Mar 12, 2014 at 12:09 PM, Tom Lane  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Robert Haas]

On Wed, Mar 12, 2014 at 12:09 PM, Tom Lane  wrote:
> David Johnston  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Tom Lane]

David Johnston  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Tom Lane]

Rajeev rastogi  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
4567890123456789123
45678901234567894567890123456789
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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Pavel Stehule]

2014-03-12 7:10 GMT+01:00 David Johnston :

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

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

[David Johnston]

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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Rajeev rastogi]

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





-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Tom Lane]

Rajeev rastogi  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Rajeev rastogi]

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




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Tom Lane]

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

diff -c new/common.c new-wholepatch/common.c
*** new/common.c	Mon Mar 10 14:55:49 2014
--- new-wholepatch/common.c	Mon Mar 10 12:49:02 2014
***
*** 631,638 
   * When the command string contained no such COPY command, this function
   * degenerates to an AcceptResult() call.
   *
!  * 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.
   *
   * Returns true on complete success, false otherwise.  Possible failure modes
   * include purely client-side problems; check the transaction status for the
--- 631,637 
   * When the command string contained no such COPY command, this function
   * degenerates to an AcceptResult() call.
   *
!  * Changes its argument to point to the last PGresult of the command string.
   *
   * Returns true on complete success, false otherwise.  Possible failure modes
   * include purely client-side problems; check the transaction status for the
***
*** 641,654 
  static bool
  ProcessResult(PGresult **results)
  {
- 	PGresult   *next_result;
  	bool		success = true;
  	bool		first_cycle = true;
  
! 	do
  	{
  		ExecStatusType result_status;
  		bool		is_copy;
  
  		if (!AcceptResult(*results))
  		{
--- 640,653 
  static bool
  ProcessResult(PGresult **results)
  {
  	bool		success = true;
  	bool		first_cycle = true;
  
! 	for (;;)
  	{
  		ExecStatusType result_status;
  		bool		is_copy;
+ 		PGresult   *next_result;
  
  		if (!AcceptResult(*results))
  		{
***
*** 693,698 
--- 692,698 
  			 * otherwise use queryFout or cur_cmd_source as appropriate.
  			 */
  			FILE	   *copystream = pset.copyStream;
+ 			PGresult   *copy_result;
  
  			SetCancelConn();
  			if (result_status == PGRES_COPY_OUT)
***
*** 700,706 
  if (!copystream)
  	copystream = pset.queryFout;
  success = handleCopyOut(pset.db,
! 		copystream) && success;
  			}
  			else
  			{
--- 700,707 
  if (!copystream)
  	copystream = pset.queryFout;
  success = handleCopyOut(pset.db,
! 		copystream,
! 		©_result) && success;
  			}
  			else
  			{
***
*** 708,737 
  	copystream = pset.cur_cmd_source;
  success = handleCopyIn(pset.db,
  	   copystream,
! 	   PQbinaryTuples(*results)) && success;
  			}
  			ResetCancelConn();
  
! 			/*
! 			 * Call PQgetResult() once more.  In the typical case of a
! 			 * single-command string, it will return NULL.	Otherwise, we'll
! 			 * have other results to process that may include other COPYs.
! 			 */
  			PQclear(*results);
! 			*results = next_result = PQgetResult(pset.db);
  		}
  		else if (first_cycle)
  			/* fast path: no COPY commands; PQexec visited all results */
  			break;
- 		else if ((next_result = PQgetResult(pset.db)))
- 		{
- 			/* non-COPY command(s) after a COPY: keep the last one */
- 			PQclear(*results);
- 			*results = next_result;
  		}
  
  		first_cycle = false;
! 	} while (next_result);
  
  	/* may need this to recover from conn loss during COPY */
  	if (!first_cycle && !CheckConnection())
--- 709,742 
  	copystream = pset.cur_cmd_source;
  success = handleCopyIn(pset.db,
  	   copystream,
! 	   PQbinaryTuples(*results),
! 	   ©_result) && success;
  			}
  			ResetCancelConn();
  
! 			/* replace the COPY_OUT/IN result with COPY command exit status */
  			PQclear(*results);
! 			*results = copy_result;
  		}
  		else if (first_cycle)
+ 		{
  			/* fast path: no COPY commands; PQexec visited all results */
  			break;
  		}
  
+ 		/*
+ 		 * Check PQgetResult() again.  In the typical case of a single-command
+ 		 * string, it will return NULL.  Otherwise, we'll have other results
+ 		 * to process that may include other COPYs.  We keep the last result.
+ 		 */
+ 		next_result = PQgetResult(pset.db);
+ 		if (!next_result)
+ 			break;
+ 
+ 		PQclear(*results);
+ 		*results = next_result;
  		first_cycle = false;
! 	}
  
  	/* may need this to recover from conn loss during COPY */
  	if (!first_cycle && !CheckConnection())
diff -c new/copy.c new-wholepatch/copy.c
*** new/copy.c	Mon Mar 10 14:56:21 2014
--- new-wholepatch/copy.c	Mon Mar 10 12:50:27 2014
***
*** 429,444 
   * conn should be a database connection that you just issued COPY TO on
   * and got back a PGRES_COPY_OUT result.
   * copystream is the file stream for the data to go to.
   *
   * result is true if successful, false if not.
   */
  bool
! handleCopyOut(PGconn *conn, FILE *copystream)
  {
  	bool		OK = 

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

[Tom Lane]

Rajeev rastogi  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
4567890123456789123
45678901234567894567890123456789
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

diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 3a820fa..136eed1 100644
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
*** StoreQueryTuple(const PGresult *result)
*** 628,638 
   * command.  In that event, we'll marshal data for the COPY and then cycle
   * through any subsequent PGresult objects.
   *
!  * When the command string contained no affected COPY command, this function
   * degenerates to an AcceptResult() call.
   *
!  * 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.
   *
   * Returns true on complete success, false otherwise.  Possible 

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

[Rajeev rastogi]

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
>copystdin,
>copystdout,
>\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




slashcopyissuev1.patch
Description: slashcopyissuev1.patch


initialcopyissuev2_ontopofslashcopy.patch
Description: initialcopyissuev2_ontopofslashcopy.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Rajeev rastogi]

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.


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

>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
copystdin,
copystdout,
\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?

>I don't think we need to do any doc changes, because the doc already mentions 
>that COPY should show the COUNT tag, and does not mention anything specific to 
>client-side COPY.
 OK.

Please provide you opinion, based on which I shall prepare new patch and share 
the same.

Thanks and Regards,
Kumar Rajeev Rastogi

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

[Amit Khandekar]

On 29 November 2013 19:20, Rajeev rastogi  wrote:

>  On 26 November 2013, Amit Khandelkar wrote:
>
> >Can you please submit the \COPY patch as a separate patch ? Since these
> are two different issues, I would like to have these two fixed and
> committed separately. You can always test the \COPY issue using \COPY TO
> followed by INSERT.
>
>
>
> Please find the attached two separate patches:
>

Thanks.


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


+   FILE   *copyStream; /* Stream to read/write for copy
command */

There is no tab between FILE and *copystream, hence it is not aligned.


 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.


Regression results show all passed.

Other than this, the patch needs a new regression test.

I don't think we need to do any doc changes, because the doc already
mentions that COPY should show the COUNT tag, and does not mention anything
specific to client-side COPY.


>
>
> Thanks and Regards,
>
> Kumar Rajeev Rastogi
>
>
>
>
>

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

[Rajeev rastogi]

On 26 November 2013, Amit Khandelkar wrote:
>Can you please submit the \COPY patch as a separate patch ? Since these are 
>two different issues, I would like to have these two fixed and committed 
>separately. You can always test the \COPY issue using \COPY TO followed by 
>INSERT.

Please find the attached two separate patches:


1.  slashcopyissuev1.patch :- This patch fixes the \COPY issue.

2.  initialcopyissuev1_ontopofslashcopy.patch : Fix for "COPY table FROM 
STDIN/STDOUT doesn't show count tag".

Thanks and Regards,
Kumar Rajeev Rastogi




slashcopyissuev1.patch
Description: slashcopyissuev1.patch


initialcopyissuev1_ontopofslashcopy.patch
Description: initialcopyissuev1_ontopofslashcopy.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Amit Khandekar]

On 27 November 2013 09:59, Rajeev rastogi  wrote:

>  On 26 November 2013, Amit Khandelkar wrote:
>
>
> On 26 November 2013 18:59, Amit Khandekar  > wrote:
>
>>
>>
>>
>> On 25 November 2013 15:25, Rajeev rastogi 
>>  wrote:
>>
>>> OK. I have revised the patch as per the discussion.
>>>
>> Could you please submit only the \COPY fix first ? The attached patch
>> also contains the fix for the original COPY status fix.
>>
> Can you please submit the \COPY patch as a separate patch ? Since these
are two different issues, I would like to have these two fixed and
committed separately. You can always test the \COPY issue using \COPY TO
followed by INSERT.

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

[Rajeev rastogi]

On 26 November 2013, Amit Khandelkar wrote:
>>Now if \copy command is called then, we are setting the appropriate value of 
>>_psqlSettings->copystream in do_copy and same is being used inside 
>>handleCopyIn() and handleCopyOut(). Once the \copy command execution 
>>finishes, we are resetting
>> the value of _psqlSettings->copystream to NULL. And if COPY(No slash) 
>> command is used, then in that case _psqlSettings->copystream will be NULL. 
>> So based on this value being NULL, copyStream will be assigned as 
>> stdout/stdin depending on
>>TO/FROM respectively inside the function handleCopyOut()/handleCopyIn().
 >>Also in order to address the queries like
>>./psql -d postgres -c "\copy tbl to 
>>'/home/rajeev/9.4gitcode/install/bin/data/temp.txt'; copy tbl from stdin;"
>>Inside the function ProcessResult, we check that if it is the second cycle 
>>and result status is COPY OUT or IN, then we reset the value of 
>>_psqlSettings->copystream to NULL, so that it can take the value as 
>>stdout/stdin for further processing.


>Yes, that's right, the second cycle should not use pset.copyStream.


>>handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res)
 >>{
>>boolOK = true;
>>char   *buf;
>>int ret;
>>-   PGresult   *res;
>>+
>>+   if (!copystream)
>>+   copystream = stdout;

>It should use pset.queryFout if it's NULL. Same in hadleCopyIn(). Otherwise, 
>the result of the following command goes to stdout, when it should go to the 
>output file :
>psql -d postgres -o /tmp/p.out -c "copy tab to stdout"
Yes you are right, I have changed it accordingly.


>>+   /*
>>+* If this is second copy; then it will be 
>>definately not \copy,
>>+* and also it can not be from any user given 
>>file.
>>+* So reset the value of copystream to NULL, 
>>so that read/wrie
>>+* happens from stdin/stdout.
>>+*/
>>+   if (!first_cycle)
>>+   pset.copyStream = NULL;

>Let ProcessResult() not change pset.copyStream. Let only do_copy() update it. 
>Instead of the above location, I suggest, just before calling 
>handleCopyOut/In(), we decide what to pass them as their copyStream parameter 
>depending upon whether it is
>first cycle or not.
OK. I have changed as per your suggestion.

Also I had removed the below line
if (copystream == pset.cur_cmd_source)
from the function handleCopyIn in my last patch itself. Reason for removal is 
that as per the earlier code the condition result was always true.

Please provide your opinion.

Thanks and Regards,
Kumar Rajeev Rastogi







copyerrorV5.patch
Description: copyerrorV5.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Amit Khandekar]

On 25 November 2013 15:25, Rajeev rastogi  wrote:

>  OK. I have revised the patch as per the discussion.
>
Could you please submit only the \COPY fix first ? The attached patch also
contains the fix for the original COPY status fix.

Now if \copy command is called then, we are setting the appropriate value
> of _psqlSettings->copystream in do_copy and same is being used inside
> handleCopyIn() and handleCopyOut(). Once the \copy command execution
> finishes, we are resetting the value of _psqlSettings->copystream to NULL.
> And if COPY(No slash) command is used, then in that case
> _psqlSettings->copystream will be NULL. So based on this value being NULL,
> copyStream will be assigned as stdout/stdin depending on TO/FROM
> respectively inside the function handleCopyOut()/handleCopyIn().
>


> Also in order to address the queries like
>
 *./psql -d postgres -c "\copy tbl to
> '/home/rajeev/9.4gitcode/install/bin/data/temp.txt'; copy tbl from stdin;"*
>
> Inside the function ProcessResult, we check that if it is the second cycle
> and result status is COPY OUT or IN, then we reset the value of
> _psqlSettings->copystream to NULL, so that it can take the value as
> stdout/stdin for further processing.
>
>
>

Yes, that's right, the second cycle should not use pset.copyStream.


handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res)
 {
boolOK = true;
char   *buf;
int ret;
-   PGresult   *res;
+
+   if (!copystream)
+   copystream = stdout;

It should use pset.queryFout if it's NULL. Same in hadleCopyIn().
Otherwise, the result of the following command goes to stdout, when it
should go to the output file :
psql -d postgres -o /tmp/p.out -c "copy tab to stdout"


+   /*
+* If this is second copy; then it will be
definately not \copy,
+* and also it can not be from any user
given file.
+* So reset the value of copystream to
NULL, so that read/wrie
+* happens from stdin/stdout.
+*/
+   if (!first_cycle)
+   pset.copyStream = NULL;

Let ProcessResult() not change pset.copyStream. Let only do_copy() update
it. Instead of the above location, I suggest, just before calling
handleCopyOut/In(), we decide what to pass them as their copyStream
parameter depending upon whether it is first cycle or not.





>  Please provide your opinion.
>
>
>
> Thanks and Regards,
>
> Kumar Rajeev Rastogi
>
>
>

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

[Rajeev rastogi]

On 25 November 2013, Amit Khandekar 
mailto:amit.khande...@enterprisedb.com>> wrote:
>>>Ok. we will then first fix the \COPY TO issue where it does not revert back 
>>>the overriden psql output file handle. Once this is solved, fix for both 
>>>COPY FROM and COPY TO, like how it is done in the patch earlier 
>>>(copydefectV2.patch).

>>I analyzed the solution to fix \COPY TO issue but unfortunately I observed 
>>that do_copy is already resetting the value of cur_cmd_source and queryFout 
>>but before that itself result status is printed. So we'll have to reset the 
>>value before result status is
>>being displayed.
>>So as other alternative solutions, I have two approaches:

>>1.  We can store current file destination queryFout in some local 
>>variable and pass the same to SendQuery function as a parameter. Same can be 
>>used to reset the value of queryFout after return from ProcessResult

>>From all other callers of SendQuery , we can pass NULL value for this new 
>>parameter.
 >>2.  We can add new structure member variable FILE *prevQueryFout in 
 >>structure "struct _psqlSettings", which hold the value of queryFout before 
 >>being changed in do_copy. And then same can be used to reset value in 
 >>SendQuery or ProcessResult.

>I think approach #2 is fine. Rather than prevQueryFout, I suggest defining a 
>separate FILE * handle for COPY. I don't see any other client-side command 
>that uses its own file pointer for reading and writing, like how COPY does. 
>And this handle has
> nothing to do with pset stdin and stdout. So we can have this special 
> _psqlSettings->copystream specifically for COPY. Both handleCopyIn() and 
> handleCopyOut() will be passed pset.copystream. In do_copy(),  instead of 
> overriding
>pset.queryFout, we can set pset.copystream to copystream, or to stdin/stdout 
>if copystream is NULL.

OK. I have revised the patch as per the discussion. Now if \copy command is 
called then, we are setting the appropriate value of _psqlSettings->copystream 
in do_copy and same is being used inside handleCopyIn() and handleCopyOut(). 
Once the \copy command execution finishes, we are resetting the value of 
_psqlSettings->copystream to NULL. And if COPY(No slash) command is used, then 
in that case _psqlSettings->copystream will be NULL. So based on this value 
being NULL, copyStream will be assigned as stdout/stdin depending on TO/FROM 
respectively inside the function handleCopyOut()/handleCopyIn().

Also in order to address the queries like
./psql -d postgres -c "\copy tbl to 
'/home/rajeev/9.4gitcode/install/bin/data/temp.txt'; copy tbl from stdin;"
Inside the function ProcessResult, we check that if it is the second cycle and 
result status is COPY OUT or IN, then we reset the value of 
_psqlSettings->copystream to NULL, so that it can take the value as 
stdout/stdin for further processing.

Please provide your opinion.

Thanks and Regards,
Kumar Rajeev Rastogi



copyerrorV4.patch
Description: copyerrorV4.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Amit Khandekar]

On 22 November 2013 16:14, Rajeev rastogi  wrote:

>  On 21 November 2013, Amit Khandekar 
> wrote:
>
> >Ok. we will then first fix the \COPY TO issue where it does not revert
> back the overriden psql output file handle. Once this is solved, fix for
> both COPY FROM and COPY TO, like how it is done in the patch earlier (
> copydefectV2.patch).
>
>
>
> I analyzed the solution to fix \COPY TO issue but unfortunately I observed
> that *do_copy* is already resetting the value of *cur_cmd_source and
> queryFout* but before that itself result status is printed. So we’ll have
> to reset the value before result status is being displayed.
>
>
>
> So as other alternative solutions, I have two approaches:
>
> 1.  We can store current file destination *queryFout *in some local
> variable and pass the same to *SendQuery* function as a parameter. Same
> can be used to reset the value of queryFout after return from ProcessResult
>
> From all other callers of SendQuery , we can pass NULL value for this new
> parameter.
>
>

> 2.  We can add new structure member variable FILE *prevQueryFout in
> structure “struct _*psqlSettings”, *which hold the value of queryFout
> before being changed in do_copy. And then same can be used to reset value
> in SendQuery or ProcessResult.
>
I think approach #2 is fine. Rather than prevQueryFout, I suggest defining
a separate FILE * handle for COPY. I don't see any other client-side
command that uses its own file pointer for reading and writing, like how
COPY does. And this handle has nothing to do with pset stdin and stdout. So
we can have this special _psqlSettings->copystream specifically for COPY.
Both handleCopyIn() and handleCopyOut() will be passed pset.copystream. In
do_copy(),  instead of overriding pset.queryFout, we can set
pset.copystream to copystream, or to stdin/stdout if copystream is NULL.


>
> Please let me know which approach is OK or if any other approach suggested.
>
> Based on feedback I shall prepare the new patch and share the same.
>
>
>
> Thanks and Regards,
>
> Kumar Rajeev Rastogi
>
>
>
>
>

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

[Rajeev rastogi]

On 21 November 2013, Amit Khandekar 
mailto:amit.khande...@enterprisedb.com>> wrote:
>Ok. we will then first fix the \COPY TO issue where it does not revert back 
>the overriden psql output file handle. Once this is solved, fix for both COPY 
>FROM and COPY TO, like how it is done in the patch earlier 
>(copydefectV2.patch).

I analyzed the solution to fix \COPY TO issue but unfortunately I observed that 
do_copy is already resetting the value of cur_cmd_source and queryFout but 
before that itself result status is printed. So we'll have to reset the value 
before result status is being displayed.

So as other alternative solutions, I have two approaches:

1.  We can store current file destination queryFout in some local variable 
and pass the same to SendQuery function as a parameter. Same can be used to 
reset the value of queryFout after return from ProcessResult

>From all other callers of SendQuery , we can pass NULL value for this new 
>parameter.

2.  We can add new structure member variable FILE *prevQueryFout in 
structure "struct _psqlSettings", which hold the value of queryFout before 
being changed in do_copy. And then same can be used to reset value in SendQuery 
or ProcessResult.

Please let me know which approach is OK or if any other approach suggested.
Based on feedback I shall prepare the new patch and share the same.

Thanks and Regards,
Kumar Rajeev Rastogi

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

[Rajeev rastogi]

On 20 November, Amit Khandekar wrote:
I hope you meant to write test case as psql -d postgres -c "\copy tab from 
stdin; insert into tab values ('lll', 3)", as if we are reading from file, 
then the above issue does not come.
>>>I meant COPY with a slash. \COPY is equivalent to COPY FROM STDIN. So the 
>>>issue can also be reproduced by :
>>>\COPY tab from 'client_filename' ...

 I have modified the patch as per your comment and same is attached with 
 this mail.

>>>Thanks. The COPY FROM looks good.
>>OK..Thanks
>>>With the patch applied, \COPY TO 'data_file' command outputs the  COPY 
>>>status into the data file, instead of printing it in the psql session.
>>>postgres=# \copy tab to '/tmp/fout';
>>>postgres=#
 >>>$ cat /tmp/fout
>>>ee  909
>>>COPY 1
>>>This is probably because client-side COPY overrides the pset.queryFout with 
>>>its own destination file, and while printing the COPY status, the overridden 
>>>file pointer is not yet reverted back.
 >>This looks to be an issue without our new patch also. Like I tried following 
 >>command and output was as follows:
>>rajeev@linux-ltr9:~/9.4gitcode/install/bin>
>> ./psql -d postgres -c "\copy tbl to 'new.txt';insert into tbl values(55);"
>>rajeev@linux-ltr9:~/9.4gitcode/install/bin>
>> cat new.txt
>>5
>>67
>>5
>>67
>>2
>>2
>>99
>>1
>>1
>INSERT 0 1

>Ok. Yes it is an existing issue. Because we are now printing the COPY status 
>even for COPY TO, the existing issue surfaces too easily with the patch. \COPY 
>TO is a pretty common scenario. And it does not have to have a subsequent 
>another command
>to reproduce the issue Just a single \COPY TO command reproduces the issue.
>>I have fixed the same as per your suggestion by resetting the pset.queryFout 
>>after the function call "handleCopyOut".
>>!  pset.queryFout = stdout;

>The original pset.queryFout may not be stdout. psql -o option can override the 
>stdout default.

>I think solving the \COPY TO part is going to be a  different (and an 
>involved) issue to solve than the COPY FROM. Even if we manage to revert back 
>the queryFout, I think ProcessResult() is not the right place to do it. 
>ProcessResult() should not
> assume that  somebody else has changed queryFout. Whoever has changed it 
> should revert it. Currently, do_copy() is indeed doing this correctly:

>  save_file = *override_file;
>  *override_file = copystream;
>  success = SendQuery(query.data);
>  *override_file = save_file;

>But the way SendQuery() itself processes the results and prints them, is 
>conflicting with the above.

>So I think it is best to solve this as a different issue, and we should , for 
>this commitfest,  fix only COPY FROM. Once the \COPY existing issue is solved, 
>only then we can start printing the \COPY TO status as well.

You mean to say that I should change the patch to keep only COPY FROM related 
changes and remove changes related to COPY TO.
If yes, then I shall change the patch accordingly  and also mention same in 
documentation also.
Please let me know about this so that I can share the modified patch.

Thanks and Regards,
Kumar Rajeev Rastogi

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

[Rajeev rastogi]

On 19 November 2013, Amit Khandekar wrote:
>On 18 November 2013 18:00, Rajeev rastogi 
>mailto:rajeev.rast...@huawei.com>> wrote:
>>On 18 November 2013, Amit Khandekar wrote:
> >>Please find the patch for the same and let me know your suggestions.
>>>In this call :
> >> success = handleCopyIn(pset.db, 
> >> pset.cur_cmd_source,
> >>
> >>PQbinaryTuples(*results), &intres) && success;
> >> if (success && intres)
> >> success = 
> >> PrintQueryResults(intres);
>>>Instead of handling of the result status this way, what if we use the 
>>>ProcessResult()  argument 'result' to pass back the COPY result status to 
>>>the caller ? We already call PrintQueryResults(results) after the 
>>>ProcessResult() call. So we don't have to  have a COPY-specific 
>>>PrintQueryResults() call. Also, if there is a subsequent SQL command in the 
>>>same query string, the consequence of the patch is that the client prints 
>>>both COPY output and the last command output. So my suggestion would also 
>>>allow us to be consistent with the general behaviour that only the last SQL 
>>>command output is printed in case of multiple SQL commands. Here is how it 
>>>gets printed with your patch :
>> Thank you for valuable comments. Your suggestion is absolutely correct.
 >>>psql -d postgres -c "\copy tab from '/tmp/st.sql' delimiter ' '; insert 
 >>>into tab values ('lll', 3)"
>>>COPY 1
>>>INSERT 0 1
>>>This is not harmful, but just a matter of consistency.
>>I hope you meant to write test case as psql -d postgres -c "\copy tab from 
>>stdin; insert into tab values ('lll', 3)", as if we are reading from file, 
>>then the above issue does not come.
>I meant COPY with a slash. \COPY is equivalent to COPY FROM STDIN. So the 
>issue can also be reproduced by :
>\COPY tab from 'client_filename' ...

 >>I have modified the patch as per your comment and same is attached with this 
 >>mail.

>Thanks. The COPY FROM looks good.
OK..Thanks

>With the patch applied, \COPY TO 'data_file' command outputs the  COPY status 
>into the data file, instead of printing it in the psql session.

>postgres=# \copy tab to '/tmp/fout';
>postgres=#

>$ cat /tmp/fout
>ee  909
>COPY 1
>This is probably because client-side COPY overrides the pset.queryFout with 
>its own destination file, and while printing the COPY status, the overridden 
>file pointer is not yet reverted back.

This looks to be an issue without our new patch also. Like I tried following 
command and output was as follows:
rajeev@linux-ltr9:~/9.4gitcode/install/bin> ./psql -d postgres -c "\copy tbl to 
'new.txt';insert into tbl values(55);"
rajeev@linux-ltr9:~/9.4gitcode/install/bin> cat new.txt
5
67
5
67
2
2
99
1
1
INSERT 0 1

I have fixed the same as per your suggestion by resetting the pset.queryFout 
after the function call "handleCopyOut".

Please let me know in-case of any other issues.

Thanks and Regards,
Kumar Rajeev Rastogi


copydefectV3.patch
Description: copydefectV3.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Amit Khandekar]

On 21 November 2013 10:13, Tom Lane  wrote:

> Amit Khandekar  writes:
> > Rather than a behaviour change, it is a bug that we are fixing. User
> > already expects to see copy status printed, so as per user there would be
> > no behaviour change.
>
> This is arrant nonsense.  It's a behavior change.  You can't make it
> not that by claiming something about user expectations.  Especially
> since this isn't exactly a corner case that nobody has seen in
> the fifteen years or so that it's worked like that.  People do know
> how this works.
>

Yes, I agree that this is not a corner case, so users may already know the
current behaviour.

>
> I don't object to changing it, but I do agree with Robert that it's
> important to quantize such changes, ie, try to get a set of related
> changes to appear in the same release.  People don't like repeatedly
> revising their code for such things.
>

Ok. we will then first fix the \COPY TO issue where it does not revert back
the overriden psql output file handle. Once this is solved, fix for both
COPY FROM and COPY TO, like how it is done in the patch earlier (
copydefectV2.patch).

>
> regards, tom lane
>

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

[Tom Lane]

Amit Khandekar  writes:
> Rather than a behaviour change, it is a bug that we are fixing. User
> already expects to see copy status printed, so as per user there would be
> no behaviour change.

This is arrant nonsense.  It's a behavior change.  You can't make it
not that by claiming something about user expectations.  Especially
since this isn't exactly a corner case that nobody has seen in
the fifteen years or so that it's worked like that.  People do know
how this works.

I don't object to changing it, but I do agree with Robert that it's
important to quantize such changes, ie, try to get a set of related
changes to appear in the same release.  People don't like repeatedly
revising their code for such things.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Amit Khandekar]

On 20 November 2013 18:11, Robert Haas  wrote:

> On Wed, Nov 20, 2013 at 4:56 AM, Amit Khandekar
>  wrote:
> > So I think it is best to solve this as a different issue, and we should ,
> > for this commitfest,  fix only COPY FROM. Once the \COPY existing issue
> is
> > solved, only then we can start printing the \COPY TO status as well.
>
> I actually think that we should probably fix the \COPY issue first.
> Otherwise, we may end up (for example) changing COPY FROM in one
> release and COPY TO in the next release, and that would be annoying.
> It does cause application compatibility problems to some degree when
> we change things like this, so it's useful to avoid doing it multiple
> times.  And I can't really see a principled reason for COPY FROM and
> COPY TO to behave differently, either.
>

Rather than a behaviour change, it is a bug that we are fixing. User
already expects to see copy status printed, so as per user there would be
no behaviour change.

So the idea is to fix it in two places independently. Whatever fix we are
doing for COPY FROM , we would not revert or change that fix when we fix
the COPY TO issue. The base changes will go in COPY FROM fix, and then we
will be extending (not rewriting) the fix for COPY TO after fixing the
\COPY-TO issue.

Also, we already have a fix ready for COPY FROM, so I thought we better
commit this first.


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

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

[Robert Haas]

On Wed, Nov 20, 2013 at 4:56 AM, Amit Khandekar
 wrote:
> So I think it is best to solve this as a different issue, and we should ,
> for this commitfest,  fix only COPY FROM. Once the \COPY existing issue is
> solved, only then we can start printing the \COPY TO status as well.

I actually think that we should probably fix the \COPY issue first.
Otherwise, we may end up (for example) changing COPY FROM in one
release and COPY TO in the next release, and that would be annoying.
It does cause application compatibility problems to some degree when
we change things like this, so it's useful to avoid doing it multiple
times.  And I can't really see a principled reason for COPY FROM and
COPY TO to behave differently, either.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Amit Khandekar]

On 20 November 2013 17:40, Rajeev rastogi  wrote:

> You mean to say that I should change the patch to keep only COPY FROM
> related changes and remove changes related to COPY TO.
>
> If yes, then I shall change the patch accordingly  and also mention same
> in documentation also.
>
> Please let me know about this so that I can share the modified patch.
>

RIght.


>
>
> Thanks and Regards,
>
> Kumar Rajeev Rastogi
>

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

[Amit Khandekar]

On 19 November 2013 16:05, Rajeev rastogi  wrote:

>  On 19 November 2013, Amit Khandekar wrote:
>
> >On 18 November 2013 18:00, Rajeev rastogi 
> wrote:
>
> >>On 18 November 2013, Amit Khandekar wrote:
>
> > >>Please find the patch for the same and let me know your suggestions.
>
> >>>In this call :
>
> > >> success = handleCopyIn(pset.db,
> pset.cur_cmd_source,
>
> > >>
>   PQbinaryTuples(*results), &intres) && success;
>
> > >> if (success && intres)
>
> > >> success =
> PrintQueryResults(intres);
>
> >>>Instead of handling of the result status this way, what if we use the
> ProcessResult()  argument 'result' to pass back the COPY result status to
> the caller ? We already call PrintQueryResults(results) after the
> ProcessResult() call. So we don't have to  have a COPY-specific
> PrintQueryResults() call. Also, if there is a subsequent SQL command in the
> same query string, the consequence of the patch is that the client prints
> both COPY output and the last command output. So my suggestion would also
> allow us to be consistent with the general behaviour that only the last SQL
> command output is printed in case of multiple SQL commands. Here is how it
> gets printed with your patch :
>
> >> Thank you for valuable comments. Your suggestion is absolutely correct.
>
>  >>>psql -d postgres -c "\copy tab from '/tmp/st.sql' delimiter ' ';
> insert into tab values ('lll', 3)"
>
> >>>COPY 1
>
> >>>INSERT 0 1
>
> >>>This is not harmful, but just a matter of consistency.
>
> >>I hope you meant to write test case as *psql -d postgres -c "\copy tab
> from stdin; insert into tab values ('lll', 3)", *as if we are reading
> from file, then the above issue does not come.
>
> >I meant COPY with a slash. \COPY is equivalent to COPY FROM STDIN. So the
> issue can also be reproduced by :
>
> >\COPY tab from 'client_filename' ...
>
>
>
>  >>I have modified the patch as per your comment and same is attached with
> this mail.
>
>
>
> >Thanks. The COPY FROM looks good.
>
> OK..Thanks
>
>
>
> >With the patch applied, \COPY TO 'data_file' command outputs the  COPY
> status into the data file, instead of printing it in the psql session.
>
>
>
> >postgres=# \copy tab to '/tmp/fout';
>
> >postgres=#
>
>
>
> >$ cat /tmp/fout
>
> >ee  909
>
> >COPY 1
>
> >This is probably because client-side COPY overrides the pset.queryFout
> with its own destination file, and while printing the COPY status,
> the overridden file pointer is not yet reverted back.
>
>
>
> This looks to be an issue without our new patch also. Like I tried
> following command and output was as follows:
>
> rajeev@linux-ltr9:~/9.4gitcode/install/bin> ./psql -d postgres -c "\copy
> tbl to 'new.txt';insert into tbl values(55);"
>
> rajeev@linux-ltr9:~/9.4gitcode/install/bin> cat new.txt
>
> 5
>
> 67
>
> 5
>
> 67
>
> 2
>
> 2
>
> 99
>
> 1
>
> 1
>
> INSERT 0 1
>

Ok. Yes it is an existing issue. Because we are now printing the COPY
status even for COPY TO, the existing issue surfaces too easily with the
patch. \COPY TO is a pretty common scenario. And it does not have to have a
subsequent another command to reproduce the issue Just a single \COPY TO
command reproduces the issue.


>
> I have fixed the same as per your suggestion by resetting the
> pset.queryFout after the function call “handleCopyOut”.
>

! pset.queryFout = stdout;

The original pset.queryFout may not be stdout. psql -o option can override
the stdout default.

I think solving the \COPY TO part is going to be a  different (and an
involved) issue to solve than the COPY FROM. Even if we manage to revert
back the queryFout, I think ProcessResult() is not the right place to do
it. ProcessResult() should not assume that  somebody else has changed
queryFout. Whoever has changed it should revert it. Currently, do_copy() is
indeed doing this correctly:

save_file = *override_file;
*override_file = copystream;
success = SendQuery(query.data);
 *override_file = save_file;

But the way SendQuery() itself processes the results and prints them, is
conflicting with the above.

So I think it is best to solve this as a different issue, and we should ,
for this commitfest,  fix only COPY FROM. Once the \COPY existing issue is
solved, only then we can start printing the \COPY TO status as well.



>
> Please let me know in-case of any other issues.
>
>
>
> Thanks and Regards,
>
> Kumar Rajeev Rastogi
>

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

[Amit Khandekar]

On 18 November 2013 18:00, Rajeev rastogi  wrote:

>  On 18 November 2013, Amit Khandekar wrote:
>
> >> On 18 October 2013 17:07, Rajeev rastogi 
> wrote:
>
> >>From the following mail, copy behaviour between stdin and normal file
> having some inconsistency.
>
> >>
> http://www.postgresql.org/message-id/ce85a517.4878e%tim.k...@gmail.com
>
> >>The issue was that if copy  execute "from stdin", then it goes to the
> server to execute the command and then server request for the input, it
> sends back the control to client to enter the data. So
>
> >> once client sends the input to server, server execute the copy command
> and sends back the result to client but client does not print the result
> instead it just clear it out.
>
> >> Changes are made to ensure the final result from server get printed
> before clearing the result.
>
> > Please find the patch for the same and let me know your suggestions.
>
> >In this call :
>
> >  success = handleCopyIn(pset.db,
> pset.cur_cmd_source,
>
> >
>   PQbinaryTuples(*results), &intres) && success;
>
> >
>
> >  if (success && intres)
>
> >  success =
> PrintQueryResults(intres);
>
> >
>
> >Instead of handling of the result status this way, what if we use the
> ProcessResult()  argument 'result' to pass back the COPY result status to
> the caller ? We already call PrintQueryResults(results) after the
> ProcessResult() call. So we don't have to have a
>
> > COPY-specific PrintQueryResults() call. Also, if there is a subsequent
> SQL command in the same query string, the consequence of the patch is that
> the client prints both COPY output and the last command output. So my
> suggestion would also allow us
>
> > to be consistent with the general behaviour that only the last SQL
> command output is printed in case of multiple SQL commands. Here is how it
> gets printed with your patch :
>
>  Thank you for valuable comments. Your suggestion is absolutely correct.
>
>  >psql -d postgres -c "\copy tab from '/tmp/st.sql' delimiter ' '; insert
> into tab values ('lll', 3)"
>
> >COPY 1
>
> >INSERT 0 1
>
> >
>
> >This is not harmful, but just a matter of consistency.
>
> I hope you meant to write test case as *psql -d postgres -c "\copy tab
> from stdin; insert into tab values ('lll', 3)", *as if we are reading
> from file, then the above issue does not come.
>
I meant COPY with a slash. \COPY is equivalent to COPY FROM STDIN. So the
issue can also be reproduced by :
\COPY tab from 'client_filename' ...


>   I have modified the patch as per your comment and same is attached with
> this mail.
>

Thanks. The COPY FROM looks good.

With the patch applied, \COPY TO 'data_file' command outputs the  COPY
status into the data file, instead of printing it in the psql session.

postgres=# \copy tab to '/tmp/fout';
postgres=#

$ cat /tmp/fout
ee 909
COPY 1

This is probably because client-side COPY overrides the pset.queryFout with
its own destination file, and while printing the COPY status,
the overridden file pointer is not yet reverted back.


 Please let me know in-case of any other issues or suggestions.
>
>
>
> *Thanks and Regards,*
>
> *Kumar Rajeev Rastogi *
>
>
>

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

[Rajeev rastogi]

On 18 November 2013, Amit Khandekar wrote:
>> On 18 October 2013 17:07, Rajeev rastogi 
>> mailto:rajeev.rast...@huawei.com>> wrote:
>>From the following mail, copy behaviour between stdin and normal file having 
>>some inconsistency.
>>   
>> http://www.postgresql.org/message-id/ce85a517.4878e%tim.k...@gmail.com
>>The issue was that if copy  execute "from stdin", then it goes to the server 
>>to execute the command and then server request for the input, it sends back 
>>the control to client to enter the data. So
>> once client sends the input to server, server execute the copy command and 
>> sends back the result to client but client does not print the result instead 
>> it just clear it out.
>> Changes are made to ensure the final result from server get printed before 
>> clearing the result.
> Please find the patch for the same and let me know your suggestions.
>In this call :
>  success = handleCopyIn(pset.db, 
> pset.cur_cmd_source,
>   
>  PQbinaryTuples(*results), &intres) && success;
>
>  if (success && intres)
>  success = 
> PrintQueryResults(intres);
>
>Instead of handling of the result status this way, what if we use the 
>ProcessResult()  argument 'result' to pass back the COPY result status to the 
>caller ? We already call PrintQueryResults(results) after the ProcessResult() 
>call. So we don't have to have a
> COPY-specific PrintQueryResults() call. Also, if there is a subsequent SQL 
> command in the same query string, the consequence of the patch is that the 
> client prints both COPY output and the last command output. So my suggestion 
> would also allow us
> to be consistent with the general behaviour that only the last SQL command 
> output is printed in case of multiple SQL commands. Here is how it gets 
> printed with your patch :
 Thank you for valuable comments. Your suggestion is absolutely correct.
 >psql -d postgres -c "\copy tab from '/tmp/st.sql' delimiter ' '; insert into 
 >tab values ('lll', 3)"
>COPY 1
>INSERT 0 1
>
>This is not harmful, but just a matter of consistency.
I hope you meant to write test case as psql -d postgres -c "\copy tab from 
stdin; insert into tab values ('lll', 3)", as if we are reading from file, then 
the above issue does not come.
 I have modified the patch as per your comment and same is attached with this 
mail.
Please let me know in-case of any other issues or suggestions.

Thanks and Regards,
Kumar Rajeev Rastogi



copydefectV2.patch
Description: copydefectV2.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Amit Khandekar]

On 18 October 2013 17:07, Rajeev rastogi  wrote:

>  From the following mail, copy behaviour between stdin and normal file
> having some inconsistency.
>
>
> http://www.postgresql.org/message-id/ce85a517.4878e%tim.k...@gmail.com
>
>
>
> The issue was that if copy  execute "from stdin", then it goes to the
> server to execute the command and then server request for the input, it
> sends back the control to client to enter the data. So once client sends
> the input to server, server execute the copy command and sends back the
> result to client but client does not print the result instead it just clear
> it out.
>
> Changes are made to ensure the final result from server get printed before
> clearing the result.
>
>
>
> Please find the patch for the same and let me know your suggestions.
>

In this call :
success = handleCopyIn(pset.db, pset.cur_cmd_source,
   PQbinaryTuples(*results), &intres) && success;

if (success && intres)
 success = PrintQueryResults(intres);

Instead of handling of the result status this way, what if we use the
ProcessResult()  argument 'result' to pass back the COPY result status to
the caller ? We already call PrintQueryResults(results) after the
ProcessResult() call. So we don't have to have a COPY-specific
PrintQueryResults() call. Also, if there is a subsequent SQL command in the
same query string, the consequence of the patch is that the client prints
both COPY output and the last command output. So my suggestion would also
allow us to be consistent with the general behaviour that only the last SQL
command output is printed in case of multiple SQL commands. Here is how it
gets printed with your patch :

psql -d postgres -c "\copy tab from '/tmp/st.sql' delimiter ' '; insert
into tab values ('lll', 3)"
COPY 1
INSERT 0 1

This is not harmful, but just a matter of consistency.


>
>
> *Thanks and Regards,*
>
> *Kumar Rajeev Rastogi *
>
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

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

[Rajeev rastogi]

On 21 October 2013 20:48,  Robert Haas  wrote:
>On Fri, Oct 18, 2013 at 7:37 AM, Rajeev rastogi  
>wrote:
>> From the following mail, copy behaviour between stdin and normal file 
>> having some inconsistency.
>>
>>
>> http://www.postgresql.org/message-id/ce85a517.4878e%tim.k...@gmail.com
>>
>>
>>
>> The issue was that if copy  execute "from stdin", then it goes to the 
>> server to execute the command and then server request for the input, 
>> it sends back the control to client to enter the data. So once client 
>> sends the input to server, server execute the copy command and sends 
>> back the result to client but client does not print the result instead it 
>> just clear it out.
>>
>> Changes are made to ensure the final result from server get printed 
>> before clearing the result.
>>
>>
>>
>> Please find the patch for the same and let me know your suggestions.
>>
>>
>
>Please add your patch to the currently-open CommitFest so that it does not get 
>forgotten:
>
>https://commitfest.postgresql.org/action/commitfest_view/open

Added to the currently-open CommitFest.

Thanks and Regards,
Kumar Rajeev Rastogi


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Robert Haas]

On Fri, Oct 18, 2013 at 7:37 AM, Rajeev rastogi
 wrote:
> From the following mail, copy behaviour between stdin and normal file having
> some inconsistency.
>
>
> http://www.postgresql.org/message-id/ce85a517.4878e%tim.k...@gmail.com
>
>
>
> The issue was that if copy  execute "from stdin", then it goes to the server
> to execute the command and then server request for the input, it sends back
> the control to client to enter the data. So once client sends the input to
> server, server execute the copy command and sends back the result to client
> but client does not print the result instead it just clear it out.
>
> Changes are made to ensure the final result from server get printed before
> clearing the result.
>
>
>
> Please find the patch for the same and let me know your suggestions.
>
>
>
> Thanks and Regards,
>
> Kumar Rajeev Rastogi

Please add your patch to the currently-open CommitFest so that it does
not get forgotten:

https://commitfest.postgresql.org/action/commitfest_view/open

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers