Re: [HACKERS] path toward faster partition pruning - Mailing list pgsql-hackers

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@lab.ntt.co.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