On Tue, Jan 20, 2026 at 6:48 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Mon, 19 Jan 2026 at 10:57, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Mon, Jan 19, 2026 at 9:42 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > Some review comments for v22-0003.
> > >
> > > ======
> > >
> > > 1.
> > > It looks like none of my previous v20-0003 review comments [1] have
> > > been addressed. Maybe accidentally overlooked?
> > >
> > > ======
> > >
> > > 2.
> > > + <caution>
> > > + <para>
> > > + The internal conflict logging table is strictly tied to
> > > the lifecycle of the
> > > + subscription or the
> > > <literal>conflict_log_destination</literal> setting. If
> > > + the subscription is dropped, or if the destination is changed to
> > > + <literal>log</literal>, the table and all its recorded
> > > conflict data are
> > > + <emphasis>permanently deleted</emphasis>. To perform a
> > > post-mortem analysis
> > > + after removing a subscription, users must manually back up
> > > or rename the
> > > + conflict table before the deletion occurs.
> > > + </para>
> > > + </caution>
> > >
> > > 2a.
> > > Let's consistently call this the "Conflict log table", same as
> > > everywhere else, not "logging table".
> > >
> > > ~
> > >
> > > 2b.
> > > This is only a caution for the CLT, so I felt it's better to put this
> > > in the scope of the 'table' param value.
> > >
> > > ~~~
> > >
> > > 3.
> > > + analysis of conflicts. This table is automatically
> > > dropped when the
> > > + subscription is removed.
> > >
> > > If you move the <caution> to this scope, as suggested above in #2b,
> > > then you can remove the sentence "This table is automatically dropped
> > > when the subscription is removed", because that is duplicate
> > > information you already wrote in the caution.
> >
> > The attached patch fixes above comments and other comments reported in
> > v22-0001 and v22-0002
>
> The tests are failing randomly at the following places
> +# Wait for the conflict to be logged in the CLT
> +my $log_check = $node_subscriber->poll_query_until(
> + 'postgres',
> + "SELECT count(*) > 0 FROM $clt;"
> +);
> +
> +my $conflict_check = $node_subscriber->safe_psql('postgres',
> + "SELECT count(*) FROM $clt WHERE conflict_type =
> 'multiple_unique_conflicts';");
> +is($conflict_check, 1, 'Verified multiple_unique_conflicts logged
> into conflict log table');
>
>
> +# Wait for the conflict to be logged in the CLT
> +$log_check = $node_subscriber->poll_query_until(
> + 'postgres',
> + "SELECT count(*) > 0 FROM $clt;"
> +);
> +
> +$conflict_check = $node_subscriber->safe_psql('postgres',
> + "SELECT count(*) FROM $clt WHERE conflict_type =
> 'multiple_unique_conflicts';");
> +is($conflict_check, 1, 'Verified multiple_unique_conflicts logged
> into conflict log table');
>
>
> In both places it fails because the number of conflict records
> inserted can be more than 1 like below:
> [18:35:58.342](1.786s) not ok 1 - Verified multiple_unique_conflicts
> logged into conflict log table
> [18:35:58.346](0.004s)
> [18:35:58.347](0.002s) # Failed test 'Verified
> multiple_unique_conflicts logged into conflict log table'
> # at t/035_conflicts.pl line 104.
> [18:35:58.348](0.000s) # got: '2'
> # expected: '1'
>
> How about we check that the record count >= 1 and check for 't'.
Yeah that makes sense, fixed that and also fixed Shveta's comments,
now only doc changes suggestions from Peter are pending.
--
Regards,
Dilip Kumar
Google
