COPY table FROM STDIN doesn't show count tag

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.

/messages/by-id/CE85A517.4878E%25tim.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

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

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.

/messages/by-id/CE85A517.4878E%25tim.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

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

/messages/by-id/CE85A517.4878E%25tim.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 :

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.

On 18 November 2013, Amit Khandekar wrote:

On 18 October 2013 17:07, Rajeev rastogi <rajeev.rastogi@huawei.com<mailto:rajeev.rastogi@huawei.com>> wrote:
From the following mail, copy behaviour between stdin and normal file having some inconsistency.
/messages/by-id/CE85A517.4878E%25tim.kane@gmail.com&lt;http://www.postgresql.org/message-id/CE85A517.4878E%25tim.kane@gmail.com&gt;
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 18 November 2013 18:00, Rajeev rastogi <rajeev.rastogi@huawei.com> wrote:

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.

/messages/by-id/CE85A517.4878E%25tim.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 meant COPY with a slash. \COPY is equivalent to COPY FROM STDIN. So the
issue can also be reproduced by :
\COPY tab from 'client_filename' ...

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

Thanks. The COPY FROM looks good.

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

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

$ cat /tmp/fout
ee 909
COPY 1

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

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

On 19 November 2013, Amit Khandekar wrote:

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

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

On 19 November 2013, Amit Khandekar wrote:

On 18 November 2013 18:00, Rajeev rastogi <rajeev.rastogi@huawei.com>

wrote:

On 18 November 2013, Amit Khandekar wrote:

Please find the patch for the same and let me know your suggestions.

In this call :

success = handleCopyIn(pset.db,

pset.cur_cmd_source,

PQbinaryTuples(*results), &intres) && success;

if (success && intres)

success =

PrintQueryResults(intres);

Instead of handling of the result status this way, what if we use the

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

Thank you for valuable comments. Your suggestion is absolutely correct.

psql -d postgres -c "\copy tab from '/tmp/st.sql' delimiter ' ';

insert into tab values ('lll', 3)"

COPY 1

INSERT 0 1

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

I hope you meant to write test case as *psql -d postgres -c "\copy tab

from stdin; insert into tab values ('lll', 3)", *as if we are reading
from file, then the above issue does not come.

I meant COPY with a slash. \COPY is equivalent to COPY FROM STDIN. So the

issue can also be reproduced by :

\COPY tab from 'client_filename' ...

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

this mail.

Thanks. The COPY FROM looks good.

OK..Thanks

With the patch applied, \COPY TO 'data_file' command outputs the COPY

status into the data file, instead of printing it in the psql session.

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

postgres=#

$ cat /tmp/fout

ee 909

COPY 1

This is probably because client-side COPY overrides the pset.queryFout

with its own destination file, and while printing the COPY status,
the overridden file pointer is not yet reverted back.

This looks to be an issue without our new patch also. Like I tried
following command and output was as follows:

rajeev@linux-ltr9:~/9.4gitcode/install/bin> ./psql -d postgres -c "\copy
tbl to 'new.txt';insert into tbl values(55);"

rajeev@linux-ltr9:~/9.4gitcode/install/bin> cat new.txt

5

67

5

67

2

2

99

1

1

INSERT 0 1

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

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

! pset.queryFout = stdout;

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

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

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

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

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

On 20 November, Amit Khandekar wrote:

I hope you meant to write test case as psql -d postgres -c "\copy tab from stdin; insert into tab values ('lll', 3)", as if we are reading from file, then the above issue does not come.

I meant COPY with a slash. \COPY is equivalent to COPY FROM STDIN. So the issue can also be reproduced by :
\COPY tab from 'client_filename' ...

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

Thanks. The COPY FROM looks good.

OK..Thanks

With the patch applied, \COPY TO 'data_file' command outputs the COPY status into the data file, instead of printing it in the psql session.
postgres=# \copy tab to '/tmp/fout';
postgres=#
$ cat /tmp/fout
ee 909
COPY 1
This is probably because client-side COPY overrides the pset.queryFout with its own destination file, and while printing the COPY status, the overridden file pointer is not yet reverted back.

This looks to be an issue without our new patch also. Like I tried following command and output was as follows:
rajeev@linux-ltr9:~/9.4gitcode/install/bin<mailto: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<mailto:rajeev@linux-ltr9:~/9.4gitcode/install/bin>> cat new.txt
5
67
5
67
2
2
99
1
1

INSERT 0 1

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

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

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

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

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

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

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

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

Thanks and Regards,
Kumar Rajeev Rastogi

On 20 November 2013 17:40, Rajeev rastogi <rajeev.rastogi@huawei.com> wrote:

You mean to say that I should change the patch to keep only COPY FROM
related changes and remove changes related to COPY TO.

If yes, then I shall change the patch accordingly and also mention same
in documentation also.

Please let me know about this so that I can share the modified patch.

RIght.

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

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

On 20 November 2013 18:11, Robert Haas <robertmhaas@gmail.com> wrote:

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.

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

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

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

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

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

On 21 November 2013 10:13, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

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

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

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

On 21 November 2013, Amit Khandekar <amit.khandekar@enterprisedb.com<mailto: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 22 November 2013 16:14, Rajeev rastogi <rajeev.rastogi@huawei.com> wrote:

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.

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.

On 25 November 2013, Amit Khandekar <amit.khandekar@enterprisedb.com<mailto: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

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.

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"

+                               /*
+                                * 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.

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

On 27 November 2013 09:59, Rajeev rastogi <rajeev.rastogi@huawei.com> wrote:

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.

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.