Remove unused isCommit parameter from AtEOXact_LocalBuffers
Hi all,
I see the `isCommit` parameter of `AtEOXact_LocalBuffers` is unused:
```
void
AtEOXact_LocalBuffers(bool isCommit)
{
CheckForLocalBufferLeaks();
}
```
Commit 5df307c7782518c4a3c19ffd05c7cb591b97e23c introduced `AtEOXact_LocalBuffers(bool isCommit)` for the following logic:
```
if (isCommit)
elog(WARNING,
"local buffer leak: [%03d] (rel=%u/%u/%u, blockNum=%u, flags=0x%x, refcount=%u %d)",
i,
buf->tag.rnode.spcNode, buf->tag.rnode.dbNode,
buf->tag.rnode.relNode, buf->tag.blockNum, buf->flags,
buf->refcount, LocalRefCount[i]);
```
Commit fdd13f156814f81732c188788ab1b7b14c59f4da removed the above code, but the `isCommit` parameter was left intact.
I intend to remove this unused parameter, and also clean up the call site in `AtEOXact_Buffers` since it invokes `AtEOXact_LocalBuffers`.
The code contains multiple calls to `AtEOXact_Buffers`, some of which pass `false` and others pass `true`, which may be confusing.
Attached is the patch for these changes.
--
regards,
Man Zeng
Attachments:
0001-Refactor-AtEOXact_Buffers-and-AtEOXact_LocalBuffers-.patchapplication/octet-stream; charset=ISO-8859-1; name=0001-Refactor-AtEOXact_Buffers-and-AtEOXact_LocalBuffers-.patchDownload+12-13
"=?ISO-8859-1?B?emVuZ21hbg==?=" <zengman@halodbtech.com> writes:
Commit fdd13f156814f81732c188788ab1b7b14c59f4da removed the above code, but the `isCommit` parameter was left intact.
I intend to remove this unused parameter, and also clean up the call site in `AtEOXact_Buffers` since it invokes `AtEOXact_LocalBuffers`.
I think we should reject this as useless code churn. The parameter
was needed in the past and might be needed again in the future.
It's fairly common for other AtEOXact functions to take an isCommit
flag, so I don't find it surprising for these to have one.
regards, tom lane
On Tue, Feb 03, 2026 at 09:45:49AM -0500, Tom Lane wrote:
I think we should reject this as useless code churn. The parameter
was needed in the past and might be needed again in the future.
It's fairly common for other AtEOXact functions to take an isCommit
flag, so I don't find it surprising for these to have one.
+1
--
nathan
On Tue, Feb 03, 2026 at 09:34:33AM -0600, Nathan Bossart wrote:
On Tue, Feb 03, 2026 at 09:45:49AM -0500, Tom Lane wrote:
I think we should reject this as useless code churn. The parameter
was needed in the past and might be needed again in the future.
It's fairly common for other AtEOXact functions to take an isCommit
flag, so I don't find it surprising for these to have one.+1
+1. Symmetry is usually relevant in the signature of these cleanup
functions.
--
MichaelI think we should reject this as useless code churn. The parameter
was needed in the past and might be needed again in the future.
It's fairly common for other AtEOXact functions to take an isCommit
flag, so I don't find it surprising for these to have one.+1
+1. Symmetry is usually relevant in the signature of these cleanup
functions.
Hi all,
I still want to do this simple cleanup, and I’d like to state my thoughts:
1. It has been a very long time, about more than twenty years, since the `isCommit` parameter of `AtEOXact_LocalBuffers` last functioned, namely commit fdd13f156814f81732c188788ab1b7b14c59f4da.
2. This modification mainly involves `AtEOXact_Buffers`. Judging from the comment, the work undertaken by this function seems simple and stable.
```
/*
* AtEOXact_Buffers - clean up at end of transaction.
*
* As of PostgreSQL 8.0, buffer pins should get released by the
* ResourceOwner mechanism. This routine is just a debugging
* cross-check that no pins remain.
*/
void
AtEOXact_Buffers(bool isCommit)
{
CheckForBufferLeaks();
AtEOXact_LocalBuffers(isCommit);
Assert(PrivateRefCountOverflowed == 0);
}
```
3. Some other AtEOXact functions take no parameters, although this is not a strong argument here.
However, I understand everyone's thoughts and respect your opinions, so I choose to keep my own opinion on this.
--
regards,
Man Zeng