On Thu, Sep 16, 2021 at 9:54 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Sep 15, 2021 at 12:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Sep 14, 2021 at 2:08 PM vignesh C <vignesh21@gmail.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?
This works fine without locking even after concurrent drop, I felt
this works because of MVCC.
Regards,
Vignesh
