[BUG]Update Toast data failure in logical replication

On Friday, May 28, 2021 2:16 PM, tanghy.fnst@fujitsu.com wrote:

I think I just found a bug in logical replication. Data couldn't be synchronized while updating toast data.
Could anyone take a look at it?

FYI. The problem also occurs in PG-13. I will try to check from which version it got introduced.

Regards,
Tang

On Friday, May 28, 2021 3:02 PM, tanghy.fnst@fujitsu.com wrote:

FYI. The problem also occurs in PG-13. I will try to check from which version it
got introduced.

I reproduced it in PG-10,11,12,13.
I think the problem has been existing since Logical replication introduced in PG-10.

Regards
Tang

On Fri, May 28, 2021 at 12:31 PM tanghy.fnst@fujitsu.com
<tanghy.fnst@fujitsu.com> wrote:

On Friday, May 28, 2021 3:02 PM, tanghy.fnst@fujitsu.com wrote:

FYI. The problem also occurs in PG-13. I will try to check from which version it
got introduced.

I reproduced it in PG-10,11,12,13.
I think the problem has been existing since Logical replication introduced in PG-10.

Seems you did not set the replica identity for updating the tuple.
Try this before updating, and it should work.

ALTER TABLE toasted_key REPLICA IDENTITY USING INDEX toasted_key_pkey;

or

ALTER TABLE toasted_key REPLICA IDENTITY FULL.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Friday, May 28, 2021 6:51 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:

Seems you did not set the replica identity for updating the tuple.
Try this before updating, and it should work.

Thanks for your reply. I tried it.

ALTER TABLE toasted_key REPLICA IDENTITY USING INDEX toasted_key_pkey;

This didn't work.

or

ALTER TABLE toasted_key REPLICA IDENTITY FULL.

It worked.

And I noticed if the length of PRIMARY KEY (toasted_key) is short, data could be synchronized successfully with default replica identity.
Could you tell me why we need to set replica identity?

Regards
Tang

On Mon, May 31, 2021 at 8:04 AM tanghy.fnst@fujitsu.com
<tanghy.fnst@fujitsu.com> wrote:

On Friday, May 28, 2021 6:51 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:

Seems you did not set the replica identity for updating the tuple.
Try this before updating, and it should work.

Thanks for your reply. I tried it.

ALTER TABLE toasted_key REPLICA IDENTITY USING INDEX toasted_key_pkey;

This didn't work.

or

ALTER TABLE toasted_key REPLICA IDENTITY FULL.

It worked.

And I noticed if the length of PRIMARY KEY (toasted_key) is short, data could be synchronized successfully with default replica identity.
Could you tell me why we need to set replica identity?

Looks like some problem if the replica identity is an index and the
value is stored externally, I will debug this and let you know.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Mon, May 31, 2021 at 12:20 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, May 31, 2021 at 8:04 AM tanghy.fnst@fujitsu.com
<tanghy.fnst@fujitsu.com> wrote:

On Friday, May 28, 2021 6:51 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:

Seems you did not set the replica identity for updating the tuple.
Try this before updating, and it should work.

Thanks for your reply. I tried it.

ALTER TABLE toasted_key REPLICA IDENTITY USING INDEX toasted_key_pkey;

This didn't work.

or

ALTER TABLE toasted_key REPLICA IDENTITY FULL.

It worked.

And I noticed if the length of PRIMARY KEY (toasted_key) is short, data could be synchronized successfully with default replica identity.
Could you tell me why we need to set replica identity?

Looks like some problem if the replica identity is an index and the
value is stored externally, I will debug this and let you know.

The problem is if the key attribute is not changed we don't log it as
it should get logged along with the updated tuple, but if it is
externally stored then the complete key will never be logged because
there is no log from the toast table. For fixing this if the key is
externally stored then always log that.

Please test with the attached patch.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Mon, May 31, 2021 5:12 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

The problem is if the key attribute is not changed we don't log it as
it should get logged along with the updated tuple, but if it is
externally stored then the complete key will never be logged because
there is no log from the toast table. For fixing this if the key is
externally stored then always log that.

Please test with the attached patch.

Thanks for your patch. I tested it and the bug was fixed.
I'm still trying to understand your fix, please allow me to ask more(maybe silly) questions if I found any.

+ * if the key hasn't changedand we're only logging the key, we're done.

I think "changedand" should be "changed and".

Regards
Tang

On Mon, 31 May 2021 at 3:33 PM, tanghy.fnst@fujitsu.com <
tanghy.fnst@fujitsu.com> wrote:

On Mon, May 31, 2021 5:12 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

The problem is if the key attribute is not changed we don't log it as
it should get logged along with the updated tuple, but if it is
externally stored then the complete key will never be logged because
there is no log from the toast table. For fixing this if the key is
externally stored then always log that.

Please test with the attached patch.

Thanks for your patch. I tested it and the bug was fixed.

Thanks for confirming this.

I'm still trying to understand your fix, please allow me to ask more(maybe
silly) questions if I found any.

+ * if the key hasn't changedand we're only logging the key, we're
done.

I think "changedand" should be "changed and".

Okay, I will fix it. Lets see what others have to say about this fix, if
we agree with this then I think we might have to change the test output. I
will do that in the next version along with your comment fix.

Hi

I have some questions with your patch. Here are two cases I used to check the bug.

Case1(PK toasted_key is short), data could be synchronized on HEAD.
---------------
INSERT INTO toasted_key(toasted_key, toasted_col1) VALUES('111', repeat('9876543210', 200));
UPDATE toasted_key SET toasted_col2 = toasted_col1;
---------------

Case2(PK toasted_key is very long), data couldn’t be synchronized on HEAD.(which is the bug)
---------------
INSERT INTO toasted_key(toasted_key, toasted_col1) VALUES(repeat('9876543210', 200), '111');
UPDATE toasted_key SET toasted_col2 = toasted_col1;
---------------

So I think the bug is only related with the length of primary key.
I noticed that in case1, ExtractReplicaIdentity function returned NULL on HEAD. But after your fix, it didn’t return NULL. There is no problem with this case on HEAD, but the patch modified its return value. I’m not sure if it would bring new problems. Have you checked it?

Regards
Tang

On Tue, Jun 1, 2021 at 12:29 PM tanghy.fnst@fujitsu.com
<tanghy.fnst@fujitsu.com> wrote:

Hi

I have some questions with your patch. Here are two cases I used to check the bug.

Case1(PK toasted_key is short), data could be synchronized on HEAD.

---------------

INSERT INTO toasted_key(toasted_key, toasted_col1) VALUES('111', repeat('9876543210', 200));

UPDATE toasted_key SET toasted_col2 = toasted_col1;

---------------

Case2(PK toasted_key is very long), data couldn’t be synchronized on HEAD.(which is the bug)

---------------

INSERT INTO toasted_key(toasted_key, toasted_col1) VALUES(repeat('9876543210', 200), '111');

UPDATE toasted_key SET toasted_col2 = toasted_col1;

---------------

So I think the bug is only related with the length of primary key.

I noticed that in case1, ExtractReplicaIdentity function returned NULL on HEAD. But after your fix, it didn’t return NULL. There is no problem with this case on HEAD, but the patch modified its return value. I’m not sure if it would bring new problems. Have you checked it?

Good observation, basically, my check says that any field in the tuple
is toasted then prepare the key tuple, actually, after that, I should
recheck whether the key field specifically toasted or not and if it is
not then we can continue returning NULL. I will fix this in the next
version.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Tue, Jun 1, 2021 at 3:39 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Tue, Jun 1, 2021 at 12:29 PM tanghy.fnst@fujitsu.com

I noticed that in case1, ExtractReplicaIdentity function returned NULL on HEAD. But after your fix, it didn’t return NULL. There is no problem with this case on HEAD, but the patch modified its return value. I’m not sure if it would bring new problems. Have you checked it?

Good observation, basically, my check says that any field in the tuple
is toasted then prepare the key tuple, actually, after that, I should
recheck whether the key field specifically toasted or not and if it is
not then we can continue returning NULL. I will fix this in the next
version.

Attached patch fixes that, I haven't yet added the test case. Once
someone confirms on the approach then I will add a test case to the
patch.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Wed, Jun 2, 2021 2:44 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

Attached patch fixes that, I haven't yet added the test case. Once
someone confirms on the approach then I will add a test case to the
patch.

	key_tuple = heap_form_tuple(desc, values, nulls);
	*copy = true;
...
		key_tuple = toast_flatten_tuple(oldtup, desc);
 		heap_freetuple(oldtup);
 	}
+	/*
+	 * If key tuple doesn't have any external data and key is not changed then
+	 * just free the key tuple and return NULL.
+	 */
+	else if (!key_changed)
+	{
+		heap_freetuple(key_tuple);
+		return NULL;
+	}

return key_tuple;
}

I think "*copy = false" should be added before return NULL because we don't return a modified copy tuple here. Thoughts?

Regards
Tang

On Wed, Jun 2, 2021 at 2:37 PM tanghy.fnst@fujitsu.com
<tanghy.fnst@fujitsu.com> wrote:

On Wed, Jun 2, 2021 2:44 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

Attached patch fixes that, I haven't yet added the test case. Once
someone confirms on the approach then I will add a test case to the
patch.

