Re: Proposal: Conflict log history table for Logical Replication - Mailing list pgsql-hackers

On Sat, Dec 20, 2025 at 3:17 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, 16 Dec 2025 at 09:54, vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Sun, 14 Dec 2025 at 21:17, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > On Sun, Dec 14, 2025 at 3:51 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > >
> > > > On Fri, Dec 12, 2025 at 3:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > >
> > > > > On Thu, Dec 11, 2025 at 7:49 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > > > >
> > > > > > I was considering the interdependence between the subscription and the
> > > > > > conflict log table (CLT). IMHO, it would be logical to establish the
> > > > > > subscription as dependent on the CLT. This way, if someone attempts to
> > > > > > drop the CLT, the system would recognize the dependency of the
> > > > > > subscription and prevent the drop unless the subscription is removed
> > > > > > first or the CASCADE option is used.
> > > > > >
> > > > > > However, while investigating this, I encountered an error [1] stating
> > > > > > that global objects are not supported in this context. This indicates
> > > > > > that global objects cannot be made dependent on local objects.
> > > > > >
> > > > >
> > > > > What we need here is an equivalent of DEPENDENCY_INTERNAL for database
> > > > > objects. For example, consider following case:
> > > > > postgres=# create table t1(c1 int primary key);
> > > > > CREATE TABLE
> > > > > postgres=# \d+ t1
> > > > >                                            Table "public.t1"
> > > > >  Column |  Type   | Collation | Nullable | Default | Storage |
> > > > > Compression | Stats target | Description
> > > > > --------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
> > > > >  c1     | integer |           | not null |         | plain   |
> > > > >     |              |
> > > > > Indexes:
> > > > >     "t1_pkey" PRIMARY KEY, btree (c1)
> > > > > Publications:
> > > > >     "pub1"
> > > > > Not-null constraints:
> > > > >     "t1_c1_not_null" NOT NULL "c1"
> > > > > Access method: heap
> > > > > postgres=# drop index t1_pkey;
> > > > > ERROR:  cannot drop index t1_pkey because constraint t1_pkey on table
> > > > > t1 requires it
> > > > > HINT:  You can drop constraint t1_pkey on table t1 instead.
> > > > >
> > > > > Here, the PK index is created as part for CREATE TABLE operation and
> > > > > pk_index is not allowed to be dropped independently.
> > > > >
> > > > > > Although making an object dependent on global/shared objects is
> > > > > > possible for certain types of shared objects [2], this is not our main
> > > > > > objective.
> > > > > >
> > > > >
> > > > > As per my understanding from the above example, we need something like
> > > > > that only for shared object subscription and (internally created)
> > > > > table.
> > > >
> > > > Yeah that seems to be exactly what we want, so I tried doing that by
> > > > recording DEPENDENCY_INTERNAL dependency of CLT on subscription[1] and
> > > > it is behaving as we want[2].  And while dropping the subscription or
> > > > altering CLT we can delete internal dependency so that CLT get dropped
> > > > automatically[3]
> > > >
> > > > I will send an updated patch after testing a few more scenarios and
> > > > fixing other pending issues.
> > > >
> > > > [1]
> > > > +       ObjectAddressSet(myself, RelationRelationId, relid);
> > > > +       ObjectAddressSet(subaddr, SubscriptionRelationId, subid);
> > > > +       recordDependencyOn(&myself, &subaddr, DEPENDENCY_INTERNAL);
> > > >
> > > >
> > > > [2]
> > > > postgres[670778]=# DROP TABLE myschema.conflict_log_history2;
> > > > ERROR:  2BP01: cannot drop table myschema.conflict_log_history2
> > > > because subscription sub requires it
> > > > HINT:  You can drop subscription sub instead.
> > > > LOCATION:  findDependentObjects, dependency.c:788
> > > > postgres[670778]=#
> > > >
> > > > [3]
> > > > ObjectAddressSet(object, SubscriptionRelationId, subid);
> > > > performDeletion(&object, DROP_CASCADE
> > > >                            PERFORM_DELETION_INTERNAL |
> > > >                            PERFORM_DELETION_SKIP_ORIGINAL);
> > > >
> > > >
> > >
> > > Here is the patch which implements the dependency and fixes other
> > > comments from Shveta.
> >
> > Thanks for the changes, the new implementation based on dependency
> > creates a cycle while dumping:
> > ./pg_dump -d postgres -f dump1.txt -p 5433
> > pg_dump: warning: could not resolve dependency loop among these items:
> > pg_dump: detail: TABLE conflict  (ID 225 OID 16397)
> > pg_dump: detail: SUBSCRIPTION (ID 3484 OID 16396)
> > pg_dump: detail: POST-DATA BOUNDARY  (ID 3491)
> > pg_dump: detail: TABLE DATA t1  (ID 3485 OID 16384)
> > pg_dump: detail: PRE-DATA BOUNDARY  (ID 3490)
> >
> > This can be seen with a simple subscription with conflict_log_table.
> > This was working fine with the v11 version patch.
>
> The attached v13 patch includes the fix for this issue. In addition,
> it now raises an error when attempting to configure a conflict log
> table that belongs to a temporary schema or is not a permanent
> (persistent) relation.

