On Thu, Aug 5, 2021 at 3:54 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Aug 4, 2021 at 4:10 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Tue, Aug 3, 2021 at 8:38 PM vignesh C <vignes...@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