key_tuple = heap_form_tuple(desc, values, nulls);
*copy = true;
...
key_tuple = toast_flatten_tuple(oldtup, desc);
heap_freetuple(oldtup);
}
+       /*
+        * If key tuple doesn't have any external data and key is not changed then
+        * just free the key tuple and return NULL.
+        */
+       else if (!key_changed)
+       {
+               heap_freetuple(key_tuple);
+               return NULL;
+       }

return key_tuple;
}

I think "*copy = false" should be added before return NULL because we don't return a modified copy tuple here. Thoughts?

Yes, you are right. I will change it in the next version, along with
the test case.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Wed, Jun 2, 2021 at 3:10 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

Yes, you are right. I will change it in the next version, along with
the test case.

+    /*
+     * if the key hasn't changed and we're only logging the key, we're done.
+     * But if tuple has external data then we might have to detoast the key.
+     */
This doesn't really mention why we need to detoast the key even when
the key remains the same. I guess we can add some more details here.

--
Thanks & Regards,
Kuntal Ghosh

On Wed, Jun 2, 2021 at 7:20 PM Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:

On Wed, Jun 2, 2021 at 3:10 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

Yes, you are right. I will change it in the next version, along with
the test case.

+    /*
+     * if the key hasn't changed and we're only logging the key, we're done.
+     * But if tuple has external data then we might have to detoast the key.
+     */
This doesn't really mention why we need to detoast the key even when
the key remains the same. I guess we can add some more details here.

Noted, let's see what others have to say about fixing this, then I
will fix this along with one other pending comment and I will also add
the test case. Thanks for looking into this.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Wed, Jun 2, 2021 at 7:23 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Wed, Jun 2, 2021 at 7:20 PM Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:

On Wed, Jun 2, 2021 at 3:10 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

Yes, you are right. I will change it in the next version, along with
the test case.

+    /*
+     * if the key hasn't changed and we're only logging the key, we're done.
+     * But if tuple has external data then we might have to detoast the key.
+     */
This doesn't really mention why we need to detoast the key even when
the key remains the same. I guess we can add some more details here.

Noted, let's see what others have to say about fixing this, then I
will fix this along with one other pending comment and I will also add
the test case. Thanks for looking into this.

I have fixed all the pending issues, I see there is already a test
case for this so I have changed the output for that.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Thu, Jun 3, 2021 7:45 PMDilip Kumar <dilipbalaut@gmail.com> wrote:

I have fixed all the pending issues, I see there is already a test
case for this so I have changed the output for that.

Thanks for your patch. I tested it for all branches(10,11,12,13,HEAD) and all of them passed.(This bug was introduced in PG-10.)
I also tested the scenario where I found this bug, data could be synchronized after your fix.

Regards
Tang

On Fri, Jun 4, 2021 at 8:25 AM tanghy.fnst@fujitsu.com
<tanghy.fnst@fujitsu.com> wrote:

On Thu, Jun 3, 2021 7:45 PMDilip Kumar <dilipbalaut@gmail.com> wrote:

I have fixed all the pending issues, I see there is already a test
case for this so I have changed the output for that.

Thanks for your patch. I tested it for all branches(10,11,12,13,HEAD) and all of them passed.(This bug was introduced in PG-10.)
I also tested the scenario where I found this bug, data could be synchronized after your fix.

Thanks for verifying this.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Thu, Jun 3, 2021 at 5:15 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Wed, Jun 2, 2021 at 7:23 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Wed, Jun 2, 2021 at 7:20 PM Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:

On Wed, Jun 2, 2021 at 3:10 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

Yes, you are right. I will change it in the next version, along with
the test case.

+    /*
+     * if the key hasn't changed and we're only logging the key, we're done.
+     * But if tuple has external data then we might have to detoast the key.
+     */
This doesn't really mention why we need to detoast the key even when
the key remains the same. I guess we can add some more details here.

Noted, let's see what others have to say about fixing this, then I
will fix this along with one other pending comment and I will also add
the test case. Thanks for looking into this.

I have fixed all the pending issues, I see there is already a test
case for this so I have changed the output for that.

IIUC, this issue occurs because we don't log the actual key value for
unchanged toast key. It is neither logged as part of old_key_tuple nor
for new tuple due to which we are not able to find it in the
apply-side when we searched it via FindReplTupleInLocalRel. Now, I
think it will work if we LOG the entire unchanged toasted value as you
have done in the patch but can we explore some other way to fix it. In
the subscriber-side, can we detect that the key column has toasted
value in the new tuple and try to first fetch it and then do the index
search for the fetched toasted value? I am not sure about the
feasibility of this but wanted to see if we can someway avoid logging
unchanged toasted key value as that might save us from additional WAL.

--
With Regards,
Amit Kapila.