Обсуждение: [BUG]Invalidate relcache when setting REPLICA IDENTITY

Hi

I think I found a bug related to logical replication(REPLICA IDENTITY in specific).
If I change REPLICA IDENTITY after creating publication,  the DELETE/UPDATE operations won't be replicated as expected.


For example:
-- publisher
CREATE TABLE tbl(a int, b int);
ALTER TABLE tbl ALTER COLUMN a SET NOT NULL;
CREATE UNIQUE INDEX idx_a ON tbl(a);
ALTER TABLE tbl ALTER COLUMN b SET NOT NULL;
CREATE UNIQUE INDEX idx_b ON tbl(b);
ALTER TABLE tbl REPLICA IDENTITY USING INDEX idx_a;
CREATE PUBLICATION pub FOR TABLE tbl;
ALTER TABLE tbl REPLICA IDENTITY USING INDEX idx_b;
INSERT INTO tbl VALUES (1,1);

-- subscriber
CREATE TABLE tbl(a int, b int);
ALTER TABLE tbl ALTER COLUMN b SET NOT NULL;
CREATE UNIQUE INDEX idx_b ON tbl(b);
ALTER TABLE tbl REPLICA IDENTITY USING INDEX idx_b;
CREATE SUBSCRIPTION sub CONNECTION 'dbname=postgres port=5432' PUBLICATION pub;
SELECT * FROM tbl;

-- publisher
postgres=# UPDATE tbl SET a=-a;
UPDATE 1
postgres=# SELECT * FROM tbl;
a  | b
----+---
 -1 | 1
(1 row)

-- subscriber
postgres=# SELECT * FROM tbl;
 a | b
---+---
 1 | 1
(1 row)

(a in subscriber should be -1)

But if I restart a psql client before executing UPDATE operation in publication, it works well.
So I think the problem is: when using "ALTER TABLE ... REPLICA IDENTITY USING INDEX ...", relcahe was not invalidated.

Attach a patch to fix it, I also added a test case for it.(Hou helped me to write/review this patch, which is very kind
ofhim) 

FYI: I also tested that same problem could be reproduced on PG10 ~ PG14. (Logical replication is introduced in PG10.)
So I think backpatch is needed here.

Regards
Tang

On Wed, Nov 10, 2021 at 12:42 PM tanghy.fnst@fujitsu.com
<tanghy.fnst@fujitsu.com> wrote:
>
> Hi
>
> I think I found a bug related to logical replication(REPLICA IDENTITY in specific).
> If I change REPLICA IDENTITY after creating publication,  the DELETE/UPDATE operations won't be replicated as
expected.
>
> For example:
> -- publisher
> CREATE TABLE tbl(a int, b int);
> ALTER TABLE tbl ALTER COLUMN a SET NOT NULL;
> CREATE UNIQUE INDEX idx_a ON tbl(a);
> ALTER TABLE tbl ALTER COLUMN b SET NOT NULL;
> CREATE UNIQUE INDEX idx_b ON tbl(b);
> ALTER TABLE tbl REPLICA IDENTITY USING INDEX idx_a;
> CREATE PUBLICATION pub FOR TABLE tbl;
> ALTER TABLE tbl REPLICA IDENTITY USING INDEX idx_b;
> INSERT INTO tbl VALUES (1,1);
>
> -- subscriber
> CREATE TABLE tbl(a int, b int);
> ALTER TABLE tbl ALTER COLUMN b SET NOT NULL;
> CREATE UNIQUE INDEX idx_b ON tbl(b);
> ALTER TABLE tbl REPLICA IDENTITY USING INDEX idx_b;
> CREATE SUBSCRIPTION sub CONNECTION 'dbname=postgres port=5432' PUBLICATION pub;
> SELECT * FROM tbl;
>
> -- publisher
> postgres=# UPDATE tbl SET a=-a;
> UPDATE 1
> postgres=# SELECT * FROM tbl;
> a  | b
> ----+---
>  -1 | 1
> (1 row)
>
> -- subscriber
> postgres=# SELECT * FROM tbl;
>  a | b
> ---+---
>  1 | 1
> (1 row)
>
> (a in subscriber should be -1)
>

