On Thu, 25 Dec 2025 at 13:10, Dilip Kumar <[email protected]> wrote:
>
> On Wed, Dec 24, 2025 at 4:02 PM shveta malik <[email protected]> wrote:
> >
> > On Tue, Dec 23, 2025 at 5:52 PM Dilip Kumar <[email protected]> wrote:
> > >
> > > On Tue, Dec 23, 2025 at 5:18 PM Amit Kapila <[email protected]>
> > > wrote:
> > > >
> > > > On Tue, Dec 23, 2025 at 10:55 AM shveta malik <[email protected]>
> > > > wrote:
> > > > >
> > > > > On Mon, Dec 22, 2025 at 9:11 PM Dilip Kumar <[email protected]>
> > > > > wrote:
> > > > > >
> > > > > > On Mon, Dec 22, 2025 at 3:09 PM shveta malik
> > > > > > <[email protected]> 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 felt this code cannot be reached now, as the table cannot be
dropped by user and if the table does not exist, error will be thrown
from relation_open:
+ conflictlogrel = table_open(conflictlogrelid, RowExclusiveLock);
+
+ /* Conflict log table is dropped or not accessible. */
+ if (conflictlogrel == NULL)
+ ereport(WARNING,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+ errmsg("conflict log table with OID
%u does not exist",
+ conflictlogrelid)));
2) Since altering of the table is not possible, this table validation
check can be removed:
+ /* Insert to table if destination is 'table' or 'all' */
+ if (conflictlogrel)
+ {
+ Assert((dest & CONFLICT_LOG_DEST_TABLE) != 0);
+
+ if (ValidateConflictLogTable(conflictlogrel))
+ {
3) This function also can be removed:
+/*
+ * ValidateConflictLogTable - Validate conflict log table schema
+ *
+ * Checks whether the table definition including its column names, data
+ * types, and column ordering meet the requirements for conflict log
+ * table.
+ */
+bool
+ValidateConflictLogTable(Relation rel)
4) Similarly here too this check is not required:
+ /* Open conflict log table and insert
the tuple. */
+ conflictlogrel = GetConflictLogTableInfo(&dest);
+ if (ValidateConflictLogTable(conflictlogrel))
+ InsertConflictLogTuple(conflictlogrel);
5) Here jsonb should be included before fmgroids header to maintain
the ordering:
+#include "utils/builtins.h"
+#include "utils/fmgroids.h"
#include "utils/lsyscache.h"
+#include "utils/pg_lsn.h"
+#include "utils/jsonb.h"
Regards,
Vignesh
