Re: Proposal: Conflict log history table for Logical Replication

On Thu, Dec 4, 2025 at 7:31 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Wed, Dec 3, 2025 at 3:27 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Wed, Dec 3, 2025 at 9:49 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> > > >
> > > > relid | 16391
> > > > schemaname | public
> > > > relname | conf_tab
> > > > conflict_type | multiple_unique_conflicts
> > > > remote_xid | 761
> > > > remote_commit_lsn | 0/01761400
> > > > remote_commit_ts | 2025-12-02 15:02:07.045935+00
> > > > remote_origin | pg_16406
> > > > key_tuple |
> > > > remote_tuple | {"a":2,"b":3,"c":4}
> > > > local_conflicts |
> > > > {"{\"xid\":\"773\",\"commit_ts\":\"2025-12-02T15:02:00.640253+00:00\",\"origin\":\"\",\"tuple\":{\"a\":2,\"b\":2,\"c\":2}}","{\"xid\":\"
> > > > 773\",\"commit_ts\":\"2025-12-02T15:02:00.640253+00:00\",\"origin\":\"\",\"tuple\":{\"a\":3,\"b\":3,\"c\":3}}","{\"xid\":\"773\",\"commit_ts\":\"2025-12-02T
> > > > 15:02:00.640253+00:00\",\"origin\":\"\",\"tuple\":{\"a\":4,\"b\":4,\"c\":4}}"}
> > > >
> > >
> > > Thanks, it looks good. For the benefit of others, could you include a
> > > brief note, perhaps in the commit message for now, describing how to
> > > access or read this array column? We can remove it later.
> >
> > Thanks, okay, temporarily I have added in a commit message how we can
> > fetch the data from the JSON array field. In next version I will add
> > a test to get the conflict stored in conflict log history table and
> > fetch from it.
> >
>
> I've reviewed the v9 patch and here are some comments:

Thanks for reviewing this and your valuable comments.

> The patch utilizes SPI for creating and dropping the conflict history
> table, but I'm really not sure if it's okay because it's actually
> affected by some GUC parameters such as default_tablespace and
> default_toast_compression etc. Also, probably some hooks and event
> triggers could be fired during the creation and removal. Is it
> intentional behavior? I'm concerned that it would make investigation
> harder if an issue happened in the user environment.

Hmm, interesting point, well we can control the value of default
parameters while creating the table using SPI, but I don't see any
reason to not use heap_create_with_catalog() directly, so maybe that's
a better choice than using SPI because then we don't need to bother
about any event triggers/utility hooks etc. Although I don't see any
specific issue with that, unless the user intentionally wants to
create trouble while creating this table. What do others think about
it?

> ---
> + /* build and execute the CREATE TABLE query. */
> + appendStringInfo(&querybuf,
> + "CREATE TABLE %s.%s ("
> + "relid Oid,"
> + "schemaname TEXT,"
> + "relname TEXT,"
> + "conflict_type TEXT,"
> + "remote_xid xid,"
> + "remote_commit_lsn pg_lsn,"
> + "remote_commit_ts TIMESTAMPTZ,"
> + "remote_origin TEXT,"
> + "key_tuple JSON,"
> + "remote_tuple JSON,"
> + "local_conflicts JSON[])",
> + quote_identifier(get_namespace_name(namespaceId)),
> + quote_identifier(conflictrel));
>
> If we want to use SPI for history table creation, we should use
> qualified names in all the places including data types.

That's true, so that we can avoid interference of any user created types.

> ---
> The patch doesn't create the dependency between the subscription and
> the conflict history table. So users can entirely drop the schema
> (with CASCADE option) where the history table is created.

I think as part of the initial discussion we thought since it is
created under the subscription owner privileges so only that user can
drop that table and if the user intentionally drops the table the
conflict will not be recorded in the table and that's acceptable. But
now I think it would be a good idea to maintain the dependency with
subscription so that users can not drop it without dropping the
subscription.

And once
> dropping the schema along with the history table, ALTER SUBSCRIPTION
> ... SET (conflict_history_table = '') seems not to work (I got a
> SEGV).

I will check this, thanks

> ---
> We can create the history table in pg_temp namespace but it should not
> be allowed.

Right, will check this and also add the test for the same.

> ---
> I think the conflict history table should not be transferred to the
> new cluster when pg_upgrade since the table definition could be
> different across major versions.

Let me think more on this with respect to behaviour of other factors
like subscriptions etc.

> I got the following log when the publisher disables track_commit_timestamp:
>
> local_conflicts |
> {"{\"xid\":\"790\",\"commit_ts\":\"1999-12-31T16:00:00-08:00\",\"origin\":\"\",\"tuple\":{\"c\":1}}"}
>
> I think we can omit commit_ts when it's omitted.

+1

> ---
> I think we should keep the history table name case-sensitive:

Yeah we can do that, it looks good to me, what do others think about it?

--
Regards,
Dilip Kumar
Google