Re: COPY table FROM STDIN doesn't show count tag
От | Tom Lane |
---|---|
Тема | Re: COPY table FROM STDIN doesn't show count tag |
Дата | |
Msg-id | 14537.1394475259@sss.pgh.pa.us обсуждение исходный текст |
Ответ на | Re: COPY table FROM STDIN doesn't show count tag (Rajeev rastogi <rajeev.rastogi@huawei.com>) |
Ответы |
Re: COPY table FROM STDIN doesn't show count tag
(Tom Lane <tgl@sss.pgh.pa.us>)
Re: COPY table FROM STDIN doesn't show count tag (Rajeev rastogi <rajeev.rastogi@huawei.com>) Re: COPY table FROM STDIN doesn't show count tag (David Johnston <polobo@yahoo.com>) |
Список | pgsql-hackers |
Rajeev rastogi <rajeev.rastogi@huawei.com> writes: > On 12th December 2013, Rajeev Rastogi Wrote: >> On 9th December, Amit Khandelkar wrote: >>> But copystream can be different than pset.cur_cmd_source , right ? >> As per the earlier code, condition result was always true. So pset.lineno was always incremented. >> In the earlier code pset.cur_cmd_source was sent as parameter to function and inside the function same parameter was usedwith the name copystream. So on entry of this function both will be one and same. The problem with that argument is you're assuming that the previous behavior was correct :-(. It isn't. If you try a case like this: $ cat int8data 123 456 123 4567890123456789 4567890123456789 123 4567890123456789 4567890123456789 4567890123456789 -4567890123456789 $ cat testcase.sql select 1+1; \copy int8_tbl from 'int8data' select 1/0; select 2/0; $ psql -f testcase.sql regression ?column? ---------- 2 (1 row) psql:testcase.sql:11: ERROR: division by zero psql:testcase.sql:13: ERROR: division by zero the script line numbers shown in the error messages are *wrong*, because handleCopyIn has incorrectly incremented pset.lineno because it thought it was reading from the current script file. So the override_file business is wrong, and getting rid of it with a separate copyStream variable is a good thing. However, there wasn't much else that I liked about the patch :-(. It seemed bizarre to me that the copy source/sink selection logic was partially in ProcessResult and partially in handleCopyOut/handleCopyIn. Also you'd created a memory leak because ProcessResult now failed to PQclear the original PGRES_COPY_OUT/IN PGresult. I did a bit of work to clean that up, and the attached updated patch is the result. Unfortunately, while testing it I noticed that there's a potentially fatal backwards-compatibility problem, namely that the "COPY n" status gets printed on stdout, which is the same place that COPY OUT data is going. While this isn't such a big problem for interactive use, usages like this one are pretty popular: psql -c 'copy mytable to stdout' mydatabase | some-program With the patch, "COPY n" gets included in the data sent to some-program, which never happened before and is surely not what the user wants. The same if the -c string uses \copy. There are several things we could do about this: 1. Treat this as a non-backwards-compatible change, and document that people have to use -q if they don't want the COPY tag in the output. I'm not sure this is acceptable. 2. Kluge ProcessResult so that it continues to not pass back a PGresult for the COPY TO STDOUT case, or does so only in limited circumstances (perhaps only if isatty(stdout), for instance). 3. Modify PrintQueryStatus so that command status goes to stderr not stdout. While this is probably how it should've been done in the first place, this would be a far more severe compatibility break than #1. (For one thing, there are probably scripts out there that think that any output to stderr is an error message.) I'm afraid this one is definitely not acceptable, though it would be by far the cleanest solution were it not for compatibility concerns. 4. As #3, but print the command status to stderr only if it's "COPY n", otherwise to stdout. This is a smaller compatibility break than #3, but still a break since COPY status was formerly issued to stdout in non TO STDOUT/FROM STDIN cases. (Note that PrintQueryStatus can't tell whether it was COPY TO STDOUT rather than any other kind of COPY; if we want that to factor into the behavior, we need ProcessResult to do it.) 5. Give up on the print-the-tag aspect of the change, and just fix the wrong-line-number issue (so we'd still introduce the copyStream variable, but not change how PGresults are passed around). I'm inclined to think #2 is the best answer if we can't stomach #1. But the exact rule for when to print a COPY OUT result probably still requires some debate. Or maybe someone has another idea? Also, I'm thinking we should back-patch the aspects of the patch needed to fix the wrong-line-number issue. That appears to have been introduced in 9.2; older versions of PG get the above example right. Comments? regards, tom lane diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index 3a820fa..136eed1 100644 *** a/src/bin/psql/common.c --- b/src/bin/psql/common.c *************** StoreQueryTuple(const PGresult *result) *** 628,638 **** * command. In that event, we'll marshal data for the COPY and then cycle * through any subsequent PGresult objects. * ! * When the command string contained no affected COPY command, this function * degenerates to an AcceptResult() call. * ! * Changes its argument to point to the last PGresult of the command string, ! * or NULL if that result was for a COPY FROM STDIN or COPY TO STDOUT. * * Returns true on complete success, false otherwise. Possible failure modes * include purely client-side problems; check the transaction status for the --- 628,637 ---- * command. In that event, we'll marshal data for the COPY and then cycle * through any subsequent PGresult objects. * ! * When the command string contained no such COPY command, this function * degenerates to an AcceptResult() call. * ! * Changes its argument to point to the last PGresult of the command string. * * Returns true on complete success, false otherwise. Possible failure modes * include purely client-side problems; check the transaction status for the *************** StoreQueryTuple(const PGresult *result) *** 641,654 **** static bool ProcessResult(PGresult **results) { - PGresult *next_result; bool success = true; bool first_cycle = true; ! do { ExecStatusType result_status; bool is_copy; if (!AcceptResult(*results)) { --- 640,653 ---- static bool ProcessResult(PGresult **results) { bool success = true; bool first_cycle = true; ! for (;;) { ExecStatusType result_status; bool is_copy; + PGresult *next_result; if (!AcceptResult(*results)) { *************** ProcessResult(PGresult **results) *** 688,722 **** * Marshal the COPY data. Either subroutine will get the * connection out of its COPY state, then call PQresultStatus() * once and report any error. */ SetCancelConn(); if (result_status == PGRES_COPY_OUT) ! success = handleCopyOut(pset.db, pset.queryFout) && success; else ! success = handleCopyIn(pset.db, pset.cur_cmd_source, ! PQbinaryTuples(*results)) && success; ResetCancelConn(); ! /* ! * Call PQgetResult() once more. In the typical case of a ! * single-command string, it will return NULL. Otherwise, we'll ! * have other results to process that may include other COPYs. ! */ PQclear(*results); ! *results = next_result = PQgetResult(pset.db); } else if (first_cycle) /* fast path: no COPY commands; PQexec visited all results */ break; - else if ((next_result = PQgetResult(pset.db))) - { - /* non-COPY command(s) after a COPY: keep the last one */ - PQclear(*results); - *results = next_result; } first_cycle = false; ! } while (next_result); /* may need this to recover from conn loss during COPY */ if (!first_cycle && !CheckConnection()) --- 687,742 ---- * Marshal the COPY data. Either subroutine will get the * connection out of its COPY state, then call PQresultStatus() * once and report any error. + * + * If pset.copyStream is set, use that as data source/sink, + * otherwise use queryFout or cur_cmd_source as appropriate. */ + FILE *copystream = pset.copyStream; + PGresult *copy_result; + SetCancelConn(); if (result_status == PGRES_COPY_OUT) ! { ! if (!copystream) ! copystream = pset.queryFout; ! success = handleCopyOut(pset.db, ! copystream, ! ©_result) && success; ! } else ! { ! if (!copystream) ! copystream = pset.cur_cmd_source; ! success = handleCopyIn(pset.db, ! copystream, ! PQbinaryTuples(*results), ! ©_result) && success; ! } ResetCancelConn(); ! /* replace the COPY_OUT/IN result with COPY command exit status */ PQclear(*results); ! *results = copy_result; } else if (first_cycle) + { /* fast path: no COPY commands; PQexec visited all results */ break; } + /* + * Check PQgetResult() again. In the typical case of a single-command + * string, it will return NULL. Otherwise, we'll have other results + * to process that may include other COPYs. We keep the last result. + */ + next_result = PQgetResult(pset.db); + if (!next_result) + break; + + PQclear(*results); + *results = next_result; first_cycle = false; ! } /* may need this to recover from conn loss during COPY */ if (!first_cycle && !CheckConnection()) diff --git a/src/bin/psql/copy.c b/src/bin/psql/copy.c index 9e815b1..d706206 100644 *** a/src/bin/psql/copy.c --- b/src/bin/psql/copy.c *************** do_copy(const char *args) *** 269,279 **** { PQExpBufferData query; FILE *copystream; - FILE *save_file; - FILE **override_file; struct copy_options *options; bool success; - struct stat st; /* parse options */ options = parse_slash_copy(args); --- 269,276 ---- *************** do_copy(const char *args) *** 287,294 **** if (options->from) { - override_file = &pset.cur_cmd_source; - if (options->file) { if (options->program) --- 284,289 ---- *************** do_copy(const char *args) *** 308,315 **** } else { - override_file = &pset.queryFout; - if (options->file) { if (options->program) --- 303,308 ---- *************** do_copy(const char *args) *** 345,350 **** --- 338,344 ---- if (!options->program) { + struct stat st; int result; /* make sure the specified file is not a directory */ *************** do_copy(const char *args) *** 375,385 **** if (options->after_tofrom) appendPQExpBufferStr(&query, options->after_tofrom); ! /* Run it like a user command, interposing the data source or sink. */ ! save_file = *override_file; ! *override_file = copystream; success = SendQuery(query.data); ! *override_file = save_file; termPQExpBuffer(&query); if (options->file != NULL) --- 369,378 ---- if (options->after_tofrom) appendPQExpBufferStr(&query, options->after_tofrom); ! /* run it like a user command, but with copystream as data source/sink */ ! pset.copyStream = copystream; success = SendQuery(query.data); ! pset.copyStream = NULL; termPQExpBuffer(&query); if (options->file != NULL) *************** do_copy(const char *args) *** 436,451 **** * conn should be a database connection that you just issued COPY TO on * and got back a PGRES_COPY_OUT result. * copystream is the file stream for the data to go to. * * result is true if successful, false if not. */ bool ! handleCopyOut(PGconn *conn, FILE *copystream) { bool OK = true; char *buf; int ret; - PGresult *res; for (;;) { --- 429,445 ---- * conn should be a database connection that you just issued COPY TO on * and got back a PGRES_COPY_OUT result. * copystream is the file stream for the data to go to. + * The final status for the COPY is returned into *res (but note + * we already reported the error, if it's not a success result). * * result is true if successful, false if not. */ bool ! handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res) { bool OK = true; char *buf; int ret; for (;;) { *************** handleCopyOut(PGconn *conn, FILE *copyst *** 492,504 **** * but hasn't exited COPY_OUT state internally. So we ignore the * possibility here. */ ! res = PQgetResult(conn); ! if (PQresultStatus(res) != PGRES_COMMAND_OK) { psql_error("%s", PQerrorMessage(conn)); OK = false; } - PQclear(res); return OK; } --- 486,497 ---- * but hasn't exited COPY_OUT state internally. So we ignore the * possibility here. */ ! *res = PQgetResult(conn); ! if (PQresultStatus(*res) != PGRES_COMMAND_OK) { psql_error("%s", PQerrorMessage(conn)); OK = false; } return OK; } *************** handleCopyOut(PGconn *conn, FILE *copyst *** 511,516 **** --- 504,511 ---- * and got back a PGRES_COPY_IN result. * copystream is the file stream to read the data from. * isbinary can be set from PQbinaryTuples(). + * The final status for the COPY is returned into *res (but note + * we already reported the error, if it's not a success result). * * result is true if successful, false if not. */ *************** handleCopyOut(PGconn *conn, FILE *copyst *** 519,530 **** #define COPYBUFSIZ 8192 bool ! handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary) { bool OK; const char *prompt; char buf[COPYBUFSIZ]; - PGresult *res; /* * Establish longjmp destination for exiting from wait-for-input. (This is --- 514,524 ---- #define COPYBUFSIZ 8192 bool ! handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res) { bool OK; const char *prompt; char buf[COPYBUFSIZ]; /* * Establish longjmp destination for exiting from wait-for-input. (This is *************** copyin_cleanup: *** 686,706 **** * connection is lost. But that's fine; it will get us out of COPY_IN * state, which is what we need.) */ ! while (res = PQgetResult(conn), PQresultStatus(res) == PGRES_COPY_IN) { OK = false; ! PQclear(res); /* We can't send an error message if we're using protocol version 2 */ PQputCopyEnd(conn, (PQprotocolVersion(conn) < 3) ? NULL : _("trying to exit copy mode")); } ! if (PQresultStatus(res) != PGRES_COMMAND_OK) { psql_error("%s", PQerrorMessage(conn)); OK = false; } - PQclear(res); return OK; } --- 680,699 ---- * connection is lost. But that's fine; it will get us out of COPY_IN * state, which is what we need.) */ ! while (*res = PQgetResult(conn), PQresultStatus(*res) == PGRES_COPY_IN) { OK = false; ! PQclear(*res); /* We can't send an error message if we're using protocol version 2 */ PQputCopyEnd(conn, (PQprotocolVersion(conn) < 3) ? NULL : _("trying to exit copy mode")); } ! if (PQresultStatus(*res) != PGRES_COMMAND_OK) { psql_error("%s", PQerrorMessage(conn)); OK = false; } return OK; } diff --git a/src/bin/psql/copy.h b/src/bin/psql/copy.h index ec1f0d0..2c71da0 100644 *** a/src/bin/psql/copy.h --- b/src/bin/psql/copy.h *************** *** 12,22 **** /* handler for \copy */ ! bool do_copy(const char *args); /* lower level processors for copy in/out streams */ ! bool handleCopyOut(PGconn *conn, FILE *copystream); ! bool handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary); #endif --- 12,24 ---- /* handler for \copy */ ! extern bool do_copy(const char *args); /* lower level processors for copy in/out streams */ ! extern bool handleCopyOut(PGconn *conn, FILE *copystream, ! PGresult **res); ! extern bool handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, ! PGresult **res); #endif diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h index 3e8328d..eecffb1 100644 *** a/src/bin/psql/settings.h --- b/src/bin/psql/settings.h *************** typedef struct _psqlSettings *** 70,75 **** --- 70,77 ---- FILE *queryFout; /* where to send the query results */ bool queryFoutPipe; /* queryFout is from a popen() */ + FILE *copyStream; /* Stream to read/write for \copy command */ + printQueryOpt popt; char *gfname; /* one-shot file output argument for \g */ diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c index d5f1c0d..45653a1 100644 *** a/src/bin/psql/startup.c --- b/src/bin/psql/startup.c *************** main(int argc, char *argv[]) *** 118,123 **** --- 118,124 ---- pset.encoding = PQenv2encoding(); pset.queryFout = stdout; pset.queryFoutPipe = false; + pset.copyStream = NULL; pset.cur_cmd_source = stdin; pset.cur_cmd_interactive = false;
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Robert HaasДата:
Сообщение: Re: Retain dynamic shared memory segments for postmaster lifetime