Re: Declarative partitioning - another take

On 2016/11/28 23:42, Ashutosh Bapat wrote:
> Here are some comments on 0003 patch.

Thanks for the review!

> 1. In ALTER TABLE documentation we should refer to CREATE TABLE documentation
> for partition_bound_spec syntax, similar ADD table_constraint note.

That makes sense, done.

>
> 2. I think, "of the target table" is missing after all the ... constraints
> + match. Also, it must have all the <literal>NOT NULL</literal> and
> + <literal>CHECK</literal> constraints present in the target table.

The "present in the target table" part at the end of the sentence is
supposed to mean just that, but I changed it to say "of the target table"
instead, replacing "present in" by "of".

> 3. I think, using "any of the" instead of "some" is preferred in the following
> sentence. In the following sentence, we should use "such a constraint" instead
> of "constraints" to avoid mixed singular and plural usage.
> + If some <literal>CHECK</literal> constraint of the table being attached

OK, done.

> 4. This construction is ambiguous. "are not considered" - considered where? for
> what? Constraints on which object?
> + clause. Currently <literal>UNIQUE</literal>, <literal>PRIMARY
> KEY</literal>,
> + and <literal>FOREIGN KEY</literal> constraints are not considered.

That is essentially the same sentence as last sentence in the description
of INHERITS. But I moved it next to: "Also, it must have all the NOT NULL
and CHECK constraints present in the target table." Hopefully, that makes
it less ambiguous. It's supposed to mean that those constraints are not
considered for inheritance unlike NOT NULL and CHECK constraints.

> 5. What is a partition constraint? Do we define that term anywhere in the
> documentation? If not we should define that term and then use it here.
> + A full table scan is performed on the table being attached to check that
> + no existing row in the table violates the partition constraint. It is

Partition constraint is an implicit constraint derived from the parent's
partition key and its partition bounds enforced whenever an action is
applied to a partition directly - such as, inserting/updating a row and
when attaching a table as partition. In fact, it also comes into play
during constraint exclusion when selecting from the parent table.

Do you think it's better to mention something like the above in the
description of CREATE TABLE PARTITION OF?

> 6. The paragraph following
> + A full table scan is performed on the table
> being attached to check that seems to be confusing. It says that the
> potentially expensive scan can be avoided by adding a constraint equivalent to
> partition constraint. That seems to be wrong. The table will be scanned anyway,
> when adding a valid constraint per ALTER TABLE ADD table_constraint
> documentation. Instead you might want to say that the table scan will not be
> carried out if the existing constraints imply the partition constraints.

Ah, I see the confusion. What's written is not supposed to mean that the
constraint be added after attaching the table as a partition, it's before.
It's right that the table will be scanned anyway to validate the
constraint, but at that point, it's not related to the partitioned table
in any way and hence does not affect it. As soon as the ATTACH PARTITION
command is executed, an exclusive lock will be acquired on the
parent/partitioned table, during which it's better to avoid any actions
that will take long - such as the validation scan. If the constraint
added *beforehand* to the table being attached is such that it would allow
only a subset or a strict subset of the rows that the partition constraint
will allow, then it is not necessary to *repeat* the scan and hence is
not. If it's not necessarily true that the constraints of the table being
attached will only allow a subset of a strict subset of the rows that the
partition constraint will allow, then the validation scan cannot be
avoided. IOW, the advice is to help avoid the scan while taking an
exclusive lock on the parent.

Anyway, I have modified some sentences to be slightly clearer, see if that
works for you.

> 7. You might want to specify the fate of range or lists covered by the
> partition being detached in DETACH PARTITION section.

The partitioned table simply ceases to accept the rows falling in such a
range or a list. The detached partition turn into a standalone table
which happens to contain only the rows with values of certain columns
(partition keys) within the range or the list. However, there is no
explicit (catalogued) check constraint to that effect on the newly
standalone table, if that's what you were asking.

> 8. Since partition bound specification is more than a few words, you might want
> to refer to the actual syntax in CREATE TABLE command.
> + <term><replaceable
> class="PARAMETER">partition_bound_spec</replaceable></term>
> + <listitem>
> + <para>
> + The partition bound specification for a new partition.
> + </para>
> + </listitem>
> + </varlistentry>

OK, added a link to the CREATE TABLE page.

>
> 9. I think, we need to use "may be scanned" instead of "is scanned", given that
> some other part of the documentation talks about "avoiding" the table scan. May
> be refer to that documentation to clarify the uncertainty.
> + Similarly, when attaching a new partition it is scanned to verify that

Agreed, done.

> 10. The same paragraph talks about multiple ALTER TABLE commands being run
> together to avoid multiple table rewrites. Do we want to document whether
> ATTACH/DETACH partition can be run with any other command or not.

While currently ATTACH/DETACH can be run with any other ALTER TABLE
sub-command (much as ALTER TABLE INHERIT can), I think it's perhaps better
to prevent that. For example, following awkward-sounding thing happens:

alter table p attach partition p1 for values in (1, 2, 3), add b int;
ERROR: child table is missing column "b"

I have modified the patch so that the syntax does not allow such
combination of sub-commands. Also, moved ATTACH PARTITION and DETACH
PARTITION in the synopsis section from "where action is one of:" area to
above where various forms of ALTER TABLE are listed. Also specified down
below that ATTACH/DETACH PARTITION cannot be run in parallel with other
alterations.

> 11. ATTACH partition documentation is not talking about the unlogged or
> temporary tables being attached to is logged or non-temporary. What is current
> code doing about these cases? Do we allow such mixed partition hierarchy?

I tried to match the behavior of CREATE TABLE INHERITS() and ALTER TABLE
INHERIT

In ALTER TABLE child INHERIT parent case (as in CREATE TABLE child()
INHERITS(parent) case), failure occurs if the child is permanent rel and
parent temporary or if both are temporary, but if either parent or child
is of another session. The same happens in case of ALTER TABLE parent
ATTACH PARTITION child and CREATE TABLE child PARTITION OF parent,
respectively.

LOGGED-ness is not considered here at all.

Attached updated patches.

Thanks,
Amit