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

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