Re: Proposal: Conflict log history table for Logical Replication

On Thu, 4 Jun 2026 at 11:59, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Some review comments for patch v45-0003.
>
> 3.
> + relid = get_relname_relid(relname, PG_CONFLICT_NAMESPACE);
> + if (OidIsValid(relid))
> + {
> + /* Existing table from upgrade */
> + Assert(IsBinaryUpgrade);
> + }
> + else
>
> Should this be the other way around?
>
> if (IsBinaryUpgrade)
> {
> relid = get_relname_relid(relname, PG_CONFLICT_NAMESPACE);
> Assert(OidIsValid(relid));
> }

This is done intentionally so that the Assert fails if the table
exists in case of non binary upgrade mode.

> 6.
> + if (fout->remoteVersion >= 190000)
> + appendPQExpBufferStr(query,
> + " s.subconflictlogrelid, s.subconflictlogdest\n");
> else
> - appendPQExpBufferStr(query, " NULL AS subservername\n");
> + appendPQExpBufferStr(query,
> + " NULL AS subconflictlogrelid, NULL AS subconflictlogdest\n");
>
> It seems more natural to think of the 'subconflictlogdest' before the
> subconflictlogrelid. Perhaps the SQL should swap those around.

I think the existing is better as it does select in the same order as
it is maintained in the table.

> ~~~
>
> 7.
> + if (PQgetisnull(res, i, i_subconflictlogrelid))
> + subinfo[i].subconflictlogrelid = InvalidOid;
> + else
> + {
> + TableInfo *tableInfo;
> +
> + subinfo[i].subconflictlogrelid =
> + atooid(PQgetvalue(res, i, i_subconflictlogrelid));
> +
> + if (subinfo[i].subconflictlogrelid)
> + {
> + tableInfo = findTableByOid(subinfo[i].subconflictlogrelid);
> + if (!tableInfo)
> + pg_fatal("could not find conflict log table with OID %u",
> + subinfo[i].subconflictlogrelid);
> +
> + addObjectDependency(&subinfo[i].dobj, tableInfo->dobj.dumpId);
> + }
> + }
> +
> + if (PQgetisnull(res, i, i_sublogdestination))
> + subinfo[i].subconflictlogdest = NULL;
> + else
> + subinfo[i].subconflictlogdest =
> + pg_strdup(PQgetvalue(res, i, i_sublogdestination));
> +
>
> 7a.
> Those new attributes come as a pair -- e.g. the relid only makes sense
> depending on the destination value; maybe only look for the CLT relid
> is the destination is not LOG/NULL

I think the existing is better as it does select in the same order as
it is maintained in the table.

> Knowing the destination also means you can also do integrity checks
> for PQgetisnull(res, i, i_subconflictlogrelid) -- e.g. must be valid
> Oid, or must be invalid Oid.

I'm not sure if it is pg_dump's responsibility to do these integrity
checks in this case.

> ~~~
>
> dumpSubscription:
>
> 8.
> + if (subinfo->subconflictlogdest &&
> + (pg_strcasecmp(subinfo->subconflictlogdest, "log") != 0))
> + appendPQExpBuffer(query,
> + "\n\nALTER SUBSCRIPTION %s SET (conflict_log_destination = %s);\n",
> + qsubname,
> + subinfo->subconflictlogdest);
> +

> 8b.
> Not sure why this has a double \n\n prefix instead of just \n. I
> didn't see that pattern elsewhere in this file.

Generally there is 2 line gaps between the create and alter
subscription, it was done to maintain the same line gaps like below:
CREATE SUBSCRIPTION sub3 CONNECTION 'dbname=doesnotexist' PUBLICATION
pub1 WITH (connect = false, slot_name = 'sub3', streaming = on);

ALTER SUBSCRIPTION sub3 SET (conflict_log_destination = table);

ALTER SUBSCRIPTION sub3 OWNER TO vignesh;

The rest of the comments were fixed. The attached v47 version patch
has the changes for the same.

Regards,
Vignesh