Re: Added schema level support for publication.

On Wed, Aug 4, 2021 at 4:10 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Aug 3, 2021 at 8:38 PM vignesh C <vignesh21(at)gmail(dot)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.