On Thu, May 7, 2026 at 5:24 PM Nisha Moond <nisha.moond412@gmail.com> 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
