RE: Added schema level support for publication.

On Tuesday, September 14, 2021 4:39 PM vignesh C <vignesh21@gmail.com> wrote:
> 
> I have handled this in the patch attached.

Thanks for updating the patch.
Here are some comments.

1)
+static void
+AlterPublicationSchemas(AlterPublicationStmt *stmt, Relation rel,
...
+        /*
+         * If the table option was not specified remove the existing tables
+         * from the publication.
+         */
+        if (!tables)
+        {
+            rels = GetPublicationRelations(pubform->oid, PUBLICATION_PART_ROOT);
+            PublicationDropTables(pubform->oid, rels, false, true);
+        }


It seems not natural to drop tables in AlterPublication*Schemas*,
I think we'd better do it in AlterPublicationTables.

2)
 static void
 AlterPublicationTables(AlterPublicationStmt *stmt, Relation rel,
 ...
+            /*
+             * If ALL TABLES IN SCHEMA option was not specified remove the
+             * existing schemas from the publication.
+             */
+            List *pubschemas = GetPublicationSchemas(pubid);
+            PublicationDropSchemas(pubform->oid, pubschemas, false);

Same as 1), Is it better to modify the schema list in AlterPublicationSchemas ?


3)
 static void
 AlterPublicationTables(AlterPublicationStmt *stmt, Relation rel,
...
        /* check if the relation is member of the schema list specified */
        RelSchemaIsMemberOfSchemaList(rels, schemaidlist, false);

IIRC, The check here is to check the specified tables and schemas in the
command. Personally, this seems a common operation which can be placed in
function AlterPublication(). If we move this check to AlterPublication() and if
comment 1) and 2) makes sense to you, then we don't need the new function
parameters in AlterPublicationTables() and AlterPublicationSchemas().


Best regards,
Hou zj