|From:||Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>|
|To:||Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>|
|Cc:||Rajkumar Raghuwanshi <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: Expression errors with "FOR UPDATE" and postgres_fdw with partition wise join enabled.|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
(2018/05/14 15:34), Ashutosh Bapat wrote:
> On Mon, May 14, 2018 at 10:21 AM, Ashutosh Bapat
> <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
>> On Fri, May 11, 2018 at 6:31 PM, Etsuro Fujita
>> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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;
>>> 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);
postgres=# create table child (a int, b text);
postgres=# alter table child inherit parent ;
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
>> 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
+ /* 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.
|Next Message||Etsuro Fujita||2018-05-16 10:41:34||Re: postgres_fdw: Oddity in pushing down inherited UPDATE/DELETE joins to remote servers|
|Previous Message||Heikki Linnakangas||2018-05-16 10:09:07||Re: Postgres 11 release notes|