message style

Started by Alvaro Herreraover 6 years ago4 messages
#1Alvaro Herrera
alvherre@2ndquadrant.com

I have this two message patches that I've been debating with myself
about:

--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1282,7 +1282,7 @@ heap_getnext(TableScanDesc sscan, ScanDirection direction)
 	if (unlikely(sscan->rs_rd->rd_tableam != GetHeapamTableAmRoutine()))
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-				 errmsg("only heap AM is supported")));
+				 errmsg("only heap table access method is supported")));

I think the original is not great, but I'm not sure that the new is much
better either. I think this message says "only AMs that behave using
the heapam routines are supported"; we cannot say use the literal
"heapam" AM name because, as the comment two lines above says, it's
possible to copy the AM with a different name and it would be
acceptable. OTOH maybe this code will not survive for long, so it
doesn't matter much that the message is 100% correct; perhaps we should
just change errmsg() to errmsg_internal() and be done with it.

diff --git a/src/backend/access/table/tableamapi.c b/src/backend/access/table/tableamapi.c
index 0053dc95cab..c8b7598f785 100644
--- a/src/backend/access/table/tableamapi.c
+++ b/src/backend/access/table/tableamapi.c
@@ -103,7 +103,8 @@ check_default_table_access_method(char **newval, void **extra, GucSource source)
 {
 	if (**newval == '\0')
 	{
-		GUC_check_errdetail("default_table_access_method may not be empty.");
+		GUC_check_errdetail("%s may not be empty.",
+							"default_table_access_method");
 		return false;
 	}

My problem here is not really the replacement of the name to %s, but the
"may not be" part of it. We don't use "may not be" anywhere else; most
places seem to use "foo cannot be X" and a small number of other places
use "foo must not be Y". I'm not able to judge which of the two is
better (so change all messages to use that form), or if there's a
semantic difference and if so which one to use in this case.

I do notice that there's no place where we would benefit from changing
the "foo" part to %s --that is, there are no duplicate strings-- but I'd
change it anyway to reduce the chance of typos in translations.

git grep -E 'err[a-z]*\(.* (can|must |may )not be ' -- *.c

contrib/amcheck/verify_nbtree.c: errmsg("index \"%s\" cannot be verified using transaction snapshot",
contrib/fuzzystrmatch/fuzzystrmatch.c: errmsg("output cannot be empty string")));
contrib/jsonb_plpython/jsonb_plpython.c: (errmsg("Python type \"%s\" cannot be transformed to jsonb",
contrib/pg_prewarm/pg_prewarm.c: errmsg("relation cannot be null")));
contrib/pg_prewarm/pg_prewarm.c: (errmsg("prewarm type cannot be null"))));
contrib/pg_prewarm/pg_prewarm.c: (errmsg("relation fork cannot be null"))));
contrib/tcn/tcn.c: errmsg("triggered_change_notification: must not be called with more than one parameter")));
contrib/tsm_system_rows/tsm_system_rows.c: errmsg("sample size must not be negative")));
contrib/tsm_system_time/tsm_system_time.c: errmsg("sample collection time must not be negative")));
src/backend/access/brin/brin.c: errhint("BRIN control functions cannot be executed during recovery.")));
src/backend/access/brin/brin.c: errhint("BRIN control functions cannot be executed during recovery.")));
src/backend/access/common/tupdesc.c: errmsg("column \"%s\" cannot be declared SETOF",
src/backend/access/gin/ginfast.c: errhint("GIN pending list cannot be cleaned up during recovery.")));
src/backend/access/hash/hashinsert.c: errhint("Values larger than a buffer page cannot be indexed.")));
src/backend/access/nbtree/nbtutils.c: errhint("Values larger than 1/3 of a buffer page cannot be indexed.\n"
src/backend/access/spgist/spgdoinsert.c: errhint("Values larger than a buffer page cannot be indexed.")));
src/backend/access/spgist/spgutils.c: errhint("Values larger than a buffer page cannot be indexed.")));
src/backend/access/table/tableamapi.c: GUC_check_errdetail("%s may not be empty.",
src/backend/access/transam/xact.c: errmsg("%s cannot be executed from a function", stmtType)));
src/backend/access/transam/xlog.c: errhint("WAL control functions cannot be executed during recovery.")));
src/backend/access/transam/xlog.c: errhint("WAL control functions cannot be executed during recovery.")));
src/backend/access/transam/xlogfuncs.c: errhint("WAL control functions cannot be executed during recovery.")));
src/backend/access/transam/xlogfuncs.c: errhint("WAL control functions cannot be executed during recovery."))));
src/backend/access/transam/xlogfuncs.c: errhint("WAL control functions cannot be executed during recovery.")));
src/backend/access/transam/xlogfuncs.c: errhint("WAL control functions cannot be executed during recovery.")));
src/backend/access/transam/xlogfuncs.c: errhint("WAL control functions cannot be executed during recovery.")));
src/backend/access/transam/xlogfuncs.c: errhint("%s cannot be executed during recovery.",
src/backend/access/transam/xlogfuncs.c: errhint("%s cannot be executed during recovery.",
src/backend/access/transam/xlogfuncs.c: errmsg("\"wait_seconds\" cannot be negative or equal zero")));
src/backend/catalog/aclchk.c: errmsg("default privileges cannot be set for columns")));
src/backend/catalog/dependency.c: errmsg("constant of the type %s cannot be used here",
src/backend/catalog/heap.c: errmsg("composite type %s cannot be made a member of itself",
src/backend/catalog/index.c: errmsg("primary keys cannot be expressions")));
src/backend/catalog/index.c: errmsg("shared indexes cannot be created after initdb")));
src/backend/catalog/objectaddress.c: errmsg("large object OID may not be null")));
src/backend/catalog/pg_aggregate.c: errmsg("final function with extra arguments must not be declared STRICT")));
src/backend/catalog/pg_aggregate.c: errmsg("combine function with transition type %s must not be declared STRICT",
src/backend/catalog/pg_aggregate.c: errmsg("final function with extra arguments must not be declared STRICT")));
src/backend/catalog/pg_operator.c: errmsg("operator cannot be its own negator or sort operator")));
src/backend/catalog/pg_publication.c: errdetail("System tables cannot be added to publications.")));
src/backend/catalog/pg_publication.c: errmsg("table \"%s\" cannot be replicated",
src/backend/catalog/pg_publication.c: errdetail("Temporary and unlogged relations cannot be replicated.")));
src/backend/commands/aggregatecmds.c: errmsg("aggregate msfunc must not be specified without mstype")));
src/backend/commands/aggregatecmds.c: errmsg("aggregate minvfunc must not be specified without mstype")));
src/backend/commands/aggregatecmds.c: errmsg("aggregate mfinalfunc must not be specified without mstype")));
src/backend/commands/aggregatecmds.c: errmsg("aggregate msspace must not be specified without mstype")));
src/backend/commands/aggregatecmds.c: errmsg("aggregate minitcond must not be specified without mstype")));
src/backend/commands/aggregatecmds.c: errmsg("aggregate transition data type cannot be %s",
src/backend/commands/aggregatecmds.c: errmsg("aggregate transition data type cannot be %s",
src/backend/commands/async.c: errmsg("channel name cannot be empty")));
src/backend/commands/async.c: errhint("The NOTIFY queue cannot be emptied until that process ends its current transaction.")
src/backend/commands/cluster.c: errdetail("%.0f dead row versions cannot be removed yet.\n"
src/backend/commands/collationcmds.c: errmsg("collation \"default\" cannot be copied")));
src/backend/commands/copy.c: errmsg("COPY delimiter cannot be newline or carriage return")));
src/backend/commands/copy.c: errmsg("COPY delimiter cannot be \"%s\"", cstate->delim)));
src/backend/commands/copy.c: errdetail("Generated columns cannot be used in COPY.")));
src/backend/commands/dbcommands.c: errmsg("pg_global cannot be used as default tablespace")));
src/backend/commands/dbcommands.c: errmsg("current database cannot be renamed")));
src/backend/commands/dbcommands.c: errmsg("pg_global cannot be used as default tablespace")));
src/backend/commands/dbcommands.c: errmsg("option \"%s\" cannot be specified with other options",
src/backend/commands/extension.c: errdetail("Extension names must not be empty.")));
src/backend/commands/extension.c: errdetail("Version names must not be empty.")));
src/backend/commands/extension.c: errmsg("parameter \"%s\" cannot be set in a secondary extension control file",
src/backend/commands/extension.c: errmsg("parameter \"%s\" cannot be set in a secondary extension control file",
src/backend/commands/extension.c: errmsg("parameter \"schema\" cannot be specified when \"relocatable\" is true")));
src/backend/commands/functioncmds.c: errmsg("type modifier cannot be specified for shell type \"%s\"",
src/backend/commands/functioncmds.c: errmsg("cast function must not be volatile")));
src/backend/commands/functioncmds.c: errmsg("domain data types must not be marked binary-compatible")));
src/backend/commands/functioncmds.c: errmsg("transform function must not be volatile")));
src/backend/commands/indexcmds.c: errdetail("%s constraints cannot be used when partition keys include expressions.",
src/backend/commands/matview.c: errmsg("CONCURRENTLY cannot be used when the materialized view is not populated")));
src/backend/commands/matview.c: errmsg("CONCURRENTLY and WITH NO DATA options cannot be used together")));
src/backend/commands/opclasscmds.c: errmsg("storage type cannot be different from data type for access method \"%s\"",
src/backend/commands/opclasscmds.c: errmsg("STORAGE cannot be specified in ALTER OPERATOR FAMILY")));
src/backend/commands/operatorcmds.c: errmsg("operator attribute \"%s\" cannot be changed",
src/backend/commands/policy.c: errmsg("WITH CHECK cannot be applied to SELECT or DELETE")));
src/backend/commands/portalcmds.c: errmsg("invalid cursor name: must not be empty")));
src/backend/commands/portalcmds.c: errmsg("invalid cursor name: must not be empty")));
src/backend/commands/portalcmds.c: errmsg("invalid cursor name: must not be empty")));
src/backend/commands/prepare.c: errmsg("invalid statement name: must not be empty")));
src/backend/commands/prepare.c: errmsg("utility statements cannot be prepared")));
src/backend/commands/prepare.c: errmsg("parameter $%d of type %s cannot be coerced to the expected type %s",
src/backend/commands/publicationcmds.c: errdetail("Tables cannot be added to or dropped from FOR ALL TABLES publications.")));
src/backend/commands/sequence.c: errmsg("INCREMENT must not be zero")));
src/backend/commands/sequence.c: errmsg("START value (%s) cannot be less than MINVALUE (%s)",
src/backend/commands/sequence.c: errmsg("START value (%s) cannot be greater than MAXVALUE (%s)",
src/backend/commands/sequence.c: errmsg("RESTART value (%s) cannot be less than MINVALUE (%s)",
src/backend/commands/sequence.c: errmsg("RESTART value (%s) cannot be greater than MAXVALUE (%s)",
src/backend/commands/statscmds.c: errmsg("column \"%s\" cannot be used in statistics because its type %s has no default btree operator class",
src/backend/commands/tablecmds.c: errmsg("foreign key constraint \"%s\" cannot be implemented",
src/backend/commands/tablecmds.c: errmsg("column \"%s\" cannot be cast automatically to type %s",
src/backend/commands/tablecmds.c: errmsg("generation expression for column \"%s\" cannot be cast automatically to type %s",
src/backend/commands/tablecmds.c: errmsg("default for column \"%s\" cannot be cast automatically to type %s",
src/backend/commands/tablecmds.c: errmsg("index \"%s\" cannot be used as replica identity because column %d is a system column",
src/backend/commands/tablecmds.c: errmsg("index \"%s\" cannot be used as replica identity because column \"%s\" is nullable",
src/backend/commands/tablecmds.c: errdetail("Unlogged relations cannot be replicated.")));
src/backend/commands/trigger.c: errmsg("transition tables cannot be specified for triggers with more than one event")));
src/backend/commands/trigger.c: errmsg("transition tables cannot be specified for triggers with column lists")));
src/backend/commands/trigger.c: errmsg("NEW TABLE cannot be specified multiple times")));
src/backend/commands/trigger.c: errmsg("OLD TABLE cannot be specified multiple times")));
src/backend/commands/trigger.c: errmsg("OLD TABLE name and NEW TABLE name cannot be the same")));
src/backend/commands/typecmds.c: errmsg("array element type cannot be %s",
src/backend/commands/typecmds.c: errmsg("check constraints for domains cannot be marked NO INHERIT")));
src/backend/commands/typecmds.c: errmsg("range subtype cannot be %s",
src/backend/commands/user.c: errmsg("current user cannot be dropped")));
src/backend/commands/user.c: errmsg("current user cannot be dropped")));
src/backend/commands/user.c: errmsg("session user cannot be dropped")));
src/backend/commands/user.c: errmsg("role \"%s\" cannot be dropped because some objects depend on it",
src/backend/commands/user.c: errmsg("session user cannot be renamed")));
src/backend/commands/user.c: errmsg("current user cannot be renamed")));
src/backend/commands/user.c: errmsg("column names cannot be included in GRANT/REVOKE ROLE")));
src/backend/commands/vacuum.c: errmsg("%s cannot be executed from VACUUM or ANALYZE",
src/backend/commands/vacuum.c: errmsg("VACUUM option DISABLE_PAGE_SKIPPING cannot be used with FULL")));
src/backend/commands/variable.c: GUC_check_errmsg("SET TRANSACTION ISOLATION LEVEL must not be called in a subtransaction");
src/backend/commands/variable.c: GUC_check_errmsg("SET TRANSACTION [NOT] DEFERRABLE cannot be called within a subtransaction");
src/backend/commands/view.c: errmsg("views cannot be unlogged because they do not have storage")));
src/backend/executor/execExpr.c: errmsg("window function calls cannot be nested")));
src/backend/executor/execExprInterp.c: errdetail("Array with element type %s cannot be "
src/backend/executor/execExprInterp.c: errmsg("array subscript in assignment must not be null")));
src/backend/executor/nodeAgg.c: errmsg("aggregate function calls cannot be nested")));
src/backend/executor/nodeAgg.c: errmsg("combine function with transition type %s must not be declared STRICT",
src/backend/executor/nodeLimit.c: errmsg("OFFSET must not be negative")));
src/backend/executor/nodeLimit.c: errmsg("LIMIT must not be negative")));
src/backend/executor/nodeSamplescan.c: errmsg("TABLESAMPLE parameter cannot be null")));
src/backend/executor/nodeSamplescan.c: errmsg("TABLESAMPLE REPEATABLE parameter cannot be null")));
src/backend/executor/nodeTableFuncscan.c: errmsg("namespace URI must not be null")));
src/backend/executor/nodeTableFuncscan.c: errmsg("row filter expression must not be null")));
src/backend/executor/nodeTableFuncscan.c: errmsg("column filter expression must not be null"),
src/backend/executor/nodeWindowAgg.c: errmsg("frame starting offset must not be null")));
src/backend/executor/nodeWindowAgg.c: errmsg("frame starting offset must not be negative")));
src/backend/executor/nodeWindowAgg.c: errmsg("frame ending offset must not be null")));
src/backend/executor/nodeWindowAgg.c: errmsg("frame ending offset must not be negative")));
src/backend/libpq/be-fsstubs.c: errmsg("requested length cannot be negative")));
src/backend/libpq/be-secure-openssl.c: errmsg("private key file \"%s\" cannot be reloaded because it requires a passphrase",
src/backend/libpq/hba.c: errmsg("list of RADIUS servers cannot be empty"),
src/backend/libpq/hba.c: errmsg("list of RADIUS secrets cannot be empty"),
src/backend/optimizer/plan/initsplan.c: errmsg("%s cannot be applied to the nullable side of an outer join",
src/backend/parser/analyze.c: errmsg("%s cannot be applied to VALUES",
src/backend/parser/analyze.c: errmsg("materialized views may not be defined using bound parameters")));
src/backend/parser/analyze.c: errmsg("materialized views cannot be UNLOGGED")));
src/backend/parser/analyze.c: errmsg("%s cannot be applied to a join",
src/backend/parser/analyze.c: errmsg("%s cannot be applied to a function",
src/backend/parser/analyze.c: errmsg("%s cannot be applied to a table function",
src/backend/parser/analyze.c: errmsg("%s cannot be applied to VALUES",
src/backend/parser/analyze.c: errmsg("%s cannot be applied to a WITH query",
src/backend/parser/analyze.c: errmsg("%s cannot be applied to a named tuplestore",
src/backend/parser/gram.y: errmsg("current database cannot be changed"),
src/backend/parser/gram.y: errmsg("frame start cannot be UNBOUNDED FOLLOWING"),
src/backend/parser/gram.y: errmsg("frame start cannot be UNBOUNDED FOLLOWING"),
src/backend/parser/gram.y: errmsg("frame end cannot be UNBOUNDED PRECEDING"),
src/backend/parser/gram.y: errmsg("%s cannot be used as a role name here",
src/backend/parser/gram.y: errmsg("%s cannot be used as a role name here",
src/backend/parser/gram.y: errmsg("%s constraints cannot be marked DEFERRABLE",
src/backend/parser/gram.y: errmsg("%s constraints cannot be marked DEFERRABLE",
src/backend/parser/gram.y: errmsg("%s constraints cannot be marked NOT VALID",
src/backend/parser/gram.y: errmsg("%s constraints cannot be marked NO INHERIT",
src/backend/parser/parse_agg.c: errmsg("aggregate function calls cannot be nested"),
src/backend/parser/parse_agg.c: errmsg("aggregate function calls cannot be nested"),
src/backend/parser/parse_agg.c: errmsg("window function calls cannot be nested"),
src/backend/parser/parse_clause.c: errmsg("relation \"%s\" cannot be the target of a modifying statement",
src/backend/parser/parse_clause.c: errmsg("WITH ORDINALITY cannot be used with a column definition list"),
src/backend/parser/parse_clause.c: errmsg("column \"%s\" cannot be declared SETOF",
src/backend/parser/parse_coerce.c: errmsg("%s types %s and %s cannot be matched",
src/backend/parser/parse_relation.c: errhint("There is an entry for table \"%s\", but it cannot be referenced from this part of the query.",
src/backend/parser/parse_relation.c: errdetail("There is a WITH item named \"%s\", but it cannot be referenced from this part of the query.",
src/backend/parser/parse_relation.c: errmsg("column \"%s\" cannot be declared SETOF",
src/backend/parser/parse_relation.c: errhint("There is an entry for table \"%s\", but it cannot be referenced from this part of the query.",
src/backend/parser/parse_relation.c: errhint("There is a column named \"%s\" in table \"%s\", but it cannot be referenced from this part of the query.",
src/backend/parser/parse_type.c: errmsg("type modifier cannot be specified for shell type \"%s\"",
src/backend/parser/parse_utilcmd.c: errmsg("specified value cannot be cast to type %s for column \"%s\"",
src/backend/parser/scan.l: errdetail("String constants with Unicode escapes cannot be used when standard_conforming_strings is off."),
src/backend/parser/scan.l: yyerror("Unicode escape values cannot be used for code point values above 007F when the server encoding is not UTF8");
src/backend/parser/scan.l: yyerror("Unicode escape values cannot be used for code point values above 007F when the server encoding is not UTF8");
src/backend/postmaster/bgworker.c: errmsg("background worker \"%s\": parallel workers may not be configured for restart",
src/backend/postmaster/postmaster.c: (errmsg("WAL archival cannot be enabled when wal_level is \"minimal\"")));
src/backend/replication/logical/logical.c: errmsg("logical decoding cannot be used while in recovery")));
src/backend/replication/logical/logicalfuncs.c: errmsg("slot name must not be null")));
src/backend/replication/logical/logicalfuncs.c: errmsg("options array must not be null")));
src/backend/rewrite/rewriteDefine.c: errhint("In particular, the table cannot be involved in any foreign key relationships.")));
src/backend/rewrite/rewriteHandler.c: errmsg("INSERT with ON CONFLICT clause cannot be used with table that has INSERT or UPDATE rules")));
src/backend/rewrite/rewriteHandler.c: errmsg("WITH cannot be used in a query that is rewritten by rules into multiple queries")));
src/backend/storage/lmgr/predicate.c: errmsg("a snapshot-importing transaction must not be READ ONLY DEFERRABLE")));
src/backend/utils/adt/acl.c: errmsg("grant options cannot be granted back to your own grantor")));
src/backend/utils/adt/array_userfuncs.c: errmsg("initial position must not be null")));
src/backend/utils/adt/arrayfuncs.c: errmsg("upper bound cannot be less than lower bound")));
src/backend/utils/adt/arrayfuncs.c: errmsg("upper bound cannot be less than lower bound")));
src/backend/utils/adt/arrayfuncs.c: errmsg("upper bound cannot be less than lower bound")));
src/backend/utils/adt/arrayfuncs.c: errmsg("upper bound cannot be less than lower bound")));
src/backend/utils/adt/arrayfuncs.c: errmsg("dimension array or low bound array cannot be null")));
src/backend/utils/adt/arrayfuncs.c: errmsg("dimension array or low bound array cannot be null")));
src/backend/utils/adt/arrayfuncs.c: errmsg("dimension values cannot be null")));
src/backend/utils/adt/arrayfuncs.c: errmsg("dimension values cannot be null")));
src/backend/utils/adt/date.c: errmsg("TIME(%d)%s precision must not be negative",
src/backend/utils/adt/float.c: errmsg("operand, lower bound, and upper bound cannot be NaN")));
src/backend/utils/adt/genfile.c: errmsg("requested length cannot be negative")));
src/backend/utils/adt/genfile.c: errmsg("requested length cannot be negative")));
src/backend/utils/adt/genfile.c: errmsg("requested length cannot be negative")));
src/backend/utils/adt/geo_ops.c: errmsg("open path cannot be converted to polygon")));
src/backend/utils/adt/json.c: errdetail("\\u0000 cannot be converted to text."),
src/backend/utils/adt/json.c: errdetail("Unicode escape values cannot be used for code point values above 007F when the server encoding is not UTF8."),
src/backend/utils/adt/json.c: errmsg("field name must not be null")));
src/backend/utils/adt/json.c: errmsg("argument %d cannot be null", i + 1),
src/backend/utils/adt/jsonb.c: errmsg("argument %d: key must not be null", i + 1)));
src/backend/utils/adt/jsonb.c: errmsg("field name must not be null")));
src/backend/utils/adt/jsonpath_scan.l: errdetail("\\u0000 cannot be converted to text.")));
src/backend/utils/adt/jsonpath_scan.l: errdetail("Unicode escape values cannot be used for code "
src/backend/utils/adt/misc.c: errdetail("Quoted identifier must not be empty.")));
src/backend/utils/adt/numeric.c: errmsg("start value cannot be NaN")));
src/backend/utils/adt/numeric.c: errmsg("stop value cannot be NaN")));
src/backend/utils/adt/numeric.c: errmsg("step size cannot be NaN")));
src/backend/utils/adt/numeric.c: errmsg("operand, lower bound, and upper bound cannot be NaN")));
src/backend/utils/adt/rangetypes.c: errmsg("range constructor flags argument must not be null")));
src/backend/utils/adt/timestamp.c: errmsg("TIMESTAMP(%d)%s precision must not be negative",
src/backend/utils/adt/timestamp.c: errmsg("timestamp cannot be NaN")));
src/backend/utils/adt/timestamp.c: errmsg("INTERVAL(%d) precision must not be negative",
src/backend/utils/adt/tsvector_op.c: errmsg("configuration column \"%s\" must not be null",
src/backend/utils/adt/varlena.c: errmsg("null values cannot be formatted as an SQL identifier")));
src/backend/utils/adt/xml.c: errdetail("XML processing instruction target name cannot be \"%s\".", target)));
src/backend/utils/adt/xml.c: errmsg("row path filter must not be empty string")));
src/backend/utils/adt/xml.c: errmsg("column path filter must not be empty string")));
src/backend/utils/misc/guc-file.l: errmsg("parameter \"%s\" cannot be changed without restarting the server",
src/backend/utils/misc/guc-file.l: record_config_file_error(psprintf("parameter \"%s\" cannot be changed without restarting the server",
src/backend/utils/misc/guc.c: errmsg("parameter \"%s\" cannot be changed",
src/backend/utils/misc/guc.c: errmsg("parameter \"%s\" cannot be changed without restarting the server",
src/backend/utils/misc/guc.c: errmsg("parameter \"%s\" cannot be changed now",
src/backend/utils/misc/guc.c: errmsg("parameter \"%s\" cannot be set after connection start",
src/backend/utils/misc/guc.c: errmsg("parameter \"%s\" cannot be changed without restarting the server",
src/backend/utils/misc/guc.c: errmsg("parameter \"%s\" cannot be changed without restarting the server",
src/backend/utils/misc/guc.c: errmsg("parameter \"%s\" cannot be changed without restarting the server",
src/backend/utils/misc/guc.c: errmsg("parameter \"%s\" cannot be changed without restarting the server",
src/backend/utils/misc/guc.c: errmsg("parameter \"%s\" cannot be changed without restarting the server",
src/backend/utils/misc/guc.c: errmsg("parameter \"%s\" cannot be changed",
src/backend/utils/misc/guc.c: GUC_check_errdetail("\"temp_buffers\" cannot be changed after any temporary tables have been accessed in the session.");
src/backend/utils/mmgr/portalmem.c: errmsg("portal \"%s\" cannot be run", portal->name)));
src/bin/initdb/initdb.c: pg_log_error("password prompt and password file cannot be specified together");
src/bin/pg_basebackup/pg_basebackup.c: pg_log_error("--no-slot cannot be used with slot name");
src/bin/pg_ctl/pg_ctl.c: write_stderr(_("%s: cannot be run as root\n"
src/bin/pg_dump/pg_dump.c: pg_log_error("options -s/--schema-only and -a/--data-only cannot be used together");
src/bin/pg_dump/pg_dump.c: pg_log_error("options -c/--clean and -a/--data-only cannot be used together");
src/bin/pg_dump/pg_dumpall.c: pg_log_error("option --exclude-database cannot be used together with -g/--globals-only, -r/--roles-only or -t/--tablespaces-only");
src/bin/pg_dump/pg_dumpall.c: pg_log_error("options -g/--globals-only and -r/--roles-only cannot be used together");
src/bin/pg_dump/pg_dumpall.c: pg_log_error("options -g/--globals-only and -t/--tablespaces-only cannot be used together");
src/bin/pg_dump/pg_dumpall.c: pg_log_error("options -r/--roles-only and -t/--tablespaces-only cannot be used together");
src/bin/pg_dump/pg_restore.c: pg_log_error("options -d/--dbname and -f/--file cannot be used together");
src/bin/pg_dump/pg_restore.c: pg_log_error("options -s/--schema-only and -a/--data-only cannot be used together");
src/bin/pg_dump/pg_restore.c: pg_log_error("options -c/--clean and -a/--data-only cannot be used together");
src/bin/pg_dump/pg_restore.c: pg_log_error("options -C/--create and -1/--single-transaction cannot be used together");
src/bin/pg_resetwal/pg_resetwal.c: pg_log_error("transaction ID epoch (-e) must not be -1");
src/bin/pg_resetwal/pg_resetwal.c: pg_log_error("transaction ID (-x) must not be 0");
src/bin/pg_resetwal/pg_resetwal.c: pg_log_error("OID (-o) must not be 0");
src/bin/pg_resetwal/pg_resetwal.c: pg_log_error("multitransaction ID (-m) must not be 0");
src/bin/pg_resetwal/pg_resetwal.c: pg_log_error("oldest multitransaction ID (-m) must not be 0");
src/bin/pg_resetwal/pg_resetwal.c: pg_log_error("multitransaction offset (-O) must not be -1");
src/bin/psql/command.c: pg_log_error("\\pset: csv_fieldsep cannot be a double quote, a newline, or a carriage return");
src/bin/psql/command.c: pg_log_error("\\watch cannot be used with an empty query");
src/bin/psql/common.c: pg_log_error("\\watch cannot be used with an empty query");
src/bin/psql/common.c: pg_log_error("\\watch cannot be used with COPY");
src/pl/plpgsql/src/pl_exec.c: errmsg("GET STACKED DIAGNOSTICS cannot be used outside an exception handler")));
src/pl/plpgsql/src/pl_exec.c: errmsg("lower bound of FOR loop cannot be null")));
src/pl/plpgsql/src/pl_exec.c: errmsg("upper bound of FOR loop cannot be null")));
src/pl/plpgsql/src/pl_exec.c: errmsg("BY value of FOR loop cannot be null")));
src/pl/plpgsql/src/pl_exec.c: errmsg("FOREACH expression must not be null")));
src/pl/plpgsql/src/pl_exec.c: errmsg("FOREACH loop variable must not be of an array type")));
src/pl/plpgsql/src/pl_exec.c: errmsg("RAISE without parameters cannot be used outside an exception handler")));
src/pl/plpgsql/src/pl_exec.c: errmsg("RAISE statement option cannot be null")));
src/pl/plpgsql/src/pl_exec.c: errmsg("null value cannot be assigned to variable \"%s\" declared NOT NULL",
src/pl/plpgsql/src/pl_exec.c: errmsg("null value cannot be assigned to variable \"%s\" declared NOT NULL",
src/pl/plpgsql/src/pl_exec.c: errmsg("array subscript in assignment must not be null")));
src/pl/plpgsql/src/pl_gram.y: errmsg("block label \"%s\" cannot be used in CONTINUE",
src/pl/plpgsql/src/pl_gram.y: errmsg("EXIT cannot be used outside a loop, unless it has a label") :
src/pl/plpgsql/src/pl_gram.y: errmsg("CONTINUE cannot be used outside a loop"),
src/pl/plpgsql/src/pl_gram.y: errmsg("record variable cannot be part of multiple-item INTO list"),
src/pl/plpgsql/src/pl_handler.c: GUC_check_errdetail("Key word \"%s\" cannot be combined with other key words.", tok);
src/pl/plpython/plpy_exec.c: errmsg("returned object cannot be iterated"),
src/pl/tcl/pltcl.c: errmsg("function \"%s\" must not be SECURITY DEFINER",

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#2Andres Freund
andres@anarazel.de
In reply to: Alvaro Herrera (#1)
Re: message style

Hi,

On 2019-04-30 10:58:13 -0400, Alvaro Herrera wrote:

I have this two message patches that I've been debating with myself
about:

--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1282,7 +1282,7 @@ heap_getnext(TableScanDesc sscan, ScanDirection direction)
if (unlikely(sscan->rs_rd->rd_tableam != GetHeapamTableAmRoutine()))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-				 errmsg("only heap AM is supported")));
+				 errmsg("only heap table access method is supported")));

