Re: No-op updates with partitioning and logical replication started failing in version 13
От | Peter Smith |
---|---|
Тема | Re: No-op updates with partitioning and logical replication started failing in version 13 |
Дата | |
Msg-id | CAHut+PvknZuB3VFV9sm0zsxGN8B_vFNJg2CMFnuD7sWJ_9h1+A@mail.gmail.com обсуждение исходный текст |
Ответ на | RE: No-op updates with partitioning and logical replication started failing in version 13 ("houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>) |
Ответы |
RE: No-op updates with partitioning and logical replication started failing in version 13
("houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>)
|
Список | pgsql-bugs |
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
В списке pgsql-bugs по дате отправления:
Предыдущее
От: "巨鲸"Дата:
Сообщение: Re: BUG #17580: use pg_terminate_backend to terminate a wal sender process may wait a long time
Следующее
От: Masahiko SawadaДата:
Сообщение: Re: BUG #17580: use pg_terminate_backend to terminate a wal sender process may wait a long time