On Fri, May 22, 2026 at 10:21 AM Nisha Moond <[email protected]> wrote:
>
> On Wed, May 20, 2026 at 3:05 PM vignesh C <[email protected]> wrote:
> >
> > Rest of the comments were fixed.
> > The attached v37 version patch has the changes for the same. Also
> > Peter's comments on the documentation patch from [1] and Shveta's
> > comments from [2] are addressed in the attached patch.
> >
>
> Here are few comments based on v37 testing:
>
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,
Nisha
