Re: Declarative partitioning - another take

Hi Rushabh,

On 2016/11/22 22:11, Rushabh Lathia wrote:
> Hi Amit,
> 
> I was just reading through your patches and here are some quick review
> comments
> for 0001-Catalog-and-DDL-for-partitioned-tables-17.patch.

Thanks for the review!

> 
> Review comments for  0001-Catalog-and-DDL-for-partitioned-tables-17.patch:
> 
> 1)
> @@ -1102,9 +1104,10 @@ heap_create_with_catalog(const char *relname,
>      {
>          /* Use binary-upgrade override for pg_class.oid/relfilenode? */
>          if (IsBinaryUpgrade &&
> -            (relkind == RELKIND_RELATION || relkind == RELKIND_SEQUENCE ||
> -             relkind == RELKIND_VIEW || relkind == RELKIND_MATVIEW ||
> -             relkind == RELKIND_COMPOSITE_TYPE || relkind ==
> RELKIND_FOREIGN_TABLE))
> +            (relkind == RELKIND_RELATION || relkind ==
> RELKIND_PARTITIONED_TABLE ||
> +             relkind == RELKIND_SEQUENCE || relkind == RELKIND_VIEW ||
> +             relkind == RELKIND_MATVIEW || relkind ==
> RELKIND_COMPOSITE_TYPE ||
> +             relkind == RELKIND_FOREIGN_TABLE))
> 
> You should add the RELKIND_PARTITIONED_TABLE at the end of if condition that
> will make diff minimal. While reading through the patch I noticed that there
> is inconsistency - someplace its been added at the end and at few places its
> at the start. I think you can make add it at the end of condition and be
> consistent with each place.

OK, done.

> 
> 2)
> 
> +        /*
> +         * We need to transform the raw parsetrees corresponding to
> partition
> +         * expressions into executable expression trees.  Like column
> defaults
> +         * and CHECK constraints, we could not have done the transformation
> +         * earlier.
> +         */
> 
> 
> Additional space before "Like column defaults".

I think it's a common practice to add two spaces after a sentence-ending
period [https://www.gnu.org/prep/standards/html_node/Comments.html], which
it seems, is followed more or less regularly in the formatting of the
comments in the PostgreSQL source code.

> 3)
> -    char        relkind;
> +    char        relkind,
> +                expected_relkind;
> 
> Newly added variable should be define separately with its type. Something
> like:
> 
>     char        relkind;
> +    char        expected_relkind;

OK, done.

> 
> 4)
> 
> a)
> +    /* Prevent partitioned tables from becoming inheritance parents */
> +    if (parent_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> +        ereport(ERROR,
> +                (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> +                 errmsg("cannot inherit from partitioned table \"%s\"",
> +                         parent->relname)));
> +
> 
> need alignment for last line.

Fixed.

> b)
> +            atttuple = SearchSysCacheAttName(RelationGetRelid(rel),
> pelem->name);
> +            if (!HeapTupleIsValid(atttuple))
> +                ereport(ERROR,
> +                        (errcode(ERRCODE_UNDEFINED_COLUMN),
> +                         errmsg("column \"%s\" named in partition key does
> not exist",
> +                         pelem->name)));
> +            attform = (Form_pg_attribute) GETSTRUCT(atttuple);
> +
> +            if (attform->attnum <= 0)
> +                ereport(ERROR,
> +                        (errcode(ERRCODE_UNDEFINED_COLUMN),
> +                         errmsg("cannot use system column \"%s\" in
> partition key",
> +                         pelem->name)));
> 
> need alignment for last line of ereport

Fixed.

> c)
> +        /* Disallow ROW triggers on partitioned tables */
> +        if (stmt->row && rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> +            ereport(ERROR,
> +                    (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> +                    errmsg("\"%s\" is a partitioned table",
> +                            RelationGetRelationName(rel)),
> +              errdetail("Partitioned tables cannot have ROW triggers.")));
> 
> need alignment

Fixed.

> 5)
> 
> @@ -2512,6 +2579,7 @@ transformAlterTableStmt(Oid relid, AlterTableStmt
> *stmt,
>      cxt.blist = NIL;
>      cxt.alist = NIL;
>      cxt.pkey = NULL;
> +    cxt.ispartitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
> 
> I think adding bracket will look code more clear.
> 
> +    cxt.ispartitioned = (rel->rd_rel->relkind ==
> RELKIND_PARTITIONED_TABLE);

Agreed, done.

> 6)
> 
> + * RelationBuildPartitionKey
> + *        Build and attach to relcache partition key data of relation
> + *
> + * Partitioning key data is stored in CacheMemoryContext to ensure it
> survives
> + * as long as the relcache.  To avoid leaking memory in that context in
> case
> + * of an error partway through this function, we build the structure in the
> + * working context (which must be short-lived) and copy the completed
> + * structure into the cache memory.
> 
> extra space before "To avoid leaking memory"

Same thing I said above.

> 7)
> +    /* variable-length fields start here, but we allow direct access to
> partattrs */
> +    int2vector        partattrs;        /* attribute numbers of columns in
> the
> 
> Why partattrs is allow direct access - its not really clear from the
> comments.

I have copied the comment from another catalog header file (a number of
catalog headers have the same comment).  I updated the new file's comment
to say a little bit more; I wonder if the comment should be updated in
other files as well?  However, I noticed that there are explanatory notes
elsewhere (for example, around the code that reads such a field from the
catalog) about why the first variable-length field of a catalog's tuple
(especially of type int2vector or oidvector) are directly accessible via
their C struct offsets.

> I will continue reading more patch and testing functionality.. will share
> the
> comments as I have it.

Updated patches attached.  I have also considered Robert's comments [1] as
follows and fixed a bug that Ashutosh reported [2]:

- Force partition key columns to be NOT NULL at the table creation time
  if using range partitioning
- Use enum to represent the content of individual range datums (viz.
  finite datum, -infinity, +infinity) in the relcache struct

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CA+TgmoZ-feQsxc7U_JerM_AFChp3Qf6btK708SAe7M8Vdv5=jA@mail.gmail.com
[2]
https://www.postgresql.org/message-id/306c85e9-c702-3742-eeff-9b7a40498afc%40lab.ntt.co.jp