psql COPY vs. ON_ERROR_ROLLBACK, multi-command strings
It has bothered me that psql's \copy ignores the ON_ERROR_ROLLBACK setting.
Only SendQuery() takes note of ON_ERROR_ROLLBACK, and \copy, like all
backslash commands, does not route through SendQuery(). Looking into this
turned up several other weaknesses in psql's handling of COPY. For example,
SendQuery() does not release the savepoint after an ordinary COPY:
[local] test=# set log_statement = 'all'; set client_min_messages = 'log'; copy (select 0) to stdout;
SET
SET
LOG: statement: RELEASE pg_psql_temporary_savepoint
LOG: statement: SAVEPOINT pg_psql_temporary_savepoint
LOG: statement: copy (select 0) to stdout;
0
When psql sends a command string containing more than one command, at least
one of which is a COPY, we stop processing results at the first COPY:
[local] test=# set log_statement = 'all'; set client_min_messages = 'log'; copy (select 0) to stdout\; select 1/0; select 1;
SET
SET
LOG: statement: RELEASE pg_psql_temporary_savepoint
LOG: statement: SAVEPOINT pg_psql_temporary_savepoint
LOG: statement: copy (select 0) to stdout; select 1/0;
0
LOG: statement: select 1;
ERROR: current transaction is aborted, commands ignored until end of transaction block
To make the above work normally, this patch improves SendQuery()-based COPY
command handling to process the entire queue of results whenever we've
processed a COPY. It also brings sensible handling in the face of multiple
COPY commands in a single command string. See the included test cases for
some examples.
We must prepare for COPY to fail for a local reason, like client-side malloc()
failure or network I/O problems. The server will still have the connection in
a COPY mode, and we must get it out of that mode. The \copy command was
prepared for the COPY FROM case with this code block:
/*
* Make sure we have pumped libpq dry of results; else it may still be in
* ASYNC_BUSY state, leading to false readings in, eg, get_prompt().
*/
while ((result = PQgetResult(pset.db)) != NULL)
{
success = false;
psql_error("\\copy: unexpected response (%d)\n",
PQresultStatus(result));
/* if still in COPY IN state, try to get out of it */
if (PQresultStatus(result) == PGRES_COPY_IN)
PQputCopyEnd(pset.db, _("trying to exit copy mode"));
PQclear(result);
}
It arose from these threads:
http://archives.postgresql.org/pgsql-hackers/2006-11/msg00694.php
http://archives.postgresql.org/pgsql-general/2009-08/msg00268.php
However, psql still enters an infinite loop when COPY TO STDOUT encounters a
client-side failure, such as malloc() failure. I've merged the above
treatment into lower-level routines, granting plain COPY commands similar
benefits, and fixed the COPY TO handling. To help you test the corner cases
involved here, I've attached a gdb script that will inject client side
failures into both kinds of COPY commands. Attach gdb to your psql process,
source the script, and compare the behavior of commands like these with and
without the patch:
\copy (select 0) to pstdout
create table t (c int);
\copy t from stdin
1
\.
With plain COPY handled thoroughly, I fixed \copy by having it override the
source or destination stream, then call SendQuery(). This gets us support for
ON_ERROR_ROLLBACK, \timing, \set ECHO and \set SINGLESTEP for free. I found
it reasonable to treat \copy's SQL commmand more like a user-entered command
than an internal command, because \copy is such a thin wrapper around COPY
FROM STDIN/COPY TO STDOUT.
This patch makes superfluous the second argument of PSQLexec(), but I have not
removed that argument.
Incidentally, psql's common.c had several switches on PQresultStatus(res) that
did not include a case for PGRES_COPY_BOTH and also silently assumed any
unlisted status was some kind of error. I tightened these to distinguish all
known statuses and give a diagnostic upon receiving an unknown status.
Thanks,
nm
Attachments:
psql-copy-v1.patchtext/plain; charset=us-asciiDownload
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 69fac83..c7d024a 100644
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
***************
*** 320,344 **** exec_command(const char *cmd,
/* \copy */
else if (pg_strcasecmp(cmd, "copy") == 0)
{
- /* Default fetch-it-all-and-print mode */
- instr_time before,
- after;
-
char *opt = psql_scan_slash_option(scan_state,
OT_WHOLE_LINE, NULL, false);
- if (pset.timing)
- INSTR_TIME_SET_CURRENT(before);
-
success = do_copy(opt);
-
- if (pset.timing && success)
- {
- INSTR_TIME_SET_CURRENT(after);
- INSTR_TIME_SUBTRACT(after, before);
- printf(_("Time: %.3f ms\n"), INSTR_TIME_GET_MILLISEC(after));
- }
-
free(opt);
}
--- 320,329 ----
diff --git a/src/bin/psql/comindex 889c157..6eaf794 100644
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
***************
*** 438,444 **** ResetCancelConn(void)
static bool
AcceptResult(const PGresult *result)
{
! bool OK = true;
if (!result)
OK = false;
--- 438,444 ----
static bool
AcceptResult(const PGresult *result)
{
! bool OK;
if (!result)
OK = false;
***************
*** 450,460 **** AcceptResult(const PGresult *result)
--- 450,470 ----
case PGRES_EMPTY_QUERY:
case PGRES_COPY_IN:
case PGRES_COPY_OUT:
+ case PGRES_COPY_BOTH:
/* Fine, do nothing */
+ OK = true;
+ break;
+
+ case PGRES_BAD_RESPONSE:
+ case PGRES_NONFATAL_ERROR:
+ case PGRES_FATAL_ERROR:
+ OK = false;
break;
default:
OK = false;
+ psql_error("unexpected PQresultStatus (%d)",
+ PQresultStatus(result));
break;
}
***************
*** 620,664 **** PrintQueryTuples(const PGresult *results)
/*
! * ProcessCopyResult: if command was a COPY FROM STDIN/TO STDOUT, handle it
*
! * Note: Utility function for use by SendQuery() only.
*
! * Returns true if the query executed successfully, false otherwise.
*/
static bool
! ProcessCopyResult(PGresult *results)
{
! bool success = false;
! if (!results)
! return false;
!
! switch (PQresultStatus(results))
{
! case PGRES_TUPLES_OK:
! case PGRES_COMMAND_OK:
! case PGRES_EMPTY_QUERY:
! /* nothing to do here */
! success = true;
! break;
! case PGRES_COPY_OUT:
! SetCancelConn();
! success = handleCopyOut(pset.db, pset.queryFout);
! ResetCancelConn();
break;
! case PGRES_COPY_IN:
SetCancelConn();
! success = handleCopyIn(pset.db, pset.cur_cmd_source,
! PQbinaryTuples(results));
ResetCancelConn();
- break;
! default:
break;
! }
/* may need this to recover from conn loss during COPY */
if (!CheckConnection())
--- 630,743 ----
/*
! * ProcessResult: utility function for use by SendQuery() only
*
! * When our command string contained a COPY FROM STDIN or COPY TO STDOUT,
! * PQexec() has stopped at the PGresult associated with the first such
! * 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
! * server-side opinion.
*/
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))
! {
! /*
! * Failure at this point is always a server-side failure or a
! * failure to submit the command string. Either way, we're
! * finished with this command string.
! */
! success = false;
break;
+ }
! result_status = PQresultStatus(*results);
! switch (result_status)
! {
! case PGRES_COPY_BOTH:
! /*
! * No now-existing SQL command can yield PGRES_COPY_BOTH, but
! * defend against the future. PQexec() can't short-circuit
! * it's way out of a PGRES_COPY_BOTH, so the connection will
! * be useless at this point. XXX is there a method for
! * clearing this status that's likely to work with every
! * future command that can initiate it?
! */
! psql_error("unexpected PQresultStatus (%d)", result_status);
! return false;
!
! case PGRES_COPY_OUT:
! case PGRES_COPY_IN:
! is_copy = true;
! break;
!
! case PGRES_EMPTY_QUERY:
! case PGRES_COMMAND_OK:
! case PGRES_TUPLES_OK:
! is_copy = false;
! break;
!
! default:
! /* AcceptResult() should have caught anything else. */
! is_copy = false;
! psql_error("unexpected PQresultStatus (%d)", result_status);
! break;
! }
!
! if (is_copy)
! {
! /*
! * 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 (!CheckConnection())
***************
*** 708,714 **** PrintQueryStatus(PGresult *results)
static bool
PrintQueryResults(PGresult *results)
{
! bool success = false;
const char *cmdstatus;
if (!results)
--- 787,793 ----
static bool
PrintQueryResults(PGresult *results)
{
! bool success;
const char *cmdstatus;
if (!results)
***************
*** 738,748 **** PrintQueryResults(PGresult *results)
--- 817,837 ----
case PGRES_COPY_OUT:
case PGRES_COPY_IN:
+ case PGRES_COPY_BOTH:
/* nothing to do here */
success = true;
break;
+ case PGRES_BAD_RESPONSE:
+ case PGRES_NONFATAL_ERROR:
+ case PGRES_FATAL_ERROR:
+ success = false;
+ break;
+
default:
+ success = false;
+ psql_error("unexpected PQresultStatus (%d)",
+ PQresultStatus(results));
break;
}
***************
*** 867,873 **** SendQuery(const char *query)
/* these operations are included in the timing result: */
ResetCancelConn();
! OK = (AcceptResult(results) && ProcessCopyResult(results));
if (pset.timing)
{
--- 956,962 ----
/* these operations are included in the timing result: */
ResetCancelConn();
! OK = ProcessResult(&results);
if (pset.timing)
{
***************
*** 877,883 **** SendQuery(const char *query)
}
/* but printing results isn't: */
! if (OK)
OK = PrintQueryResults(results);
}
else
--- 966,972 ----
}
/* but printing results isn't: */
! if (OK && results)
OK = PrintQueryResults(results);
}
else
***************
*** 891,924 **** SendQuery(const char *query)
/* If we made a temporary savepoint, possibly release/rollback */
if (on_error_rollback_savepoint)
{
! const char *svptcmd;
transaction_status = PQtransactionStatus(pset.db);
! if (transaction_status == PQTRANS_INERROR)
! {
! /* We always rollback on an error */
! svptcmd = "ROLLBACK TO pg_psql_temporary_savepoint";
! }
! else if (transaction_status != PQTRANS_INTRANS)
{
! /* If they are no longer in a transaction, then do nothing */
! svptcmd = NULL;
! }
! else
! {
! /*
! * Do nothing if they are messing with savepoints themselves: If
! * the user did RELEASE or ROLLBACK, our savepoint is gone. If
! * they issued a SAVEPOINT, releasing ours would remove theirs.
! */
! if (results &&
! (strcmp(PQcmdStatus(results), "SAVEPOINT") == 0 ||
! strcmp(PQcmdStatus(results), "RELEASE") == 0 ||
! strcmp(PQcmdStatus(results), "ROLLBACK") == 0))
! svptcmd = NULL;
! else
! svptcmd = "RELEASE pg_psql_temporary_savepoint";
}
if (svptcmd)
--- 980,1023 ----
/* If we made a temporary savepoint, possibly release/rollback */
if (on_error_rollback_savepoint)
{
! const char *svptcmd = NULL;
transaction_status = PQtransactionStatus(pset.db);
! switch (transaction_status)
{
! case PQTRANS_INERROR:
! /* We always rollback on an error */
! svptcmd = "ROLLBACK TO pg_psql_temporary_savepoint";
! break;
!
! case PQTRANS_IDLE:
! /* If they are no longer in a transaction, then do nothing */
! break;
!
! case PQTRANS_INTRANS:
! /*
! * Do nothing if they are messing with savepoints themselves:
! * If the user did RELEASE or ROLLBACK, our savepoint is
! * gone. If they issued a SAVEPOINT, releasing ours would
! * remove theirs.
! */
! if (results &&
! (strcmp(PQcmdStatus(results), "SAVEPOINT") == 0 ||
! strcmp(PQcmdStatus(results), "RELEASE") == 0 ||
! strcmp(PQcmdStatus(results), "ROLLBACK") == 0))
! svptcmd = NULL;
! else
! svptcmd = "RELEASE pg_psql_temporary_savepoint";
! break;
!
! case PQTRANS_ACTIVE:
! case PQTRANS_UNKNOWN:
! default:
! OK = false;
! psql_error("unexpected transaction status (%d)\n",
! transaction_status);
! break;
}
if (svptcmd)
diff --git a/src/bin/psql/coindex eaf633d..a1dea95 100644
*** a/src/bin/psql/copy.c
--- b/src/bin/psql/copy.c
***************
*** 244,251 **** do_copy(const char *args)
{
PQExpBufferData query;
FILE *copystream;
struct copy_options *options;
- PGresult *result;
bool success;
struct stat st;
--- 244,252 ----
{
PQExpBufferData query;
FILE *copystream;
+ FILE *save_file;
+ FILE **override_file;
struct copy_options *options;
bool success;
struct stat st;
***************
*** 261,266 **** do_copy(const char *args)
--- 262,269 ----
if (options->from)
{
+ override_file = &pset.cur_cmd_source;
+
if (options->file)
copystream = fopen(options->file, PG_BINARY_R);
else if (!options->psql_inout)
***************
*** 270,275 **** do_copy(const char *args)
--- 273,280 ----
}
else
{
+ override_file = &pset.queryFout;
+
if (options->file)
copystream = fopen(options->file, PG_BINARY_W);
else if (!options->psql_inout)
***************
*** 308,359 **** do_copy(const char *args)
if (options->after_tofrom)
appendPQExpBufferStr(&query, options->after_tofrom);
! result = PSQLexec(query.data, true);
termPQExpBuffer(&query);
- switch (PQresultStatus(result))
- {
- case PGRES_COPY_OUT:
- SetCancelConn();
- success = handleCopyOut(pset.db, copystream);
- ResetCancelConn();
- break;
- case PGRES_COPY_IN:
- SetCancelConn();
- success = handleCopyIn(pset.db, copystream,
- PQbinaryTuples(result));
- ResetCancelConn();
- break;
- case PGRES_NONFATAL_ERROR:
- case PGRES_FATAL_ERROR:
- case PGRES_BAD_RESPONSE:
- success = false;
- psql_error("\\copy: %s", PQerrorMessage(pset.db));
- break;
- default:
- success = false;
- psql_error("\\copy: unexpected response (%d)\n",
- PQresultStatus(result));
- break;
- }
-
- PQclear(result);
-
- /*
- * Make sure we have pumped libpq dry of results; else it may still be in
- * ASYNC_BUSY state, leading to false readings in, eg, get_prompt().
- */
- while ((result = PQgetResult(pset.db)) != NULL)
- {
- success = false;
- psql_error("\\copy: unexpected response (%d)\n",
- PQresultStatus(result));
- /* if still in COPY IN state, try to get out of it */
- if (PQresultStatus(result) == PGRES_COPY_IN)
- PQputCopyEnd(pset.db, _("trying to exit copy mode"));
- PQclear(result);
- }
-
if (options->file != NULL)
{
if (fclose(copystream) != 0)
--- 313,325 ----
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)
{
if (fclose(copystream) != 0)
***************
*** 425,432 **** handleCopyOut(PGconn *conn, FILE *copystream)
OK = false;
}
! /* Check command status and return to normal libpq state */
! res = PQgetResult(conn);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
psql_error("%s", PQerrorMessage(conn));
--- 391,420 ----
OK = false;
}
! /*
! * Check command status and return to normal libpq state. After a
! * client-side error, the server will remain ready to deliver data. The
! * cleanest thing is to fully drain and discard that data. If the
! * client-side error happened early in a large file, this takes a long
! * time. Instead, take advantage of the fact that PQexec() will silently
! * end any ongoing PGRES_COPY_OUT state. This does cause us to lose the
! * results of any commands following the COPY in a single command string.
! * It also only works for protocol version 3. XXX should we clean up
! * using the slow way when the connection is using protocol version 2?
! *
! * We must not ever return with the status still PGRES_COPY_OUT. Our
! * caller is unable to distinguish that situation from reaching the next
! * COPY in a command string that happened to contain two consecutive COPY
! * 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));
***************
*** 471,483 **** handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary)
/* Terminate data transfer */
PQputCopyEnd(conn, _("canceled by user"));
! /* Check command status and return to normal libpq state */
! res = PQgetResult(conn);
! if (PQresultStatus(res) != PGRES_COMMAND_OK)
! psql_error("%s", PQerrorMessage(conn));
! PQclear(res);
!
! return false;
}
/* Prompt if interactive input */
--- 459,466 ----
/* Terminate data transfer */
PQputCopyEnd(conn, _("canceled by user"));
! OK = false;
! goto copyin_cleanup;
}
/* Prompt if interactive input */
***************
*** 600,607 **** handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary)
OK ? NULL : _("aborted because of read failure")) <= 0)
OK = false;
! /* Check command status and return to normal libpq state */
! res = PQgetResult(conn);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
psql_error("%s", PQerrorMessage(conn));
--- 583,606 ----
OK ? NULL : _("aborted because of read failure")) <= 0)
OK = false;
! copyin_cleanup:
! /*
! * Check command status and return to normal libpq state
! *
! * We must not ever return with the status still PGRES_COPY_IN. Our
! * caller is unable to distinguish that situation from reaching the next
! * COPY in a command string that happened to contain two consecutive COPY
! * FROM STDIN commands. XXX if something makes PQputCopyEnd() fail
! * 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));
diff --git a/src/test/regrindex cbc140c..7c16a61 100644
*** a/src/test/regress/expected/copyselect.out
--- b/src/test/regress/expected/copyselect.out
***************
*** 111,118 **** t
\copy v_test1 to stdout
ERROR: cannot copy from view "v_test1"
HINT: Try the COPY (SELECT ...) TO variant.
- \copy: ERROR: cannot copy from view "v_test1"
- HINT: Try the COPY (SELECT ...) TO variant.
--
-- Test \copy (select ...)
--
--- 111,116 ----
***************
*** 124,126 **** HINT: Try the COPY (SELECT ...) TO variant.
--- 122,153 ----
drop table test2;
drop view v_test1;
drop table test1;
+ -- psql handling of COPY in multi-command strings
+ copy (select 1) to stdout\; select 1/0; -- row, then error
+ 1
+ ERROR: division by zero
+ select 1/0\; copy (select 1) to stdout; -- error only
+ ERROR: division by zero
+ copy (select 1) to stdout\; copy (select 2) to stdout\; select 0\; select 3; -- 1 2 3
+ 1
+ 2
+ ?column?
+ ----------
+ 3
+ (1 row)
+
+ create table test3 (c int);
+ select 0\; copy test3 from stdin\; copy test3 from stdin\; select 1; -- 1
+ ?column?
+ ----------
+ 1
+ (1 row)
+
+ select * from test3;
+ c
+ ---
+ 1
+ 2
+ (2 rows)
+
+ drop table test3;
diff --git a/src/test/regress/sql/copyselect.sqindex 621d494..1d98dad 100644
*** a/src/test/regress/sql/copyselect.sql
--- b/src/test/regress/sql/copyselect.sql
***************
*** 80,82 **** copy (select t from test1 where id = 1) to stdout csv header force quote t;
--- 80,96 ----
drop table test2;
drop view v_test1;
drop table test1;
+
+ -- psql handling of COPY in multi-command strings
+ copy (select 1) to stdout\; select 1/0; -- row, then error
+ select 1/0\; copy (select 1) to stdout; -- error only
+ copy (select 1) to stdout\; copy (select 2) to stdout\; select 0\; select 3; -- 1 2 3
+
+ create table test3 (c int);
+ select 0\; copy test3 from stdin\; copy test3 from stdin\; select 1; -- 1
+ 1
+ \.
+ 2
+ \.
+ select * from test3;
+ drop table test3;
Excerpts from Noah Misch's message of sáb ene 14 12:40:02 -0300 2012:
It has bothered me that psql's \copy ignores the ON_ERROR_ROLLBACK setting.
Only SendQuery() takes note of ON_ERROR_ROLLBACK, and \copy, like all
backslash commands, does not route through SendQuery(). Looking into this
turned up several other weaknesses in psql's handling of COPY.
Interesting.
Committed, thanks.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Wed, Jan 25, 2012 at 06:25:46PM -0300, Alvaro Herrera wrote:
Excerpts from Noah Misch's message of s??b ene 14 12:40:02 -0300 2012:
It has bothered me that psql's \copy ignores the ON_ERROR_ROLLBACK setting.
Only SendQuery() takes note of ON_ERROR_ROLLBACK, and \copy, like all
backslash commands, does not route through SendQuery(). Looking into this
turned up several other weaknesses in psql's handling of COPY.Interesting.
Committed, thanks.
Thanks. While testing a crashing function, I noticed that my above patch
added some noise to psql output when the server crashes:
[local] test=# select crashme();
The connection to the server was lost. Attempting reset: Failed.
The connection to the server was lost. Attempting reset: Failed.
unexpected transaction status (4)
Time: 6.681 ms
!> \q
Status 4 is PQTRANS_UNKNOWN, which is expected when the connection is not
CONNECTION_OK. The double message arrives because ProcessResult() now calls
CheckConnection() at least twice, for the benefit of COPY. (Incidentally, the
reconnect fails because the server has not yet finished recovering; that part
is nothing new.)
The attached small patch has SendQuery() keep quiet about PQTRANS_UNKNOWN when
the connection is down. It makes ProcessResult() skip the second
CheckConnection() when the command string had no COPY results. This restores
the pre-08146775acd8bfe0fcc509c71857abb928697171 psql output:
[local] test=# select crashme();
The connection to the server was lost. Attempting reset: Failed.
Time: 4.798 ms
!> \q
Thanks,
nm
Attachments:
psql-crashoutput-v1.patchtext/plain; charset=us-asciiDownload
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 5c9bd96..715e231 100644
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
***************
*** 740,746 **** ProcessResult(PGresult **results)
} while (next_result);
/* may need this to recover from conn loss during COPY */
! if (!CheckConnection())
return false;
return success;
--- 740,746 ----
} while (next_result);
/* may need this to recover from conn loss during COPY */
! if (!first_cycle && !CheckConnection())
return false;
return success;
***************
*** 1015,1022 **** SendQuery(const char *query)
case PQTRANS_UNKNOWN:
default:
OK = false;
! psql_error("unexpected transaction status (%d)\n",
! transaction_status);
break;
}
--- 1015,1024 ----
case PQTRANS_UNKNOWN:
default:
OK = false;
! /* PQTRANS_UNKNOWN is expected given a broken connection. */
! if (transaction_status != PQTRANS_UNKNOWN || ConnectionUp())
! psql_error("unexpected transaction status (%d)\n",
! transaction_status);
break;
}
On Sat, Feb 25, 2012 at 12:57 AM, Noah Misch <noah@leadboat.com> wrote:
Thanks. While testing a crashing function, I noticed that my above patch
added some noise to psql output when the server crashes:[local] test=# select crashme();
The connection to the server was lost. Attempting reset: Failed.
The connection to the server was lost. Attempting reset: Failed.
unexpected transaction status (4)
Time: 6.681 ms
!> \qStatus 4 is PQTRANS_UNKNOWN, which is expected when the connection is not
CONNECTION_OK. The double message arrives because ProcessResult() now calls
CheckConnection() at least twice, for the benefit of COPY. (Incidentally, the
reconnect fails because the server has not yet finished recovering; that part
is nothing new.)The attached small patch has SendQuery() keep quiet about PQTRANS_UNKNOWN when
the connection is down. It makes ProcessResult() skip the second
CheckConnection() when the command string had no COPY results. This restores
the pre-08146775acd8bfe0fcc509c71857abb928697171 psql output:[local] test=# select crashme();
The connection to the server was lost. Attempting reset: Failed.
Time: 4.798 ms
!> \q
Committed, but... why do we EVER need to call CheckConnection() at the
bottom of ProcessResult(), after exiting that function's main loop? I
don't see any way to get out of that loop without first calling
AcceptResult on every PGresult we've seen, and that function already
calls CheckConnection() if any failure is observed.
As a side note, the documentation for PQexec() is misleading about
what will happen if COPY is present in a multi-command string. It
says: "Note however that the returned PGresult structure describes
only the result of the last command executed from the string. Should
one of the commands fail, processing of the string stops with it and
the returned PGresult describes the error condition. It does not
explain that it also stops if it hits a COPY. I had to read the
source code for libpq to understand why this psql logic was coded the
way it is.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, Mar 07, 2012 at 04:57:12PM -0500, Robert Haas wrote:
On Sat, Feb 25, 2012 at 12:57 AM, Noah Misch <noah@leadboat.com> wrote:
Thanks. ?While testing a crashing function, I noticed that my above patch
added some noise to psql output when the server crashes:[local] test=# select crashme();
The connection to the server was lost. Attempting reset: Failed.
The connection to the server was lost. Attempting reset: Failed.
unexpected transaction status (4)
Time: 6.681 ms
?!> \qStatus 4 is PQTRANS_UNKNOWN, which is expected when the connection is not
CONNECTION_OK. ?The double message arrives because ProcessResult() now calls
CheckConnection() at least twice, for the benefit of COPY. ?(Incidentally, the
reconnect fails because the server has not yet finished recovering; that part
is nothing new.)The attached small patch has SendQuery() keep quiet about PQTRANS_UNKNOWN when
the connection is down. ?It makes ProcessResult() skip the second
CheckConnection() when the command string had no COPY results. ?This restores
the pre-08146775acd8bfe0fcc509c71857abb928697171 psql output:[local] test=# select crashme();
The connection to the server was lost. Attempting reset: Failed.
Time: 4.798 ms
?!> \qCommitted, but... why do we EVER need to call CheckConnection() at the
bottom of ProcessResult(), after exiting that function's main loop? I
don't see any way to get out of that loop without first calling
AcceptResult on every PGresult we've seen, and that function already
calls CheckConnection() if any failure is observed.
You can reproduce a case where it helps by sending SIGKILL to a backend
serving a long-running COPY TO, such as this:
copy (select n, pg_sleep(case when n > 1000 then 1 else 0 end)
from generate_series(1,1030) t(n)) to stdout;
The connection dies during handleCopyOut(). By the time control returns to
ProcessResult(), there's no remaining PGresult to check. Only that last-ditch
CheckConnection() notices the lost connection.
[I notice more examples of poor error reporting cosmetics in some of these
COPY corner cases, so I anticipate another patch someday.]
As a side note, the documentation for PQexec() is misleading about
what will happen if COPY is present in a multi-command string. It
says: "Note however that the returned PGresult structure describes
only the result of the last command executed from the string. Should
one of the commands fail, processing of the string stops with it and
the returned PGresult describes the error condition. It does not
explain that it also stops if it hits a COPY. I had to read the
source code for libpq to understand why this psql logic was coded the
way it is.
Agreed; I went through a similar process. Awhile after reading the code, I
found the behavior documented in section "Functions Associated with the COPY
Command":
If a COPY command is issued via PQexec in a string that could contain
additional commands, the application must continue fetching results via
PQgetResult after completing the COPY sequence. Only when PQgetResult
returns NULL is it certain that the PQexec command string is done and it is
safe to issue more commands.
I'm not quite sure what revision would help most here -- a cross reference,
moving that content, duplicating that content. Offhand, I'm inclined to move
it to the PQexec() documentation with some kind of reference back from its
original location. Thoughts?
Thanks,
nm
Excerpts from Noah Misch's message of jue mar 08 12:11:37 -0300 2012:
On Wed, Mar 07, 2012 at 04:57:12PM -0500, Robert Haas wrote:
As a side note, the documentation for PQexec() is misleading about
what will happen if COPY is present in a multi-command string. It
says: "Note however that the returned PGresult structure describes
only the result of the last command executed from the string. Should
one of the commands fail, processing of the string stops with it and
the returned PGresult describes the error condition. It does not
explain that it also stops if it hits a COPY. I had to read the
source code for libpq to understand why this psql logic was coded the
way it is.Agreed; I went through a similar process. Awhile after reading the code, I
found the behavior documented in section "Functions Associated with the COPY
Command":If a COPY command is issued via PQexec in a string that could contain
additional commands, the application must continue fetching results via
PQgetResult after completing the COPY sequence. Only when PQgetResult
returns NULL is it certain that the PQexec command string is done and it is
safe to issue more commands.I'm not quite sure what revision would help most here -- a cross reference,
moving that content, duplicating that content. Offhand, I'm inclined to move
it to the PQexec() documentation with some kind of reference back from its
original location. Thoughts?
I would vote for moving it and adding a reference in the COPY functions
section. That way, the PQexec doc is complete by itself without having
to duplicate anything.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support