Re: Added schema level support for publication. - Mailing list pgsql-hackers

On Thu, Aug 5, 2021 at 3:54 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Aug 4, 2021 at 4:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Aug 3, 2021 at 8:38 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > Thanks for reporting this, this is fixed in the v18 patch attached.
> > >
> >
> > I have started looking into this patch and below are some initial comments.
> >
>
> Few more comments:
> ===================
> 1. Do we need the previous column 'puballtables' after adding pubtype
> or pubkind in pg_publication?

I felt this should be retained as old client will still be using puballtables, like in case of old client executing \dRp+  commands.

> 2.
> @@ -224,6 +279,20 @@ CreatePublication(ParseState *pstate,
> CreatePublicationStmt *stmt)
> ..
> + nspcrel = table_open(NamespaceRelationId, ShareUpdateExclusiveLock);
> + PublicationAddSchemas(puboid, schemaoidlist, true, NULL);
> + table_close(nspcrel, ShareUpdateExclusiveLock);
>
> What is the reason for opening and taking a lock on
> NamespaceRelationId? Do you want to avoid dropping the corresponding
> schema during this duration? If so, that is not sufficient because
> what if somebody drops it just before you took lock on
> NamespaceRelationId. I think you need to use LockDatabaseObject to
> avoid dropping the schema and note that it should be unlocked only at
> the end of the transaction similar to what we do for tables. I guess
> you need to add this locking inside the function
> PublicationAddSchemas. Also, it would be good if you can add few
> comments in this part of the code to explain the reason for locking.

Modified.

> 3. The function PublicationAddSchemas is called from multiple places
> in the patch but the locking protection is not there at all places. I
> think if you add locking as suggested in the previous point then it
> should be okay. I think you need similar locking for
> PublicationDropSchemas.

Modified.

> 4.
> @@ -421,16 +537,84 @@ AlterPublicationTables(AlterPublicationStmt
> *stmt, Relation rel,
>   PublicationAddTables(pubid, rels, true, stmt);
>
>   CloseTableList(delrels);
> + if (pubform->pubtype == PUBTYPE_EMPTY)
> + UpdatePublicationTypeTupleValue(rel, tup,
> + Anum_pg_publication_pubtype,
> + PUBTYPE_TABLE);
>   }
>
> At the above and all other similar places, the patch first updates the
> pg_publication after performing the rel/schema operation. Isn't it
> better to first update pg_publication to remain in sync with how
> CreatePublication works? I am not able to see any issue with the way
> you have it in the patch but it is better to keep the code consistent
> across various similar functions to avoid confusion in the future.

Modified.

Thanks for the comments, the changes for the above are available in the v19 patch posted at [1].

[1] - https://www.postgresql.org/message-id/CALDaNm3BMLBpWOSdS3Q2vwpsM%3D0yovpJm8dKHRqNyFpANbrhpw%40mail.gmail.com

Regards,
Vignesh