PS-7806: Column compression breaks async replication on PS #4460

nitendra-bhosle · 2021-09-10T14:30:20Z

https://jira.percona.com/browse/PS-7806

Analysis
compress_heap memory was released inside
method row_sel_store_mysql_rec , even though ,it was reused.
Which caused incorrect bin logging for comressed column,
leading to async replication to break.

Fix

Removed compress_heap free statements,
hence memory is reused.

https://jira.percona.com/browse/PS-7806 Analysis compress_heap memory was released inside method row_sel_store_mysql_rec , even though ,it was reused. Which caused incorrect bin logging for comressed column, leading to async replication to break. Fix Removed compress_heap free statements, hence memory is reused.

nitendra-bhosle · 2021-09-10T14:40:51Z

Jenkins : https://ps80.cd.percona.com/view/8.0/job/percona-server-8.0-param/1050/

storage/innobase/row/row0sel.cc

https://jira.percona.com/browse/PS-7806 Analysis compress_heap memory was not emptied inside method row_sel_store_mysql_rec ,instead it was getting freed. Which caused incorrect bin logging for comressed column, leading to async replication to break. Fix Removed compress_heap free statements, and added code to empty compress_heap heap.

venkatesh-prasad-v · 2021-09-15T13:26:22Z

mysql-test/suite/binlog/t/binlog_stm_delete_with_compress_column.test

+# This is a wrapper for binlog_delete_with_compress_column.test so that the same
+# test case can be used for both statement and row based logging
+
+-- source include/have_binlog_format_mixed_or_statement.inc
+-- source extra/binlog_tests/binlog_delete_with_compress_column.test


I don't think there is no any need to run the test in statement format, as corrupted data is not propagated in statement based replication. So, I suggest you to please remove this test.

venkatesh-prasad-v · 2021-09-15T13:33:20Z

mysql-test/extra/binlog_tests/binlog_delete_with_compress_column.test

+#
+# PS-7806: Column compression breaks async replication on PS
+#


The tests pass even without fix, so please add a replication test (rpl suite) as this bug was initially found in a replication scenario.

------------------------------------------------------------------------------ [ 14%] binlog.binlog_row_delete_with_compress_column 'mix' [ skipped ] Doesn't support --binlog-format = 'mixed' [ 28%] binlog.binlog_stm_delete_with_compress_column 'mix' [ pass ] 4712 [ 42%] binlog.binlog_row_delete_with_compress_column 'row' [ pass ] 5768 [ 57%] binlog.binlog_stm_delete_with_compress_column 'row' [ skipped ] Doesn't support --binlog-format = 'row' [ 71%] binlog.binlog_row_delete_with_compress_column 'stmt' [ skipped ] Doesn't support --binlog-format = 'statement' [ 85%] binlog.binlog_stm_delete_with_compress_column 'stmt' [ pass ] 4456 [100%] shutdown_report [ pass ]

satya-bodapati · 2021-09-16T05:23:57Z

storage/innobase/row/row0sel.cc

-  if (UNIV_LIKELY_NULL(prebuilt->compress_heap))
-    row_mysql_prebuilt_free_compress_heap(prebuilt);
+  if (prebuilt->compress_heap != nullptr) {
+    mem_heap_empty(prebuilt->compress_heap);


If we are reusing, even emptying is a problem isn't it? I think partitions are supposed to their partition-specific blob heaps? Can you please check again?

Sorry used the wrong button.. "not approved yet" :)

@nitendra-bhosle can you please reply to these questions?

I don't understand the issue. Specifically why free is a problem. and empty is not?
AFAIU, empty doesn't the remove the first block. So may be the compressed heap is staying within the first block of heap.

Can you please create a blob text such that it occupies more than the first block in heap?

This is outdated changes.

https://jira.percona.com/browse/PS-7806 Analysis compress_heap memory was released inside method row_sel_store_mysql_rec , even though ,it was reused. Which caused incorrect bin logging for comressed column, leading to async replication to break. Fix Removed compress_heap free statements, hence memory is reused.

