On Thu, May 16, 2024 at 3:43 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, May 15, 2024 at 05:58:18PM +0530, Amit Kapila wrote:
> > I guess it could be more work if we want to enhance the test for
> > ERRORs other than the primary key violation.
>
> And? You could pass the ERROR string expected as argument of the
> function if more flexibility is wanted at some point, no?
>
Normally, we consider error_codes for comparison as they are less
prone to change but here it may be okay to use error_string as we can
change it, if required. But let's discuss a bit more on the other
solution being discussed below.
> It happens
> that you don't require that now, which is fine for the three scenarios
> calling test_skip_lsn.
>
> > One simple fix is to
> > update the log_offset to the location of the LOG after successful
> > replication of un-conflicted data. For example, the Log location after
> > we execute the below line in the test:
> >
> > # Check replicated data
> > my $res =
> > $node_subscriber->safe_psql('postgres', "SELECT
> > count(*) FROM tbl");
> > is($res, $expected, $msg);
>
> That still looks like a shortcut to me, weak to race conditions on
> slow machines where more log entries could be generated in-between.
> So it seems to me that you'd still want to make sure that the CONTEXT
> from which the LSN is retrieved maps to the sole expected error. This
> is not going to be stable unless there are stronger checks to avoid
> log entries that can parasite the output, and a stronger matching
> ensures that.
>
This can only be a problem if the apply worker generates more LOG
entries with the required context but it won't do that unless there is
an operation on the publisher to replicate. If we see any such danger
then I agree this can break on some slow machines but as of now, I
don't see any such race condition.
--
With Regards,
Amit Kapila.