On Wed, Nov 19, 2025 at 7:01 AM Peter Smith <[email protected]> wrote: > > Hi Dilip. > > I started to look at this thread. Here are some comments for patch v4-0001.
Thanks Peter for your review, worked on most of the comments for 0001
>
> =====
> GENERAL
>
> 1.
> There's some inconsistency in how this new table is called at different times
> :
> a) "conflict table"
> b) "conflict log table"
> c) "conflict log history table"
> d) "conflict history"
>
> My preference was (b). Making this consistent will have impacts on
> many macros, variables, comments, function names, etc.
Yeah even my preference is b) so used everywhere.
> ~~~
>
> 2.
> What about enhancements to description \dRs+ so the subscription
> conflict log table is displayed?
Done, I have displayed the conflict log table name, not sure shall we
display complete schema qualified name, if so we might need to join
with pg_namespace.
> ~~~
>
> 3.
> What about enhancements to the tab-complete code?
Done
> ======
> src/backend/commands/subscriptioncmds.c
>
> 4.
> #define SUBOPT_MAX_RETENTION_DURATION 0x00008000
> #define SUBOPT_LSN 0x00010000
> #define SUBOPT_ORIGIN 0x00020000
> +#define SUBOPT_CONFLICT_TABLE 0x00030000
>
> Bug? Shouldn't that be 0x00040000.
Yeah, fixed.
> ~~~
>
> 5.
> + char *conflicttable;
> XLogRecPtr lsn;
> } SubOpts;
>
> IMO 'conflicttable' looks too much like 'conflictable', which may
> cause some confusion on first reading.
Changed to conflictlogtable
> ~~~
>
> 6.
> +static void CreateConflictLogTable(Oid namespaceId, char *conflictrel);
> +static void DropConflictLogTable(Oid namespaceId, char *conflictrel);
>
> AFAIK it is more conventional for the static functions to be
> snake_case and the extern functions to use CamelCase. So these would
> be:
> - create_conflict_log_table
> - drop_conflict_log_table
Done
> ~~~
>
> CreateSubscription:
>
> 7.
> + /* If conflict log table name is given than create the table. */
> + if (opts.conflicttable)
> + CreateConflictLogTable(conflict_table_nspid, conflict_table);
> +
>
> typo: /If conflict/If a conflict/
>
> typo: "than"
Fixed
> ~~~
>
> AlterSubscription:
>
> 8.
> - SUBOPT_ORIGIN);
> + SUBOPT_ORIGIN |
> + SUBOPT_CONFLICT_TABLE);
>
> The line wrapping doesn't seem necessary.
Without wrapping it crosses 80 characters per line limit.
> ~~~
>
> 9.
> + replaces[Anum_pg_subscription_subconflictnspid - 1] = true;
> + replaces[Anum_pg_subscription_subconflicttable - 1] = true;
> +
> + CreateConflictLogTable(nspid, relname);
> + }
> +
>
> What are the rules regarding replacing one log table with a different
> log table for the same subscription? I didn't see anything about this
> scenario, nor any test cases.
Added test and updated the code as well, so if we set different log
table, we will drop the old and create new table, however if you set
the same table, just NOTICE will be issued and table will not be
created again.
> ~~~
>
> CreateConflictLogTable:
>
> 10.
> + /*
> + * Check if table with same name already present, if so report an error
> + * as currently we do not support user created table as conflict log
> + * table.
> + */
>
> Is the comment about "user-created table" strictly correct? e.g. Won't
> you encounter the same problem if there are 2 subscriptions trying to
> set the same-named conflict log table?
>
> SUGGESTION
> Report an error if the specified conflict log table already exists.
Done
> ~~~
>
> DropConflictLogTable:
>
> 11.
> + /*
> + * Drop conflict log table if exist, use if exists ensures the command
> + * won't error if the table is already gone.
> + */
>
> The reason for EXISTS was already mentioned in the function comment.
>
> SUGGESTION
> Drop the conflict log table if it exists.
Done
> ======
> src/backend/replication/logical/conflict.c
>
> 12.
> +static Datum TupleTableSlotToJsonDatum(TupleTableSlot *slot);
> +
> +static void InsertConflictLog(Relation rel,
> + TransactionId local_xid,
> + TimestampTz local_ts,
> + ConflictType conflict_type,
> + RepOriginId origin_id,
> + TupleTableSlot *searchslot,
> + TupleTableSlot *localslot,
> + TupleTableSlot *remoteslot);
>
> Same as earlier comment #6 -- isn't it conventional to use snake_case
> for the static function names?
Done
> ~~~
>
> TupleTableSlotToJsonDatum:
>
> 13.
> + * This would be a new internal helper function for logical replication
> + * Needs to handle various data types and potentially TOASTed data
>
> What's this comment about? Something doesn't look quite right.
Hmm, that's bad, fixed.
> ~~~
>
> InsertConflictLog:
>
> 14.
> + /* TODO: proper error code */
> + relid = get_relname_relid(relname, nspid);
> + if (!OidIsValid(relid))
> + elog(ERROR, "conflict log history table does not exists");
> + conflictrel = table_open(relid, RowExclusiveLock);
> + if (conflictrel == NULL)
> + elog(ERROR, "could not open conflict log history table");
>
> 14a.
> What's the TODO comment for? Are you going to replace these elogs?
replaced with ereport
> ~
>
> 14b.
> Typo: "does not exists"
fixed
> ~
>
> 14c.
> An unnecessary double-blank line follows this code fragment.
fixed
> ~~~
>
> 15.
> + /* Populate the values and nulls arrays */
> + attno = 0;
> + values[attno] = ObjectIdGetDatum(RelationGetRelid(rel));
> + attno++;
> +
> + if (TransactionIdIsValid(local_xid))
> + values[attno] = TransactionIdGetDatum(local_xid);
> + else
> + nulls[attno] = true;
> + attno++;
> +
> + if (TransactionIdIsValid(remote_xid))
> + values[attno] = TransactionIdGetDatum(remote_xid);
> + else
> + nulls[attno] = true;
> + attno++;
> +
> + values[attno] = LSNGetDatum(remote_final_lsn);
> + attno++;
> +
> + if (local_ts > 0)
> + values[attno] = TimestampTzGetDatum(local_ts);
> + else
> + nulls[attno] = true;
> + attno++;
> +
> + if (remote_commit_ts > 0)
> + values[attno] = TimestampTzGetDatum(remote_commit_ts);
> + else
> + nulls[attno] = true;
> + attno++;
> +
> + values[attno] =
> + CStringGetTextDatum(get_namespace_name(RelationGetNamespace(rel)));
> + attno++;
> +
> + values[attno] = CStringGetTextDatum(RelationGetRelationName(rel));
> + attno++;
> +
> + values[attno] = CStringGetTextDatum(ConflictTypeNames[conflict_type]);
> + attno++;
> +
> + if (origin_id != InvalidRepOriginId)
> + replorigin_by_oid(origin_id, true, &origin);
> +
> + if (origin != NULL)
> + values[attno] = CStringGetTextDatum(origin);
> + else
> + nulls[attno] = true;
> + attno++;
> +
> + if (replorigin_session_origin != InvalidRepOriginId)
> + replorigin_by_oid(replorigin_session_origin, true, &remote_origin);
> +
> + if (remote_origin != NULL)
> + values[attno] = CStringGetTextDatum(remote_origin);
> + else
> + nulls[attno] = true;
> + attno++;
> +
> + if (searchslot != NULL)
> + values[attno] = TupleTableSlotToJsonDatum(searchslot);
> + else
> + nulls[attno] = true;
> + attno++;
> +
> + if (localslot != NULL)
> + values[attno] = TupleTableSlotToJsonDatum(localslot);
> + else
> + nulls[attno] = true;
> + attno++;
> +
> + if (remoteslot != NULL)
> + values[attno] = TupleTableSlotToJsonDatum(remoteslot);
> + else
> + nulls[attno] = true;
> +
>
> 15a.
> It might be simpler to just post-increment that 'attno' in all the
> assignments and save a dozen lines of code:
> e.g. values[attno++] = ...
Yeah done that
> ~
>
> 15b.
> Also, put a sanity Assert check at the end, like:
> Assert(attno + 1 == MAX_CONFLICT_ATTR_NUM);
Done
>
> ======
> src/backend/utils/cache/lsyscache.c
>
> 16.
> + if (isnull)
> + {
> + ReleaseSysCache(tup);
> + return NULL;
> + }
> +
> + *nspid = subform->subconflictnspid;
> + relname = pstrdup(TextDatumGetCString(datum));
> +
> + ReleaseSysCache(tup);
> +
> + return relname;
>
> It would be tidier to have a single release/return by coding this
> slightly differently.
>
> SUGGESTION:
>
> char *relname = NULL;
> ...
> if (!isnull)
> {
> *nspid = subform->subconflictnspid;
> relname = pstrdup(TextDatumGetCString(datum));
> }
>
> ReleaseSysCache(tup);
> return relname;
Right, changed it.
> ======
> src/include/catalog/pg_subscription.h
>
> 17.
> + Oid subconflictnspid; /* Namespace Oid in which the conflict history
> + * table is created. */
>
> Would it be better to make these 2 new member names more alike, since
> they go together. e.g.
> confl_table_nspid
> confl_table_name
In pg_subscription.h all field follows same convention without "_" so
I have changed to
subconflictlognspid
subconflictlogtable
> ======
> src/include/replication/conflict.h
>
> 18.
> +#define MAX_CONFLICT_ATTR_NUM 15
>
> I felt this doesn't really belong here. Just define it atop/within the
> function InsertConflictLog()
Done
> ~~~
>
> 19.
> extern void InitConflictIndexes(ResultRelInfo *relInfo);
> +
> #endif
>
> Spurious whitespace change not needed for this patch.
Fixed
> ======
> src/test/regress/sql/subscription.sql
>
> 20.
> How about adding some more test scenarios:
> e.g.1. ALTER the conflict log table of some subscription that already has one
> e.g.2. Have multiple subscriptions that specify the same conflict log table
Added
Pending:
1) fixed review comments of 0002 and 0003
2) Need to add replica identity tuple instead of full tuple - reported by Shveta
3) Keeping the logs in case of outer transaction failure by moving log
insertion outside the main transaction - reported by Shveta
--
Regards,
Dilip Kumar
Google
v5-0001-Add-configurable-conflict-log-history-table-for.patch
Description: Binary data
