Re: Added schema level support for publication. - Mailing list pgsql-hackers

On Tue, Sep 28, 2021 at 8:15 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Mon, Sep 27, 2021 at 12:15 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
>
> Attached v34 patch has the changes for the same.
>

Few comments on v34-0001-Added-schema-level-support-for-publication
==========================================================
1.
+ * This rule parses publication object with and without keyword prefix.

I think we should write it as: "This rule parses publication objects
with and without keyword prefixes."

2.
+ * For the object without keyword prefix, we cannot just use
relation_expr here,
+ * because some extended expression in relation_expr cannot be used as a

/expression/expressions

3.
+/*
+ * Process pubobjspec_list to check for errors in any of the objects and
+ * convert PUBLICATIONOBJ_CONTINUATION into appropriate PublicationObjSpecType
+ * type.

4.
+ /*
+ * Check if setting the relation to a different schema will result in the
+ * publication having schema and same schema's table in the publication.
+ */
+ if (stmt->objectType == OBJECT_TABLE)
+ {
+ ListCell   *lc;
+ List    *schemaPubids = GetSchemaPublications(nspOid);
+ foreach(lc, schemaPubids)
+ {
+ Oid pubid = lfirst_oid(lc);
+ if (list_member_oid(GetPublicationRelations(pubid, PUBLICATION_PART_ALL),
+ relid))
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot move table \"%s\" to schema \"%s\"",
+    RelationGetRelationName(rel), stmt->newschema),
+ errdetail("Altering table will result in having schema \"%s\" and
same schema's table \"%s\" in the publication \"%s\" which is not
supported.",
+   stmt->newschema,
+   RelationGetRelationName(rel),
+   get_publication_name(pubid, false)));
+ }
+ }

Let's slightly change the comment as: "Check that setting the relation
to a different schema won't result in the publication having schema
and the same schema's table." and errdetail as: "The schema \"%s\" and
same schema's table \"%s\" cannot be part of the same publication
\"%s\"."

Maybe it is better to specify that this will disallow the partition table case.

5.
ObjectsInPublicationToOids()
{
..
+ pubobj = (PublicationObjSpec *) lfirst(cell);
+ if (pubobj->pubobjtype == PUBLICATIONOBJ_TABLE)

It is better to keep an empty line between above two lines.

6.
List    *schemaPubids = GetSchemaPublications(nspOid);
foreach(lc, schemaPubids)
..
Oid pubid = lfirst_oid(lc);
if (list_member_oid(GetPublicationRelations(pubid, PUBLICATION_PART_ALL),

Add an empty line between each of the above two lines.

7.
+ /*
+ * Schema lock is held until the publication is altered to prevent
+ * concurrent schema deletion. No need to unlock the schemas, the locks
+ * will be released automatically at the end of alter publication command.
+ */
+ LockSchemaList(schemaidlist);

I think it is better to add a similar comment at other places where
this function is called. And we can shorten the comment atop
LockSchemaList to something like: "The schemas specified in the schema
list are locked in AccessShareLock mode in order to prevent concurrent
schema deletion."

8. In CreatePublication(), the check if (stmt->for_all_tables) can be
the first check and then in else if we can process tables and schemas.

9.
AlterPublication()
{
..
+ /* Lock the publication so nobody else can do anything with it. */
+ LockDatabaseObject(PublicationRelationId, pubform->oid, 0,
+    AccessExclusiveLock);

I think it is better to say why we need this lock. So, can we change
the comment to something like: "Lock the publication so nobody else
can do anything with it. This prevents concurrent alter to add
table(s) that were already going to become part of the publication by
adding corresponding schema(s) via this command and similarly it will
prevent the concurrent addition of schema(s) for which there is any
corresponding table being added by this command."

-- 
With Regards,
Amit Kapila.