Re: operator exclusion constraints

On Sun, Nov 8, 2009 at 4:41 PM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> On Sat, 2009-11-07 at 10:56 -0800, Jeff Davis wrote:
>> EXCLUDE probably flows most nicely with the optional USING clause or
>> without. My only complaint was that it's a transitive verb, so it seems
>> to impart more meaning than it actually can. I doubt anyone would
>> actually be more confused in practice, though. If a couple of people
>> agree, I'll change it to EXCLUDE.
>
> It looks like EXCLUDE is the winner. Updated patch attached.
>
> The feature is still called "operator exclusion constraints", and the
> docs still make reference to that name, but the syntax specification has
> been updated.

[ reviewing ]

First thoughts:

I think the create_table documentation gets into a little too much
gorey detail. I'm willing to take a pass at improving it, if you'd
like, but generally I think it should avoid discussion of
implementation details. For example, saying that it's not as fast as
a UNIQUE constraint is good; the fact that an extra index lookup is
involved is probably overkill. Incidentally, the wording in the
first paragraph fails to take into account the possibility that there
are multiple operators.

In index_create(), the elog() where relopxconstraints < 0 should just
complain about the value being negative, I think, rather than listing
the value. If you just say the value is -3, it doesn't give the user
a clue why that's bad.

There is a spurious diff hunk for reindex_relation().

In ATRewriteTable() you reindent a bunch of variable declarations;
pg_indent will undo this, so you should nix this part.

In ATAddOperatorExclusionConstraint(), the message "method %s does not
support gettuple" seems a bit user-unfriendly. Can we explain the
problem by referring to the functionality of getttuple(), rather than
the name of it?

I don't really like this message, for a number of reasons.

alter table foo add constraint bar exclude (a check with =, b check with =);
ERROR: operator exclusion constraint violation detected: "foo_a_exclusion"
DETAIL: Tuple "(1, 1, 2)" conflicts with existing tuple "(1, 1, 3)".

The corresponding error for a UNIQUE index is: could not create unique
index "bar", which I like better. Only the relevant columns from the
tuples are dumped, and the tuple is not surrounded by double quotes;
any reason not to parallel that here? Also, the message is all
lower-case. Similarly, for an insert/update situation, it seems that
a message like "key value violates exclusion constraint \"%s\"" would
be better than the existing message.

alter table X add constraint Y exclude (...) seems to ignore the value
of Y and create both the constraint and the index with a name of its
own choosing.

As a quick performance test, I inserted a million 3-integer tuples
into a 3-column table with a unique constraint or an operator
exclusion constraint over all three columns. The former took ~ 15 s,
the latter ~ 21.5 s. That seems acceptable.

I think preprocessOpExConstraints should be called
transformOpxConstraints - opx rather than opex because that's what you
most frequently use elsewhere in the patch, and transform rather than
preprocess for consistency with other, similar functions.

In ruleutils.c, the prototype for pg_get_opxdef_worker() has a small
whitespace inconsistency relative to the surrounding declarations.

I haven't really grokked the substantive things that this patch is
doing yet - these are just preliminary comments based on a quick
read-through. I'll write more after I have a chance to look at it in
more detail.

...Robert