Fix a relcache reference leak in reorderbuffer.c

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

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.

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

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.

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/

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