On 7 November 2017 at 01:52, David Rowley <david.row...@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
/*
* get_append_rel_partitions
* Return the list of partitions of rel that pass the clauses mentioned
* in rel->baserestrictinfo. An empty list is returned if no matching
* partitions were found.
*
* Returned list contains the AppendRelInfos of chosen partitions.
*/
static List *
get_append_rel_partitions(PlannerInfo *root,
RelOptInfo *rel,
RangeTblEntry *rte)
{
List *partclauses;
bool contains_const,
constfalse;
/*
* Get the clauses that match the partition key, including information
* about any nullness tests against partition keys. Set keynullness to
* a invalid value of NullTestType, which 0 is not.
*/
partclauses = match_clauses_to_partkey(rel,
list_copy(rel->baserestrictinfo),
&contains_const,
&constfalse);
if (!constfalse)
{
Relation parent = heap_open(rte->relid, NoLock);
PartitionDesc partdesc = RelationGetPartitionDesc(parent);
Bitmapset *partindexes;
List *result = NIL;
int i;
/*
* If we have matched clauses that contain at least one constant
* operand, then use these to prune partitions.
*/
if (partclauses != NIL && contains_const)
partindexes = get_partitions_from_clauses(parent, partclauses);
/*
* Else there are no clauses that are useful to prune any paritions,
* so we must scan all partitions.
*/
else
partindexes = bms_add_range(NULL, 0, partdesc->nparts - 1);
/* Fetch the partition appinfos. */
i = -1;
while ((i = bms_next_member(partindexes, i)) >= 0)
{
AppendRelInfo *appinfo = rel->part_appinfos[i];
#ifdef USE_ASSERT_CHECKING
RangeTblEntry *rte = planner_rt_fetch(appinfo->child_relid, root);
/*
* Must be the intended child's RTE here, because appinfos are ordered
* the same way as partitions in the partition descriptor.
*/
Assert(partdesc->oids[i] == rte->relid);
#endif
result = lappend(result, appinfo);
}
/* Record which partitions must be scanned. */
rel->live_part_appinfos = result;
heap_close(parent, NoLock);
return result;
}
return NIL;
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
