Re: Compute attr_needed for child relations (was Re: inherit support for foreign tables)

Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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