COPY table FROM STDIN doesn't show count tag

Started by Rajeev rastogiabout 12 years ago37 messages
#1Rajeev rastogi
rajeev.rastogi@huawei.com
1 attachment(s)

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

Attachments:

copydefect.patchapplication/octet-stream; name=copydefect.patchDownload
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 71f0c8a..925bb72 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -29,6 +29,7 @@
 static bool ExecQueryUsingCursor(const char *query, double *elapsed_msec);
 static bool command_no_begin(const char *query);
 static bool is_select_command(const char *query);
+static bool PrintQueryResults(PGresult *results);
 
 /*
  * setQFout
@@ -644,6 +645,7 @@ ProcessResult(PGresult **results)
 	PGresult   *next_result;
 	bool		success = true;
 	bool		first_cycle = true;
+	PGresult	*intres;
 
 	do
 	{
@@ -691,10 +693,16 @@ ProcessResult(PGresult **results)
 			 */
 			SetCancelConn();
 			if (result_status == PGRES_COPY_OUT)
-				success = handleCopyOut(pset.db, pset.queryFout) && success;
+				success = handleCopyOut(pset.db, pset.queryFout, &intres) && success;
 			else
 				success = handleCopyIn(pset.db, pset.cur_cmd_source,
-									   PQbinaryTuples(*results)) && success;
+									   PQbinaryTuples(*results), &intres) && success;
+
+			if (success && intres)
+				success = PrintQueryResults(intres);
+			
+			PQclear(intres);
+
 			ResetCancelConn();
 
 			/*
diff --git a/src/bin/psql/copy.c b/src/bin/psql/copy.c
index 6db063c..347b866 100644
--- a/src/bin/psql/copy.c
+++ b/src/bin/psql/copy.c
@@ -433,12 +433,11 @@ do_copy(const char *args)
  * result is true if successful, false if not.
  */
 bool
-handleCopyOut(PGconn *conn, FILE *copystream)
+handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res)
 {
 	bool		OK = true;
 	char	   *buf;
 	int			ret;
-	PGresult   *res;
 
 	for (;;)
 	{
@@ -490,19 +489,18 @@ handleCopyOut(PGconn *conn, FILE *copystream)
 	 * TO STDOUT commands.	We trust that no condition can make PQexec() fail
 	 * indefinitely while retaining status PGRES_COPY_OUT.
 	 */
-	while (res = PQgetResult(conn), PQresultStatus(res) == PGRES_COPY_OUT)
+	while (*res = PQgetResult(conn), PQresultStatus(*res) == PGRES_COPY_OUT)
 	{
 		OK = false;
-		PQclear(res);
+		PQclear(*res);
 
 		PQexec(conn, "-- clear PGRES_COPY_OUT state");
 	}
-	if (PQresultStatus(res) != PGRES_COMMAND_OK)
+	if (PQresultStatus(*res) != PGRES_COMMAND_OK)
 	{
 		psql_error("%s", PQerrorMessage(conn));
 		OK = false;
 	}
-	PQclear(res);
 
 	return OK;
 }
@@ -523,12 +521,11 @@ handleCopyOut(PGconn *conn, FILE *copystream)
 #define COPYBUFSIZ 8192
 
 bool
-handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary)
+handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res)
 {
 	bool		OK;
 	const char *prompt;
 	char		buf[COPYBUFSIZ];
-	PGresult   *res;
 
 	/*
 	 * Establish longjmp destination for exiting from wait-for-input. (This is
@@ -682,19 +679,18 @@ copyin_cleanup:
 	 * indefinitely while retaining status PGRES_COPY_IN, we get an infinite
 	 * loop.  This is more realistic than handleCopyOut()'s counterpart risk.
 	 */
-	while (res = PQgetResult(conn), PQresultStatus(res) == PGRES_COPY_IN)
+	while (*res = PQgetResult(conn), PQresultStatus(*res) == PGRES_COPY_IN)
 	{
 		OK = false;
-		PQclear(res);
+		PQclear(*res);
 
 		PQputCopyEnd(pset.db, _("trying to exit copy mode"));
 	}
-	if (PQresultStatus(res) != PGRES_COMMAND_OK)
+	if (PQresultStatus(*res) != PGRES_COMMAND_OK)
 	{
 		psql_error("%s", PQerrorMessage(conn));
 		OK = false;
 	}
-	PQclear(res);
 
 	return OK;
 }
diff --git a/src/bin/psql/copy.h b/src/bin/psql/copy.h
index dfc61ee..99e923a 100644
--- a/src/bin/psql/copy.h
+++ b/src/bin/psql/copy.h
@@ -16,7 +16,7 @@ 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);
+bool		handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res);
+bool		handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res);
 
 #endif
