From: | "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com> |
---|---|
To: | Peter Smith <smithpb2250(at)gmail(dot)com> |
Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Brad Nicholson <brad(dot)nicholson(at)instacart(dot)com>, "pgsql-bugs(at)lists(dot)postgresql(dot)org" <pgsql-bugs(at)lists(dot)postgresql(dot)org>, Amit Langote <amitlangote09(at)gmail(dot)com> |
Subject: | RE: No-op updates with partitioning and logical replication started failing in version 13 |
Date: | 2022-08-15 05:37:20 |
Message-ID: | OS0PR01MB5716C5D947047B98A36C851594689@OS0PR01MB5716.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-bugs |
On Monday, August 15, 2022 11:47 AM Peter Smith <smithpb2250(at)gmail(dot)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
Attachment | Content-Type | Size |
---|---|---|
v3-0001-fix-RI-check-for-partitioned-table.patch | application/octet-stream | 2.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Francisco Olarte | 2022-08-15 11:20:39 | Re: [PATCH] Fix segfault calling PQflush on invalid connection |
Previous Message | Masahiko Sawada | 2022-08-15 04:02:59 | Re: BUG #17580: use pg_terminate_backend to terminate a wal sender process may wait a long time |