On Thu, May 14, 2026 at 12:45 PM vignesh C <[email protected]> wrote:
>
> I have fixed the other comments except this one. I will think more
> about this and reply separately. The attached patch has the changes
> for the rest of the comments. The patch also addresses comments from
> [1].
>
Thanks for the patches. Please find below comments for v34 patch-set.
1) Bug report:
When disable_on_error = true for a subscription, and an ERROR-level
conflict such as insert_exists occurs, the subscription gets disabled
without logging the conflict into the CLT.
patch-001:
2) execMain.c:
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("cannot modify or insert data into conflict log table \"%s\"",
+ RelationGetRelationName(resultRel)),
Is ERRCODE_INSUFFICIENT_PRIVILEGE the right error code here? It gives
the impression that the operation might succeed with higher
privileges. Should we instead use ERRCODE_WRONG_OBJECT_TYPE, similar
to nearby restrictions?
3) No notice is shown when the conflict log table is removed after
changing conflict_log_destination from table/all to log.
Example:
postgres=# alter subscription sub1 set (conflict_log_destination = table);
NOTICE: created conflict log table
"pg_conflict.pg_conflict_log_16400" for subscription "sub1"
ALTER SUBSCRIPTION
postgres=# alter subscription sub1 set (conflict_log_destination = log);
ALTER SUBSCRIPTION
We already show a notice when changing from log to table/all. Should
we add a similar notice as in DROP SUBSCRIPTION for above case?
patch-003:
4) conflict.c: ReportApplyConflict()
+ bool log_dest_clt = false;
+ bool log_dest_logfile;
log_dest_logfile should also be initialized to false, since for dest
== CONFLICT_LOG_DEST_TABLE, it is never assigned.
5) worker_internal.h
extern PGDLLIMPORT List *table_states_not_ready;
+extern XLogRecPtr remote_final_lsn;
+extern TimestampTz remote_commit_ts;
+extern TransactionId remote_xid;
Should these new declarations also use PGDLLIMPORT?
6) worker.c: apply_handle_stream_start()
+ remote_xid = stream_xid;
+ remote_final_lsn = InvalidXLogRecPtr;
+ remote_commit_ts = 0;
+
if (!TransactionIdIsValid(stream_xid))
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
Should the remote_xid assignment be moved after the validity check? We
could move all three assignments below the check.
patch-005:
7) subscriptioncmds.c: DropSubscription()
+ if (OidIsValid(form->subconflictlogrelid))
+ {
+ char *conflictrelname = get_rel_name(form->subconflictlogrelid);
....
"form" is being used here after the tuple it points to has already been deleted:
/* Remove the tuple from catalog. */
CatalogTupleDelete(rel, &tup->t_self);
ReleaseSysCache(tup);
I think form->subconflictlogrelid should be saved beforehand and then
used later, similar to subid.
--
Thanks,
Nisha
