Thread: relcache not invalidated when ADD PRIMARY KEY USING INDEX

Hi,

When reviewing some replica identity related patches, I found that when adding
primary key using an existing unique index on not null columns, the
target table's relcache won't be invalidated.

This would cause an error When REPLICA IDENTITY is default and we are
UPDATE/DELETE a published table , because we will refer to
RelationData::rd_pkindex to check if the UPDATE or DELETE can be safety
executed in this case.

---reproduction steps
CREATE TABLE test(a int not null, b int);
CREATE UNIQUE INDEX a ON test(a);
CREATE PUBLICATION PUB for TABLE test;
UPDATE test SET a = 2;
    ERROR:  cannot update table "test" because it does not have a replica identity and publishes updates
    HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.

alter table test add primary key using index a;
UPDATE test SET a = 2;
    ERROR:  cannot update table "test" because it does not have a replica identity and publishes updates
    HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
---

I think the bug exists in HEAD ~ PG11 after the commit(f66e8bf) which remove
relhaspkey from pg_class. In PG10, when adding a primary key, it will always
update the relhaspkey in pg_class which will invalidate the relcache, so it
was OK.

I tried to write a patch to fix this by invalidating the relcache after marking
primary key in index_constraint_create().

Best regards,
Hou zj

Hi,

When reviewing some replica identity related patches, I found that when adding

primary key using an existing unique index on not null columns, the

target table's relcache won't be invalidated.

Hi,

On Mon, Dec 20, 2021 at 06:45:33AM +0000, houzj.fnst@fujitsu.com wrote:
> 
> I tried to write a patch to fix this by invalidating the relcache after marking
> primary key in index_constraint_create().

The cfbot reports that you have a typo in your regression tests:

https://api.cirrus-ci.com/v1/artifact/task/4806573636714496/regress_diffs/contrib/test_decoding/regression.diffs
diff -U3 /tmp/cirrus-ci-build/contrib/test_decoding/expected/ddl.out
/tmp/cirrus-ci-build/contrib/test_decoding/results/ddl.out
--- /tmp/cirrus-ci-build/contrib/test_decoding/expected/ddl.out    2022-01-11 16:46:51.684727957 +0000
+++ /tmp/cirrus-ci-build/contrib/test_decoding/results/ddl.out    2022-01-11 16:50:35.584089784 +0000
@@ -594,7 +594,7 @@
 DELETE FROM table_dropped_index_no_pk WHERE b = 1;
 DELETE FROM table_dropped_index_no_pk WHERE a = 3;
 DROP TABLE table_dropped_index_no_pk;
--- check table with newly added primary key
+-- check tables with newly added primary key

Could you send an updated patch?  As far as I can see the rest of the
regression tests succeed so the patch can probably still be reviewed.  That
being said, there are pending comments by Euler so I think it's appropriate to
zsh:1: command not found: q

"Euler Taveira" <euler@eulerto.com> writes:
> On Mon, Dec 20, 2021, at 3:45 AM, houzj.fnst@fujitsu.com wrote:
>> When reviewing some replica identity related patches, I found that when adding
>> primary key using an existing unique index on not null columns, the
>> target table's relcache won't be invalidated.

> Good catch.

Indeed.

> It seems you can simplify your checking indexForm->indisprimary directly, no?

The logic seems fine to me --- it avoids an unnecessary cache flush
if the index was already the pkey.  (Whether we actually reach this
code in such a case, I dunno, but it's not costing much to be smart
if we are.)

> Why did you add new tests for test_decoding? I think the TAP tests alone are
> fine. BTW, this test is similar to publisher3/subscriber3. Isn't it better to
> use the same pub/sub to reduce the test execution time?

I agree, the proposed test cases are expensive overkill.  The repro
shown in the original message is sufficient and much cheaper.
Moreover, we already have some test cases very much like that in
regress/sql/publication.sql, so that's where I put it.

Pushed with some minor cosmetic adjustments.

            regards, tom lane