[ not a review, just some drive-by comments on David's comments ]
David Rowley <dgrowleyml@gmail.com> writes:
> 2. The following change does not seem like it should be part of this
> patch. I understand you perhaps have done as you think it will
> improve the performance of checking if an expression is in a list of
> expressions.
> - COMPARE_SCALAR_FIELD(varno);
> + /* Compare varattno first since it has higher selectivity than varno */
> COMPARE_SCALAR_FIELD(varattno);
> + COMPARE_SCALAR_FIELD(varno);
> If you think that is true, then please do it as a separate effort and
> provide benchmarks with your findings.
By and large, I'd reject such micro-optimizations on their face.
The rule in the nodes/ support files is to list fields in the same
order they're declared in. There is no chance that it's worth
deviating from that for this.
I can believe that there'd be value in, say, comparing all
scalar fields before all non-scalar ones. But piecemeal hacks
wouldn't be the way to handle that either. In any case, I'd
prefer to implement such a plan within the infrastructure to
auto-generate these files that Andres keeps muttering about.
> a. including a function call in the foreach macro is not a practise
> that we really follow. It's true that the macro now assigns the 2nd
> param to a variable. Previous to 1cff1b95ab6 this was not the case and
> it's likely best not to leave any bad examples around that code which
> might get backported might follow.
No, I think you're misremembering. foreach's second arg is
single-evaluation in all branches. There were some preliminary
versions of 1cff1b95ab6 in which it would not have been, but that
was sufficiently dangerous that I found a way to get rid of it.
> b. We generally subtract InvalidAttrNumber from varattno when
> including in a Bitmapset.
ITYM FirstLowInvalidHeapAttributeNumber, but yeah. Otherwise
the code fails on system columns, and there's seldom a good
reason to risk that.
regards, tom lane
