Re: Proposal: Conflict log history table for Logical Replication

On Tue, Dec 23, 2025 at 5:52 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Tue, Dec 23, 2025 at 5:18 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Tue, Dec 23, 2025 at 10:55 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> > >
> > > On Mon, Dec 22, 2025 at 9:11 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > > >
> > > > On Mon, Dec 22, 2025 at 3:09 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> > > >
> > > > I think this needs more thought, others can be fixed.
> > > >
> > > > > 2)
> > > > > postgres=# drop schema shveta cascade;
> > > > > NOTICE: drop cascades to subscription sub1
> > > > > ERROR: global objects cannot be deleted by doDeletion
> > > > >
> > > > > Is this expected? Is the user supposed to see this error?
> > > > >
> > > > See below code, so this says if the object being dropped is the
> > > > outermost object (i.e. if we are dropping the table directly) then it
> > > > will disallow dropping the object on which it has INTERNAL DEPENDENCY,
> > > > OTOH if the object is being dropped via recursive drop (i.e. the table
> > > > is being dropped while dropping the schema) then object on which it
> > > > has INTERNAL dependency will also be added to the deletion list and
> > > > later will be dropped via doDeletion and later we are getting error as
> > > > subscription is a global object. I thought maybe we can handle an
> > > > additional case that the INTERNAL DEPENDENCY, is on subscription the
> > > > disallow dropping it irrespective of whether it is being called
> > > > directly or via recursive drop but then it will give an issue even
> > > > when we are trying to drop table during subscription drop, we can make
> > > > handle this case as well via 'flags' passed in findDependentObjects()
> > > > but need more investigation.
> > > >
> > > > Seeing this complexity makes me think more on is it really worth it to
> > > > maintain this dependency? Because during subscription drop we anyway
> > > > have to call performDeletion externally because this dependency is
> > > > local so we are just disallowing the conflict table drop, however the
> > > > ALTER table is allowed so what we are really protecting by protecting
> > > > the table drop, I think it can be just documented that if user try to
> > > > drop the table then conflict will not be inserted anymore?
> > > >
> > > > findDependentObjects()
> > > > {
> > > > ...
> > > > switch (foundDep->deptype)
> > > > {
> > > > ....
> > > > case DEPENDENCY_INTERNAL:
> > > > * 1. At the outermost recursion level, we must disallow the
> > > > * DROP. However, if the owning object is listed in
> > > > * pendingObjects, just release the caller's lock and return;
> > > > * we'll eventually complete the DROP when we reach that entry
> > > > * in the pending list.
> > > > }
> > > > }
> > > >
> > > > [1]
> > > > postgres[1333899]=# select * from pg_depend where objid > 16410;
> > > > classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype
> > > > ---------+-------+----------+------------+----------+-------------+---------
> > > > 1259 | 16420 | 0 | 2615 | 16410 | 0 | n
> > > > 1259 | 16420 | 0 | 6100 | 16419 | 0 | i
> > > > (4 rows)
> > > >
> > > > 16420 -> conflict_log_table_16419
> > > > 16419 -> subscription
> > > > 16410 -> schema s1
> > > >
> > >
> > > One approach could be to use something similar to
> > > PERFORM_DELETION_SKIP_EXTENSIONS in our case, but only for recursive
> > > drops. The effect would be that 'DROP SCHEMA ... CASCADE' would
> > > proceed without error, i.e., it would drop the tables as well without
> > > including the subscription in the dependency list. But if we try to
> > > drop a table directly (e.g., DROP TABLE CLT), it will still result in:
> > > ERROR: cannot drop table because subscription sub1 requires it
> > >
> >
> > I think this way of allowing dropping the conflict table without
> > caring for the parent object (subscription) is not a good idea. How
> > about creating a dedicated schema, say pg_conflict for the purpose of
> > storing conflict tables? This will be similar to the pg_toast schema
> > for toast tables. So, similar to that each database will have a
> > pg_conflict schema. It prevents the "orphan" problem where a user
> > accidentally drops the logging schema but the Subscription is still
> > trying to write to it. pg_dump needs to ignore all system schemas
> > EXCEPT pg_conflict. This ensures the history is preserved during
> > migrations while still protecting the tables from accidental user
> > deletion. About permissions, I think we need to set the schema
> > permissions so that USAGE is public (so users can SELECT from their
> > logs) but CREATE is restricted to the superuser/subscription owner. We
> > may need to think some more about permissions.
> >
> > I also tried to reason out if we can allow storing the conflict table
> > in pg_catalog but here are a few reasons why it won't be a good idea.
> > I think by default, pg_dump completely ignores the pg_catalog schema.
> > It assumes pg_catalog contains static system definitions (like
> > pg_class, pg_proc, etc.) that are re-generated by the initdb process,
> > not user data. If we place a conflict table in pg_catalog, it will not
> > be backed up. If a user runs pg_dump/all to migrate to a new server,
> > their subscription definition will survive, but their entire history
> > of conflict logs will vanish. Also from the permissions angle, If a
> > user wants to write a custom PL/pgSQL function to "retry" conflicts,
> > they might need to DELETE rows from the conflict table after fixing
> > them. Granting DELETE permissions on a table inside pg_catalog is
> > non-standard and often frowned upon by security auditors. It blurs the
> > line between "System Internals" (immutable) and "User Data" (mutable).
> > So, in short a separate pg_conflict schema appears to be a better solution.
>
> Yeah that makes sense. Although I haven't thought about all cases
> whether it can be a problem anywhere, but meanwhile I tried
> prototyping with this and it behaves what we want.
>
> postgres[1651968]=# select * from pg_conflict.conflict_log_table_16406 ;
> relid | schemaname | relname | conflict_type | remote_xid |
> remote_commit_lsn | remote_commit_ts | remote_origin |
> replica_identity | remote_tuple
> |
> local_conflicts
> -------+------------+---------+-----------------------+------------+-------------------+-------------------------------+---------------+------------------+----------------
> +------------------------------------------------------------------------------------------------------------------------------------
> 16385 | public | test | update_origin_differs | 761 |
> 0/01760BD8 | 2025-12-23 11:08:30.583816+00 | pg_16406 |
> {"a":1} | {"a":1,"b":20}
> | {"{\"xid\":\"772\",\"commit_ts\":\"2025-12-23T11:08:25.568561+00:00\",\"origin\":null,\"key\":null,\"tuple\":{\"a\":1,\"b\":10}}"}
> (1 row)
>
> -- Case1: Alter is not allowed
> postgres[1651968]=# ALTER TABLE pg_conflict.conflict_log_table_16406
> ADD COLUMN a int;
> ERROR: 42501: permission denied: "conflict_log_table_16406" is a system catalog
> LOCATION: RangeVarCallbackForAlterRelation, tablecmds.c:19634
>

How was this achieved? Did you modify IsSystemClass to behave
similarly to IsToastClass?

I tried to analyze whether there are alternative approaches. The
possible options I see are:

1)
heap_create_with_catalog() provides the boolean argument use_user_acl,
which is meant to apply user-defined default privileges. In theory, we
could predefine default ACLs for our schema and then invoke
heap_create_with_catalog() with use_user_acl = true. But it’s not
clear how to do this purely from internal code. We would need to mimic
or reuse the logic behind SetDefaultACLsInSchemas.

2)
Another option is to create the table using heap_create_with_catalog()
with use_user_acl = false, and then explicitly update pg_class.relacl
for that table, similar to what ExecGrant_Relation does when
processing GRANT/REVOKE. But I couldn’t find any existing internal
code paths (outside of the GRANT/REVOKE implementation itself) that do
this kind of post-creation ACL manipulation.
~~

So overall, I feel changing IsSystemClass is the simpler way right
now. To set ACL before/after/during heap_create_with_catalog is a
tricky thing, at-least I could not find an easier way to do this,
unless I have missed something.
Thoughts on possible approaches?

thanks
Shveta