Horton

RE: Replica Identity check of partition table on subscriber

Yu Shi (Fujitsu) <shiy.fnst@fujitsu.com>

View thread View on postgr.es
From: "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>
To: Amit Kapila <amit.kapila16@gmail.com>, Amit Langote <amitlangote09@gmail.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>
Date: 2022-06-10T08:45:04Z
Lists: pgsql-hackers

Attachments

On Thu, June 9, 2022 7:02 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> > 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.
> 

OK. Modified.

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

Please see the following case.

-- publisher
create table tbl (a int primary key, b int);
create publication pub for table tbl;

-- subscriber
create table tbl (a int primary key, b int, c int) partition by range (a);
create table child partition of tbl default;

-- publisher, make cache
insert into tbl values (1,1);
update tbl set a=a+1;
alter table tbl add column c int;
update tbl set c=1 where a=2;

-- subscriber
postgres=# select * from tbl;
 a | b | c
---+---+---
 2 | 1 |
(1 row)

The value of column c updated failed on subscriber.
And after applying the first patch, it would work fine.

I have added this case to the first patch. Also add a test case for the second
patch.

Attach the new patches.

Regards,
Shi yu

Commits

  1. Fix memory leak due to LogicalRepRelMapEntry.attrmap.

  2. Fix stale values in partition map entries on subscribers.

  3. Fix partition table's REPLICA IDENTITY checking on the subscriber.

  4. Fix data inconsistency between publisher and subscriber.

  5. Fix cache look-up failures while applying changes in logical replication.

  6. Add logical replication support to replicate into partitioned tables