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