Re: Proposed refactoring of planner header files

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Jan 30, 2019 at 10:08 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> However ... there are three pretty clearly identifiable groups of
>> functions in selfuncs.c: operator-specific estimators, support
>> functions, and AM-specific indexscan cost estimation functions.
>> There's a case to be made for splitting them into three separate files.

> I think splitting it along these lines would be pretty sensible. I'm
> not really a fan of giant source files, especially if there's no
> really good reason to put all that stuff in one source file ... then
> again, I'm also not volunteering to do the work.

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 think 2b would make more sense in the abstract, but it would also
result in more #include churn for extensions. OTOH we already
forced some churn in that area with the planner-header-refactoring
patch, so the delta cost is probably not that much.

In any case, I'm hoping to get rid of the exported declarations for
pattern selectivity (Pattern_Prefix_Status, pattern_fixed_prefix,
etc) altogether. Those are exported because indxpath.c currently
calls them from its wired-in lossy-index-clause logic. When the
dust settles, all that code should be associated with planner
support functions for the LIKE and regex operators, and will not
need to be visible outside of whatever module we put those in.
But that patch is yet to be written.

Opinions?

regards, tom lane