Re: Proposal: Conflict log history table for Logical Replication

On Thu, Jan 8, 2026 at 12:29 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Wed, Jan 7, 2026 at 12:12 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > Hi Dilip,
> > Please find a few comments on v19-001:
> >
>
> Please find a few comments for v19-002's part I have reviewed so far:
>
> 1)
> ReportApplyConflict:
> + if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)
> + if ((dest & CONFLICT_LOG_DEST_LOG) != 0)
>
> We can use IsSet everywhere
>
> 2)
> GetConflictLogTableInfo
> This function gets dest and opens table, shall we rename to :
> GetConflictLogDestAndTable
>
> 3)
> GetConflictLogTableInfo:
> + *log_dest = GetLogDestination(MySubscription->conflictlogdest);
> + conflictlogrelid = MySubscription->conflictlogrelid;
> +
> + /* If destination is 'log' only, no table to open. */
> + if (*log_dest == CONFLICT_LOG_DEST_LOG)
> + return NULL;
> +
> + Assert(OidIsValid(conflictlogrelid));
>
> We don't need to fetch conflictlogrelid until after 'if (*log_dest ==
> CONFLICT_LOG_DEST_LOG)' check. We shall move it after the 'if' check.
>
> 4)
> GetConflictLogTableInfo:
> + /* 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)));
>
> Shall we replace it with elog(ERROR)? IMO, it should never happen and
> if it happens, we should raise it as an internal error as we do for
> various other cases.
>
>
> 5)
> ReportApplyConflict():
>
> Currently the function structure is:
>
> /* Insert to table if destination is 'table' or 'all' */
> if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)
>
>   /* Decide what detail to show in server logs. */
> if ((dest & CONFLICT_LOG_DEST_LOG) != 0)
> else <table-only: put reduced info in log>
>
>
> It will be good to make it:
>
> /*
>  * Insert to table if destination is 'table' or 'all' and
>  * also log the error msg to serverlog
>  */
> if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)
> {...}
> else <CONFLICT_LOG_DEST_LOG case>
> {log complete detail}
>
>
> 6)
> tuple_table_slot_to_indextup_json:
> + tuple = heap_form_tuple(tupdesc, values, isnull);
>
> Do we need to do: heap_freetuple at the end?
>

Dilip, few more comments on 002:

7)
+$node_subscriber->safe_psql(
+ 'postgres', qq[
+ CREATE TABLE conf_tab_2 (a int PRIMARY KEY, b int, c int,
unique(a,b)) PARTITION BY RANGE (a);
+ CREATE TABLE conf_tab_2_p1 PARTITION OF conf_tab_2 FOR VALUES FROM
(MINVALUE) TO (100);
+]);
+
I don't see conf_tab_2 being used in  tests.

8)
+##################################################
+# 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);");

Do we really need this? IIUC, we are not using the data inserted here
for any tests.

9)
+# Test conflict insertion into the internal conflict log table
Shall we mention insert_exists test like you have mentioned for subsequent tests

10)
+# CASE 3: Switching Destination to 'log' (Server Log Verification)
No CASE 1 and CASE 2 anywhere above. So term 'CASE 3' can be removed.

11)
+# Final cleanup for subsequent bidirectional tests in the script
+$node_subscriber->safe_psql('postgres', "TRUNCATE conf_tab;");

There are no bidirectional tests.

12)
Do we even need a new file, or can we adjust these in
t/035_conflicts.pl? What do you think?
~~

Overall, tests can be reviewed and structured better.  When you get a
chance, please review it once yourself too.

thanks
Shveta