On 19 November 2013 16:05, Rajeev rastogi <rajeev.rast...@huawei.com> wrote:
> On 19 November 2013, Amit Khandekar wrote:
>
> >On 18 November 2013 18:00, Rajeev rastogi <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
>
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
>
