Re: [BUG] Unexpected action when publishing partition tables

On Thu, Oct 7, 2021 at 12:39 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> 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.
>

I see your point but then on the same lines didn't the existing code
"for all tables" case (where we call CacheInvalidateRelcacheAll()
without locking all relations) have a similar problem. Also, in your
patch, you are assuming that the callers of GetPublicationRelations()
will lock all the relations but what when it gets called from
AlterPublicationOptions()?

--
With Regards,
Amit Kapila.