Re: Skipping logical replication transactions on subscriber side

On Sat, Jan 22, 2022 at 12:41 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> On Fri, Jan 21, 2022 at 10:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Fri, Jan 21, 2022 at 10:00 PM David G. Johnston
>> <david.g.johnston@gmail.com> wrote:
>> >
>> > On Fri, Jan 21, 2022 at 4:55 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>> >>
>> >> Apart from this, I have changed a few comments and ran pgindent. Do
>> >> let me know what you think of the changes?
>> >
>> >
>> > The paragraph describing ALTER SUBSCRIPTION SKIP seems unnecessarily repetitive.  Consider:
>> > """
>> > Skips applying all changes of the specified remote transaction, whose value should be obtained from
pg_stat_subscription_workers.last_error_xid.
>> >
>>
>> Here, you can also say that the value can be found from server logs as well.
>
>
> subscriber's server logs, right?
>

Right.

>  I would agree that adding that for completeness is warranted.
>
>>
>> > Then change the subskipxid column description to be:
>> > """
>> > ID of the transaction whose changes are to be skipped.  It is 0 when there are no pending skips.  This is set by
issuingALTER SUBSCRIPTION SKIP and resets back to 0 when the identified transactions passes through the subscription
streamand is successfully ignored. 
>> > """
>> >
>>
>> Users can manually reset it by specifying NONE, so that should be
>> covered in the above text, otherwise, looks good.
>
>
> I agree with incorporating "reset" into the paragraph somehow - does not have to mention NONE, just that ALTER
SUBSCRIPTIONSKIP (not a family friendly abbreviation...) is what does it. 
>

It is not clear to me what you have in mind here but to me in this
context saying "Setting <literal>NONE</literal> resets the transaction
ID." seems quite reasonable.

>>
>> > I don't understand why/how ", if a valid transaction ID;" comes into play (how would we know whether it is valid,
orif we do ALTER SUBSCRIPTION SKIP should prohibit the invalid value from being chosen). 
>> >
>>
>> What do you mean by invalid value here? Is it the value lesser than
>> FirstNormalTransactionId or a value that is of the non-error
>> transaction? For the former, we already have a check in the patch and
>> for later we can't identify it with any certainty because the error
>> stats are collected by the stats collector.
>
>
> The original proposal qualifies the non-zero transaction id in subskipxid as being a "valid transaction ID" and that
invalidones (which is how "otherwise" is interpreted given the "valid" qualification preceding it) are shown as 0.  As
anend-user that makes me wonder what it means for a transaction ID to be invalid.  My point is that dropping the
mentionof "valid transaction ID" avoids that and lets the reader operate with an understanding that things should "just
work". If I see a non-zero in the column I have a pending skip and if I see zero I do not.  My wording assumes it is
thatsimple.  If it isn't I would need some clarity as to why it is not in order to write something I could read and
understandfrom my inexperienced user-centric point-of-view. 
>
> I get that I may provide a transaction ID that is invalid such that the system could never see it (or at least not
fora long while) - say we error on transaction 102 and I typo it as 1002 or 101.  But I would expect either an error
whereI make the typo or the numbers 1002 or 101 to appear on the table.  I would not expect my 101 typo to result in a
0appearing on the table (and if it does so today I argue that is a POLA violation).  Thus, "if a valid transaction ID"
fromthe original text just doesn't make sense to me. 
>
> In typical usage it would seem strange to allow a skip to be recorded if there is no existing error in the
subscription. Should we (do we, haven't read the code) warn in that situation? 
>

Yeah, we will error in that situation. The only invalid values are
system reserved values (1,2).

> Or, why even force them to specify a number instead of just saying SKIP and if there is a current error we skip its
transaction,otherwise we warn them that nothing happened because there is no last error. 
>

The idea is that we might extend this feature to skip specific
operations on relations or maybe by having other identifiers. One idea
we discussed was to automatically fetch the last error xid but then
decided it can be done as a later patch.

> Additionally, the description for pg_stat_subscription_workers should describe what happens once the transaction
representedby last_error_xid has either been successfully processed or skipped.  Does this "last error" stick around
untilanother error happens (which is hopefully very rare) or does it reset to blanks? 
>

It will be reset only on subscription drop, otherwise, it will stick
around until another error happens.

>  Seems like it should reset, which really makes this more of an "active_error" instead of a "last_error".  This
systemis linear, we are stuck until this error is resolved, making it active. 
>
>>
>> > I'm against mentioning subtransactions in the skip_option description.
>> >
>>
>> We have mentioned that because currently, we don't support it but in
>> the future one can come up with an idea to support it. What problem do
>> you see with it?
>
>
> If you ever get around to implementing the feature then by all means add it.  My main issue is that we basically
nevertalk about subtransactions in the user-facing documentation and it doesn't seem desirable to do so here.  Knowing
thata whole transaction is skipped is all I need to care about as a user.  I believe that no users will be asking "what
aboutsubtransactions (savepoints)" but by mentioning it less experienced ones will now have something to be curious
aboutthat they really do not need to be. 
>

It is not that we don't mention subtransactions in the docs but I see
your point and I think we can avoid mentioning it in this case.

--
With Regards,
Amit Kapila.