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
