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

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