Re: Added schema level support for publication.

On Wed, Sep 29, 2021 at 10:46 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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."

Modified

> 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

Modified

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

Modified to remove type as discussed offline.

> 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.

Modified, I did not add the above as we are allowing it.

> 5.
> ObjectsInPublicationToOids()
> {
> ..
> + pubobj = (PublicationObjSpec *) lfirst(cell);
> + if (pubobj->pubobjtype == PUBLICATIONOBJ_TABLE)
>
> It is better to keep an empty line between above two lines.

Modified

> 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.

Modified

> 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."

Modified

> 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.

Modified

> 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."

Modified

These comments are handled in the v35 version patch attached at [1]
[1] - https://www.postgresql.org/message-id/CALDaNm2yJOEPCqR%3DgTMEwveJujH9c9_z4LhKmk2T3vZH7T1DLQ%40mail.gmail.com

Regards,
Vignesh