Re: Added schema level support for publication.

On Wed, Sep 15, 2021 at 12:30 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Sep 14, 2021 at 2:08 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > I have handled this in the patch attached.
> >
>
> 4.
> AlterPublicationSchemas()
> {
> ..
> + /*
> + * If the table option was not specified remove the existing tables
> + * from the publication.
> + */
> + if (!tables)
> + {
> + rels = GetPublicationRelations(pubform->oid, PUBLICATION_PART_ROOT);
> + PublicationDropTables(pubform->oid, rels, false, true);
> + }
> +
> + /* Identify which schemas should be dropped */
> + delschemas = list_difference_oid(oldschemaids, schemaidlist);
> +
> + /* And drop them */
> + PublicationDropSchemas(pubform->oid, delschemas, true);
>
> Here, you have neither locked tables to be dropped nor schemas. I
> think both need to be locked as we do for tables in similar code in
> AlterPublicationTables(). Can you please test via debugger what
> happens if we try to drop without taking lock here and concurrently
> try to drop the actual object? It should give some error. If we decide
> to lock here then we should be able to pass the list of relations to
> PublicationDropTables() instead of Oids which would then obviate the
> need for any change to that function.
>
> Similarly don't we need to lock schemas before dropping them in
> AlterPublicationTables()?
>

I think there is one more similar locking problem.
AlterPublicationSchemas()
{
..
+ if (stmt->action == DEFELEM_ADD)
+ {
+ List *rels;
+
+ rels = GetPublicationRelations(pubform->oid, PUBLICATION_PART_ROOT);
+ RelSchemaIsMemberOfSchemaList(rels, schemaidlist, true);
...
...
}

Here, we don't have a lock on the relation. So, what if the relation
is concurrently dropped after you get the rel list by
GetPublicationRelations?

--
With Regards,
Amit Kapila.