Re: [HACKERS] [POC] hash partitioning - Mailing list pgsql-hackers

On Fri, 23 Jun 2017 13:41:15 +0900
Yugo Nagata <nagata@sraoss.co.jp> wrote:

> On Tue, 6 Jun 2017 13:03:58 +0530
> amul sul <sulamul@gmail.com> wrote:
> 
> 
> > Updated patch attached.
> 
> I looked into the latest patch (v13) and have some comments
> althogh they might be trivial.

One more comment:

+       if (spec->remainder < 0)
+           ereport(ERROR,
+                   (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+                    errmsg("remainder for hash partition must be a non-negative integer")));

The value of remainder is defined as Iconst in gram.y, so it never be negative.
Hence, I think this check is not necessary or Assert is enough.

> 
> First, I couldn't apply this patch to the latest HEAD due to
> a documentation fix and pgintend updates. It needes rebase.
> 
> $ git apply /tmp/0002-hash-partitioning_another_design-v13.patch  
> error: patch failed: doc/src/sgml/ref/create_table.sgml:87
> error: doc/src/sgml/ref/create_table.sgml: patch does not apply
> error: patch failed: src/backend/catalog/partition.c:76
> error: src/backend/catalog/partition.c: patch does not apply
> error: patch failed: src/backend/commands/tablecmds.c:13371
> error: src/backend/commands/tablecmds.c: patch does not apply
> 
> 
>        <varlistentry>
> +       <term>Hash Partitioning</term>
> +
> +       <listitem>
> +        <para>
> +         The table is partitioned by specifying modulus and remainder for each
> +         partition. Each partition holds rows for which the hash value of
> +         partition keys when divided by specified modulus produces specified
> +         remainder. For more clarification on modulus and remainder please refer
> +         <xref linkend="sql-createtable-partition">.
> +        </para>
> +       </listitem>
> +      </varlistentry>
> +
> +      <varlistentry>
>         <term>Range Partitioning</term>
> 
> I think this section should be inserted after List Partitioning section because
> the order of the descriptions is Range, List, then Hash in other places of
> the documentation. At least, 
> 
> 
> -    <firstterm>partition bounds</firstterm>.  Currently supported
> -    partitioning methods include range and list, where each partition is
> -    assigned a range of keys and a list of keys, respectively.
> +    <firstterm>partition bounds</firstterm>.  The currently supported
> +    partitioning methods are list, range, and hash.
>     </para>
> 
> Also in this hunk. I think "The currently supported partitioning methods are
> range, list, and hash." is better. We don't need to change the order of
> the original description.
>  
> 
>        <listitem>
>         <para>
> -        Declarative partitioning only supports list and range partitioning,
> -        whereas table inheritance allows data to be divided in a manner of
> -        the user's choosing.  (Note, however, that if constraint exclusion is
> -        unable to prune partitions effectively, query performance will be very
> -        poor.)
> +        Declarative partitioning only supports hash, list and range
> +        partitioning, whereas table inheritance allows data to be divided in a
> +        manner of the user's choosing.  (Note, however, that if constraint
> +        exclusion is unable to prune partitions effectively, query performance
> +        will be very poor.)
> 
> Similarly, I think "Declarative partitioning only supports range, list and hash
> partitioning," is better.
> 
> 
> +
> +  <para>
> +   Create a hash partitioned table:
> +<programlisting>
> +CREATE TABLE orders (
> +    order_id     bigint not null,
> +    cust_id      bigint not null,
> +    status       text
> +) PARTITION BY HASH (order_id);
> +</programlisting></para>
> +
> 
> This paragraph should be inserted between "Create a list partitioned table:"
> paragraph and "Ceate partition of a range partitioned table:" paragraph
> as well as range and list.
> 
> 
>          *strategy = PARTITION_STRATEGY_LIST;
>      else if (pg_strcasecmp(partspec->strategy, "range") == 0)
>          *strategy = PARTITION_STRATEGY_RANGE;
> +    else if (pg_strcasecmp(partspec->strategy, "hash") == 0)
> +        *strategy = PARTITION_STRATEGY_HASH;
>      else
>          ereport(ERROR,
> 
> In the most of codes, the order is hash, range, then list, but only
> in transformPartitionSpec(), the order is list, range, then hash,
> as above. Maybe it is better to be uniform.
> 
> 
> +            {
> +                if (strategy == PARTITION_STRATEGY_HASH)
> +                    ereport(ERROR,
> +                            (errcode(ERRCODE_UNDEFINED_OBJECT),
> +                             errmsg("data type %s has no default hash operator class",
> +                                    format_type_be(atttype)),
> +                             errhint("You must specify a hash operator class or define a default hash operator class
forthe data type.")));
 
> +                else
> +                    ereport(ERROR,
> +                            (errcode(ERRCODE_UNDEFINED_OBJECT),
> +                             errmsg("data type %s has no default btree operator class",
> +                                    format_type_be(atttype)),
> +                             errhint("You must specify a btree operator class or define a default btree operator
classfor the data type.")));
 
> +
> +
> 
>                                                 atttype,
> -                                               "btree",
> -                                               BTREE_AM_OID);
> +                                               am_oid == HASH_AM_OID ? "hash" : "btree",
> +                                               am_oid);
> 
> How about writing this part as following to reduce code redundancy?
> 
> +    Oid            am_oid;
> +    char       *am_name;
> 
> <snip> 
> 
> +        if (strategy == PARTITION_STRATEGY_HASH)
> +        {
> +            am_oid = HASH_AM_OID;
> +            am_name = pstrdup("hash");
> +        }
> +        else
> +        {
> +            am_oid = BTREE_AM_OID;
> +            am_name = pstrdup("btree");
> +        }
> +
>          if (!pelem->opclass)
>          {
> -            partopclass[attn] = GetDefaultOpClass(atttype, BTREE_AM_OID);
> +            partopclass[attn] = GetDefaultOpClass(atttype, am_oid);
>  
>              if (!OidIsValid(partopclass[attn]))
>                  ereport(ERROR,
>                          (errcode(ERRCODE_UNDEFINED_OBJECT),
> -                   errmsg("data type %s has no default btree operator class",
> -                          format_type_be(atttype)),
> -                         errhint("You must specify a btree operator class or define a default btree operator class
forthe data type.")));
 
> +                         errmsg("data type %s has no default %s operator class",
> +                                format_type_be(atttype), am_name),
> +                         errhint("You must specify a %s operator class or define a default %s operator class for the
datatype.",
 
> +                                 am_name, am_name)));
> +
>          }
>          else
>              partopclass[attn] = ResolveOpClass(pelem->opclass,
>                                                 atttype,
> -                                               "btree",
> -                                               BTREE_AM_OID);
> +                                               am_name,
> +                                               am_oid);
> 
> 
> There is meaningless indentation change.
> 
> @@ -2021,7 +2370,8 @@ get_partition_for_tuple(PartitionDispatch *pd,
>                     /* bsearch in partdesc->boundinfo */
>                     cur_offset = partition_bound_bsearch(key,
>                                                          partdesc->boundinfo,
> -                                                        values, false, &equal);
> +                                                     values, false, &equal);
> +
>                     /*
>                      * Offset returned is such that the bound at offset is
> 
> 
> Fixing the comment of pg_get_partkeydef() is missing.
> 
>  * pg_get_partkeydef
>  *
>  * Returns the partition key specification, ie, the following:
>  *
>  * PARTITION BY { RANGE | LIST } (column opt_collation opt_opclass [, ...])
>  */
> Datum
> pg_get_partkeydef(PG_FUNCTION_ARGS)
> {
> 
> Regards,
> 
> > 
> > Regards,
> > Amul Sul
> 
> 
> -- 
> Yugo Nagata <nagata@sraoss.co.jp>
> 
> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


-- 
Yugo Nagata <nagata@sraoss.co.jp>