mysql-test/suite/rpl/t/rpl_column_compression_async_rpl.test

https://jira.percona.com/browse/PS-7806 Analysis compress_heap memory was released inside method row_sel_store_mysql_rec , even though ,it was reused. Which caused incorrect bin logging for comressed column, leading to async replication to break. Fix Modified test case to use repl_diff.

mysql-test/suite/rpl/t/rpl_column_compression_async_rpl.test

https://jira.percona.com/browse/PS-7806 Analysis compress_heap memory was released inside method row_sel_store_mysql_rec , even though ,it was reused. Which caused incorrect bin logging for comressed column, leading to async replication to break. Fix Modified test case as per review comments.

venkatesh-prasad-v

Looks fine with the test. But please wait for final approval from @satya-bodapati

satya-bodapati

Please see the question on thread

nitendra-bhosle · 2021-09-23T10:40:00Z

@satya-bodapati : #4460 (comment)
This is outdated. I have removed both empty and free heap statements.

satya-bodapati · 2021-09-23T10:43:53Z

@satya-bodapati : #4460 (comment)
This is outdated. I have removed both empty and free heap statements.

Well, I still the empty function.

If you remove this empty, Do we call empty at the end of statement execution?
Can you explain how the 'reuse' is happening?

nitendra-bhosle · 2021-09-23T11:12:16Z

@satya-bodapati : #4460 (comment)
This is outdated. I have removed both empty and free heap statements.

Well, I still the empty function.

If you remove this empty, Do we call empty at the end of statement execution?

This empty statement , I have added just to be consistent with blob_heap .
If we remove this, we will not call empty at the end of statement execution, as it's already being
getting free's inside : row_prebuilt_free

Can you explain how the 'reuse' is happening?

We are allocating memory column_get_compress_header and row_compress_column
using statements like :
if (!prebuilt->compress_heap)
prebuilt->compress_heap = mem_heap_create(ut_max(UNIV_PAGE_SIZE, buf_len));

I reffered reuse , because this is diverging from blob_heap memory management in a sense that memory is getting
allowcated inside these two methods , and reuse might be happening , otherwise , no need to allocated and check whether it's already allocated inside other method.

bratao · 2021-09-24T19:02:44Z

Any chance of this making into 8.0.26 release? I just got bitten by this

nitendra-bhosle · 2021-09-27T11:29:38Z

Any chance of this making into 8.0.26 release? I just got bitten by this

I think this Issue will be part of 8.0.27 release

https://jira.percona.com/browse/PS-7806 Analysis compress_heap memory was released inside method row_sel_store_mysql_rec , even though ,it was reused. Which caused incorrect bin logging for comressed column, leading to async replication to break. Fix Reverted Changes , so that Can implement different memory allocation scheme

https://jira.percona.com/browse/PS-7806 Analysis compress_heap memory was released inside method row_sel_store_mysql_rec , even though ,it was reused. Which caused incorrect bin logging for comressed column, leading to async replication to break. Fix Implemented individual partition compress_heap, inline with implementation of m_blob_heap_parts. Earlier we had common compress heap across partitions.

storage/innobase/row/row0sel.cc

satya-bodapati

Also please test PS-7657 by reverting the fix and using this fix

https://jira.percona.com/browse/PS-7806 Analysis compress_heap memory was released inside method row_sel_store_mysql_rec , even though ,it was reused. Which caused incorrect bin logging for comressed column, leading to async replication to break. Fix Implemented individual partition compress_heap, inline with implementation of m_blob_heap_parts. Earlier we had common compress heap across partitions.

nitendra-bhosle · 2021-10-06T15:01:52Z

Also please test PS-7657 by reverting the fix and using this fix

Tested , All tests are passing

satya-bodapati · 2021-10-06T15:04:11Z

Also please test PS-7657 by reverting the fix and using this fix

Tested , All tests are passing

Thats great news! Please include the revert of PS-7657 code change in this PR

nitendra-bhosle · 2021-10-06T15:09:58Z

Also please test PS-7657 by reverting the fix and using this fix

