pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedFix a relcache reference leak in reorderbuffer.c
7+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2026-08-19 bdbf662b1b 4a710d6887 ecf2492b44 26a9d8ea64 e8d079313a 402e962444 29b52d2b47 Fix relcache reference leak when decoding TRUNCATE
* Fix a relcache reference leak in reorderbuffer.c
@ 2026-08-18 03:51 Chao Li <li.evan.chao@gmail.com>
2026-08-18 05:55 ` Re: Fix a relcache reference leak in reorderbuffer.c Michael Paquier <michael@paquier.xyz>
2026-08-18 05:59 ` Re: Fix a relcache reference leak in reorderbuffer.c Xuneng Zhou <xunengzhou@gmail.com>
0 siblings, 2 replies; 7+ messages in thread
From: Chao Li @ 2026-08-18 03:51 UTC (permalink / raw)
To: pgsql-hackers
Hi,
While working on a feature patch that I’m going to post soon, I noticed a small issue in reorderbuffer.c. In the REORDER_BUFFER_CHANGE_TRUNCATE branch of ReorderBufferProcessTXN(), some opened relations might be skipped without being closed, leading to leaked relcache references.
The relevant code is:
```
relations = palloc0_array(Relation, nrelids);
for (i = 0; i < nrelids; i++)
{
Oid relid = change->data.truncate.relids[i];
Relation rel;
rel = RelationIdGetRelation(relid);
if (!RelationIsValid(rel))
elog(ERROR, "could not open relation with OID %u", relid);
if (!RelationIsLogicallyLogged(rel))
continue; <===== it should close rel before skipping it
relations[nrelations++] = rel;
}
/* Apply the truncate. */
ReorderBufferApplyTruncate(rb, txn, nrelations,
relations, change,
streaming);
for (i = 0; i < nrelations; i++)
RelationClose(relations[i]);
```
In the loop, each relation that is appended to relations is closed after the loop. However, when RelationIsLogicallyLogged(rel) returns false, the relation is skipped without being closed, causing the leak.
The attached patch makes a small fix to close the relation before continuing.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
v1-0001-Fix-relcache-reference-leak-when-decoding-TRUNCAT.patch (application/octet-stream, 1.2K) download
From 30a53ee7a12f42181615a61e625596f6dd00014c Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <lic@highgo.com>
Date: Tue, 18 Aug 2026 11:37:54 +0800
Subject: [PATCH v1] Fix relcache reference leak when decoding TRUNCATE
ReorderBufferProcessTXN() opens every relation referenced by a
TRUNCATE change. When RelationIsLogicallyLogged() returns false,
it skips the relation without releasing the reference acquired
by RelationIdGetRelation().
Close the relation before continuing.
Author: Chao Li <lic@highgo.com>
---
src/backend/replication/logical/reorderbuffer.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 6aed6346366..b34ec5f2145 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2492,7 +2492,10 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
elog(ERROR, "could not open relation with OID %u", relid);
if (!RelationIsLogicallyLogged(rel))
+ {
+ RelationClose(rel);
continue;
+ }
relations[nrelations++] = rel;
}
--
2.50.1 (Apple Git-155)
=
* Re: Fix a relcache reference leak in reorderbuffer.c
2026-08-18 03:51 Fix a relcache reference leak in reorderbuffer.c Chao Li <li.evan.chao@gmail.com>
@ 2026-08-18 05:55 ` Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 7+ messages in thread
From: Michael Paquier @ 2026-08-18 05:55 UTC (permalink / raw)
To: Chao Li <li.evan.chao@gmail.com>; +Cc: pgsql-hackers
On Tue, Aug 18, 2026 at 11:51:48AM +0800, Chao Li wrote:
> In the loop, each relation that is appended to relations is closed
> after the loop. However, when RelationIsLogicallyLogged(rel) returns
> false, the relation is skipped without being closed, causing the
> leak.
Good catch. This code may over-allocate the set of relations it is
working on, and we would forget to close the once not logged. That's
wrong since 5dfd1e5a6696.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix a relcache reference leak in reorderbuffer.c
2026-08-18 03:51 Fix a relcache reference leak in reorderbuffer.c Chao Li <li.evan.chao@gmail.com>
@ 2026-08-18 05:59 ` Xuneng Zhou <xunengzhou@gmail.com>
2026-08-18 22:35 ` Re: Fix a relcache reference leak in reorderbuffer.c Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 7+ messages in thread
From: Xuneng Zhou @ 2026-08-18 05:59 UTC (permalink / raw)
To: Chao Li <li.evan.chao@gmail.com>; +Cc: pgsql-hackers
Hi Evan,
On Tue, Aug 18, 2026 at 11:52 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
> Hi,
>
> While working on a feature patch that I’m going to post soon, I noticed a small issue in reorderbuffer.c. In the REORDER_BUFFER_CHANGE_TRUNCATE branch of ReorderBufferProcessTXN(), some opened relations might be skipped without being closed, leading to leaked relcache references.
>
> The relevant code is:
> ```
> relations = palloc0_array(Relation, nrelids);
> for (i = 0; i < nrelids; i++)
> {
> Oid relid = change->data.truncate.relids[i];
> Relation rel;
>
> rel = RelationIdGetRelation(relid);
>
> if (!RelationIsValid(rel))
> elog(ERROR, "could not open relation with OID %u", relid);
>
> if (!RelationIsLogicallyLogged(rel))
> continue; <===== it should close rel before skipping it
>
> relations[nrelations++] = rel;
> }
>
> /* Apply the truncate. */
> ReorderBufferApplyTruncate(rb, txn, nrelations,
> relations, change,
> streaming);
>
> for (i = 0; i < nrelations; i++)
> RelationClose(relations[i]);
> ```
>
> In the loop, each relation that is appended to relations is closed after the loop. However, when RelationIsLogicallyLogged(rel) returns false, the relation is skipped without being closed, causing the leak.
>
> The attached patch makes a small fix to close the relation before continuing.
I think this is not a session-level *leak*. The resource owner of
ongoing txn would take care of the reference once the
ReorderBufferProcessTXN aborts its internal transaction. Yeah, it
could be more troublesome if the skipped references keep accumulating
until the decoding of txn finishes. That said, it's not very clear to
me whether this code is actually excerised. Can you provide a
reproducer or a test?
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix a relcache reference leak in reorderbuffer.c
2026-08-18 03:51 Fix a relcache reference leak in reorderbuffer.c Chao Li <li.evan.chao@gmail.com>
2026-08-18 05:59 ` Re: Fix a relcache reference leak in reorderbuffer.c Xuneng Zhou <xunengzhou@gmail.com>
@ 2026-08-18 22:35 ` Michael Paquier <michael@paquier.xyz>
2026-08-19 00:16 ` Re: Fix a relcache reference leak in reorderbuffer.c Xuneng Zhou <xunengzhou@gmail.com>
2026-08-19 02:18 ` Re: Fix a relcache reference leak in reorderbuffer.c Chao Li <li.evan.chao@gmail.com>
0 siblings, 2 replies; 7+ messages in thread
From: Michael Paquier @ 2026-08-18 22:35 UTC (permalink / raw)
To: Xuneng Zhou <xunengzhou@gmail.com>; +Cc: Chao Li <li.evan.chao@gmail.com>; pgsql-hackers
On Tue, Aug 18, 2026 at 01:59:01PM +0800, Xuneng Zhou wrote:
> I think this is not a session-level *leak*. The resource owner of
> ongoing txn would take care of the reference once the
> ReorderBufferProcessTXN aborts its internal transaction. Yeah, it
> could be more troublesome if the skipped references keep accumulating
> until the decoding of txn finishes. That said, it's not very clear to
> me whether this code is actually excerised. Can you provide a
> reproducer or a test?
More to the point, looking at code paths producing XLOG_HEAP_TRUNCATE
records in core, we specifically discard any relation that does not
satisfy RelationIsLogicallyLogged().
In short, it is not possible in practice to reach this code at all.
We would do a decoding of the generated record after an historical
snapshot, so something like a SET UNLOGGED should not be seen in
practice. I think that it does not hurt to be defensive in practice
here, still I am wondering about code outside of core that may play it
unfair, even if the consequences are minimal.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix a relcache reference leak in reorderbuffer.c
2026-08-18 03:51 Fix a relcache reference leak in reorderbuffer.c Chao Li <li.evan.chao@gmail.com>
2026-08-18 05:59 ` Re: Fix a relcache reference leak in reorderbuffer.c Xuneng Zhou <xunengzhou@gmail.com>
2026-08-18 22:35 ` Re: Fix a relcache reference leak in reorderbuffer.c Michael Paquier <michael@paquier.xyz>
@ 2026-08-19 00:16 ` Xuneng Zhou <xunengzhou@gmail.com>
1 sibling, 0 replies; 7+ messages in thread
From: Xuneng Zhou @ 2026-08-19 00:16 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Chao Li <li.evan.chao@gmail.com>; pgsql-hackers
Hi Michael,
On Wed, Aug 19, 2026 at 6:35 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Aug 18, 2026 at 01:59:01PM +0800, Xuneng Zhou wrote:
> > I think this is not a session-level *leak*. The resource owner of
> > ongoing txn would take care of the reference once the
> > ReorderBufferProcessTXN aborts its internal transaction. Yeah, it
> > could be more troublesome if the skipped references keep accumulating
> > until the decoding of txn finishes. That said, it's not very clear to
> > me whether this code is actually excerised. Can you provide a
> > reproducer or a test?
>
> More to the point, looking at code paths producing XLOG_HEAP_TRUNCATE
> records in core, we specifically discard any relation that does not
> satisfy RelationIsLogicallyLogged().
>
> In short, it is not possible in practice to reach this code at all.
> We would do a decoding of the generated record after an historical
> snapshot, so something like a SET UNLOGGED should not be seen in
> practice.
Thanks for clarifying and confirming it.
I think that it does not hurt to be defensive in practice
> here, still I am wondering about code outside of core that may play it
> unfair, even if the consequences are minimal.
Yeah, I agree that this code still seems fragile and better to be
harnessed. The code of patch v1 LGTM.
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix a relcache reference leak in reorderbuffer.c
2026-08-18 03:51 Fix a relcache reference leak in reorderbuffer.c Chao Li <li.evan.chao@gmail.com>
2026-08-18 05:59 ` Re: Fix a relcache reference leak in reorderbuffer.c Xuneng Zhou <xunengzhou@gmail.com>
2026-08-18 22:35 ` Re: Fix a relcache reference leak in reorderbuffer.c Michael Paquier <michael@paquier.xyz>
@ 2026-08-19 02:18 ` Chao Li <li.evan.chao@gmail.com>
2026-08-19 02:34 ` Re: Fix a relcache reference leak in reorderbuffer.c Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 7+ messages in thread
From: Chao Li @ 2026-08-19 02:18 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Xuneng Zhou <xunengzhou@gmail.com>; pgsql-hackers
> On Aug 19, 2026, at 06:35, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Aug 18, 2026 at 01:59:01PM +0800, Xuneng Zhou wrote:
>> I think this is not a session-level *leak*. The resource owner of
>> ongoing txn would take care of the reference once the
>> ReorderBufferProcessTXN aborts its internal transaction. Yeah, it
>> could be more troublesome if the skipped references keep accumulating
>> until the decoding of txn finishes. That said, it's not very clear to
>> me whether this code is actually excerised. Can you provide a
>> reproducer or a test?
>
> More to the point, looking at code paths producing XLOG_HEAP_TRUNCATE
> records in core, we specifically discard any relation that does not
> satisfy RelationIsLogicallyLogged().
>
> In short, it is not possible in practice to reach this code at all.
> We would do a decoding of the generated record after an historical
> snapshot, so something like a SET UNLOGGED should not be seen in
> practice. I think that it does not hurt to be defensive in practice
> here, still I am wondering about code outside of core that may play it
> unfair, even if the consequences are minimal.
> --
> Michael
I just took a deeper look at the code. If we handle the continue defensively, then in theory nrelations could be 0. We should also defensively skip ReorderBufferApplyTruncate() when nrelations == 0. Looking at a callback function, for example, pgoutput_truncate() in src/backend/replication/pgoutput/pgoutput.c appears to assume that nrelations > 0, it doesn't specially handle nrelations == 0.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix a relcache reference leak in reorderbuffer.c
2026-08-18 03:51 Fix a relcache reference leak in reorderbuffer.c Chao Li <li.evan.chao@gmail.com>
2026-08-18 05:59 ` Re: Fix a relcache reference leak in reorderbuffer.c Xuneng Zhou <xunengzhou@gmail.com>
2026-08-18 22:35 ` Re: Fix a relcache reference leak in reorderbuffer.c Michael Paquier <michael@paquier.xyz>
2026-08-19 02:18 ` Re: Fix a relcache reference leak in reorderbuffer.c Chao Li <li.evan.chao@gmail.com>
@ 2026-08-19 02:34 ` Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 7+ messages in thread
From: Michael Paquier @ 2026-08-19 02:34 UTC (permalink / raw)
To: Chao Li <li.evan.chao@gmail.com>; +Cc: Xuneng Zhou <xunengzhou@gmail.com>; pgsql-hackers
On Wed, Aug 19, 2026 at 10:18:53AM +0800, Chao Li wrote:
> I just took a deeper look at the code. If we handle the continue
> defensively, then in theory nrelations could be 0. We should also
> defensively skip ReorderBufferApplyTruncate() when nrelations ==
> 0. Looking at a callback function, for example, pgoutput_truncate()
> in src/backend/replication/pgoutput/pgoutput.c appears to assume
> that nrelations > 0, it doesn't specially handle nrelations == 0.
Right. We tend to be careful in terms of the closes in
reorderbuffer.c anyway, so I've just applied the patch.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2026-08-19 02:34 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-08-18 03:51 Fix a relcache reference leak in reorderbuffer.c Chao Li <li.evan.chao@gmail.com>
2026-08-18 05:55 ` Michael Paquier <michael@paquier.xyz>
2026-08-18 05:59 ` Xuneng Zhou <xunengzhou@gmail.com>
2026-08-18 22:35 ` Michael Paquier <michael@paquier.xyz>
2026-08-19 00:16 ` Xuneng Zhou <xunengzhou@gmail.com>
2026-08-19 02:18 ` Chao Li <li.evan.chao@gmail.com>
2026-08-19 02:34 ` Michael Paquier <michael@paquier.xyz>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox