No title

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

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 issue or suggestion.

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