I don't understand the purpose of idx_b in the above test case, why is
it required to reproduce the problem?

@@ -15488,6 +15488,7 @@ relation_mark_replica_identity(Relation rel,
char ri_type, Oid indexOid,
  CatalogTupleUpdate(pg_index, &pg_index_tuple->t_self, pg_index_tuple);
  InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0,
  InvalidOid, is_internal);
+ CacheInvalidateRelcache(rel);

CatalogTupleUpdate internally calls heap_update which calls
CacheInvalidateHeapTuple(), why is that not sufficient for
invalidation?

-- 
With Regards,
Amit Kapila.

On Wed, Nov 10, 2021 7:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Nov 10, 2021 at 12:42 PM tanghy.fnst@fujitsu.com
> <tanghy.fnst@fujitsu.com> wrote:
> >
> > Hi
> >
> > I think I found a bug related to logical replication(REPLICA IDENTITY in
> specific).
> > If I change REPLICA IDENTITY after creating publication,  the
> DELETE/UPDATE operations won't be replicated as expected.
> >
> > For example:
> > -- publisher
> > CREATE TABLE tbl(a int, b int);
> > ALTER TABLE tbl ALTER COLUMN a SET NOT NULL; CREATE UNIQUE INDEX
> idx_a
> > ON tbl(a); ALTER TABLE tbl ALTER COLUMN b SET NOT NULL; CREATE UNIQUE
> > INDEX idx_b ON tbl(b); ALTER TABLE tbl REPLICA IDENTITY USING INDEX
> > idx_a; CREATE PUBLICATION pub FOR TABLE tbl; ALTER TABLE tbl REPLICA
> > IDENTITY USING INDEX idx_b; INSERT INTO tbl VALUES (1,1);
> >
> > -- subscriber
> > CREATE TABLE tbl(a int, b int);
> > ALTER TABLE tbl ALTER COLUMN b SET NOT NULL; CREATE UNIQUE INDEX
> idx_b
> > ON tbl(b); ALTER TABLE tbl REPLICA IDENTITY USING INDEX idx_b; CREATE
> > SUBSCRIPTION sub CONNECTION 'dbname=postgres port=5432'
> PUBLICATION
> > pub; SELECT * FROM tbl;
> >
> > -- publisher
> > postgres=# UPDATE tbl SET a=-a;
> > UPDATE 1
> > postgres=# SELECT * FROM tbl;
> > a  | b
> > ----+---
> >  -1 | 1
> > (1 row)
> >
> > -- subscriber
> > postgres=# SELECT * FROM tbl;
> >  a | b
> > ---+---
> >  1 | 1
> > (1 row)
> >
> > (a in subscriber should be -1)
> >
> 
> I don't understand the purpose of idx_b in the above test case, why is it
> required to reproduce the problem?
> @@ -15488,6 +15488,7 @@ relation_mark_replica_identity(Relation rel, char
> ri_type, Oid indexOid,
>   CatalogTupleUpdate(pg_index, &pg_index_tuple->t_self, pg_index_tuple);
>   InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0,
>   InvalidOid, is_internal);
> + CacheInvalidateRelcache(rel);
> 
> CatalogTupleUpdate internally calls heap_update which calls
> CacheInvalidateHeapTuple(), why is that not sufficient for invalidation?

I think it's because the bug happens only when changing REPLICA IDENTITY index
from one(idx_a) to another one(idx_b).

When first time specify the REPLICA IDENTITY index, the code works well because
it will invoke 'CatalogTupleUpdate(pg_class,...' Which will invalidate the
target table's relcache.

    if (pg_class_form->relreplident != ri_type)
    {
        pg_class_form->relreplident = ri_type;
        CatalogTupleUpdate(pg_class, &pg_class_tuple->t_self, pg_class_tuple);
    }

