On Tue, Oct 5, 2021 at 6:57 AM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Mon, Oct 4, 2021 at 4:55 AM vignesh C <vignesh21@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 */
Modified
> (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;
Modified
> (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
Modified
> (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
Modified
>
> 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
I have changed the alter table behavior to allow setting it to an
unlogged table to keep the behavior similar to "ALL TABLES"
publication. I have kept the create publication behavior as it is, it
will be similar to "ALL TABLES" publication i.e to allow create
publication even if there are unlogged tables present.
These comments are handled in the v37 patch attached at [1].
[1] - https://www.postgresql.org/message-id/CALDaNm0ON%3D012jGC3oquSVVWTWXhHG0q8yOyRROVGFR9PjWa-g%40mail.gmail.com
Regards,
Vignesh
