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

On Wed, Dec 24, 2025 at 4:02 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Tue, Dec 23, 2025 at 5:52 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Tue, Dec 23, 2025 at 5:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Tue, Dec 23, 2025 at 10:55 AM shveta malik <shveta.malik@gmail.com> wrote:
> > > >
> > > > On Mon, Dec 22, 2025 at 9:11 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > > >
> > > > > On Mon, Dec 22, 2025 at 3:09 PM shveta malik <shveta.malik@gmail.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?

Right

> 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.

I haven't analyzed this options, I will do that but not before Jan 3rd
as I will be away from my laptop for a week.

> 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?

Here is the patches I have changed by using IsSystemClass(), based on
this many other things changed like we don't need to check for the
temp schema and also the caller of create_conflict_log_table() now
don't need to find the creation schema so it don't need to generate
the relname so that part is also moved within
create_conflict_log_table().  Fixed most of the comments given by
Peter and Shveta, although some of them are still open e.g. the name
of the conflict log table as of now I have kept as
conflict_log_table_<subid> other options are

1. pg_conflict_<subid>
2. conflict_log_<subid>
3. sub_conflict_log_<subid>

I prefer 3, considering it says this table holds subscription conflict
logs.  Thoughts?

Vignesh, your patches have to be rebased on the new version.

--
Regards,
Dilip Kumar
Google