Re: [HACKERS] path toward faster partition pruning

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, Jesper Pedersen <jesper(dot)pedersen(at)redhat(dot)com>, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, Amit Langote <amitlangote09(at)gmail(dot)com>, Rajkumar Raghuwanshi <rajkumar(dot)raghuwanshi(at)enterprisedb(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-02-02 06:04:17
Message-ID: 159153b4-0360-f696-769a-0e0d6e1d6a5b@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
v24-0001-Refactor-code-for-partition-bound-searching.patch text/plain 12.4 KB
v24-0002-Introduce-a-get_partitions_from_clauses.patch text/plain 69.8 KB
v24-0003-Move-some-code-of-set_append_rel_size-to-separat.patch text/plain 8.6 KB
v24-0004-More-refactoring-around-partitioned-table-Append.patch text/plain 13.4 KB
v24-0005-Teach-planner-to-use-get_partitions_from_clauses.patch text/plain 34.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Jeff Davis 2018-02-02 06:06:49 Re: JIT compiling with LLVM v9.0
Previous Message Bruce Momjian 2018-02-02 05:39:28 Re: proposal: alternative psql commands quit and exit