Re: [BUG] Unexpected action when publishing partition tables

On Mon, Sep 6, 2021 at 1:49 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> From Mon, Sep 6, 2021 3:59 PM tanghy(dot)fnst(at)fujitsu(dot)com <tanghy(dot)fnst(at)fujitsu(dot)com> wrote:
> > I met a problem when using logical replication. Maybe it's a bug in logical
> > replication.
> > When publishing a partition table without replica identity, update
> > or delete operation can be successful in some cases.
> >
> > For example:
> > create table tbl1 (a int) partition by range ( a );
> > create table tbl1_part1 partition of tbl1 for values from (1) to (101);
> > create table tbl1_part2 partition of tbl1 for values from (101) to (200);
> > insert into tbl1 select generate_series(1, 10);
> > delete from tbl1 where a=1;
> > create publication pub for table tbl1;
> > delete from tbl1 where a=2;
> >
> > The last DELETE statement can be executed successfully, but it should report
> > error message about missing a replica identity.
> >
> > I found this problem on HEAD and I could reproduce this problem at PG13 and
> > PG14. (Logical replication of partition table was introduced in PG13.)

Adding Amit L and Peter E who were involved in this work (commit:
17b9e7f9) to see if they have opinions on this matter.

>
> 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()?

--
With Regards,
Amit Kapila.