> 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
