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

On Tue, Jan 13, 2026 at 5:02 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Tue, Jan 13, 2026 at 4:09 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Tue, Jan 13, 2026 at 3:59 PM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > On Sat, Jan 10, 2026 at 6:45 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > >
> > > >>
> > > > Here is the updated patch
> > > >
> > >
> > > Thanks, I will review the code. Few concerns while experimenting with 001 alone:
> > >
> > > 1)
> > > I am able to insert and update rows in pg_conflict.pg_conflict_16394.
> > > It should be restricted.
> > >
> > > 2)
> > > I think we need to block 'SELECT for UPDATE' and similar operations on
> > > CLT. Currently it is allowed on CLT.
> > > See this:
> > >
> > > postgres=# SELECT * FROM  pg_toast.pg_toast_3466 for UPDATE;
> > > ERROR:  cannot lock rows in TOAST relation "pg_toast_3466"
> > > postgres=# SELECT * FROM pg_conflict.pg_conflict_16394 for UPDATE;
> > > ....
> >
> > I sent an analysis on this a few days ago but realized it only went to
> > Amit. Resending to the full list:
> >
> > So the summary is, currently, all INSERT/UPDATE/DELETE operations are
> > blocked for TOAST tables for safety. However, I have allowed these
> > operations for the pg-conflict table. Unlike TOAST, the system does
> > not internally reference conflict data, so user manipulation doesn't
> > pose a safety risk to the system. If a user modifies or deletes this
> > data, they simply lose their logs without impacting system stability.
> > What are your thoughts on this approach?
> >
>
> I don’t see a strong reason for a user to manually perform INSERT or
> UPDATE here. But on rethinking, I also agree that allowing it does no
> harm. It simply gives the user flexibility to modify data they “own”,
> i.e., data that the system does not internally reference. Personally,
> I’m okay with leaving it unrestricted, but it will be good to document
> it as a NOTE/CAUTION, stating that these DML operations are allowed
> and it is the user’s responsibility to manage any changes they make
> manually.

To clarify, I’m allowing INSERT and UPDATE alongside DELETE not
because I anticipate a specific use case, but to avoid adding
unnecessary code complexity. Since restricting these operations isn't
a safety requirement, I felt it was better to keep the implementation
simple rather than adding extra checks that don't provide real value.

So let's get consensus on this decision and then we can change accordingly.

> >
> > >> I  Wrote
> > > I have been exploring the enforcement of these restrictions.
> > > Currently, DML is validated via CheckValidResultRel(). If we do not
> > > explicitly check for the conflict log table in that function, DML
> > > operations, including DELETE, will be permitted. While I am not overly
> > > concerned about users attempting manual INSERT or UPDATE operations.
> >
> > > Now for TRUNCATE, the truncate_check_rel() currently relies on
> > > IsSystemClass() to restrict truncations. Since the conflict namespace
> > > is included in IsSystemClass() to prevent unauthorized DDL, TRUNCATE
> > > is blocked by default. We could allow it by adding an exception in
> > > truncate_check_rel() (e.g., IsSystemClass() &&
> > > !IsConflictNamespace()), but I am unsure if this is necessary. Do we
> > > really need to permit TRUNCATE, or is allowing DELETE sufficient for
> > > user-driven cleanup?
> >
>
> I am okay if we allow DELETE alone.

Thanks for your opinion.

> > >
> > >
> > > 3)
> > > We currently skip ANALYZE on TOAST tables, but I’m not sure whether
> > > the same restriction should apply to CLT. Since users are expected to
> > > query CLT frequently, collecting statistics could be beneficial. Even
> > > though there are currently no indexes or other structures to enable
> > > more optimal plans, having statistics should not harm. Thoughts?
> > >
> > > postgres=# analyze pg_toast.pg_toast_16399;
> > > WARNING:  skipping "pg_toast_16399" --- cannot analyze non-tables or
> > > special system tables
> > >
> > > postgres=# analyze pg_conflict.pg_conflict_16394;
> > > ANALYZE
> >
> > I think its good to ANALYZE CLT data because user logically never need
> > to query the toast data, but they would have to query the conflict
> > data.
>
> Right. Do you think we shall allow index creation as well on this
> table? It may help if the table is huge. As an example, a user can
> create an index on relid to query it faster. Currently it is not
> allowed.

That’s an interesting point. I initially considered creating internal
indexes to simplify management and avoid requiring users to have DDL
permissions on the pg_conflict schema. However, I realized that
predefined indexes might be too restrictive for diverse search
requirements.  Perhaps we should omit indexes from the initial
version? We can then decide whether to add predefined indexes or allow
external ones based on actual usage patterns and feedback.  Thoughts?

--
Regards,
Dilip Kumar
Google