Re: Conflict detection and logging in logical replication

On Thu, Jul 25, 2024 at 12:04 PM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Monday, July 22, 2024 5:03 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > On Fri, Jul 19, 2024 at 2:06 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> > >
> > > On Thu, Jul 18, 2024 at 7:52 AM Zhijie Hou (Fujitsu)
> > > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > > >
> > > > Attach the V5 patch set which changed the following.
> > >
> >
> > Please find last batch of comments on v5:
>
> Thanks Shveta and Nisha for giving comments!
>
> >
> >
> > 2)
> > 013_partition.pl: Since we have added update_differ testcase here, we shall
> > add delete_differ as well.
>
> I didn't add tests for delete_differ in partition test, because I think the main
> codes and functionality of delete_differ have been tested in 030_origin.pl.
> The test for update_differ is needed because the patch adds new codes in
> partition code path to report this conflict.
>
> Here is the V6 patch set which addressed Shveta and Nisha's comments
> in [1][2][3][4].

Thanks for addressing the comments.

> [1] https://www.postgresql.org/message-id/CAJpy0uDWdw2W-S8boFU0KOcZjw0%2BsFFgLrHYrr1TROtrcTPZMg%40mail.gmail.com
> [2] https://www.postgresql.org/message-id/CAJpy0uDGJXdVCGoaRHP-5G0pL0zhuZaRJSqxOxs%3DCNsSwc%2BSJQ%40mail.gmail.com
> [3] https://www.postgresql.org/message-id/CAJpy0uC%2B1puapWdOnAMSS%3DQUp_1jj3GfAeivE0JRWbpqrUy%3Dug%40mail.gmail.com
> [4] https://www.postgresql.org/message-id/CABdArM6%2BN1Xy_%2BtK%2Bu-H%3DsCB%2B92rAUh8qH6GDsB%2B1naKzgGKzQ%40mail.gmail.com

I was re-testing all the issues reported so far. I think the issue
reported in [4] above is not fixed yet.

Please find a few more comments:

patch001:

1)
030_origin.pl:
I feel tests added in this file may fail. Since there are 3 nodes here
and if the actual order of replication is not as per the expected
order by your test, it will fail.

Example:
---------
$node_B->safe_psql('postgres', "DELETE FROM tab;");
$node_A->safe_psql('postgres', "INSERT INTO tab VALUES (33);");

# The delete should remove the row on node B that was inserted by node A.
$node_C->safe_psql('postgres', "DELETE FROM tab WHERE a = 33;");

$node_B->wait_for_log(
qr/conflict delete_differ detected..);
---------

The third line assumes Node A's change is replicated to Node B already
before Node C's change reaches NodeB, but it may not be true. Should
we do wait_for_catchup and have a verification step that Node A data
is replicated to Node B before we execute Node C query?
Same for the rest of the tests.

2) 013_partition.pl:
---------
$logfile = slurp_file($node_subscriber1->logfile(), $log_location);
ok( $logfile =~
qr/Updating a row that was modified by a different origin [0-9]+ in
transaction [0-9]+ at .*/,
'updating a tuple that was modified by a different origin');
---------

To be consistent, here as well, we can have 'conflict update_differ
detected on relation ....'

patch002:
3) monitoring.sgml:
'Number of times that the updated value of a row violates a NOT
DEFERRABLE unique constraint while applying changes.'

To be consistent, we can change: 'violates' --> 'violated'

thanks
Shveta