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. -- Regards, Dilip Kumar Google
