Re: Proposal: Conflict log history table for Logical Replication

On Fri, Dec 12, 2025 at 3:33 PM shveta malik <shveta.malik@gmail.com> wrote:
>
>
> Few comments for v11:
>
> 1)
> +#include "executor/spi.h"
> +#include "replication/conflict.h"
> +#include "utils/fmgroids.h"
> +#include "utils/regproc.h"
>
> subscriptioncmds.c compiles without the above inclusions.

I think we need utils/regproc.h for "stringToQualifiedNameList()"

> 2)
> postgres=# create subscription sub3 connection '...' publication pub1
> WITH(conflict_log_table='pg_temp.clt');
> NOTICE:  created replication slot "sub3" on publisher
> CREATE SUBSCRIPTION
>
> Should we restrict clt creation in pg_temp?

Done and added a test.

> 3)
> + /* Fetch the eixsting conflict table table information. */
>
> typos: eixsting->existing,
>           table table -> table

Fixed

> 4)
> AlterSubscription():
> + values[Anum_pg_subscription_subconflictlognspid - 1] =
> + ObjectIdGetDatum(nspid);
> +
> + if (relname != NULL)
> + values[Anum_pg_subscription_subconflictlogtable - 1] =
> + CStringGetTextDatum(relname);
> + else
> + nulls[Anum_pg_subscription_subconflictlogtable - 1] =
> + true;
>
> Should we move the nspid setting inside 'if(relname != NULL)' block?

Since subconflictlognspid is part of the fixed size structure so we
will always have to set it so I prefer it to keep it out.

> 5)
> Is there a way to reset/remove conflict_log_table? I did not see any
> such handling in AlterSubscription? It gives error:
>
> postgres=# alter subscription sub3 set (conflict_log_table='');
> ERROR:  invalid name syntax

Fixed and added a test case

> 6)
> +char *
> +get_subscription_conflict_log_table(Oid subid, Oid *nspid)
> +{
> + HeapTuple tup;
> + Datum datum;
> + bool isnull;
> + char    *relname = NULL;
> + Form_pg_subscription subform;
> +
> + *nspid = InvalidOid;
> +
> + tup = SearchSysCache1(SUBSCRIPTIONOID, ObjectIdGetDatum(subid));
> +
> + if (!HeapTupleIsValid(tup))
> + return NULL;
>
> Should we have elog(ERROR) here for cache lookup failure? Callers like
> AlterSubscription, DropSubscription lock the sub entry, so it being
> missing at this stage is not normal. I have not seen all the callers
> though.

Yeah we can do that.

> 7)
> +#include "access/htup.h"
> +#include "access/skey.h"
>
> +#include "access/table.h"
> +#include "catalog/pg_attribute.h"
> +#include "catalog/indexing.h"
> +#include "catalog/namespace.h"
> +#include "catalog/pg_namespace.h"
> +#include "catalog/pg_type.h"
>
> +#include "executor/spi.h"
> +#include "utils/array.h"
>
> conflict.c compiles without above inclusions.

Done


--
Regards,
Dilip Kumar
Google