Feedback-ID: ie0a040ee:Fastmail
Message-ID: <7be8724a-5c25-46d7-8325-1bd8be6fa523@eisentraut.org>
Date: Thu, 11 Jan 2024 15:44:50 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: SQL:2011 application time
Content-Language: en-US
To: Paul Jungwirth <pj@illuminatedcomputing.com>,
 jian he <jian.universality@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <CA+renyUApHgSZF9-nd-a0+OPGharLQLO=mDHcY4_qQ0+noCUVg@mail.gmail.com>
 <b8ef855d-216b-5499-6a6e-ec883a32bd96@illuminatedcomputing.com>
 <CACJufxGnN6-513copYkpgJia6Qq7vKtO6q7cigUpA=4Evozppg@mail.gmail.com>
 <CACJufxGsjGXbKT-qbPYtspLxN74B8Q8dBXRZV+HfgqC0hh-ZNQ@mail.gmail.com>
 <CACJufxGt4oYfa7UN2eAXymgOKH1irbhC5eY_oNkkdj30w-fefw@mail.gmail.com>
 <CACJufxExyO_mN8-OkgaTdgqmE5OfE=RBoQXizf=8uRj4c-pAYw@mail.gmail.com>
 <CACJufxGeUsmxOhnd2760kBv-hUWRwCy7A0s45pGSPTe8DJVTGw@mail.gmail.com>
 <2d9740ad-3bb7-473c-8441-344351caa8ee@illuminatedcomputing.com>
 <fd41e772-296e-43e9-a70e-2e27bb8b1cd5@eisentraut.org>
 <af62a16b-9f88-4746-8e87-fdf0c242b266@illuminatedcomputing.com>
 <a37f180d-b54b-46d6-9e52-cc15f32be200@eisentraut.org>
 <596fefdd-6202-4228-b7c6-f240b1cbe06e@eisentraut.org>
 <df018b96-f0dc-49ba-98af-3a3983f683fe@illuminatedcomputing.com>
 <35400587-6865-4a2c-9ea4-4761669478ea@eisentraut.org>
 <71b45adb-b496-4e58-b134-0a091014e63f@illuminatedcomputing.com>
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: <71b45adb-b496-4e58-b134-0a091014e63f@illuminatedcomputing.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/7be8724a-5c25-46d7-8325-1bd8be6fa523%40eisentraut.org>
Precedence: bulk

On 31.12.23 09:51, Paul Jungwirth wrote:
> On Wed, Dec 6, 2023 at 12:59 AM Peter Eisentraut <peter@eisentraut.org> 
> wrote:
>  >
>  > On 02.12.23 19:41, Paul Jungwirth wrote:
>  > > So what do you think of this idea instead?:
>  > >
>  > > We could add a new (optional) support function to GiST that translates
>  > > "well-known" strategy numbers into the opclass's own strategy numbers.
>  >
>  > I had some conversations about this behind the scenes.  I think this
>  > idea makes sense.
> 
> Here is a patch series with the GiST stratnum support function added. I 
> put this into a separate patch (before all the temporal ones), so it's 
> easier to review. Then in the PK patch (now #2) we call that function to 
> figure out the = and && operators. I think this is a big improvement.

I like this solution.

Here is some more detailed review of the first two patches.  (I reviewed 
v20; I see you have also posted v21, but they don't appear very 
different for this purpose.)

v20-0001-Add-stratnum-GiST-support-function.patch

* contrib/btree_gist/Makefile

Needs corresponding meson.build updates.

* contrib/btree_gist/btree_gist--1.7--1.8.sql

Should gist_stratnum_btree() live in contrib/btree_gist/ or in core?
Are there other extensions that use the btree strategy numbers for
gist?

+ALTER OPERATOR FAMILY gist_vbit_ops USING gist ADD
+   FUNCTION  12 (varbit, varbit) gist_stratnum_btree (int2) ;

Is there a reason for the extra space after FUNCTION here (repeated
throughout the file)?

+-- added in 1.4:

What is the purpose of these "added in" comments?


v20-0002-Add-temporal-PRIMARY-KEY-and-UNIQUE-constraints.patch

* contrib/btree_gist/Makefile

Also update meson.build.

* contrib/btree_gist/sql/without_overlaps.sql

Maybe also insert a few values, to verify that the constraint actually
does something?

* doc/src/sgml/ref/create_table.sgml

Is "must have a range type" still true?  With the changes to the
strategy number mapping, any type with a supported operator class
should work?

* src/backend/utils/adt/ruleutils.c

Is it actually useful to add an argument to
decompile_column_index_array()?  Wouldn't it be easier to just print
the " WITHOUT OVERLAPS" in the caller after returning from it?

* src/include/access/gist_private.h

The added function gistTranslateStratnum() isn't really "private" to
gist.  So access/gist.h would be a better place for it.

Also, most other functions there appear to be named "GistSomething",
so a more consistent name might be GistTranslateStratnum.

* src/include/access/stratnum.h

The added StrategyIsValid() doesn't seem that useful?  Plenty of
existing code just compares against InvalidStrategy, and there is only
one caller for the new function.  I suggest to do without it.

* src/include/commands/defrem.h

We are using two terms here, well-known strategy number and canonical
strategy number, to mean the same thing (I think?).  Let's try to
stick with one.  Or explain the relationship?


If these points are addressed, and maybe with another round of checking 
that all corner cases are covered, I think these patches (0001 and 0002) 
are close to ready.