>
> Yeah, I modified the patch so, as I thought that would be consistent with
> the aggregate pushdown patch.
aggregate pushdown needs the tlist to judge the shippability of
targetlist. For joins that's not required, so we should defer, if we
can.
>
>>> Instead we should calculate tlist, the
>>> first time we need it and then add it to the fpinfo.
>
>
> Having said that, I agree on that point. I'd like to propose (1) adding a
> new member to fpinfo, to store a list of output Vars from the subquery, and
> (2) creating a tlist from it in deparseRangeTblRef, then, which would allow
> us to calculate the tlist only when we need it. The member added to fpinfo
> would be also useful to address the comments on the DML/UPDATE pushdown
> patch. See the attached patch in [1]. I named the member a bit differently
> in that patch, though.
Again the list of Vars will be wasted if we don't choose that path for
final planning. So, I don't see the point of adding list of Vars
there. It looks like we are replacing one list with the other when
none of those are useful, if the path doesn't get chosen for the final
plan. If you think that the member is useful for DML/UDPATE pushdown,
you may want to add it in the other patch.
>
> You modified the comments I added to deparseLockingClause into this:
>
> /*
> + * Ignore relation if it appears in a lower subquery. Locking clause
> + * for such a relation is included in the subquery.
> + */
>
> I don't think the second sentence is 100% correct because a locking clause
> isn't always required for such a relation, so I modified the sentence a bit.
>
I guess, "if required" part was implicit in construct "such a
relation". Your version seems to make it explicit. I am fine with it.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