I think the original is not great

Agreed.

but I'm not sure that the new is much
better either. I think this message says "only AMs that behave using
the heapam routines are supported"; we cannot say use the literal
"heapam" AM name because, as the comment two lines above says, it's
possible to copy the AM with a different name and it would be
acceptable.

I'm not sure that's something worth being bothered about - the only
reason to do that is for testing. I don't think that needs to be
refelected in error messages.

OTOH maybe this code will not survive for long, so it
doesn't matter much that the message is 100% correct; perhaps we should
just change errmsg() to errmsg_internal() and be done with it.

I'd suspect some of them will survive for a while. What should a heap
specific pageinspect function do if not called for heap etc?

diff --git a/src/backend/access/table/tableamapi.c b/src/backend/access/table/tableamapi.c
index 0053dc95cab..c8b7598f785 100644
--- a/src/backend/access/table/tableamapi.c
+++ b/src/backend/access/table/tableamapi.c
@@ -103,7 +103,8 @@ check_default_table_access_method(char **newval, void **extra, GucSource source)
{
if (**newval == '\0')
{
-		GUC_check_errdetail("default_table_access_method may not be empty.");
+		GUC_check_errdetail("%s may not be empty.",
+							"default_table_access_method");
return false;
}

My problem here is not really the replacement of the name to %s, but the
"may not be" part of it. We don't use "may not be" anywhere else; most
places seem to use "foo cannot be X" and a small number of other places
use "foo must not be Y". I'm not able to judge which of the two is
better (so change all messages to use that form), or if there's a
semantic difference and if so which one to use in this case.

No idea about what's better here either. I don't think there's an
intentional semantic difference.

Thanks for looking at this!

Greetings,

Andres Freund

#3Robert Haas
robertmhaas@gmail.com
In reply to: Alvaro Herrera (#1)
Re: message style

Replying to myself to resend to the list, since my previous attempt
seems to have been eaten by a grue.

On Tue, Apr 30, 2019 at 12:05 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Apr 30, 2019 at 10:58 AM Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

My problem here is not really the replacement of the name to %s, but the
"may not be" part of it. We don't use "may not be" anywhere else; most
places seem to use "foo cannot be X" and a small number of other places
use "foo must not be Y". I'm not able to judge which of the two is
better (so change all messages to use that form), or if there's a
semantic difference and if so which one to use in this case.

The message style guidelines specifically discourage the use of "may",
IMHO for good reason. "mumble may not be flidgy" could be trying to
tell you that something is impermissible, as in "the children may not
run in the house." But it could also be warning you that something is
doubtful, as in "the children may not receive Easter candy this year
because there is a worldwide chocolate shortage." Sometimes the same
sentence can be read either way, like "this table may not be
truncated," which can mean either that TRUNCATE is going to fail if
run in the future, or that it is unclear whether TRUNCATE was already
run in the past.

As far as "cannot" and "must not" is murkier, but it looks to me as
though we prefer "cannot" to "must not" about 9:1, so most often
"cannot" is the right thing, but not always. The distinction seems to
be that we use "cannot" to talk about things that we are unwilling or
unable to do in the future, whereas "must not" is used to admonish the
user about what has already taken place. Consider:

array must not contain nulls
header key must not contain newlines
cast function must not return a set
interval time zone \"%s\" must not include months or days
function \"%s\" must not be SECURITY DEFINER

vs.

cannot drop %s because %s requires it
cannot PREPARE a transaction that has manipulated logical replication workers
cannot reindex temporary tables of other sessions
cannot inherit from partitioned table \"%s\"

The first set of messages are essentially complaints about the past.
The array shouldn't have contained nulls, but it did! The header key
should not have contained newlines, but it did! The cast function
should not return a set, but it does! Hence, we are sad and are
throwing an error. The second set are statements that we are
unwilling or unable to proceed, but they don't necessarily carry the
connotation that there is a problem already in the past. You've just
asked for something you are not going to get.

I think principle that still leaves some ambiguity. For example, you
could phrase that second of the "cannot" message as "must not try to
PREPARE a transaction that has manipulated logical replication
workers." That's grammatical and everything, but it sounds a bit
accusatory, like the user is in trouble or something. I think that's
probably why we tend to prefer "cannot" in most cases. But sometimes
that would lead to a longer or less informative message. For example,
you can't just change

function \"%s\" must not be SECURITY DEFINER

to

function \"%s\" can not be SECURITY DEFINER

...because the user will rightly respond "well, I guess it can,
because it is." We could say

can not execute security definer functions from PL/Tcl

...but that sucks because we now have no reasonable place to put the
function name. We could say

can not execute security definer function \"%s\" from PL/Tcl

...but that also sucks because now the message only says that this one
particular security definer function cannot be executed, rather than
saying that ALL security definer functions cannot be executed. To
really get there, you'd have to do something like

function "\%s" cannot be executed by PL/Tcl because it is a security
definer function

...which is fine, but kinda long. On the plus side it's more clear
about the source of the error (PL/Tcl) than the current message which
doesn't state that explicitly, so perhaps it's an improvement anyway,
but the general point is that sometimes I think there is no succinct
way of expressing the complaint clearly without using "must not".

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

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

#4Robert Haas
robertmhaas@gmail.com
In reply to: Alvaro Herrera (#1)
Re: message style

While reading another thread that attempted to link to this email, I
discovered that this email never made it to the list archives. I am
trying again.

On Tue, Apr 30, 2019 at 12:05 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Apr 30, 2019 at 10:58 AM Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

My problem here is not really the replacement of the name to %s, but the
"may not be" part of it. We don't use "may not be" anywhere else; most
places seem to use "foo cannot be X" and a small number of other places
use "foo must not be Y". I'm not able to judge which of the two is
better (so change all messages to use that form), or if there's a
semantic difference and if so which one to use in this case.

The message style guidelines specifically discourage the use of "may",
IMHO for good reason. "mumble may not be flidgy" could be trying to
tell you that something is impermissible, as in "the children may not
run in the house." But it could also be warning you that something is
doubtful, as in "the children may not receive Easter candy this year
because there is a worldwide chocolate shortage." Sometimes the same
sentence can be read either way, like "this table may not be
truncated," which can mean either that TRUNCATE is going to fail if
run in the future, or that it is unclear whether TRUNCATE was already
run in the past.

As far as "cannot" and "must not" is murkier, but it looks to me as
though we prefer "cannot" to "must not" about 9:1, so most often
"cannot" is the right thing, but not always. The distinction seems to
be that we use "cannot" to talk about things that we are unwilling or
unable to do in the future, whereas "must not" is used to admonish the
user about what has already taken place. Consider:

array must not contain nulls
header key must not contain newlines
cast function must not return a set
interval time zone \"%s\" must not include months or days
function \"%s\" must not be SECURITY DEFINER

vs.

cannot drop %s because %s requires it
cannot PREPARE a transaction that has manipulated logical replication workers
cannot reindex temporary tables of other sessions
cannot inherit from partitioned table \"%s\"

The first set of messages are essentially complaints about the past.
The array shouldn't have contained nulls, but it did! The header key
should not have contained newlines, but it did! The cast function
should not return a set, but it does! Hence, we are sad and are
throwing an error. The second set are statements that we are
unwilling or unable to proceed, but they don't necessarily carry the
connotation that there is a problem already in the past. You've just
asked for something you are not going to get.

I think principle that still leaves some ambiguity. For example, you
could phrase that second of the "cannot" message as "must not try to
PREPARE a transaction that has manipulated logical replication
workers." That's grammatical and everything, but it sounds a bit
accusatory, like the user is in trouble or something. I think that's
probably why we tend to prefer "cannot" in most cases. But sometimes
that would lead to a longer or less informative message. For example,
you can't just change

function \"%s\" must not be SECURITY DEFINER

to

function \"%s\" can not be SECURITY DEFINER

...because the user will rightly respond "well, I guess it can,
because it is." We could say

can not execute security definer functions from PL/Tcl

...but that sucks because we now have no reasonable place to put the
function name. We could say

can not execute security definer function \"%s\" from PL/Tcl

...but that also sucks because now the message only says that this one
particular security definer function cannot be executed, rather than
saying that ALL security definer functions cannot be executed. To
really get there, you'd have to do something like

function "\%s" cannot be executed by PL/Tcl because it is a security
definer function

...which is fine, but kinda long. On the plus side it's more clear
about the source of the error (PL/Tcl) than the current message which
doesn't state that explicitly, so perhaps it's an improvement anyway,
but the general point is that sometimes I think there is no succinct
way of expressing the complaint clearly without using "must not".

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

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