nbtree: Cache operator family OID in _bt_setup_array_cmp
Hi Hackers,
While reviewing a separate patch related to nbtree, I noticed
that _bt_setup_array_cmp in nbtpreprocesskeys.c performs multiple redundant
lookups of the operator family OID via rel->rd_opfamily[skey->sk_attno - 1].
The attached patch caches this value in a local opfamily variable. This
matches the pattern used in several other functions within the same file
(such as _bt_skiparray_strat_decrement, _bt_preprocess_array_keys, etc.),
making the code more consistent and readable.
The assignment is placed after the early-return check for (elemtype ==
opcintype) to ensure we only perform the array indexing when the cross-type
comparison logic is actually reached.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
v1-0001-nbtree-Cache-operator-family-OID-in-_bt_setup_arr.patchapplication/octet-stream; name=v1-0001-nbtree-Cache-operator-family-OID-in-_bt_setup_arr.patchDownload+5-5
On Wed, 31 Dec 2025 at 13:16, Chao Li <li.evan.chao@gmail.com> wrote:
Hi Hackers,
While reviewing a separate patch related to nbtree, I noticed that _bt_setup_array_cmp in nbtpreprocesskeys.c performs multiple redundant lookups of the operator family OID via rel->rd_opfamily[skey->sk_attno - 1].
The attached patch caches this value in a local opfamily variable. This matches the pattern used in several other functions within the same file (such as _bt_skiparray_strat_decrement, _bt_preprocess_array_keys, etc.), making the code more consistent and readable.
The assignment is placed after the early-return check for (elemtype == opcintype) to ensure we only perform the array indexing when the cross-type comparison logic is actually reached.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Hi!
We do not cache anything here, this is just a refactoring for nbtree internals?
--
Best regards,
Kirill Reshke
On Dec 31, 2025, at 18:04, Kirill Reshke <reshkekirill@gmail.com> wrote:
On Wed, 31 Dec 2025 at 13:16, Chao Li <li.evan.chao@gmail.com> wrote:
Hi Hackers,
While reviewing a separate patch related to nbtree, I noticed that _bt_setup_array_cmp in nbtpreprocesskeys.c performs multiple redundant lookups of the operator family OID via rel->rd_opfamily[skey->sk_attno - 1].
The attached patch caches this value in a local opfamily variable. This matches the pattern used in several other functions within the same file (such as _bt_skiparray_strat_decrement, _bt_preprocess_array_keys, etc.), making the code more consistent and readable.
The assignment is placed after the early-return check for (elemtype == opcintype) to ensure we only perform the array indexing when the cross-type comparison logic is actually reached.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/Hi!
We do not cache anything here, this is just a refactoring for nbtree internals?
Yes, just cache rel->rd_opfamily[skey->sk_attno - 1] into a local variable.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On 31.12.25 11:13, Chao Li wrote:
On Dec 31, 2025, at 18:04, Kirill Reshke <reshkekirill@gmail.com> wrote:
On Wed, 31 Dec 2025 at 13:16, Chao Li <li.evan.chao@gmail.com> wrote:
Hi Hackers,
While reviewing a separate patch related to nbtree, I noticed that _bt_setup_array_cmp in nbtpreprocesskeys.c performs multiple redundant lookups of the operator family OID via rel->rd_opfamily[skey->sk_attno - 1].
The attached patch caches this value in a local opfamily variable. This matches the pattern used in several other functions within the same file (such as _bt_skiparray_strat_decrement, _bt_preprocess_array_keys, etc.), making the code more consistent and readable.
The assignment is placed after the early-return check for (elemtype == opcintype) to ensure we only perform the array indexing when the cross-type comparison logic is actually reached.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/Hi!
We do not cache anything here, this is just a refactoring for nbtree internals?Yes, just cache rel->rd_opfamily[skey->sk_attno - 1] into a local variable.
But that's not caching, at least in a compiled language with an
optimizing compiler.
Are you claiming that this has a performance impact, or that it makes
the code easier to understand, or something else? The patch isn't
necessarily wrong, but a clear description of the motivation would be good.
On Wed, Jan 7, 2026 at 9:45 AM Peter Eisentraut <peter@eisentraut.org> wrote:
Are you claiming that this has a performance impact, or that it makes
the code easier to understand, or something else? The patch isn't
necessarily wrong, but a clear description of the motivation would be good.
I've noticed that making a local copy of a variable can sometimes help
in tight inner loops, by avoiding an aliasing issue. But it is only
something I've seen help in extreme, rare cases.
--
Peter Geoghegan
On Jan 7, 2026, at 22:45, Peter Eisentraut <peter@eisentraut.org> wrote:
On 31.12.25 11:13, Chao Li wrote:
On Dec 31, 2025, at 18:04, Kirill Reshke <reshkekirill@gmail.com> wrote:
On Wed, 31 Dec 2025 at 13:16, Chao Li <li.evan.chao@gmail.com> wrote:
Hi Hackers,
While reviewing a separate patch related to nbtree, I noticed that
_bt_setup_array_cmp in nbtpreprocesskeys.c performs multiple redundant
lookups of the operator family OID via rel->rd_opfamily[skey->sk_attno - 1].
The attached patch caches this value in a local opfamily variable. This
matches the pattern used in several other functions within the same file
(such as _bt_skiparray_strat_decrement, _bt_preprocess_array_keys, etc.),
making the code more consistent and readable.
The assignment is placed after the early-return check for (elemtype ==
opcintype) to ensure we only perform the array indexing when the cross-type
comparison logic is actually reached.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Hi!
We do not cache anything here, this is just a refactoring for nbtree
internals?
Yes, just cache rel->rd_opfamily[skey->sk_attno - 1] into a local variable.
But that's not caching, at least in a compiled language with an optimizing
compiler.
Are you claiming that this has a performance impact, or that it makes the
code easier to understand, or something else? The patch isn't necessarily
wrong, but a clear description of the motivation would be good.
Fair point -- thanks for the clarification. You’re right, I didn’t mean to
imply any measurable performance benefit here.
The motivation is readability and local consistency rather than caching in
an optimization sense. In nbtpreprocesskeys.c, several nearby helper
functions already assign rel->rd_opfamily[skey->sk_attno - 1] to a local
variable and then use that for opfamily lookups. This change brings
_bt_setup_array_cmp() in line with that existing pattern and avoids
repeating the same expression, which I consider a small improvement to
readability and maintainability.
I have updated the commit message to remove the efficiency language and
focus on clarity and consistency instead. Please see attached v2.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/