public inbox for [email protected]
help / color / mirror / Atom feedFrom: Zhijie Hou (Fujitsu) <[email protected]>
To: vignesh C <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: RE: Memory leak in pg_logical_slot_{get,peek}_changes
Date: Wed, 11 Dec 2024 06:09:15 +0000
Message-ID: <OS0PR01MB5716AD5128F16AA1A02B6D29943E2@OS0PR01MB5716.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CALDaNm1hewNAsZ_e6FF52a=9drmkRJxtEPrzCB6-9mkJyeBBqA@mail.gmail.com>
References: <CALDaNm1hewNAsZ_e6FF52a=9drmkRJxtEPrzCB6-9mkJyeBBqA@mail.gmail.com>
On Wednesday, December 11, 2024 12:28 PM vignesh C <[email protected]> wrote:
> Hi,
>
> I'm starting a new thread for one of the issues reported by Sawada-san at [1].
>
> This is a memory leak on CacheMemoryContext when using pgoutput via SQL
> APIs:
> /* Map must live as long as the session does. */
> oldctx = MemoryContextSwitchTo(CacheMemoryContext);
>
> entry->attrmap = build_attrmap_by_name_if_req(indesc, outdesc, false);
>
> MemoryContextSwitchTo(oldctx);
> RelationClose(ancestor);
>
> entry->attrmap is pfree'd only when validating the RelationSyncEntry
> so remains even after logical decoding API calls.
>
> This issue can be reproduced with the following test:
> -- Create table
> create table t1( c1 int, c2 int, c3 int) PARTITION BY LIST (c1);
> create table t1_1( c2 int, c1 int, c3 int);
> ALTER TABLE t1 ATTACH PARTITION t1_1 FOR VALUES IN (0, 1, 2, 3);
>
> -- Create publication
> create publication pub1 FOR TABLE t1, t1_1 WITH
> (publish_via_partition_root = true);
>
> -- Create replication slot
> SELECT 'init' FROM pg_create_logical_replication_slot('test', 'pgoutput');
>
> -- Run the below steps between Test start and Test end many times to
> see the memory usage getting increased
> -- Test start
> insert into t1 values( 1);
> SELECT count(*) FROM pg_logical_slot_get_binary_changes('test', NULL,
> NULL, 'proto_version', '4', 'publication_names', 'pub1');
>
> -- Memory increases after each invalidation and insert
> SELECT * FROM pg_backend_memory_contexts where name =
> 'CacheMemoryContext';
> -- Test end
>
> The attached patch resolves a memory leak by ensuring that the
> attribute map is properly freed during plugin shutdown. This process
> is triggered by the SQL API when the decoding context is being
> released. Additionally, I am conducting a review to identify and
> address any similar memory leaks that may exist elsewhere in the code.
Thanks for reporting the issue and share the fix.
I am not sure if freeing them in shutdown callback is safe, because shutdown
callback Is not invoked in case of ERRORs. I think we'd better allocate them
under cachectx in the beginning to avoid freeing them explicitly.
The attachment is a POC that I internally experimented before. It replaces not
only the attrmap's context, but also others which were allocated under
CacheMemoryContext, to be consistent. Note that I have not tested it deeply.
Feel free to use if it works for you.
But the patch can only be used since PG15 where cachectx is introduced. In
older braches, we may need to think some other ways.
Best Regards,
Hou zj
Attachments:
[application/octet-stream] 0002-Fix-memory-leak-under-CacheMemoryContext-in-pgoutput.patch (2.4K, ../OS0PR01MB5716AD5128F16AA1A02B6D29943E2@OS0PR01MB5716.jpnprd01.prod.outlook.com/2-0002-Fix-memory-leak-under-CacheMemoryContext-in-pgoutput.patch)
download | inline diff:
From d7bde54c7a08407513755bed59148e4c03f0df40 Mon Sep 17 00:00:00 2001
From: Hou Zhijie <[email protected]>
Date: Wed, 4 Dec 2024 15:14:26 +0800
Subject: [PATCH 2/2] Fix memory leak under CacheMemoryContext in pgoutput
---
src/backend/replication/pgoutput/pgoutput.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 7a27ee38a0..2d81cd423e 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -222,7 +222,8 @@ static void send_relation_and_attrs(Relation relation, TransactionId xid,
static void rel_sync_cache_relation_cb(Datum arg, Oid relid);
static void rel_sync_cache_publication_cb(Datum arg, int cacheid,
uint32 hashvalue);
-static void set_schema_sent_in_streamed_txn(RelationSyncEntry *entry,
+static void set_schema_sent_in_streamed_txn(PGOutputData *data,
+ RelationSyncEntry *entry,
TransactionId xid);
static bool get_schema_sent_in_streamed_txn(RelationSyncEntry *entry,
TransactionId xid);
@@ -748,7 +749,7 @@ maybe_send_schema(LogicalDecodingContext *ctx,
send_relation_and_attrs(relation, xid, ctx, relentry);
if (data->in_streaming)
- set_schema_sent_in_streamed_txn(relentry, topxid);
+ set_schema_sent_in_streamed_txn(data, relentry, topxid);
else
relentry->schema_sent = true;
}
@@ -1197,8 +1198,8 @@ init_tuple_slot(PGOutputData *data, Relation relation,
TupleDesc indesc = RelationGetDescr(relation);
TupleDesc outdesc = RelationGetDescr(ancestor);
- /* Map must live as long as the session does. */
- oldctx = MemoryContextSwitchTo(CacheMemoryContext);
+ /* Map must live as long as the logical decoding context. */
+ oldctx = MemoryContextSwitchTo(data->cachectx);
entry->attrmap = build_attrmap_by_name_if_req(indesc, outdesc, false);
@@ -1991,11 +1992,12 @@ get_schema_sent_in_streamed_txn(RelationSyncEntry *entry, TransactionId xid)
* of the relation.
*/
static void
-set_schema_sent_in_streamed_txn(RelationSyncEntry *entry, TransactionId xid)
+set_schema_sent_in_streamed_txn(PGOutputData *data, RelationSyncEntry *entry,
+ TransactionId xid)
{
MemoryContext oldctx;
- oldctx = MemoryContextSwitchTo(CacheMemoryContext);
+ oldctx = MemoryContextSwitchTo(data->cachectx);
entry->streamed_txns = lappend_xid(entry->streamed_txns, xid);
--
2.30.0.windows.2
view thread (9+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: RE: Memory leak in pg_logical_slot_{get,peek}_changes
In-Reply-To: <OS0PR01MB5716AD5128F16AA1A02B6D29943E2@OS0PR01MB5716.jpnprd01.prod.outlook.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox