public inbox for [email protected]
help / color / mirror / Atom feedFrom: Ashutosh Bapat <[email protected]>
To: Etsuro Fujita <[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: Mon, 14 May 2018 12:04:04 +0530
Message-ID: <CAFjFpRc8ZoDm0+zhx+MckwGyEqkOzWcpVqbvjaxwdGarZSNrmA@mail.gmail.com> (raw)
In-Reply-To: <CAFjFpRfkafBFJSEEkGwzLf=-y9Q8TUEGfpR=+K7-zCOw80R10w@mail.gmail.com>
References: <CAKcux6ktu-8tefLWtQuuZBYFaZA83vUzuRd7c1YHC-yEWyYFpg@mail.gmail.com>
<CAFjFpRfD5=LsOqG1eUf-vMpm9KoEZBBGJo68D9wkUBJxop_emQ@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>
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:
>>
>> 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
Attachments:
[application/x-gzip] expr_ref_error_pwj_v7.tar.gz (11.2K, ../CAFjFpRc8ZoDm0+zhx+MckwGyEqkOzWcpVqbvjaxwdGarZSNrmA@mail.gmail.com/2-expr_ref_error_pwj_v7.tar.gz)
download
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: <CAFjFpRc8ZoDm0+zhx+MckwGyEqkOzWcpVqbvjaxwdGarZSNrmA@mail.gmail.com>
* 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