From: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
---|---|
To: | David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> |
Cc: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>, Jesper Pedersen <jesper(dot)pedersen(at)redhat(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, Rajkumar Raghuwanshi <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com>, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, Amit Langote <amitlangote09(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Beena Emerson <memissemerson(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [HACKERS] path toward faster partition pruning |
Date: | 2018-04-06 14:38:23 |
Message-ID: | 20180406143823.nnhpwgfz25qpucan@alvherre.pgsql |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
David Rowley wrote:
> 2. I guess this will be removed before commit.
>
> +#if 0
> <large section of comments>
> +#endif
Yeah, there is one sentence there I didn't quite understand and would
like to add it to the rewritten version of the comment before I remove
the whole ifdeffed-out comment.
* PARTCLAUSE_MATCH_STEPS: *clause_steps set to list of "partition pruning
* step(s)" generated for the clause due to it being a BoolExpr or a
* ScalarArrayOpExpr that's turned into one
Exactly what does "ScalarArrayOpExpr that's turned into one" means?
Does it mean we turn SAOP into BoolExpr?
(Yes, I know "#if 0" inside a comment doesn't do anything. It's only
documentation for myself.)
If you look at the rest of the rewritten comment, you'll notice some
things probably need more explaining. Wording suggestions welcome.
> 3. This comment seems like a strange thing to write just before
> testing if the clause matches the partition key.
>
> + /* Clause does not match this partition key. */
> + if (equal(leftop, partkey))
> + *rightop = not_clause((Node *) clause)
> + ? (Expr *) makeBoolConst(false, false)
> + : (Expr *) makeBoolConst(true, false);
Yeah. Looking at this function, I noticed it tests for BooleanTest, and
falls back to checking "not_clause" and a few equals. Does it make
sense if the clause is a SAOP? I added this assert:
Assert(IsA(clause, BooleanTest) ||
IsA(clause, BoolExpr) ||
IsA(clause, RelabelType));
and it failed:
#3 0x0000556cf04505db in match_boolean_partition_clause (partopfamily=424,
clause=0x556cf1041670, partkey=0x556cf1042218, rightop=0x7ffe520ec068)
at /pgsql/source/master/src/backend/optimizer/util/partprune.c:2159
2159 Assert(IsA(clause, BooleanTest) ||
(gdb) print *clause
$1 = {type = T_ScalarArrayOpExpr}
I'm not sure whether or not this function can trust that what's incoming
must absolutely be only those node types.
> 4. Comment needs removed.
>
> + * has_default_part - Whether the table has a default partition
Done.
> The only other thing I noted on this pass is that we could get rid of:
>
> + /* go check the next clause. */
> + if (unsupported_clause)
> + break;
>
> and just "continue" instead of "break" in all cases apart from case
> PARTCLAUSE_UNSUPPORTED:
>
> it would save a few lines and a single condition. What's there works,
> but thought this might be better...
Makes sense -- looking.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2018-04-06 14:39:10 | Re: WIP: a way forward on bootstrap data |
Previous Message | Peter Eisentraut | 2018-04-06 14:25:19 | Re: [HACKERS] Subscription code improvements |