Re: Proposal: Conflict log history table for Logical Replication

On Mon, Dec 15, 2025 at 4:02 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, Dec 15, 2025 at 3:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Sun, Dec 14, 2025 at 9:16 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > Here is the patch which implements the dependency and fixes other
> > > comments from Shveta.
> > >
> >
> > +/*
> > + * Check if the specified relation is used as a conflict log table by any
> > + * subscription.
> > + */
> > +bool
> > +IsConflictLogTable(Oid relid)
> > +{
> > + Relation rel;
> > + TableScanDesc scan;
> > + HeapTuple tup;
> > + bool is_clt = false;
> > +
> > + rel = table_open(SubscriptionRelationId, AccessShareLock);
> > + scan = table_beginscan_catalog(rel, 0, NULL);
> > +
> > + while (HeapTupleIsValid(tup = heap_getnext(scan, ForwardScanDirection)))
> >
> > This function has been used at multiple places in the patch, though
> > not in any performance-critical paths, but still, it seems like the
> > impact can be noticeable for a large number of subscriptions. Also, I
> > am not sure it is a good design to scan the entire system table to
> > find whether some other relation is publishable or not. I see below
> > kinds of usages for it:
> >
> > + /* Subscription conflict log tables are not published */
> > + result = is_publishable_class(relid, (Form_pg_class) GETSTRUCT(tuple)) &&
> > + !IsConflictLogTable(relid);
> >
> > In this regard, I see a comment atop is_publishable_class which
> > suggests as follows:
> >
> > The best
> >  * long-term solution may be to add a "relispublishable" bool to pg_class,
> >  * and depend on that instead of OID checks.
> >  */
> > static bool
> > is_publishable_class(Oid relid, Form_pg_class reltuple)
> >
> > I feel that is a good idea for reasons mentioned atop
> > is_publishable_class and for the conflict table. What do you think?
>
> On quick thought, this seems like a good idea and may simplify a
> couple of places.  And might be good for future extension as we can
> mark publishable at individual relation instead of targeting broad
> categories like IsCatalogRelationOid() or checking individual items by
> its Oid.  IMHO this can be done as an individual patch in a separate
> thread, or as a base patch.
>

I prefer to do it in a separate thread, so that it can get some more
attention. But it should be done before the main conflict patch. I
think we can subdivide the main patch into (a) DDL handling,
everything except inserting data into conflict table, (b) inserting
data into conflict table, (c) upgrade handling. That way it will be
easier to review.

--
With Regards,
Amit Kapila.