Tested , All tests are passing

Thats great news! Please include the revert of PS-7657 code change in this PR

Revert is conflicting with this change :
if (UNIV_LIKELY_NULL(prebuilt->compress_heap))
row_mysql_prebuilt_free_compress_heap(prebuilt);

Earlier we had
if (UNIV_LIKELY_NULL(prebuilt->compress_heap))
mem_heap_empty(prebuilt->compress_heap);

satya-bodapati · 2021-10-07T12:40:07Z

Also please test PS-7657 by reverting the fix and using this fix

Tested , All tests are passing

Thats great news! Please include the revert of PS-7657 code change in this PR

Revert is conflicting with this change : if (UNIV_LIKELY_NULL(prebuilt->compress_heap)) row_mysql_prebuilt_free_compress_heap(prebuilt);

Earlier we had if (UNIV_LIKELY_NULL(prebuilt->compress_heap)) mem_heap_empty(prebuilt->compress_heap);

You can add a new commit which will bring to original state.

nitendra-bhosle · 2021-10-07T13:05:56Z

Also this change of PS-7657 is still required -
if (UNIV_LIKELY_NULL(prebuilt->compress_heap))
mem_heap_empty(prebuilt->compress_heap);

Inside function row_insert_for_mysql_using_ins_graph

satya-bodapati · 2021-10-08T13:48:04Z

Also this change of PS-7657 is still required - if (UNIV_LIKELY_NULL(prebuilt->compress_heap)) mem_heap_empty(prebuilt->compress_heap);

Inside function row_insert_for_mysql_using_ins_graph

Yes @nitendra-bhosle, please bring that emptying back. with the new logic, emptying or freeing compress_heap shouldn't matter.

https://jira.percona.com/browse/PS-7806 Analysis compress_heap memory was released inside method row_sel_store_mysql_rec , even though ,it was reused. Which caused incorrect bin logging for comressed column, leading to async replication to break. Fix in this commit Reverted changes made for PS-7657

satya-bodapati · 2021-10-11T06:51:28Z

Also this change of PS-7657 is still required - if (UNIV_LIKELY_NULL(prebuilt->compress_heap)) mem_heap_empty(prebuilt->compress_heap);
Inside function row_insert_for_mysql_using_ins_graph

Yes @nitendra-bhosle, please bring that emptying back. with the new logic, emptying or freeing compress_heap shouldn't matter.

@nitendra-bhosle I still don't see the emptying code in row_insert_for_mysql_using_ins_graph()

  if (UNIV_LIKELY_NULL(prebuilt->compress_heap))
    mem_heap_empty(prebuilt->compress_heap);

This was present before PS-7657 and it should be brought back. It will not have any effect with your latest fix.

satya-bodapati

Please check my latest comment

nitendra-bhosle · 2021-10-12T13:31:30Z

Also this change of PS-7657 is still required - if (UNIV_LIKELY_NULL(prebuilt->compress_heap)) mem_heap_empty(prebuilt->compress_heap);
Inside function row_insert_for_mysql_using_ins_graph

Yes @nitendra-bhosle, please bring that emptying back. with the new logic, emptying or freeing compress_heap shouldn't matter.

Also this change of PS-7657 is still required - if (UNIV_LIKELY_NULL(prebuilt->compress_heap)) mem_heap_empty(prebuilt->compress_heap);
Inside function row_insert_for_mysql_using_ins_graph

Yes @nitendra-bhosle, please bring that emptying back. with the new logic, emptying or freeing compress_heap shouldn't matter.

@nitendra-bhosle I still don't see the emptying code in row_insert_for_mysql_using_ins_graph()
  if (UNIV_LIKELY_NULL(prebuilt->compress_heap))
    mem_heap_empty(prebuilt->compress_heap);
This was present before PS-7657 and it should be brought back. It will not have any effect with your latest fix.

After my investigation today, I found that above change is required to fix - PS-7657.It will not have any effect on my fix for PS-7806.
But this will break for PS-7657 , hence I kept it as is.

