RE: No-op updates with partitioning and logical replication started failing in version 13

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