Re: SQL:2011 application time

On Thu, Jan 11, 2024 at 10:44 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> 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.

fixed

>
> * 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)?
>

fixed.

> +-- 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.

fixed.

> * contrib/btree_gist/sql/without_overlaps.sql
>
> Maybe also insert a few values, to verify that the constraint actually
> does something?
>

I added an ok and failed INSERT.

> * 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?

fixed. i just print it right after decompile_column_index_array.

> * 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.
>

If more StrategyNumber are used in the future, will StrategyIsValid()
make sense?

> * 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?
>

In my words:
for range type, well-known strategy number and canonical strategy
number are the same thing.
For types Gist does not natively support equality, like int4,
GetOperatorFromCanonicalStrategy will pass RTEqualStrategyNumber from
ComputeIndexAttrs
and return BTEqualStrategyNumber.

> 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.
>

the following are my review:

+ /* exclusionOpNames can be non-NIL if we are creating a partition */
+ if (iswithoutoverlaps && exclusionOpNames == NIL)
+ {
+ indexInfo->ii_ExclusionOps = palloc_array(Oid, nkeycols);
+ indexInfo->ii_ExclusionProcs = palloc_array(Oid, nkeycols);
+ indexInfo->ii_ExclusionStrats = palloc_array(uint16, nkeycols);
+ }
I am not sure the above comment is related to the code

+/*
+ * Returns the btree number for equals, otherwise invalid.
+ *
+ * This is for GiST opclasses in btree_gist (and maybe elsewhere)
+ * that use the BT*StrategyNumber constants.
+ */
+Datum
+gist_stratnum_btree(PG_FUNCTION_ARGS)
+{
+ StrategyNumber strat = PG_GETARG_UINT16(0);
+
+ switch (strat)
+ {
+ case RTEqualStrategyNumber:
+ PG_RETURN_UINT16(BTEqualStrategyNumber);
+ case RTLessStrategyNumber:
+ PG_RETURN_UINT16(BTLessStrategyNumber);
+ case RTLessEqualStrategyNumber:
+ PG_RETURN_UINT16(BTLessEqualStrategyNumber);
+ case RTGreaterStrategyNumber:
+ PG_RETURN_UINT16(BTGreaterStrategyNumber);
+ case RTGreaterEqualStrategyNumber:
+ PG_RETURN_UINT16(BTGreaterEqualStrategyNumber);
+ default:
+ PG_RETURN_UINT16(InvalidStrategy);
+ }
the above comment seems not right?
even though currently strat will only be RTEqualStrategyNumber.

+void
+GetOperatorFromCanonicalStrategy(Oid opclass,
+ Oid atttype,
+ const char *opname,
+ Oid *opid,
+ StrategyNumber *strat)
+{
+ Oid opfamily;
+ Oid opcintype;
+ StrategyNumber opstrat = *strat;
+
+ *opid = InvalidOid;
+
+ if (get_opclass_opfamily_and_input_type(opclass,
+ &opfamily,
+ &opcintype))
+ {
+ /*
+ * Ask the opclass to translate to its internal stratnum
+ *
+ * For now we only need GiST support, but this could support
+ * other indexams if we wanted.
+ */
+ *strat = gistTranslateStratnum(opclass, opstrat);
+ if (!StrategyIsValid(*strat))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("no %s operator found for WITHOUT OVERLAPS constraint", opname),
+ errdetail("Could not translate strategy number %u for opclass %d.",
+ opstrat, opclass),
+ errhint("Define a stratnum support function for your GiST opclass.")));
+
+ *opid = get_opfamily_member(opfamily, opcintype, opcintype, *strat);
+ }
+
+ if (!OidIsValid(*opid))
+ {
+ HeapTuple opftuple;
+ Form_pg_opfamily opfform;
+
+ /*
+ * attribute->opclass might not explicitly name the opfamily,
+ * so fetch the name of the selected opfamily for use in the
+ * error message.
+ */
+ opftuple = SearchSysCache1(OPFAMILYOID,
+   ObjectIdGetDatum(opfamily));
+ if (!HeapTupleIsValid(opftuple))
+ elog(ERROR, "cache lookup failed for opfamily %u",
+ opfamily);
+ opfform = (Form_pg_opfamily) GETSTRUCT(opftuple);
+
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("no %s operator found for WITHOUT OVERLAPS constraint", opname),
+ errdetail("There must be an %s operator within opfamily \"%s\" for
type \"%s\".",
+   opname,
+   NameStr(opfform->opfname),
+   format_type_be(atttype))));
+ }
+}
I refactored this function.
GetOperatorFromCanonicalStrategy called both for normal and WITHOUT OVERLAPS.
so errmsg("no %s operator found for WITHOUT OVERLAPS constraint",
opname) would be misleading
for columns without "WITHOUT OVERLAPS".
Also since that error part was deemed unreachable, it would make the
error verbose, I guess.

--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2379,6 +2379,10 @@ describeOneTableDetails(const char *schemaname,
  else
  appendPQExpBufferStr(&buf, ", false AS indisreplident");
  appendPQExpBufferStr(&buf, ", c2.reltablespace");
+ if (pset.sversion >= 170000)
+ appendPQExpBufferStr(&buf, ", con.conwithoutoverlaps");
+ else
+ appendPQExpBufferStr(&buf, ", false AS conwithoutoverlaps");

I don't know how to verify it.
I think it should be:
+ if (pset.sversion >= 170000)
+      appendPQExpBufferStr(&buf, ", con.conwithoutoverlaps");

I refactored the 0002 commit message.
The original commit message seems outdated.
I put all the related changes into one attachment.