On Mon, Oct 4, 2021 at 4:55 AM vignesh C <vignes...@gmail.com> wrote: > > Attached v36 patch has the changes for the same. >
I have some comments on the v36-0001 patch: src/backend/commands/publicationcmds.c (1) GetPublicationSchemas() + /* Find all publications associated with the schema */ + pubschsrel = table_open(PublicationNamespaceRelationId, AccessShareLock); I think the comment is not correct. It should say: + /* Find all schemas associated with the publication */ (2) AlterPublicationSchemas I think that a comment should be added for the following lines, something like the comment used in the similar check in AlterPublicationTables(): + if (!schemaidlist && stmt->action != DEFELEM_SET) + return; (3) CheckAlterPublication Minor comment fix suggested: BEFORE: + * Check if relations and schemas can be in given publication and throws AFTER: + * Check if relations and schemas can be in a given publication and throw (4) LockSchemaList() Suggest re-word of comment, to match imperative comment style used elsewhere in this code. BEFORE: + * The schemas specified in the schema list are locked in AccessShareLock mode AFTER: + * Lock the schemas specified in the schema list in AccessShareLock mode src/backend/commands/tablecmds.c (5) Code has been added to prevent a table being set (via ALTER TABLE) to UNLOGGED if it is part of a publication, but I found that I could still add all tables of a schema having a table that is UNLOGGED: postgres=# create schema sch; CREATE SCHEMA postgres=# create unlogged table sch.test(i int); CREATE TABLE postgres=# create publication pub for table sch.test; ERROR: cannot add relation "test" to publication DETAIL: Temporary and unlogged relations cannot be replicated. postgres=# create publication pub for all tables in schema sch; CREATE PUBLICATION Regards, Greg Nancarrow Fujitsu Australia