Re: Proposal: Conflict log history table for Logical Replication

On Tue, Dec 23, 2025 at 5:49 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Dilip.
>
> Here are some review comments after a first pass of patch v15-0001.
>

And, some more review comments for patch v15-0001.

======
src/backend/catalog/pg_subscription.c

1.
+ /* Always set the destination, default will be log. */
+ values[Anum_pg_subscription_sublogdestination - 1] =
+ CStringGetTextDatum(ConflictLogDestLabels[opts.logdest]);
+

None of the other values[] assignments here have a comment talking
about defaults, etc, so I don't think this needs one either.

======
src/backend/commands/subscriptioncmds.c

CreateSubscription:

2.
+ {
+ char conflict_table_name[NAMEDATALEN];
+ Oid namespaceId, logrelid;

In similar code in AlterSubscription, this was just called 'relname'.
Better to be consistent where possible. I think 'relname' would be
fine here too.

~~~

3.
+ else
+ {
+ /* Destination is "log"; no table is needed. */
+ values[Anum_pg_subscription_subconflictlogrelid - 1] =
+ ObjectIdGetDatum(InvalidOid);
+ }

I think it's better to say this using coded Asserts instead of just
assertions in comments.

e.g.

/* There is no conflict log table */
Assert(opts.logdest == CONFLICT_LOG_DEST_LOG)
values[...] = ObjectIdGetDatum(InvalidOid);

~~~

4.
+ if (isTempNamespace(namespaceId))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("could not generate conflict log table \"%s\"",
+ conflictrel),
+ errdetail("Conflict log tables cannot be created in a temporary namespace."),
+ errhint("Ensure your 'search_path' is set to permanent schema.")));
+
+ /* Report an error if the specified conflict log table already exists. */
+ if (OidIsValid(get_relname_relid(conflictrel, namespaceId)))
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_TABLE),
+ errmsg("could not generate conflict log table \"%s.%s\"",
+ get_namespace_name(namespaceId), conflictrel),
+ errdetail("A table with the internally generated name already exists."),
+ errhint("Drop the existing table or change your 'search_path' to use
a different schema.")));

I'm not sure about these messages:

4a.
"could not generate conflict log table".
- Why say "generate"?
- We don't need to say "conflict log table" -- that's already in the detail

SUGGESTION (something like)
"could not create relation \"%s\""

~

4b.
For the 2nd error, I think errmsg should look like below, same as any
other duplicate table error.
"relation \"%s.%s\" already exists"

~

4c.
+ errdetail("A table with the internally generated name already exists."),

I don't think this errdetail added anything useful. It already exists
-- that's all you need to know. Why does it matter that the name was
generated automatically?

~~~

GetLogDestination:

5.
+ for (int i = CONFLICT_LOG_DEST_LOG; i <= CONFLICT_LOG_DEST_ALL; i++)
+ {
+ if (pg_strcasecmp(dest, ConflictLogDestLabels[i]) == 0)
+ return (ConflictLogDest) i;
+ }
+
+ /* Unrecognized string. */
+ return CONFLICT_LOG_DEST_INVALID;

This code is making rash assumptions about the enums values being the
same as ordinals.

IMO it should be written like:

if (strcmp(dest, "log") == 0)
return CONFLICT_LOG_DEST_LOG;

if (strcmp(dest, "table") == 0)
return CONFLICT_LOG_DEST_TABLE;

if (strcmp(dest, "all") == 0)
return CONFLICT_LOG_DEST_ALL;

/* Unrecognized dest. */
ereport(ERROR, ...);

~~~

IsConflictLogTable

6.
+bool
+IsConflictLogTable(Oid relid)
+{
+ Relation rel;

If you enforce (as I've suggested elsewhere previously) a name
convention that the CLT must have "pg_" prefix, then perhaps you can
exit early from this function without having to scan all the OIDs,
just by checking first that the RelationGetRelationName(rel) must
start with "pg_".

======
src/test/regress/sql/subscription.sql

7.
+-- fail - unrecognized format value

/format/parameter/

~~

8.
Some of these tests are grouped together like

"ALTER: State transitions"
and
"Ensure drop table is not allowed, and DROP SUBSCRIPTION reaps the table"
etc.

These group boundaries should be identified more clearly with more
substantial comments.
e.g
#-- ==================================
#-- ALTER - state transition tests
#-- ==================================

~~~

9.
The "pg_relation_is_publishable" seems misplaced because it is buried
among the drop/reap tests. Maybe it should come before all that.

======
src/tools/pgindent/typedefs.list

10.
What about "typedef enum ConflictLogDest"

======
Kind Regards,
Peter Smith.
Fujitsu Australia