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(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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