#2Robert Haas
robertmhaas@gmail.com
In reply to: Rajeev rastogi (#1)
Re: 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

#3Rajeev rastogi
rajeev.rastogi@huawei.com
In reply to: Robert Haas (#2)
Re: COPY table FROM STDIN doesn't show count tag

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

#4Amit Khandekar
amit.khandekar@enterprisedb.com
In reply to: Rajeev rastogi (#1)
Re: COPY table FROM STDIN doesn't show count tag

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.

Show quoted text

*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

#5Rajeev rastogi
rajeev.rastogi@huawei.com
In reply to: Amit Khandekar (#4)
1 attachment(s)
Re: COPY table FROM STDIN doesn't show count tag

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

Attachments:

copydefectV2.patchapplication/octet-stream; name=copydefectV2.patchDownload
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
***************
*** 691,709 **** ProcessResult(PGresult **results)
  			 */
  			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 */
--- 691,713 ----
  			 */
  			SetCancelConn();
  			if (result_status == PGRES_COPY_OUT)
! 				success = handleCopyOut(pset.db, pset.queryFout, results) && success;
  			else
  				success = handleCopyIn(pset.db, pset.cur_cmd_source,
! 									   PQbinaryTuples(*results), 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.
+ 			 * If it is NULL, then the last result will be returned back.
  			 */
! 			if ((next_result = PQgetResult(pset.db)))
! 			{
! 				PQclear(*results);
! 				*results = next_result;
! 			}
  		}
  		else if (first_cycle)
  			/* fast path: no COPY commands; PQexec visited all results */
*** a/src/bin/psql/copy.c
--- b/src/bin/psql/copy.c
***************
*** 433,444 **** do_copy(const char *args)
   * result is true if successful, false if not.
   */
  bool
! handleCopyOut(PGconn *conn, FILE *copystream)
  {
  	bool		OK = true;
  	char	   *buf;
  	int			ret;
- 	PGresult   *res;
  
  	for (;;)
  	{
--- 433,443 ----
   * result is true if successful, false if not.
   */
  bool
! handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res)
  {
  	bool		OK = true;
  	char	   *buf;
  	int			ret;
  
  	for (;;)
  	{
***************
*** 490,508 **** handleCopyOut(PGconn *conn, FILE *copystream)
  	 * TO STDOUT commands.	We trust that no condition can make PQexec() fail
  	 * indefinitely while retaining status PGRES_COPY_OUT.
  	 */
! 	while (res = PQgetResult(conn), PQresultStatus(res) == PGRES_COPY_OUT)
  	{
  		OK = false;
! 		PQclear(res);
  
  		PQexec(conn, "-- clear PGRES_COPY_OUT state");
  	}
! 	if (PQresultStatus(res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
- 	PQclear(res);
  
  	return OK;
  }
--- 489,506 ----
  	 * TO STDOUT commands.	We trust that no condition can make PQexec() fail
  	 * indefinitely while retaining status PGRES_COPY_OUT.
  	 */
! 	while (*res = PQgetResult(conn), PQresultStatus(*res) == PGRES_COPY_OUT)
  	{
  		OK = false;
! 		PQclear(*res);
  
  		PQexec(conn, "-- clear PGRES_COPY_OUT state");
  	}
! 	if (PQresultStatus(*res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
  
  	return OK;
  }
***************
*** 523,534 **** handleCopyOut(PGconn *conn, FILE *copystream)
  #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
--- 521,531 ----
  #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
***************
*** 682,700 **** copyin_cleanup:
  	 * indefinitely while retaining status PGRES_COPY_IN, we get an infinite
  	 * loop.  This is more realistic than handleCopyOut()'s counterpart risk.
  	 */
! 	while (res = PQgetResult(conn), PQresultStatus(res) == PGRES_COPY_IN)
  	{
  		OK = false;
! 		PQclear(res);
  
  		PQputCopyEnd(pset.db, _("trying to exit copy mode"));
  	}
! 	if (PQresultStatus(res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
- 	PQclear(res);
  
  	return OK;
  }
--- 679,696 ----
  	 * indefinitely while retaining status PGRES_COPY_IN, we get an infinite
  	 * loop.  This is more realistic than handleCopyOut()'s counterpart risk.
  	 */
! 	while (*res = PQgetResult(conn), PQresultStatus(*res) == PGRES_COPY_IN)
  	{
  		OK = false;
! 		PQclear(*res);
  
  		PQputCopyEnd(pset.db, _("trying to exit copy mode"));
  	}
! 	if (PQresultStatus(*res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
  
  	return OK;
  }
*** a/src/bin/psql/copy.h
--- b/src/bin/psql/copy.h
***************
*** 16,22 **** 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
--- 16,22 ----
  
  /* lower level processors for copy in/out streams */
  
! bool		handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res);
! bool		handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res);
  
  #endif
#6Amit Khandekar
amit.khandekar@enterprisedb.com
In reply to: Rajeev rastogi (#5)
Re: COPY table FROM STDIN doesn't show count tag

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.

Show quoted text

*Thanks and Regards,*

*Kumar Rajeev Rastogi *

#7Rajeev rastogi
rajeev.rastogi@huawei.com
In reply to: Amit Khandekar (#6)
1 attachment(s)
Re: COPY table FROM STDIN doesn't show count tag

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

Attachments:

copydefectV3.patchapplication/octet-stream; name=copydefectV3.patchDownload
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
***************
*** 691,709 **** ProcessResult(PGresult **results)
  			 */
  			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 */
--- 691,723 ----
  			 */
  			SetCancelConn();
  			if (result_status == PGRES_COPY_OUT)
! 			{
! 				success = handleCopyOut(pset.db, pset.queryFout, results) && success;
! 
! 				/* 
! 				 * May be query was \copy tbl to 'filename'; in that case queryFout will be 
! 				 * pointing to the file filename descriptor. So since by this time table  output is
! 				 * already redirected to given file, so now in order to print the status of 
! 				 * command execution queryFout should point to stdout.
! 				 */
! 				pset.queryFout = stdout;
! 			}
  			else
  				success = handleCopyIn(pset.db, pset.cur_cmd_source,
! 									   PQbinaryTuples(*results), 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.
+ 			 * If it is NULL, then the last result will be returned back.
  			 */
! 			if ((next_result = PQgetResult(pset.db)))
! 			{
! 				PQclear(*results);
! 				*results = next_result;
! 			}
  		}
  		else if (first_cycle)
  			/* fast path: no COPY commands; PQexec visited all results */
*** a/src/bin/psql/copy.c
--- b/src/bin/psql/copy.c
***************
*** 433,444 **** do_copy(const char *args)
   * result is true if successful, false if not.
   */
  bool
! handleCopyOut(PGconn *conn, FILE *copystream)
  {
  	bool		OK = true;
  	char	   *buf;
  	int			ret;
- 	PGresult   *res;
  
  	for (;;)
  	{
--- 433,443 ----
   * result is true if successful, false if not.
   */
  bool
! handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res)
  {
  	bool		OK = true;
  	char	   *buf;
  	int			ret;
  
  	for (;;)
  	{
***************
*** 490,508 **** handleCopyOut(PGconn *conn, FILE *copystream)
  	 * TO STDOUT commands.	We trust that no condition can make PQexec() fail
  	 * indefinitely while retaining status PGRES_COPY_OUT.
  	 */
! 	while (res = PQgetResult(conn), PQresultStatus(res) == PGRES_COPY_OUT)
  	{
  		OK = false;
! 		PQclear(res);
  
  		PQexec(conn, "-- clear PGRES_COPY_OUT state");
  	}
! 	if (PQresultStatus(res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
- 	PQclear(res);
  
  	return OK;
  }
--- 489,506 ----
  	 * TO STDOUT commands.	We trust that no condition can make PQexec() fail
  	 * indefinitely while retaining status PGRES_COPY_OUT.
  	 */
! 	while (*res = PQgetResult(conn), PQresultStatus(*res) == PGRES_COPY_OUT)
  	{
  		OK = false;
! 		PQclear(*res);
  
  		PQexec(conn, "-- clear PGRES_COPY_OUT state");
  	}
! 	if (PQresultStatus(*res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
  
  	return OK;
  }
***************
*** 523,534 **** handleCopyOut(PGconn *conn, FILE *copystream)
  #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
--- 521,531 ----
  #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
***************
*** 682,700 **** copyin_cleanup:
  	 * indefinitely while retaining status PGRES_COPY_IN, we get an infinite
  	 * loop.  This is more realistic than handleCopyOut()'s counterpart risk.
  	 */
! 	while (res = PQgetResult(conn), PQresultStatus(res) == PGRES_COPY_IN)
  	{
  		OK = false;
! 		PQclear(res);
  
  		PQputCopyEnd(pset.db, _("trying to exit copy mode"));
  	}
! 	if (PQresultStatus(res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
- 	PQclear(res);
  
  	return OK;
  }
--- 679,696 ----
  	 * indefinitely while retaining status PGRES_COPY_IN, we get an infinite
  	 * loop.  This is more realistic than handleCopyOut()'s counterpart risk.
  	 */
! 	while (*res = PQgetResult(conn), PQresultStatus(*res) == PGRES_COPY_IN)
  	{
  		OK = false;
! 		PQclear(*res);
  
  		PQputCopyEnd(pset.db, _("trying to exit copy mode"));
  	}
! 	if (PQresultStatus(*res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
  
  	return OK;
  }
*** a/src/bin/psql/copy.h
--- b/src/bin/psql/copy.h
***************
*** 16,22 **** 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
--- 16,22 ----
  
  /* lower level processors for copy in/out streams */
  
! bool		handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res);
! bool		handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res);
  
  #endif
#8Amit Khandekar
amit.khandekar@enterprisedb.com
In reply to: Rajeev rastogi (#7)
Re: COPY table FROM STDIN doesn't show count tag

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.

Show quoted text

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

Thanks and Regards,

Kumar Rajeev Rastogi

#9Rajeev rastogi
rajeev.rastogi@huawei.com
In reply to: Amit Khandekar (#8)
Re: COPY table FROM STDIN doesn't show count tag

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

#10Amit Khandekar
amit.khandekar@enterprisedb.com
In reply to: Rajeev rastogi (#9)
Re: COPY table FROM STDIN doesn't show count tag

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.

Show quoted text

Thanks and Regards,

Kumar Rajeev Rastogi

#11Robert Haas
robertmhaas@gmail.com
In reply to: Amit Khandekar (#8)
Re: COPY table FROM STDIN doesn't show count tag

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

#12Amit Khandekar
amit.khandekar@enterprisedb.com
In reply to: Robert Haas (#11)
Re: COPY table FROM STDIN doesn't show count tag

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.

Show quoted text

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

#13Tom Lane
tgl@sss.pgh.pa.us
In reply to: Amit Khandekar (#12)
Re: COPY table FROM STDIN doesn't show count tag

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

#14Amit Khandekar
amit.khandekar@enterprisedb.com
In reply to: Tom Lane (#13)
Re: COPY table FROM STDIN doesn't show count tag

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

Show quoted text

regards, tom lane

#15Rajeev rastogi
rajeev.rastogi@huawei.com
In reply to: Amit Khandekar (#14)
Re: COPY table FROM STDIN doesn't show count tag

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

#16Amit Khandekar
amit.khandekar@enterprisedb.com
In reply to: Rajeev rastogi (#15)
Re: COPY table FROM STDIN doesn't show count tag

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.

Show quoted text

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

#17Rajeev rastogi
rajeev.rastogi@huawei.com
In reply to: Amit Khandekar (#16)
1 attachment(s)
Re: COPY table FROM STDIN doesn't show count tag

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

Attachments:

copyerrorV4.patchapplication/octet-stream; name=copyerrorV4.patchDownload
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
***************
*** 672,677 **** ProcessResult(PGresult **results)
--- 672,685 ----
  
  			case PGRES_COPY_OUT:
  			case PGRES_COPY_IN:
+ 				/*
+ 				 * 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;
  				is_copy = true;
  				break;
  
***************
*** 691,709 **** ProcessResult(PGresult **results)
  			 */
  			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 */
--- 699,721 ----
  			 */
  			SetCancelConn();
  			if (result_status == PGRES_COPY_OUT)
! 				success = handleCopyOut(pset.db, pset.copyStream, results) && success;
  			else
! 				success = handleCopyIn(pset.db, pset.copyStream,
! 									   PQbinaryTuples(*results), 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.
+ 			 * If it is NULL, then the last result will be returned back.
  			 */
! 			if ((next_result = PQgetResult(pset.db)))
! 			{
! 				PQclear(*results);
! 				*results = next_result;
! 			}
  		}
  		else if (first_cycle)
  			/* fast path: no COPY commands; PQexec visited all results */
*** a/src/bin/psql/copy.c
--- b/src/bin/psql/copy.c
***************
*** 269,276 **** do_copy(const char *args)
  {
  	PQExpBufferData query;
  	FILE	   *copystream;
- 	FILE	   *save_file;
- 	FILE	  **override_file;
  	struct copy_options *options;
  	bool		success;
  	struct stat st;
--- 269,274 ----
***************
*** 287,294 **** do_copy(const char *args)
  
  	if (options->from)
  	{
- 		override_file = &pset.cur_cmd_source;
- 
  		if (options->file)
  		{
  			if (options->program)
--- 285,290 ----
***************
*** 308,315 **** do_copy(const char *args)
  	}
  	else
  	{
- 		override_file = &pset.queryFout;
- 
  		if (options->file)
  		{
  			if (options->program)
--- 304,309 ----
***************
*** 369,378 **** do_copy(const char *args)
  		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)
--- 363,371 ----
  		appendPQExpBufferStr(&query, options->after_tofrom);
  
  	/* Run it like a user command, interposing the data source or sink. */
! 	pset.copyStream = copystream;
  	success = SendQuery(query.data);
! 	pset.copyStream = NULL;
  	termPQExpBuffer(&query);
  
  	if (options->file != NULL)
***************
*** 433,444 **** do_copy(const char *args)
   * result is true if successful, false if not.
   */
  bool
! handleCopyOut(PGconn *conn, FILE *copystream)
  {
  	bool		OK = true;
  	char	   *buf;
  	int			ret;
! 	PGresult   *res;
  
  	for (;;)
  	{
--- 426,439 ----
   * result is true if successful, false if not.
   */
  bool
! handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res)
  {
  	bool		OK = true;
  	char	   *buf;
  	int			ret;
! 
! 	if (!copystream)
! 		copystream = stdout;
  
  	for (;;)
  	{
***************
*** 490,508 **** handleCopyOut(PGconn *conn, FILE *copystream)
  	 * TO STDOUT commands.	We trust that no condition can make PQexec() fail
  	 * indefinitely while retaining status PGRES_COPY_OUT.
  	 */
! 	while (res = PQgetResult(conn), PQresultStatus(res) == PGRES_COPY_OUT)
  	{
  		OK = false;
! 		PQclear(res);
  
  		PQexec(conn, "-- clear PGRES_COPY_OUT state");
  	}
! 	if (PQresultStatus(res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
- 	PQclear(res);
  
  	return OK;
  }
--- 485,502 ----
  	 * TO STDOUT commands.	We trust that no condition can make PQexec() fail
  	 * indefinitely while retaining status PGRES_COPY_OUT.
  	 */
! 	while (*res = PQgetResult(conn), PQresultStatus(*res) == PGRES_COPY_OUT)
  	{
  		OK = false;
! 		PQclear(*res);
  
  		PQexec(conn, "-- clear PGRES_COPY_OUT state");
  	}
! 	if (PQresultStatus(*res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
  
  	return OK;
  }
***************
*** 523,534 **** handleCopyOut(PGconn *conn, FILE *copystream)
  #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
--- 517,531 ----
  #define COPYBUFSIZ 8192
  
  bool
! handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res)
  {
  	bool		OK;
  	const char *prompt;
  	char		buf[COPYBUFSIZ];
! 
! 	if (!copystream)
! 		copystream = stdin;
! 
  
  	/*
  	 * Establish longjmp destination for exiting from wait-for-input. (This is
***************
*** 656,663 **** handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary)
  				}
  			}
  
! 			if (copystream == pset.cur_cmd_source)
! 				pset.lineno++;
  		}
  	}
  
--- 653,659 ----
  				}
  			}
  
! 			pset.lineno++;
  		}
  	}
  
***************
*** 682,700 **** copyin_cleanup:
  	 * indefinitely while retaining status PGRES_COPY_IN, we get an infinite
  	 * loop.  This is more realistic than handleCopyOut()'s counterpart risk.
  	 */
! 	while (res = PQgetResult(conn), PQresultStatus(res) == PGRES_COPY_IN)
  	{
  		OK = false;
! 		PQclear(res);
  
  		PQputCopyEnd(pset.db, _("trying to exit copy mode"));
  	}
! 	if (PQresultStatus(res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
- 	PQclear(res);
  
  	return OK;
  }
--- 678,695 ----
  	 * indefinitely while retaining status PGRES_COPY_IN, we get an infinite
  	 * loop.  This is more realistic than handleCopyOut()'s counterpart risk.
  	 */
! 	while (*res = PQgetResult(conn), PQresultStatus(*res) == PGRES_COPY_IN)
  	{
  		OK = false;
! 		PQclear(*res);
  
  		PQputCopyEnd(pset.db, _("trying to exit copy mode"));
  	}
! 	if (PQresultStatus(*res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
  
  	return OK;
  }
*** a/src/bin/psql/copy.h
--- b/src/bin/psql/copy.h
***************
*** 16,22 **** 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
--- 16,22 ----
  
  /* lower level processors for copy in/out streams */
  
! bool		handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res);
! bool		handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res);
  
  #endif
*** a/src/bin/psql/settings.h
--- b/src/bin/psql/settings.h
***************
*** 70,75 **** typedef struct _psqlSettings
--- 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 */
#18Amit Khandekar
amit.khandekar@enterprisedb.com
In reply to: Rajeev rastogi (#17)
Re: COPY table FROM STDIN doesn't show count tag

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.

Show quoted text

Please provide your opinion.

Thanks and Regards,

Kumar Rajeev Rastogi

#19Rajeev rastogi
rajeev.rastogi@huawei.com
In reply to: Amit Khandekar (#18)
1 attachment(s)
Re: COPY table FROM STDIN doesn't show count tag

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

Attachments:

copyerrorV5.patchapplication/octet-stream; name=copyerrorV5.patchDownload
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
***************
*** 689,709 **** ProcessResult(PGresult **results)
  			 * 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 */
--- 689,720 ----
  			 * connection out of its COPY state, then call PQresultStatus()
  			 * once and report any error.
  			 */
+ 			/*
+ 			 * If this is second copy; then it will be definately not \copy,
+ 			 * and also it can not be from any user given file.
+ 			 * So pass the value of copystream as NULL, so that read/wrie
+ 			 * happens on already set cur_cmd_source/queryFout.
+ 			 */
  			SetCancelConn();
  			if (result_status == PGRES_COPY_OUT)
! 				success = handleCopyOut(pset.db, (first_cycle ? pset.copyStream : NULL),
! 														results) && success;
  			else
! 				success = handleCopyIn(pset.db, (first_cycle ? pset.copyStream : NULL),
! 									   PQbinaryTuples(*results), 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.
+ 			 * If it is NULL, then the last result will be returned back.
  			 */
! 			if ((next_result = PQgetResult(pset.db)))
! 			{
! 				PQclear(*results);
! 				*results = next_result;
! 			}
  		}
  		else if (first_cycle)
  			/* fast path: no COPY commands; PQexec visited all results */
*** a/src/bin/psql/copy.c
--- b/src/bin/psql/copy.c
***************
*** 269,276 **** do_copy(const char *args)
  {
  	PQExpBufferData query;
  	FILE	   *copystream;
- 	FILE	   *save_file;
- 	FILE	  **override_file;
  	struct copy_options *options;
  	bool		success;
  	struct stat st;
--- 269,274 ----
***************
*** 287,294 **** do_copy(const char *args)
  
  	if (options->from)
  	{
- 		override_file = &pset.cur_cmd_source;
- 
  		if (options->file)
  		{
  			if (options->program)
--- 285,290 ----
***************
*** 308,315 **** do_copy(const char *args)
  	}
  	else
  	{
- 		override_file = &pset.queryFout;
- 
  		if (options->file)
  		{
  			if (options->program)
--- 304,309 ----
***************
*** 369,378 **** do_copy(const char *args)
  		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)
--- 363,371 ----
  		appendPQExpBufferStr(&query, options->after_tofrom);
  
  	/* Run it like a user command, interposing the data source or sink. */
! 	pset.copyStream = copystream;
  	success = SendQuery(query.data);
! 	pset.copyStream = NULL;
  	termPQExpBuffer(&query);
  
  	if (options->file != NULL)
***************
*** 433,444 **** do_copy(const char *args)
   * result is true if successful, false if not.
   */
  bool
! handleCopyOut(PGconn *conn, FILE *copystream)
  {
  	bool		OK = true;
  	char	   *buf;
  	int			ret;
! 	PGresult   *res;
  
  	for (;;)
  	{
--- 426,439 ----
   * result is true if successful, false if not.
   */
  bool
! handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res)
  {
  	bool		OK = true;
  	char	   *buf;
  	int			ret;
! 
! 	if (!copystream)
! 		copystream = pset.queryFout;
  
  	for (;;)
  	{
***************
*** 490,508 **** handleCopyOut(PGconn *conn, FILE *copystream)
  	 * TO STDOUT commands.	We trust that no condition can make PQexec() fail
  	 * indefinitely while retaining status PGRES_COPY_OUT.
  	 */
! 	while (res = PQgetResult(conn), PQresultStatus(res) == PGRES_COPY_OUT)
  	{
  		OK = false;
! 		PQclear(res);
  
  		PQexec(conn, "-- clear PGRES_COPY_OUT state");
  	}
! 	if (PQresultStatus(res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
- 	PQclear(res);
  
  	return OK;
  }
--- 485,502 ----
  	 * TO STDOUT commands.	We trust that no condition can make PQexec() fail
  	 * indefinitely while retaining status PGRES_COPY_OUT.
  	 */
! 	while (*res = PQgetResult(conn), PQresultStatus(*res) == PGRES_COPY_OUT)
  	{
  		OK = false;
! 		PQclear(*res);
  
  		PQexec(conn, "-- clear PGRES_COPY_OUT state");
  	}
! 	if (PQresultStatus(*res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
  
  	return OK;
  }
***************
*** 523,534 **** handleCopyOut(PGconn *conn, FILE *copystream)
  #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
--- 517,531 ----
  #define COPYBUFSIZ 8192
  
  bool
! handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res)
  {
  	bool		OK;
  	const char *prompt;
  	char		buf[COPYBUFSIZ];
! 
! 	if (!copystream)
! 		copystream = pset.cur_cmd_source;
! 
  
  	/*
  	 * Establish longjmp destination for exiting from wait-for-input. (This is
***************
*** 656,663 **** handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary)
  				}
  			}
  
! 			if (copystream == pset.cur_cmd_source)
! 				pset.lineno++;
  		}
  	}
  
--- 653,659 ----
  				}
  			}
  
! 			pset.lineno++;
  		}
  	}
  
***************
*** 682,700 **** copyin_cleanup:
  	 * indefinitely while retaining status PGRES_COPY_IN, we get an infinite
  	 * loop.  This is more realistic than handleCopyOut()'s counterpart risk.
  	 */
! 	while (res = PQgetResult(conn), PQresultStatus(res) == PGRES_COPY_IN)
  	{
  		OK = false;
! 		PQclear(res);
  
  		PQputCopyEnd(pset.db, _("trying to exit copy mode"));
  	}
! 	if (PQresultStatus(res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
- 	PQclear(res);
  
  	return OK;
  }
--- 678,695 ----
  	 * indefinitely while retaining status PGRES_COPY_IN, we get an infinite
  	 * loop.  This is more realistic than handleCopyOut()'s counterpart risk.
  	 */
! 	while (*res = PQgetResult(conn), PQresultStatus(*res) == PGRES_COPY_IN)
  	{
  		OK = false;
! 		PQclear(*res);
  
  		PQputCopyEnd(pset.db, _("trying to exit copy mode"));
  	}
! 	if (PQresultStatus(*res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
  
  	return OK;
  }
*** a/src/bin/psql/copy.h
--- b/src/bin/psql/copy.h
***************
*** 16,22 **** 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
--- 16,22 ----
  
  /* lower level processors for copy in/out streams */
  
! bool		handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res);
! bool		handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res);
  
  #endif
*** a/src/bin/psql/settings.h
--- b/src/bin/psql/settings.h
***************
*** 70,75 **** typedef struct _psqlSettings
--- 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 */
#20Amit Khandekar
amit.khandekar@enterprisedb.com
In reply to: Rajeev rastogi (#19)
Re: COPY table FROM STDIN doesn't show count tag

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.

#21Rajeev rastogi
rajeev.rastogi@huawei.com
In reply to: Amit Khandekar (#20)
2 attachment(s)
Re: COPY table FROM STDIN doesn't show count tag

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

Attachments:

slashcopyissuev1.patchapplication/octet-stream; name=slashcopyissuev1.patchDownload
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
***************
*** 689,699 **** ProcessResult(PGresult **results)
  			 * 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();
  
--- 689,706 ----
  			 * connection out of its COPY state, then call PQresultStatus()
  			 * once and report any error.
  			 */
+ 
+ 			/*
+ 			 * If this is second copy; then it will be definately not \copy,
+ 			 * and also it can not be from any user given file. So pass the
+ 			 * value of copystream as NULL, so that read/wrie happens on
+ 			 * already set cur_cmd_source/queryFout.
+ 			 */
  			SetCancelConn();
  			if (result_status == PGRES_COPY_OUT)
! 				success = handleCopyOut(pset.db, (first_cycle ? pset.copyStream : NULL)) && success;
  			else
! 				success = handleCopyIn(pset.db, (first_cycle ? pset.copyStream : NULL),
  									   PQbinaryTuples(*results)) && success;
  			ResetCancelConn();
  
*** a/src/bin/psql/copy.c
--- b/src/bin/psql/copy.c
***************
*** 269,276 **** do_copy(const char *args)
  {
  	PQExpBufferData query;
  	FILE	   *copystream;
- 	FILE	   *save_file;
- 	FILE	  **override_file;
  	struct copy_options *options;
  	bool		success;
  	struct stat st;
--- 269,274 ----
***************
*** 287,294 **** do_copy(const char *args)
  
  	if (options->from)
  	{
- 		override_file = &pset.cur_cmd_source;
- 
  		if (options->file)
  		{
  			if (options->program)
--- 285,290 ----
***************
*** 308,315 **** do_copy(const char *args)
  	}
  	else
  	{
- 		override_file = &pset.queryFout;
- 
  		if (options->file)
  		{
  			if (options->program)
--- 304,309 ----
***************
*** 369,378 **** do_copy(const char *args)
  		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)
--- 363,371 ----
  		appendPQExpBufferStr(&query, options->after_tofrom);
  
  	/* Run it like a user command, interposing the data source or sink. */
! 	pset.copyStream = copystream;
  	success = SendQuery(query.data);
! 	pset.copyStream = NULL;
  	termPQExpBuffer(&query);
  
  	if (options->file != NULL)
***************
*** 440,445 **** handleCopyOut(PGconn *conn, FILE *copystream)
--- 433,441 ----
  	int			ret;
  	PGresult   *res;
  
+ 	if (!copystream)
+ 		copystream = pset.queryFout;
+ 
  	for (;;)
  	{
  		ret = PQgetCopyData(conn, &buf, 0);
***************
*** 530,535 **** handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary)
--- 526,534 ----
  	char		buf[COPYBUFSIZ];
  	PGresult   *res;
  
+ 	if (!copystream)
+ 		copystream = pset.cur_cmd_source;
+ 
  	/*
  	 * Establish longjmp destination for exiting from wait-for-input. (This is
  	 * only effective while sigint_interrupt_enabled is TRUE.)
***************
*** 656,663 **** handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary)
  				}
  			}
  
! 			if (copystream == pset.cur_cmd_source)
! 				pset.lineno++;
  		}
  	}
  
--- 655,661 ----
  				}
  			}
  
! 			pset.lineno++;
  		}
  	}
  
*** a/src/bin/psql/settings.h
--- b/src/bin/psql/settings.h
***************
*** 70,75 **** typedef struct _psqlSettings
--- 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 */
initialcopyissuev1_ontopofslashcopy.patchapplication/octet-stream; name=initialcopyissuev1_ontopofslashcopy.patchDownload
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
***************
*** 698,716 **** ProcessResult(PGresult **results)
  			 */
  			SetCancelConn();
  			if (result_status == PGRES_COPY_OUT)
! 				success = handleCopyOut(pset.db, (first_cycle ? pset.copyStream : NULL)) && success;
  			else
  				success = handleCopyIn(pset.db, (first_cycle ? pset.copyStream : NULL),
! 									   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 */
--- 698,721 ----
  			 */
  			SetCancelConn();
  			if (result_status == PGRES_COPY_OUT)
! 				success = handleCopyOut(pset.db, (first_cycle ? pset.copyStream : NULL),
! 										results) && success;
  			else
  				success = handleCopyIn(pset.db, (first_cycle ? pset.copyStream : NULL),
! 							   PQbinaryTuples(*results), 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. If
! 			 * it is NULL, then the last result will be returned back.
  			 */
! 			if ((next_result = PQgetResult(pset.db)))
! 			{
! 				PQclear(*results);
! 				*results = next_result;
! 			}
  		}
  		else if (first_cycle)
  			/* fast path: no COPY commands; PQexec visited all results */
*** a/src/bin/psql/copy.c
--- b/src/bin/psql/copy.c
***************
*** 426,437 **** do_copy(const char *args)
   * result is true if successful, false if not.
   */
  bool
! handleCopyOut(PGconn *conn, FILE *copystream)
  {
  	bool		OK = true;
  	char	   *buf;
  	int			ret;
- 	PGresult   *res;
  
  	if (!copystream)
  		copystream = pset.queryFout;
--- 426,436 ----
   * result is true if successful, false if not.
   */
  bool
! handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res)
  {
  	bool		OK = true;
  	char	   *buf;
  	int			ret;
  
  	if (!copystream)
  		copystream = pset.queryFout;
***************
*** 486,504 **** handleCopyOut(PGconn *conn, FILE *copystream)
  	 * TO STDOUT commands.	We trust that no condition can make PQexec() fail
  	 * indefinitely while retaining status PGRES_COPY_OUT.
  	 */
! 	while (res = PQgetResult(conn), PQresultStatus(res) == PGRES_COPY_OUT)
  	{
  		OK = false;
! 		PQclear(res);
  
  		PQexec(conn, "-- clear PGRES_COPY_OUT state");
  	}
! 	if (PQresultStatus(res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
- 	PQclear(res);
  
  	return OK;
  }
--- 485,502 ----
  	 * TO STDOUT commands.	We trust that no condition can make PQexec() fail
  	 * indefinitely while retaining status PGRES_COPY_OUT.
  	 */
! 	while (*res = PQgetResult(conn), PQresultStatus(*res) == PGRES_COPY_OUT)
  	{
  		OK = false;
! 		PQclear(*res);
  
  		PQexec(conn, "-- clear PGRES_COPY_OUT state");
  	}
! 	if (PQresultStatus(*res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
  
  	return OK;
  }
***************
*** 519,530 **** handleCopyOut(PGconn *conn, FILE *copystream)
  #define COPYBUFSIZ 8192
  
  bool
! handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary)
  {
  	bool		OK;
  	const char *prompt;
  	char		buf[COPYBUFSIZ];
- 	PGresult   *res;
  
  	if (!copystream)
  		copystream = pset.cur_cmd_source;
--- 517,527 ----
  #define COPYBUFSIZ 8192
  
  bool
! handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res)
  {
  	bool		OK;
  	const char *prompt;
  	char		buf[COPYBUFSIZ];
  
  	if (!copystream)
  		copystream = pset.cur_cmd_source;
***************
*** 680,698 **** copyin_cleanup:
  	 * indefinitely while retaining status PGRES_COPY_IN, we get an infinite
  	 * loop.  This is more realistic than handleCopyOut()'s counterpart risk.
  	 */
! 	while (res = PQgetResult(conn), PQresultStatus(res) == PGRES_COPY_IN)
  	{
  		OK = false;
! 		PQclear(res);
  
  		PQputCopyEnd(pset.db, _("trying to exit copy mode"));
  	}
! 	if (PQresultStatus(res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
- 	PQclear(res);
  
  	return OK;
  }
--- 677,694 ----
  	 * indefinitely while retaining status PGRES_COPY_IN, we get an infinite
  	 * loop.  This is more realistic than handleCopyOut()'s counterpart risk.
  	 */
! 	while (*res = PQgetResult(conn), PQresultStatus(*res) == PGRES_COPY_IN)
  	{
  		OK = false;
! 		PQclear(*res);
  
  		PQputCopyEnd(pset.db, _("trying to exit copy mode"));
  	}
! 	if (PQresultStatus(*res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
  
  	return OK;
  }
*** a/src/bin/psql/copy.h
--- b/src/bin/psql/copy.h
***************
*** 16,22 **** 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
--- 16,22 ----
  
  /* lower level processors for copy in/out streams */
  
! bool		handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res);
! bool		handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res);
  
  #endif
#22Amit Khandekar
amit.khandekar@enterprisedb.com
In reply to: Rajeev rastogi (#21)
Re: COPY table FROM STDIN doesn't show count tag

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

On 26 November 2013, Amit Khandelkar wrote:

Can you please submit the \COPY patch as a separate patch ? Since these

are two different issues, I would like to have these two fixed and
committed separately. You can always test the \COPY issue using \COPY TO
followed by INSERT.

Please find the attached two separate patches:

Thanks.

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

You have removed the if condition in this statement, mentioning that it is
always true now:
-                       if (copystream == pset.cur_cmd_source)
-                               pset.lineno++;
+                       pset.lineno++;

But copystream can be different than pset.cur_cmd_source , right ?

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

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

2. initialcopyissuev1_ontopofslashcopy.patch : Fix for “COPY table

FROM STDIN/STDOUT doesn't show count tag”.

The following header comments of ProcessResult() need to be modified:
* Changes its argument to point to the last PGresult of the command string,
* or NULL if that result was for a COPY FROM STDIN or COPY TO STDOUT.

Regression results show all passed.

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

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

Show quoted text

Thanks and Regards,

Kumar Rajeev Rastogi

#23Rajeev rastogi
rajeev.rastogi@huawei.com
In reply to: Amit Khandekar (#22)
Re: COPY table FROM STDIN doesn't show count tag

On 9th December, Amit Khandelkar wrote:

1.      slashcopyissuev1.patch :- This patch fixes the \COPY issue.
You have removed the if condition in this statement, mentioning that it is always true now:
-                       if (copystream == pset.cur_cmd_source)
-                               pset.lineno++;
+                       pset.lineno++;

But copystream can be different than pset.cur_cmd_source , right ?

As per the earlier code, condition result was always true. So pset.lineno was always incremented.
In the earlier code pset.cur_cmd_source was sent as parameter to function and inside the function same parameter was used with the name copystream. So on entry of this function both will be one and same.
I checked inside the function handleCopyIn, both of these parameters are not changing before above check. Also since pset is specific to single session, so it cannot change concurrently.
Please let me know, if I am missing something.

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

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

OK. I shall change accodingly.

2. initialcopyissuev1_ontopofslashcopy.patch : Fix for "COPY table FROM STDIN/STDOUT doesn't show count tag".
The following header comments of ProcessResult() need to be modified:
* Changes its argument to point to the last PGresult of the command string,
* or NULL if that result was for a COPY FROM STDIN or COPY TO STDOUT.

OK. I shall change accodingly.

Regression results show all passed.
Other than this, the patch needs a new regression test.

I had checked the existing regression test cases and observed that it has already got all kind of test cases. Like
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?

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

OK.

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

Thanks and Regards,
Kumar Rajeev Rastogi

#24Rajeev rastogi
rajeev.rastogi@huawei.com
In reply to: Rajeev rastogi (#23)
2 attachment(s)
Re: COPY table FROM STDIN doesn't show count tag

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

Attachments:

slashcopyissuev1.patchapplication/octet-stream; name=slashcopyissuev1.patchDownload
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
***************
*** 689,699 **** ProcessResult(PGresult **results)
  			 * 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();
  
--- 689,706 ----
  			 * connection out of its COPY state, then call PQresultStatus()
  			 * once and report any error.
  			 */
+ 
+ 			/*
+ 			 * If this is second copy; then it will be definately not \copy,
+ 			 * and also it can not be from any user given file. So pass the
+ 			 * value of copystream as NULL, so that read/wrie happens on
+ 			 * already set cur_cmd_source/queryFout.
+ 			 */
  			SetCancelConn();
  			if (result_status == PGRES_COPY_OUT)
! 				success = handleCopyOut(pset.db, (first_cycle ? pset.copyStream : NULL)) && success;
  			else
! 				success = handleCopyIn(pset.db, (first_cycle ? pset.copyStream : NULL),
  									   PQbinaryTuples(*results)) && success;
  			ResetCancelConn();
  
*** a/src/bin/psql/copy.c
--- b/src/bin/psql/copy.c
***************
*** 269,276 **** do_copy(const char *args)
  {
  	PQExpBufferData query;
  	FILE	   *copystream;
- 	FILE	   *save_file;
- 	FILE	  **override_file;
  	struct copy_options *options;
  	bool		success;
  	struct stat st;
--- 269,274 ----
***************
*** 287,294 **** do_copy(const char *args)
  
  	if (options->from)
  	{
- 		override_file = &pset.cur_cmd_source;
- 
  		if (options->file)
  		{
  			if (options->program)
--- 285,290 ----
***************
*** 308,315 **** do_copy(const char *args)
  	}
  	else
  	{
- 		override_file = &pset.queryFout;
- 
  		if (options->file)
  		{
  			if (options->program)
--- 304,309 ----
***************
*** 369,378 **** do_copy(const char *args)
  		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)
--- 363,371 ----
  		appendPQExpBufferStr(&query, options->after_tofrom);
  
  	/* Run it like a user command, interposing the data source or sink. */
! 	pset.copyStream = copystream;
  	success = SendQuery(query.data);
! 	pset.copyStream = NULL;
  	termPQExpBuffer(&query);
  
  	if (options->file != NULL)
***************
*** 440,445 **** handleCopyOut(PGconn *conn, FILE *copystream)
--- 433,441 ----
  	int			ret;
  	PGresult   *res;
  
+ 	if (!copystream)
+ 		copystream = pset.queryFout;
+ 
  	for (;;)
  	{
  		ret = PQgetCopyData(conn, &buf, 0);
***************
*** 530,535 **** handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary)
--- 526,534 ----
  	char		buf[COPYBUFSIZ];
  	PGresult   *res;
  
+ 	if (!copystream)
+ 		copystream = pset.cur_cmd_source;
+ 
  	/*
  	 * Establish longjmp destination for exiting from wait-for-input. (This is
  	 * only effective while sigint_interrupt_enabled is TRUE.)
***************
*** 656,663 **** handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary)
  				}
  			}
  
! 			if (copystream == pset.cur_cmd_source)
! 				pset.lineno++;
  		}
  	}
  
--- 655,661 ----
  				}
  			}
  
! 			pset.lineno++;
  		}
  	}
  
*** a/src/bin/psql/settings.h
--- b/src/bin/psql/settings.h
***************
*** 70,75 **** typedef struct _psqlSettings
--- 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 */
initialcopyissuev2_ontopofslashcopy.patchapplication/octet-stream; name=initialcopyissuev2_ontopofslashcopy.patchDownload
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
***************
*** 631,638 **** StoreQueryTuple(const PGresult *result)
   * 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
--- 631,637 ----
   * 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.
   *
   * Returns true on complete success, false otherwise.  Possible failure modes
   * include purely client-side problems; check the transaction status for the
***************
*** 698,716 **** ProcessResult(PGresult **results)
  			 */
  			SetCancelConn();
  			if (result_status == PGRES_COPY_OUT)
! 				success = handleCopyOut(pset.db, (first_cycle ? pset.copyStream : NULL)) && success;
  			else
  				success = handleCopyIn(pset.db, (first_cycle ? pset.copyStream : NULL),
! 									   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 */
--- 697,720 ----
  			 */
  			SetCancelConn();
  			if (result_status == PGRES_COPY_OUT)
! 				success = handleCopyOut(pset.db, (first_cycle ? pset.copyStream : NULL),
! 										results) && success;
  			else
  				success = handleCopyIn(pset.db, (first_cycle ? pset.copyStream : NULL),
! 							   PQbinaryTuples(*results), 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. If
! 			 * it is NULL, then the last result will be returned back.
  			 */
! 			if ((next_result = PQgetResult(pset.db)))
! 			{
! 				PQclear(*results);
! 				*results = next_result;
! 			}
  		}
  		else if (first_cycle)
  			/* fast path: no COPY commands; PQexec visited all results */
*** a/src/bin/psql/copy.c
--- b/src/bin/psql/copy.c
***************
*** 426,437 **** do_copy(const char *args)
   * result is true if successful, false if not.
   */
  bool
! handleCopyOut(PGconn *conn, FILE *copystream)
  {
  	bool		OK = true;
  	char	   *buf;
  	int			ret;
- 	PGresult   *res;
  
  	if (!copystream)
  		copystream = pset.queryFout;
--- 426,436 ----
   * result is true if successful, false if not.
   */
  bool
! handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res)
  {
  	bool		OK = true;
  	char	   *buf;
  	int			ret;
  
  	if (!copystream)
  		copystream = pset.queryFout;
***************
*** 486,504 **** handleCopyOut(PGconn *conn, FILE *copystream)
  	 * TO STDOUT commands.	We trust that no condition can make PQexec() fail
  	 * indefinitely while retaining status PGRES_COPY_OUT.
  	 */
! 	while (res = PQgetResult(conn), PQresultStatus(res) == PGRES_COPY_OUT)
  	{
  		OK = false;
! 		PQclear(res);
  
  		PQexec(conn, "-- clear PGRES_COPY_OUT state");
  	}
! 	if (PQresultStatus(res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
- 	PQclear(res);
  
  	return OK;
  }
--- 485,502 ----
  	 * TO STDOUT commands.	We trust that no condition can make PQexec() fail
  	 * indefinitely while retaining status PGRES_COPY_OUT.
  	 */
! 	while (*res = PQgetResult(conn), PQresultStatus(*res) == PGRES_COPY_OUT)
  	{
  		OK = false;
! 		PQclear(*res);
  
  		PQexec(conn, "-- clear PGRES_COPY_OUT state");
  	}
! 	if (PQresultStatus(*res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
  
  	return OK;
  }
***************
*** 519,530 **** handleCopyOut(PGconn *conn, FILE *copystream)
  #define COPYBUFSIZ 8192
  
  bool
! handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary)
  {
  	bool		OK;
  	const char *prompt;
  	char		buf[COPYBUFSIZ];
- 	PGresult   *res;
  
  	if (!copystream)
  		copystream = pset.cur_cmd_source;
--- 517,527 ----
  #define COPYBUFSIZ 8192
  
  bool
! handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res)
  {
  	bool		OK;
  	const char *prompt;
  	char		buf[COPYBUFSIZ];
  
  	if (!copystream)
  		copystream = pset.cur_cmd_source;
***************
*** 680,698 **** copyin_cleanup:
  	 * indefinitely while retaining status PGRES_COPY_IN, we get an infinite
  	 * loop.  This is more realistic than handleCopyOut()'s counterpart risk.
  	 */
! 	while (res = PQgetResult(conn), PQresultStatus(res) == PGRES_COPY_IN)
  	{
  		OK = false;
! 		PQclear(res);
  
  		PQputCopyEnd(pset.db, _("trying to exit copy mode"));
  	}
! 	if (PQresultStatus(res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
- 	PQclear(res);
  
  	return OK;
  }
--- 677,694 ----
  	 * indefinitely while retaining status PGRES_COPY_IN, we get an infinite
  	 * loop.  This is more realistic than handleCopyOut()'s counterpart risk.
  	 */
! 	while (*res = PQgetResult(conn), PQresultStatus(*res) == PGRES_COPY_IN)
  	{
  		OK = false;
! 		PQclear(*res);
  
  		PQputCopyEnd(pset.db, _("trying to exit copy mode"));
  	}
! 	if (PQresultStatus(*res) != PGRES_COMMAND_OK)
  	{
  		psql_error("%s", PQerrorMessage(conn));
  		OK = false;
  	}
  
  	return OK;
  }
*** a/src/bin/psql/copy.h
--- b/src/bin/psql/copy.h
***************
*** 16,22 **** 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
--- 16,22 ----
  
  /* lower level processors for copy in/out streams */
  
! bool		handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res);
! bool		handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res);
  
  #endif
#25Tom Lane
tgl@sss.pgh.pa.us
In reply to: Rajeev rastogi (#24)
1 attachment(s)
Re: COPY table FROM STDIN doesn't show count tag

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 used with the name copystream. So on entry of this function both will be one and same.

The problem with that argument is you're assuming that the previous
behavior was correct :-(. It isn't. If you try a case like this:

$ cat int8data
123 456
123 4567890123456789
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

Attachments:

psql-copy-count-tag-20140310.patchtext/x-diff; charset=us-ascii; name=psql-copy-count-tag-20140310.patchDownload
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,
! 										&copy_result) && success;
! 			}
  			else
! 			{
! 				if (!copystream)
! 					copystream = pset.cur_cmd_source;
! 				success = handleCopyIn(pset.db,
! 									   copystream,
! 									   PQbinaryTuples(*results),
! 									   &copy_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;
  
#26Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tom Lane (#25)
1 attachment(s)
Re: COPY table FROM STDIN doesn't show count tag

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

Attachments:

psql-copy-count-tag-20140310-2.patchtext/x-diff; charset=us-ascii; name=psql-copy-count-tag-20140310-2.patchDownload
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,
! 										&copy_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),
! 									   &copy_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
#27Rajeev rastogi
rajeev.rastogi@huawei.com
In reply to: Tom Lane (#25)
Re: COPY table FROM STDIN doesn't show count tag

On 10 March 2014 23:44, Tom Lane wrote:

Unfortunately, while testing it I noticed that there's a potentially
fatal backwards-compatibility problem, namely that the "COPY n" status
gets printed on stdout, which is the same place that COPY OUT data is
going. While this isn't such a big problem for interactive use, usages
like this one are pretty popular:

psql -c 'copy mytable to stdout' mydatabase | some-program

With the patch, "COPY n" gets included in the data sent to some-program,
which never happened before and is surely not what the user wants.
The same if the -c string uses \copy.

There are several things we could do about this:

1. Treat this as a non-backwards-compatible change, and document that
people have to use -q if they don't want the COPY tag in the output.
I'm not sure this is acceptable.

2. Kluge ProcessResult so that it continues to not pass back a PGresult
for the COPY TO STDOUT case, or does so only in limited circumstances
(perhaps only if isatty(stdout), for instance).

I'm inclined to think #2 is the best answer if we can't stomach #1.

Is it OK to have different status output for different flavor of COPY command?
I am afraid that It will become kind of inconsistent result.

Also not providing the command result status may be inconsistent from
behavior of any other SQL commands.

I agree that it breaks the backward compatibility but I am not sure if anyone
is so tightly coupled with this ( or whether they will be effected with additional status result).

To me option #1 seems to be more suitable specially since there is an option to disable
the status output by giving -q.

Please provide your opinion or let me know If I have missed something.

Thanks and Regards,
Kumar Rajeev Rastogi

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

#28Tom Lane
tgl@sss.pgh.pa.us
In reply to: Rajeev rastogi (#27)
Re: COPY table FROM STDIN doesn't show count tag

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

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

#29Rajeev rastogi
rajeev.rastogi@huawei.com
In reply to: Tom Lane (#28)
Re: COPY table FROM STDIN doesn't show count tag

On 11 March 2014 19:52, Tom Lane wrote:

After sleeping on it, I'm inclined to think we should continue to not
print status for COPY TO STDOUT. Aside from the risk of breaking
scripts, there's a decent analogy to be made to SELECT: we don't print
a status tag for that either.

It is correct that SELECT does not print conventional way of status tag but still it prints the number
of rows selected (e.g. (2 rows)) along with rows actual value, which can be very well considered
as kind of status. User can make out with this result, that how many rows have been selected.

But in-case of COPY TO STDOUT, if we don't print anything, then user does not have any direct way of finding
that how many rows were copied from table to STDOUT, which might have been very useful.

Please let me know your opinion or if I have missed something.

Thanks and Regards,
Kumar Rajeev Rastogi

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

#30David Johnston
polobo@yahoo.com
In reply to: Tom Lane (#25)
Re: COPY table FROM STDIN doesn't show count tag

Tom Lane-2 wrote

Unfortunately, while testing it I noticed that there's a potentially
fatal backwards-compatibility problem, namely that the "COPY n" status
gets printed on stdout, which is the same place that COPY OUT data is
going. While this isn't such a big problem for interactive use,
usages like this one are pretty popular:

psql -c 'copy mytable to stdout' mydatabase | some-program

With the patch, "COPY n" gets included in the data sent to some-program,
which never happened before and is surely not what the user wants.
The same if the -c string uses \copy.

There are several things we could do about this:

1. Treat this as a non-backwards-compatible change, and document that
people have to use -q if they don't want the COPY tag in the output.
I'm not sure this is acceptable.

I've mostly used copy to with files and so wouldn't mind if STDOUT had the
COPY n sent to it as long as the target file is just the copy contents.

2. Kluge ProcessResult so that it continues to not pass back a PGresult
for the COPY TO STDOUT case, or does so only in limited circumstances
(perhaps only if isatty(stdout), for instance).

The main problem with this is that people will test by sending output to a
TTY and see the COPY n. Although if it can be done consistently then you
minimize backward incompatibility and encourage people to enforce quiet mode
while the command runs...

3. Modify PrintQueryStatus so that command status goes to stderr not
stdout. While this is probably how it should've been done in the first
place, this would be a far more severe compatibility break than #1.
(For one thing, there are probably scripts out there that think that any
output to stderr is an error message.) I'm afraid this one is definitely
not acceptable, though it would be by far the cleanest solution were it
not for compatibility concerns.

Yes, it's a moot point but I'm not sure it would be best anyway.

4. As #3, but print the command status to stderr only if it's "COPY n",
otherwise to stdout. This is a smaller compatibility break than #3,
but still a break since COPY status was formerly issued to stdout
in non TO STDOUT/FROM STDIN cases. (Note that PrintQueryStatus can't
tell whether it was COPY TO STDOUT rather than any other kind of COPY;
if we want that to factor into the behavior, we need ProcessResult to
do it.)

Since we are considering stderr my (inexperienced admittedly) gut says that
using stderr for this is generally undesirable and especially given our
existing precedence. stdout is the seemingly correct target, typically, and
the existing quiet-mode toggle provides sufficient control for typical
needs.

5. Give up on the print-the-tag aspect of the change, and just fix the
wrong-line-number issue (so we'd still introduce the copyStream variable,
but not change how PGresults are passed around).

I'm inclined to think #2 is the best answer if we can't stomach #1.
But the exact rule for when to print a COPY OUT result probably
still requires some debate. Or maybe someone has another idea?

Also, I'm thinking we should back-patch the aspects of the patch
needed to fix the wrong-line-number issue. That appears to have been
introduced in 9.2; older versions of PG get the above example right.

Comments?

I'd like COPY TO to anything but STDOUT to emit a "COPY n" on STDOUT -
unless suppressed by -q(uiet)

Document that COPY TO STDOUT does not emit "COPY n" because STDOUT is
already assigned for data and so is not available for notifications. Since
COPY is more typically used for ETL than a bare-select, in addition to
back-compatibility concerns, this default behavior seems reasonable.

Would it be possible to store the "n" somewhere and provide a command - like
GET DIAGNOSTICS in pl/pgsql - if the user really wants to know how many rows
were sent to STDOUT? I'm doubt this is even useful in the typical use-case
for COPY TO STDOUT but figured I'd toss the idea out there.

Is there anything besides a desire for consistency that anyone has or can
put forth as a use-case for COPY TO STDOUT emitting "COPY n" on STDOUT as
well? If you are going to view the content inline, and also want a quick
count, ISTM you would be more likely to use SELECT to take advantage of all
its pretty-print features.

If we really need to cater to this use then maybe a "--loud-copy-to-stdout"
switch can be provided to override its default quiet-mode.

David J.

--
View this message in context: http://postgresql.1045698.n5.nabble.com/COPY-table-FROM-STDIN-doesn-t-show-count-tag-tp5775018p5795611.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

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

#31Pavel Stehule
pavel.stehule@gmail.com
In reply to: David Johnston (#30)
Re: COPY table FROM STDIN doesn't show count tag

2014-03-12 7:10 GMT+01:00 David Johnston <polobo@yahoo.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

This information can be really interesting and sometimes important, when
people has no idea, how they tables are long

Regards

Pavel

Show quoted text

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

#32Tom Lane
tgl@sss.pgh.pa.us
In reply to: Rajeev rastogi (#29)
Re: COPY table FROM STDIN doesn't show count tag

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

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

#33Tom Lane
tgl@sss.pgh.pa.us
In reply to: David Johnston (#30)
Re: COPY table FROM STDIN doesn't show count tag

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

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

#34Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#33)
Re: COPY table FROM STDIN doesn't show count tag

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

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

#35Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#34)
Re: COPY table FROM STDIN doesn't show count tag

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

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

#36Rajeev rastogi
rajeev.rastogi@huawei.com
In reply to: Tom Lane (#35)
1 attachment(s)
Re: COPY table FROM STDIN doesn't show count tag

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

Attachments:

psql-copy-count-tag-20140313.patchapplication/octet-stream; name=psql-copy-count-tag-20140313.patchDownload
*** a/doc/src/sgml/ref/copy.sgml
--- b/doc/src/sgml/ref/copy.sgml
***************
*** 369,374 **** COPY <replaceable class="parameter">count</replaceable>
--- 369,382 ----
  </screen>
     The <replaceable class="parameter">count</replaceable> is the number
     of rows copied.
+    
+     <note>
+     <para>
+     In case of <literal>COPY ... TO STDOUT</literal>, 
+     command tag is not displayed.  	
+     </para>
+     </note>
+     
    </para>
   </refsect1>
  
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
***************
*** 904,909 **** testdb=&gt;
--- 904,925 ----
          </para>
          </tip>
  
+         <note>
+         <para>
+         On successful completion, a <command>COPY</> command returns a command
+         tag of the form
+         <screen>
+             COPY <replaceable class="parameter">count</replaceable>
+         </screen>
+         The <replaceable class="parameter">count</replaceable> is the number
+         of rows copied.
+         </para>
+         <para>
+         But in case of <literal>\COPY ... TO ... </literal>, 
+         command tag is not displayed.  	
+         </para>
+         </note>
+ 
          </listitem>
        </varlistentry>
  
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
***************
*** 631,638 **** StoreQueryTuple(const PGresult *result)
   * 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
***************
*** 693,737 **** ProcessResult(PGresult **results)
  			 * otherwise use queryFout or cur_cmd_source as appropriate.
  			 */
  			FILE	   *copystream = pset.copyStream;
  
  			SetCancelConn();
  			if (result_status == PGRES_COPY_OUT)
  			{
! 				if (!copystream)
  					copystream = pset.queryFout;
  				success = handleCopyOut(pset.db,
! 										copystream) && success;
  			}
  			else
  			{
! 				if (!copystream)
  					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())
--- 692,764 ----
  			 * otherwise use queryFout or cur_cmd_source as appropriate.
  			 */
  			FILE	   *copystream = pset.copyStream;
+ 			PGresult   *copy_result;
  
  			SetCancelConn();
  			if (result_status == PGRES_COPY_OUT)
  			{
! 				/*
! 				 * If this is second cycle, then it will definitely not be
! 				 * \copy, so pset.copyStream should not be used.
! 				 */
! 				if (!pset.copyStream || !first_cycle)
  					copystream = pset.queryFout;
+ 
  				success = handleCopyOut(pset.db,
! 										copystream,
! 										&copy_result) && success;
  			}
  			else
  			{
! 				if (!pset.copyStream || !first_cycle)
  					copystream = pset.cur_cmd_source;
+ 
  				success = handleCopyIn(pset.db,
  									   copystream,
! 									   PQbinaryTuples(*results),
! 									   &copy_result) && success;
  			}
  			ResetCancelConn();
  
+ 			/* replace the COPY_OUT/IN result with COPY command exit status */
+ 			PQclear(*results);
+ 			*results = copy_result;
+ 
  			/*
! 			 * 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.
! 			 * If there is other result to process then to print status or not
! 			 * will be decided on next command handling otherwise decide here
! 			 * only.
  			 */
! 			if ((next_result = PQgetResult(pset.db)))
! 			{
! 				PQclear(*results);
! 				*results = next_result;
! 			}
! 			else if (result_status == PGRES_COPY_OUT)
! 			{
! 				/* If last command is COPY TO then no need to print status. */
! 				PQclear(*results);
! 				*results = NULL;
! 			}
  		}
  		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 */
+ 			/* Printing of status for any other command will be always done*/
  			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())
*** a/src/bin/psql/copy.c
--- b/src/bin/psql/copy.c
***************
*** 429,444 **** do_copy(const char *args)
   * 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 **** handleCopyOut(PGconn *conn, FILE *copystream)
  	 * 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 **** handleCopyOut(PGconn *conn, FILE *copystream)
--- 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 **** handleCopyOut(PGconn *conn, FILE *copystream)
  #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 **** copyin_cleanup:
  	 * 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;
  }
*** 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
#37Tom Lane
tgl@sss.pgh.pa.us
In reply to: Rajeev rastogi (#36)
Re: COPY table FROM STDIN doesn't show count tag

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

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