With current patch , which is part of these commits , all tests for PS--7657 and PS-7806 are passing

https://jira.percona.com/browse/PS-7806 Analysis compress_heap memory was released inside method row_sel_store_mysql_rec , even though ,it was reused. Which caused incorrect bin logging for comressed column, leading to async replication to break. Fix in this commit Reverted changes made for PS-7657

satya-bodapati · 2021-11-11T11:51:01Z

storage/innobase/row/row0mysql.cc

+  if (UNIV_LIKELY_NULL(prebuilt->compress_heap))
+    mem_heap_empty(prebuilt->compress_heap);
+


previously before 7657, the empty was before trx->op_info = "inserting" line. Any reason, it is placed here? Does it have to be done after conversion of row to innodb format?

Can it be moved to before trx->op_info?

I see that you replied below.
So this means, we cannot free a buffer from the previous handler call and it is still in use?

Ideally, we should be able to free before the mysql_to_innodb row format conversion. The buffer would have last row insertion/handler call. So technically we should be allowed to free it.

I think I asked this question (may be on slack), can you please add empty or freeing of normal heap before trx->op_info/row_mysql_convert_row_to_innobase()? Will it have the same problem if we free earlier?

nitendra-bhosle · 2021-11-15T13:51:07Z

previously before 7657, the empty was before trx->op_info = "inserting" line. Any reason, it is placed here? Does it have to be done after conversion of row to innodb format?

Can it be moved to before trx->op_info?

We can't move it before trx->op_info , as row_mysql_convert_row_to_innobase call uses prebuilt->compress_heap

satya-bodapati

Please check my last comment

kamil-holubicki · 2023-11-17T10:21:23Z

Will be handled by
#5161
#5162

nitendra-bhosle requested review from satya-bodapati and venkatesh-prasad-v September 10, 2021 14:30

satya-bodapati requested changes Sep 13, 2021

View reviewed changes

storage/innobase/row/row0sel.cc Show resolved Hide resolved

nitendra-bhosle requested a review from satya-bodapati September 15, 2021 12:51

venkatesh-prasad-v requested changes Sep 15, 2021

View reviewed changes

satya-bodapati approved these changes Sep 16, 2021

View reviewed changes

nitendra-bhosle requested review from satya-bodapati and venkatesh-prasad-v September 17, 2021 16:51

venkatesh-prasad-v requested changes Sep 20, 2021

View reviewed changes

nitendra-bhosle requested a review from venkatesh-prasad-v September 21, 2021 17:13

venkatesh-prasad-v requested changes Sep 22, 2021

View reviewed changes

mysql-test/suite/rpl/t/rpl_column_compression_async_rpl.test Show resolved Hide resolved

mysql-test/suite/rpl/t/rpl_column_compression_async_rpl.test Outdated Show resolved Hide resolved

nitendra-bhosle requested a review from venkatesh-prasad-v September 22, 2021 11:18

venkatesh-prasad-v approved these changes Sep 22, 2021

View reviewed changes

satya-bodapati requested changes Sep 23, 2021

View reviewed changes

Nitendra Bhosle added 2 commits September 30, 2021 19:01

nitendra-bhosle requested review from satya-bodapati and venkatesh-prasad-v October 1, 2021 14:10

satya-bodapati requested changes Oct 5, 2021

View reviewed changes

storage/innobase/row/row0sel.cc Show resolved Hide resolved

satya-bodapati requested changes Oct 5, 2021

View reviewed changes

nitendra-bhosle requested a review from satya-bodapati October 6, 2021 15:02

satya-bodapati requested changes Oct 11, 2021

View reviewed changes

nitendra-bhosle requested a review from satya-bodapati October 28, 2021 14:15

satya-bodapati requested changes Nov 11, 2021

View reviewed changes

nitendra-bhosle requested a review from satya-bodapati November 15, 2021 13:51

satya-bodapati requested changes Nov 16, 2021

View reviewed changes

kamil-holubicki closed this Nov 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PS-7806: Column compression breaks async replication on PS #4460

