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

On Wed, Jan 14, 2026 at 5:09 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Wed, Jan 14, 2026 at 3:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Jan 13, 2026 at 6:15 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > 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.
> > >
> >
> > What kind of complexity you are anticipating, can you show by top-up
> > patch? I think allowing just DELETE and TRUNCATE to table owners would
> > be ideal.
>
> I mean by code complexity i.e. additional check for DELETE/TRUNCATE,
> but if we think that's ideal, I can update it.
>
> > > 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.

So now I have changed this so DELETE/TRUNCATE are allowed and other
DML/DDLs are not allowed, added tests for the same, fixed other open
comments from Peter and also added clt name in \dRs+, haven't added
test for that because it will display the dynamic table name, not sure
what's the best way to hide that and if we hide then what the purpose
of that test anyway because there are other test for \dRs+ when log
destination is log.


--
Regards,
Dilip Kumar
Google