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? 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. 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. 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. -- With Regards, Amit Kapila.