Re: No-op updates with partitioning and logical replication started failing in version 13 - Mailing list pgsql-bugs

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