Re: [BUG] Unexpected action when publishing partition tables

On Tue, Sep 7, 2021 at 11:38 AM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> From Tues, Sep 7, 2021 12:02 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > On Mon, Sep 6, 2021 at 1:49 PM houzj(dot)fnst(at)fujitsu(dot)com <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > >
> > > I can reproduce this bug.
> > >
> > > I think the reason is it didn't invalidate all the leaf partitions'
> > > relcache when add a partitioned table to the publication, so the
> > > publication info was not rebuilt.
> > >
> > > The following code only invalidate the target table:
> > > ---
> > > PublicationAddTables
> > > publication_add_relation
> > > /* Invalidate relcache so that publication info is rebuilt. */
> > > CacheInvalidateRelcache(targetrel);
> > > ---
> > >
> > > In addition, this problem can happen in both ADD TABLE, DROP TABLE,
> > > and SET TABLE cases, so we need to invalidate the leaf partitions'
> > > recache in all these cases.
> > >
> >
> > Few comments:
> > =============
> > {
> > @@ -664,7 +673,13 @@ PublicationDropTables(Oid pubid, List *rels, bool
> > missing_ok)
> >
> > ObjectAddressSet(obj, PublicationRelRelationId, prid);
> > performDeletion(&obj, DROP_CASCADE, 0);
> > +
> > + relids = GetPubPartitionOptionRelations(relids, PUBLICATION_PART_LEAF,
> > + relid);
> > }
> > +
> > + /* Invalidate relcache so that publication info is rebuilt. */
> > + InvalidatePublicationRels(relids);
> > }
> >
> > We already register the invalidation for the main table in
> > RemovePublicationRelById which is called via performDeletion. I think it is
> > better to perform invalidation for partitions at that place.
> > Similarly is there a reason for not doing invalidations of partitions in
> > publication_add_relation()?
>
> Thanks for the comment. I originally intended to reduce the number of invalid
> message when add/drop serval tables while each table has lots of partitions which
> could exceed the MAX_RELCACHE_INVAL_MSGS. But that seems a rare case, so ,
> I changed the code as suggested.
>
> Attach new version patches which addressed the comment.

Thanks for fixing this issue. The bug gets fixed by the patch, I did
not find any issues in my testing.
I just had one minor comment:

We could clean the table at the end by calling DROP TABLE testpub_parted2:
+-- still fail, because parent's publication replicates updates
+UPDATE testpub_parted2 SET a = 2;
+ERROR: cannot update table "testpub_parted2" because it does not
have a replica identity and publishes updates
+HINT: To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
+ALTER PUBLICATION testpub_forparted DROP TABLE testpub_parted;
+-- works again, because update is no longer replicated
+UPDATE testpub_parted2 SET a = 2;

Regards,
Vignesh