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.
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.
