Re: Collation versioning

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!