But when changing REPLICA IDENTITY index, the code only invoke
'CatalogTupleUpdate(pg_index,' which seems only invalidate the index's cache not the
target table.

        if (dirty)
        {
            CatalogTupleUpdate(pg_index, &pg_index_tuple->t_self, pg_index_tuple);
            InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0,
                                         InvalidOid, is_internal);
        }

Best regards,
Hou zj

On Thu, Nov 11, 2021 at 7:07 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Wed, Nov 10, 2021 7:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > I don't understand the purpose of idx_b in the above test case, why is it
> > required to reproduce the problem?
> > @@ -15488,6 +15488,7 @@ relation_mark_replica_identity(Relation rel, char
> > ri_type, Oid indexOid,
> >   CatalogTupleUpdate(pg_index, &pg_index_tuple->t_self, pg_index_tuple);
> >   InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0,
> >   InvalidOid, is_internal);
> > + CacheInvalidateRelcache(rel);
> >
> > CatalogTupleUpdate internally calls heap_update which calls
> > CacheInvalidateHeapTuple(), why is that not sufficient for invalidation?
>
> I think it's because the bug happens only when changing REPLICA IDENTITY index
> from one(idx_a) to another one(idx_b).
>

Okay, but do we need to invalidate the rel cache each time the dirty
flag is set? Can't we do it once outside the foreach index loop? I
think we can record whether to do relcache invalidation in a new
boolean variable need_rel_inval or something like that. Also, it is
better if you can add a comment on the lines of: "Invalidate the
relcache for the table so that after we commit all sessions will
refresh the table's replica identity index before attempting any
update on the table.".

-- 
With Regards,
Amit Kapila.

On Thu, Nov 11, 2021 at 9:37 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Nov 11, 2021 at 7:07 AM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Wed, Nov 10, 2021 7:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > I don't understand the purpose of idx_b in the above test case, why is it
> > > required to reproduce the problem?
> > > @@ -15488,6 +15488,7 @@ relation_mark_replica_identity(Relation rel, char
> > > ri_type, Oid indexOid,
> > >   CatalogTupleUpdate(pg_index, &pg_index_tuple->t_self, pg_index_tuple);
> > >   InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0,
> > >   InvalidOid, is_internal);
> > > + CacheInvalidateRelcache(rel);
> > >
> > > CatalogTupleUpdate internally calls heap_update which calls
> > > CacheInvalidateHeapTuple(), why is that not sufficient for invalidation?
> >
> > I think it's because the bug happens only when changing REPLICA IDENTITY index
> > from one(idx_a) to another one(idx_b).
> >
>
> Okay, but do we need to invalidate the rel cache each time the dirty
> flag is set? Can't we do it once outside the foreach index loop? I
> think we can record whether to do relcache invalidation in a new
> boolean variable need_rel_inval or something like that. Also, it is
> better if you can add a comment on the lines of: "Invalidate the
> relcache for the table so that after we commit all sessions will
> refresh the table's replica identity index before attempting any
> update on the table.".
>

Few comments on testcase added by the patch:
==================================
1.
+$node_subscriber->safe_psql('postgres',
+);

It is not clear what is the purpose served by this statement.

2.
+# replication of the table without replica identity index but not primary key
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab_replidentity_index(a int not null, b int not null)");

I think the part of the comment "but not primary key" is not required.

3. Can we move this test to subscription/t/100_bugs?

-- 
With Regards,
Amit Kapila.

On Thur, Nov 11, 2021 12:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Nov 11, 2021 at 7:07 AM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote:
> >
> > On Wed, Nov 10, 2021 7:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > I don't understand the purpose of idx_b in the above test case, why is it
> > > required to reproduce the problem?
> > > @@ -15488,6 +15488,7 @@ relation_mark_replica_identity(Relation rel,
> char
> > > ri_type, Oid indexOid,
> > >   CatalogTupleUpdate(pg_index, &pg_index_tuple->t_self,
> pg_index_tuple);
> > >   InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0,
> > >   InvalidOid, is_internal);
> > > + CacheInvalidateRelcache(rel);
> > >
> > > CatalogTupleUpdate internally calls heap_update which calls
> > > CacheInvalidateHeapTuple(), why is that not sufficient for invalidation?
> >
> > I think it's because the bug happens only when changing REPLICA IDENTITY
> index
> > from one(idx_a) to another one(idx_b).
> >
> 
> Okay, but do we need to invalidate the rel cache each time the dirty
> flag is set? Can't we do it once outside the foreach index loop? I
> think we can record whether to do relcache invalidation in a new
> boolean variable need_rel_inval or something like that. Also, it is
> better if you can add a comment on the lines of: "Invalidate the
> relcache for the table so that after we commit all sessions will
> refresh the table's replica identity index before attempting any
> update on the table.".

I agree.

Attach the new version fix patch which move the invalidation out of the loop
and addressed the comments[1] for the testcases.

[1] https://www.postgresql.org/message-id/CAA4eK1Ki-kUx52uRER3bZSC6bLc%3Diix%2BQnBxs1pZSHHYZOEF1A%40mail.gmail.com

Best regards,
Hou zj

On Thursday, November 11, 2021 5:36 PM houzj.fnst@fujitsu.com wrote:
> On Thur, Nov 11, 2021 12:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Thu, Nov 11, 2021 at 7:07 AM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> > >
> > > On Wed, Nov 10, 2021 7:29 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> > > >
> > > > I don't understand the purpose of idx_b in the above test case,
> > > > why is it required to reproduce the problem?
> > > > @@ -15488,6 +15488,7 @@ relation_mark_replica_identity(Relation
> > > > rel,
> > char
> > > > ri_type, Oid indexOid,
> > > >   CatalogTupleUpdate(pg_index, &pg_index_tuple->t_self,
> > pg_index_tuple);
> > > >   InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0,
> > > >   InvalidOid, is_internal);
> > > > + CacheInvalidateRelcache(rel);
> > > >
> > > > CatalogTupleUpdate internally calls heap_update which calls
> > > > CacheInvalidateHeapTuple(), why is that not sufficient for invalidation?
> > >
> > > I think it's because the bug happens only when changing REPLICA
> > > IDENTITY
> > index
> > > from one(idx_a) to another one(idx_b).
> > >
> >
> > Okay, but do we need to invalidate the rel cache each time the dirty
> > flag is set? Can't we do it once outside the foreach index loop? I
> > think we can record whether to do relcache invalidation in a new
> > boolean variable need_rel_inval or something like that. Also, it is
> > better if you can add a comment on the lines of: "Invalidate the
> > relcache for the table so that after we commit all sessions will
> > refresh the table's replica identity index before attempting any
> > update on the table.".
> 
> I agree.
> 
> Attach the new version fix patch which move the invalidation out of the loop and
> addressed the comments[1] for the testcases.

Also attach the patches for back branch and remove some unnecessary
changes from pgindent.

Best regards,
Hou zj

Also attach the patches for back branch and remove some unnecessary

changes from pgindent.

On Friday, November 12, 2021 8:15 AM Euler Taveira <euler@eulerto.com> wrote:
> I reviewed your patch and I think the fix could be simplified by
>
> if (OidIsValid(indexOid))
> CacheInvalidateRelcache(rel);
>
> If indexOid is valid it is a REPLICA IDENTITY INDEX. A few lines above there is
> a check for a valid indexOid that makes sure the index is already marked as a
> replica identity; if so, it bail out. If it is not, the relation should be
> invalidated. Am I missing something?

