Re: Exclusion constraints on partitioned tables

On Thu, Jul 6, 2023 at 1:03 AM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
> This looks all pretty good to me. A few more comments:

Thanks for the feedback! New patch attached here. Responses below:

> It seems to me that many of the test cases added in indexing.sql are
> redundant with create_table.sql/alter_table.sql (or vice versa). Is
> there a reason for this?

Yes, there is some overlap. I think that's just because there was
overlap before, and I didn't want to delete the old tests completely.
But since indexing.sql has a fuller list of tests and is a superset of
the others, this new patch removes the redundant tests from
{create,alter}_table.sql.

Btw speaking of tests, I want to make sure this new feature will still
work when you're using btree_gist and and `EXCLUDE WITH (myint =,
mytsrange &&)` (and not just `(myint4range =, mytsrange &&)`). Some of
my early attempts writing this patch worked w/o btree_gist but not w/
(or vice versa). But as far as I know there's no way to test that in
regress. I wound up writing a private shell script that just does
this:

```
--------
-- test against btree_gist since we can't do that in the postgres
regress test suite:

CREATE EXTENSION btree_gist;

create table partitioned (id int, valid_at tsrange, exclude using gist
(id with =, valid_at with &&)) partition by range (id);
-- should fail with a good error message:
create table partitioned2 (id int, valid_at tsrange, exclude using
gist (id with <>, valid_at with &&)) partition by range (id);
```

Is there some place in the repo to include a test like that? It seems
a little funny to put it in the btree_gist suite, but maybe that's the
right answer.

> This is not really a problem in your patch, but I think in
>
> - if (partitioned && (stmt->unique || stmt->primary))
> + if (partitioned && (stmt->unique || stmt->primary ||
> stmt->excludeOpNames != NIL))
>
> the stmt->primary is redundant and should be removed. Right now
> "primary" is always a subset of "unique", but presumably a future patch
> of yours wants to change that.

Done! I don't think my temporal work changes that primary ⊆ unique. It
does change that some primary/unique constraints will have non-null
excludeOpNames, which will require small changes here eventually. But
that should be part of the temporal patches, not this one.

> Furthermore, I think it would be more elegant in your patch if you wrote
> stmt->excludeOpNames without the "== NIL" or "!= NIL", so that it
> becomes a peer of stmt->unique. (I understand some people don't like
> that style. But it is already used in that file.)

Done.

> I would consider rearranging some of the conditionals more as a
> selection of cases, like "is it a unique constraint?", "else, is it an
> exclusion constraint?" -- rather than the current "is it an exclusion
> constraint?, "else, various old code".

Done.

> Also, I would push the code
>
> if (accessMethodId == BTREE_AM_OID)
> eq_strategy = BTEqualStrategyNumber;
>
> further down into the loop, so that you don't have to remember in which
> cases eq_strategy is assigned or not.
>
> (It's also confusing that the eq_strategy variable is used for two
> different things in this function, and that would clean that up.)

Agreed that it's confusing. Done.

> Finally, this code
>
> + att = TupleDescAttr(RelationGetDescr(rel),
> + key->partattrs[i] - 1);
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("cannot match partition key
> to index on column \"%s\" using non-equal operator \"%s\".",
> + NameStr(att->attname),
> get_opname(indexInfo->ii_ExclusionOps[j]))));
>
> could be simplified by using get_attname().

Okay, done. I changed the similar error message just below too.

> This is all just a bit of polishing. I think it would be good to go
> after that.

Thanks!

--
Paul ~{:-)
pj(at)illuminatedcomputing(dot)com