Decoding speculative insert with toast leaks memory
Hi All,
We saw OOM in a system where WAL sender consumed Gigabttes of memory
which was never released. Upon investigation, we found out that there
were many ReorderBufferToastHash memory contexts linked to
ReorderBuffer context, together consuming gigs of memory. They were
running INSERT ... ON CONFLICT .. among other things. A similar report
at [1]https://www.postgresql-archive.org/Diagnose-memory-leak-in-logical-replication-td6161318.html
I could reproduce a memory leak in wal sender using following steps
Session 1
postgres=# create table t_toast (a int primary key, b text);
postgres=# CREATE PUBLICATION dbz_minimal_publication FOR TABLE public.t_toast;
Terminal 4
$ pg_recvlogical -d postgres --slot pgoutput_minimal_test_slot
--create-slot -P pgoutput
$ pg_recvlogical -d postgres --slot pgoutput_minimal_test_slot --start
-o proto_version=1 -o publication_names='dbz_minimal_publication' -f
/dev/null
Session 1
postgres=# select * from pg_replication_slots ;
slot_name | plugin | slot_type | datoid | database
| temporary | active | active_pid | xmin | catalog_xmin | restart_lsn
| confirmed_flush_lsn
----------------------------+----------+-----------+--------+----------+-----------+--------+------------+------+--------------+-------------+---------------------
pgoutput_minimal_test_slot | pgoutput | logical | 12402 | postgres
| f | f | | | 570 | 0/15FFFD0
| 0/1600020
postgres=# begin;
postgres=# insert into t_toast values (500, repeat('something' ||
txid_current()::text, 100000)) ON CONFLICT (a) DO NOTHING;
INSERT 0 1
Session 2 (xid = 571)
postgres=# begin;
postgres=# insert into t_toast values (500, repeat('something' ||
txid_current()::text, 100000)) ON CONFLICT (a) DO NOTHING;
Session 3 (xid = 572)
postgres=# begin;
postgres=# insert into t_toast values (500, repeat('something' ||
txid_current()::text, 100000)) ON CONFLICT (a) DO NOTHING;
Session 1 (this session doesn't modify the table but is essential for
speculative insert to happen.)
postgres=# rollback;
Session 2 and 3 in the order in which you get control back (in my case
session 2 with xid = 571)
INSERT 0 1
postgres=# commit;
COMMIT
other session (in my case session 3 with xid = 572)
INSERT 0 0
postgres=# commit;
COMMIT
With the attached patch, we see following in the server logs
2021-03-25 09:57:20.469 IST [12424] LOG: starting logical decoding
for slot "pgoutput_minimal_test_slot"
2021-03-25 09:57:20.469 IST [12424] DETAIL: Streaming transactions
committing after 0/1600020, reading WAL from 0/15FFFD0.
2021-03-25 09:57:20.469 IST [12424] LOG: logical decoding found
consistent point at 0/15FFFD0
2021-03-25 09:57:20.469 IST [12424] DETAIL: There are no running transactions.
2021-03-25 09:59:45.494 IST [12424] LOG: initializing hash table for
transaction 571
2021-03-25 09:59:45.494 IST [12424] LOG: speculative insert
encountered in transaction 571
2021-03-25 09:59:45.494 IST [12424] LOG: speculative insert confirmed
in transaction 571
2021-03-25 09:59:45.504 IST [12424] LOG: destroying toast hash for
transaction 571
2021-03-25 09:59:50.806 IST [12424] LOG: initializing hash table for
transaction 572
2021-03-25 09:59:50.806 IST [12424] LOG: speculative insert
encountered in transaction 572
2021-03-25 09:59:50.806 IST [12424] LOG: toast hash for transaction
572 is not cleared
Observe that the toast_hash was cleaned for the transaction 571 which
successfully inserted the row but was not cleaned for the transaction
572 which performed DO NOTHING instead of INSERT.
Here's the sequence of events which leads to memory leak in wal sender
1. Transaction 571 performs a speculative INSERT which is decoded as
toast insert followed by speculative insert of row
2. decoding toast tuple, causes the toast hash to be created
3. Speculative insert is ignored while decoding
4. Speculative insert is confimed and decoded as a normal INSERT, also
destroying the toast hash
5. Transaction 572 performs speculative insert which is decoded as
toast insert followed by speculative insert of row
6. decoding toast tuple causes the toast hash to be created
7. speculative insert is ignored while decoding
... Speculative INSERT is never confirmed and thus toast hash is never
destroyed leaking memory
In memory context dump we see as many ReorderBufferToastHash as the
number of times the above sequence is repeated.
TopMemoryContext: 1279640 total in 7 blocks; 23304 free (17 chunks);
1256336 used
...
Replication command context: 32768 total in 3 blocks; 10952 free
(9 chunks); 21816 used
...
ReorderBuffer: 8192 total in 1 blocks; 7656 free (7 chunks); 536 used
ReorderBufferToastHash: 8192 total in 1 blocks; 2056 free (0
chunks); 6136 used
ReorderBufferToastHash: 8192 total in 1 blocks; 2056 free (0
chunks); 6136 used
ReorderBufferToastHash: 8192 total in 1 blocks; 2056 free (0
chunks); 6136 used
The relevant code is all in ReoderBufferCommit() in cases
REORDER_BUFFER_CHANGE_INSERT,
REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT and
REORDER_BUFFER_CHANGE_INTERNAL_SPEC_CONFIRM.
About the solution: The speculative insert needs to be ignored since
it can be rolled back later. If speculative insert is not confirmed,
there is no way to know that the speculative insert change required a
toast_hash table and destroy it before the next change starts.
ReorderBufferCommit seems to notice a speculative insert that was
never confirmed in the following code
1624 change_done:
1625
1626 /*
1627 * Either speculative insertion was
confirmed, or it was
1628 * unsuccessful and the record isn't needed anymore.
1629 */
1630 if (specinsert != NULL)
1631 {
1632 ReorderBufferReturnChange(rb, specinsert);
1633 specinsert = NULL;
1634 }
1635
1636 if (relation != NULL)
1637 {
1638 RelationClose(relation);
1639 relation = NULL;
1640 }
1641 break;
but by then we might have reused the toast_hash and thus can not be
destroyed. But that isn't the problem since the reused toast_hash will
be destroyed eventually.
It's only when the change next to speculative insert is something
other than INSERT/UPDATE/DELETE that we have to worry about a
speculative insert that was never confirmed. So may be for those
cases, we check whether specinsert != null and destroy toast_hash if
it exists.
[1]: https://www.postgresql-archive.org/Diagnose-memory-leak-in-logical-replication-td6161318.html
--
Best Wishes,
Ashutosh Bapat
Attachments:
rb_toas_hash_mem_leak.patchtext/x-patch; charset=US-ASCII; name=rb_toas_hash_mem_leak.patchDownload+262-0
On Wed, Mar 24, 2021 at 10:34 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
Hi All,
We saw OOM in a system where WAL sender consumed Gigabttes of memory
which was never released. Upon investigation, we found out that there
were many ReorderBufferToastHash memory contexts linked to
ReorderBuffer context, together consuming gigs of memory. They were
running INSERT ... ON CONFLICT .. among other things. A similar report
at [1]
What is the relationship between this bug and commit 7259736a6e5,
dealt specifically with TOAST and speculative insertion resource
management issues within reorderbuffer.c? Amit?
--
Peter Geoghegan
On Thu, Mar 25, 2021 at 11:04 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
Hi All,
We saw OOM in a system where WAL sender consumed Gigabttes of memory
which was never released. Upon investigation, we found out that there
were many ReorderBufferToastHash memory contexts linked to
ReorderBuffer context, together consuming gigs of memory. They were
running INSERT ... ON CONFLICT .. among other things. A similar report
at [1]
..
but by then we might have reused the toast_hash and thus can not be
destroyed. But that isn't the problem since the reused toast_hash will
be destroyed eventually.It's only when the change next to speculative insert is something
other than INSERT/UPDATE/DELETE that we have to worry about a
speculative insert that was never confirmed. So may be for those
cases, we check whether specinsert != null and destroy toast_hash if
it exists.
Can we consider the possibility to destroy the toast_hash in
ReorderBufferCleanupTXN/ReorderBufferTruncateTXN? It will delay the
clean up of memory till the end of stream or txn but there won't be
any memory leak.
--
With Regards,
Amit Kapila.
On Thu, May 27, 2021 at 8:27 AM Peter Geoghegan <pg@bowt.ie> wrote:
On Wed, Mar 24, 2021 at 10:34 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:Hi All,
We saw OOM in a system where WAL sender consumed Gigabttes of memory
which was never released. Upon investigation, we found out that there
were many ReorderBufferToastHash memory contexts linked to
ReorderBuffer context, together consuming gigs of memory. They were
running INSERT ... ON CONFLICT .. among other things. A similar report
at [1]What is the relationship between this bug and commit 7259736a6e5,
dealt specifically with TOAST and speculative insertion resource
management issues within reorderbuffer.c? Amit?
This seems to be a pre-existing bug. This should be reproduced in
PG-13 and or prior to that commit. Ashutosh can confirm?
--
With Regards,
Amit Kapila.
On Thu, May 27, 2021 at 9:02 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Mar 25, 2021 at 11:04 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:Hi All,
We saw OOM in a system where WAL sender consumed Gigabttes of memory
which was never released. Upon investigation, we found out that there
were many ReorderBufferToastHash memory contexts linked to
ReorderBuffer context, together consuming gigs of memory. They were
running INSERT ... ON CONFLICT .. among other things. A similar report
at [1]..
but by then we might have reused the toast_hash and thus can not be
destroyed. But that isn't the problem since the reused toast_hash will
be destroyed eventually.It's only when the change next to speculative insert is something
other than INSERT/UPDATE/DELETE that we have to worry about a
speculative insert that was never confirmed. So may be for those
cases, we check whether specinsert != null and destroy toast_hash if
it exists.Can we consider the possibility to destroy the toast_hash in
ReorderBufferCleanupTXN/ReorderBufferTruncateTXN? It will delay the
clean up of memory till the end of stream or txn but there won't be
any memory leak.
The other possibility could be to clean it up when we clean the spec
insert change in the below code:
/*
* There's a speculative insertion remaining, just clean in up, it
* can't have been successful, otherwise we'd gotten a confirmation
* record.
*/
if (specinsert)
{
ReorderBufferReturnChange(rb, specinsert, true);
specinsert = NULL;
}
But I guess we might miss cleaning it up in case of an error. A
similar problem could be there in the idea where we will try to tie
the clean up with the next change.
--
With Regards,
Amit Kapila.
On Thu, May 27, 2021 at 9:03 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Mar 25, 2021 at 11:04 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:Hi All,
We saw OOM in a system where WAL sender consumed Gigabttes of memory
which was never released. Upon investigation, we found out that there
were many ReorderBufferToastHash memory contexts linked to
ReorderBuffer context, together consuming gigs of memory. They were
running INSERT ... ON CONFLICT .. among other things. A similar report
at [1]..
but by then we might have reused the toast_hash and thus can not be
destroyed. But that isn't the problem since the reused toast_hash will
be destroyed eventually.It's only when the change next to speculative insert is something
other than INSERT/UPDATE/DELETE that we have to worry about a
speculative insert that was never confirmed. So may be for those
cases, we check whether specinsert != null and destroy toast_hash if
it exists.Can we consider the possibility to destroy the toast_hash in
ReorderBufferCleanupTXN/ReorderBufferTruncateTXN? It will delay the
clean up of memory till the end of stream or txn but there won't be
any memory leak.
Currently, we are ignoring XLH_DELETE_IS_SUPER, so maybe we can do
something based on this flag?
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Thu, May 27, 2021 at 9:26 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, May 27, 2021 at 9:02 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Mar 25, 2021 at 11:04 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:Hi All,
We saw OOM in a system where WAL sender consumed Gigabttes of memory
which was never released. Upon investigation, we found out that there
were many ReorderBufferToastHash memory contexts linked to
ReorderBuffer context, together consuming gigs of memory. They were
running INSERT ... ON CONFLICT .. among other things. A similar report
at [1]..
but by then we might have reused the toast_hash and thus can not be
destroyed. But that isn't the problem since the reused toast_hash will
be destroyed eventually.It's only when the change next to speculative insert is something
other than INSERT/UPDATE/DELETE that we have to worry about a
speculative insert that was never confirmed. So may be for those
cases, we check whether specinsert != null and destroy toast_hash if
it exists.Can we consider the possibility to destroy the toast_hash in
ReorderBufferCleanupTXN/ReorderBufferTruncateTXN? It will delay the
clean up of memory till the end of stream or txn but there won't be
any memory leak.The other possibility could be to clean it up when we clean the spec
insert change in the below code:
Yeah that could be done.
/*
* There's a speculative insertion remaining, just clean in up, it
* can't have been successful, otherwise we'd gotten a confirmation
* record.
*/
if (specinsert)
{
ReorderBufferReturnChange(rb, specinsert, true);
specinsert = NULL;
}But I guess we might miss cleaning it up in case of an error. A
similar problem could be there in the idea where we will try to tie
the clean up with the next change.
In error case also we can handle it in the CATCH block no?
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Thu, May 27, 2021 at 9:40 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Thu, May 27, 2021 at 9:26 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
Can we consider the possibility to destroy the toast_hash in
ReorderBufferCleanupTXN/ReorderBufferTruncateTXN? It will delay the
clean up of memory till the end of stream or txn but there won't be
any memory leak.The other possibility could be to clean it up when we clean the spec
insert change in the below code:Yeah that could be done.
/*
* There's a speculative insertion remaining, just clean in up, it
* can't have been successful, otherwise we'd gotten a confirmation
* record.
*/
if (specinsert)
{
ReorderBufferReturnChange(rb, specinsert, true);
specinsert = NULL;
}But I guess we might miss cleaning it up in case of an error. A
similar problem could be there in the idea where we will try to tie
the clean up with the next change.In error case also we can handle it in the CATCH block no?
True, but if you do this clean-up in ReorderBufferCleanupTXN then you
don't need to take care at separate places. Also, toast_hash is stored
in txn so it appears natural to clean it up in while releasing TXN.
--
With Regards,
Amit Kapila.
On Thu, May 27, 2021 at 9:47 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, May 27, 2021 at 9:40 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
True, but if you do this clean-up in ReorderBufferCleanupTXN then you
don't need to take care at separate places. Also, toast_hash is stored
in txn so it appears natural to clean it up in while releasing TXN.
Make sense, basically, IMHO we will have to do in TruncateTXN and
ReturnTXN as attached?
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v1-0001-Cleanup-toast-hash.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Cleanup-toast-hash.patchDownload+8-1
On 5/27/21 6:36 AM, Dilip Kumar wrote:
On Thu, May 27, 2021 at 9:47 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, May 27, 2021 at 9:40 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
True, but if you do this clean-up in ReorderBufferCleanupTXN then you
don't need to take care at separate places. Also, toast_hash is stored
in txn so it appears natural to clean it up in while releasing TXN.Make sense, basically, IMHO we will have to do in TruncateTXN and
ReturnTXN as attached?
Yeah, I've been working on a fix over the last couple days (because of a
customer issue), and I ended up with the reset in ReorderBufferReturnTXN
too - it does solve the issue in the case I've been investigating.
I'm not sure the reset in ReorderBufferTruncateTXN is correct, though.
Isn't it possible that we'll need the TOAST data after streaming part of
the transaction? After all, we're not resetting invalidations, tuplecids
and snapshot either ... And we'll eventually clean it after the streamed
transaction gets commited (ReorderBufferStreamCommit ends up calling
ReorderBufferReturnTXN too).
I wonder if there's a way to free the TOASTed data earlier, instead of
waiting until the end of the transaction (as this patch does). But I
suspect it'd be way more complex, harder to backpatch, and destroying
the hash table is a good idea anyway.
Also, I think the "if (txn->toast_hash != NULL)" checks are not needed,
because it's the first thing ReorderBufferToastReset does.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Fri, May 28, 2021 at 5:16 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
On 5/27/21 6:36 AM, Dilip Kumar wrote:
On Thu, May 27, 2021 at 9:47 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, May 27, 2021 at 9:40 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
True, but if you do this clean-up in ReorderBufferCleanupTXN then you
don't need to take care at separate places. Also, toast_hash is stored
in txn so it appears natural to clean it up in while releasing TXN.Make sense, basically, IMHO we will have to do in TruncateTXN and
ReturnTXN as attached?Yeah, I've been working on a fix over the last couple days (because of a
customer issue), and I ended up with the reset in ReorderBufferReturnTXN
too - it does solve the issue in the case I've been investigating.I'm not sure the reset in ReorderBufferTruncateTXN is correct, though.
Isn't it possible that we'll need the TOAST data after streaming part of
the transaction? After all, we're not resetting invalidations, tuplecids
and snapshot either
Actually, as per the current design, we don't need the toast data
after the streaming. Because currently, we don't allow to stream the
transaction if we need to keep the toast across stream e.g. if we only
have toast insert without the main insert we assure this as partial
changes, another case is if we have multi-insert with toast then we
keep the transaction as mark partial until we get the last insert of
the multi-insert. So with the current design we don't have any case
where we need to keep toast data across streams.
... And we'll eventually clean it after the streamed
transaction gets commited (ReorderBufferStreamCommit ends up calling
ReorderBufferReturnTXN too).
Right, but generally after streaming we assert that txn->size should
be 0. That could be changed if we change the above design but this is
what it is today.
I wonder if there's a way to free the TOASTed data earlier, instead of
waiting until the end of the transaction (as this patch does). But I
suspect it'd be way more complex, harder to backpatch, and destroying
the hash table is a good idea anyway.
Right.
Also, I think the "if (txn->toast_hash != NULL)" checks are not needed,
because it's the first thing ReorderBufferToastReset does.
I see, I will change this. If we all agree with this idea.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On 5/28/21 2:17 PM, Dilip Kumar wrote:
On Fri, May 28, 2021 at 5:16 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:On 5/27/21 6:36 AM, Dilip Kumar wrote:
On Thu, May 27, 2021 at 9:47 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, May 27, 2021 at 9:40 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
True, but if you do this clean-up in ReorderBufferCleanupTXN then you
don't need to take care at separate places. Also, toast_hash is stored
in txn so it appears natural to clean it up in while releasing TXN.Make sense, basically, IMHO we will have to do in TruncateTXN and
ReturnTXN as attached?Yeah, I've been working on a fix over the last couple days (because of a
customer issue), and I ended up with the reset in ReorderBufferReturnTXN
too - it does solve the issue in the case I've been investigating.I'm not sure the reset in ReorderBufferTruncateTXN is correct, though.
Isn't it possible that we'll need the TOAST data after streaming part of
the transaction? After all, we're not resetting invalidations, tuplecids
and snapshot eitherActually, as per the current design, we don't need the toast data
after the streaming. Because currently, we don't allow to stream the
transaction if we need to keep the toast across stream e.g. if we only
have toast insert without the main insert we assure this as partial
changes, another case is if we have multi-insert with toast then we
keep the transaction as mark partial until we get the last insert of
the multi-insert. So with the current design we don't have any case
where we need to keep toast data across streams.... And we'll eventually clean it after the streamed
transaction gets commited (ReorderBufferStreamCommit ends up calling
ReorderBufferReturnTXN too).Right, but generally after streaming we assert that txn->size should
be 0. That could be changed if we change the above design but this is
what it is today.
Can we add some assert to enforce this?
I wonder if there's a way to free the TOASTed data earlier, instead of
waiting until the end of the transaction (as this patch does). But I
suspect it'd be way more complex, harder to backpatch, and destroying
the hash table is a good idea anyway.Right.
Also, I think the "if (txn->toast_hash != NULL)" checks are not needed,
because it's the first thing ReorderBufferToastReset does.I see, I will change this. If we all agree with this idea.
+1 from me. I think it's good enough to do the cleanup at the end, and
it's an improvement compared to current state. There might be cases of
transactions doing many such speculative inserts and accumulating a lot
of data in the TOAST hash, but I find it very unlikely.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Fri, May 28, 2021 at 6:01 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
On 5/28/21 2:17 PM, Dilip Kumar wrote:
On Fri, May 28, 2021 at 5:16 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:On 5/27/21 6:36 AM, Dilip Kumar wrote:
On Thu, May 27, 2021 at 9:47 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, May 27, 2021 at 9:40 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
True, but if you do this clean-up in ReorderBufferCleanupTXN then you
don't need to take care at separate places. Also, toast_hash is stored
in txn so it appears natural to clean it up in while releasing TXN.Make sense, basically, IMHO we will have to do in TruncateTXN and
ReturnTXN as attached?Yeah, I've been working on a fix over the last couple days (because of a
customer issue), and I ended up with the reset in ReorderBufferReturnTXN
too - it does solve the issue in the case I've been investigating.I'm not sure the reset in ReorderBufferTruncateTXN is correct, though.
Isn't it possible that we'll need the TOAST data after streaming part of
the transaction? After all, we're not resetting invalidations, tuplecids
and snapshot eitherActually, as per the current design, we don't need the toast data
after the streaming. Because currently, we don't allow to stream the
transaction if we need to keep the toast across stream e.g. if we only
have toast insert without the main insert we assure this as partial
changes, another case is if we have multi-insert with toast then we
keep the transaction as mark partial until we get the last insert of
the multi-insert. So with the current design we don't have any case
where we need to keep toast data across streams.... And we'll eventually clean it after the streamed
transaction gets commited (ReorderBufferStreamCommit ends up calling
ReorderBufferReturnTXN too).Right, but generally after streaming we assert that txn->size should
be 0. That could be changed if we change the above design but this is
what it is today.Can we add some assert to enforce this?
There is already an Assert for this. See ReorderBufferCheckMemoryLimit.
--
With Regards,
Amit Kapila.
On Fri, May 28, 2021 at 5:16 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
I wonder if there's a way to free the TOASTed data earlier, instead of
waiting until the end of the transaction (as this patch does).
IIUC we are anyway freeing the toasted data at the next
insert/update/delete. We can try to free at other change message types
like REORDER_BUFFER_CHANGE_MESSAGE but as you said that may make the
patch more complex, so it seems better to do the fix on the lines of
what is proposed in the patch.
--
With Regards,
Amit Kapila.
On 5/29/21 6:29 AM, Amit Kapila wrote:
On Fri, May 28, 2021 at 5:16 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:I wonder if there's a way to free the TOASTed data earlier, instead of
waiting until the end of the transaction (as this patch does).IIUC we are anyway freeing the toasted data at the next
insert/update/delete. We can try to free at other change message types
like REORDER_BUFFER_CHANGE_MESSAGE but as you said that may make the
patch more complex, so it seems better to do the fix on the lines of
what is proposed in the patch.
+1
Even if we started doing what you mention (freeing the hash for other
change types), we'd still need to do what the patch proposes because the
speculative insert may be the last change in the transaction. For the
other cases it works as a mitigation, so that we don't leak the memory
forever.
So let's get this committed, perhaps with a comment explaining that it
might be possible to reset earlier if needed.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Sat, May 29, 2021 at 5:45 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
On 5/29/21 6:29 AM, Amit Kapila wrote:
On Fri, May 28, 2021 at 5:16 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:I wonder if there's a way to free the TOASTed data earlier, instead of
waiting until the end of the transaction (as this patch does).IIUC we are anyway freeing the toasted data at the next
insert/update/delete. We can try to free at other change message types
like REORDER_BUFFER_CHANGE_MESSAGE but as you said that may make the
patch more complex, so it seems better to do the fix on the lines of
what is proposed in the patch.+1
Even if we started doing what you mention (freeing the hash for other
change types), we'd still need to do what the patch proposes because the
speculative insert may be the last change in the transaction. For the
other cases it works as a mitigation, so that we don't leak the memory
forever.
Right.
So let's get this committed, perhaps with a comment explaining that it
might be possible to reset earlier if needed.
Okay, I think it would be better if we can test this once for the
streaming case as well. Dilip, would you like to do that and send the
updated patch as per one of the comments by Tomas?
--
With Regards,
Amit Kapila.
On Mon, 31 May 2021 at 8:21 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sat, May 29, 2021 at 5:45 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:On 5/29/21 6:29 AM, Amit Kapila wrote:
On Fri, May 28, 2021 at 5:16 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:I wonder if there's a way to free the TOASTed data earlier, instead of
waiting until the end of the transaction (as this patch does).IIUC we are anyway freeing the toasted data at the next
insert/update/delete. We can try to free at other change message types
like REORDER_BUFFER_CHANGE_MESSAGE but as you said that may make the
patch more complex, so it seems better to do the fix on the lines of
what is proposed in the patch.+1
Even if we started doing what you mention (freeing the hash for other
change types), we'd still need to do what the patch proposes because the
speculative insert may be the last change in the transaction. For the
other cases it works as a mitigation, so that we don't leak the memory
forever.Right.
So let's get this committed, perhaps with a comment explaining that it
might be possible to reset earlier if needed.Okay, I think it would be better if we can test this once for the
streaming case as well. Dilip, would you like to do that and send the
updated patch as per one of the comments by Tomas?
I will do that in sometime.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Mon, May 31, 2021 at 8:50 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Mon, 31 May 2021 at 8:21 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
Okay, I think it would be better if we can test this once for the
streaming case as well. Dilip, would you like to do that and send the
updated patch as per one of the comments by Tomas?I will do that sometime.
I have changed patches as Tomas suggested and also created back patches.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v1-0001-Fix-memory-leak-in-toast-hash_v13-to-v9.6.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Fix-memory-leak-in-toast-hash_v13-to-v9.6.patchDownload+3-1
v1-0001-Fix-memory-leak-in-toast-hash-v14.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Fix-memory-leak-in-toast-hash-v14.patchDownload+6-1
On Mon, 31 May 2021 at 4:29 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Mon, May 31, 2021 at 8:50 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Mon, 31 May 2021 at 8:21 AM, Amit Kapila <amit.kapila16@gmail.com>
wrote:
Okay, I think it would be better if we can test this once for the
streaming case as well. Dilip, would you like to do that and send the
updated patch as per one of the comments by Tomas?I will do that sometime.
I have changed patches as Tomas suggested and also created back patches.
I missed to do the test for streaming. I will to that tomorrow and reply
back.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Mon, May 31, 2021 at 6:32 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Mon, 31 May 2021 at 4:29 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Mon, May 31, 2021 at 8:50 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Mon, 31 May 2021 at 8:21 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
Okay, I think it would be better if we can test this once for the
streaming case as well. Dilip, would you like to do that and send the
updated patch as per one of the comments by Tomas?I will do that sometime.
I have changed patches as Tomas suggested and also created back patches.
I missed to do the test for streaming. I will to that tomorrow and reply back.
For streaming transactions this issue is not there. Because this
problem will only occur if the last change is *SPEC INSERT * and after
that there is no other UPDATE/INSERT change because on that change we
are resetting the toast table. Now, if the transaction has only *SPEC
INSERT* without SPEC CONFIRM or any other INSERT/UPDATE then we will
not stream it. And if we get any next INSERT/UPDATE then only we can
select this for stream but in that case toast will be reset. So as of
today for streaming mode we don't have this problem.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