I have updated the patch and here are changes done
1. Splitted into 2 patches, 0001- for catalog related changes
0002-inserting conflict into the conflict table, Vignesh need to
rebase the dump and upgrade related patch on this latest changes
2. Subscription option changed to conflict_log_destination=(log/table/all/'')
3. For internal processing we will use ConflictLogDest enum whereas
for taking input or storing into catalog we will use string [1].
4. As suggested by Sawada San, if conflict_log_destination is 'table'
we log the information about conflict but don't log the tuple
details[3]

Pending:
1. tap test for conflict insertion
2. Still need to work on caching related changes discussed at [2], so
currently we don't allow conflict log tables to be added to
publication at all and might change this behavior as discussed at [2]
and for that we will need to implement the caching.
3. Need to add conflict insertion test and doc changes.
4. Still need to check on the latest comments from Peter Smith.


[1]
typedef enum ConflictLogDest
{
CONFLICT_LOG_DEST_INVALID = 0,
CONFLICT_LOG_DEST_LOG, /* "log" (default) */
CONFLICT_LOG_DEST_TABLE, /* "table" */
CONFLICT_LOG_DEST_ALL /* "all" */
} ConflictLogDest;

/*
* Array mapping for converting internal enum to string.
*/
static const char *const ConflictLogDestLabels[] = {
[CONFLICT_LOG_DEST_LOG] = "log",
[CONFLICT_LOG_DEST_TABLE] = "table",
[CONFLICT_LOG_DEST_ALL] = "all"
};

[2] https://www.postgresql.org/message-id/CAA4eK1LNjWigHb5YKz2nBwcGQr18WnNZHv3Gyo8GNCshSkAb-A%40mail.gmail.com

[3]
/* Decide what detail to show in server logs. */
if (dest == CONFLICT_LOG_DEST_LOG || dest == CONFLICT_LOG_DEST_ALL)
{
/* Standard reporting with full internal details. */
ereport(elevel,
errcode_apply_conflict(type),
errmsg("conflict detected on relation \"%s.%s\": conflict=%s",
get_namespace_name(RelationGetNamespace(localrel)),
RelationGetRelationName(localrel),
ConflictTypeNames[type]),
errdetail_internal("%s", err_detail.data));
}
else
{
/*
* 'table' only: Report the error msg but omit raw tuple data from
* server logs since it's already captured in the internal table.
*/
ereport(elevel,
errcode_apply_conflict(type),
errmsg("conflict detected on relation \"%s.%s\": conflict=%s",
get_namespace_name(RelationGetNamespace(localrel)),
RelationGetRelationName(localrel),
ConflictTypeNames[type]),
errdetail("Conflict details logged to internal table with OID %u.",
MySubscription->conflictrelid));
}

--
Regards,
Dilip Kumar
Google