On Thu, Jun 25, 2026 at 11:38 AM shveta malik <[email protected]> wrote:
>
> Thanks for v56. A few comments:
>
> 0001:
> 1)
> + * We complain if either the old or new namespaces is a temporary schema,
> + * temporary toast schema, the TOAST schema, or the conflict schema.
> */
> void
> CheckSetNamespace(Oid oldNspOid, Oid nspOid)
>
> Shall we remove this comment change now as the check has been removed?
Yes
> 2)
> + /*
> + * Conflict log tables are managed by the system to record logical
> + * replication conflicts.
> + */
> + if (IsConflictLogTableNamespace(RelationGetNamespace(rel)))
> + ereport(ERROR,
> + (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> + errmsg("cannot lock rows in the conflict log table \"%s\"",
> + RelationGetRelationName(rel))));
>
> postgres=# SELECT * FROM pg_conflict.pg_conflict_log_16387 for UPDATE;
> ERROR: cannot lock rows in the conflict log table "pg_conflict_log_16387"
>
> Why don't we add detail-message here as well, similar to all other cases:
> errdetail("Conflict log tables are system-managed tables for logical
> replication conflicts.")));
It is kept this way as it is more align with all nearby ereports
> patch-0002
> 3)
> + SELECT 'pg_conflict.' || relname INTO tab_name
> + FROM pg_class c JOIN pg_subscription s ON c.relname =
> 'pg_conflict_log_' || s.oid
> + WHERE s.subname = 'regress_conflict_protection_test';
>
> This query runs many times (10-15). Can we save the result into
> clt_name and use it everywhere? This will reduce test-size.
>
> SELECT 'pg_conflict.' || relname AS clt_name
> FROM pg_class c JOIN pg_subscription s ON c.relname =
> 'pg_conflict_log_' || s.oid
> WHERE s.subname = 'regress_conflict_protection_test'
> \gset
>
> DO
> BEGIN
> EXECUTE 'CREATE TABLE public.conflict_child () INHERITS (' ||
> :'clt_name' || ')';
> ....
>
> DO
> BEGIN
> EXECUTE 'ALTER TABLE ' || :'clt_name' || ' RENAME COLUMN relname
> TO new_relname';
> ---
>
> 4)
> We can also add a test for paritioning along with inheritance:
> CREATE TABLE clt_partition PARTITION OF
> pg_conflict.pg_conflict_log_16387 FOR VALUES FROM ('2026-01-01') TO
> ('2027-01-01');
I think we do not need that, because inheritance is possible from any
table but partitions are only allowed from a partitioned table, so
this test might not be necessary now.
--
Regards,
Dilip Kumar
Google
