On Thu, Jan 11, 2024 at 10:44 PM Peter Eisentraut <pe...@eisentraut.org> wrote: > > On 31.12.23 09:51, Paul Jungwirth wrote: > > On Wed, Dec 6, 2023 at 12:59 AM Peter Eisentraut <pe...@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.
v1-0001-minor-refactor.no-cfbot
Description: Binary data
