Subject: Re: Proposal: Conflict log history table for Logical Replication

From: vignesh C <[email-masked]>
Date: Fri, 2 Jan 2026 17:53:02 +0530

On Thu, 25 Dec 2025 at 13:10, Dilip Kumar <[email-masked]> wrote:
>
> On Wed, Dec 24, 2025 at 4:02 PM shveta malik <[email-masked]> wrote:
> >
> > On Tue, Dec 23, 2025 at 5:52 PM Dilip Kumar <[email-masked]> wrote:
> > >
> > > On Tue, Dec 23, 2025 at 5:18 PM Amit Kapila <[email-masked]> wrote:
> > > >
> > > > On Tue, Dec 23, 2025 at 10:55 AM shveta malik <[email-masked]> wrote:
> > > > >
> > > > > On Mon, Dec 22, 2025 at 9:11 PM Dilip Kumar <[email-masked]> wrote:
> > > > > >
> > > > > > On Mon, Dec 22, 2025 at 3:09 PM shveta malik <[email-masked]> 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>

Few comments:
1) I was thinking it would be good to combine the tests with
035_conflicts, where the subscription can be changed to include
conflict_log_destination and include the additional verification. That
way we might save some execution time.

2) I felt this data is not required for this test:
+##################################################
+# INSERT data on Pub and Sub
+##################################################
+
+# Insert data in the publisher table
+$node_publisher->safe_psql('postgres',
+       "INSERT INTO conf_tab VALUES (1,1,1);");
+
+# Insert data in the subscriber table
+$node_subscriber->safe_psql('postgres',
+       "INSERT INTO conf_tab VALUES (2,2,2), (3,3,3), (4,4,4);");

3) You can name it starting with regress_ to avoid warnings when
ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS is defined:
+$node_subscriber->safe_psql(
+       'postgres',
+       "CREATE SUBSCRIPTION sub_tab
+        CONNECTION '$publisher_connstr application_name=$appname'
+        PUBLICATION pub_tab WITH (conflict_log_destination=table)");

4) Similarly here too:
+# Setup logical replication
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+       "CREATE PUBLICATION pub_tab FOR TABLE conf_tab, conf_tab_2");

5) Since we support conflict logging to logfile and table, we can
mention inserted into clt to make it more clear. That way it will
avoid it to misinterpret with clt logfile logging:
+# Wait for the conflict to be logged
+my $log_check = $node_subscriber->poll_query_until('postgres',
+       "SELECT count(*) > 0 FROM $conflict_table;");

Regards,
Vignesh