Re: Declarative partitioning - another take

On Wed, Nov 2, 2016 at 3:41 AM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> As for which parts of the system need to know about these implicit
> partition constraints to *enforce* them for data integrity, we could say
> that it's really just one site - ExecConstraints() called from
> ExecInsert()/ExecUpdate().

Well, that's a slightly optimistic view of the situation, because the
issue is whether ExecConstraints() is going to get called in the first
place. But now that I look at it, there are only a handful of
callers, so maybe it's not so bad.

> Admittedly, the current error message style as in this patch exposes the
> implicit constraint approach to a certain criticism: "ERROR: new row
> violates the partition boundary specification of \"%s\"". It would say
> the following if it were a named constraint: "ERROR: new row for relation
> \"%s\" violates check constraint \"%s\""
>
> For constraint exclusion optimization, we teach get_relation_constraints()
> to look at these constraints. Although greatly useful, it's not the case
> of being absolutely critical.
>
> Beside the above two cases, there is bunch of code (relcache, DDL) that
> looks at regular constraints, but IMHO, we need not let any of that code
> concern itself with the implicit partition constraints. Especially, I
> wonder why the client tools should want the implicit partitioning
> constraint to be shown as a CHECK constraint? As the proposed patch 0004
> (psql) currently does, isn't it better to instead show the partition
> bounds as follows?
>
> +CREATE TABLE part_b PARTITION OF parted (
> + b WITH OPTIONS NOT NULL DEFAULT 1 CHECK (b >= 0),
> + CONSTRAINT check_a CHECK (length(a) > 0)
> +) FOR VALUES IN ('b');

Good point.

> +\d part_b
> + Table "public.part_b"
> + Column | Type | Modifiers
> +--------+---------+--------------------
> + a | text |
> + b | integer | not null default 1
> +Partition of: parted FOR VALUES IN ('b')
> +Check constraints:
> + "check_a" CHECK (length(a) > 0)
> + "part_b_b_check" CHECK (b >= 0)
>
> Needless to say, that could save a lot of trouble thinking about
> generating collision-free names of these constraints, their dependency
> handling, unintended altering of these constraints, pg_dump, etc.

Well, that's certainly true, but those problems don't strike me as so
serious that they couldn't be solved with a reasonable amount of
labor.

>> In fact, as far as I can see, the only advantage of this approach is
>> that when the insert arrives through the parent and is routed to the
>> child by whatever tuple-routing code we end up with (I guess that's
>> what 0008 does), we get to skip checking the constraint, saving CPU
>> cycles. That's probably an important optimization, but I don't think
>> that putting the partitioning constraint in the catalog in any way
>> rules out the possibility of performing that optimization. It's just
>> that instead of having the partitioning excluded-by-default and then
>> sometimes choosing to include it, you'll have it included-by-default
>> and then sometimes choose to exclude it.
>
> Hmm, doesn't it seem like we would be making *more* modifications to the
> existing code (backend or otherwise) to teach it about excluding the
> implicit partition constraints than the other way around? The other way
> around being to modify the existing code to include the implicit
> constraints which is what this patch is about.

I'm not sure which way is actually going to be more code modification,
but I guess you've persuaded me that the way you have it is
reasonable, so let's stick with that.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company