Re: Documentation improvements for partitioning

On 2017/02/10 19:19, Simon Riggs wrote:
> On 10 February 2017 at 08:18, Amit Langote
> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>
>> I agree that getting the proposed documentation changes committed would be
>> a step ahead.
>
> Committed. I tested how it works and added documentation that matched
> my experiences, correcting what you'd said and adding more information
> for clarity as I went.

Thanks for making the improvements to and committing the patch!

> Few points from tests
>
> * "ERROR: cannot create index on partitioned table "measurement_year_month""
> is misleading because you can create indexes on partitions

Do you mean that this should not cause an error at all, but create the
specified index on partitions as part of running the command? Should the
code to handle that be part of this release?

Or do you simply mean that we should rewrite the error message and/or add
a HINT asking to create the index on partitions instead?

> * You can create additional CHECK (column is NOT NULL) as a check
> constraint, even if you can't create a not null constraint

Sorry but I am not exactly sure which "cannot create a not null
constraint" you are referring to here.

There are no restrictions on *creating* constraints on partitions, but
there are on dropping. Regular inheritance rules prevent dropping
inherited constraints (just the same for partitioned tables), of which
there are only named CHECK constraints at the moment. A new rule
introduced for partitions prevents dropping a column's NOT NULL constraint
if it's been "inherited" (i.e., the same constraint is present in the
parent partitioned table), although it's not in the same sense as for
CHECK constraints, because NOT NULL constraints are not tracked with
pg_constraints.

> * The OID inheritance needs work - you shouldn't need to specify a
> partition needs OIDS if the parent has it already.

That sounds right. It's better to keep the behavior same as for regular
inheritance. Will post a patch to get rid of the unnecessary check.

FWIW, the check was added to prevent the command from succeeding in the
case where WITHOUT OIDS has been specified for a partition and the parent
partitioned table has the OID column. Regular inheritance simply
*overrides* the WITHOUT OIDS specification, which might be seen as surprising.

> * There's no tab completion, which prevents people from testing this
> (maybe better now with some docs)

Will post a patch as well.

> * ERROR: no partition of relation "measurement_year_month" found for row
> DETAIL: Failing row contains (2016-12-02, 1, 1).
> should not return the whole row, just the partition keys

I think that makes sense. Something along the lines of
BuildIndexValueDescription() for partition keys will be necessary. Will
post a patch.

> * It's very weird you can't DROP a partitioned table. I think you need
> to add dependencies.

Do you mean it should be possible to DROP a partitioned table without
needing to specify CASCADE? Currently, same thing happens for a
partitioned table as will for a inheritance parent.

> * ERROR: TO must specify exactly one value per partitioning column
> should say something more like "you specified one column value,
> whereas the partitioning key requires two columns"

Should that be a DETAIL or HINT message?

> Good work so far, but there is still a very significant amount of work
> to do. And as this feature evolves it must now contain full
> documentation at every step, otherwise it won't be able to receive
> full and fair review. So please make sure each new patch contains docs
> changes for that patch.

Agreed that comprehensive documentation of any new feature is crucial both
during development and after the feature is released.

Thanks,
Amit