On Tue, May 1, 2018 at 5:00 PM, Etsuro Fujita <fujita.ets...@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
expr_ref_error_pwj_v5.tar.gz
Description: GNU Zip compressed data