PS-7806: Column compression breaks async replication on PS #4460

nitendra-bhosle commented Sep 10, 2021

nitendra-bhosle commented Sep 10, 2021 •

edited

Loading

venkatesh-prasad-v Sep 15, 2021

venkatesh-prasad-v Sep 15, 2021

satya-bodapati Sep 16, 2021

satya-bodapati Sep 16, 2021

satya-bodapati Sep 23, 2021 •

edited by nitendra-bhosle

Loading

venkatesh-prasad-v left a comment •

edited

Loading

satya-bodapati left a comment

nitendra-bhosle commented Sep 23, 2021

satya-bodapati commented Sep 23, 2021

nitendra-bhosle commented Sep 23, 2021

bratao commented Sep 24, 2021

nitendra-bhosle commented Sep 27, 2021

satya-bodapati left a comment

nitendra-bhosle commented Oct 6, 2021

satya-bodapati commented Oct 6, 2021

nitendra-bhosle commented Oct 6, 2021

satya-bodapati commented Oct 7, 2021

nitendra-bhosle commented Oct 7, 2021

satya-bodapati commented Oct 8, 2021 •

edited

Loading

satya-bodapati commented Oct 11, 2021

satya-bodapati left a comment

nitendra-bhosle commented Oct 12, 2021

satya-bodapati Nov 11, 2021

satya-bodapati Nov 16, 2021

nitendra-bhosle commented Nov 15, 2021

satya-bodapati left a comment

kamil-holubicki commented Nov 17, 2023 •

edited

Loading

		if (UNIV_LIKELY_NULL(prebuilt->compress_heap))
		mem_heap_empty(prebuilt->compress_heap);

PS-7806: Column compression breaks async replication on PS #4460

PS-7806: Column compression breaks async replication on PS #4460

Conversation

nitendra-bhosle commented Sep 10, 2021

nitendra-bhosle commented Sep 10, 2021 • edited Loading

venkatesh-prasad-v Sep 15, 2021

Choose a reason for hiding this comment

venkatesh-prasad-v Sep 15, 2021

Choose a reason for hiding this comment

satya-bodapati Sep 16, 2021

Choose a reason for hiding this comment

satya-bodapati Sep 16, 2021

Choose a reason for hiding this comment

satya-bodapati Sep 23, 2021 • edited by nitendra-bhosle Loading

Choose a reason for hiding this comment

venkatesh-prasad-v left a comment • edited Loading

Choose a reason for hiding this comment

satya-bodapati left a comment

Choose a reason for hiding this comment

nitendra-bhosle commented Sep 23, 2021

satya-bodapati commented Sep 23, 2021

nitendra-bhosle commented Sep 23, 2021

bratao commented Sep 24, 2021

nitendra-bhosle commented Sep 27, 2021

satya-bodapati left a comment

Choose a reason for hiding this comment

nitendra-bhosle commented Oct 6, 2021

satya-bodapati commented Oct 6, 2021

nitendra-bhosle commented Oct 6, 2021

satya-bodapati commented Oct 7, 2021

nitendra-bhosle commented Oct 7, 2021

satya-bodapati commented Oct 8, 2021 • edited Loading

satya-bodapati commented Oct 11, 2021

satya-bodapati left a comment

Choose a reason for hiding this comment

nitendra-bhosle commented Oct 12, 2021

satya-bodapati Nov 11, 2021

Choose a reason for hiding this comment

satya-bodapati Nov 16, 2021

Choose a reason for hiding this comment

nitendra-bhosle commented Nov 15, 2021

satya-bodapati left a comment

Choose a reason for hiding this comment

kamil-holubicki commented Nov 17, 2023 • edited Loading

nitendra-bhosle commented Sep 10, 2021 •

edited

Loading

satya-bodapati Sep 23, 2021 •

edited by nitendra-bhosle

Loading

venkatesh-prasad-v left a comment •

edited

Loading

satya-bodapati commented Oct 8, 2021 •

edited

Loading

kamil-holubicki commented Nov 17, 2023 •

edited

Loading