Re: Collation versioning - Mailing list pgsql-hackers
From | Julien Rouhaud |
---|---|
Subject | Re: Collation versioning |
Date | |
Msg-id | CAOBaU_b1nmVh4gbj4DG_+=o6cgNqOS_EvJTsGEp4M+DDB1rMtA@mail.gmail.com Whole thread Raw |
In response to | Re: Collation versioning (Thomas Munro <thomas.munro@gmail.com>) |
Responses |
Re: Collation versioning
|
List | pgsql-hackers |
On Thu, Oct 22, 2020 at 8:00 PM Thomas Munro <thomas.munro@gmail.com> wrote: > > On Thu, Sep 24, 2020 at 9:49 PM Julien Rouhaud <rjuju123@gmail.com> wrote: > > On Sun, Sep 20, 2020 at 10:24:26AM +0800, Julien Rouhaud wrote: > > > On the other hand the *_pattern_ops are entirely hardcoded, and I > > > don't think that we'll ever have an extensible way to have this kind > > > of magic exception. So maybe having a flag at the am level is > > > acceptable? > > > > Hearing no complaint, I kept the flag at the AM level and added hardcoded > > exceptions for the *_pattern_ops opclasses to avoid false positive as much as > > possible, and no false negative (at least that I'm aware of). I added many > > indexes to the regression tests to make sure that all the cases are correctly > > handled. > > > > Unfortunately, there's still one case that can't be fixed easily. Here's an > > example of such case: > > > > CREATE INDEX ON sometable ((collatable_col || collatable_col) text_pattern_ops) > > I think we should try to get the basic feature into the tree, and then > look at these kinds of subtleties as later improvements. Agreed. > Here's a new > version with the following changes: > > 1. Update the doc patch to mention that this stuff now works on > Windows too (see commit 352f6f2d). > 2. Drop non_deterministic_only argument for from GetTypeCollations(); > it was unused. > 3. Drop that "stable collation order" field at the AM level for now. > This means that we'll warn you about collation versions changes for > hash and bloom indexes, even when it's technically unnecessary, for > now. > > The pattern ops stuff seems straightforward however, so let's keep > that bit in the initial commit of the feature. That's a finite set of > hard coded op classes which exist specifically to ignore collations. Thanks a lot! I'm fine with all the changes. The "stable collation order" part would definitely benefit from more thoughts, so it's good if we can focus on that later. While reviewing the changes, I found a couple of minor issues (inherited from the previous versions). It's missing a free_objects_addresses in recordDependencyOnCollations, and there's a small typo. Inline diff: diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c index cab552eb32..4680b4e538 100644 --- a/src/backend/catalog/dependency.c +++ b/src/backend/catalog/dependency.c @@ -1674,6 +1674,8 @@ recordDependencyOnCollations(ObjectAddress *myself, eliminate_duplicate_dependencies(addrs); recordMultipleDependencies(myself, addrs->refs, addrs->numrefs, DEPENDENCY_NORMAL, record_version); + + free_object_addresses(addrs); } /* diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 69978fb409..048a41f446 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -1154,7 +1154,7 @@ index_create(Relation heapRelation, colls_pattern_ops = list_difference_oid(colls_pattern_ops, colls); /* - * Record the dependencies for collation declares with any of the + * Record the dependencies for collation declared with any of the * *_pattern_ops opclass, without version tracking. */ if (colls_pattern_ops != NIL) Other than that it all looks good to me!
pgsql-hackers by date: