public inbox for [email protected]
help / color / mirror / Atom feedFrom: Etsuro Fujita <[email protected]>
To: Ashutosh Bapat <[email protected]>
Cc: Rajkumar Raghuwanshi <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Expression errors with "FOR UPDATE" and postgres_fdw with partition wise join enabled.
Date: Wed, 16 May 2018 19:31:01 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAFjFpRc8ZoDm0+zhx+MckwGyEqkOzWcpVqbvjaxwdGarZSNrmA@mail.gmail.com>
References: <CAKcux6ktu-8tefLWtQuuZBYFaZA83vUzuRd7c1YHC-yEWyYFpg@mail.gmail.com>
<CAFjFpReD2rwzw64T5ZHS9+S8avREuazLu4nnCRAXToUN=WC+-Q@mail.gmail.com>
<[email protected]>
<CAFjFpRcHQ9p0JEpx4eMhuNvD7GbyMYm1TykfCeBrNTdK+mSbgA@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAFjFpRennScZ47UwYaE+3SB_KcGVToPmR2dOTEb7T7w2=j-jbQ@mail.gmail.com>
<[email protected]>
<CAFjFpRetD0aPMgw_mhOtJ11AqWJFCTo9dNJ758Zh4SPqgN1qvw@mail.gmail.com>
<[email protected]>
<CAFjFpRdSvXxOkSMc=jEHfTVGnCFgziLA7m+onp=ak=Tfa_V1zA@mail.gmail.com>
<[email protected]>
<CAFjFpRcFucgvrzEFNi10g00-va640poSmJDmBrZNfFarB6eP-Q@mail.gmail.com>
<[email protected]>
<CAFjFpRczEuG9b0_j_qve2JAi-HHqS9_m1_yK5QVQa5=oDr5mJA@mail.gmail.com>
<[email protected]>
<CAFjFpRfkafBFJSEEkGwzLf=-y9Q8TUEGfpR=+K7-zCOw80R10w@mail.gmail.com>
<CAFjFpRc8ZoDm0+zhx+MckwGyEqkOzWcpVqbvjaxwdGarZSNrmA@mail.gmail.com>
(2018/05/14 15:34), Ashutosh Bapat wrote:
> On Mon, May 14, 2018 at 10:21 AM, Ashutosh Bapat
> <[email protected]> wrote:
>> On Fri, May 11, 2018 at 6:31 PM, Etsuro Fujita
>> <[email protected]> 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
view thread (115+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Expression errors with "FOR UPDATE" and postgres_fdw with partition wise join enabled.
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox