Re: Replica Identity check of partition table on subscriber

On Wed, Jun 8, 2022 at 2:17 PM shiy.fnst@fujitsu.com
<shiy.fnst@fujitsu.com> wrote:
>
> I saw a problem in logical replication, when the target table on subscriber is a
> partitioned table, it only checks whether the Replica Identity of partitioned
> table is consistent with the publisher, and doesn't check Replica Identity of
> the partition.
>
...
>
> It caused an assertion failure on subscriber:
> TRAP: FailedAssertion("OidIsValid(idxoid) || (remoterel->replident == REPLICA_IDENTITY_FULL)", File: "worker.c", Line: 2088, PID: 1616523)
>
> The backtrace is attached.
>
> We got the assertion failure because idxoid is invalid, as table child has no
> Replica Identity or Primary Key. We have a check in check_relation_updatable(),
> but what it checked is table tbl (the parent table) and it passed the check.
>

I can reproduce the problem. This seems to be the problem since commit
f1ac27bf (Add logical replication support to replicate into
partitioned tables), so adding Amit L. and Peter E.

> I think one approach to fix it is to check the target partition in this case,
> instead of the partitioned table.
>

This approach sounds reasonable to me. One minor point:
+/*
+ * Check that replica identity matches.
+ *
+ * We allow for stricter replica identity (fewer columns) on subscriber as
+ * that will not stop us from finding unique tuple. IE, if publisher has
+ * identity (id,timestamp) and subscriber just (id) this will not be a
+ * problem, but in the opposite scenario it will.
+ *
+ * Don't throw any error here just mark the relation entry as not updatable,
+ * as replica identity is only for updates and deletes but inserts can be
+ * replicated even without it.
+ */
+static void
+logicalrep_check_updatable(LogicalRepRelMapEntry *entry)

Can we name this function as logicalrep_rel_mark_updatable as we are
doing that? If so, change the comments as well.

> When trying to fix it, I saw some other problems about updating partition map
> cache.
>
> a) In logicalrep_partmap_invalidate_cb(), the type of the entry in
> LogicalRepPartMap should be LogicalRepPartMapEntry, instead of
> LogicalRepRelMapEntry.
>
> b) In logicalrep_partition_open(), it didn't check if the entry is valid.
>
> c) When the publisher send new relation mapping, only relation map cache will be
> updated, and partition map cache wouldn't. I think it also should be updated
> because it has remote relation information, too.
>

Is there any test case that can show the problem due to your above observations?

-- 
With Regards,
Amit Kapila.