Etsuro Fujita <fujita.ets...@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 for joins 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
