On Fri, May 26, 2023 at 2:49 PM Andres Freund <andres@anarazel.de> wrote:
> What do we gain by not passing down the heap relation to those places?
Just clearer code, free from noisey changes. Easier when backpatching, too.
> If you're concerned about the efficiency of passing down the parameters,
> I doubt it will make a meaningful difference, because the parameter can
> just stay in the register to be passed down further.
I'm not concerned about the efficiency of passing down heapRel in so
many places.
As things stand, there is no suggestion that any _bt_getbuf() call is
exempt from the requirement to pass down a heaprel -- every caller
(internal and external) goes to the trouble of making sure that they
comply with the apparent requirement to supply a heapRel. In some
cases callers do so just to be able to read the metapage. Even code as
far removed from nbtree as heapam_relation_copy_for_cluster() will now
go to the trouble of passing its own heap rel, just to perform a
CLUSTER-based tuplesort. The relevant tuplesort call site even has
comments that try to justify this approach, with a reference to
_bt_log_reuse_page(). So heapam_handler.c now references a static
helper function private to nbtpage.c -- an obvious modularity
violation.
It's not even the modularity violation itself that bothers me. It's
just 100% unnecessary for heapam_relation_copy_for_cluster() to do any
of this, because there simply isn't going to be a call to
_bt_log_reuse_page() during its cluster tuplesort, no matter what.
This has nothing to do with any underlying implementation detail from
nbtree, or from any other index AM.
--
Peter Geoghegan