Nils Dijk <me@thanod.nl> writes:
> Attached you will find 3 patches that implement a way for extensions
> to introduce 'a figure of merit' by the means of path comparisons.
I took a brief look through this. I quite like your idea of expressing
PathComparison merging as an OR of suitably-chosen values. I do have
some minor criticisms of the patch, which potentially make for small
performance improvements so I've not bothered yet to try to measure
performance.
* I think you could do without PATH_COMPARISON_MASK. Use of the enum
already implies that we only allow valid values of the enum, and given
that the inputs of path_comparison_combine are valid, so is the output
of the "|". There's no need to expend cycles masking it, and if there
were it would be dubious whether the masking restores correctness.
What *is* needed, though, is a comment pointing out that the values of
PathComparison are chosen with malice aforethought to cause ORing of
them to give semantically-correct results.
* I'd also drop enum AddPathDecision and add_path_decision(),
and just let add_path() do what it must based on the PathComparison
result. I don't think the extra layer of mapping adds anything,
and it's probably costing some cycles.
* Perhaps it's worth explicitly marking the new small functions
as "static inline"? Probably modern compilers will do that without
being prompted, but we might as well be clear about what we want.
* Some more attention to comments is needed, eg the header comment
for compare_path_costs_fuzzily still refers to COSTS_DIFFERENT.
(However, on the whole I'm not sure s/COSTS_DIFFERENT/PATHS_DIFFERENT/
etc is an improvement. Somebody looking at this code for the first
time would probably think the answer should always be that the two
paths are "different", because one would hope we're not generating
redundant identical paths. What we want to say is that the paths'
figures of merit are different; but "PATH_FIGURES_OF_MERIT_DIFFERENT"
is way too verbose. Unless somebody has got a good proposal for
a short name I'd lean to sticking with the COSTS_XXX names.)
regards, tom lane