On Tues, Jun 28, 2022 at 12:15 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Tue, Jun 28, 2022 at 8:51 AM wangw.f...@fujitsu.com
> <wangw.f...@fujitsu.com> wrote:
> >
> > On Thu, Jun 23, 2022 at 16:44 PM Amit Kapila <amit.kapil...@gmail.com>
> wrote:
> > > On Thu, Jun 23, 2022 at 12:51 PM wangw.f...@fujitsu.com
> > > <wangw.f...@fujitsu.com> wrote:
> > > >
> > > > On Mon, Jun 20, 2022 at 11:00 AM Amit Kapila <amit.kapil...@gmail.com>
> > > wrote:
> > > > > I have improved the comments in this and other related sections of the
> > > > > patch. See attached.
> > > > Thanks for your comments and patch!
> > > > Improved the comments as you suggested.
> > > >
> > > > > > > 3.
> > > > > > > +
> > > > > > > + <para>
> > > > > > > + Setting streaming mode to <literal>apply</literal> could
> > > > > > > export
> invalid
> > > > > LSN
> > > > > > > + as finish LSN of failed transaction. Changing the streaming
> > > > > > > mode
> and
> > > > > making
> > > > > > > + the same conflict writes the finish LSN of the failed
> > > > > > > transaction in
> the
> > > > > > > + server log if required.
> > > > > > > + </para>
> > > > > > >
> > > > > > > How will the user identify that this is an invalid LSN value and
> > > > > > > she
> > > > > > > shouldn't use it to SKIP the transaction? Can we change the second
> > > > > > > sentence to: "User should change the streaming mode to 'on' if
> > > > > > > they
> > > > > > > would instead wish to see the finish LSN on error. Users can use
> > > > > > > finish LSN to SKIP applying the transaction." I think we can give
> > > > > > > reference to docs where the SKIP feature is explained.
> > > > > > Improved the sentence as suggested.
> > > > > >
> > > > >
> > > > > You haven't answered first part of the comment: "How will the user
> > > > > identify that this is an invalid LSN value and she shouldn't use it to
> > > > > SKIP the transaction?". Have you checked what value it displays? For
> > > > > example, in one of the case in apply_error_callback as shown in below
> > > > > code, we don't even display finish LSN if it is invalid.
> > > > > else if (XLogRecPtrIsInvalid(errarg->finish_lsn))
> > > > > errcontext("processing remote data for replication origin \"%s\"
> > > > > during \"%s\" in transaction %u",
> > > > > errarg->origin_name,
> > > > > logicalrep_message_type(errarg->command),
> > > > > errarg->remote_xid);
> > > > I am sorry that I missed something in my previous reply.
> > > > The invalid LSN value here is to say InvalidXLogRecPtr (0/0).
> > > > Here is an example :
> > > > ```
> > > > 2022-06-23 14:30:11.343 CST [822333] logical replication worker CONTEXT:
> > > processing remote data for replication origin "pg_16389" during "INSERT"
> > > for
> > > replication target relation "public.tab" in transaction 727 finished at
> > > 0/0
> > > > ```
> > > >
> > >
> > > I don't think it is a good idea to display invalid values. We can mask
> > > this as we are doing in other cases in function apply_error_callback.
> > > The ideal way is that we provide a view/system table for users to
> > > check these errors but that is a matter of another patch. So users
> > > probably need to check Logs to see if the error is from a background
> > > apply worker to decide whether or not to switch streaming mode.
> >
> > Thanks for your comments.
> > I improved it as you suggested. I mask the LSN if it is invalid LSN(0/0).
> > Also, I improved the related pg-doc as following:
> > ```
> > When the streaming mode is <literal>parallel</literal>, the finish LSN of
> > failed transactions may not be logged. In that case, it may be necessary
> > to
> > change the streaming mode to <literal>on</literal> and cause the same
> > conflicts again so the finish LSN of the failed transaction will be
> > written
> > to the server log. For the usage of finish LSN, please refer to <link
> > linkend="sql-altersubscription"><command>ALTER SUBSCRIPTION ...
> > SKIP</command></link>.
> > ```
> > After improving this (mask invalid LSN), I found that this improvement and
> > parallel apply patch do not seem to have a strong correlation. Would it be
> > better to improve and commit in another separate patch?
> >
>
> Is there any other case where we can hit this code path (mask
> invalidLSN) without this patch?
I realized that there is no normal case that could hit this code path in HEAD.
If we want to hit this code path, we must set apply_error_callback_arg.rel to
valid relation and set finish LSN to InvalidXLogRecPtr.
But now in HEAD, we only set apply_error_callback_arg.rel to valid relation
after setting finish LSN to valid LSN.
So it seems fine change this along with the parallel apply patch.
Regards,
Wang wei
