On Thu, May 17, 2018 at 4:50 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: > (2018/05/16 22:49), Ashutosh Bapat wrote: >> >> On Wed, May 16, 2018 at 4:01 PM, Etsuro Fujita >> <fujita.ets...@lab.ntt.co.jp> wrote: >>> >>> However, considering that >>> pull_var_clause is used in many places where partitioning is *not* >>> involved, >>> I still think it's better to avoid spending extra cycles needed only for >>> partitioning in that function. >> >> >> Right. The changes done in inheritance_planner() imply that we can >> encounter a ConvertRowtypeExpr even in the expressions for a relation >> which is not a child relation in the translated query. It was a child >> in the original query but after translating it becomes a parent and >> has ConvertRowtypeExpr somewhere there. > > > Sorry, I don't understand this. Could you show me such an example?
Even without inheritance we can induce a ConvertRowtypeExpr on a base relation which is not "other" relation create table parent (a int, b int); create table child () inherits(parent); alter table child add constraint whole_par_const check ((child::parent).a = 1); There is no way to tell by looking at the parsed query whether pull_var_clause() from StoreRelCheck() will encounter a ConvertRowtypeExpr or not. We could check whether the tables involved are inherited or not, but that's more expensive than is_converted_whole_row_reference(). > >> So, there is no way to know >> whether a given expression will have ConvertRowtypeExpr in it. That's >> my understanding. But if we can device such a test, I am happy to >> apply that test before or witin the call to pull_var_clause(). >> >> We don't need that expensive test if user has passed >> PVC_RECURSE_CONVERTROWTYPE. So we could test that first and then check >> the shape of expression tree. It would cause more asymmetry in >> pull_var_clause(), but we can live with it or change the order of >> tests for all the three options. I will differ that to a committer. > > > Sounds more invasive. Considering where we are in the development cycle for > PG11, I think it'd be better to address this by something like the approach > I proposed. Anyway, +1 for asking the committer's opinion. Thanks. > > - On patch 0001-Handle-ConvertRowtypeExprs-in-pull_vars_clause.patch: > > +extern bool > +is_converted_whole_row_reference(Node *node) > > I think we should remove "extern" from the definition. Done. > > - On patch 0003-postgres_fdw-child-join-with-ConvertRowtypeExprs-cau.patch: > > > I think we can fix this by adding another flag to indicate whether we > deparsed the first live column of the relation, as in the "first" bool flag > in deparseTargetList. Thanks. Fixed. > > One more thing to add is: the patch adds support for deparsing a > ConvertRowtypeExpr that translates a wholerow of a childrel into a wholerow > of its parentrel's rowtype, so by modifying is_foreign_expr accordingly, we > could push down such CREs in JOIN ON conditions to the remote for example. > But that would be an improvement, not a fix, so I think we should leave that > for another patch for PG12. Right. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Description: GNU Zip compressed data