On Wed, May 9, 2018 at 5:20 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: > > (2018/04/25 18:51), Ashutosh Bapat wrote: >> Actually I noticed that ConvertRowtypeExpr are used to cast a child's >> whole row reference expression (not just a Var node) into that of its >> parent and not. For example a cast like NULL::child::parent produces a >> ConvertRowtypeExpr encapsulating a NULL constant node and not a Var >> node. We are interested only in ConvertRowtypeExprs embedding Var >> nodes with Var::varattno = 0. I have changed this code to use function >> is_converted_whole_row_reference() instead of the above code with >> Assert. In order to use that function, I had to take it out of >> setrefs.c and put it in nodeFuncs.c which seemed to be a better fit. > > This change seems a bit confusing to me because the flag bits > "PVC_INCLUDE_CONVERTROWTYPES" and "PVC_RECURSE_CONVERTROWTYPES" passed to > pull_var_clause look as if it handles any ConvertRowtypeExpr nodes from a > given clause, but really, it only handles ConvertRowtypeExprs you mentioned > above. To make that function easy to understand and use, I think it'd be > better to use the IsA(node, ConvertRowtypeExpr) test as in the first version > of the patch, instead of is_converted_whole_row_reference, which would be > more consistent with other cases such as PlaceHolderVar.
I agree that using is_converted_whole_row_reference() is not consistent with the other nodes that are handled by pull_var_clause(). I also agree that PVC_*_CONVERTROWTYPES doesn't reflect exactly what's being done with those options. But using is_converted_whole_row_reference() is the correct thing to do since we are interested only in the whole-row references embedded in ConvertRowtypeExpr. There can be anything encapsulated in ConvertRowtypeExpr(), a non-shippable function for example. We don't want to try to push that down in postgres_fdw's case. Neither in other cases. So, it boils down to changing names of PVC_*_CONVERTROWTYPES to something more meaningful. How about PVC_*_CONVERTWHOLEROWS? -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company