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

On Mon, Dec 29, 2025 at 11:32 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Thu, Dec 25, 2025 at 1:10 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > 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?
> >
>
> I was checking how pg_toast does it. It creates tables with names:
> "pg_toast_%u", relOid
>
> We can do similar i.e., the schema name as pg_conflict and table name
> as pg_conflict_<subid>. Thoughts?

I think this make sense to name the table as pg_conflict_<subid>

> Few comments on 001:
>
>
> 1)
> It will be good to display conflict tablename in \dRs command

+1

> 2)
> postgres=# ALTER TABLE  sch1.t3 set schema pg_toast;
> ERROR:  cannot move objects into or out of TOAST schema
>
> But when we move to pg_conflict, it works. It should error out as well.
> postgres=# ALTER TABLE  sch1.t1 set schema pg_conflict;
> ALTER TABLE

Yeah this should not be allowed and similarly
create/drop/alter/truncate on tables under this schema should be
restricted, I will check all the cases and fix them wherever there are
gaps.

> 3)
> Shall we LOG CLT creation and drop during create/alter sub?

We may but not sure how useful it would be.

> 4)
> create_conflict_log_table()
> + /* Report an error if the specified conflict log table already exists. */
> + if (OidIsValid(get_relname_relid(relname, PG_CONFLICT_NAMESPACE)))
> + ereport(ERROR,
> + (errcode(ERRCODE_DUPLICATE_TABLE),
> + errmsg("relation \"%s.%s\" already exists",
> + get_namespace_name(PG_CONFLICT_NAMESPACE), relname)));
>
>
> I am unable to think of a valid user-scenario when the above will be
> hit. Do we need this as a user-error or simply an Assert or
> internal-error will do?

Yeah it doesn't make sense anymore, I think we can just put an assertion.

> 5)
> + /*
> + * Establish an internal dependency between the conflict log table and the
> + * subscription.  By using DEPENDENCY_INTERNAL, we ensure the table is
> + * automatically reaped when the subscription is dropped. This also
> + * prevents the table from being dropped independently unless the
> + * subscription itself is removed.
> + */
> + ObjectAddressSet(myself, RelationRelationId, relid);
> + ObjectAddressSet(subaddr, SubscriptionRelationId, subid);
> + recordDependencyOn(&myself, &subaddr, DEPENDENCY_INTERNAL);
>
> Now that we have pg_conflict, which is treated similarly to a system
> catalog, I’m wondering whether we actually need to maintain this
> dependency to prevent the CLT table or schema from being dropped.
> Also, given that this currently goes against the convention that a
> shared object cannot be present in pg_depend, could DropSubscription()
> and AlterSubscription() instead handle dropping the table explicitly
> in required scenarios?

I thought about it while implementing the catalog schema, but then
left as it is considering pg_toast tables also maintain internal
dependency on the table, having said that, during drop
subscription/alter subscription we anyway have to explicitly call the
performDeletion of the table so seems like we are not achieving
anything by maintaining dependency.  Lets see what others have to say
on this?  I prefer removing this dependency because this is an
exceptional case where we are maintaining dependency from a local
object to a shared object.  And now if we do not have any need for
this we better get rid of it.

> 6)
> +  descr => 'reserved schema for conflict tables',
>
> Shall we say:  'reserved schema for subscription-specific conflict tables'
>
> or anything better to include that it is subscription related?

IMHO this makes sense.


--
Regards,
Dilip Kumar
Google