Re: Proposed refactoring of planner header files

On Thu, Jan 31, 2019 at 2:46 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I'm happy to do the work if there's consensus on what to do. After
> a few moments' thought, I'd suggest:
>
> 1. Move the indexscan cost estimation functions into a new file
> adt/index_selfuncs.c. Most of them already are declared in
> utils/index_selfuncs.h, and we'd move the remaining declarations
> (primarily, genericcostestimate()) to that file as well. This
> would affect #includes in contrib/bloom/ and any 3rd-party index
> AMs that might be using genericcostestimate().
>
> 2a. Leave the support functions in selfuncs.c with declarations in
> utils/selfuncs.h, and move the operator-specific estimators to
> a new file, perhaps opr_selfuncs.c. This would minimize pain for
> existing includers of selfuncs.h, most of which (particularly outside
> core) are presumably interested in the support functions.
>
> 2b. Alternatively, leave the operator-specific estimators where they
> are, and make new files under optimizer/ for the support functions.

I do not have a powerful opinion on exactly what to do here, but I
offer the following for what it's worth:

- I do not really think much of the selfuncs.c naming. Nobody who is
not deeply immersed in the PostgreSQL code knows what a "selfunc" is.
Therefore, breaking selfuncs.c into opr_selfuncs.c and
index_selfuncs.c doesn't strike me as the ideal naming choice. I
would suggest looking for a name that is less steeped in venerable
tradition; perhaps estimator.c or indexsupport.c or selectivity.c or
whatever could be considered for whatever new files we are creating.

- I have always found the placement of selfuncs.c a bit strange: why
is it in utils/adt? Perhaps there is an argument for having the
things that are specific to a particular type or a particular operator
in that directory, but if so, shouldn't they be grouped with the type
or operator to which they relate, rather than each other? And if
they're not specific to a certain thing, or if grouping them with that
thing would suck because of what we'd have to #include or some other
grounds, then why not put them in the optimizer? I think that a large
fraction of this code is actually generic, and putting it in the
optimizer directory someplace would be more logical than what we have
now. Alternatively, since these have to be exposed via pg_proc, maybe
the shims should go in this directory and the core logic elsewhere.
Again, I don't have a strong opinion here, but I've never thought this
made a ton of sense the way it is.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company