Thanks for reviewing !
But I am not sure it's better to simplify the code like
"if (OidIsValid(indexOid)) CacheInvalidate". After this change, it will
also invalidate the relcache when changing REPLICA IDENTITY from
DEFAULT/FULL/NOTHING to USING INDEX which I think is unnecessary, because the
invalidate for these cases was already handled by
"CatalogTupleUpdate(pg_class".

So, I still think the flag need_rel_inval is needed.

> I also modified your test case to include a DELETE command, wait the initial
> table sync to avoid failing a subsequent test and improve some comments.
Thanks!

Attach the patch which changed the code use need_rel_inval flag.
Also ran pgperltidy for the testcases. I will post patches for backbranch
if there are no more comments.

Best regards,
Hou zj

On Friday, November 12, 2021 10:46 AM I wrote:
> On Friday, November 12, 2021 8:15 AM Euler Taveira <euler@eulerto.com>
> wrote:
> > I reviewed your patch and I think the fix could be simplified by
> >
> > if (OidIsValid(indexOid))
> > CacheInvalidateRelcache(rel);
> >
> > If indexOid is valid it is a REPLICA IDENTITY INDEX. A few lines above
> > there is a check for a valid indexOid that makes sure the index is
> > already marked as a replica identity; if so, it bail out. If it is
> > not, the relation should be invalidated. Am I missing something?
>
> Thanks for reviewing !
> But I am not sure it's better to simplify the code like "if (OidIsValid(indexOid))
> CacheInvalidate".

Oh, I got confused with the logic in relation_mark_replica_identity, sorry for that.
I now realize that you are right, we can just check "if (OidIsValid(indexOid))" here
to simplify the code.

Attach the new version patch which simplify the check as pointed out by Euler.

Best regards,
Hou zj

On Fri, Nov 12, 2021 at 10:27 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Friday, November 12, 2021 10:46 AM I wrote:
> > On Friday, November 12, 2021 8:15 AM Euler Taveira <euler@eulerto.com>
> > wrote:
> > > I reviewed your patch and I think the fix could be simplified by
> > >
> > > if (OidIsValid(indexOid))
> > > CacheInvalidateRelcache(rel);
> > >
> > > If indexOid is valid it is a REPLICA IDENTITY INDEX. A few lines above
> > > there is a check for a valid indexOid that makes sure the index is
> > > already marked as a replica identity; if so, it bail out. If it is
> > > not, the relation should be invalidated. Am I missing something?
> >
> > Thanks for reviewing !
> > But I am not sure it's better to simplify the code like "if (OidIsValid(indexOid))
> > CacheInvalidate".
>
> Oh, I got confused with the logic in relation_mark_replica_identity, sorry for that.
> I now realize that you are right, we can just check "if (OidIsValid(indexOid))" here
> to simplify the code.
>

But won't that generate invalidation for the rel twice in the case
(change Replica Identity from Nothing to some index) you mentioned in
the previous email?


-- 
With Regards,
Amit Kapila.

On Fri, Nov 12, 2021 at 10:50 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Nov 12, 2021 at 10:27 AM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Friday, November 12, 2021 10:46 AM I wrote:
> > > On Friday, November 12, 2021 8:15 AM Euler Taveira <euler@eulerto.com>
> > > wrote:
> > > > I reviewed your patch and I think the fix could be simplified by
> > > >
> > > > if (OidIsValid(indexOid))
> > > > CacheInvalidateRelcache(rel);
> > > >
> > > > If indexOid is valid it is a REPLICA IDENTITY INDEX. A few lines above
> > > > there is a check for a valid indexOid that makes sure the index is
> > > > already marked as a replica identity; if so, it bail out. If it is
> > > > not, the relation should be invalidated. Am I missing something?
> > >
> > > Thanks for reviewing !
> > > But I am not sure it's better to simplify the code like "if (OidIsValid(indexOid))
> > > CacheInvalidate".
> >
> > Oh, I got confused with the logic in relation_mark_replica_identity, sorry for that.
> > I now realize that you are right, we can just check "if (OidIsValid(indexOid))" here
> > to simplify the code.
> >
>
> But won't that generate invalidation for the rel twice in the case
> (change Replica Identity from Nothing to some index) you mentioned in
> the previous email?
>

