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

On Sat, Dec 20, 2025 at 4:51 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> I have updated the patch and here are changes done

Thank You for the patch.  Few comments on 001 alone:


1)
postgres=# create subscription sub1 connection ...' publication pub1
WITH(conflict_log_destination = 'table');
ERROR:  could not generate conflict log table "conflict_log_table_16395"
DETAIL:  Conflict log tables cannot be created in a temporary namespace.
HINT:  Ensure your 'search_path' is set to permanent schema.

Based on such existing errors:
errmsg("cannot create relations in temporary schemas of other sessions")));
errmsg("cannot create temporary relation in non-temporary schema")));
errmsg("cannot create relations in temporary schemas of other sessions")));

Shall we tweak:
--temporary namespace --> temporary schema
--permanent --> non-temporary

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?

3)
ConflictLogDestLabels enum starts from 0/INVALID while mapping
ConflictLogDestLabels has values starting from index 1. The index 0
has no value. Thus IMO, wherever we access ConflictLogDestLabels, we
should make a sanity check that index accessed is not
CONFLICT_LOG_DEST_INVALID i.e. opts.logdest !=
CONFLICT_LOG_DEST_INVALID

4)
I find 'Labels' in ConflictLogDestLabels slightly odd. There could be
other names for this variables such as ConflictLogDestValues,
ConflictLogDestStrings or ConflictLogDestNames.

See similar: ConflictTypeNames, SlotInvalidationCauses

5)
+ /*
+ * Strategy for logging replication conflicts:
+ * log - server log only,
+ * table - internal table only,
+ * all - both log and table.
+ */
+ text sublogdestination;

sublogdestination can be confused with regular log_destination. Shall
we rename to subconflictlogdest.

6)
Should the \dRs+ command display the 'Conflict Log Table:' at the end?
This would be similar to how \dRp+ shows 'Tables:', even though the
relation IDs can already be obtained from pg_publication_rel. I think
this would be a useful improvement.

7)
One observation, not sure if it needs any fix, please review and share thoughts.

--CLT created in default public schema present in serach_path
create subscription sub1 connection '..' publication pub1
WITH(conflict_log_destination = 'table');

--Change search path
create schema sch1;
SET search_path=sch1, "$user";

After this, if I create a new sub with destination as 'table', CLT is
generated in sch1. But if I do below:
alter subscription sub1 set (conflict_log_destination='table');

It does not move the table to sch1. This is because
conflict_log_destination is not changed; and as per current
implementation, alter-sub becomes no-op. But search_path is changed.
So what should be the behaviour here?

--let the table be in the old schema, which is currently not in
search_path (existing behaviour)?
--drop the table in the old schema and create a new one present in
search_path?

I could not find a similar case in postgres to compare the behaviour.

If we do
alter subscription sub1 set (conflict_log_destination='log');
alter subscription sub1 set (conflict_log_destination='table');

Then it moves the table to a new schema as internally setting
destination to 'log' drops the table.

thanks
Shveta