Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
> [ attr_needed-v4.patch ]
I looked this over, and TBH I'm rather disappointed. The patch adds
150 lines of dubiously-correct code in order to save ... uh, well,
actually it *adds* code, because the places that are supposedly getting
a benefit are changed like this:
*** 799,806 **** check_selective_binary_conversion(RelOptInfo *baserel, } /* Collect all the attributes needed
forjoins or final output. */
! pull_varattnos((Node *) baserel->reltargetlist, baserel->relid,
! &attrs_used); /* Add all the attributes used by restriction clauses. */ foreach(lc,
baserel->baserestrictinfo)
--- 799,810 ---- } /* Collect all the attributes needed for joins or final output. */
! for (i = baserel->min_attr; i <= baserel->max_attr; i++)
! {
! if (!bms_is_empty(baserel->attr_needed[i - baserel->min_attr]))
! attrs_used = bms_add_member(attrs_used,
! i - FirstLowInvalidHeapAttributeNumber);
! } /* Add all the attributes used by restriction clauses. */ foreach(lc, baserel->baserestrictinfo)
That's not simpler, it's not easier to understand, and it's probably not
faster either. We could address some of those complaints by encapsulating
the above loop into a utility function, but still, I come away with the
feeling that it's not worth changing this. Considering that all the
places that are doing this then proceed to use pull_varattnos to add on
attnos from the restriction clauses, it seems like using pull_varattnos
on the reltargetlist isn't such a bad thing after all.
So I'm inclined to reject this. It seemed like a good idea in the
abstract, but the concrete result isn't very attractive, and doesn't
seem like an improvement over what we have.
regards, tom lane
