On Mon, May 14, 2018 at 10:21 AM, Ashutosh Bapat <ashutosh.ba...@enterprisedb.com> wrote: > On Fri, May 11, 2018 at 6:31 PM, Etsuro Fujita > <fujita.ets...@lab.ntt.co.jp> wrote: >> >> I guess you forgot to show the query. > > Sorry. Here's the query. > > explain verbose select * from fprt1 t1 join fprt2 t2 on t1.a = t2.b > where t1 = row_as_is(row(t2.a, t2.b, t2.c)::ftprt2_p1)::fprt2; > >> >> Yet yet another case where pull_var_clause produces an error: >> >> postgres=# create table list_pt (a int, b text) partition by list (a); >> CREATE TABLE >> postgres=# create table list_ptp1 partition of list_pt for values in (1); >> CREATE TABLE >> postgres=# alter table list_ptp1 add constraint list_ptp1_check check >> (list_ptp1::list_pt::text != NULL); >> ERROR: ConvertRowtypeExpr found where not expected >> >> The patch kept the flags passed to pull_var_clause in >> src/backend/catalog/heap.c as-is, but I think we need to modify that >> accordingly. Probably, the flags passed to pull_var_clause in CreateTrigger >> as well. > > With all the support that we have added in partitioning for v11, I > think we need to add PVC_RECURSE_CONVERTROWTYPE at all the places, > which were left unchanged in the earlier versions of patches, which > were written when all that support wasn't in v11. > >> >> Having said that, I started to think this approach would make code much >> complicated. Another idea I came up with to avoid that would be to use >> pull_var_clause as-is and then rewrite wholerows of partitions back to >> ConvertRowtypeExprs translating those wholerows to wholerows of their >> parents tables' rowtypes if necessary. That would be annoying and a little >> bit inefficient, but the places where we need to rewrite is limited: >> prepare_sort_from_pathkeys and build_tlist_to_deparse, IIUC. > > Other reason why we can't use your approach is we can not decide > whether ConvertRowtypeExpr is needed by just looking at the Var node. > You might say, that we add a ConvertRowtypeExpr if the Var::varno > references a child relation. But that's not true. For example a whole > row reference embedded in ConvertRowtypeExpr added by query > adjustments in inheritance_planner() is not a child's whole-row > reference in the adjusted query. So, it's not clear to me when we add > a ConvertRowtypeExpr and we don't. I am not sure if those are the only > two places which require a fix. Right now it looks like those are the > places which need PVC_INCLUDE_CONVERTROWTYPE, but that may not be true > in the future, esp. given the amount of work we expect to happen in > the partitioning area. When that happens, fixing all those places in > that way wouldn't be good. > >> So we could >> really minimize the code change and avoid the additional overhead caused by >> the is_converted_whole_row_reference test added to pull_var_clause. I think >> the latter would be rather important, because PWJ is disabled by default. >> What do you think about that approach? > > Partition-wise join and partition-wise aggregation are disabled by > default temporarily. We will change it in near future once the memory > consumption issue has been tackled. The features are useful and users > would want those to be enabled by default like other query > optimizations. So, we can't take a decision based on that.
Here's set of updated patches. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
expr_ref_error_pwj_v7.tar.gz
Description: GNU Zip compressed data
