On Fri, 8 May 2026 at 17:40, Dilip Kumar <[email protected]> wrote: > > On Fri, May 8, 2026 at 8:28 AM Dilip Kumar <[email protected]> wrote: > > > > On Thu, May 7, 2026 at 5:24 PM Nisha Moond <[email protected]> wrote: > > > > > > > The attached v31 version has the changes to fix this issue by > > > > initializing the variable. > > > > This also has the rebased version along with the rebased version of > > > > the 'Preserve conflict log destination and subscription OID for > > > > subscriptions' patch which is present in the 0005 patch. > > > > > > Thanks for the patches, please find a few comments on the patches 002 to > > > 004: > > > > > > 1) I noticed that if a non-superuser creates the subscription, but a > > > superuser later runs: > > > ALTER SUBSCRIPTION ... SET (conflict_log_table = all) > > > then the conflict table ends up being owned by the superuser instead > > > of the subscription owner. Though, apply_worker would be able to > > > insert into the CLT, but the subscription owner cannot access its > > > associated conflict log table, > > > > > > I think this happens because the heap_create_with_catalog() call uses > > > GetUserId(). It is fine during CREATE SUBSCRIPTION, but during ALTER > > > SUBSCRIPTION, it causes the table to be created under the ALTER > > > command executor’s ownership instead of the subscription owner. > > > > > > Since only the subscription owner or a superuser can run ALTER > > > SUBSCRIPTION, should we always create the table with the subscription > > > owner as the owner? > > > > Yeah that makes sense. > > > > > 2) In GetConflictLogDestAndTable(): > > > + conflictlogrel = table_open(conflictlogrelid, RowExclusiveLock); > > > + if (conflictlogrel == NULL) > > > + elog(ERROR, "could not open conflict log table (OID %u)", > > > + conflictlogrelid); > > > + > > > + return conflictlogrel; > > > > > > I think the "if (conflictlogrel == NULL)" check is unreachable. The > > > table_open()->relation_open() will error-out if it fails to open the > > > relation. > > > > Yeah, that's a valid point. > > > > > 3) Minor typo in create_conflict_log_table() comments: > > > + /* > > > + * Check for an existing table with the sname name in the pg_conflict > > > namespace. > > > + * A collision should not occur under normal operation, but we must > > > handle cases > > > + * where a table has been created manually. > > > + */ > > > ==> double space in "A collision should not" > > > > > > 4) The document patch-0004 is still referring to the old name > > > "pg_conflict_<subid>", it should be "pg_conflict_log_<subid>". > > > > I will fix these in next version. > > > > This fixes all 4 comments Nisha reported. And 0002 is an add-on patch > to allow ownership transfer. I haven't yet changed the clt display > witjh \dRs+ reported by shveta. I have a work-in-progress patch, but > I couldn't get it to work. I will try to debug that tomorrow or next > week whenever I get time. > > Open Items: > - Add comments explaining the reasoning for the ownership change > - change clt display > - Test cases for ownership change, truncation, deletion, and select > from a non-superuser owner of subscriber. > > @vignesh C Your patch needs to be rebased. > Hi Dilip,
I started reviewing the patches. Here are minor comments for 0001 patch: 1. If allow_system_table_mods=on we can add/drop columns of conflict log tables But the same for pg_toast or other catalog tables are prohibited. Also for other system tables we are getting following error. postgres=# ALTER TABLE pg_toast.pg_toast_16413 DROP COLUMN chunk_seq; ERROR: ALTER action DROP COLUMN cannot be performed on relation "pg_toast_16413" DETAIL: This operation is not supported for TOAST tables. postgres=# ALTER TABLE pg_publication DROP COLUMN pubname; ERROR: cannot drop column pubname of table pg_publication because it is required by the database system postgres=# ALTER TABLE pg_description DROP COLUMN description; ERROR: cannot drop column description of table pg_description because it is required by the database system postgres=# ALTER TABLE pg_conflict.pg_conflict_log_16408 DROP COLUMN relname; ALTER TABLE Should we prohibit it for conflict log tables as well? 2. Should we also have a 'dropped conflict log table' NOTICE, when the subscription is dropped? postgres=# CREATE SUBSCRIPTION sub1 connection 'dbname=postgres host=localhost port=5432' publication pub1 WITH (conflict_log_destination = 'TABLE'); NOTICE: created conflict log table "pg_conflict.pg_conflict_log_16394" for subscription "sub1" NOTICE: created replication slot "sub1" on publisher CREATE SUBSCRIPTION postgres=# drop subscription sub1; NOTICE: dropped replication slot "sub1" on publisher DROP SUBSCRIPTION 3. Typo: + /* + * Check for an existing table with the sname name in the pg_conflict namespace. sname -> same Thanks, Shlok Kyal