Oh, I see the point. I think this is okay because
AddRelcacheInvalidationMessage doesn't allow to add duplicate rel
invalidation. If that is the case I wonder why not simply register
invalidation without any check in the for loop as was the case with
Tang's original patch?


-- 
With Regards,
Amit Kapila.

On Fri, Nov 12, 2021 1:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Nov 12, 2021 at 10:50 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > But won't that generate invalidation for the rel twice in the case
> > (change Replica Identity from Nothing to some index) you mentioned in
> > the previous email?
> >
> 
> Oh, I see the point. I think this is okay because
> AddRelcacheInvalidationMessage doesn't allow to add duplicate rel
> invalidation. If that is the case I wonder why not simply register
> invalidation without any check in the for loop as was the case with
> Tang's original patch?

OK, I also think the code in Tang's original patch is fine.
Attach the patch which register invalidation without any check in the for loop.

Best regards,
Hou zj

On Fri, Nov 12, 2021 1:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

> On Fri, Nov 12, 2021 at 10:50 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

> > But won't that generate invalidation for the rel twice in the case

> > (change Replica Identity from Nothing to some index) you mentioned in

> > the previous email?

> >

> 

> Oh, I see the point. I think this is okay because

> AddRelcacheInvalidationMessage doesn't allow to add duplicate rel

> invalidation. If that is the case I wonder why not simply register

> invalidation without any check in the for loop as was the case with

> Tang's original patch?

OK, I also think the code in Tang's original patch is fine.

Attach the patch which register invalidation without any check in the for loop.

On Sat, Nov 13, 2021 at 12:47 AM Euler Taveira <euler@eulerto.com> wrote:
>
> On Fri, Nov 12, 2021, at 3:10 AM, houzj.fnst@fujitsu.com wrote:
>
> On Fri, Nov 12, 2021 1:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Fri, Nov 12, 2021 at 10:50 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > But won't that generate invalidation for the rel twice in the case
> > > (change Replica Identity from Nothing to some index) you mentioned in
> > > the previous email?
> > >
> >
> > Oh, I see the point. I think this is okay because
> > AddRelcacheInvalidationMessage doesn't allow to add duplicate rel
> > invalidation. If that is the case I wonder why not simply register
> > invalidation without any check in the for loop as was the case with
> > Tang's original patch?
>
> OK, I also think the code in Tang's original patch is fine.
> Attach the patch which register invalidation without any check in the for loop.
>
> WFM.
>

The patch looks mostly good to me. I have slightly tweaked the
comments in the code (as per my previous suggestion) and test. Also, I
have slightly modified the commit message. If the attached looks good
to you then kindly prepare patches for back-branches.

-- 
With Regards,
Amit Kapila.

On Sat, Nov 13, 2021 6:50 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> The patch looks mostly good to me. I have slightly tweaked the comments in
> the code (as per my previous suggestion) and test. Also, I have slightly
> modified the commit message. If the attached looks good to you then kindly
> prepare patches for back-branches.

Thanks for reviewing, the latest patch looks good to me.
Attach the patches for back branch (HEAD ~ v10).

Best regards,
Hou zj

On Mon, Nov 15, 2021 at 7:20 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Sat, Nov 13, 2021 6:50 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > The patch looks mostly good to me. I have slightly tweaked the comments in
> > the code (as per my previous suggestion) and test. Also, I have slightly
> > modified the commit message. If the attached looks good to you then kindly
> > prepare patches for back-branches.
>
> Thanks for reviewing, the latest patch looks good to me.
> Attach the patches for back branch (HEAD ~ v10).
>

Pushed.

-- 
With Regards,
Amit Kapila.