On 2022-Sep-27, houzj.f...@fujitsu.com wrote:
> Thanks for reviewing!
>
> Just in case I misunderstand, it seems you mean the message style[1] is OK,
> right ?
>
> [1]
> errdetail("Column list cannot be specified for a partitioned table when %s is
> false.",
> "publish_via_partition_root")));
Yeah, since you're changing another word in that line, it's ok to move
the parameter line off-string. (If you were only changing the parameter
to %s and there was no message duplication, I would reject the patch as
useless.)
I'm going over that patch now, I have a few other changes as attached,
intend to commit soon.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Ni aún el genio muy grande llegaría muy lejos
si tuviera que sacarlo todo de su propio interior" (Goethe)
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index ba1afc21ac..35a93ad200 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -125,7 +125,8 @@ parse_publication_options(ParseState *pstate,
if (!SplitIdentifierString(publish, ',', &publish_list))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("invalid list syntax for \"publish\" option")));
+ errmsg("invalid list syntax in parameter \"%s\"",
+ "publish")));
/* Process the option list. */
foreach(lc, publish_list)
@@ -143,7 +144,8 @@ parse_publication_options(ParseState *pstate,
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("unrecognized \"publish\" value: \"%s\"", publish_opt)));
+ errmsg("unrecognized value for publication option \"%s\": \"%s\"",
+ "publish", publish_opt)));
}
}
else if (strcmp(defel->defname, "publish_via_partition_root") == 0)
@@ -444,14 +446,18 @@ contain_mutable_or_user_functions_checker(Oid func_id, void *context)
}
/*
- * Check if the node contains any unallowed object. See
+ * Check if the node contains any unallowed object. Subroutine for
* check_simple_rowfilter_expr_walker.
*
- * Returns the error detail message in errdetail_msg for unallowed expressions.
+ * If a disallowed object is found, *errdetail_msg is set to a (possibly
+ * translated) message to use as errdetail. If none, *errdetail_msg is not
+ * modified.
*/
static void
expr_allowed_in_node(Node *node, ParseState *pstate, char **errdetail_msg)
{
+ Assert(*errdetail_msg == NULL);
+
if (IsA(node, List))
{
/*
@@ -590,7 +596,7 @@ check_simple_rowfilter_expr_walker(Node *node, ParseState *pstate)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("invalid publication WHERE expression"),
- errdetail("%s", errdetail_msg),
+ errdetail_internal("%s", errdetail_msg),
parser_errposition(pstate, exprLocation(node))));
return expression_tree_walker(node, check_simple_rowfilter_expr_walker,
@@ -714,7 +720,7 @@ CheckPubRelationColumnList(List *tables, char *pubname, bool publish_schema,
/*
* If the publication doesn't publish changes via the root partitioned
* table, the partition's column list will be used. So disallow using
- * the column list on partitioned table in this case.
+ * a column list on the partitioned table in this case.
*/
if (!pubviaroot &&
pri->relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
@@ -723,7 +729,7 @@ CheckPubRelationColumnList(List *tables, char *pubname, bool publish_schema,
errmsg("cannot use column list for relation \"%s.%s\" in publication \"%s\"",
get_namespace_name(RelationGetNamespace(pri->relation)),
RelationGetRelationName(pri->relation), pubname),
- errdetail("Column list cannot be specified for a partitioned table when %s is false.",
+ errdetail("Column lists cannot be specified for partitioned tables when %s is false.",
"publish_via_partition_root")));
}
}
@@ -1302,7 +1308,7 @@ AlterPublicationSchemas(AlterPublicationStmt *stmt,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("cannot add schema to publication \"%s\"",
stmt->pubname),
- errdetail("Schema cannot be added if any table that specifies column list is already part of the publication."));
+ errdetail("Schemas cannot be added if any tables that specify a column list are already part of the publication."));
ReleaseSysCache(coltuple);
}
