Re: COPY table FROM STDIN doesn't show count tag - Mailing list pgsql-hackers

From Amit Khandekar
Subject Re: COPY table FROM STDIN doesn't show count tag
Date
Msg-id CACoZds3kF33gDXBfeCno9xOchUDA_HSnyGXroAtSOHbiObJVwA@mail.gmail.com
Whole thread Raw
In response to COPY table FROM STDIN doesn't show count tag  (Rajeev rastogi <rajeev.rastogi@huawei.com>)
Responses Re: COPY table FROM STDIN doesn't show count tag  (Robert Haas <robertmhaas@gmail.com>)
List pgsql-hackers



On 19 November 2013 16:05, Rajeev rastogi <rajeev.rastogi@huawei.com> wrote:

On 19 November 2013, Amit Khandekar wrote:

>On 18 November 2013 18:00, Rajeev rastogi <rajeev.rastogi@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


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


pgsql-hackers by date:

Previous
From: Fujii Masao
Date:
Subject: Re: -d option for pg_isready is broken
Next
From: Michael Paquier
Date:
Subject: Re: VACUUM for TOASTed objects