On Thu, Sep 15, 2022 at 6:27 PM houzj.f...@fujitsu.com <houzj.f...@fujitsu.com> wrote: > > Attach the new version patch which added suggested restriction for column list > and merged Vignesh's patch. >
Few comments:
============
1.
static void
-CheckPubRelationColumnList(List *tables, const char *queryString,
+CheckPubRelationColumnList(List *tables, bool publish_schema,
+ const char *queryString,
bool pubviaroot)
It is better to keep bool parameters together at the end.
2.
/*
+ * Disallow using column list if any schema is in the publication.
+ */
+ if (publish_schema)
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot use publication column list for relation \"%s.%s\"",
+ get_namespace_name(RelationGetNamespace(pri->relation)),
+ RelationGetRelationName(pri->relation)),
+ errdetail("Column list cannot be specified if any schema is part of
the publication or specified in the list."));
I think it would be better to explain why we disallow this case.
3.
+ if (!heap_attisnull(coltuple, Anum_pg_publication_rel_prattrs, NULL))
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot add schema to the publication"),
+ errdetail("Schema cannot be added if any table that specifies column
list is already part of the publication"));
A full stop is missing at the end in the errdetail message.
4. I have modified a few comments in the attached. Please check and if
you like the changes then please include those in the next version.
--
With Regards,
Amit Kapila.
change_comments_amit_1.patch
Description: Binary data
