Re: Added schema level support for publication.

On Mon, Oct 4, 2021 at 4:55 AM vignesh C <vignesh21(at)gmail(dot)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