[ not a review, just some drive-by comments on David's comments ] David Rowley <dgrowle...@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