> 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? 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. 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>". -- Thanks, Nisha
