Thread
-
Re: Proposal: Conflict log history table for Logical Replication
shveta malik <shveta.malik@gmail.com> — 2026-05-11T06:21:19Z
On Fri, May 8, 2026 at 5:40 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Fri, May 8, 2026 at 8:28 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > 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. > > > > This fixes all 4 comments Nisha reported. And 0002 is an add-on patch > to allow ownership transfer. I haven't yet changed the clt display > witjh \dRs+ reported by shveta. I have a work-in-progress patch, but > I couldn't get it to work. I will try to debug that tomorrow or next > week whenever I get time. > > Open Items: > - Add comments explaining the reasoning for the ownership change > - change clt display > - Test cases for ownership change, truncation, deletion, and select > from a non-superuser owner of subscriber. > > @vignesh C Your patch needs to be rebased. > Few comments on 001: 1) + /* + * 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. + */ We can extend the comment to mention 'allow_system_table_mods' otherwise it may be difficult to understand how a table could be created in pg_conflict. Suggestion: ...has been created manually when allow_system_table_mods is ON. 2) + /* Create conflict log table. */ + relid = heap_create_with_catalog(relname, + PG_CONFLICT_NAMESPACE, Post this, it will be good to have sanity check on relid before we start using it. Assert(relid != InvalidOid); 3) Currently the structure of CLT is: +const ConflictLogColumnDef ConflictLogSchema[] = { + { .attname = "relid", .atttypid = OIDOID }, + { .attname = "schemaname", .atttypid = TEXTOID }, + { .attname = "relname", .atttypid = TEXTOID }, + { .attname = "conflict_type", .atttypid = TEXTOID }, + { .attname = "remote_xid", .atttypid = XIDOID }, + { .attname = "remote_commit_lsn",.atttypid = LSNOID }, + { .attname = "remote_commit_ts", .atttypid = TIMESTAMPTZOID }, + { .attname = "remote_origin", .atttypid = TEXTOID }, + { .attname = "replica_identity", .atttypid = JSONOID }, + { .attname = "remote_tuple", .atttypid = JSONOID }, + { .attname = "local_conflicts", .atttypid = JSONARRAYOID } +}; So if user has to delete a conflict from CLT after resolving it, then what is the user-friendly way to do it? IMO, it will be cumbersome (and perhaps error-prone) to write a query with remote_commit_lsn, remote_commit_ts, remote_xid etc in WHERE clause. Do you (or others) think we shall add a log_id column (perhaps a bigint GENERATED ALWAYS AS IDENTITY). This provides a simple, unique identifier so the user can easily target a single row (WHERE log_id = 105) or purge a batch of old conflicts (WHERE log_id < 1000). 4) When querying pg_subscription, I noticed that the two CLT-related fields (subconflictlogrelid and subconflictlogdest) are positioned far apart, making them difficult to track and relate. Do you think we shall have both next to each other. If we do that, that will mean 'subconflictlogdes' coming before 'subconninfo', but is should be fine (IMO), as it will be right next to 'subconflictlogrelid' postgres=# select * from pg_subscription; oid | subdbid | subskiplsn | subname | subowner | subenabled | subbinary | substream | subtwophasestate | subdisableonerr | subpasswordrequired | subrunasowner | subfailover | subretaindeadtuples | submaxretent ion | subretentionactive | subserver | subconflictlogrelid | subconninfo | subslotname | subsynccommit | subwalrcvtimeout | subpublications | subconflictlogdes t | suborigin -------+---------+------------+---------+----------+------------+-----------+-----------+------------------+-----------------+---------------------+---------------+-------------+---------------------+------------- ----+--------------------+-----------+---------------------+-------------------------------------------------------------------+-------------+---------------+------------------+-----------------+------------------ --+----------- 16387 | 5 | 0/00000000 | sub1 | 10 | t | f | p | d | f | t | f | f | f | 0 | f | 0 | 16388 | dbname=postgres host=localhost user=shveta port=5433 | sub1 | off | -1 | {pub1} | all 5) +-- verify subconflictlogdest is 'log' and relid is 0 (InvalidOid) for default case We can mention 'subconflictlogrelid' instead of 'relid' thanks Shveta