Re: [HACKERS] path toward faster partition pruning

Thanks for the review.

On 2018/02/02 7:38, Robert Haas wrote:
> +/*
> + * PartitionBoundCmpArg - Caller-defined argument to be passed to
> + *                                               partition_bound_cmp()
> + *
> + * The first (fixed) argument involved in a comparison is the partition bound
> + * found in the catalog, while an instance of the following struct describes
> + * either a new partition bound being compared against existing bounds
> + * (caller should set is_bound to true and set bound), or a new tuple's
> + * partition key specified in datums (caller should set ndatums to the number
> + * of valid datums that are passed in the array).
> + */
> +typedef struct PartitionBoundCmpArg
> +{
> +       bool    is_bound;
> +       union
> +       {
> +               PartitionListValue         *lbound;
> +               PartitionRangeBound        *rbound;
> +               PartitionHashBound         *hbound;
> +       }       bound;
> +
> +       Datum  *datums;
> +       int             ndatums;
> +} PartitionBoundCmpArg;
> 
> This is a pretty strange definition.  datums/ndatums are never valid
> at the same time as any of lbound/rbound/hbound, but are not included
> in the union.  Also, is_bound doesn't tell you which of
> rbound/lbound/hbound is actually valid.  Granted, the current calling
> convention looks like a mess, too.  Apparently, the argument to
> partition_bound_cmp is a PartitionBoundSpec if using hash
> partitioning, a Datum if list partitioning, and either a
> PartitionRangeBound or a Datum * if range partitioning depending on
> the value of probe_is_bound, and I use the word "apparently" because
> there are zero words of comments explaining what the argument to
> partition_bound_cmp of type "void *" is supposed to mean.  I really
> should have noticed that and insisted that it be fixed before
> partitioning got committed.

Yeah, I was trying to fix the status quo by introducing that new struct,
but I agree it's much better to modify the functions around a bit like the
way you describe below.

> Looking a bit further, there are exactly two calls to
> partition_bound_cmp().  One is in partition_bound_bsearch() and the
> other is in check_new_partition_bound(). Now, looking at this, both
> the call to partition_bound_cmp() and every single call to
> partition_bound_bsearch() are inside a switch branch where we've
> dispatched on the partitioning type, which means that from code that
> is already specialized by partitioning type we are calling generic
> code which then turns back around and calls code that is specialized
> by partitioning type.  Now, that could make sense if the generic code
> is pretty complex, but here's it's basically just the logic to do a
> bsearch.  It seems to me that a cleaner solution here would be to
> duplicate that logic.  Then we could have...
> 
> static int partition_list_bsearch(PartitionKey key, PartitionBoundInfo
> boundinfo,
>                         Datum value, bool *is_equal);
> static int partition_range_bsearch(PartitionKey key,
> PartitionBoundInfo boundinfo,
>                         PartitionRangeBound *probe);
> static int partition_range_datum_bsearch(PartitionKey key,
> PartitionBoundInfo boundinfo,
>                         int nvalues, Datum *values);
> static int partition_hash_bsearch(PartitionKey key, PartitionBoundInfo
> boundinfo,
>                         int modulus, int remainder, bool *is_equal);
> 
> ...which would involve fewer branches at runtime and more type-safety
> at compile time.  partition_hash_bsearch could directly call
> partition_hbound_cmp, partition_list_bsearch could directly invoke
> FunctionCall2Coll, partition_range_bsearch could directly call
> partition_rbound_cmp, and partition_range_datum_bsearch could directly
> call partition_rbound_datum_cmp.
> 
> All-in-all that seems a lot nicer to me than what we have here now.
> IIUC, the purpose of this patch is to let you search on a prefix of
> the partition keys, but I think that's really only possible for range
> partitioning, and it seems like the proposed nvalues argument to
> partition_range_datum_bsearch would give you what you need.

Your proposed cleanup sounds much better, so I implemented it in the
attached updated 0001, while dropping the previously proposed
PartitionBoundCmpArg approach.

Updated set of patches attached (patches 0002 onward mostly unchanged,
except I incorporated the delta patch posted by David upthread).

Thanks,
Amit