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