(2018/05/08 16:20), Ashutosh Bapat wrote:
On Tue, May 1, 2018 at 5:00 PM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp> wrote:
Here are my review comments on patch
0003-postgres_fdw-child-join-with-ConvertRowtypeExprs-cau.patch:
* This assertion in deparseConvertRowtypeExpr wouldn't always be true
because of cases like ALTER FOREIGN TABLE DROP COLUMN plus ALTER FOREIGN
TABLE ADD COLUMN:
+ Assert(parent_desc->natts == child_desc->natts);
I think we should remove that assertion.
Removed.
* But I don't think that change is enough. Here is another oddity with that
assertion removed.
To fix this, I think we should skip the deparseColumnRef processing for
dropped columns in the parent table.
Done.
Thanks for those changes!
I have changed the test file a bit to test these scenarios.
+1
* I think it would be better to do EXPLAIN with the VERBOSE option to the
queries this patch added to the regression tests, to see if
ConvertRowtypeExprs are correctly deparsed in the remote queries.
The reason VERBOSE option was omitted for partition-wise join was to
avoid repeated and excessive output for every child-join. I think that
still applies.
I'd like to leave that for the committer's judge.
PFA patch-set with the fixes.
Thanks for updating the patch!
I also noticed that the new function deparseConvertRowtypeExpr is not
quite following the naming convention of the other deparseFoo
functions. Foo is usually the type of the node the parser would
produced when the SQL string produced by that function is parsed. That
doesn't hold for the SQL string produced by ConvertRowtypeExpr but
then naming it as generic deparseRowExpr() wouldn't be correct either.
And then there are functions like deparseColumnRef which may produce a
variety of SQL strings which get parsed into different node types e.g.
a whole-row reference would produce ROW(...) which gets parsed as a
RowExpr. Please let me know if you have any suggestions for the name.
To be honest, I don't have any strong opinion about that. But I like
"deparseConvertRowtypeExpr" because that name seems to well represent
the functionality, so I'd vote for that.
BTW, one thing I'm a bit concerned about is this:
(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.
Best regards,
Etsuro Fujita
