Re: Expression errors with "FOR UPDATE" and postgres_fdw with partition wise join enabled.

(2018/05/08 16:20), Ashutosh Bapat wrote:
> On Tue, May 1, 2018 at 5:00 PM, Etsuro Fujita
> <fujita.etsuro@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