Thread: COPY table FROM STDIN doesn't show count tag
From the following mail, copy behaviour between stdin and normal file having some inconsistency.
http://www.postgresql.org/message-id/CE85A517.4878E%tim.kane@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
Attachment
On Fri, Oct 18, 2013 at 7:37 AM, Rajeev rastogi <rajeev.rastogi@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.kane@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
On 21 October 2013 20:48, Robert Haas <robertmhaas@gmail.com> wrote: >On Fri, Oct 18, 2013 at 7:37 AM, Rajeev rastogi <rajeev.rastogi@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.kane@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
From the following mail, copy behaviour between stdin and normal file having some inconsistency.
http://www.postgresql.org/message-id/CE85A517.4878E%tim.kane@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 18 November 2013, Amit Khandekar wrote:
>> On 18 October 2013 17:07, Rajeev rastogi <rajeev.rastogi@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.kane@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
Attachment
On 18 November 2013, Amit Khandekar wrote:
>> On 18 October 2013 17:07, Rajeev rastogi <rajeev.rastogi@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.kane@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
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
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
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
On Wed, Nov 20, 2013 at 4:56 AM, Amit Khandekar <amit.khandekar@enterprisedb.com> 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
On Wed, Nov 20, 2013 at 4:56 AM, Amit KhandekarI actually think that we should probably fix the \COPY issue first.
<amit.khandekar@enterprisedb.com> 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.
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.
Amit Khandekar <amit.khandekar@enterprisedb.com> 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
Amit Khandekar <amit.khandekar@enterprisedb.com> writes:This is arrant nonsense. It's a behavior change. You can't make it
> 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.
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
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
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
Attachment
<div class="WordSection1"><p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">On 20
November,Amit Khandekar wrote:<p class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">>>>>Ihope you meant to write test case as
<b>psql-d postgres -c "\copy tab from <span style="color:red">stdin</span>; insert into tab values ('lll', 3)", </b>as
ifwe are reading from file, then the above issue does not come.<p class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">>>>Imeant COPY with a slash. \COPY is equivalent to
COPYFROM STDIN. So the issue can also be reproduced by :<p class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">>>>\COPYtab from 'client_filename' ...<p
class="MsoNormal"style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <p class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> >>>>Ihave modified the patch as per your
commentand same is attached with this mail.<p class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <pclass="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">>>>Thanks.The COPY FROM looks good.<p
class="MsoNormal"style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">>>OK..Thanks <p class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">>>>Withthe patch applied, \COPY TO 'data_file'
commandoutputs the COPY status into the data file, instead of printing it in the psql session.<p class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">>>>postgres=#\copy tab to '/tmp/fout';<p
class="MsoNormal"style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">>>>postgres=# <p class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> >>>$cat /tmp/fout <p class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">>>>ee 909<p class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">>>>COPY1<p class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">>>>Thisis probably because client-side COPY
overridesthe pset.queryFout with its own destination file, and while printing the COPY status, the overridden file
pointeris not yet reverted back.<p class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> >><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Thislooks to be an issue without our new patch also. Like I
triedfollowing command and output was as follows:</span><p class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt">>><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif""><a
href="mailto:rajeev@linux-ltr9:~/9.4gitcode/install/bin">rajeev@linux-ltr9:~/9.4gitcode/install/bin</a>>./psql -d
postgres-c "\copy tbl to 'new.txt';insert into tbl values(55);"</span><p class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt">>><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif""><a
href="mailto:rajeev@linux-ltr9:~/9.4gitcode/install/bin">rajeev@linux-ltr9:~/9.4gitcode/install/bin</a>>cat
new.txt</span><pclass="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt">
>><spanstyle="font-size:11.0pt;font-family:"Calibri","sans-serif"">5</span><p class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt">>><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">67</span><pclass="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt">>><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">5</span><pclass="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt">>><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">67</span><pclass="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt">>><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">2</span><pclass="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt">>><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">2</span><pclass="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt">>><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">99</span><pclass="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt">>><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">1</span><pclass="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt">>><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">1</span><pclass="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt">><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">INSERT0 1</span><p class="MsoNormal"> <p
class="MsoNormal">>Ok.Yes it is an existing issue. Because we are now printing the COPY status even for COPY TO, the
existingissue surfaces too easily with the patch. \COPY TO is a pretty common scenario. And it does not have to have a
subsequentanother command <p class="MsoNormal">>to reproduce the issue Just a single \COPY TO command reproduces the
issue.<pclass="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">>><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Ihave fixed the same as per your suggestion by resetting
thepset.queryFout after the function call “handleCopyOut”.</span><p class="MsoNormal">>>!
pset.queryFout = stdout;<p class="MsoNormal"> <p class="MsoNormal">>The
originalpset.queryFout may not be stdout. psql -o option can override the stdout default.<p class="MsoNormal"> <p
class="MsoNormal">>Ithink solving the \COPY TO part is going to be a different (and an involved) issue to solve
thanthe 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 <p class="MsoNormal">> assume that somebody else has changed queryFout. Whoever has
changedit should revert it. Currently, do_copy() is indeed doing this correctly:<p class="MsoNormal"> <p
class="MsoNormal">> save_file = *override_file;<p class="MsoNormal">> *override_file =
copystream;<pclass="MsoNormal">> success = SendQuery(query.data);<p class="MsoNormal">>
*override_file= save_file;<p class="MsoNormal"> <p class="MsoNormal">>But the way SendQuery() itself processes the
resultsand prints them, is conflicting with the above. <p class="MsoNormal"> <p class="MsoNormal">>So I think it is
bestto solve this as a different issue, and we should , for this commitfest, fix only COPY FROM. Once the \COPY
existingissue is solved, only then we can start printing the \COPY TO status as well.<p class="MsoNormal"> <p
class="MsoNormal">Youmean to say that I should change the patch to keep only COPY FROM related changes and remove
changesrelated to COPY TO. <p class="MsoNormal">If yes, then I shall change the patch accordingly and also mention
samein documentation also.<p class="MsoNormal">Please let me know about this so that I can share the modified patch.<p
class="MsoNormal"> <pclass="MsoNormal">Thanks and Regards,<p class="MsoNormal">Kumar Rajeev Rastogi</div>
<div class="WordSection1"><p class="MsoNormal" style="margin-bottom:12.0pt">On 21 November 2013, Amit Khandekar <<a href="mailto:amit.khandekar@enterprisedb.com">amit.khandekar@enterprisedb.com</a>>wrote:<p class="MsoNormal">>Ok. wewill then first fix the \COPY TO issue where it does not revert back the overriden psql output file handle. Once this issolved, fix for both COPY FROM and COPY TO, like how it is done in the patch earlier (<span style="font-size:9.5pt;font-family:"Arial","sans-serif"">copydefectV2.patch).</span><pclass="MsoNormal"> <p class="MsoNormal">Ianalyzed the solution to fix \COPY TO issue but unfortunately I observed that <i>do_copy</i> is alreadyresetting the value of <i>cur_cmd_source and queryFout</i> but before that itself result status is printed. So we’llhave to reset the value before result status is being displayed.<p class="MsoNormal"> <p class="MsoNormal">So as otheralternative solutions, I have two approaches:<p class="MsoListParagraph" style="text-indent:-18.0pt;mso-list:l0 level1lfo1"><span style="mso-list:Ignore">1.<span style="font:7.0pt "Times New Roman""> </span></span>We can store currentfile destination <i>queryFout </i>in some local variable and pass the same to <i>SendQuery</i> function as a parameter.Same can be used to reset the value of queryFout after return from ProcessResult<p class="MsoListParagraph">Fromall other callers of SendQuery , we can pass NULL value for this new parameter.<p class="MsoListParagraph"style="text-indent:-18.0pt;mso-list:l0 level1 lfo1"><span style="mso-list:Ignore">2.<span style="font:7.0pt"Times New Roman""> </span></span>We can add new structure member variable FILE *prevQueryFout in structure“struct _<i>psqlSettings”, </i>which hold the value of queryFout before being changed in do_copy. And then samecan be used to reset value in SendQuery or ProcessResult.<p class="MsoNormal"> <p class="MsoNormal">Please let me knowwhich approach is OK or if any other approach suggested.<p class="MsoNormal">Based on feedback I shall prepare the newpatch and share the same.<p class="MsoNormal"> <p class="MsoNormal">Thanks and Regards,<p class="MsoNormal">Kumar RajeevRastogi<p class="MsoNormal"> <p class="MsoNormal"> </div>
On 21 November 2013, Amit Khandekar <amit.khandekar@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
On 25 November 2013, Amit Khandekar <amit.khandekar@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
Attachment
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.
handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res)
Please provide your opinion.
Thanks and Regards,
Kumar 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)
>>{
>> bool OK = 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
Attachment
On 26 November 2013, Amit Khandelkar wrote:
On 26 November 2013 18:59, Amit Khandekar <amit.khandekar@enterprisedb.com> wrote:On 25 November 2013 15:25, Rajeev rastogi <rajeev.rastogi@huawei.com> 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.
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
Attachment
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
<div class="WordSection1"><p class="MsoNormal" style="mso-margin-top-alt:auto;margin-bottom:12.0pt">On 9<sup>th</sup> December,Amit Khandelkar wrote:<p>>1.<span style="font-size:7.0pt"> </span>slashcopyissuev1.patch :- This patch fixesthe \COPY issue.<p class="MsoNormal">>You have removed the if condition in this statement, mentioning that it isalways true now:<p class="MsoNormal">>- if (copystream == pset.cur_cmd_source)<p class="MsoNormal">>- pset.lineno++;<p class="MsoNormal">>+ pset.lineno++;<pclass="MsoNormal">> <p class="MsoNormal"> >But copystream can be different than pset.cur_cmd_source, right ?<p class="MsoNormal"> <p class="MsoNormal">As per the earlier code, condition result was alwaystrue. So pset.lineno was always incremented.<p class="MsoNormal">In the earlier code pset.cur_cmd_source was sent asparameter to function and inside the function same parameter was used with the name copystream. So on entry of this functionboth will be one and same.<p class="MsoNormal">I checked inside the function handleCopyIn, both of these parametersare not changing before above check. Also since pset is specific to single session, so it cannot change concurrently.<pclass="MsoNormal">Please let me know, if I am missing something.<p class="MsoNormal"> <p class="MsoNormal">>+ FILE *copyStream; /* Stream to read/write for copy command */<p class="MsoNormal">> <pclass="MsoNormal">>There is no tab between FILE and *copystream, hence it is not aligned.<p class="MsoNormal"> <pclass="MsoNormal">OK. I shall change accodingly.<p class="MsoNormal"> <p>>2.<span style="font-size:7.0pt"> </span>initialcopyissuev1_ontopofslashcopy.patch : Fix for “COPY table FROM STDIN/STDOUT doesn'tshow count tag”.<p class="MsoNormal">>The following header comments of ProcessResult() need to be modified:<p class="MsoNormal">>*Changes its argument to point to the last PGresult of the command string,<p class="MsoNormal">>*or NULL if that result was for a COPY FROM STDIN or COPY TO STDOUT.<p class="MsoNormal"> <p class="MsoNormal">OK.I shall change accodingly.<p class="MsoNormal"> <p class="MsoNormal">>Regression results show allpassed. <p class="MsoNormal">>Other than this, the patch needs a new regression test.<p class="MsoNormal"> <p class="MsoNormal">Ihad checked the existing regression test cases and observed that it has already got all kind of test cases.Like <p class="MsoNormal" style="text-indent:36.0pt">copy….stdin, <p class="MsoNormal" style="text-indent:36.0pt">copy….stdout,<p class="MsoNormal" style="text-indent:36.0pt">\copy…..stdin <p class="MsoNormal"style="text-indent:36.0pt">\copy…..stdout.<p class="MsoNormal"> <p class="MsoNormal">But since as regressionframework runs in “quite i.e. –q” mode, so it does not show any message except query output. <p class="MsoNormal">Soour new code change does not impact regression framework.<p class="MsoNormal"> <p class="MsoNormal">Pleaselet me know if you were expecting any other test cases?<p class="MsoNormal"> <p class="MsoNormal">>Idon't think we need to do any doc changes, because the doc already mentions that COPY should showthe COUNT tag, and does not mention anything specific to client-side COPY.<p class="MsoNormal"> OK.<p class="MsoNormal"> <pclass="MsoNormal">Please provide you opinion, based on which I shall prepare new patch and share thesame.<p class="MsoNormal"> <p class="MsoNormal">Thanks and Regards,<p class="MsoNormal">Kumar Rajeev Rastogi<p class="MsoNormal"><spanstyle="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span></div>
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
Rajeev rastogi <rajeev.rastogi@huawei.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
usedwith 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
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 failure modes
* include purely client-side problems; check the transaction status for the
--- 628,637 ----
* 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 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
*************** StoreQueryTuple(const PGresult *result)
*** 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))
{
*************** ProcessResult(PGresult **results)
*** 688,722 ****
* Marshal the COPY data. Either subroutine will get the
* connection out of its COPY state, then call PQresultStatus()
* once and report any error.
*/
SetCancelConn();
if (result_status == PGRES_COPY_OUT)
! success = handleCopyOut(pset.db, pset.queryFout) && success;
else
! success = handleCopyIn(pset.db, pset.cur_cmd_source,
! 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())
--- 687,742 ----
* Marshal the COPY data. Either subroutine will get the
* connection out of its COPY state, then call PQresultStatus()
* once and report any error.
+ *
+ * If pset.copyStream is set, use that as data source/sink,
+ * otherwise use queryFout or cur_cmd_source as appropriate.
*/
+ FILE *copystream = pset.copyStream;
+ PGresult *copy_result;
+
SetCancelConn();
if (result_status == PGRES_COPY_OUT)
! {
! if (!copystream)
! copystream = pset.queryFout;
! success = handleCopyOut(pset.db,
! copystream,
! ©_result) && success;
! }
else
! {
! if (!copystream)
! 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 --git a/src/bin/psql/copy.c b/src/bin/psql/copy.c
index 9e815b1..d706206 100644
*** a/src/bin/psql/copy.c
--- b/src/bin/psql/copy.c
*************** do_copy(const char *args)
*** 269,279 ****
{
PQExpBufferData query;
FILE *copystream;
- FILE *save_file;
- FILE **override_file;
struct copy_options *options;
bool success;
- struct stat st;
/* parse options */
options = parse_slash_copy(args);
--- 269,276 ----
*************** do_copy(const char *args)
*** 287,294 ****
if (options->from)
{
- override_file = &pset.cur_cmd_source;
-
if (options->file)
{
if (options->program)
--- 284,289 ----
*************** do_copy(const char *args)
*** 308,315 ****
}
else
{
- override_file = &pset.queryFout;
-
if (options->file)
{
if (options->program)
--- 303,308 ----
*************** do_copy(const char *args)
*** 345,350 ****
--- 338,344 ----
if (!options->program)
{
+ struct stat st;
int result;
/* make sure the specified file is not a directory */
*************** do_copy(const char *args)
*** 375,385 ****
if (options->after_tofrom)
appendPQExpBufferStr(&query, options->after_tofrom);
! /* Run it like a user command, interposing the data source or sink. */
! save_file = *override_file;
! *override_file = copystream;
success = SendQuery(query.data);
! *override_file = save_file;
termPQExpBuffer(&query);
if (options->file != NULL)
--- 369,378 ----
if (options->after_tofrom)
appendPQExpBufferStr(&query, options->after_tofrom);
! /* run it like a user command, but with copystream as data source/sink */
! pset.copyStream = copystream;
success = SendQuery(query.data);
! pset.copyStream = NULL;
termPQExpBuffer(&query);
if (options->file != NULL)
*************** do_copy(const char *args)
*** 436,451 ****
* 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 = true;
char *buf;
int ret;
- PGresult *res;
for (;;)
{
--- 429,445 ----
* 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.
+ * The final status for the COPY is returned into *res (but note
+ * we already reported the error, if it's not a success result).
*
* result is true if successful, false if not.
*/
bool
! handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res)
{
bool OK = true;
char *buf;
int ret;
for (;;)
{
*************** handleCopyOut(PGconn *conn, FILE *copyst
*** 492,504 ****
* but hasn't exited COPY_OUT state internally. So we ignore the
* possibility here.
*/
! res = PQgetResult(conn);
! if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
psql_error("%s", PQerrorMessage(conn));
OK = false;
}
- PQclear(res);
return OK;
}
--- 486,497 ----
* but hasn't exited COPY_OUT state internally. So we ignore the
* possibility here.
*/
! *res = PQgetResult(conn);
! if (PQresultStatus(*res) != PGRES_COMMAND_OK)
{
psql_error("%s", PQerrorMessage(conn));
OK = false;
}
return OK;
}
*************** handleCopyOut(PGconn *conn, FILE *copyst
*** 511,516 ****
--- 504,511 ----
* and got back a PGRES_COPY_IN result.
* copystream is the file stream to read the data from.
* isbinary can be set from PQbinaryTuples().
+ * The final status for the COPY is returned into *res (but note
+ * we already reported the error, if it's not a success result).
*
* result is true if successful, false if not.
*/
*************** handleCopyOut(PGconn *conn, FILE *copyst
*** 519,530 ****
#define COPYBUFSIZ 8192
bool
! handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary)
{
bool OK;
const char *prompt;
char buf[COPYBUFSIZ];
- PGresult *res;
/*
* Establish longjmp destination for exiting from wait-for-input. (This is
--- 514,524 ----
#define COPYBUFSIZ 8192
bool
! handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res)
{
bool OK;
const char *prompt;
char buf[COPYBUFSIZ];
/*
* Establish longjmp destination for exiting from wait-for-input. (This is
*************** copyin_cleanup:
*** 686,706 ****
* connection is lost. But that's fine; it will get us out of COPY_IN
* state, which is what we need.)
*/
! while (res = PQgetResult(conn), PQresultStatus(res) == PGRES_COPY_IN)
{
OK = false;
! PQclear(res);
/* We can't send an error message if we're using protocol version 2 */
PQputCopyEnd(conn,
(PQprotocolVersion(conn) < 3) ? NULL :
_("trying to exit copy mode"));
}
! if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
psql_error("%s", PQerrorMessage(conn));
OK = false;
}
- PQclear(res);
return OK;
}
--- 680,699 ----
* connection is lost. But that's fine; it will get us out of COPY_IN
* state, which is what we need.)
*/
! while (*res = PQgetResult(conn), PQresultStatus(*res) == PGRES_COPY_IN)
{
OK = false;
! PQclear(*res);
/* We can't send an error message if we're using protocol version 2 */
PQputCopyEnd(conn,
(PQprotocolVersion(conn) < 3) ? NULL :
_("trying to exit copy mode"));
}
! if (PQresultStatus(*res) != PGRES_COMMAND_OK)
{
psql_error("%s", PQerrorMessage(conn));
OK = false;
}
return OK;
}
diff --git a/src/bin/psql/copy.h b/src/bin/psql/copy.h
index ec1f0d0..2c71da0 100644
*** a/src/bin/psql/copy.h
--- b/src/bin/psql/copy.h
***************
*** 12,22 ****
/* handler for \copy */
! bool do_copy(const char *args);
/* lower level processors for copy in/out streams */
! bool handleCopyOut(PGconn *conn, FILE *copystream);
! bool handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary);
#endif
--- 12,24 ----
/* handler for \copy */
! extern bool do_copy(const char *args);
/* lower level processors for copy in/out streams */
! extern bool handleCopyOut(PGconn *conn, FILE *copystream,
! PGresult **res);
! extern bool handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary,
! PGresult **res);
#endif
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
index 3e8328d..eecffb1 100644
*** a/src/bin/psql/settings.h
--- b/src/bin/psql/settings.h
*************** typedef struct _psqlSettings
*** 70,75 ****
--- 70,77 ----
FILE *queryFout; /* where to send the query results */
bool queryFoutPipe; /* queryFout is from a popen() */
+ FILE *copyStream; /* Stream to read/write for \copy command */
+
printQueryOpt popt;
char *gfname; /* one-shot file output argument for \g */
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index d5f1c0d..45653a1 100644
*** a/src/bin/psql/startup.c
--- b/src/bin/psql/startup.c
*************** main(int argc, char *argv[])
*** 118,123 ****
--- 118,124 ----
pset.encoding = PQenv2encoding();
pset.queryFout = stdout;
pset.queryFoutPipe = false;
+ pset.copyStream = NULL;
pset.cur_cmd_source = stdin;
pset.cur_cmd_interactive = false;
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 = true;
char *buf;
int ret;
- PGresult *res;
for (;;)
{
--- 429,445 ----
* 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.
+ * The final status for the COPY is returned into *res (but note
+ * we already reported the error, if it's not a success result).
*
* result is true if successful, false if not.
*/
bool
! handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res)
{
bool OK = true;
char *buf;
int ret;
for (;;)
{
***************
*** 485,497 ****
* but hasn't exited COPY_OUT state internally. So we ignore the
* possibility here.
*/
! res = PQgetResult(conn);
! if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
psql_error("%s", PQerrorMessage(conn));
OK = false;
}
- PQclear(res);
return OK;
}
--- 486,497 ----
* but hasn't exited COPY_OUT state internally. So we ignore the
* possibility here.
*/
! *res = PQgetResult(conn);
! if (PQresultStatus(*res) != PGRES_COMMAND_OK)
{
psql_error("%s", PQerrorMessage(conn));
OK = false;
}
return OK;
}
***************
*** 504,509 ****
--- 504,511 ----
* and got back a PGRES_COPY_IN result.
* copystream is the file stream to read the data from.
* isbinary can be set from PQbinaryTuples().
+ * The final status for the COPY is returned into *res (but note
+ * we already reported the error, if it's not a success result).
*
* result is true if successful, false if not.
*/
***************
*** 512,523 ****
#define COPYBUFSIZ 8192
bool
! handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary)
{
bool OK;
const char *prompt;
char buf[COPYBUFSIZ];
- PGresult *res;
/*
* Establish longjmp destination for exiting from wait-for-input. (This is
--- 514,524 ----
#define COPYBUFSIZ 8192
bool
! handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res)
{
bool OK;
const char *prompt;
char buf[COPYBUFSIZ];
/*
* Establish longjmp destination for exiting from wait-for-input. (This is
***************
*** 679,699 ****
* connection is lost. But that's fine; it will get us out of COPY_IN
* state, which is what we need.)
*/
! while (res = PQgetResult(conn), PQresultStatus(res) == PGRES_COPY_IN)
{
OK = false;
! PQclear(res);
/* We can't send an error message if we're using protocol version 2 */
PQputCopyEnd(conn,
(PQprotocolVersion(conn) < 3) ? NULL :
_("trying to exit copy mode"));
}
! if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
psql_error("%s", PQerrorMessage(conn));
OK = false;
}
- PQclear(res);
return OK;
}
--- 680,699 ----
* connection is lost. But that's fine; it will get us out of COPY_IN
* state, which is what we need.)
*/
! while (*res = PQgetResult(conn), PQresultStatus(*res) == PGRES_COPY_IN)
{
OK = false;
! PQclear(*res);
/* We can't send an error message if we're using protocol version 2 */
PQputCopyEnd(conn,
(PQprotocolVersion(conn) < 3) ? NULL :
_("trying to exit copy mode"));
}
! if (PQresultStatus(*res) != PGRES_COMMAND_OK)
{
psql_error("%s", PQerrorMessage(conn));
OK = false;
}
return OK;
}
diff -c new/copy.h new-wholepatch/copy.h
*** new/copy.h Mon Mar 10 14:55:49 2014
--- new-wholepatch/copy.h Mon Mar 10 12:49:03 2014
***************
*** 12,22 ****
/* handler for \copy */
! bool do_copy(const char *args);
/* lower level processors for copy in/out streams */
! bool handleCopyOut(PGconn *conn, FILE *copystream);
! bool handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary);
#endif
--- 12,24 ----
/* handler for \copy */
! extern bool do_copy(const char *args);
/* lower level processors for copy in/out streams */
! extern bool handleCopyOut(PGconn *conn, FILE *copystream,
! PGresult **res);
! extern bool handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary,
! PGresult **res);
#endif
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
Rajeev rastogi <rajeev.rastogi@huawei.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
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
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.
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
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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Rajeev rastogi <rajeev.rastogi@huawei.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
David Johnston <polobo@yahoo.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
On Wed, Mar 12, 2014 at 12:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > David Johnston <polobo@yahoo.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
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Mar 12, 2014 at 12:09 PM, Tom Lane <tgl@sss.pgh.pa.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
On 12 March 2014 23:57, Tom Lane Wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Wed, Mar 12, 2014 at 12:09 PM, Tom Lane <tgl@sss.pgh.pa.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
Rajeev rastogi <rajeev.rastogi@huawei.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