Re: [HACKERS] path toward faster partition pruning

Thanks Ashutosh and David for reviewing. Replying to both.

On 2018/02/28 20:25, Ashutosh Bapat wrote:
> On Tue, Feb 27, 2018 at 3:03 PM, Amit Langote
> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> Attached an updated version in which I incorporated some of the revisions
>> that David Rowley suggested to OR clauses handling (in partprune.c) that
>> he posted as a separate patch on the run-time pruning thread [1].
>
> Some comments on 0001.
>
> partnatts != part_scheme->partnatts)
> continue;
>
> - /* Match the partition key types. */
> + /*
> + * Match the partition key types and partitioning-specific collations.
> + */
>
> We are comparing opfamily and opclass input type as well, but this comment
> doesn't explicitly mention those like it mentions collation. Instead, I think
> we should just say, "Match partition key type properties"

Sounds good, done.

> You had commented on "advanced partition matching code" about asserting that
> parsupfuncs also match. Robert too has expressed similar opinion upthread. May
> be we should at least try to assert that the function OIDs match.

I guess you're referring to this email of mine:

https://www.postgresql.org/message-id/e681c283-5fc6-b1c6-1bb9-7102c37e2d55%40lab.ntt.co.jp

Note that I didn't say that we should Assert the equality of partsupfunc
members themselves, but rather whether we could add a comment explaining
why we don't. Although, like you say, we could Assert the equality of
function OIDs, so added a block like this:

+ /*
+ * If partopfamily and partopcintype matched, must have the same
+ * partition comparison functions. Note that we cannot reliably
+ * Assert the equality of function structs themselves for they might
+ * be different across PartitionKey's, so just Assert for the function
+ * OIDs.
+ */
+#ifdef USE_ASSERT_CHECKING
+ {
+ int i;
+
+ for (i = 0; i < partkey->partnatts; i++)
+ Assert(partkey->partsupfunc[i].fn_oid ==
+ part_scheme->partsupfunc[i].fn_oid);
+ }
+#endif

> - Oid *parttypcoll; /* OIDs of collations of partition keys. */
> +
> + /*
> + * We store both the collation implied by the partition key's type and the
> + * one specified for partitioning. Values in the former are used as
> + * varcollid in the Vars corresponding to simple column partition keys so
> + * as to make them match corresponding Vars appearing elsewhere in the
> + * query tree. Whereas, the latter is used when actually comparing values
> + * against partition bounds datums, such as, when doing partition pruning.
> + */
> + Oid *parttypcoll;
> + Oid *partcollation;
>
> As you have already mentioned upthread only partcollation is needed, not
> parttypcoll.

This hunk is gone after rebasing over 2af28e603319 (For partitionwise
join, match on partcollation, not parttypcoll) that was committed earlier
today.

> /* Cached information about partition key data types. */
> int16 *parttyplen;
> bool *parttypbyval;
> +
> + /*
> + * Cached array of partitioning comparison functions' fmgr structs. We
> + * don't compare these when trying to match two partition schemes.
> + */
>
> I think this comment should go away. The second sentence doesn't explain why
> and if it did so it should do that in find_partition_scheme() not here.
> partsupfunc is another property of partition keys that is cached like
> parttyplen, parttypbyval. Why does it deserve a separate comment when others
> don't?

Replaced that comment with:

+ /* Cached information about partition comparison functions. */

As mentioned above, I already added a comment in find_partition_scheme().

On 2018/02/28 20:35, David Rowley wrote:
> Micro review of v34:
>
> 1. Looks like you've renamed the parttypid parameter in the definition
> of partkey_datum_from_expr and partition_cmp_args, but not updated the
> declaration too.
>
> +static bool partkey_datum_from_expr(Oid parttypid, Expr *expr, Datum
*value);
>
> +static bool
> +partkey_datum_from_expr(Oid partopcintype, Expr *expr, Datum *value)
>
> +static bool partition_cmp_args(Oid parttypid, Oid partopfamily,
> + PartClause *pc, PartClause *leftarg, PartClause *rightarg,
> + bool *result);
>
> +static bool
> +partition_cmp_args(Oid partopcintype, Oid partopfamily,
> + PartClause *pc, PartClause *leftarg, PartClause *rightarg,
> + bool *result)

Oops, forgot about the declarations. Fixed.

> 2. In prune_append_rel_partitions(), it's not exactly illegal, but int
> i is declared twice in different scopes. Looks like there's no need
> for the inner one.

Removed the inner one.

Attached updated patches.

Thanks,
Amit