Fix memory counter update in reorderbuffer
Hi,
I found a bug in the memory counter update in reorderbuffer. It was
introduced by commit 5bec1d6bc5e, so pg17 and master are affected.
In ReorderBufferCleanupTXN() we zero the transaction size and then
free the transaction entry as follows:
/* Update the memory counter */
ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);
/* deallocate */
ReorderBufferReturnTXN(rb, txn);
However, if the transaction entry has toast changes we free them in
ReorderBufferToastReset() called from ReorderBufferToastReset(), and
end up subtracting the memory usage from zero. Which results in an
assertion failure:
TRAP: failed Assert("(rb->size >= sz) && (txn->size >= sz)"), File:
"reorderbuffer.c"
This can happen for example if an error occurs while replaying
transaction changes including toast changes.
I've attached the patch that fixes the problem and includes a test
case (the test part might not be committed as it slows the test case).
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
fix_memory_counter_update_in_reorderbuffer.patchapplication/octet-stream; name=fix_memory_counter_update_in_reorderbuffer.patchDownload
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 00a8327e77..f229ff700a 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -467,6 +467,13 @@ ReorderBufferReturnTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
/* Reset the toast hash */
ReorderBufferToastReset(rb, txn);
+ /*
+ * Update the memory counter of the transaction, removing it from
+ * the max-heap.
+ */
+ ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);
+ Assert(txn->size == 0);
+
pfree(txn);
}
@@ -1594,9 +1601,6 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
if (rbtxn_is_serialized(txn))
ReorderBufferRestoreCleanup(rb, txn);
- /* Update the memory counter */
- ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);
-
/* deallocate */
ReorderBufferReturnTXN(rb, txn);
}
diff --git a/src/test/subscription/t/031_column_list.pl b/src/test/subscription/t/031_column_list.pl
index 9a97fa5020..20454ae45d 100644
--- a/src/test/subscription/t/031_column_list.pl
+++ b/src/test/subscription/t/031_column_list.pl
@@ -1255,7 +1255,7 @@ is( $node_subscriber->safe_psql(
$node_publisher->safe_psql(
'postgres', qq(
- CREATE TABLE test_mix_1 (a int PRIMARY KEY, b int, c int);
+ CREATE TABLE test_mix_1 (a int PRIMARY KEY, b int, c text);
CREATE PUBLICATION pub_mix_1 FOR TABLE test_mix_1 (a, b);
CREATE PUBLICATION pub_mix_2 FOR TABLE test_mix_1 (a, c);
));
@@ -1263,7 +1263,7 @@ $node_publisher->safe_psql(
$node_subscriber->safe_psql(
'postgres', qq(
DROP SUBSCRIPTION sub1;
- CREATE TABLE test_mix_1 (a int PRIMARY KEY, b int, c int);
+ CREATE TABLE test_mix_1 (a int PRIMARY KEY, b int, c text);
));
my ($cmdret, $stdout, $stderr) = $node_subscriber->psql(
@@ -1290,10 +1290,14 @@ $node_subscriber->safe_psql(
$node_publisher->wait_for_catchup('sub1');
+# XXX: walsender raises an error during the replay of the INSERT transaction. Then during cleanup, it frees
+# the transaction entry while having the TOAST changes in memory. Previously, since we zeroed the transaction
+# size before freeing TOAST changes, we ended up further subtracting the memory counter from zero, resulting
+# in an assertion failure (with assertion build) or SIGV (wihtout assertion build).
$node_publisher->safe_psql(
'postgres', qq(
ALTER PUBLICATION pub_mix_1 SET TABLE test_mix_1 (a, b);
- INSERT INTO test_mix_1 VALUES(1, 1, 1);
+ INSERT INTO test_mix_1 (a, c) SELECT 1, repeat(string_agg(to_char(g.i, 'FM0000'), ''), 50) FROM generate_series(1, 500) g(i);
));
$offset = $node_publisher->wait_for_log(
On Sat, Aug 3, 2024 at 1:21 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I found a bug in the memory counter update in reorderbuffer. It was
introduced by commit 5bec1d6bc5e, so pg17 and master are affected.In ReorderBufferCleanupTXN() we zero the transaction size and then
free the transaction entry as follows:/* Update the memory counter */
ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);/* deallocate */
ReorderBufferReturnTXN(rb, txn);
Why do we need to zero the transaction size explicitly? Shouldn't it
automatically become zero after freeing all the changes?
However, if the transaction entry has toast changes we free them in
ReorderBufferToastReset() called from ReorderBufferToastReset(),
Here, you mean ReorderBufferToastReset() called from
ReorderBufferReturnTXN(), right?
BTW, commit 5bec1d6bc5e also introduced
ReorderBufferChangeMemoryUpdate() in ReorderBufferTruncateTXN() which
is also worth considering while fixing the reported problem. It may
not have the same problem as you have reported but we can consider
whether setting txn size as zero explicitly is required or not.
--
With Regards,
Amit Kapila.
On Tue, Aug 6, 2024 at 1:12 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sat, Aug 3, 2024 at 1:21 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I found a bug in the memory counter update in reorderbuffer. It was
introduced by commit 5bec1d6bc5e, so pg17 and master are affected.In ReorderBufferCleanupTXN() we zero the transaction size and then
free the transaction entry as follows:/* Update the memory counter */
ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);/* deallocate */
ReorderBufferReturnTXN(rb, txn);Why do we need to zero the transaction size explicitly? Shouldn't it
automatically become zero after freeing all the changes?
It will become zero after freeing all the changes. However, since
updating the max-heap when freeing each change could bring some
overhead, we freed the changes without updating the memory counter,
and then zerod it.
However, if the transaction entry has toast changes we free them in
ReorderBufferToastReset() called from ReorderBufferToastReset(),Here, you mean ReorderBufferToastReset() called from
ReorderBufferReturnTXN(), right?
Right. Thank you for pointing it out.
BTW, commit 5bec1d6bc5e also introduced
ReorderBufferChangeMemoryUpdate() in ReorderBufferTruncateTXN() which
is also worth considering while fixing the reported problem. It may
not have the same problem as you have reported but we can consider
whether setting txn size as zero explicitly is required or not.
The reason why it introduced ReorderBufferChangeMemoryUpdate() is the
same as I mentioned above. And yes, as you mentioned, it doesn't have
the same problem that I reported here.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On Wed, Aug 7, 2024 at 7:42 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Tue, Aug 6, 2024 at 1:12 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sat, Aug 3, 2024 at 1:21 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I found a bug in the memory counter update in reorderbuffer. It was
introduced by commit 5bec1d6bc5e, so pg17 and master are affected.In ReorderBufferCleanupTXN() we zero the transaction size and then
free the transaction entry as follows:/* Update the memory counter */
ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);/* deallocate */
ReorderBufferReturnTXN(rb, txn);Why do we need to zero the transaction size explicitly? Shouldn't it
automatically become zero after freeing all the changes?It will become zero after freeing all the changes. However, since
updating the max-heap when freeing each change could bring some
overhead, we freed the changes without updating the memory counter,
and then zerod it.
I think this should be covered in comments as it is not apparent.
BTW, commit 5bec1d6bc5e also introduced
ReorderBufferChangeMemoryUpdate() in ReorderBufferTruncateTXN() which
is also worth considering while fixing the reported problem. It may
not have the same problem as you have reported but we can consider
whether setting txn size as zero explicitly is required or not.The reason why it introduced ReorderBufferChangeMemoryUpdate() is the
same as I mentioned above. And yes, as you mentioned, it doesn't have
the same problem that I reported here.
I checked again and found that ReorderBufferResetTXN() first calls
ReorderBufferTruncateTXN() and then ReorderBufferToastReset(). After
that, it also tries to free spec_insert change which should have the
same problem. So, what saves this path from the same problem?
*
+ /*
+ * Update the memory counter of the transaction, removing it from
+ * the max-heap.
+ */
+ ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);
+ Assert(txn->size == 0);
+
pfree(txn);Just before freeing the TXN, updating the size looks odd though I
understand the idea is to remove TXN from max_heap. Anyway, let's
first discuss whether the same problem exists in
ReorderBufferResetTXN() code path, and if so, how we want to fix it.
--
With Regards,
Amit Kapila.
On Wed, Aug 7, 2024 at 3:17 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Wed, Aug 7, 2024 at 7:42 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Tue, Aug 6, 2024 at 1:12 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sat, Aug 3, 2024 at 1:21 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I found a bug in the memory counter update in reorderbuffer. It was
introduced by commit 5bec1d6bc5e, so pg17 and master are affected.In ReorderBufferCleanupTXN() we zero the transaction size and then
free the transaction entry as follows:/* Update the memory counter */
ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);/* deallocate */
ReorderBufferReturnTXN(rb, txn);Why do we need to zero the transaction size explicitly? Shouldn't it
automatically become zero after freeing all the changes?It will become zero after freeing all the changes. However, since
updating the max-heap when freeing each change could bring some
overhead, we freed the changes without updating the memory counter,
and then zerod it.I think this should be covered in comments as it is not apparent.
Agreed.
BTW, commit 5bec1d6bc5e also introduced
ReorderBufferChangeMemoryUpdate() in ReorderBufferTruncateTXN() which
is also worth considering while fixing the reported problem. It may
not have the same problem as you have reported but we can consider
whether setting txn size as zero explicitly is required or not.The reason why it introduced ReorderBufferChangeMemoryUpdate() is the
same as I mentioned above. And yes, as you mentioned, it doesn't have
the same problem that I reported here.I checked again and found that ReorderBufferResetTXN() first calls
ReorderBufferTruncateTXN() and then ReorderBufferToastReset(). After
that, it also tries to free spec_insert change which should have the
same problem. So, what saves this path from the same problem?
Good catch. I've not created a test case for that but it seems to be
possible to happen.
I think that subtracting txn->size to reduce the memory counter to
zero seems to be a wrong idea in the first place. If we want to save
updating memory counter and max-heap, we should use the exact memory
size that we freed. In other words, just change the memory usage
update to a batch operation.
* + /* + * Update the memory counter of the transaction, removing it from + * the max-heap. + */ + ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size); + Assert(txn->size == 0); + pfree(txn);Just before freeing the TXN, updating the size looks odd though I
understand the idea is to remove TXN from max_heap. Anyway, let's
first discuss whether the same problem exists in
ReorderBufferResetTXN() code path, and if so, how we want to fix it.
Agreed.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On Thu, Aug 8, 2024 at 9:43 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Wed, Aug 7, 2024 at 3:17 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Wed, Aug 7, 2024 at 7:42 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
BTW, commit 5bec1d6bc5e also introduced
ReorderBufferChangeMemoryUpdate() in ReorderBufferTruncateTXN() which
is also worth considering while fixing the reported problem. It may
not have the same problem as you have reported but we can consider
whether setting txn size as zero explicitly is required or not.The reason why it introduced ReorderBufferChangeMemoryUpdate() is the
same as I mentioned above. And yes, as you mentioned, it doesn't have
the same problem that I reported here.I checked again and found that ReorderBufferResetTXN() first calls
ReorderBufferTruncateTXN() and then ReorderBufferToastReset(). After
that, it also tries to free spec_insert change which should have the
same problem. So, what saves this path from the same problem?Good catch. I've not created a test case for that but it seems to be
possible to happen.I think that subtracting txn->size to reduce the memory counter to
zero seems to be a wrong idea in the first place. If we want to save
updating memory counter and max-heap, we should use the exact memory
size that we freed. In other words, just change the memory usage
update to a batch operation.
Sounds reasonable but how would you find the size for a batch update
operation? Are you planning to track it while freeing the individual
changes?
--
With Regards,
Amit Kapila.
On Fri, Aug 9, 2024 at 3:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Aug 8, 2024 at 9:43 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Wed, Aug 7, 2024 at 3:17 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Wed, Aug 7, 2024 at 7:42 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
BTW, commit 5bec1d6bc5e also introduced
ReorderBufferChangeMemoryUpdate() in ReorderBufferTruncateTXN() which
is also worth considering while fixing the reported problem. It may
not have the same problem as you have reported but we can consider
whether setting txn size as zero explicitly is required or not.The reason why it introduced ReorderBufferChangeMemoryUpdate() is the
same as I mentioned above. And yes, as you mentioned, it doesn't have
the same problem that I reported here.I checked again and found that ReorderBufferResetTXN() first calls
ReorderBufferTruncateTXN() and then ReorderBufferToastReset(). After
that, it also tries to free spec_insert change which should have the
same problem. So, what saves this path from the same problem?Good catch. I've not created a test case for that but it seems to be
possible to happen.I think that subtracting txn->size to reduce the memory counter to
zero seems to be a wrong idea in the first place. If we want to save
updating memory counter and max-heap, we should use the exact memory
size that we freed. In other words, just change the memory usage
update to a batch operation.Sounds reasonable but how would you find the size for a batch update
operation? Are you planning to track it while freeing the individual
changes?
Yes, one idea is to make ReorderBufferReturnChange() return the memory
size that it just freed. Then the caller who wants to update the
memory counter in a batch sums up the memory size.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On Sat, Aug 10, 2024 at 5:48 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Fri, Aug 9, 2024 at 3:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Aug 8, 2024 at 9:43 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Wed, Aug 7, 2024 at 3:17 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Wed, Aug 7, 2024 at 7:42 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
BTW, commit 5bec1d6bc5e also introduced
ReorderBufferChangeMemoryUpdate() in ReorderBufferTruncateTXN() which
is also worth considering while fixing the reported problem. It may
not have the same problem as you have reported but we can consider
whether setting txn size as zero explicitly is required or not.The reason why it introduced ReorderBufferChangeMemoryUpdate() is the
same as I mentioned above. And yes, as you mentioned, it doesn't have
the same problem that I reported here.I checked again and found that ReorderBufferResetTXN() first calls
ReorderBufferTruncateTXN() and then ReorderBufferToastReset(). After
that, it also tries to free spec_insert change which should have the
same problem. So, what saves this path from the same problem?Good catch. I've not created a test case for that but it seems to be
possible to happen.I think that subtracting txn->size to reduce the memory counter to
zero seems to be a wrong idea in the first place. If we want to save
updating memory counter and max-heap, we should use the exact memory
size that we freed. In other words, just change the memory usage
update to a batch operation.Sounds reasonable but how would you find the size for a batch update
operation? Are you planning to track it while freeing the individual
changes?Yes, one idea is to make ReorderBufferReturnChange() return the memory
size that it just freed. Then the caller who wants to update the
memory counter in a batch sums up the memory size.
I've drafted the patch. I didn't change ReorderBufferReturnChange()
but called ReorderBufferChangeSize() for individual change instead, as
it's simpler.gi
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
fix_memory_counter_update_in_reorderbuffer_v2.patchapplication/octet-stream; name=fix_memory_counter_update_in_reorderbuffer_v2.patchDownload
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 00a8327e77..bf5cf416bf 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -467,6 +467,9 @@ ReorderBufferReturnTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
/* Reset the toast hash */
ReorderBufferToastReset(rb, txn);
+ /* All changes must be returned */
+ Assert(txn->size == 0);
+
pfree(txn);
}
@@ -1506,6 +1509,7 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
{
bool found;
dlist_mutable_iter iter;
+ Size mem_freed = 0;
/* cleanup subtransactions & their changes */
dlist_foreach_modify(iter, &txn->subtxns)
@@ -1535,9 +1539,20 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
/* Check we're not mixing changes from different transactions. */
Assert(change->txn == txn);
+ /*
+ * Instead of updating the memory counter for individual changes,
+ * we sum up the size of memory to free so we can update the memory
+ * counter all together below. This saves costs of maintaining
+ * the max-heap.
+ */
+ mem_freed += ReorderBufferChangeSize(change);
+
ReorderBufferReturnChange(rb, change, false);
}
+ /* Update the memory counter */
+ ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, mem_freed);
+
/*
* Cleanup the tuplecids we stored for decoding catalog snapshot access.
* They are always stored in the toplevel transaction.
@@ -1594,9 +1609,6 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
if (rbtxn_is_serialized(txn))
ReorderBufferRestoreCleanup(rb, txn);
- /* Update the memory counter */
- ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);
-
/* deallocate */
ReorderBufferReturnTXN(rb, txn);
}
@@ -1616,6 +1628,7 @@ static void
ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, bool txn_prepared)
{
dlist_mutable_iter iter;
+ Size mem_freed = 0;
/* cleanup subtransactions & their changes */
dlist_foreach_modify(iter, &txn->subtxns)
@@ -1648,11 +1661,19 @@ ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, bool txn_prep
/* remove the change from it's containing list */
dlist_delete(&change->node);
+ /*
+ * Instead of updating the memory counter for individual changes,
+ * we sum up the size of memory to free so we can update the memory
+ * counter all together below. This saves costs of maintaining
+ * the max-heap.
+ */
+ mem_freed += ReorderBufferChangeSize(change);
+
ReorderBufferReturnChange(rb, change, false);
}
/* Update the memory counter */
- ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);
+ ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, mem_freed);
/*
* Mark the transaction as streamed.
@@ -2062,6 +2083,9 @@ ReorderBufferResetTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
rb->stream_stop(rb, txn, last_lsn);
ReorderBufferSaveTXNSnapshot(rb, txn, snapshot_now, command_id);
}
+
+ /* All changes must be returned */
+ Assert(txn->size == 0);
}
/*
diff --git a/src/test/subscription/t/031_column_list.pl b/src/test/subscription/t/031_column_list.pl
index 9a97fa5020..4106c29350 100644
--- a/src/test/subscription/t/031_column_list.pl
+++ b/src/test/subscription/t/031_column_list.pl
@@ -1255,7 +1255,7 @@ is( $node_subscriber->safe_psql(
$node_publisher->safe_psql(
'postgres', qq(
- CREATE TABLE test_mix_1 (a int PRIMARY KEY, b int, c int);
+ CREATE TABLE test_mix_1 (a int PRIMARY KEY, b int, c text);
CREATE PUBLICATION pub_mix_1 FOR TABLE test_mix_1 (a, b);
CREATE PUBLICATION pub_mix_2 FOR TABLE test_mix_1 (a, c);
));
@@ -1263,7 +1263,7 @@ $node_publisher->safe_psql(
$node_subscriber->safe_psql(
'postgres', qq(
DROP SUBSCRIPTION sub1;
- CREATE TABLE test_mix_1 (a int PRIMARY KEY, b int, c int);
+ CREATE TABLE test_mix_1 (a int PRIMARY KEY, b int, c text);
));
my ($cmdret, $stdout, $stderr) = $node_subscriber->psql(
@@ -1290,10 +1290,14 @@ $node_subscriber->safe_psql(
$node_publisher->wait_for_catchup('sub1');
+# XXX: walsender raises an error during the replay of the INSERT transaction. Then during cleanup, it frees
+# the transaction entry while having the TOAST changes in memory. Previously, since we zeroed the transaction
+# size before freeing TOAST changes, we ended up further subtracting the memory counter from zero, resulting
+# in an assertion failure (with assertion build) or SEGV (wihtout assertion build).
$node_publisher->safe_psql(
'postgres', qq(
ALTER PUBLICATION pub_mix_1 SET TABLE test_mix_1 (a, b);
- INSERT INTO test_mix_1 VALUES(1, 1, 1);
+ INSERT INTO test_mix_1 (a, c) SELECT 1, repeat(string_agg(to_char(g.i, 'FM0000'), ''), 50) FROM generate_series(1, 500) g(i);
));
$offset = $node_publisher->wait_for_log(
On Wed, 7 Aug 2024 at 11:48, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Wed, Aug 7, 2024 at 7:42 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Tue, Aug 6, 2024 at 1:12 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sat, Aug 3, 2024 at 1:21 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I found a bug in the memory counter update in reorderbuffer. It was
introduced by commit 5bec1d6bc5e, so pg17 and master are affected.In ReorderBufferCleanupTXN() we zero the transaction size and then
free the transaction entry as follows:/* Update the memory counter */
ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);/* deallocate */
ReorderBufferReturnTXN(rb, txn);Why do we need to zero the transaction size explicitly? Shouldn't it
automatically become zero after freeing all the changes?It will become zero after freeing all the changes. However, since
updating the max-heap when freeing each change could bring some
overhead, we freed the changes without updating the memory counter,
and then zerod it.I think this should be covered in comments as it is not apparent.
BTW, commit 5bec1d6bc5e also introduced
ReorderBufferChangeMemoryUpdate() in ReorderBufferTruncateTXN() which
is also worth considering while fixing the reported problem. It may
not have the same problem as you have reported but we can consider
whether setting txn size as zero explicitly is required or not.The reason why it introduced ReorderBufferChangeMemoryUpdate() is the
same as I mentioned above. And yes, as you mentioned, it doesn't have
the same problem that I reported here.I checked again and found that ReorderBufferResetTXN() first calls
ReorderBufferTruncateTXN() and then ReorderBufferToastReset(). After
that, it also tries to free spec_insert change which should have the
same problem. So, what saves this path from the same problem?
I tried testing this scenario and I was able to reproduce the crash in
HEAD with this scenario. I have created a patch for the testcase.
I also tested the same scenario with the latest patch shared by
Sawada-san in [1]/messages/by-id/CAD21AoDHC4Sob=NEYTxgu5wd4rzCpSLY_hWapMUqf4WKrAxmyw@mail.gmail.com. And confirm that this resolves the issue.
[1]: /messages/by-id/CAD21AoDHC4Sob=NEYTxgu5wd4rzCpSLY_hWapMUqf4WKrAxmyw@mail.gmail.com
Thanks and Regards,
Shlok Kyal
Attachments:
fix_memory_counter_update_in_reorderbuffer_v2.patchapplication/octet-stream; name=fix_memory_counter_update_in_reorderbuffer_v2.patchDownload
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 00a8327e77..bf5cf416bf 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -467,6 +467,9 @@ ReorderBufferReturnTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
/* Reset the toast hash */
ReorderBufferToastReset(rb, txn);
+ /* All changes must be returned */
+ Assert(txn->size == 0);
+
pfree(txn);
}
@@ -1506,6 +1509,7 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
{
bool found;
dlist_mutable_iter iter;
+ Size mem_freed = 0;
/* cleanup subtransactions & their changes */
dlist_foreach_modify(iter, &txn->subtxns)
@@ -1535,9 +1539,20 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
/* Check we're not mixing changes from different transactions. */
Assert(change->txn == txn);
+ /*
+ * Instead of updating the memory counter for individual changes,
+ * we sum up the size of memory to free so we can update the memory
+ * counter all together below. This saves costs of maintaining
+ * the max-heap.
+ */
+ mem_freed += ReorderBufferChangeSize(change);
+
ReorderBufferReturnChange(rb, change, false);
}
+ /* Update the memory counter */
+ ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, mem_freed);
+
/*
* Cleanup the tuplecids we stored for decoding catalog snapshot access.
* They are always stored in the toplevel transaction.
@@ -1594,9 +1609,6 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
if (rbtxn_is_serialized(txn))
ReorderBufferRestoreCleanup(rb, txn);
- /* Update the memory counter */
- ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);
-
/* deallocate */
ReorderBufferReturnTXN(rb, txn);
}
@@ -1616,6 +1628,7 @@ static void
ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, bool txn_prepared)
{
dlist_mutable_iter iter;
+ Size mem_freed = 0;
/* cleanup subtransactions & their changes */
dlist_foreach_modify(iter, &txn->subtxns)
@@ -1648,11 +1661,19 @@ ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, bool txn_prep
/* remove the change from it's containing list */
dlist_delete(&change->node);
+ /*
+ * Instead of updating the memory counter for individual changes,
+ * we sum up the size of memory to free so we can update the memory
+ * counter all together below. This saves costs of maintaining
+ * the max-heap.
+ */
+ mem_freed += ReorderBufferChangeSize(change);
+
ReorderBufferReturnChange(rb, change, false);
}
/* Update the memory counter */
- ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);
+ ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, mem_freed);
/*
* Mark the transaction as streamed.
@@ -2062,6 +2083,9 @@ ReorderBufferResetTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
rb->stream_stop(rb, txn, last_lsn);
ReorderBufferSaveTXNSnapshot(rb, txn, snapshot_now, command_id);
}
+
+ /* All changes must be returned */
+ Assert(txn->size == 0);
}
/*
diff --git a/src/test/subscription/t/031_column_list.pl b/src/test/subscription/t/031_column_list.pl
index 9a97fa5020..4106c29350 100644
--- a/src/test/subscription/t/031_column_list.pl
+++ b/src/test/subscription/t/031_column_list.pl
@@ -1255,7 +1255,7 @@ is( $node_subscriber->safe_psql(
$node_publisher->safe_psql(
'postgres', qq(
- CREATE TABLE test_mix_1 (a int PRIMARY KEY, b int, c int);
+ CREATE TABLE test_mix_1 (a int PRIMARY KEY, b int, c text);
CREATE PUBLICATION pub_mix_1 FOR TABLE test_mix_1 (a, b);
CREATE PUBLICATION pub_mix_2 FOR TABLE test_mix_1 (a, c);
));
@@ -1263,7 +1263,7 @@ $node_publisher->safe_psql(
$node_subscriber->safe_psql(
'postgres', qq(
DROP SUBSCRIPTION sub1;
- CREATE TABLE test_mix_1 (a int PRIMARY KEY, b int, c int);
+ CREATE TABLE test_mix_1 (a int PRIMARY KEY, b int, c text);
));
my ($cmdret, $stdout, $stderr) = $node_subscriber->psql(
@@ -1290,10 +1290,14 @@ $node_subscriber->safe_psql(
$node_publisher->wait_for_catchup('sub1');
+# XXX: walsender raises an error during the replay of the INSERT transaction. Then during cleanup, it frees
+# the transaction entry while having the TOAST changes in memory. Previously, since we zeroed the transaction
+# size before freeing TOAST changes, we ended up further subtracting the memory counter from zero, resulting
+# in an assertion failure (with assertion build) or SEGV (wihtout assertion build).
$node_publisher->safe_psql(
'postgres', qq(
ALTER PUBLICATION pub_mix_1 SET TABLE test_mix_1 (a, b);
- INSERT INTO test_mix_1 VALUES(1, 1, 1);
+ INSERT INTO test_mix_1 (a, c) SELECT 1, repeat(string_agg(to_char(g.i, 'FM0000'), ''), 50) FROM generate_series(1, 500) g(i);
));
$offset = $node_publisher->wait_for_log(
test.patchapplication/octet-stream; name=test.patchDownload
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index cb36ca7b16..b6853e881d 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -490,4 +490,45 @@ is( $result, qq(2|f
$node_publisher->stop('fast');
$node_subscriber->stop('fast');
+# https://www.postgresql.org/message-id/flat/CAD21AoAqkNUvicgKPT_dXzNoOwpPkVTg0QPPxEcWmzT0moCJ1g%40mail.gmail.com
+# This bug happens when we rollback a subtransaction which have TOAST changes and we set
+# 'debug_logical_replication_streaming = immediate'. During reset, it frees the subtransaction entry while having
+# the TOAST changes in memory. Previously, since we zeroed the subtransaction size before freeing TOAST changes, we
+# ended up further subtracting the memory counter from zero, resulting in an assertion failure.
+$node_publisher->rotate_logfile();
+$node_publisher->append_conf('postgresql.conf', 'logical_decoding_work_mem = 64kB');
+$node_publisher->append_conf('postgresql.conf', 'debug_logical_replication_streaming = immediate');
+$node_publisher->start();
+
+$node_subscriber->rotate_logfile();
+$node_subscriber->start();
+
+$node_publisher->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE test_tab (a text);
+ CREATE PUBLICATION pub_test_tab FOR TABLE test_tab;
+));
+
+$node_subscriber->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE test_tab (a text, b int);
+ CREATE SUBSCRIPTION sub_test_tab CONNECTION '$publisher_connstr' PUBLICATION pub_test_tab WITH (streaming = on);
+));
+
+$node_publisher->safe_psql(
+ 'postgres', qq(
+ BEGIN;
+ INSERT INTO test_tab (a) SELECT repeat(string_agg(to_char(g.i, 'FM0000'), ''), 50) FROM generate_series(1, 500) g(i);
+ ALTER TABLE test_tab ADD COLUMN b INT;
+ SAVEPOINT s1;
+ INSERT INTO test_tab (b, a) SELECT 1, repeat(string_agg(to_char(g.i, 'FM0000'), ''), 50) FROM generate_series(1, 500) g(i);
+ ROLLBACK TO s1;
+ COMMIT;
+));
+
+$node_publisher->wait_for_catchup('sub_test_tab');
+
+$node_publisher->stop('fast');
+$node_subscriber->stop('fast');
+
done_testing();
Hi,
On Fri, Aug 16, 2024 at 12:22 AM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
On Wed, 7 Aug 2024 at 11:48, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Wed, Aug 7, 2024 at 7:42 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Tue, Aug 6, 2024 at 1:12 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sat, Aug 3, 2024 at 1:21 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I found a bug in the memory counter update in reorderbuffer. It was
introduced by commit 5bec1d6bc5e, so pg17 and master are affected.In ReorderBufferCleanupTXN() we zero the transaction size and then
free the transaction entry as follows:/* Update the memory counter */
ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);/* deallocate */
ReorderBufferReturnTXN(rb, txn);Why do we need to zero the transaction size explicitly? Shouldn't it
automatically become zero after freeing all the changes?It will become zero after freeing all the changes. However, since
updating the max-heap when freeing each change could bring some
overhead, we freed the changes without updating the memory counter,
and then zerod it.I think this should be covered in comments as it is not apparent.
BTW, commit 5bec1d6bc5e also introduced
ReorderBufferChangeMemoryUpdate() in ReorderBufferTruncateTXN() which
is also worth considering while fixing the reported problem. It may
not have the same problem as you have reported but we can consider
whether setting txn size as zero explicitly is required or not.The reason why it introduced ReorderBufferChangeMemoryUpdate() is the
same as I mentioned above. And yes, as you mentioned, it doesn't have
the same problem that I reported here.I checked again and found that ReorderBufferResetTXN() first calls
ReorderBufferTruncateTXN() and then ReorderBufferToastReset(). After
that, it also tries to free spec_insert change which should have the
same problem. So, what saves this path from the same problem?I tried testing this scenario and I was able to reproduce the crash in
HEAD with this scenario. I have created a patch for the testcase.
I also tested the same scenario with the latest patch shared by
Sawada-san in [1]. And confirm that this resolves the issue.
Thank you for testing the patch!
I'm slightly hesitant to add a test under src/test/subscription since
it's a bug in ReorderBuffer and not specific to logical replication.
If we reasonably cannot add a test under contrib/test_decoding, I'm
okay with adding it under src/test/subscription.
I've attached the updated patch with the commit message (but without a
test case for now).
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
v3-0001-Fix-memory-counter-update-in-ReorderBuffer.patchapplication/octet-stream; name=v3-0001-Fix-memory-counter-update-in-ReorderBuffer.patchDownload
From bec9ffcaf60593986f88c2a717f61fe6dd8cdc30 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Tue, 20 Aug 2024 04:57:41 -0700
Subject: [PATCH v3] Fix memory counter update in ReorderBuffer.
Commit 5bec1d6bc5e changed the memory usage updates of the
ReorderBuffer to zero all at once by subtracting txn->size, rather
than updating it for each change. However, if TOAST reconstruction
data remained in the transaction when freeing it, there were cases
where it further subtracted the memory counter from zero, resulting in
an assertion failure.
This change calculates the memory size for each change and updates the
memory usage to precisely the amount that has been freed.
Backpatch to v17, where this was introducd.
Reviewed-by: Amit Kapila, Shlok Kyal
Discussion: https://postgr.es/m/CAD21AoAqkNUvicgKPT_dXzNoOwpPkVTg0QPPxEcWmzT0moCJ1g%40mail.gmail.com
Backpatch-through: 17
---
.../replication/logical/reorderbuffer.c | 32 ++++++++++++++++---
1 file changed, 28 insertions(+), 4 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 00a8327e77..bf5cf416bf 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -467,6 +467,9 @@ ReorderBufferReturnTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
/* Reset the toast hash */
ReorderBufferToastReset(rb, txn);
+ /* All changes must be returned */
+ Assert(txn->size == 0);
+
pfree(txn);
}
@@ -1506,6 +1509,7 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
{
bool found;
dlist_mutable_iter iter;
+ Size mem_freed = 0;
/* cleanup subtransactions & their changes */
dlist_foreach_modify(iter, &txn->subtxns)
@@ -1535,9 +1539,20 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
/* Check we're not mixing changes from different transactions. */
Assert(change->txn == txn);
+ /*
+ * Instead of updating the memory counter for individual changes,
+ * we sum up the size of memory to free so we can update the memory
+ * counter all together below. This saves costs of maintaining
+ * the max-heap.
+ */
+ mem_freed += ReorderBufferChangeSize(change);
+
ReorderBufferReturnChange(rb, change, false);
}
+ /* Update the memory counter */
+ ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, mem_freed);
+
/*
* Cleanup the tuplecids we stored for decoding catalog snapshot access.
* They are always stored in the toplevel transaction.
@@ -1594,9 +1609,6 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
if (rbtxn_is_serialized(txn))
ReorderBufferRestoreCleanup(rb, txn);
- /* Update the memory counter */
- ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);
-
/* deallocate */
ReorderBufferReturnTXN(rb, txn);
}
@@ -1616,6 +1628,7 @@ static void
ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, bool txn_prepared)
{
dlist_mutable_iter iter;
+ Size mem_freed = 0;
/* cleanup subtransactions & their changes */
dlist_foreach_modify(iter, &txn->subtxns)
@@ -1648,11 +1661,19 @@ ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, bool txn_prep
/* remove the change from it's containing list */
dlist_delete(&change->node);
+ /*
+ * Instead of updating the memory counter for individual changes,
+ * we sum up the size of memory to free so we can update the memory
+ * counter all together below. This saves costs of maintaining
+ * the max-heap.
+ */
+ mem_freed += ReorderBufferChangeSize(change);
+
ReorderBufferReturnChange(rb, change, false);
}
/* Update the memory counter */
- ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);
+ ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, mem_freed);
/*
* Mark the transaction as streamed.
@@ -2062,6 +2083,9 @@ ReorderBufferResetTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
rb->stream_stop(rb, txn, last_lsn);
ReorderBufferSaveTXNSnapshot(rb, txn, snapshot_now, command_id);
}
+
+ /* All changes must be returned */
+ Assert(txn->size == 0);
}
/*
--
2.39.3
On Tue, Aug 20, 2024 at 5:55 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Fri, Aug 16, 2024 at 12:22 AM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
Thank you for testing the patch!
I'm slightly hesitant to add a test under src/test/subscription since
it's a bug in ReorderBuffer and not specific to logical replication.
If we reasonably cannot add a test under contrib/test_decoding, I'm
okay with adding it under src/test/subscription.
Sounds like a reasonable approach.
I've attached the updated patch with the commit message (but without a
test case for now).
The patch LGTM except for one minor comment.
+ /* All changes must be returned */
+ Assert(txn->size == 0);
Isn't it better to say: "All changes must be deallocated." in the above comment?
--
With Regards,
Amit Kapila.
On Tue, Aug 20, 2024 at 9:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 20, 2024 at 5:55 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Fri, Aug 16, 2024 at 12:22 AM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
Thank you for testing the patch!
I'm slightly hesitant to add a test under src/test/subscription since
it's a bug in ReorderBuffer and not specific to logical replication.
If we reasonably cannot add a test under contrib/test_decoding, I'm
okay with adding it under src/test/subscription.Sounds like a reasonable approach.
I've managed to reproduce this issue without logical replication based
on the test shared by Shlok Kyal.
I've attached the updated patch with the commit message (but without a
test case for now).The patch LGTM except for one minor comment.
+ /* All changes must be returned */
+ Assert(txn->size == 0);Isn't it better to say: "All changes must be deallocated." in the above comment?
Agreed.
I've updated the updated patch with regression tests.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
v4-0001-Fix-memory-counter-update-in-ReorderBuffer.patchapplication/octet-stream; name=v4-0001-Fix-memory-counter-update-in-ReorderBuffer.patchDownload
From 64f190d0d88a15bf342543fe1d04408ea4a6dff5 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Tue, 20 Aug 2024 04:57:41 -0700
Subject: [PATCH v4] Fix memory counter update in ReorderBuffer.
Commit 5bec1d6bc5e changed the memory usage updates of the
ReorderBuffer to zero all at once by subtracting txn->size, rather
than updating it for each change. However, if TOAST reconstruction
data remained in the transaction when freeing it, there were cases
where it further subtracted the memory counter from zero, resulting in
an assertion failure.
This change calculates the memory size for each change and updates the
memory usage to precisely the amount that has been freed.
Backpatch to v17, where this was introducd.
Reviewed-by: Amit Kapila, Shlok Kyal
Discussion: https://postgr.es/m/CAD21AoAqkNUvicgKPT_dXzNoOwpPkVTg0QPPxEcWmzT0moCJ1g%40mail.gmail.com
Backpatch-through: 17
---
contrib/test_decoding/expected/stream.out | 19 +++++++++++
contrib/test_decoding/sql/stream.sql | 15 +++++++++
.../replication/logical/reorderbuffer.c | 32 ++++++++++++++++---
3 files changed, 62 insertions(+), 4 deletions(-)
diff --git a/contrib/test_decoding/expected/stream.out b/contrib/test_decoding/expected/stream.out
index 4ab2d47bf8..a76f77601e 100644
--- a/contrib/test_decoding/expected/stream.out
+++ b/contrib/test_decoding/expected/stream.out
@@ -109,6 +109,25 @@ SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL,NULL, 'incl
committing streamed transaction
(17 rows)
+/*
+ * Test concurrent abort with toast data. When streaming the second insertion, we
+ * detect that the subtransaction was aborted, and reset the transaction while having
+ * the TOAST changes in memory, resulting in deallocating both decoded changes and
+ * TOAST reconstruction data. Memory usage counters must be updated correctly.
+ */
+BEGIN;
+INSERT INTO stream_test SELECT repeat(string_agg(to_char(g.i, 'FM0000'), ''), 50) FROM generate_series(1, 500) g(i);
+ALTER TABLE stream_test ADD COLUMN i INT;
+SAVEPOINT s1;
+INSERT INTO stream_test(data, i) SELECT repeat(string_agg(to_char(g.i, 'FM0000'), ''), 50), 1 FROM generate_series(1, 500) g(i);
+ROLLBACK TO s1;
+COMMIT;
+SELECT count(*) FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1', 'stream-changes', '1');
+ count
+-------
+ 5
+(1 row)
+
DROP TABLE stream_test;
SELECT pg_drop_replication_slot('regression_slot');
pg_drop_replication_slot
diff --git a/contrib/test_decoding/sql/stream.sql b/contrib/test_decoding/sql/stream.sql
index 4feec62972..7f43f0c2ab 100644
--- a/contrib/test_decoding/sql/stream.sql
+++ b/contrib/test_decoding/sql/stream.sql
@@ -44,5 +44,20 @@ toasted-123456789012345678901234567890123456789012345678901234567890123456789012
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL,NULL, 'include-xids', '0', 'skip-empty-xacts', '1', 'stream-changes', '1');
+/*
+ * Test concurrent abort with toast data. When streaming the second insertion, we
+ * detect that the subtransaction was aborted, and reset the transaction while having
+ * the TOAST changes in memory, resulting in deallocating both decoded changes and
+ * TOAST reconstruction data. Memory usage counters must be updated correctly.
+ */
+BEGIN;
+INSERT INTO stream_test SELECT repeat(string_agg(to_char(g.i, 'FM0000'), ''), 50) FROM generate_series(1, 500) g(i);
+ALTER TABLE stream_test ADD COLUMN i INT;
+SAVEPOINT s1;
+INSERT INTO stream_test(data, i) SELECT repeat(string_agg(to_char(g.i, 'FM0000'), ''), 50), 1 FROM generate_series(1, 500) g(i);
+ROLLBACK TO s1;
+COMMIT;
+SELECT count(*) FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1', 'stream-changes', '1');
+
DROP TABLE stream_test;
SELECT pg_drop_replication_slot('regression_slot');
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 00a8327e77..b3139c41e2 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -467,6 +467,9 @@ ReorderBufferReturnTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
/* Reset the toast hash */
ReorderBufferToastReset(rb, txn);
+ /* All changes must be deallocated */
+ Assert(txn->size == 0);
+
pfree(txn);
}
@@ -1506,6 +1509,7 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
{
bool found;
dlist_mutable_iter iter;
+ Size mem_freed = 0;
/* cleanup subtransactions & their changes */
dlist_foreach_modify(iter, &txn->subtxns)
@@ -1535,9 +1539,20 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
/* Check we're not mixing changes from different transactions. */
Assert(change->txn == txn);
+ /*
+ * Instead of updating the memory counter for individual changes,
+ * we sum up the size of memory to free so we can update the memory
+ * counter all together below. This saves costs of maintaining
+ * the max-heap.
+ */
+ mem_freed += ReorderBufferChangeSize(change);
+
ReorderBufferReturnChange(rb, change, false);
}
+ /* Update the memory counter */
+ ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, mem_freed);
+
/*
* Cleanup the tuplecids we stored for decoding catalog snapshot access.
* They are always stored in the toplevel transaction.
@@ -1594,9 +1609,6 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
if (rbtxn_is_serialized(txn))
ReorderBufferRestoreCleanup(rb, txn);
- /* Update the memory counter */
- ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);
-
/* deallocate */
ReorderBufferReturnTXN(rb, txn);
}
@@ -1616,6 +1628,7 @@ static void
ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, bool txn_prepared)
{
dlist_mutable_iter iter;
+ Size mem_freed = 0;
/* cleanup subtransactions & their changes */
dlist_foreach_modify(iter, &txn->subtxns)
@@ -1648,11 +1661,19 @@ ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, bool txn_prep
/* remove the change from it's containing list */
dlist_delete(&change->node);
+ /*
+ * Instead of updating the memory counter for individual changes,
+ * we sum up the size of memory to free so we can update the memory
+ * counter all together below. This saves costs of maintaining
+ * the max-heap.
+ */
+ mem_freed += ReorderBufferChangeSize(change);
+
ReorderBufferReturnChange(rb, change, false);
}
/* Update the memory counter */
- ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);
+ ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, mem_freed);
/*
* Mark the transaction as streamed.
@@ -2062,6 +2083,9 @@ ReorderBufferResetTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
rb->stream_stop(rb, txn, last_lsn);
ReorderBufferSaveTXNSnapshot(rb, txn, snapshot_now, command_id);
}
+
+ /* All changes must be deallocated */
+ Assert(txn->size == 0);
}
/*
--
2.39.3
On Fri, Aug 23, 2024 at 3:44 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Tue, Aug 20, 2024 at 9:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
I've updated the updated patch with regression tests.
LGTM.
--
With Regards,
Amit Kapila.
On Sun, Aug 25, 2024 at 9:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Fri, Aug 23, 2024 at 3:44 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Tue, Aug 20, 2024 at 9:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
I've updated the updated patch with regression tests.
LGTM.
Thank you for reviewing the patch. Pushed.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On Mon, Aug 26, 2024 at 11:25:53AM -0700, Masahiko Sawada wrote:
Thank you for reviewing the patch. Pushed.
nitpick: I think this one needs a pgindent run.
--
nathan
On Mon, Aug 26, 2024 at 2:27 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
On Mon, Aug 26, 2024 at 11:25:53AM -0700, Masahiko Sawada wrote:
Thank you for reviewing the patch. Pushed.
nitpick: I think this one needs a pgindent run.
Thank you for pointing it out. I forgot to check with pgindent. I've
pushed the change. Hope it
fixes the problem.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com