On 7 November 2017 at 01:52, David Rowley <david.rowley@2ndquadrant.com> wrote:
> Thanks. I'll look over it all again starting my Tuesday morning. (UTC+13)
I have a little more review to share:
1. Missing "in" in comment. Should be "mentioned in"
* get_append_rel_partitions
* Return the list of partitions of rel that pass the clauses mentioned
* rel->baserestrictinfo
2. Variable should be declared in inner scope with the following fragment:
void
set_basic_child_rel_properties(PlannerInfo *root,
RelOptInfo *rel,
RelOptInfo *childrel,
AppendRelInfo *appinfo)
{
AttrNumber attno;
if (rel->part_scheme)
{
which makes the code the same as where you moved it from.
3. Normally lfirst() is assigned to a variable at the start of a
foreach() loop. You have code which does not follow this.
foreach(lc, clauses)
{
Expr *clause;
int i;
if (IsA(lfirst(lc), RestrictInfo))
{
RestrictInfo *rinfo = lfirst(lc);
You could assign this to a Node * since the type is unknown to you at
the start of the loop.
4.
/*
* Useless if what we're thinking of as a constant is actually
* a Var coming from this relation.
*/
if (bms_is_member(rel->relid, constrelids))
continue;
should this be moved to just above the op_strict() test? This one seems cheaper.
5. Typo "paritions": /* No clauses to prune paritions, so scan all
partitions. */
But thinking about it more the comment should something more along the
lines of /* No useful clauses for partition pruning. Scan all
partitions. */
The key difference is that there might be clauses, just without Consts.
Actually, the more I look at get_append_rel_partitions() I think it
would be better if you re-shaped that if/else if test so that it only
performs the loop over the partindexes if it's been set.
I ended up with the attached version of the function after moving
things around a little bit.
I'm still reviewing but thought I'd share this part so far.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
