On Thu, Oct 7, 2021 at 12:39 PM Amit Langote <amitlangot...@gmail.com> wrote: > > On Fri, Sep 17, 2021 at 7:38 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Fri, Sep 17, 2021 at 11:36 AM houzj.f...@fujitsu.com > > <houzj.f...@fujitsu.com> wrote: > > > On Thursday, September 16, 2021 6:05 PM Amit Kapila > > > <amit.kapil...@gmail.com> > > > > > On Tuesday, September 14, 2021 10:41 PM vignesh C > > > > > <vignes...@gmail.com> wrote: > > > > > > On Tue, Sep 7, 2021 at 11:38 AM houzj.f...@fujitsu.com > > > > > > <houzj.f...@fujitsu.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.
