Re: Exclusion constraints on partitioned tables
От | Paul A Jungwirth |
---|---|
Тема | Re: Exclusion constraints on partitioned tables |
Дата | |
Msg-id | CA+renyVbDFMwhsECw4LkyfCFULk=8SKnMDDe70Wyw661EC_xkA@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Exclusion constraints on partitioned tables (Peter Eisentraut <peter@eisentraut.org>) |
Ответы |
Re: Exclusion constraints on partitioned tables
|
Список | pgsql-hackers |
On Thu, Jul 6, 2023 at 1:03 AM Peter Eisentraut <peter@eisentraut.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@illuminatedcomputing.com
Вложения
В списке pgsql-hackers по дате отправления:
Предыдущее
От: David RowleyДата:
Сообщение: Re: Check lateral references within PHVs for memoize cache keys
Следующее
От: Zhang MingliДата:
Сообщение: Re: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns