Обсуждение: No-op updates with partitioning and logical replication started failing in version 13

On Thursday, August 4, 2022 4:53 AM Brad Nicholson <brad.nicholson@instacart.com>  wrote:
> We've hit an interesting change with table partitioning and logical replication that was introduced in Postgres 13. 
> I've tested this on PG 14.4, 13.7, 12.11, 11.16 and 10.18. These are brew installed binaries on OSX, but we've also 
> seen this on other platforms.
> 
> The error happens under the following conditions:
> - parent table does not have a primary key
> - parent table is part of a logical replication publication
> - a no-op update that does not include the partition key is run against the parent.
> 
> In Postgres versions < 13, the update succeeds with UPDATE 0.
> 
> In Postgres versions >= 13, it fails with:
>  
> ERROR:  cannot update table "t1" because it does not have a replica identity and publishes updates
> HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
> 
> Here is a self contained test case:
> 
> create table t1 (id int, created_at timestamp, dat varchar) partition by range (created_at);
> create table t1_child partition of t1 for values from ( '2022-01-01 00:00:00') to ('2022-01-31 00:00:00');
> insert into t1 (id, dat,created_at) values (1, 'test', '2022-01-02 00:00:00');
> create publication test_pub for all tables;
> update t1 set dat = 'foo1' where id = 1 and 1=0;
> 
> Worth mentioning, the 1=0 is a Rails thing.
> 
> I would expect to see an UPDATE 0 in the newer versions instead of the failure.

Hi,

From the error message, it seems we checked the pub action and replica
identity on the partitioned table ('t1'), but it looks uncommon because we
should only check the replica identity on the leaf partition which we actually
perform DML on.

Here is a tiny patch to fix this.

Best regards,
Hou zj

On Thu, Aug 4, 2022 at 3:00 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Thursday, August 4, 2022 4:53 AM Brad Nicholson <brad.nicholson@instacart.com>  wrote:
> > We've hit an interesting change with table partitioning and logical replication that was introduced in Postgres
13.
> > I've tested this on PG 14.4, 13.7, 12.11, 11.16 and 10.18. These are brew installed binaries on OSX, but we've
also
> > seen this on other platforms.
> >
> > The error happens under the following conditions:
> > - parent table does not have a primary key
> > - parent table is part of a logical replication publication
> > - a no-op update that does not include the partition key is run against the parent.
> >
> > In Postgres versions < 13, the update succeeds with UPDATE 0.
> >
> > In Postgres versions >= 13, it fails with:
> >
> > ERROR:  cannot update table "t1" because it does not have a replica identity and publishes updates
> > HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
> >
> > Here is a self contained test case:
> >
> > create table t1 (id int, created_at timestamp, dat varchar) partition by range (created_at);
> > create table t1_child partition of t1 for values from ( '2022-01-01 00:00:00') to ('2022-01-31 00:00:00');
> > insert into t1 (id, dat,created_at) values (1, 'test', '2022-01-02 00:00:00');
> > create publication test_pub for all tables;
> > update t1 set dat = 'foo1' where id = 1 and 1=0;
> >
> > Worth mentioning, the 1=0 is a Rails thing.
> >
> > I would expect to see an UPDATE 0 in the newer versions instead of the failure.
>
> Hi,
>
> From the error message, it seems we checked the pub action and replica
> identity on the partitioned table ('t1'), but it looks uncommon because we
> should only check the replica identity on the leaf partition which we actually
> perform DML on.
>

I agree with your analysis and fix. Adding Amit L., the author of this
feature to check his views on this issue.

-- 
With Regards,
Amit Kapila.

On Thursday, August 4, 2022 8:19 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Thu, Aug 4, 2022 at 3:00 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Thursday, August 4, 2022 4:53 AM Brad Nicholson
> <brad.nicholson@instacart.com>  wrote:
> > > We've hit an interesting change with table partitioning and logical
> replication that was introduced in Postgres 13.
> > > I've tested this on PG 14.4, 13.7, 12.11, 11.16 and 10.18. These are
> > > brew installed binaries on OSX, but we've also seen this on other platforms.
> > >
> > > The error happens under the following conditions:
> > > - parent table does not have a primary key
> > > - parent table is part of a logical replication publication
> > > - a no-op update that does not include the partition key is run against the
> parent.
> > >
> > > In Postgres versions < 13, the update succeeds with UPDATE 0.
> > >
> > > In Postgres versions >= 13, it fails with:
> > >
> > > ERROR:  cannot update table "t1" because it does not have a replica
> > > identity and publishes updates
> > > HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER
> TABLE.
> > >
> > > Here is a self contained test case:
> > >
> > > create table t1 (id int, created_at timestamp, dat varchar)
> > > partition by range (created_at); create table t1_child partition of
> > > t1 for values from ( '2022-01-01 00:00:00') to ('2022-01-31
> > > 00:00:00'); insert into t1 (id, dat,created_at) values (1, 'test',
> > > '2022-01-02 00:00:00'); create publication test_pub for all tables;
> > > update t1 set dat = 'foo1' where id = 1 and 1=0;
> > >
> > > Worth mentioning, the 1=0 is a Rails thing.
> > >
> > > I would expect to see an UPDATE 0 in the newer versions instead of the
> failure.
> >
> > Hi,
> >
> > From the error message, it seems we checked the pub action and replica
> > identity on the partitioned table ('t1'), but it looks uncommon
> > because we should only check the replica identity on the leaf
> > partition which we actually perform DML on.
> >
> 
> I agree with your analysis and fix.

Thanks. Attach the patch with a new testcase.

Best regards,
Hou zj

Here are some review comment for the v2* patch

(fix is ok, but I think comments/tests can be improved).

======

1. Commit message

1a.
On publisher, we will check if UPDATE or DELETE can be executed with current
replica identity on the table even if it's a partitioned table.

SUGGESTION
The current publisher code checks if UPDATE or DELETE can be executed
with the replica identity of the table even if it's a partitioned
table.

1b.
But we only need to check the replica identity for leaf partition as operations
are performed on leaf partitions. So, fix it by skipping checking the
the replica identity for partitioned table on publisher.

SUGGESTION
In fact, we can skip checking the replica identity for partitioned
tables, because the operations are actually performed on the
partitions (not the partitioned table).

======

2. src/backend/executor/execReplication.c - CheckCmdReplicaIdentity

\+ /*
+ * We only need to check the replica identity for leaf partition as
+ * operations are actually performed on leaf partitions.
+ */
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ return;

Code is OK, but the comment does not strictly match the code, because
if "we only need to check ... partitions" then code would say if
(relkind != PARTITION) return. Also I think "leaf partition" is a
tautology. So I modified the code comment below.

SUGGESTION
Skip checking the replica identity for partitioned tables, because the
operations are actually performed on the partitions (not the
partitioned table).

====

3. src/test/regress/sql/publication.sql

I'm not sure this test is doing everything it needs to do.  AFAICT you
only tested the NO-OP case (as it was reported). But IIUC the point of
the code change is that the RI check should only be done on the
partition. So I think there need to be more test cases like:

i. Partitioned Table has no RI and Partition being updated also has no
RI => should fail
ii. Partitioned Table has no RI but Partition being updated DOES have
RI => should succeed

~~
I added another column 'b' to the partitioned table, and hacked some
extra tests (XXX) to demonstrate what I mean:

-- works despite missing REPLICA IDENTITY, because no actual update happened
UPDATE testpub_parted SET a = 1 WHERE false;
-- should now fail, because parent's publication replicates updates
UPDATE testpub_parted1 SET a = 1;
ERROR:  cannot update table "testpub_parted1" because it does not have
a replica identity and publishes updates
HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
-- XXX expect to fail because partition has no R.I
UPDATE testpub_parted SET b = 'updated' WHERE a = 1;
ERROR:  cannot update table "testpub_parted1" because it does not have
a replica identity and publishes updates
HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
-- XXX give the partition a R.I. and try again - now it should work
ALTER TABLE testpub_parted1 ADD PRIMARY KEY(a);
INSERT INTO testpub_parted VALUES (1, 'one');
SELECT * FROM testpub_parted;
 a |  b
---+-----
 1 | one
(1 row)

UPDATE testpub_parted SET b = 'updated' WHERE a = 1;
SELECT * FROM testpub_parted;
 a |    b
---+---------
 1 | updated
(1 row)

~~~

4. src/test/regress/sql/publication.sql

I found the test case table names are really confusing ('pub' instead
of 'tab'?). Although it is not the fault of this patch, adding more
tests is going to make it worse. Maybe you can fix these names at the
same time as updating the tests?

e.g. testpub_parted  => testtab_parted
e.g. testpub_parted1 => testtab_part1
e.g. testpub_parted2 => testtab_part2

-----
Kind Regards,
Peter Smith.
Fujitsu Australia

On Monday, August 15, 2022 11:47 AM Peter Smith <smithpb2250@gmail.com> wrote:
> 
> Here are some review comment for the v2* patch
> 

Thanks for the comments!

> ======
> 
> 1. Commit message
> 
> 1a.
> On publisher, we will check if UPDATE or DELETE can be executed with current
> replica identity on the table even if it's a partitioned table.
> 
> SUGGESTION
> The current publisher code checks if UPDATE or DELETE can be executed
> with the replica identity of the table even if it's a partitioned
> table.
> 
> 1b.
> But we only need to check the replica identity for leaf partition as operations
> are performed on leaf partitions. So, fix it by skipping checking the
> the replica identity for partitioned table on publisher.
> 
> SUGGESTION
> In fact, we can skip checking the replica identity for partitioned
> tables, because the operations are actually performed on the
> partitions (not the partitioned table).
> 
> ======
> 
> 2. src/backend/executor/execReplication.c - CheckCmdReplicaIdentity
> 
> \+ /*
> + * We only need to check the replica identity for leaf partition as
> + * operations are actually performed on leaf partitions.
> + */
> + if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> + return;
> 
> Code is OK, but the comment does not strictly match the code, because
> if "we only need to check ... partitions" then code would say if
> (relkind != PARTITION) return. Also I think "leaf partition" is a
> tautology. So I modified the code comment below.

I think the "leaf" is necessary as it refer to the partition which is
not a partitioned table at the same time. And it's widely used in
other codes. I improved the comments as suggested and keep
the "leaf" word.

> 
> 3. src/test/regress/sql/publication.sql
> 
> I'm not sure this test is doing everything it needs to do.  AFAICT you
> only tested the NO-OP case (as it was reported). But IIUC the point of
> the code change is that the RI check should only be done on the
> partition. So I think there need to be more test cases like:
> 
> i. Partitioned Table has no RI and Partition being updated also has no
> RI => should fail
> ii. Partitioned Table has no RI but Partition being updated DOES have
> RI => should succeed

I think these two cases are already tested in the same file.
So, I didn't add them again.

> ~~~
> 
> 4. src/test/regress/sql/publication.sql
> 
> I found the test case table names are really confusing ('pub' instead
> of 'tab'?). Although it is not the fault of this patch, adding more
> tests is going to make it worse. Maybe you can fix these names at the
> same time as updating the tests?
> 
> e.g. testpub_parted  => testtab_parted
> e.g. testpub_parted1 => testtab_part1
> e.g. testpub_parted2 => testtab_part2

I'm not sure. "TABLE testpub_xxx" are used in lots of places in this file.
It might be better to write a separate patch to improve them.

Attach the new version patch which improved the comments and
commit messages.

Best regards,
Hou zj

Hi Hou-san, thanks for the updates.

I reviewed the v3-0001 patch.

Now it looks all good to me (but see my feedback on #3 below).

On Mon, Aug 15, 2022 at 3:37 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Monday, August 15, 2022 11:47 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Here are some review comment for the v2* patch
> >
>
> Thanks for the comments!
>
> > ======
> >
> > 1. Commit message
> >
> > 1a.
> > On publisher, we will check if UPDATE or DELETE can be executed with current
> > replica identity on the table even if it's a partitioned table.
> >
> > SUGGESTION
> > The current publisher code checks if UPDATE or DELETE can be executed
> > with the replica identity of the table even if it's a partitioned
> > table.
> >
> > 1b.
> > But we only need to check the replica identity for leaf partition as operations
> > are performed on leaf partitions. So, fix it by skipping checking the
> > the replica identity for partitioned table on publisher.
> >
> > SUGGESTION
> > In fact, we can skip checking the replica identity for partitioned
> > tables, because the operations are actually performed on the
> > partitions (not the partitioned table).
> >
> > ======
> >
> > 2. src/backend/executor/execReplication.c - CheckCmdReplicaIdentity
> >
> > \+ /*
> > + * We only need to check the replica identity for leaf partition as
> > + * operations are actually performed on leaf partitions.
> > + */
> > + if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> > + return;
> >
> > Code is OK, but the comment does not strictly match the code, because
> > if "we only need to check ... partitions" then code would say if
> > (relkind != PARTITION) return. Also I think "leaf partition" is a
> > tautology. So I modified the code comment below.
>
> I think the "leaf" is necessary as it refer to the partition which is
> not a partitioned table at the same time. And it's widely used in
> other codes. I improved the comments as suggested and keep
> the "leaf" word.
>

OK.

> >
> > 3. src/test/regress/sql/publication.sql
> >
> > I'm not sure this test is doing everything it needs to do.  AFAICT you
> > only tested the NO-OP case (as it was reported). But IIUC the point of
> > the code change is that the RI check should only be done on the
> > partition. So I think there need to be more test cases like:
> >
> > i. Partitioned Table has no RI and Partition being updated also has no
> > RI => should fail
> > ii. Partitioned Table has no RI but Partition being updated DOES have
> > RI => should succeed
>
> I think these two cases are already tested in the same file.
> So, I didn't add them again.

(thanks for providing me with the below references offline)

i)
Line:940(publication.sql)
CREATE TABLE pub_testpart1.parent1 (a int) partition by list (a);
CREATE TABLE pub_testpart2.child_parent1 partition of
pub_testpart1.parent1 for values in (1);
INSERT INTO pub_testpart2.child_parent1 values(1);

OK - That does seem to be testing inserting data where neither the
partitioned table, nor the partition have replica identity. OTOH this
is in the scope of testing of different schemas...

ii)
Line:452(publication.sql)
-- column list for partitioned tables has to cover replica identities for
-- all child relations

OK - That does seem to have leaf partitions with PK and a partitioned
table without PK, but OTOH this is in the scope of column list testing

So, I agree with you - it does seem all the cases are covered. But,
IMO this is quite a large file which is too hard to digest all at
once, and because of that I still felt that keeping all related test
cases grouped together (even if that causes some doubling-up of a few
scenarios) would be better for future readability/maintenance than
relying on some distantly scattered tests to cover everything. Anyway,
I leave it for you to decide whether to add more tests or not.

>
> > ~~~
> >
> > 4. src/test/regress/sql/publication.sql
> >
> > I found the test case table names are really confusing ('pub' instead
> > of 'tab'?). Although it is not the fault of this patch, adding more
> > tests is going to make it worse. Maybe you can fix these names at the
> > same time as updating the tests?
> >
> > e.g. testpub_parted  => testtab_parted
> > e.g. testpub_parted1 => testtab_part1
> > e.g. testpub_parted2 => testtab_part2
>
> I'm not sure. "TABLE testpub_xxx" are used in lots of places in this file.
> It might be better to write a separate patch to improve them.
>

Yeah, you are right - this big change should be done separately. I
will keep this idea aside and maybe propose it another day.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Mon, Aug 15, 2022 at 11:07 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> Attach the new version patch which improved the comments and
> commit messages.
>

Thanks, the patch looks good to me. Can you please prepare back branch
patches unless you or someone else sees a reason not to back patch
this?

-- 
With Regards,
Amit Kapila.

On Tuesday, August 16, 2022 2:58 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Mon, Aug 15, 2022 at 11:07 AM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > Attach the new version patch which improved the comments and commit
> > messages.
> >
> 
> Thanks, the patch looks good to me. Can you please prepare back branch
> patches unless you or someone else sees a reason not to back patch this?

OK, here are the patches for HEAD ~ PG13.
I have confirmed that the problem is fixed after applying the patch on both
HEAD And back branches.

Best regards,
Hou zj

On Tue, Aug 16, 2022 at 8:30 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> ii)
> Line:452(publication.sql)
> -- column list for partitioned tables has to cover replica identities for
> -- all child relations
>
> OK - That does seem to have leaf partitions with PK and a partitioned
> table without PK, but OTOH this is in the scope of column list testing
>
> So, I agree with you - it does seem all the cases are covered. But,
> IMO this is quite a large file which is too hard to digest all at
> once, and because of that I still felt that keeping all related test
> cases grouped together (even if that causes some doubling-up of a few
> scenarios) would be better for future readability/maintenance than
> relying on some distantly scattered tests to cover everything. Anyway,
> I leave it for you to decide whether to add more tests or not.
>

