Re: Skipping logical replication transactions on subscriber side

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.

>
  While this will result in avoiding the last error on the
subscription, thus allowing it to resume working.  See "link to a more
holistic description in the Logical Replication chapter" for
alternative means of resolving subscription errors.  Removing an
entire transaction from the history of a table should be considered a
last resort as it can leave the system in a very inconsistent state.
>
> Note, this feature will not accept transactions prepared under two-phase commit.
>
> This command sets pg_subscription.subskipxid field upon issuance and the system clears the same field upon seeing and
successfullyskipped the identified transaction.  Issuing this command again while a skipped transaction is pending
replacesthe existing transaction with the new one. 
> """
>

The proposed text sounds better to me except for a minor change as
suggested above.

> 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 don't understand why/how ", if a valid transaction ID;" comes into play (how would we know whether it is valid, or
ifwe 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.

> 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?

> The Logical Replication page changes provide good content overall but I dislike going into detail about how to
performconflict resolution in the third paragraph and then summarize the various forms of conflict resolution in the
newlyadded forth.  Maybe re-work things like: 
>
> 1. Logical replication behaves...
> 2. A conflict will produce...details can be found in places...
> 3. Resolving conflicts can be done by...
> 4. (split and reworded) If choosing to simply skip the offending transaction you take the
pg_stat_subscription_worker.last_error_xidvalue (716 in the example above) and provide it while executing ALTER
SUBSCRIPTIONSKIP... 
> 5. (split and reworded) Prior to v15 ALTER SUBSCRIPTION SKIP was not available and instead you had to use the
pg_replication_origin_advance()function... 
>
> Don't just list out two options for the user to perform the same action.  Tell a story about why we felt compelled to
addALTER SYSTEM SKIP and why either the function is now deprecated or is useful given different circumstances (the
formerseems likely). 
>

Personally, I don't see much value in the split (especially giving
context like "Prior to v15 ..) but specifying the circumstances where
each of the options could be useful.

--
With Regards,
Amit Kapila.