RE: Conflict detection and logging in logical replication

On Friday, July 26, 2024 7:34 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Thu, Jul 25, 2024 at 4:12 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> > > 
> A few more comments:

Thanks for the comments.

> 1.
> For duplicate key, the patch reports conflict as following:
> ERROR:  conflict insert_exists detected on relation "public.t1"
> 2024-07-26 11:06:34.570 IST [27800] DETAIL:  Key (c1)=(1) already exists in
> unique index "t1_pkey", which was modified by origin 1 in transaction 770 at
> 2024-07-26 09:16:47.79805+05:30.
> 2024-07-26 11:06:34.570 IST [27800] CONTEXT:  processing remote data for
> replication origin "pg_16387" during message type "INSERT" for replication
> target relation "public.t1" in transaction 742, finished at 0/151A108
> 
> In detail, it is better to display the origin name instead of the origin id. This will
> be similar to what we do in CONTEXT information.


Agreed. Before modifying this, I'd like to confirm the message style in the
cases where origin id may not have a corresponding origin name (e.g., if the
data was modified locally (id = 0), or if the origin that modified the data has
been dropped). I thought of two styles:

1)
- for local change: "xxx was modified by a different origin \"(local)\" in transaction 123 at 2024.."
- for dropped origin: "xxx was modified by a different origin \"(unknown)\" in transaction 123 at 2024.."

One issue for this style is that user may create an origin with the same name
here (e.g. "(local)" and "(unknown)").

2) 
- for local change: "xxx was modified locally in transaction 123 at 2024.."
- for dropped origin: "xxx was modified by an unknown different origin 1234 in transaction 123 at 2024.."

This style slightly modifies the message format. I personally feel 2) maybe
better but am OK for other options as well.

What do you think ?

Here is the V7 patch set that addressed all the comments so far[1][2][3].
The subscription option part is splitted into the separate patch 0002 and
we will decide whether to drop this patch after finishing the perf tests.
Note that I didn't display the tuple value in the message as the discussion
is still ongoing[4].

[1] https://www.postgresql.org/message-id/CAJpy0uDhCnzvNHVYwse%3DKxmOB%3DqtXr6twnDP9xqdzT-oU0OWEQ%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAA4eK1%2BCJXKK34zJdEJZf2Mpn5QyMyaZiPDSNS6%3Dkvewr0-pdg%40mail.gmail.com
[3] https://www.postgresql.org/message-id/CAA4eK1Lmu%3DoVySfGjxEUykCT3FPnL1YFDHKr1ZMwFy7WUgfc6g%40mail.gmail.com
[4] https://www.postgresql.org/message-id/CAA4eK1%2BaK4MLxbfLtp%3DEV5bpvJozKhxGDRS6T9q8sz_s%2BLK3vw%40mail.gmail.com

Best Regards,
Hou zj