Subject: Re: Logical replication timeout problem

From: Amit Kapila <[email-masked]>
Date: Wed, 6 Apr 2022 14:01:35 +0530

On Wed, Apr 6, 2022 at 11:28 AM Amit Kapila <[email-masked]> wrote:
>
> On Wed, Apr 6, 2022 at 11:09 AM [email-masked]
> <[email-masked]> wrote:
> >
> > According to your suggestion, improve the patch to make it more generic.
> > Attach the new patch.
> >
>
>  typedef void (*LogicalOutputPluginWriterUpdateProgress) (struct
> LogicalDecodingContext *lr,
>   XLogRecPtr Ptr,
>   TransactionId xid,
> - bool skipped_xact
> + bool skipped_xact,
> + bool last_write
>
> In this approach, I don't think we need an additional parameter
> last_write. Let's do the work related to keepalive without a
> parameter, do you see any problem with that?
>

I think this patch doesn't take into account that we call
OutputPluginUpdateProgress() from APIs like pgoutput_commit_txn(). I
think we should always call the new function update_progress from
those existing call sites and arrange the function such that when
called from xact end APIs like pgoutput_commit_txn(), it always call
OutputPluginUpdateProgress and make changes_count as 0.

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 6 Apr 2022 12:59:55 +0000

On Wed, Apr 6, 2022 at 1:59 AM Amit Kapila <[email-masked]> wrote:
On Wed, Apr 6, 2022 at 4:32 AM Amit Kapila <[email-masked]> wrote:
Thanks for your comments.

> In this approach, I don't think we need an additional parameter last_write. Let's
> do the work related to keepalive without a parameter, do you see any problem
> with that?
I agree with you. Modify this point.

> OutputPluginUpdateProgress and make changes_count as 0.
Improve it.
Add two new input to function update_progress.(skipped_xact and end_xact).
Modify the function invoke from OutputPluginUpdateProgress to update_progress.

> Also, let's try to evaluate how it impacts lag functionality for large transactions?
I think this patch will not affect lag functionality. It will updates the lag
field of view pg_stat_replication more frequently.
IIUC, when invoking function WalSndUpdateProgress, it will store the lsn of
change and invoking time in lag_tracker. Then when invoking function
ProcessStandbyReplyMessage, it will calculate the lag field according to the
message from subscriber and the information in lag_tracker. This patch does
not modify this logic, but only increases the frequency of invoking.
Please let me know if I understand wrong.

Attach the new patch.
1. Remove the new function input parameters in this patch(parameter last_write
of WalSndUpdateProgress). [suggestion by Amit-San]
2. Also invoke function update_progress in other xact end APIs like
pgoutput_commit_txn. [suggestion by Amit-San]

Regards,
Wang wei

---

From: Amit Kapila <[email-masked]>
Date: Thu, 7 Apr 2022 11:04:16 +0530

> Please let me know if I understand wrong.

No, your understanding seems correct to me. But what I want to check
is if calling the progress function more often has any impact on
lag-related fields in pg_stat_replication? I think you need to check
the impact of large transaction replay.

One comment:
+static void
+update_progress(LogicalDecodingContext *ctx, bool skipped_xact, bool end_xact)
+{
+ static int changes_count = 0;
+
+ if (end_xact)
+ {
+ /* Update progress tracking at xact end. */
+ OutputPluginUpdateProgress(ctx, skipped_xact);
+ changes_count = 0;
+ }
+ /*
+ * After continuously processing CHANGES_THRESHOLD changes, update progress
+ * which will also try to send a keepalive message if required.

I think you can simply return after making changes_count = 0. There
should be an empty line before starting the next comment.

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Fri, 8 Apr 2022 05:09:50 +0000

On Wed, Apr 7, 2022 at 1:34 PM Amit Kapila <[email-masked]>  wrote:

> should be an empty line before starting the next comment.
Improve as suggested.
BTW, there is a conflict in current HEAD when applying v12 because of the
commit 2c7ea57. Also rebase it.

Attach the new patch.
1. Make some improvements to the new function update_progress. [suggestion by Amit-San]
2. Rebase the patch because the commit 2c7ea57 in current HEAD.

Regards,
Wang wei

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 11 Apr 2022 06:38:59 +0000

On Wed, Apr 7, 2022 at 1:34 PM Amit Kapila <[email-masked]>  wrote:
> > On Wed, Apr 6, 2022 at 1:58 AM Amit Kapila <[email-masked]> wrote:
Thanks for the explanation.

After doing some checks, I found that the v13 patch makes the calculations of
lag functionality inaccurate.

In short, v13 patch lets us try to track lag more frequently and try to send a
keepalive message to subscribers. But in order to prevent flooding the lag
tracker, we could not track lag more than once within
WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS (see function WalSndUpdateProgress).
This means we may lose informations that needs to be tracked.
For example, suppose there is a large transaction with lsn from lsn1 to lsn3.
In HEAD, when we calculate the lag time for lsn3, the lag time of lsn3 is
(now - lsn3.time).
But with v13 patch, when we calculate the lag time for lsn3, because there
maybe no informations of lsn3 but has informations of lsn2 in lag_tracker, the
lag time of lsn3 is (now - t2.time). (see function LagTrackerRead)
Therefore, if we lose the informations that need to be tracked, the lag time
becomes large and inaccurate.

So I skip tracking lag during a transaction just like the current HEAD.
Attach the new patch.

Regards,
Wang wei

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 11 Apr 2022 07:33:09 +0000

> Attach the new patch.
Also, share test results and details.

To check that the lsn information used for the calculation is what we expected,
I get some information by adding logs in the function LagTrackerRead.

Summary of test results:
- In current HEAD and current HEAD with v14 patch, we could found the
  information of same lsn as received from subscriber-side in lag_tracker.
- In current HEAD with v13 patch, we could hardly found the information of same
  lsn in lag_tracker.

Attach the details:
[The log by HEAD]
the lsn we received from subscriber  |  the lsn whose time we used to calculate in lag_tracker
382826584                            |  382826584
743884840                            |  743884840
1104943232                           |  1104943232
1468949424                           |  1468949424
1469521216                           |  1469521216

[The log by HEAD with v14 patch]
the lsn we received from subscriber  |  the lsn whose time we used to calculate in lag_tracker
382826584                            |  382826584
743890672                            |  743890672
1105074264                           |  1105074264
1469127040                           |  1469127040
1830591240                           |  1830591240

[The log by HEAD with v13 patch]
the lsn we received from subscriber  |  the lsn whose time we used to calculate in lag_tracker
382826584                            |  359848728 
743884840                            |  713808560 
1105010640                           |  1073978544
1468517536                           |  1447850160
1469516328                           |  1469516328

Regards,
Wang wei

---

From: Amit Kapila <[email-masked]>
Date: Wed, 13 Apr 2022 16:15:23 +0530

> So I skip tracking lag during a transaction just like the current HEAD.

Thanks, please find the updated patch where I have slightly modified
the comments.

Sawada-San, Euler, do you have any opinion on this approach? I
personally still prefer the approach implemented in v10 [1] especially
due to the latest finding by Wang-San that we can't update the
lag-tracker apart from when it is invoked at the transaction end.
However, I am fine if we like this approach more.

[1] - https://www.postgresql.org/message-id/OS3PR01MB6275E0C2B4D9E488AD7CBA209E1F9%40OS3PR01MB6275.jpnprd01.prod.outlook.com
-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 14 Apr 2022 21:20:13 +0900

> However, I am fine if we like this approach more.

Thank you for updating the patch.

The current patch looks much better than v10 which requires to call to
update_progress() every path.

Regarding v15 patch, I'm concerned a bit that the new function name,
update_progress(), is too generic. How about
update_replation_progress() or something more specific name?

---
+        if (end_xact)
+        {
+                /* Update progress tracking at xact end. */
+                OutputPluginUpdateProgress(ctx, skipped_xact, end_xact);
+                changes_count = 0;
+                return;
+        }
+
+        /*
+         * After continuously processing CHANGES_THRESHOLD changes,
we try to send
+         * a keepalive message if required.
+         *
+         * We don't want to try sending a keepalive message after
processing each
+         * change as that can have overhead. Testing reveals that there is no
+         * noticeable overhead in doing it after continuously
processing 100 or so
+         * changes.
+         */
+#define CHANGES_THRESHOLD 100
+        if (++changes_count >= CHANGES_THRESHOLD)
+        {
+                OutputPluginUpdateProgress(ctx, skipped_xact, end_xact);
+                changes_count = 0;
+        }

Can we merge two if branches since we do the same things? Or did you
separate them for better readability?

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

---

From: "Euler Taveira" <[email-masked]>
Date: Thu, 14 Apr 2022 09:21:27 -0300

> However, I am fine if we like this approach more.
It seems v15 is simpler and less error prone than v10. v10 has a mix of
OutputPluginUpdateProgress() and the new function update_progress(). The v10
also calls update_progress() for every change action in pgoutput_change(). It
is not a good approach for maintainability -- new changes like sequences need
extra calls. However, as you mentioned there should handle the track lag case.

Both patches change the OutputPluginUpdateProgress() so it cannot be
backpatched. Are you planning to backpatch it? If so, the boolean variable
(last_write or end_xacts depending of which version you are considering) could
be added to LogicalDecodingContext. (You should probably consider this approach
for skipped_xact too)

+ * For a large transaction, if we don't send any change to the downstream for a
+ * long time then it can timeout. This can happen when all or most of the
+ * changes are either not published or got filtered out.

We should probable mention that "long time" is wal_receiver_timeout on
subscriber.

+    * change as that can have overhead. Testing reveals that there is no
+    * noticeable overhead in doing it after continuously processing 100 or so
+    * changes.

Tests revealed that ...

+    * We don't have a mechanism to get the ack for any LSN other than end xact
+    * lsn from the downstream. So, we track lag only for end xact lsn's.

s/lsn/LSN/ and s/lsn's/LSNs/

I would say "end of transaction LSN".

+ * If too many changes are processed then try to send a keepalive message to
+ * receiver to avoid timeouts.

In logical replication, if too many changes are processed then try to send a
keepalive message. It might avoid a timeout in the subscriber.

Does this same issue occur for long transactions? I mean keep a long
transaction open and execute thousands of transactions.

BEGIN;
INSERT INTO foo (a) VALUES(1);
-- wait a few hours while executing 10^x transactions
INSERT INTO foo (a) VALUES(2);
COMMIT;

--
Euler Taveira
EDB   https://www.enterprisedb.com/

---

From: Amit Kapila <[email-masked]>
Date: Mon, 18 Apr 2022 09:29:36 +0530

On Thu, Apr 14, 2022 at 5:52 PM Euler Taveira <[email-masked]> wrote:
> extra calls.

Okay, let's use the v15 approach as Sawada-San also seems to have a
preference for that.

> However, as you mentioned there should handle the track lag case.
>
> be added to LogicalDecodingContext.
>

If we add it to LogicalDecodingContext then I think we have to always
reset the variable after its use which will make it look ugly and
error-prone. I was not thinking to backpatch it because of the API
change but I guess if we want to backpatch then we can add it to
LogicalDecodingContext for back-branches. I am not sure if that will
look committable but surely we can try.

> (You should probably consider this approach
> for skipped_xact too)
>

As mentioned, I think it will be more error-prone and we already have
other xact related parameters in that and similar APIs. So, I am not
sure why you want to prefer that?

> transaction open and execute thousands of transactions.

No, this problem won't happen for such cases because we will only try
to send it at the commit time. Note that this problem happens only
when we don't send anything to the subscriber till a timeout happens.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 18 Apr 2022 09:31:45 +0530

> update_replation_progress() or something more specific name?

Do you intend to say update_replication_progress()? The word
'replation' doesn't make sense to me. I am fine with this suggestion.

> separate them for better readability?

I think it is fine to merge the two checks.

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Mon, 18 Apr 2022 13:35:07 +0900

> 'replation' doesn't make sense to me. I am fine with this suggestion.

Yeah, that was a typo. I meant update_replication_progress().

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Mon, 18 Apr 2022 10:05:44 +0530

> look committable but surely we can try.

Even, if we want to add the variable in the struct in back-branches,
we need to ensure not to change the size of the struct as it is
exposed, see email [1] for a similar mistake we made in another case.

[1] - https://www.postgresql.org/message-id/2358496.1649168259%40sss.pgh.pa.us

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 18 Apr 2022 06:16:40 +0000

On Mon, Apr 18, 2022 at 00:35 PM Masahiko Sawada <[email-masked]> wrote:
> > > > Thanks, please find the updated patch where I have slightly
> > > > modified the comments.
> > > >
> > > > personally still prefer the approach implemented in v10 [1]
> > > > especially due to the latest finding by Wang-San that we can't
> > > > update the lag-tracker apart from when it is invoked at the transaction end.

> > > update_replation_progress() or something more specific name?
Improve as suggested. Change the name from update_progress to
update_replication_progress.

> > > separate them for better readability?
Improve as suggested. Merge two if-branches.

Attach the new patch.
1. Rename the new function(update_progress) to update_replication_progress. [suggestion by Sawada-San]
2. Merge two if-branches in new function update_replication_progress. [suggestion by Sawada-San.]
3. Improve comments to make them clear. [suggestions by Euler-San.]

Regards,
Wang wei

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 18 Apr 2022 06:19:15 +0000

On Thur, Apr 14, 2022 at 8:21 PM Euler Taveira <[email-masked]> wrote:

> We should probable mention that "long time" is wal_receiver_timeout on
Improve as suggested.
Add "(exceeds the wal_receiver_timeout of standby)" to explain what "long time"
means.

Improve as suggested.

Improve as suggested.

Improve as suggested.

Kindly have a look at new patch shared in [1].

[1] - https://www.postgresql.org/message-id/OS3PR01MB627561344A2C7ECF68E41D7E9EF39%40OS3PR01MB6275.jpnprd01.prod.outlook.com

Regards,
Wang wei

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 19 Apr 2022 10:32:07 +0900

+ * For a large transaction, if we don't send any change to the downstream for a
+ * long time(exceeds the wal_receiver_timeout of standby) then it can timeout.
+ * This can happen when all or most of the changes are either not published or
+ * got filtered out.

+ */
+ if(end_xact || ++changes_count >= CHANGES_THRESHOLD)
+ {

We need a whitespace before '(' at above two places. The rest looks good to me.

Regards,

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 19 Apr 2022 01:52:15 +0000

> + * For a large transaction, if we don't send any change to the
> + downstream for a
> + * This can happen when all or most of the changes are either not
> + published or
> + * got filtered out.
> 
> + */
> + if(end_xact || ++changes_count >= CHANGES_THRESHOLD) {
> 
Fix these.

Attach the new patch.
1. Fix wrong formatting. [suggestion by Sawada-San]

Regards,
Wang wei

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 20 Apr 2022 02:46:49 +0000

On Mon, Apr 18, 2022 at 00:36 PM Amit Kapila <[email-masked]> wrote:
> > > OutputPluginUpdateProgress() and the new function update_progress().
> > > The v10 also calls update_progress() for every change action in
> > > pgoutput_change(). It is not a good approach for maintainability --
> > > new changes like sequences need extra calls.
> > >
> >
> > > backpatched. Are you planning to backpatch it? If so, the boolean
> > > variable (last_write or end_xacts depending of which version you are
> > > considering) could be added to LogicalDecodingContext.
> > >
> >
> Even, if we want to add the variable in the struct in back-branches, we need to
> ensure not to change the size of the struct as it is exposed, see email [1] for a
> similar mistake we made in another case.
> 

I did some checks about adding the new variable in LogicalDecodingContext.
I found that because of padding, if we add the new variable at the end of
structure, it dose not make the structure size change. I verified this in
REL_10~REL_14.

So as suggested by Euler-San and Amit-San, I wrote the patch for REL_14. Attach
this patch. To prevent patch confusion, the patch for HEAD is also attached.
The patch for REL_14:
    REL_14_v1-0001-Fix-the-logical-replication-timeout-during-large-.patch
The patch for HEAD:
    v17-0001-Fix-the-logical-replication-timeout-during-large.patch

The following is the details of checks.
On gcc/Linux/x86-64, in REL_14, by looking at the size of each member variable
in the structure LogicalDecodingContext, I found that there are three parts
padding behind the following member variables:
- 7 bytes after fast_forward
- 4 bytes after prepared_write
- 4 bytes after write_xid

If we add the new variable at the end of structure (bool takes one byte), it
means we will only consume one byte of padding after member write_xid. And
then, at the end of the struct, 3 padding are still required. For easy
understanding, please refer to the following simple calculation.
(In REL14, the size of structure LogicalDecodingContext is 304 bytes.)
Before adding new variable (In REL14):
8+8+8+8+8+1+168+8+8+8+8+8+8+8+8+1+1+1+1+8+4  =  ‭289 (if padding is not considered)
         +7                          +4  +4  =  +15 (the padding)
So, the size of structure LogicalDecodingContext is 289+15=304.
After adding new variable (In REL14 with patch):
8+8+8+8+8+1+168+8+8+8+8+8+8+8+8+1+1+1+1+8+4+1  =  ‭290‬ (if padding is not considered)
         +7                          +4    +3  =  +14 (the padding)
So, the size of structure LogicalDecodingContext is 290+14=304.

BTW, the size of structure LogicalDecodingContext in REL_10~REL_13 is 184, 200,
200,200 respectively. And I found that at the end of the structure
LogicalDecodingContext, there are always the following members:
```
    XLogRecPtr  write_location;   --> 8
    TransactionId write_xid;      --> 4
                                  --> There are 4 padding after write_xid.
```
It means at the end of structure LogicalDecodingContext, there are 4 bytes
padding. So, if we add a new bool type variable (It takes one byte) at the end
of the structure LogicalDecodingContext, I think in the current REL_10~REL_14,
because of padding, the size of the structure LogicalDecodingContext will not
change.

Regards,
Wang wei

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 20 Apr 2022 16:20:48 +0900

>                                   --> There are 4 padding after write_xid.

I'm concerned that this 4-byte padding at the end of the struct could
depend on platforms (there might be no padding in 32-bit platforms?).
It seems to me that it's better to put it after fast_forward where the
new field should fall within the padding space.

BTW the changes in
REL_14_v1-0001-Fix-the-logical-replication-timeout-during-large-.patch,
adding end_xact to LogicalDecodingContext, seems good to me and it
might be better than the approach of v17 patch from plugin developers’
perspective? This is because they won’t need to pass true/false to
end_xact of  OutputPluginUpdateProgress(). Furthermore, if we do what
we do in update_replication_progress() in
OutputPluginUpdateProgress(), what plugins need to do will be just to
call OutputPluginUpdate() in every callback and they don't need to
have the CHANGES_THRESHOLD logic. What do you think?

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Wed, 20 Apr 2022 14:38:09 +0530

> depend on platforms (there might be no padding in 32-bit platforms?).

Good point, but ...

> new field should fall within the padding space.

Can we add the variable in between the existing variables in the
structure in the back branches? Normally, we add at the end to avoid
any breakage of existing apps. See commit 56e366f675 and discussion at
[1]. That is related to enum but I think we follow the same for
structures.

[1] - https://www.postgresql.org/message-id/7dab0929-a966-0c0a-4726-878fced2fe00%40enterprisedb.com
-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Wed, 20 Apr 2022 15:42:39 +0530

On Wed, Apr 20, 2022 at 2:38 PM Amit Kapila <[email-masked]> wrote:
> structure in the back branches?
>

I think it should be fine if it falls in the padding space. We have
done similar changes recently in back-branches [1]. I think it would
be then better to have it in the same place in HEAD as well?

[1] - https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=10520f4346876aad4941797c2255a21bdac74739

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 20 Apr 2022 21:51:52 +0900

On Wed, Apr 20, 2022 at 7:12 PM Amit Kapila <[email-masked]> wrote:
> done similar changes recently in back-branches [1].

Yes.

> I think it would

As far as I can see in the v17 patch, which is for HEAD, we don't add
a variable to LogicalDecodingContext, but did you refer to another
patch?

Regards,

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 21 Apr 2022 02:14:41 +0000

On Wed, Apr 20, 2022 at 6:13 PM Amit Kapila <[email-masked]> wrote:
> [1] -
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=10520f4346
> 876aad4941797c2255a21bdac74739

The comments by Sawada-San sound reasonable to me.
After doing check, I found that padding in HEAD is the same as in REL14.
So I change the approach of patch for HEAD just like the patch for REL14.

> new field should fall within the padding space.
Fixed. Add new variable after fast_forward.

> have the CHANGES_THRESHOLD logic. What do you think?
Change the approach of patch for HEAD. (The size of structure does not change.)
Also move the logical of function update_replication_progress to function
OutputPluginUpdateProgress.

Attach the patches. [suggestion by Sawada-San]
1. Change the position of the new variable in structure.
2. Change the approach of the patch for HEAD.
3. Delete the new function update_replication_progress.

Regards,
Wang wei

---

From: Amit Kapila <[email-masked]>
Date: Thu, 21 Apr 2022 07:49:29 +0530

> a variable to LogicalDecodingContext, but did you refer to another

No, I thought it is better to follow the same approach in HEAD as
well. Do you see any problem with it?

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 21 Apr 2022 13:59:50 +0900

> well. Do you see any problem with it?

No, that makes sense to me.

Regards,

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 21 Apr 2022 09:50:57 +0000

> So I change the approach of patch for HEAD just like the patch for REL14.

Also attach the back-branch patches for REL10~REL13.
(REL12 and REL11 patch are the same, so only post one patch for these two
branches.)

The patch for HEAD:
    HEAD_v18-0001-Fix-the-logical-replication-timeout-during-large.patch
The patch for REL14:
    REL14_v2-0001-Fix-the-logical-replication-timeout-during-large-.patch
The patch for REL13:
    REL13_v1-0001-Fix-the-logical-replication-timeout-during-large-.patch
The patch for REL12 and REL11:
    REL12-REL11_v1-0001-Fix-the-logical-replication-timeout-during-large-.patch
The patch for REL10:
    REL10_v1-0001-Fix-the-logical-replication-timeout-during-large-.patch

BTW, after doing check, I found that padding in REL11~REL13 are similar as HEAD
and REL14 (7 bytes padding after fast_forward). But in REL10, the padding is
different. There are three parts padding behind the following member variables:
- 4 bytes after options
- 6 bytes after prepared_write
- 4 bytes after write_xid
So, in the patches for branches REL11~HEAD, I add the new variable after
fast_forward. In the patch for branch REL10, I add the new variable after
prepared_write.
For each version, the size of the structure does not change after applying the
patch.

Regards,
Wang wei

---

From: Amit Kapila <[email-masked]>
Date: Thu, 28 Apr 2022 15:55:43 +0530

On Thu, Apr 21, 2022 at 3:21 PM [email-masked]
<[email-masked]> wrote:

I think it is better to keep the new variable 'end_xact' at the end of
the struct where it belongs for HEAD. In back branches, we can keep it
at the place as you have. Apart from that, I have made some cosmetic
changes and changed a few comments in the attached. Let's use this to
prepare patches for back-branches.

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Fri, 29 Apr 2022 05:35:58 +0000

> prepare patches for back-branches.
Thanks for your review and improvement.

I improved the back-branch patches according to your modifications.
Attach the back-branch patches for REL10~REL14.
(Also attach the patch for HEAD, I did not make any changes to this patch.)

BTW, I found Hou-san shared some points. After our discussion, I will update
the patches if required.

Regards,
Wang wei