Thread: _pg_relbuf() Relation paramter
I ran across this function in nbtpage.c:
/** _bt_relbuf() -- release a locked buffer.** Lock and pin (refcount) are both dropped.*/
void
_bt_relbuf(Relation rel, Buffer buf)
{ UnlockReleaseBuffer(buf);
}
Would anyone object to me removing the first parameter (including,
obviously, in all references in our code tree)?
If there is an objection, could you suggest something for the
comment block to explain why it is there?
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 02/04/2015 04:49 PM, Kevin Grittner wrote:
> I ran across this function in nbtpage.c:
>
> /*
> * _bt_relbuf() -- release a locked buffer.
> *
> * Lock and pin (refcount) are both dropped.
> */
> void
> _bt_relbuf(Relation rel, Buffer buf)
> {
> UnlockReleaseBuffer(buf);
> }
>
> Would anyone object to me removing the first parameter (including,
> obviously, in all references in our code tree)?
No objection, although I have to wonder why bother? While you're at it,
the 'size' argument to _bt_pageinit is also pretty useless. It's useless
for PageInit() too, but that's used in more places, potentially even in
extensions. (there's a call to _bt_relbuf in contrib/pgstattuple, but it
shouldn't really be used in 3rd party extensions)
- Heikki
Heikki Linnakangas <hlinnakangas@vmware.com> wrote: > On 02/04/2015 04:49 PM, Kevin Grittner wrote: >> Would anyone object to me removing the first parameter >> (including, obviously, in all references in our code tree)? > > No objection, although I have to wonder why bother? Because I ran across it while trying to work on something and wasted time checking to see what the heck it was doing with that parameter. Having invested that time and confirmed that it was useless, it seemed like it was worth cleaning up to save others the same bother. > While you're at it, the 'size' argument to _bt_pageinit is also > pretty useless. It's useless for PageInit() too, but that's used > in more places, potentially even in extensions. Well, I wasn't out searching for problems like this. I was just going to fix the one I happened to stubbed my toe on; but if you feel these should be changed at the same time, I could do that. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Kevin Grittner <kgrittn@ymail.com> writes:
> I ran across this function in nbtpage.c:
> /*
> * _bt_relbuf() -- release a locked buffer.
> *
> * Lock and pin (refcount) are both dropped.
> */
> void
> _bt_relbuf(Relation rel, Buffer buf)
> {
> UnlockReleaseBuffer(buf);
> }
> Would anyone object to me removing the first parameter (including,
> obviously, in all references in our code tree)?
-1. It's there first for symmetry with the buffer-acquiring calls,
and second because it's not impossible that the state of the index
might affect what we need to do. (It looks like back in Postgres95
the rel argument was actually needed for functionality, which it
isn't today; but we left it there on those grounds.)
If you have some evidence that this is a live performance issue, then
the thing to do would be to remove this function altogether in favor
of direct calls to UnlockReleaseBuffer, or possibly convert it into a
macro that does that. But I'm not in favor of destroying this level
of abstraction (and creating a significant stumbling block for future
back-patches in nbtree, because there are dozens of calls to this)
without some evidence that we'd be buying something meaningful.
regards, tom lane
Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> ... (there's a call to _bt_relbuf in contrib/pgstattuple, but it
> shouldn't really be used in 3rd party extensions)
Meh. I wouldn't say that. I agree that the coding in pgstat_btree_page
pretty much sucks, but on grounds of lack of consistency rather than that
this shouldn't be considered an exported function. I've not checked the
commit history, but I bet what happened is that that function originally
used _bt_getbuf and _bt_relbuf, which I would say is perfectly appropriate
for something touching pages of a btree index. Then somebody decided they
wanted to make use of a BufferAccessStrategy, so they drilled down through
the _bt_getbuf abstraction layer, but they didn't drill down through
_bt_relbuf at the same time. Which is inconsistent, and could even be the
source of a bug in future if we ever made _bt_getbuf/_bt_relbuf do things
differently than they do now.
So I'd definitely be in favor of replacing pgstat_btree_page's use of it
with a direct call on UnlockReleaseBuffer; and for that matter, since it's
abandoned reliance on nbtree's buffer access infrastructure, it should
not be using the BT_READ macro either. But changing nbtree's own internal
coding patterns is a different question.
regards, tom lane