Re: [BUG] Unexpected action when publishing partition tables

On Fri, Sep 17, 2021 at 7:38 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, Sep 17, 2021 at 11:36 AM houzj(dot)fnst(at)fujitsu(dot)com
> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > On Thursday, September 16, 2021 6:05 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> > > > On Tuesday, September 14, 2021 10:41 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > > > > On Tue, Sep 7, 2021 at 11:38 AM houzj(dot)fnst(at)fujitsu(dot)com <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > > >
> > > > Thanks for the comment.
> > > > Attach new version patches which clean the table at the end.
> > > >
> > >
> > > + * For partitioned table contained in the publication, we must
> > > + * invalidate all partitions contained in the respective partition
> > > + * trees, not just the one explicitly mentioned in the publication.
> > >
> > > Can we slightly change the above comment as: "For the partitioned tables, we
> > > must invalidate all partitions contained in the respective partition hierarchies,
> > > not just the one explicitly mentioned in the publication. This is required
> > > because we implicitly publish the child tables when the parent table is
> > > published."
> > >
> > > Apart from this, the patch looks good to me.
> > >
> > > I think we need to back-patch this till v13. What do you think?
> >
> > I agreed.
> >
> > Attach patches for back-branch, each has passed regression tests and pgindent.
>
> Thanks, your patches look good to me. I'll push them sometime next
> week after Tuesday unless there are any comments.

Thanks Amit, Tang, and Hou for this.

Sorry that I didn't comment on this earlier, but I think either
GetPubPartitionOptionRelations() or InvalidatePublicationRels()
introduced in the commit 4548c76738b should lock the partitions, just
like to the parent partitioned table would be, before invalidating
them. There may be some hazards to invalidating a relation without
locking it.

For example, maybe add a 'lockmode' parameter to
GetPubPartitionOptionRelations() which it passes down to
find_all_inheritors() instead of NoLock as now. And make all sites
except GetPublicationRelations() pass ShareUpdateExclusiveLock for it.
Maybe like the attached.

--
Amit Langote
EDB: http://www.enterprisedb.com