Re: Proposed refactoring of planner header files

[ moving this discussion to a more appropriate thread; let's keep the
original thread for discussing whether bloom actually needs fixed ]

Over in
https://www.postgresql.org/message-id/CAMkU=1yHfC+Gu84UFsz4hWn=C7tgQFMLiEQcto1Y-8WDE96vaw@mail.gmail.com

Jeff Janes <jeff(dot)janes(at)gmail(dot)com> writes:
> On Tue, Feb 12, 2019 at 4:17 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I'm just in the midst of refactoring that stuff, so if you have
>> suggestions, let's hear 'em.

> The goal would be that I can copy the entire definition of
> genericcostestimate into blcost.c, change the function's name, and get it
> to compile. I don't know the correct way accomplish that.

The private functions that genericcostestimate needs are
get_index_quals, add_predicate_to_quals, other_operands_eval_cost,
and orderby_operands_eval_cost. I don't mind exporting those,
although they should get names more appropriate to being global,
and I think the latter two should be merged, as attached.

Another thing that I've been thinking about here is that
deconstruct_indexquals() and the IndexQualInfo struct probably ought
to go away. That was useful code given our previous convoluted data
structure for index quals, but now its purposes of hiding clause
commutation and associating the right index column with each clause
are vestigial. The other goal that I originally had in inventing
that code was to allow cost estimation methods to disregard the
details of which clause type each indexqual actually is. But looking
at how it's being used, it's generally not the case that callers get
to ignore that, which is unsurprising when you think about it: there's
enough behavioral difference between say OpExpr and ScalarArrayOpExpr
that it's kinda silly to think we could really paper it over.

So the attached patch includes a proof-of-concept for getting rid of
IndexQualInfo. I didn't carry it to completion: gincostestimate is
still using that struct. The thing that was blocking me was that
given the definition that IndexClause.indexquals is NIL if we're
supposed to use the original clause as-is, it's really hard to write
code that processes all the indexquals without duplicating logic.
That is, you tend to end up with something along the lines of
this, inside a loop over the IndexClauses:

if (iclause->indexquals == NIL)
do_something_with(iclause->rinfo);
else
foreach(lc, iclause->indexquals)
do_something_with(lfirst(lc));

If you look at deconstruct_indexquals you'll see that I got around
that by making a subroutine for do_something_with, but it's not
convenient to do that if the code needs access to local variables
in the surrounding function. In btcostestimate I instead did

if (iclause->indexquals)
indexquals = iclause->indexquals;
else
indexquals = list_make1(iclause->rinfo);

foreach(lc, indexquals)
do_something_with(lfirst(lc));

but I don't like that much either. It'd be better I think if
we changed the definition of IndexClause.indexquals to always
be nonempty, and just be a singleton list of the original
iclause->rinfo in the simple case. I'd rejected that approach
to begin with on the grounds that (a) letting it be NIL saves
a list_make1() in the common case, and (b) it seemed a bit
dodgy to have the original clause doubly-linked in this structure.
But argument (a) falls apart completely if places like btcostestimate
are doing their own list_make1 because the data structure is too
hard to work with. And I don't think (b) holds much water either,
since we doubly-link RestrictInfos all over the place.

So I'm thinking about going and changing the definition of that
data structure before it's set in stone.

Comments? Does anyone know of third-party AMs that are using
IndexQualInfo and would be sad to have it go away?

regards, tom lane