Thread

Re: Proposal: Conflict log history table for Logical Replication

vignesh C <vignesh21@gmail.com> — 2026-05-25T04:42:53Z

On Fri, 22 May 2026 at 15:42, Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> Here are few more review comments -
> 1) Patch-0001 + 0002:
> In subscription.sql:
>  -- Verify the table OID for reap check
>  SELECT 'pg_conflict_log_for_subid_' || oid AS internal_tablename FROM
> pg_subscription WHERE subname = 'regress_conflict_test1' \gset
>  SET client_min_messages = WARNING;
>  DROP SUBSCRIPTION regress_conflict_test1;
>  -- should return NULL, meaning the conflict log table was reaped via dependency
>  SELECT to_regclass(:'internal_tablename');
>
> Here, internal_tablename becomes "pg_conflict_log_*" without the
> pg_conflict. schema prefix. So, "SELECT
> to_regclass(:'internal_tablename');" will always return NULL even if
> the table still exists in the pg_conflict schema, which skips the
> actual drop verification scenario.
> Should we instead use:
>    "SELECT 'pg_conflict.pg_conflict_log_' || oid AS internal_tablename..."
> ~~~
>
> For Patch-0007:
> 2)
> @@ -2067,9 +2095,31 @@ selectDumpableNamespace(NamespaceInfo *nsinfo,
> Archive *fout)
>  static void
>  selectDumpableTable(TableInfo *tbinfo, Archive *fout)
> ....
> + if (strcmp(tbinfo->dobj.namespace->dobj.name, "pg_conflict") == 0)
> ...
> + * Dump pg_conflict tables only during binary upgrade. The schema
> + * is assumed to already exist.
> + */
> + tbinfo->dobj.dump = DUMP_COMPONENT_DEFINITION;
> ....
> + else
> + tbinfo->dobj.dump = DUMP_COMPONENT_NONE;
> + }
> +
>
> For conflict log tables during binary upgrade, we set:
>    tbinfo->dobj.dump = DUMP_COMPONENT_DEFINITION;
>
> but then execution falls through to the later logic:
> ...
>   else
>     tbinfo->dobj.dump = tbinfo->dobj.namespace->dobj.dump_contains;
>
> which seems to overwrite the earlier 'dobj.dump' value. So it looks
> like DUMP_COMPONENT_DEFINITION may never actually survive here.
> Should we return from this block instead of continuing further?
>
> 3)
> @@ -5656,6 +5757,11 @@ dumpSubscription(Archive *fout, const
> SubscriptionInfo *subinfo)
>
>   appendPQExpBufferStr(query, ");\n");
>
> + appendPQExpBuffer(query,
> +   "\n\nALTER SUBSCRIPTION %s SET (conflict_log_destination = %s);\n",
> +   qsubname,
> +   subinfo->subconflictlogdest);
> +
>
> The above ALTER SUBSCRIPTION command seems to be dumped
> unconditionally for every subscription.
> Since the default value during subscription creation is already
> "subconflictlogdest = 'log' ", should we emit this command only when
> subconflictlogdest is non-NULL and not 'log'?
>
> 4)
> + if (PQgetisnull(res, i, i_sublogdestination))
> + subinfo[i].subconflictlogdest = NULL;
> + else
> + subinfo[i].subconflictlogdest =
> + pg_strdup(PQgetvalue(res, i, i_sublogdestination));
> +
> + if (PQgetisnull(res, i, i_sublogdestination))
> + subinfo[i].subconflictlogdest = NULL;
> + else
> + subinfo[i].subconflictlogdest =
> + pg_strdup(PQgetvalue(res, i, i_sublogdestination));
> +
>   /* Decide whether we want to dump it */
>
> Looks like the same if-else block is repeated twice here.

Thanks for the comments, the attached v39 version patch has the
changes for the same.

Regards,
Vignesh