(2018/05/14 15:34), Ashutosh Bapat wrote:
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:
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;
Thanks!
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.
Actually, we'd get the same error without using anything in
partitioning. Here is an example:
postgres=# create table parent (a int, b text);
CREATE TABLE
postgres=# create table child (a int, b text);
CREATE TABLE
postgres=# alter table child inherit parent ;
ALTER TABLE
postgres=# alter table child add constraint child_check check
(child::parent::text != NULL);
ERROR: ConvertRowtypeExpr found where not expected
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.
Sorry, I don't understand this fully.
So, it's not clear to me when we add
a ConvertRowtypeExpr and we don't.
I think it'd be OK to rewrite so at least in prepare_sort_from_pathkeys
and build_tlist_to_deparse.
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.
I have to admit that the approach I proposed is a band-aid fix.
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.
Yeah, I really agree on that point! However, considering that
pull_var_clause is used in many places where partitioning is *not*
involved, I still think it's better to avoid spending extra cycles
needed only for partitioning in that function.
Here's set of updated patches.
Thanks for updating the patch!
Here are some other minor comments on patch
0003-postgres_fdw-child-join-with-ConvertRowtypeExprs-cau.patch:
+ /* Construct the conversion map. */
+ parent_desc = lookup_rowtype_tupdesc(cre->resulttype, 0);
I think it'd be better to pass -1, not 0, as the typmod argument for
that function because that would be consistent with other places where
we use that function with named rowtypes (eg, ruleutils.c).
-is_subquery_var(Var *node, RelOptInfo *foreignrel, int *relno, int *colno)
+is_subquery_column(Node *node, Index varno, RelOptInfo *foreignrel,
+ int *relno, int *colno)
-1 for that change; because 1) we use "var" for implying many things as
in eg, pull_var_clause and 2) I think it'd make back-patching easy to
keep the name as-is.
Other than that the patch set looks good to me.
Best regards,
Etsuro Fujita
