On Wed, Nov 2, 2016 at 3:41 AM, Amit Langote <langote_amit...@lab.ntt.co.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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers