On Tue, May 1, 2018 at 5:00 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> (2018/04/27 14:40), Ashutosh Bapat wrote:
>>
>> Here's updated patch set.
>
>
> Thanks for updating the patch! 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);
>
> Here is such an example:
>
> 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.
I have changed the test file a bit to test these scenarios.
Thanks for testing.
>
> * 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.
PFA patch-set with the fixes.
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.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
