From feb8254518752b2cb4a8964c374dd82d49ef0e0d Mon Sep 17 00:00:00 2001 From: Tom Lane Date: 2018年3月22日 17:33:10 -0400 Subject: [PATCH] Improve style guideline compliance of assorted error-report messages. Per the project style guide, details and hints should have leading capitalization and end with a period. On the other hand, errcontext should not be capitalized and should not end with a period. To support well formatted error contexts in dblink, extend dblink_res_error() to take a format+arguments rather than a hardcoded string. Daniel Gustafsson Discussion: https://postgr.es/m/B3C002C8-21A0-4F53-A06E-8CAB29FCF295@yesql.se --- contrib/dblink/dblink.c | 68 +++++++++++++------ contrib/dblink/expected/dblink.out | 24 +++---- contrib/file_fdw/file_fdw.c | 4 +- contrib/pgcrypto/px.c | 4 +- contrib/postgres_fdw/connection.c | 2 +- .../postgres_fdw/expected/postgres_fdw.out | 14 ++-- contrib/postgres_fdw/option.c | 2 +- src/backend/access/transam/xlog.c | 2 +- src/backend/replication/logical/logical.c | 2 +- 9 files changed, 75 insertions(+), 47 deletions(-) diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c index ae7e24ad08c..8e5af5a62f7 100644 --- a/contrib/dblink/dblink.c +++ b/contrib/dblink/dblink.c @@ -113,7 +113,7 @@ static char *generate_relation_name(Relation rel); static void dblink_connstr_check(const char *connstr); static void dblink_security_check(PGconn *conn, remoteConn *rconn); static void dblink_res_error(PGconn *conn, const char *conname, PGresult *res, - const char *dblink_context_msg, bool fail); + bool fail, const char *fmt,...) pg_attribute_printf(5, 6); static char *get_connect_string(const char *servername); static char *escape_param_str(const char *from); static void validate_pkattnums(Relation rel, @@ -441,7 +441,8 @@ dblink_open(PG_FUNCTION_ARGS) res = PQexec(conn, buf.data); if (!res || PQresultStatus(res) != PGRES_COMMAND_OK) { - dblink_res_error(conn, conname, res, "could not open cursor", fail); + dblink_res_error(conn, conname, res, fail, + "while opening cursor \"%s\"", curname); PG_RETURN_TEXT_P(cstring_to_text("ERROR")); } @@ -509,7 +510,8 @@ dblink_close(PG_FUNCTION_ARGS) res = PQexec(conn, buf.data); if (!res || PQresultStatus(res) != PGRES_COMMAND_OK) { - dblink_res_error(conn, conname, res, "could not close cursor", fail); + dblink_res_error(conn, conname, res, fail, + "while closing cursor \"%s\"", curname); PG_RETURN_TEXT_P(cstring_to_text("ERROR")); } @@ -612,8 +614,8 @@ dblink_fetch(PG_FUNCTION_ARGS) (PQresultStatus(res) != PGRES_COMMAND_OK && PQresultStatus(res) != PGRES_TUPLES_OK)) { - dblink_res_error(conn, conname, res, - "could not fetch from cursor", fail); + dblink_res_error(conn, conname, res, fail, + "while fetching from cursor \"%s\"", curname); return (Datum) 0; } else if (PQresultStatus(res) == PGRES_COMMAND_OK) @@ -763,8 +765,8 @@ dblink_record_internal(FunctionCallInfo fcinfo, bool is_async) if (PQresultStatus(res) != PGRES_COMMAND_OK && PQresultStatus(res) != PGRES_TUPLES_OK) { - dblink_res_error(conn, conname, res, - "could not execute query", fail); + dblink_res_error(conn, conname, res, fail, + "while executing query"); /* if fail isn't set, we'll return an empty query result */ } else @@ -1009,8 +1011,8 @@ materializeQueryResult(FunctionCallInfo fcinfo, PGresult *res1 = res; res = NULL; - dblink_res_error(conn, conname, res1, - "could not execute query", fail); + dblink_res_error(conn, conname, res1, fail, + "while executing query"); /* if fail isn't set, we'll return an empty query result */ } else if (PQresultStatus(res) == PGRES_COMMAND_OK) @@ -1438,8 +1440,8 @@ dblink_exec(PG_FUNCTION_ARGS) (PQresultStatus(res) != PGRES_COMMAND_OK && PQresultStatus(res) != PGRES_TUPLES_OK)) { - dblink_res_error(conn, conname, res, - "could not execute command", fail); + dblink_res_error(conn, conname, res, fail, + "while executing command"); /* * and save a copy of the command status string to return as our @@ -1980,7 +1982,7 @@ dblink_fdw_validator(PG_FUNCTION_ARGS) ereport(ERROR, (errcode(ERRCODE_FDW_OUT_OF_MEMORY), errmsg("out of memory"), - errdetail("could not get libpq's default connection options"))); + errdetail("Could not get libpq's default connection options."))); } /* Validate each supplied option. */ @@ -2676,9 +2678,17 @@ dblink_connstr_check(const char *connstr) } } +/* + * Report an error received from the remote server + * + * res: the received error result (will be freed) + * fail: true for ERROR ereport, false for NOTICE + * fmt and following args: sprintf-style format and values for errcontext; + * the resulting string should be worded like "while " + */ static void dblink_res_error(PGconn *conn, const char *conname, PGresult *res, - const char *dblink_context_msg, bool fail) + bool fail, const char *fmt,...) { int level; char *pg_diag_sqlstate = PQresultErrorField(res, PG_DIAG_SQLSTATE); @@ -2691,7 +2701,8 @@ dblink_res_error(PGconn *conn, const char *conname, PGresult *res, char *message_detail; char *message_hint; char *message_context; - const char *dblink_context_conname = "unnamed"; + va_list ap; + char dblink_context_msg[512]; if (fail) level = ERROR; @@ -2720,11 +2731,25 @@ dblink_res_error(PGconn *conn, const char *conname, PGresult *res, if (message_primary == NULL) message_primary = pchomp(PQerrorMessage(conn)); + /* + * Now that we've copied all the data we need out of the PGresult, it's + * safe to free it. We must do this to avoid PGresult leakage. We're + * leaking all the strings too, but those are in palloc'd memory that will + * get cleaned up eventually. + */ if (res) PQclear(res); - if (conname) - dblink_context_conname = conname; + /* + * Format the basic errcontext string. Below, we'll add on something + * about the connection name. That's a violation of the translatability + * guidelines about constructing error messages out of parts, but since + * there's no translation support for dblink, there's no need to worry + * about that (yet). + */ + va_start(ap, fmt); + vsnprintf(dblink_context_msg, sizeof(dblink_context_msg), fmt, ap); + va_end(ap); ereport(level, (errcode(sqlstate), @@ -2732,9 +2757,12 @@ dblink_res_error(PGconn *conn, const char *conname, PGresult *res, errmsg("could not obtain message string for remote error"), message_detail ? errdetail_internal("%s", message_detail) : 0, message_hint ? errhint("%s", message_hint) : 0, - message_context ? errcontext("%s", message_context) : 0, - errcontext("Error occurred on dblink connection named \"%s\": %s.", - dblink_context_conname, dblink_context_msg))); + message_context ? (errcontext("%s", message_context)) : 0, + conname ? + (errcontext("%s on dblink connection named \"%s\"", + dblink_context_msg, conname)) : + (errcontext("%s on unnamed dblink connection", + dblink_context_msg)))); } /* @@ -2769,7 +2797,7 @@ get_connect_string(const char *servername) ereport(ERROR, (errcode(ERRCODE_FDW_OUT_OF_MEMORY), errmsg("out of memory"), - errdetail("could not get libpq's default connection options"))); + errdetail("Could not get libpq's default connection options."))); } /* first gather the server connstr options */ diff --git a/contrib/dblink/expected/dblink.out b/contrib/dblink/expected/dblink.out index 511691e57f6..dbcc6b08dba 100644 --- a/contrib/dblink/expected/dblink.out +++ b/contrib/dblink/expected/dblink.out @@ -155,7 +155,7 @@ WHERE t.a> 7; -- open a cursor with bad SQL and fail_on_error set to false SELECT dblink_open('rmt_foo_cursor','SELECT * FROM foobar',false); NOTICE: relation "foobar" does not exist -CONTEXT: Error occurred on dblink connection named "unnamed": could not open cursor. +CONTEXT: while opening cursor "rmt_foo_cursor" on unnamed dblink connection dblink_open ------------- ERROR @@ -223,7 +223,7 @@ FROM dblink_fetch('rmt_foo_cursor',4) AS t(a int, b text, c text[]); SELECT * FROM dblink_fetch('rmt_foobar_cursor',4,false) AS t(a int, b text, c text[]); NOTICE: cursor "rmt_foobar_cursor" does not exist -CONTEXT: Error occurred on dblink connection named "unnamed": could not fetch from cursor. +CONTEXT: while fetching from cursor "rmt_foobar_cursor" on unnamed dblink connection a | b | c ---+---+--- (0 rows) @@ -238,7 +238,7 @@ SELECT dblink_exec('ABORT'); -- close the wrong cursor SELECT dblink_close('rmt_foobar_cursor',false); NOTICE: cursor "rmt_foobar_cursor" does not exist -CONTEXT: Error occurred on dblink connection named "unnamed": could not close cursor. +CONTEXT: while closing cursor "rmt_foobar_cursor" on unnamed dblink connection dblink_close -------------- ERROR @@ -248,12 +248,12 @@ CONTEXT: Error occurred on dblink connection named "unnamed": could not close c SELECT * FROM dblink_fetch('rmt_foo_cursor',4) AS t(a int, b text, c text[]); ERROR: cursor "rmt_foo_cursor" does not exist -CONTEXT: Error occurred on dblink connection named "unnamed": could not fetch from cursor. +CONTEXT: while fetching from cursor "rmt_foo_cursor" on unnamed dblink connection -- this time, 'cursor "rmt_foo_cursor" not found' as a notice SELECT * FROM dblink_fetch('rmt_foo_cursor',4,false) AS t(a int, b text, c text[]); NOTICE: cursor "rmt_foo_cursor" does not exist -CONTEXT: Error occurred on dblink connection named "unnamed": could not fetch from cursor. +CONTEXT: while fetching from cursor "rmt_foo_cursor" on unnamed dblink connection a | b | c ---+---+--- (0 rows) @@ -316,7 +316,7 @@ FROM dblink('SELECT * FROM foo') AS t(a int, b text, c text[]); SELECT * FROM dblink('SELECT * FROM foobar',false) AS t(a int, b text, c text[]); NOTICE: relation "foobar" does not exist -CONTEXT: Error occurred on dblink connection named "unnamed": could not execute query. +CONTEXT: while executing query on unnamed dblink connection a | b | c ---+---+--- (0 rows) @@ -340,7 +340,7 @@ WHERE a = 11; -- botch a change to some other data SELECT dblink_exec('UPDATE foobar SET f3[2] = ''b99'' WHERE f1 = 11',false); NOTICE: relation "foobar" does not exist -CONTEXT: Error occurred on dblink connection named "unnamed": could not execute command. +CONTEXT: while executing command on unnamed dblink connection dblink_exec ------------- ERROR @@ -400,7 +400,7 @@ SELECT * FROM dblink('myconn','SELECT * FROM foobar',false) AS t(a int, b text, c text[]) WHERE t.a> 7; NOTICE: relation "foobar" does not exist -CONTEXT: Error occurred on dblink connection named "myconn": could not execute query. +CONTEXT: while executing query on dblink connection named "myconn" a | b | c ---+---+--- (0 rows) @@ -437,7 +437,7 @@ SELECT dblink_disconnect('myconn2'); -- open a cursor incorrectly SELECT dblink_open('myconn','rmt_foo_cursor','SELECT * FROM foobar',false); NOTICE: relation "foobar" does not exist -CONTEXT: Error occurred on dblink connection named "myconn": could not open cursor. +CONTEXT: while opening cursor "rmt_foo_cursor" on dblink connection named "myconn" dblink_open ------------- ERROR @@ -523,7 +523,7 @@ SELECT dblink_close('myconn','rmt_foo_cursor'); -- this should fail because there is no open transaction SELECT dblink_exec('myconn','DECLARE xact_test CURSOR FOR SELECT * FROM foo'); ERROR: DECLARE CURSOR can only be used in transaction blocks -CONTEXT: Error occurred on dblink connection named "myconn": could not execute command. +CONTEXT: while executing command on dblink connection named "myconn" -- reset remote transaction state SELECT dblink_exec('myconn','ABORT'); dblink_exec @@ -573,7 +573,7 @@ FROM dblink_fetch('myconn','rmt_foo_cursor',4) AS t(a int, b text, c text[]); SELECT * FROM dblink_fetch('myconn','rmt_foobar_cursor',4,false) AS t(a int, b text, c text[]); NOTICE: cursor "rmt_foobar_cursor" does not exist -CONTEXT: Error occurred on dblink connection named "myconn": could not fetch from cursor. +CONTEXT: while fetching from cursor "rmt_foobar_cursor" on dblink connection named "myconn" a | b | c ---+---+--- (0 rows) @@ -589,7 +589,7 @@ SELECT dblink_exec('myconn','ABORT'); SELECT * FROM dblink_fetch('myconn','rmt_foo_cursor',4) AS t(a int, b text, c text[]); ERROR: cursor "rmt_foo_cursor" does not exist -CONTEXT: Error occurred on dblink connection named "myconn": could not fetch from cursor. +CONTEXT: while fetching from cursor "rmt_foo_cursor" on dblink connection named "myconn" -- close the named persistent connection SELECT dblink_disconnect('myconn'); dblink_disconnect diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index dce3be26179..3df6fc741d8 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -277,7 +277,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("conflicting or redundant options"), - errhint("option \"force_not_null\" supplied more than once for a column"))); + errhint("Option \"force_not_null\" supplied more than once for a column."))); force_not_null = def; /* Don't care what the value is, as long as it's a legal boolean */ (void) defGetBoolean(def); @@ -289,7 +289,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("conflicting or redundant options"), - errhint("option \"force_null\" supplied more than once for a column"))); + errhint("Option \"force_null\" supplied more than once for a column."))); force_null = def; (void) defGetBoolean(def); } diff --git a/contrib/pgcrypto/px.c b/contrib/pgcrypto/px.c index 8ec920224ad..aea8e863af0 100644 --- a/contrib/pgcrypto/px.c +++ b/contrib/pgcrypto/px.c @@ -105,8 +105,8 @@ px_THROW_ERROR(int err) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("generating random data is not supported by this build"), - errdetail("This functionality requires a source of strong random numbers"), - errhint("You need to rebuild PostgreSQL using --enable-strong-random"))); + errdetail("This functionality requires a source of strong random numbers."), + errhint("You need to rebuild PostgreSQL using --enable-strong-random."))); #endif } else diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index 00c926b9830..fe4893a8e05 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -630,7 +630,7 @@ pgfdw_report_error(int elevel, PGresult *res, PGconn *conn, message_detail ? errdetail_internal("%s", message_detail) : 0, message_hint ? errhint("%s", message_hint) : 0, message_context ? errcontext("%s", message_context) : 0, - sql ? errcontext("Remote SQL command: %s", sql) : 0)); + sql ? errcontext("remote SQL command: %s", sql) : 0)); } PG_CATCH(); { diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index a2b13846e0f..2d6e387d631 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -4126,7 +4126,7 @@ FETCH c; SAVEPOINT s; SELECT * FROM ft1 WHERE 1 / (c1 - 1)> 0; -- ERROR ERROR: division by zero -CONTEXT: Remote SQL command: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE (((1 / ("C 1" - 1))> 0)) +CONTEXT: remote SQL command: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE (((1 / ("C 1" - 1))> 0)) ROLLBACK TO s; FETCH c; c1 | c2 | c3 | c4 | c5 | c6 | c7 | c8 @@ -5737,7 +5737,7 @@ ALTER TABLE "S 1"."T 1" ADD CONSTRAINT c2positive CHECK (c2>= 0); INSERT INTO ft1(c1, c2) VALUES(11, 12); -- duplicate key ERROR: duplicate key value violates unique constraint "t1_pkey" DETAIL: Key ("C 1")=(11) already exists. -CONTEXT: Remote SQL command: INSERT INTO "S 1"."T 1"("C 1", c2, c3, c4, c5, c6, c7, c8) VALUES (1,ドル 2,ドル 3,ドル 4,ドル 5,ドル 6,ドル 7,ドル 8ドル) +CONTEXT: remote SQL command: INSERT INTO "S 1"."T 1"("C 1", c2, c3, c4, c5, c6, c7, c8) VALUES (1,ドル 2,ドル 3,ドル 4,ドル 5,ドル 6,ドル 7,ドル 8ドル) INSERT INTO ft1(c1, c2) VALUES(11, 12) ON CONFLICT DO NOTHING; -- works INSERT INTO ft1(c1, c2) VALUES(11, 12) ON CONFLICT (c1, c2) DO NOTHING; -- unsupported ERROR: there is no unique or exclusion constraint matching the ON CONFLICT specification @@ -5746,11 +5746,11 @@ ERROR: there is no unique or exclusion constraint matching the ON CONFLICT spec INSERT INTO ft1(c1, c2) VALUES(1111, -2); -- c2positive ERROR: new row for relation "T 1" violates check constraint "c2positive" DETAIL: Failing row contains (1111, -2, null, null, null, null, ft1 , null). -CONTEXT: Remote SQL command: INSERT INTO "S 1"."T 1"("C 1", c2, c3, c4, c5, c6, c7, c8) VALUES (1,ドル 2,ドル 3,ドル 4,ドル 5,ドル 6,ドル 7,ドル 8ドル) +CONTEXT: remote SQL command: INSERT INTO "S 1"."T 1"("C 1", c2, c3, c4, c5, c6, c7, c8) VALUES (1,ドル 2,ドル 3,ドル 4,ドル 5,ドル 6,ドル 7,ドル 8ドル) UPDATE ft1 SET c2 = -c2 WHERE c1 = 1; -- c2positive ERROR: new row for relation "T 1" violates check constraint "c2positive" DETAIL: Failing row contains (1, -1, 00001_trig_update, 1970年01月02日 08:00:00+00, 1970年01月02日 00:00:00, 1, 1 , foo). -CONTEXT: Remote SQL command: UPDATE "S 1"."T 1" SET c2 = (- c2) WHERE (("C 1" = 1)) +CONTEXT: remote SQL command: UPDATE "S 1"."T 1" SET c2 = (- c2) WHERE (("C 1" = 1)) -- Test savepoint/rollback behavior select c2, count(*) from ft2 where c2 < 500 group by 1 order by 1; c2 | count @@ -5909,7 +5909,7 @@ savepoint s3; update ft2 set c2 = -2 where c2 = 42 and c1 = 10; -- fail on remote side ERROR: new row for relation "T 1" violates check constraint "c2positive" DETAIL: Failing row contains (10, -2, 00010_trig_update_trig_update, 1970-01-11 08:00:00+00, 1970-01-11 00:00:00, 0, 0 , foo). -CONTEXT: Remote SQL command: UPDATE "S 1"."T 1" SET c2 = (-2) WHERE ((c2 = 42)) AND (("C 1" = 10)) +CONTEXT: remote SQL command: UPDATE "S 1"."T 1" SET c2 = (-2) WHERE ((c2 = 42)) AND (("C 1" = 10)) rollback to savepoint s3; select c2, count(*) from ft2 where c2 < 500 group by 1 order by 1; c2 | count @@ -6126,11 +6126,11 @@ RESET constraint_exclusion; INSERT INTO ft1(c1, c2) VALUES(1111, -2); -- c2positive ERROR: new row for relation "T 1" violates check constraint "c2positive" DETAIL: Failing row contains (1111, -2, null, null, null, null, ft1 , null). -CONTEXT: Remote SQL command: INSERT INTO "S 1"."T 1"("C 1", c2, c3, c4, c5, c6, c7, c8) VALUES (1,ドル 2,ドル 3,ドル 4,ドル 5,ドル 6,ドル 7,ドル 8ドル) +CONTEXT: remote SQL command: INSERT INTO "S 1"."T 1"("C 1", c2, c3, c4, c5, c6, c7, c8) VALUES (1,ドル 2,ドル 3,ドル 4,ドル 5,ドル 6,ドル 7,ドル 8ドル) UPDATE ft1 SET c2 = -c2 WHERE c1 = 1; -- c2positive ERROR: new row for relation "T 1" violates check constraint "c2positive" DETAIL: Failing row contains (1, -1, 00001_trig_update, 1970-01-02 08:00:00+00, 1970-01-02 00:00:00, 1, 1 , foo). -CONTEXT: Remote SQL command: UPDATE "S 1"."T 1" SET c2 = (- c2) WHERE (("C 1" = 1)) +CONTEXT: remote SQL command: UPDATE "S 1"."T 1" SET c2 = (- c2) WHERE (("C 1" = 1)) ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c2positive; -- But inconsistent check constraints provide inconsistent results ALTER FOREIGN TABLE ft1 ADD CONSTRAINT ft1_c2negative CHECK (c2 < 0); diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index 082f79ae046..6854f1bd91e 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -196,7 +196,7 @@ InitPgFdwOptions(void) ereport(ERROR, (errcode(ERRCODE_FDW_OUT_OF_MEMORY), errmsg("out of memory"), - errdetail("could not get libpq's default connection options"))); + errdetail("Could not get libpq's default connection options."))); /* Count how many libpq options are available. */ num_libpq_opts = 0; diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 47a6c4d895f..cb9c2a29cb0 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -9328,7 +9328,7 @@ CreateRestartPoint(int flags) ereport((log_checkpoints ? LOG : DEBUG2), (errmsg("recovery restart point at %X/%X", (uint32) (lastCheckPoint.redo>> 32), (uint32) lastCheckPoint.redo), - xtime ? errdetail("last completed transaction was at log time %s", + xtime ? errdetail("Last completed transaction was at log time %s.", timestamptz_to_str(xtime)) : 0)); LWLockRelease(CheckpointLock); diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c index e2e39f45779..3d8ad7ddf82 100644 --- a/src/backend/replication/logical/logical.c +++ b/src/backend/replication/logical/logical.c @@ -417,7 +417,7 @@ CreateDecodingContext(XLogRecPtr start_lsn, ereport(LOG, (errmsg("starting logical decoding for slot \"%s\"", NameStr(slot->data.name)), - errdetail("streaming transactions committing after %X/%X, reading WAL from %X/%X", + errdetail("Streaming transactions committing after %X/%X, reading WAL from %X/%X.", (uint32) (slot->data.confirmed_flush>> 32), (uint32) slot->data.confirmed_flush, (uint32) (slot->data.restart_lsn>> 32), -- 2.39.5

AltStyle によって変換されたページ (->オリジナル) /