I am not sure if repeating tests is a good idea. However, if you find
a better way to group the tests, feel free to propose them separately.

-- 
With Regards,
Amit Kapila.

On Tue, Aug 16, 2022 at 1:19 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Tuesday, August 16, 2022 2:58 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Aug 15, 2022 at 11:07 AM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > > Attach the new version patch which improved the comments and commit
> > > messages.
> > >
> >
> > Thanks, the patch looks good to me. Can you please prepare back branch
> > patches unless you or someone else sees a reason not to back patch this?
>
> OK, here are the patches for HEAD ~ PG13.
> I have confirmed that the problem is fixed after applying the patch on both
> HEAD And back branches.
>

Pushed.

-- 
With Regards,
Amit Kapila.

On Thu, Aug 4, 2022 at 9:19 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Aug 4, 2022 at 3:00 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> > On Thursday, August 4, 2022 4:53 AM Brad Nicholson <brad.nicholson@instacart.com>  wrote:
> > > Here is a self contained test case:
> > >
> > > create table t1 (id int, created_at timestamp, dat varchar) partition by range (created_at);
> > > create table t1_child partition of t1 for values from ( '2022-01-01 00:00:00') to ('2022-01-31 00:00:00');
> > > insert into t1 (id, dat,created_at) values (1, 'test', '2022-01-02 00:00:00');
> > > create publication test_pub for all tables;
> > > update t1 set dat = 'foo1' where id = 1 and 1=0;
> >
> > Hi,
> >
> > From the error message, it seems we checked the pub action and replica
> > identity on the partitioned table ('t1'), but it looks uncommon because we
> > should only check the replica identity on the leaf partition which we actually
> > perform DML on.
> >
>
> I agree with your analysis and fix. Adding Amit L., the author of this
> feature to check his views on this issue.


Sorry for the delay in replying.  The fix sounds fine to me.

I would have thought that CheckValidResultRel() and thus
CheckCmdReplicaIdentity() would only ever get called on leaf partition
result relations (present in ModifyTable.resultRelations) and never on
any partitioned tables, but that is apparently not true in this case.
In this case, all leaf partitions would be pruned by the planner given
the WHERE clause, and in that special case, the planner puts the
partitioned table's RT index into ModifyTable.resultRelations, which
is otherwise never added there (see commit ab5fcf2b04f).

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On Wed, Aug 17, 2022 at 10:11 AM Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Thu, Aug 4, 2022 at 9:19 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Thu, Aug 4, 2022 at 3:00 PM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com> wrote:
> > > On Thursday, August 4, 2022 4:53 AM Brad Nicholson <brad.nicholson@instacart.com>  wrote:
> > > > Here is a self contained test case:
> > > >
> > > > create table t1 (id int, created_at timestamp, dat varchar) partition by range (created_at);
> > > > create table t1_child partition of t1 for values from ( '2022-01-01 00:00:00') to ('2022-01-31 00:00:00');
> > > > insert into t1 (id, dat,created_at) values (1, 'test', '2022-01-02 00:00:00');
> > > > create publication test_pub for all tables;
> > > > update t1 set dat = 'foo1' where id = 1 and 1=0;
> > >
> > > Hi,
> > >
> > > From the error message, it seems we checked the pub action and replica
> > > identity on the partitioned table ('t1'), but it looks uncommon because we
> > > should only check the replica identity on the leaf partition which we actually
> > > perform DML on.
> > >
> >
> > I agree with your analysis and fix. Adding Amit L., the author of this
> > feature to check his views on this issue.
>
>
> Sorry for the delay in replying.  The fix sounds fine to me.
>
> I would have thought that CheckValidResultRel() and thus
> CheckCmdReplicaIdentity() would only ever get called on leaf partition
> result relations (present in ModifyTable.resultRelations) and never on
> any partitioned tables, but that is apparently not true in this case.
> In this case, all leaf partitions would be pruned by the planner given
> the WHERE clause, and in that special case, the planner puts the
> partitioned table's RT index into ModifyTable.resultRelations, which
> is otherwise never added there (see commit ab5fcf2b04f).
>


Thanks for the confirmation and for providing related detailed information.

-- 
With Regards,
Amit Kapila.