pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feed[PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
25+ messages / 6 participants
[nested] [flat]
Commits from this thread:
2024-10-22 774171c4f6 Improve reporting of errors in extension script files.
2024-10-22 14e5680eee Improve parser's reporting of statement start locations.
2023-10-20 2b5154beab Extend ALTER OPERATOR to allow setting more optimization attributes.
* [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
@ 2023-06-22 16:35 Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 25+ messages in thread
From: Tommy Pavlicek @ 2023-06-22 16:35 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Tomas Vondra <tomas.vondra@enterprisedb.com>
Hi All,
I've attached a couple of patches to allow ALTER OPERATOR to add
commutators, negators, hashes and merges to operators that lack them.
The need for this arose adding hash functions to the ltree type after the
operator had been created without hash support[1]. There are potential
issues with modifying these attributes that have been discussed
previously[2], but I understand that setting them, if they have not been
set before, is ok.
I belatedly realised that it may not be desirable or necessary to allow
adding commutators and negators in ALTER OPERATOR because the linkage can
already be added when creating a new operator. I don't know what's best, so
I thought I'd post this here and get feedback before removing anything.
The first patch is create_op_fixes_v1.patch and it includes some
refactoring in preparation for the ALTER OPERATOR changes and fixes a
couple of minor bugs in CREATE OPERATOR:
- prevents self negation when filling in/creating an existing shell operator
- remove reference to sort operator in the self negation error message as
the sort attribute appears to be deprecated in Postgres 8.3
The second patch is alter_op_v1.patch which contains the changes to ALTER
OPERATOR and depends on create_op_fixes_v1.patch.
Additionally, I wasn't sure whether it was preferred to fail or succeed on
ALTERs that have no effect, such as adding hashes on an operator that
already allows them or disabling hashes on one that does not. I chose to
raise an error when this happens, on the thinking it was more explicit and
made the code simpler, even though the end result would be what the user
wanted.
Comments appreciated.
Thanks,
Tommy
[1]
https://www.postgresql.org/message-id/flat/CAEhP-W9ZEoHeaP_nKnPCVd_o1c3BAUvq1gWHrq8EbkNRiS9CvQ%40mai...
[2] https://www.postgresql.org/message-id/flat/3348985.V7xMLFDaJO@dinodell
Attachments: download all (2 files, .tar.gz)
alter_op_v1.patch (application/octet-stream, 29.7K) download
diff --git a/doc/src/sgml/ref/alter_operator.sgml b/doc/src/sgml/ref/alter_operator.sgml
index a4a1af564f..04af611cca 100644
--- a/doc/src/sgml/ref/alter_operator.sgml
+++ b/doc/src/sgml/ref/alter_operator.sgml
@@ -29,8 +29,12 @@ ALTER OPERATOR <replaceable>name</replaceable> ( { <replaceable>left_type</repla
ALTER OPERATOR <replaceable>name</replaceable> ( { <replaceable>left_type</replaceable> | NONE } , <replaceable>right_type</replaceable> )
SET ( { RESTRICT = { <replaceable class="parameter">res_proc</replaceable> | NONE }
- | JOIN = { <replaceable class="parameter">join_proc</replaceable> | NONE }
- } [, ... ] )
+ | JOIN = { <replaceable class="parameter">join_proc</replaceable> | NONE }
+ | COMMUTATOR = <replaceable class="parameter">com_op</replaceable>
+ | NEGATOR = <replaceable class="parameter">neg_op</replaceable>
+ | HASHES
+ | MERGES
+ } [, ... ] )
</synopsis>
</refsynopsisdiv>
@@ -121,6 +125,42 @@ ALTER OPERATOR <replaceable>name</replaceable> ( { <replaceable>left_type</repla
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><replaceable class="parameter">com_op</replaceable></term>
+ <listitem>
+ <para>
+ The commutator of this operator. Can only be set if the operator does not have an existing commutator.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><replaceable class="parameter">neg_op</replaceable></term>
+ <listitem>
+ <para>
+ The negator of this operator. Can only be set if the operator does not have an existing negator.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>HASHES</literal></term>
+ <listitem>
+ <para>
+ Indicates this operator can support a hash join. Can only be set when the operator does not support a hash join.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>MERGES</literal></term>
+ <listitem>
+ <para>
+ Indicates this operator can support a merge join. Can only be set when the operator does not support a merge join.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</refsect1>
diff --git a/src/backend/commands/operatorcmds.c b/src/backend/commands/operatorcmds.c
index cd7f83136f..3787c234d4 100644
--- a/src/backend/commands/operatorcmds.c
+++ b/src/backend/commands/operatorcmds.c
@@ -54,6 +54,7 @@
static Oid ValidateRestrictionEstimator(List *restrictionName);
static Oid ValidateJoinEstimator(List *joinName);
+static void ValidateBooleanIsTrue(DefElem *def, const char *paramName);
/*
* DefineOperator
@@ -359,6 +360,18 @@ ValidateJoinEstimator(List *joinName)
return joinOid;
}
+static void
+ValidateBooleanIsTrue(DefElem *def, const char *paramName)
+{
+ if (!defGetBoolean(def))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("alter operator cannot disable \"%s\"",
+ paramName)));
+ }
+}
+
/*
* Guts of operator deletion.
*/
@@ -426,6 +439,12 @@ AlterOperator(AlterOperatorStmt *stmt)
List *joinName = NIL; /* optional join sel. function */
bool updateJoin = false;
Oid joinOid;
+ List *commutatorName = NIL; /* optional commutator operator name */
+ Oid commutatorOid;
+ List *negatorName = NIL; /* optional negator operator name */
+ Oid negatorOid;
+ bool updateHashes = false;
+ bool updateMerges = false;
/* Look up the operator */
oprId = LookupOperWithArgs(stmt->opername, false);
@@ -456,6 +475,24 @@ AlterOperator(AlterOperatorStmt *stmt)
joinName = param;
updateJoin = true;
}
+ else if (strcmp(defel->defname, "commutator") == 0)
+ {
+ commutatorName = defGetQualifiedName(defel);
+ }
+ else if (strcmp(defel->defname, "negator") == 0)
+ {
+ negatorName = defGetQualifiedName(defel);
+ }
+ else if (strcmp(defel->defname, "hashes") == 0)
+ {
+ ValidateBooleanIsTrue(defel, "hashes");
+ updateHashes = true;
+ }
+ else if (strcmp(defel->defname, "merges") == 0)
+ {
+ ValidateBooleanIsTrue(defel, "merges");
+ updateMerges = true;
+ }
/*
* The rest of the options that CREATE accepts cannot be changed.
@@ -464,11 +501,7 @@ AlterOperator(AlterOperatorStmt *stmt)
else if (strcmp(defel->defname, "leftarg") == 0 ||
strcmp(defel->defname, "rightarg") == 0 ||
strcmp(defel->defname, "function") == 0 ||
- strcmp(defel->defname, "procedure") == 0 ||
- strcmp(defel->defname, "commutator") == 0 ||
- strcmp(defel->defname, "negator") == 0 ||
- strcmp(defel->defname, "hashes") == 0 ||
- strcmp(defel->defname, "merges") == 0)
+ strcmp(defel->defname, "procedure") == 0)
{
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -488,7 +521,7 @@ AlterOperator(AlterOperatorStmt *stmt)
NameStr(oprForm->oprname));
/*
- * Look up restriction and join estimators if specified
+ * Look up Oid for any parameters specified
*/
if (restrictionName)
restrictionOid = ValidateRestrictionEstimator(restrictionName);
@@ -499,28 +532,77 @@ AlterOperator(AlterOperatorStmt *stmt)
else
joinOid = InvalidOid;
- /* Perform additional checks, like OperatorCreate does */
- if (!(OidIsValid(oprForm->oprleft) && OidIsValid(oprForm->oprright)))
+ if (commutatorName)
{
- /* If it's not a binary op, these things mustn't be set: */
- if (OidIsValid(joinOid))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only binary operators can have join selectivity")));
+ commutatorOid = OperatorGetOrCreateValidCommutator(commutatorName,
+ oprForm->oid,
+ oprForm->oprname.data,
+ oprForm->oprnamespace,
+ oprForm->oprleft,
+ oprForm->oprright);
+
+ /*
+ * we don't need to do anything extra for a self commutator as in
+ * OperatorCreate as there we have to create the operator before
+ * setting the commutator, but here the operator already exists and
+ * the commutatorOid above is valid (and is the operator oid).
+ */
}
+ else
+ commutatorOid = InvalidOid;
+
+ if (negatorName)
+ negatorOid = OperatorGetOrCreateValidNegator(negatorName,
+ oprForm->oid,
+ oprForm->oprname.data,
+ oprForm->oprnamespace,
+ oprForm->oprleft,
+ oprForm->oprright);
+ else
+ negatorOid = InvalidOid;
- if (oprForm->oprresult != BOOLOID)
+ /*
+ * check that we're not changing any existing values that can't be changed
+ */
+ if (OidIsValid(commutatorOid) && OidIsValid(oprForm->oprcom))
{
- if (OidIsValid(restrictionOid))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can have restriction selectivity")));
- if (OidIsValid(joinOid))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can have join selectivity")));
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator attribute \"commutator\" cannot be changed if it has already been set")));
}
+ if (OidIsValid(negatorOid) && OidIsValid(oprForm->oprnegate))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator attribute \"negator\" cannot be changed if it has already been set")));
+ }
+
+ if (updateHashes && oprForm->oprcanhash)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator attribute \"hashes\" cannot be changed if it has already been set")));
+ }
+
+ if (updateMerges && oprForm->oprcanmerge)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator attribute \"merges\" cannot be changed if it has already been set")));
+ }
+
+ /* Perform additional checks, like OperatorCreate does */
+ OperatorValidateParams(oprForm->oprleft,
+ oprForm->oprright,
+ oprForm->oprresult,
+ OidIsValid(commutatorOid),
+ OidIsValid(negatorOid),
+ OidIsValid(joinOid),
+ OidIsValid(restrictionOid),
+ updateMerges,
+ updateHashes);
+
/* Update the tuple */
for (i = 0; i < Natts_pg_operator; ++i)
{
@@ -539,6 +621,30 @@ AlterOperator(AlterOperatorStmt *stmt)
values[Anum_pg_operator_oprjoin - 1] = joinOid;
}
+ if (OidIsValid(commutatorOid))
+ {
+ replaces[Anum_pg_operator_oprcom - 1] = true;
+ values[Anum_pg_operator_oprcom - 1] = ObjectIdGetDatum(commutatorOid);
+ }
+
+ if (OidIsValid(negatorOid))
+ {
+ replaces[Anum_pg_operator_oprnegate - 1] = true;
+ values[Anum_pg_operator_oprnegate - 1] = ObjectIdGetDatum(negatorOid);
+ }
+
+ if (updateMerges)
+ {
+ replaces[Anum_pg_operator_oprcanmerge - 1] = true;
+ values[Anum_pg_operator_oprcanmerge - 1] = true;
+ }
+
+ if (updateHashes)
+ {
+ replaces[Anum_pg_operator_oprcanhash - 1] = true;
+ values[Anum_pg_operator_oprcanhash - 1] = true;
+ }
+
tup = heap_modify_tuple(tup, RelationGetDescr(catalog),
values, nulls, replaces);
@@ -550,5 +656,8 @@ AlterOperator(AlterOperatorStmt *stmt)
table_close(catalog, NoLock);
+ if (OidIsValid(commutatorOid) || OidIsValid(negatorOid))
+ OperatorUpd(oprId, commutatorOid, negatorOid, false);
+
return address;
}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 39ab7eac0d..84017da3ee 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -10101,6 +10101,8 @@ operator_def_elem: ColLabel '=' NONE
{ $$ = makeDefElem($1, NULL, @1); }
| ColLabel '=' operator_def_arg
{ $$ = makeDefElem($1, (Node *) $3, @1); }
+ | ColLabel
+ { $$ = makeDefElem($1, NULL, @1); }
;
/* must be similar enough to def_arg to avoid reduce/reduce conflicts */
diff --git a/src/test/regress/expected/alter_operator.out b/src/test/regress/expected/alter_operator.out
index 71bd484282..57cf4df155 100644
--- a/src/test/regress/expected/alter_operator.out
+++ b/src/test/regress/expected/alter_operator.out
@@ -25,7 +25,7 @@ ORDER BY 1;
(3 rows)
--
--- Reset and set params
+-- test reset and set restrict and join
--
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = NONE);
ALTER OPERATOR === (boolean, boolean) SET (JOIN = NONE);
@@ -106,34 +106,240 @@ ORDER BY 1;
schema public | n
(3 rows)
---
--- Test invalid options.
---
-ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = ====);
-ERROR: operator attribute "commutator" cannot be changed
-ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = ====);
-ERROR: operator attribute "negator" cannot be changed
+-- test cannot set non existant function
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = non_existent_func);
ERROR: function non_existent_func(internal, oid, internal, integer) does not exist
ALTER OPERATOR === (boolean, boolean) SET (JOIN = non_existent_func);
ERROR: function non_existent_func(internal, oid, internal, smallint, internal) does not exist
-ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = !==);
-ERROR: operator attribute "commutator" cannot be changed
-ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = !==);
-ERROR: operator attribute "negator" cannot be changed
--- invalid: non-lowercase quoted identifiers
+-- test non-lowercase quoted identifiers invalid
ALTER OPERATOR & (bit, bit) SET ("Restrict" = _int_contsel, "Join" = _int_contjoinsel);
ERROR: operator attribute "Restrict" not recognized
--
--- Test permission check. Must be owner to ALTER OPERATOR.
+-- test must be owner of operator to ALTER OPERATOR.
--
CREATE USER regress_alter_op_user;
SET SESSION AUTHORIZATION regress_alter_op_user;
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = NONE);
ERROR: must be owner of operator ===
--- Clean up
RESET SESSION AUTHORIZATION;
+--
+-- test set commutator, negator, hashes, and merges which can only be set if not
+-- already set
+--
+-- for these tests create operators with different left and right types so that
+-- we can validate that function signatures are handled correctly
+CREATE FUNCTION alter_op_test_fn_bool_real(boolean, real)
+RETURNS boolean AS $$ SELECT NULL::BOOLEAN; $$ LANGUAGE sql IMMUTABLE;
+CREATE FUNCTION alter_op_test_fn_real_bool(real, boolean)
+RETURNS boolean AS $$ SELECT NULL::BOOLEAN; $$ LANGUAGE sql IMMUTABLE;
+-- operator
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+-- commutator
+CREATE OPERATOR ==== (
+ LEFTARG = real,
+ RIGHTARG = boolean,
+ PROCEDURE = alter_op_test_fn_real_bool
+);
+-- negator
+CREATE OPERATOR !==== (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+-- test cannot set hashes or merges to false
+ALTER OPERATOR === (boolean, real) SET (HASHES = false);
+ERROR: alter operator cannot disable "hashes"
+ALTER OPERATOR === (boolean, real) SET (MERGES = false);
+ERROR: alter operator cannot disable "merges"
+-- test cannot set commutator or negator without owning them
+SET SESSION AUTHORIZATION regress_alter_op_user;
+-- we need need a new operator owned by regress_alter_op_user so that we are
+-- allowed to alter it. ==== and !==== are owned by the test user so we expect
+-- the alters below to fail.
+CREATE OPERATOR ===@@@ (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+ALTER OPERATOR ===@@@ (boolean, real) SET (COMMUTATOR= ====);
+ERROR: must be owner of operator ====
+ALTER OPERATOR ===@@@ (boolean, real) SET (NEGATOR = !====);
+ERROR: must be owner of operator !====
+-- validate operator is unchanged and commutator and negator are unset
+SELECT oprcom, oprnegate FROM pg_operator WHERE oprname = '===@@@'
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcom | oprnegate
+--------+-----------
+ 0 | 0
+(1 row)
+
+DROP OPERATOR ===@@@ (boolean, real);
+RESET SESSION AUTHORIZATION;
+-- test cannot set self negator
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = ===);
+ERROR: operator cannot be its own negator
+-- validate no changes made
+SELECT oprcanmerge, oprcanhash, oprcom, oprnegate
+FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanmerge | oprcanhash | oprcom | oprnegate
+-------------+------------+--------+-----------
+ f | f | 0 | 0
+(1 row)
+
+-- test set hashes
+ALTER OPERATOR === (boolean, real) SET (HASHES);
+SELECT oprcanhash FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanhash
+------------
+ t
+(1 row)
+
+-- test set merges
+ALTER OPERATOR === (boolean, real) SET (MERGES);
+SELECT oprcanmerge FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanmerge
+-------------
+ t
+(1 row)
+
+-- test set commutator
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ====);
+-- validate that the commutator has been set on both the operator and commutator,
+-- that they reference each other, and that the operator used is the existing
+-- one we created and not a new shell operator
+SELECT op.oprname AS operator_name, com.oprname AS commutator_name,
+ com.oprcode AS commutator_func
+ FROM pg_operator op
+ INNER JOIN pg_operator com ON (op.oid = com.oprcom AND op.oprcom = com.oid)
+ WHERE op.oprname = '==='
+ AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype;
+ operator_name | commutator_name | commutator_func
+---------------+-----------------+----------------------------
+ === | ==== | alter_op_test_fn_real_bool
+(1 row)
+
+-- test set negator
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !====);
+-- validate that the negator has been set on both the operator and negator, that
+-- they reference each other, and that the operator used is the existing one we
+-- created and not a new shell operator
+SELECT op.oprname AS operator_name, neg.oprname AS negator_name,
+ neg.oprcode AS negator_func
+ FROM pg_operator op
+ INNER JOIN pg_operator neg ON (op.oid = neg.oprnegate AND op.oprnegate = neg.oid)
+ WHERE op.oprname = '==='
+ AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype;
+ operator_name | negator_name | negator_func
+---------------+--------------+----------------------------
+ === | !==== | alter_op_test_fn_bool_real
+(1 row)
+
+-- validate that the final state of the operator is as we expect
+SELECT oprcanmerge, oprcanhash,
+ pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator,
+ pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator
+ FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanmerge | oprcanhash | commutator | negator
+-------------+------------+-----------------------------+------------------------------
+ t | t | operator ====(real,boolean) | operator !====(boolean,real)
+(1 row)
+
+-- test cannot set commutator, negator, hashes, and merges when already set
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = =);
+ERROR: operator attribute "commutator" cannot be changed if it has already been set
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = =);
+ERROR: operator attribute "negator" cannot be changed if it has already been set
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = !=);
+ERROR: operator attribute "commutator" cannot be changed if it has already been set
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !=);
+ERROR: operator attribute "negator" cannot be changed if it has already been set
+ALTER OPERATOR === (boolean, real) SET (HASHES);
+ERROR: operator attribute "hashes" cannot be changed if it has already been set
+ALTER OPERATOR === (boolean, real) SET (MERGES);
+ERROR: operator attribute "merges" cannot be changed if it has already been set
+ALTER OPERATOR === (boolean, real) SET (HASHES = false);
+ERROR: alter operator cannot disable "hashes"
+ALTER OPERATOR === (boolean, real) SET (MERGES = false);
+ERROR: alter operator cannot disable "merges"
+-- validate no changes made
+SELECT oprcanmerge, oprcanhash,
+ pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator,
+ pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator
+ FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanmerge | oprcanhash | commutator | negator
+-------------+------------+-----------------------------+------------------------------
+ t | t | operator ====(real,boolean) | operator !====(boolean,real)
+(1 row)
+
+--
+-- test setting undefined operator creates shell operator (matches the
+-- behaviour of CREATE OPERATOR)
+--
+DROP OPERATOR === (boolean, real);
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ===@@@);
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !===@@@);
+-- validate that shell operators are created for the commutator and negator with
+-- the commutator having reversed args and the negator matching the operator.
+-- The shell operators should have an empty function below.
+SELECT pg_describe_object('pg_operator'::regclass, op.oid, 0) AS operator,
+ pg_describe_object('pg_operator'::regclass, com.oid, 0) AS commutator,
+ com.oprcode AS commutator_func,
+ pg_describe_object('pg_operator'::regclass, neg.oid, 0) AS negator,
+ neg.oprcode AS negator_func
+ FROM pg_operator op
+ INNER JOIN pg_operator com ON (op.oid = com.oprcom AND op.oprcom = com.oid)
+ INNER JOIN pg_operator neg ON (op.oid = neg.oprnegate AND op.oprnegate = neg.oid)
+ WHERE op.oprname = '===' AND com.oprname = '===@@@' AND neg.oprname = '!===@@@'
+ AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype;
+ operator | commutator | commutator_func | negator | negator_func
+----------------------------+-------------------------------+-----------------+--------------------------------+--------------
+ operator ===(boolean,real) | operator ===@@@(real,boolean) | - | operator !===@@@(boolean,real) | -
+(1 row)
+
+--
+-- test setting self commutator
+--
+DROP OPERATOR === (boolean, boolean);
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = boolean,
+ PROCEDURE = alter_op_test_fn
+);
+ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = ===);
+-- validate that the oprcom is the operator oid
+SELECT oprname FROM pg_operator
+ WHERE oprname = '===' AND oid = oprcom
+ AND oprleft = 'boolean'::regtype AND oprright = 'boolean'::regtype;
+ oprname
+---------
+ ===
+(1 row)
+
+--
+-- Clean up
+--
DROP USER regress_alter_op_user;
DROP OPERATOR === (boolean, boolean);
+DROP OPERATOR === (boolean, real);
+DROP OPERATOR ==== (real, boolean);
+DROP OPERATOR !==== (boolean, real);
+DROP OPERATOR ===@@@ (real, boolean);
+DROP OPERATOR !===@@@(boolean, real);
DROP FUNCTION customcontsel(internal, oid, internal, integer);
DROP FUNCTION alter_op_test_fn(boolean, boolean);
+DROP FUNCTION alter_op_test_fn_bool_real(boolean, real);
+DROP FUNCTION alter_op_test_fn_real_bool(real, boolean);
diff --git a/src/test/regress/sql/alter_operator.sql b/src/test/regress/sql/alter_operator.sql
index fd40370165..a7e0d4331f 100644
--- a/src/test/regress/sql/alter_operator.sql
+++ b/src/test/regress/sql/alter_operator.sql
@@ -22,7 +22,7 @@ WHERE classid = 'pg_operator'::regclass AND
ORDER BY 1;
--
--- Reset and set params
+-- test reset and set restrict and join
--
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = NONE);
@@ -71,30 +71,215 @@ WHERE classid = 'pg_operator'::regclass AND
objid = '===(bool,bool)'::regoperator
ORDER BY 1;
---
--- Test invalid options.
---
-ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = ====);
-ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = ====);
+-- test cannot set non existant function
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = non_existent_func);
ALTER OPERATOR === (boolean, boolean) SET (JOIN = non_existent_func);
-ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = !==);
-ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = !==);
--- invalid: non-lowercase quoted identifiers
+-- test non-lowercase quoted identifiers invalid
ALTER OPERATOR & (bit, bit) SET ("Restrict" = _int_contsel, "Join" = _int_contjoinsel);
--
--- Test permission check. Must be owner to ALTER OPERATOR.
+-- test must be owner of operator to ALTER OPERATOR.
--
CREATE USER regress_alter_op_user;
SET SESSION AUTHORIZATION regress_alter_op_user;
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = NONE);
--- Clean up
RESET SESSION AUTHORIZATION;
+
+--
+-- test set commutator, negator, hashes, and merges which can only be set if not
+-- already set
+--
+
+-- for these tests create operators with different left and right types so that
+-- we can validate that function signatures are handled correctly
+
+CREATE FUNCTION alter_op_test_fn_bool_real(boolean, real)
+RETURNS boolean AS $$ SELECT NULL::BOOLEAN; $$ LANGUAGE sql IMMUTABLE;
+
+CREATE FUNCTION alter_op_test_fn_real_bool(real, boolean)
+RETURNS boolean AS $$ SELECT NULL::BOOLEAN; $$ LANGUAGE sql IMMUTABLE;
+
+-- operator
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+
+-- commutator
+CREATE OPERATOR ==== (
+ LEFTARG = real,
+ RIGHTARG = boolean,
+ PROCEDURE = alter_op_test_fn_real_bool
+);
+
+-- negator
+CREATE OPERATOR !==== (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+
+-- test cannot set hashes or merges to false
+ALTER OPERATOR === (boolean, real) SET (HASHES = false);
+ALTER OPERATOR === (boolean, real) SET (MERGES = false);
+
+-- test cannot set commutator or negator without owning them
+SET SESSION AUTHORIZATION regress_alter_op_user;
+
+-- we need need a new operator owned by regress_alter_op_user so that we are
+-- allowed to alter it. ==== and !==== are owned by the test user so we expect
+-- the alters below to fail.
+CREATE OPERATOR ===@@@ (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+
+ALTER OPERATOR ===@@@ (boolean, real) SET (COMMUTATOR= ====);
+ALTER OPERATOR ===@@@ (boolean, real) SET (NEGATOR = !====);
+
+-- validate operator is unchanged and commutator and negator are unset
+SELECT oprcom, oprnegate FROM pg_operator WHERE oprname = '===@@@'
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+DROP OPERATOR ===@@@ (boolean, real);
+
+RESET SESSION AUTHORIZATION;
+
+-- test cannot set self negator
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = ===);
+
+-- validate no changes made
+SELECT oprcanmerge, oprcanhash, oprcom, oprnegate
+FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+-- test set hashes
+ALTER OPERATOR === (boolean, real) SET (HASHES);
+SELECT oprcanhash FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+-- test set merges
+ALTER OPERATOR === (boolean, real) SET (MERGES);
+SELECT oprcanmerge FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+-- test set commutator
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ====);
+
+-- validate that the commutator has been set on both the operator and commutator,
+-- that they reference each other, and that the operator used is the existing
+-- one we created and not a new shell operator
+SELECT op.oprname AS operator_name, com.oprname AS commutator_name,
+ com.oprcode AS commutator_func
+ FROM pg_operator op
+ INNER JOIN pg_operator com ON (op.oid = com.oprcom AND op.oprcom = com.oid)
+ WHERE op.oprname = '==='
+ AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype;
+
+-- test set negator
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !====);
+
+-- validate that the negator has been set on both the operator and negator, that
+-- they reference each other, and that the operator used is the existing one we
+-- created and not a new shell operator
+SELECT op.oprname AS operator_name, neg.oprname AS negator_name,
+ neg.oprcode AS negator_func
+ FROM pg_operator op
+ INNER JOIN pg_operator neg ON (op.oid = neg.oprnegate AND op.oprnegate = neg.oid)
+ WHERE op.oprname = '==='
+ AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype;
+
+-- validate that the final state of the operator is as we expect
+SELECT oprcanmerge, oprcanhash,
+ pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator,
+ pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator
+ FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+-- test cannot set commutator, negator, hashes, and merges when already set
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = =);
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = =);
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = !=);
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !=);
+ALTER OPERATOR === (boolean, real) SET (HASHES);
+ALTER OPERATOR === (boolean, real) SET (MERGES);
+ALTER OPERATOR === (boolean, real) SET (HASHES = false);
+ALTER OPERATOR === (boolean, real) SET (MERGES = false);
+
+-- validate no changes made
+SELECT oprcanmerge, oprcanhash,
+ pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator,
+ pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator
+ FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+--
+-- test setting undefined operator creates shell operator (matches the
+-- behaviour of CREATE OPERATOR)
+--
+
+DROP OPERATOR === (boolean, real);
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ===@@@);
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !===@@@);
+
+-- validate that shell operators are created for the commutator and negator with
+-- the commutator having reversed args and the negator matching the operator.
+-- The shell operators should have an empty function below.
+SELECT pg_describe_object('pg_operator'::regclass, op.oid, 0) AS operator,
+ pg_describe_object('pg_operator'::regclass, com.oid, 0) AS commutator,
+ com.oprcode AS commutator_func,
+ pg_describe_object('pg_operator'::regclass, neg.oid, 0) AS negator,
+ neg.oprcode AS negator_func
+ FROM pg_operator op
+ INNER JOIN pg_operator com ON (op.oid = com.oprcom AND op.oprcom = com.oid)
+ INNER JOIN pg_operator neg ON (op.oid = neg.oprnegate AND op.oprnegate = neg.oid)
+ WHERE op.oprname = '===' AND com.oprname = '===@@@' AND neg.oprname = '!===@@@'
+ AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype;
+
+
+--
+-- test setting self commutator
+--
+
+DROP OPERATOR === (boolean, boolean);
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = boolean,
+ PROCEDURE = alter_op_test_fn
+);
+
+ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = ===);
+
+-- validate that the oprcom is the operator oid
+SELECT oprname FROM pg_operator
+ WHERE oprname = '===' AND oid = oprcom
+ AND oprleft = 'boolean'::regtype AND oprright = 'boolean'::regtype;
+
+--
+-- Clean up
+--
+
DROP USER regress_alter_op_user;
+
DROP OPERATOR === (boolean, boolean);
+DROP OPERATOR === (boolean, real);
+DROP OPERATOR ==== (real, boolean);
+DROP OPERATOR !==== (boolean, real);
+DROP OPERATOR ===@@@ (real, boolean);
+DROP OPERATOR !===@@@(boolean, real);
+
DROP FUNCTION customcontsel(internal, oid, internal, integer);
DROP FUNCTION alter_op_test_fn(boolean, boolean);
+DROP FUNCTION alter_op_test_fn_bool_real(boolean, real);
+DROP FUNCTION alter_op_test_fn_real_bool(real, boolean);
create_op_fixes_v1.patch (application/octet-stream, 15.9K) download
diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index 95918a77a1..e2590cfce8 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -54,11 +54,10 @@ static Oid OperatorShellMake(const char *operatorName,
Oid leftTypeId,
Oid rightTypeId);
-static Oid get_other_operator(List *otherOp,
- Oid otherLeftTypeId, Oid otherRightTypeId,
- const char *operatorName, Oid operatorNamespace,
- Oid leftTypeId, Oid rightTypeId,
- bool isCommutator);
+static Oid get_or_create_other_operator(List *otherOp,
+ Oid otherLeftTypeId, Oid otherRightTypeId,
+ const char *operatorName, Oid operatorNamespace,
+ Oid leftTypeId, Oid rightTypeId);
/*
@@ -361,53 +360,17 @@ OperatorCreate(const char *operatorName,
errmsg("\"%s\" is not a valid operator name",
operatorName)));
- if (!(OidIsValid(leftTypeId) && OidIsValid(rightTypeId)))
- {
- /* If it's not a binary op, these things mustn't be set: */
- if (commutatorName)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only binary operators can have commutators")));
- if (OidIsValid(joinId))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only binary operators can have join selectivity")));
- if (canMerge)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only binary operators can merge join")));
- if (canHash)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only binary operators can hash")));
- }
-
operResultType = get_func_rettype(procedureId);
- if (operResultType != BOOLOID)
- {
- /* If it's not a boolean op, these things mustn't be set: */
- if (negatorName)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can have negators")));
- if (OidIsValid(restrictionId))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can have restriction selectivity")));
- if (OidIsValid(joinId))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can have join selectivity")));
- if (canMerge)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can merge join")));
- if (canHash)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can hash")));
- }
+ OperatorValidateParams(leftTypeId,
+ rightTypeId,
+ operResultType,
+ commutatorName,
+ negatorName,
+ OidIsValid(joinId),
+ OidIsValid(restrictionId),
+ canMerge,
+ canHash);
operatorObjectId = OperatorGet(operatorName,
operatorNamespace,
@@ -438,18 +401,12 @@ OperatorCreate(const char *operatorName,
if (commutatorName)
{
- /* commutator has reversed arg types */
- commutatorId = get_other_operator(commutatorName,
- rightTypeId, leftTypeId,
- operatorName, operatorNamespace,
- leftTypeId, rightTypeId,
- true);
-
- /* Permission check: must own other operator */
- if (OidIsValid(commutatorId) &&
- !object_ownercheck(OperatorRelationId, commutatorId, GetUserId()))
- aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_OPERATOR,
- NameListToString(commutatorName));
+ commutatorId = OperatorGetOrCreateValidCommutator(commutatorName,
+ operatorObjectId,
+ operatorName,
+ operatorNamespace,
+ leftTypeId,
+ rightTypeId);
/*
* self-linkage to this operator; will fix below. Note that only
@@ -462,20 +419,12 @@ OperatorCreate(const char *operatorName,
commutatorId = InvalidOid;
if (negatorName)
- {
- /* negator has same arg types */
- negatorId = get_other_operator(negatorName,
- leftTypeId, rightTypeId,
- operatorName, operatorNamespace,
- leftTypeId, rightTypeId,
- false);
-
- /* Permission check: must own other operator */
- if (OidIsValid(negatorId) &&
- !object_ownercheck(OperatorRelationId, negatorId, GetUserId()))
- aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_OPERATOR,
- NameListToString(negatorName));
- }
+ negatorId = OperatorGetOrCreateValidNegator(negatorName,
+ operatorObjectId,
+ operatorName,
+ operatorNamespace,
+ leftTypeId,
+ rightTypeId);
else
negatorId = InvalidOid;
@@ -574,17 +523,16 @@ OperatorCreate(const char *operatorName,
}
/*
- * Try to lookup another operator (commutator, etc)
+ * Try to lookup another operator (commutator, negator)
*
* If not found, check to see if it is exactly the operator we are trying
- * to define; if so, return InvalidOid. (Note that this case is only
- * sensible for a commutator, so we error out otherwise.) If it is not
- * the same operator, create a shell operator.
+ * to define; if so, return InvalidOid. If it is not the same operator, create
+ * a shell operator.
*/
static Oid
-get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
- const char *operatorName, Oid operatorNamespace,
- Oid leftTypeId, Oid rightTypeId, bool isCommutator)
+get_or_create_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
+ const char *operatorName, Oid operatorNamespace,
+ Oid leftTypeId, Oid rightTypeId)
{
Oid other_oid;
bool otherDefined;
@@ -612,13 +560,12 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
otherRightTypeId == rightTypeId)
{
/*
- * self-linkage to this operator; caller will fix later. Note that
- * only self-linkage for commutation makes sense.
+ * Do not create as the other operator is the same as the operator:
+ * self-linkage. This only occurs when the operator has not already
+ * been created and we expect that the caller is going to create it.
+ * We can't create it in this case so we return and let the caller
+ * handle this scenario.
*/
- if (!isCommutator)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("operator cannot be its own negator or sort operator")));
return InvalidOid;
}
@@ -867,3 +814,166 @@ makeOperatorDependencies(HeapTuple tuple,
return myself;
}
+
+/*
+ * OperatorGetOrCreateValidCommutator
+ *
+ * This function gets or creates a commutator for an existing or yet to be
+ * created operator.
+ *
+ * For an operator that does not exist, pass in InvalidOid for the operatorOid.
+ *
+ * It will return:
+ * - InvalidOid if the commutator is the same as the operator and the operator
+ * does not exist.
+ * - The Oid of the commutator (including the operator itself) if it exists
+ * - The Oid of a newly created commutator shell operator if the commutator does
+ * not exist and is not the same as the operator.
+ *
+ * The function will error if the commutator is not owned by the user.
+ */
+
+Oid
+OperatorGetOrCreateValidCommutator(List *commutatorName,
+ Oid operatorOid,
+ const char *operatorName,
+ Oid operatorNamespace,
+ Oid operatorLeftTypeId,
+ Oid operatorRightTypeId)
+{
+ Oid commutatorId;
+
+ /* commutator has reversed arg types */
+ commutatorId = get_or_create_other_operator(commutatorName,
+ operatorRightTypeId, operatorLeftTypeId,
+ operatorName, operatorNamespace,
+ operatorLeftTypeId, operatorRightTypeId);
+
+ /* Permission check: must own other operator */
+ if (OidIsValid(commutatorId) &&
+ !object_ownercheck(OperatorRelationId, commutatorId, GetUserId()))
+ aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_OPERATOR,
+ NameListToString(commutatorName));
+
+ return commutatorId;
+}
+
+/*
+ * OperatorGetOrCreateValidNegator
+ *
+ * This function gets or creates a negator for an existing or yet to be created
+ * operator. It will either return an existing operator or a newly created shell
+ * operator.
+ *
+ * For an operator that does not exist, pass in InvalidOid for the operatorOid.
+ *
+ * This function will either return a valid negator oid or error:
+ * - if the operator is the same as the negator as a negator cannot self link
+ * - if the negator is not owned by the user
+ */
+
+Oid
+OperatorGetOrCreateValidNegator(List *negatorName,
+ Oid operatorOid,
+ const char *operatorName,
+ Oid operatorNamespace,
+ Oid operatorLeftTypeId,
+ Oid operatorRightTypeId)
+{
+ Oid negatorId;
+
+ /* negator has same arg types */
+ negatorId = get_or_create_other_operator(negatorName,
+ operatorLeftTypeId, operatorRightTypeId,
+ operatorName, operatorNamespace,
+ operatorLeftTypeId, operatorRightTypeId);
+
+ /*
+ * Check we're not self negating. An invalid negatorID means that the
+ * neither the operator or negator exist, but that their signatures are
+ * the same and therefore it's a self negator.
+ */
+ if (!OidIsValid(negatorId) || negatorId == operatorOid)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator cannot be its own negator")));
+
+ /* Permission check: must own other operator */
+ if (!object_ownercheck(OperatorRelationId, negatorId, GetUserId()))
+ aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_OPERATOR,
+ NameListToString(negatorName));
+
+ return negatorId;
+}
+
+/*
+ * OperatorValidateParams
+ *
+ * This function validates that an operator with arguments of leftTypeId and
+ * rightTypeId returning operResultType can have the attributes that are set
+ * to true. If any attributes set are not compatible with the operator then this
+ * function will raise an error, otherwise it simply returns.
+ *
+ * This function doesn't check for missing requirements so any attribute that's
+ * false is considered valid by this function. As such, if you are modifying an
+ * operator you can set any attributes you are not modifying to false to validate
+ * the changes you are making.
+ */
+
+void
+OperatorValidateParams(Oid leftTypeId,
+ Oid rightTypeId,
+ Oid operResultType,
+ bool hasCommutator,
+ bool hasNegator,
+ bool hasJoinSelectivity,
+ bool hasRestrictionSelectivity,
+ bool canMerge,
+ bool canHash)
+{
+ if (!(OidIsValid(leftTypeId) && OidIsValid(rightTypeId)))
+ {
+ /* If it's not a binary op, these things mustn't be set: */
+ if (hasCommutator)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only binary operators can have commutators")));
+ if (hasJoinSelectivity)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only binary operators can have join selectivity")));
+ if (canMerge)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only binary operators can merge join")));
+ if (canHash)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only binary operators can hash")));
+ }
+
+ if (operResultType != BOOLOID)
+ {
+ /* If it's not a boolean op, these things mustn't be set: */
+ if (hasNegator)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only boolean operators can have negators")));
+ if (hasRestrictionSelectivity)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only boolean operators can have restriction selectivity")));
+ if (hasJoinSelectivity)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only boolean operators can have join selectivity")));
+ if (canMerge)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only boolean operators can merge join")));
+ if (canHash)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only boolean operators can hash")));
+ }
+}
diff --git a/src/include/catalog/pg_operator.h b/src/include/catalog/pg_operator.h
index e60cf782a6..7e1f8680a2 100644
--- a/src/include/catalog/pg_operator.h
+++ b/src/include/catalog/pg_operator.h
@@ -104,4 +104,28 @@ extern ObjectAddress makeOperatorDependencies(HeapTuple tuple,
extern void OperatorUpd(Oid baseId, Oid commId, Oid negId, bool isDelete);
+extern Oid OperatorGetOrCreateValidCommutator(List *commutatorName,
+ Oid operatorOid,
+ const char *operatorName,
+ Oid operatorNamespace,
+ Oid operatorLeftTypeId,
+ Oid operatorRightTypeId);
+
+extern Oid OperatorGetOrCreateValidNegator(List *negatorName,
+ Oid operatorOid,
+ const char *operatorName,
+ Oid operatorNamespace,
+ Oid operatorLeftTypeId,
+ Oid operatorRightTypeId);
+
+extern void OperatorValidateParams(Oid leftTypeId,
+ Oid rightTypeId,
+ Oid operResultType,
+ bool hasCommutator,
+ bool hasNegator,
+ bool hasJoinSelectivity,
+ bool hasRestrictionSelectivity,
+ bool canMerge,
+ bool canHash);
+
#endif /* PG_OPERATOR_H */
diff --git a/src/test/regress/expected/create_operator.out b/src/test/regress/expected/create_operator.out
index f71b601f2d..49070ce90f 100644
--- a/src/test/regress/expected/create_operator.out
+++ b/src/test/regress/expected/create_operator.out
@@ -260,6 +260,42 @@ CREATE OPERATOR #*# (
);
ERROR: permission denied for type type_op6
ROLLBACK;
+-- Should fail. An operator cannot have a self negator.
+BEGIN TRANSACTION;
+CREATE FUNCTION create_op_test_fn(boolean, boolean)
+RETURNS boolean AS $$
+ SELECT NULL::BOOLEAN;
+$$ LANGUAGE sql IMMUTABLE;
+CREATE OPERATOR === (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ negator = ===
+);
+ERROR: operator cannot be its own negator
+ROLLBACK;
+-- Should fail. An operator cannot have a self negator. Here we test that when
+-- 'creating' an existing shell operator, it checks the negator is not self.
+BEGIN TRANSACTION;
+CREATE FUNCTION create_op_test_fn(boolean, boolean)
+RETURNS boolean AS $$
+ SELECT NULL::BOOLEAN;
+$$ LANGUAGE sql IMMUTABLE;
+-- create a shell operator for ===!!! by referencing it as a commutator
+CREATE OPERATOR === (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ commutator = ===!!!
+);
+CREATE OPERATOR ===!!! (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ negator = ===!!!
+);
+ERROR: operator cannot be its own negator
+ROLLBACK;
-- invalid: non-lowercase quoted identifiers
CREATE OPERATOR ===
(
diff --git a/src/test/regress/sql/create_operator.sql b/src/test/regress/sql/create_operator.sql
index f53e24db3c..168cad3814 100644
--- a/src/test/regress/sql/create_operator.sql
+++ b/src/test/regress/sql/create_operator.sql
@@ -210,6 +210,42 @@ CREATE OPERATOR #*# (
);
ROLLBACK;
+-- Should fail. An operator cannot have a self negator.
+BEGIN TRANSACTION;
+CREATE FUNCTION create_op_test_fn(boolean, boolean)
+RETURNS boolean AS $$
+ SELECT NULL::BOOLEAN;
+$$ LANGUAGE sql IMMUTABLE;
+CREATE OPERATOR === (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ negator = ===
+);
+ROLLBACK;
+
+-- Should fail. An operator cannot have a self negator. Here we test that when
+-- 'creating' an existing shell operator, it checks the negator is not self.
+BEGIN TRANSACTION;
+CREATE FUNCTION create_op_test_fn(boolean, boolean)
+RETURNS boolean AS $$
+ SELECT NULL::BOOLEAN;
+$$ LANGUAGE sql IMMUTABLE;
+-- create a shell operator for ===!!! by referencing it as a commutator
+CREATE OPERATOR === (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ commutator = ===!!!
+);
+CREATE OPERATOR ===!!! (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ negator = ===!!!
+);
+ROLLBACK;
+
-- invalid: non-lowercase quoted identifiers
CREATE OPERATOR ===
(
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
@ 2023-06-22 16:47 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
0 siblings, 1 reply; 25+ messages in thread
From: Tom Lane @ 2023-06-22 16:47 UTC (permalink / raw)
To: Tommy Pavlicek <tommypav122@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>
Tommy Pavlicek <tommypav122@gmail.com> writes:
> I've attached a couple of patches to allow ALTER OPERATOR to add
> commutators, negators, hashes and merges to operators that lack them.
Please add this to the upcoming commitfest [1], to ensure we don't
lose track of it.
> The first patch is create_op_fixes_v1.patch and it includes some
> refactoring in preparation for the ALTER OPERATOR changes and fixes a
> couple of minor bugs in CREATE OPERATOR:
> - prevents self negation when filling in/creating an existing shell operator
> - remove reference to sort operator in the self negation error message as
> the sort attribute appears to be deprecated in Postgres 8.3
Hmm, yeah, I bet nobody has looked at those edge cases in awhile.
> Additionally, I wasn't sure whether it was preferred to fail or succeed on
> ALTERs that have no effect, such as adding hashes on an operator that
> already allows them or disabling hashes on one that does not. I chose to
> raise an error when this happens, on the thinking it was more explicit and
> made the code simpler, even though the end result would be what the user
> wanted.
You could argue that both ways I guess. We definitely need to raise error
if the command tries to change an existing nondefault setting, since that
might break things as per previous discussion. But perhaps rejecting
an attempt to set the existing setting is overly nanny-ish. Personally
I think I'd lean to "don't throw an error if we don't have to", but I'm
not strongly set on that position.
(Don't we have existing precedents that apply here? I can't offhand
think of any existing ALTER commands that would reject no-op requests,
but maybe that's not a direct precedent.)
regards, tom lane
[1] https://commitfest.postgresql.org/43/
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-06-22 16:54 ` Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 25+ messages in thread
From: Dagfinn Ilmari Mannsåker @ 2023-06-22 16:54 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tommy Pavlicek <tommypav122@gmail.com>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>
Tom Lane <tgl@sss.pgh.pa.us> writes:
> Tommy Pavlicek <tommypav122@gmail.com> writes:
>
>> Additionally, I wasn't sure whether it was preferred to fail or succeed on
>> ALTERs that have no effect, such as adding hashes on an operator that
>> already allows them or disabling hashes on one that does not. I chose to
>> raise an error when this happens, on the thinking it was more explicit and
>> made the code simpler, even though the end result would be what the user
>> wanted.
>
> You could argue that both ways I guess. We definitely need to raise error
> if the command tries to change an existing nondefault setting, since that
> might break things as per previous discussion. But perhaps rejecting
> an attempt to set the existing setting is overly nanny-ish. Personally
> I think I'd lean to "don't throw an error if we don't have to", but I'm
> not strongly set on that position.
>
> (Don't we have existing precedents that apply here? I can't offhand
> think of any existing ALTER commands that would reject no-op requests,
> but maybe that's not a direct precedent.)
Since it only supports adding these operations if they don't already
exist, should it not be ALTER OPERATOR ADD <thing>, not SET <thing>?
That makes it natural to add an IF NOT EXISTS clause, like ALTER TABLE
ADD COLUMN has, to make it a no-op instead of an error.
> regards, tom lane
- ilmari
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
@ 2023-06-22 17:36 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
0 siblings, 1 reply; 25+ messages in thread
From: Tom Lane @ 2023-06-22 17:36 UTC (permalink / raw)
To: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; +Cc: Tommy Pavlicek <tommypav122@gmail.com>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>
=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> (Don't we have existing precedents that apply here? I can't offhand
>> think of any existing ALTER commands that would reject no-op requests,
>> but maybe that's not a direct precedent.)
> Since it only supports adding these operations if they don't already
> exist, should it not be ALTER OPERATOR ADD <thing>, not SET <thing>?
> That makes it natural to add an IF NOT EXISTS clause, like ALTER TABLE
> ADD COLUMN has, to make it a no-op instead of an error.
Hmm, maybe. But it feels like choosing syntax and semantics based
on what might be only a temporary implementation restriction. We
certainly don't handle any other property-setting commands that way.
Admittedly, "can't change an existing setting" is likely to be pretty
permanent in this case, just because I don't see a use-case for it
that'd justify the work involved. (My wife recently gave me a coffee
cup that says "Nothing is as permanent as a temporary fix.") But
still, if someone did show up and do that work, we'd regret this
choice of syntax because it'd then be uselessly unlike every other
ALTER command.
regards, tom lane
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-06-23 10:34 ` Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 25+ messages in thread
From: Tommy Pavlicek @ 2023-06-23 10:34 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>
Tom Lane <tgl@sss.pgh.pa.us> writes:
> Please add this to the upcoming commitfest [1], to ensure we don't
> lose track of it.
I've added a single patch here: https://commitfest.postgresql.org/43/4389/
It wasn't obvious whether I should create a second commitfest entry
because I've included 2 patches so I've just done 1 to begin with. On
that note, is it preferred here to split patches of this size into
separate patches, and if so, additionally, separate threads?
Tom Lane <tgl@sss.pgh.pa.us> writes:
> > Additionally, I wasn't sure whether it was preferred to fail or succeed on
> > ALTERs that have no effect, such as adding hashes on an operator that
> > already allows them or disabling hashes on one that does not. I chose to
> > raise an error when this happens, on the thinking it was more explicit and
> > made the code simpler, even though the end result would be what the user
> > wanted.
>
> You could argue that both ways I guess. We definitely need to raise error
> if the command tries to change an existing nondefault setting, since that
> might break things as per previous discussion. But perhaps rejecting
> an attempt to set the existing setting is overly nanny-ish. Personally
> I think I'd lean to "don't throw an error if we don't have to", but I'm
> not strongly set on that position.
>
> (Don't we have existing precedents that apply here? I can't offhand
> think of any existing ALTER commands that would reject no-op requests,
> but maybe that's not a direct precedent.)
My initial thinking behind the error for a no-op was largely driven by
the existence of 'DROP.. IF EXISTS'. However, I did some ad hoc
testing on ALTER commands and it does seem that they mostly allow
no-ops. I did find that renaming an object to the same name will fail
due to the object already existing, but that seems to be more of a
coincidence than a design decision to me. Given this, I also lean
towards allowing the no-ops and will change it unless there are
objections.
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
@ 2023-06-23 11:21 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-07-02 14:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
0 siblings, 1 reply; 25+ messages in thread
From: Tom Lane @ 2023-06-23 11:21 UTC (permalink / raw)
To: Tommy Pavlicek <tommypav122@gmail.com>; +Cc: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>
Tommy Pavlicek <tommypav122@gmail.com> writes:
> I've added a single patch here: https://commitfest.postgresql.org/43/4389/
> It wasn't obvious whether I should create a second commitfest entry
> because I've included 2 patches so I've just done 1 to begin with. On
> that note, is it preferred here to split patches of this size into
> separate patches, and if so, additionally, separate threads?
No, our commitfest infrastructure is unable to deal with patches that have
interdependencies unless they're presented in a single email. So just use
one thread, and be sure to attach all the patches each time.
(BTW, while you seem to have gotten away with it so far, it's usually
advisable to name the patch files like 0001-foo.patch, 0002-bar.patch,
etc, to make sure the cfbot understands what order to apply them in.)
regards, tom lane
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-07-02 14:42 ` Tommy Pavlicek <tommypav122@gmail.com>
2023-09-25 10:52 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges jian he <jian.universality@gmail.com>
2023-09-28 20:18 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 25+ messages in thread
From: Tommy Pavlicek @ 2023-07-02 14:42 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>
On Fri, Jun 23, 2023 at 12:21 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Tommy Pavlicek <tommypav122@gmail.com> writes:
> > I've added a single patch here: https://commitfest.postgresql.org/43/4389/
>
> > It wasn't obvious whether I should create a second commitfest entry
> > because I've included 2 patches so I've just done 1 to begin with. On
> > that note, is it preferred here to split patches of this size into
> > separate patches, and if so, additionally, separate threads?
>
> No, our commitfest infrastructure is unable to deal with patches that have
> interdependencies unless they're presented in a single email. So just use
> one thread, and be sure to attach all the patches each time.
>
> (BTW, while you seem to have gotten away with it so far, it's usually
> advisable to name the patch files like 0001-foo.patch, 0002-bar.patch,
> etc, to make sure the cfbot understands what order to apply them in.)
>
> regards, tom lane
Thanks.
I've attached a new version of the ALTER OPERATOR patch that allows
no-ops. It should be ready to review now.
Attachments: download all (2 files, .tar.gz)
0001-create_op_fixes_v1.patch (application/octet-stream, 15.9K) download
diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index 95918a77a1..e2590cfce8 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -54,11 +54,10 @@ static Oid OperatorShellMake(const char *operatorName,
Oid leftTypeId,
Oid rightTypeId);
-static Oid get_other_operator(List *otherOp,
- Oid otherLeftTypeId, Oid otherRightTypeId,
- const char *operatorName, Oid operatorNamespace,
- Oid leftTypeId, Oid rightTypeId,
- bool isCommutator);
+static Oid get_or_create_other_operator(List *otherOp,
+ Oid otherLeftTypeId, Oid otherRightTypeId,
+ const char *operatorName, Oid operatorNamespace,
+ Oid leftTypeId, Oid rightTypeId);
/*
@@ -361,53 +360,17 @@ OperatorCreate(const char *operatorName,
errmsg("\"%s\" is not a valid operator name",
operatorName)));
- if (!(OidIsValid(leftTypeId) && OidIsValid(rightTypeId)))
- {
- /* If it's not a binary op, these things mustn't be set: */
- if (commutatorName)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only binary operators can have commutators")));
- if (OidIsValid(joinId))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only binary operators can have join selectivity")));
- if (canMerge)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only binary operators can merge join")));
- if (canHash)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only binary operators can hash")));
- }
-
operResultType = get_func_rettype(procedureId);
- if (operResultType != BOOLOID)
- {
- /* If it's not a boolean op, these things mustn't be set: */
- if (negatorName)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can have negators")));
- if (OidIsValid(restrictionId))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can have restriction selectivity")));
- if (OidIsValid(joinId))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can have join selectivity")));
- if (canMerge)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can merge join")));
- if (canHash)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can hash")));
- }
+ OperatorValidateParams(leftTypeId,
+ rightTypeId,
+ operResultType,
+ commutatorName,
+ negatorName,
+ OidIsValid(joinId),
+ OidIsValid(restrictionId),
+ canMerge,
+ canHash);
operatorObjectId = OperatorGet(operatorName,
operatorNamespace,
@@ -438,18 +401,12 @@ OperatorCreate(const char *operatorName,
if (commutatorName)
{
- /* commutator has reversed arg types */
- commutatorId = get_other_operator(commutatorName,
- rightTypeId, leftTypeId,
- operatorName, operatorNamespace,
- leftTypeId, rightTypeId,
- true);
-
- /* Permission check: must own other operator */
- if (OidIsValid(commutatorId) &&
- !object_ownercheck(OperatorRelationId, commutatorId, GetUserId()))
- aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_OPERATOR,
- NameListToString(commutatorName));
+ commutatorId = OperatorGetOrCreateValidCommutator(commutatorName,
+ operatorObjectId,
+ operatorName,
+ operatorNamespace,
+ leftTypeId,
+ rightTypeId);
/*
* self-linkage to this operator; will fix below. Note that only
@@ -462,20 +419,12 @@ OperatorCreate(const char *operatorName,
commutatorId = InvalidOid;
if (negatorName)
- {
- /* negator has same arg types */
- negatorId = get_other_operator(negatorName,
- leftTypeId, rightTypeId,
- operatorName, operatorNamespace,
- leftTypeId, rightTypeId,
- false);
-
- /* Permission check: must own other operator */
- if (OidIsValid(negatorId) &&
- !object_ownercheck(OperatorRelationId, negatorId, GetUserId()))
- aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_OPERATOR,
- NameListToString(negatorName));
- }
+ negatorId = OperatorGetOrCreateValidNegator(negatorName,
+ operatorObjectId,
+ operatorName,
+ operatorNamespace,
+ leftTypeId,
+ rightTypeId);
else
negatorId = InvalidOid;
@@ -574,17 +523,16 @@ OperatorCreate(const char *operatorName,
}
/*
- * Try to lookup another operator (commutator, etc)
+ * Try to lookup another operator (commutator, negator)
*
* If not found, check to see if it is exactly the operator we are trying
- * to define; if so, return InvalidOid. (Note that this case is only
- * sensible for a commutator, so we error out otherwise.) If it is not
- * the same operator, create a shell operator.
+ * to define; if so, return InvalidOid. If it is not the same operator, create
+ * a shell operator.
*/
static Oid
-get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
- const char *operatorName, Oid operatorNamespace,
- Oid leftTypeId, Oid rightTypeId, bool isCommutator)
+get_or_create_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
+ const char *operatorName, Oid operatorNamespace,
+ Oid leftTypeId, Oid rightTypeId)
{
Oid other_oid;
bool otherDefined;
@@ -612,13 +560,12 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
otherRightTypeId == rightTypeId)
{
/*
- * self-linkage to this operator; caller will fix later. Note that
- * only self-linkage for commutation makes sense.
+ * Do not create as the other operator is the same as the operator:
+ * self-linkage. This only occurs when the operator has not already
+ * been created and we expect that the caller is going to create it.
+ * We can't create it in this case so we return and let the caller
+ * handle this scenario.
*/
- if (!isCommutator)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("operator cannot be its own negator or sort operator")));
return InvalidOid;
}
@@ -867,3 +814,166 @@ makeOperatorDependencies(HeapTuple tuple,
return myself;
}
+
+/*
+ * OperatorGetOrCreateValidCommutator
+ *
+ * This function gets or creates a commutator for an existing or yet to be
+ * created operator.
+ *
+ * For an operator that does not exist, pass in InvalidOid for the operatorOid.
+ *
+ * It will return:
+ * - InvalidOid if the commutator is the same as the operator and the operator
+ * does not exist.
+ * - The Oid of the commutator (including the operator itself) if it exists
+ * - The Oid of a newly created commutator shell operator if the commutator does
+ * not exist and is not the same as the operator.
+ *
+ * The function will error if the commutator is not owned by the user.
+ */
+
+Oid
+OperatorGetOrCreateValidCommutator(List *commutatorName,
+ Oid operatorOid,
+ const char *operatorName,
+ Oid operatorNamespace,
+ Oid operatorLeftTypeId,
+ Oid operatorRightTypeId)
+{
+ Oid commutatorId;
+
+ /* commutator has reversed arg types */
+ commutatorId = get_or_create_other_operator(commutatorName,
+ operatorRightTypeId, operatorLeftTypeId,
+ operatorName, operatorNamespace,
+ operatorLeftTypeId, operatorRightTypeId);
+
+ /* Permission check: must own other operator */
+ if (OidIsValid(commutatorId) &&
+ !object_ownercheck(OperatorRelationId, commutatorId, GetUserId()))
+ aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_OPERATOR,
+ NameListToString(commutatorName));
+
+ return commutatorId;
+}
+
+/*
+ * OperatorGetOrCreateValidNegator
+ *
+ * This function gets or creates a negator for an existing or yet to be created
+ * operator. It will either return an existing operator or a newly created shell
+ * operator.
+ *
+ * For an operator that does not exist, pass in InvalidOid for the operatorOid.
+ *
+ * This function will either return a valid negator oid or error:
+ * - if the operator is the same as the negator as a negator cannot self link
+ * - if the negator is not owned by the user
+ */
+
+Oid
+OperatorGetOrCreateValidNegator(List *negatorName,
+ Oid operatorOid,
+ const char *operatorName,
+ Oid operatorNamespace,
+ Oid operatorLeftTypeId,
+ Oid operatorRightTypeId)
+{
+ Oid negatorId;
+
+ /* negator has same arg types */
+ negatorId = get_or_create_other_operator(negatorName,
+ operatorLeftTypeId, operatorRightTypeId,
+ operatorName, operatorNamespace,
+ operatorLeftTypeId, operatorRightTypeId);
+
+ /*
+ * Check we're not self negating. An invalid negatorID means that the
+ * neither the operator or negator exist, but that their signatures are
+ * the same and therefore it's a self negator.
+ */
+ if (!OidIsValid(negatorId) || negatorId == operatorOid)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator cannot be its own negator")));
+
+ /* Permission check: must own other operator */
+ if (!object_ownercheck(OperatorRelationId, negatorId, GetUserId()))
+ aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_OPERATOR,
+ NameListToString(negatorName));
+
+ return negatorId;
+}
+
+/*
+ * OperatorValidateParams
+ *
+ * This function validates that an operator with arguments of leftTypeId and
+ * rightTypeId returning operResultType can have the attributes that are set
+ * to true. If any attributes set are not compatible with the operator then this
+ * function will raise an error, otherwise it simply returns.
+ *
+ * This function doesn't check for missing requirements so any attribute that's
+ * false is considered valid by this function. As such, if you are modifying an
+ * operator you can set any attributes you are not modifying to false to validate
+ * the changes you are making.
+ */
+
+void
+OperatorValidateParams(Oid leftTypeId,
+ Oid rightTypeId,
+ Oid operResultType,
+ bool hasCommutator,
+ bool hasNegator,
+ bool hasJoinSelectivity,
+ bool hasRestrictionSelectivity,
+ bool canMerge,
+ bool canHash)
+{
+ if (!(OidIsValid(leftTypeId) && OidIsValid(rightTypeId)))
+ {
+ /* If it's not a binary op, these things mustn't be set: */
+ if (hasCommutator)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only binary operators can have commutators")));
+ if (hasJoinSelectivity)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only binary operators can have join selectivity")));
+ if (canMerge)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only binary operators can merge join")));
+ if (canHash)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only binary operators can hash")));
+ }
+
+ if (operResultType != BOOLOID)
+ {
+ /* If it's not a boolean op, these things mustn't be set: */
+ if (hasNegator)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only boolean operators can have negators")));
+ if (hasRestrictionSelectivity)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only boolean operators can have restriction selectivity")));
+ if (hasJoinSelectivity)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only boolean operators can have join selectivity")));
+ if (canMerge)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only boolean operators can merge join")));
+ if (canHash)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only boolean operators can hash")));
+ }
+}
diff --git a/src/include/catalog/pg_operator.h b/src/include/catalog/pg_operator.h
index e60cf782a6..7e1f8680a2 100644
--- a/src/include/catalog/pg_operator.h
+++ b/src/include/catalog/pg_operator.h
@@ -104,4 +104,28 @@ extern ObjectAddress makeOperatorDependencies(HeapTuple tuple,
extern void OperatorUpd(Oid baseId, Oid commId, Oid negId, bool isDelete);
+extern Oid OperatorGetOrCreateValidCommutator(List *commutatorName,
+ Oid operatorOid,
+ const char *operatorName,
+ Oid operatorNamespace,
+ Oid operatorLeftTypeId,
+ Oid operatorRightTypeId);
+
+extern Oid OperatorGetOrCreateValidNegator(List *negatorName,
+ Oid operatorOid,
+ const char *operatorName,
+ Oid operatorNamespace,
+ Oid operatorLeftTypeId,
+ Oid operatorRightTypeId);
+
+extern void OperatorValidateParams(Oid leftTypeId,
+ Oid rightTypeId,
+ Oid operResultType,
+ bool hasCommutator,
+ bool hasNegator,
+ bool hasJoinSelectivity,
+ bool hasRestrictionSelectivity,
+ bool canMerge,
+ bool canHash);
+
#endif /* PG_OPERATOR_H */
diff --git a/src/test/regress/expected/create_operator.out b/src/test/regress/expected/create_operator.out
index f71b601f2d..49070ce90f 100644
--- a/src/test/regress/expected/create_operator.out
+++ b/src/test/regress/expected/create_operator.out
@@ -260,6 +260,42 @@ CREATE OPERATOR #*# (
);
ERROR: permission denied for type type_op6
ROLLBACK;
+-- Should fail. An operator cannot have a self negator.
+BEGIN TRANSACTION;
+CREATE FUNCTION create_op_test_fn(boolean, boolean)
+RETURNS boolean AS $$
+ SELECT NULL::BOOLEAN;
+$$ LANGUAGE sql IMMUTABLE;
+CREATE OPERATOR === (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ negator = ===
+);
+ERROR: operator cannot be its own negator
+ROLLBACK;
+-- Should fail. An operator cannot have a self negator. Here we test that when
+-- 'creating' an existing shell operator, it checks the negator is not self.
+BEGIN TRANSACTION;
+CREATE FUNCTION create_op_test_fn(boolean, boolean)
+RETURNS boolean AS $$
+ SELECT NULL::BOOLEAN;
+$$ LANGUAGE sql IMMUTABLE;
+-- create a shell operator for ===!!! by referencing it as a commutator
+CREATE OPERATOR === (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ commutator = ===!!!
+);
+CREATE OPERATOR ===!!! (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ negator = ===!!!
+);
+ERROR: operator cannot be its own negator
+ROLLBACK;
-- invalid: non-lowercase quoted identifiers
CREATE OPERATOR ===
(
diff --git a/src/test/regress/sql/create_operator.sql b/src/test/regress/sql/create_operator.sql
index f53e24db3c..168cad3814 100644
--- a/src/test/regress/sql/create_operator.sql
+++ b/src/test/regress/sql/create_operator.sql
@@ -210,6 +210,42 @@ CREATE OPERATOR #*# (
);
ROLLBACK;
+-- Should fail. An operator cannot have a self negator.
+BEGIN TRANSACTION;
+CREATE FUNCTION create_op_test_fn(boolean, boolean)
+RETURNS boolean AS $$
+ SELECT NULL::BOOLEAN;
+$$ LANGUAGE sql IMMUTABLE;
+CREATE OPERATOR === (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ negator = ===
+);
+ROLLBACK;
+
+-- Should fail. An operator cannot have a self negator. Here we test that when
+-- 'creating' an existing shell operator, it checks the negator is not self.
+BEGIN TRANSACTION;
+CREATE FUNCTION create_op_test_fn(boolean, boolean)
+RETURNS boolean AS $$
+ SELECT NULL::BOOLEAN;
+$$ LANGUAGE sql IMMUTABLE;
+-- create a shell operator for ===!!! by referencing it as a commutator
+CREATE OPERATOR === (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ commutator = ===!!!
+);
+CREATE OPERATOR ===!!! (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ negator = ===!!!
+);
+ROLLBACK;
+
-- invalid: non-lowercase quoted identifiers
CREATE OPERATOR ===
(
0002-alter_op_v2.patch (application/octet-stream, 29.5K) download
diff --git a/doc/src/sgml/ref/alter_operator.sgml b/doc/src/sgml/ref/alter_operator.sgml
index a4a1af564f..0bee668f33 100644
--- a/doc/src/sgml/ref/alter_operator.sgml
+++ b/doc/src/sgml/ref/alter_operator.sgml
@@ -29,8 +29,12 @@ ALTER OPERATOR <replaceable>name</replaceable> ( { <replaceable>left_type</repla
ALTER OPERATOR <replaceable>name</replaceable> ( { <replaceable>left_type</replaceable> | NONE } , <replaceable>right_type</replaceable> )
SET ( { RESTRICT = { <replaceable class="parameter">res_proc</replaceable> | NONE }
- | JOIN = { <replaceable class="parameter">join_proc</replaceable> | NONE }
- } [, ... ] )
+ | JOIN = { <replaceable class="parameter">join_proc</replaceable> | NONE }
+ | COMMUTATOR = <replaceable class="parameter">com_op</replaceable>
+ | NEGATOR = <replaceable class="parameter">neg_op</replaceable>
+ | HASHES
+ | MERGES
+ } [, ... ] )
</synopsis>
</refsynopsisdiv>
@@ -121,6 +125,42 @@ ALTER OPERATOR <replaceable>name</replaceable> ( { <replaceable>left_type</repla
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><replaceable class="parameter">com_op</replaceable></term>
+ <listitem>
+ <para>
+ The commutator of this operator. Can only be changed if the operator does not have an existing commutator.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><replaceable class="parameter">neg_op</replaceable></term>
+ <listitem>
+ <para>
+ The negator of this operator. Can only be changed if the operator does not have an existing negator.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>HASHES</literal></term>
+ <listitem>
+ <para>
+ Indicates this operator can support a hash join. Can only be set when the operator does not support a hash join.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>MERGES</literal></term>
+ <listitem>
+ <para>
+ Indicates this operator can support a merge join. Can only be set when the operator does not support a merge join.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</refsect1>
diff --git a/src/backend/commands/operatorcmds.c b/src/backend/commands/operatorcmds.c
index cd7f83136f..bb976b14a5 100644
--- a/src/backend/commands/operatorcmds.c
+++ b/src/backend/commands/operatorcmds.c
@@ -426,6 +426,14 @@ AlterOperator(AlterOperatorStmt *stmt)
List *joinName = NIL; /* optional join sel. function */
bool updateJoin = false;
Oid joinOid;
+ List *commutatorName = NIL; /* optional commutator operator name */
+ Oid commutatorOid;
+ List *negatorName = NIL; /* optional negator operator name */
+ Oid negatorOid;
+ bool updateHashes = false;
+ bool canHash = false;
+ bool updateMerges = false;
+ bool canMerge = false;
/* Look up the operator */
oprId = LookupOperWithArgs(stmt->opername, false);
@@ -456,6 +464,24 @@ AlterOperator(AlterOperatorStmt *stmt)
joinName = param;
updateJoin = true;
}
+ else if (strcmp(defel->defname, "commutator") == 0)
+ {
+ commutatorName = defGetQualifiedName(defel);
+ }
+ else if (strcmp(defel->defname, "negator") == 0)
+ {
+ negatorName = defGetQualifiedName(defel);
+ }
+ else if (strcmp(defel->defname, "hashes") == 0)
+ {
+ canHash = defGetBoolean(defel);
+ updateHashes = true;
+ }
+ else if (strcmp(defel->defname, "merges") == 0)
+ {
+ canMerge = defGetBoolean(defel);
+ updateMerges = true;
+ }
/*
* The rest of the options that CREATE accepts cannot be changed.
@@ -464,11 +490,7 @@ AlterOperator(AlterOperatorStmt *stmt)
else if (strcmp(defel->defname, "leftarg") == 0 ||
strcmp(defel->defname, "rightarg") == 0 ||
strcmp(defel->defname, "function") == 0 ||
- strcmp(defel->defname, "procedure") == 0 ||
- strcmp(defel->defname, "commutator") == 0 ||
- strcmp(defel->defname, "negator") == 0 ||
- strcmp(defel->defname, "hashes") == 0 ||
- strcmp(defel->defname, "merges") == 0)
+ strcmp(defel->defname, "procedure") == 0)
{
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -488,7 +510,7 @@ AlterOperator(AlterOperatorStmt *stmt)
NameStr(oprForm->oprname));
/*
- * Look up restriction and join estimators if specified
+ * Look up Oid for any parameters specified
*/
if (restrictionName)
restrictionOid = ValidateRestrictionEstimator(restrictionName);
@@ -499,28 +521,80 @@ AlterOperator(AlterOperatorStmt *stmt)
else
joinOid = InvalidOid;
- /* Perform additional checks, like OperatorCreate does */
- if (!(OidIsValid(oprForm->oprleft) && OidIsValid(oprForm->oprright)))
+ if (commutatorName)
{
- /* If it's not a binary op, these things mustn't be set: */
- if (OidIsValid(joinOid))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only binary operators can have join selectivity")));
+ commutatorOid = OperatorGetOrCreateValidCommutator(commutatorName,
+ oprForm->oid,
+ oprForm->oprname.data,
+ oprForm->oprnamespace,
+ oprForm->oprleft,
+ oprForm->oprright);
+
+ /*
+ * we don't need to do anything extra for a self commutator as in
+ * OperatorCreate as there we have to create the operator before
+ * setting the commutator, but here the operator already exists and
+ * the commutatorOid above is valid (and is the operator oid).
+ */
}
+ else
+ commutatorOid = InvalidOid;
+
+ if (negatorName)
+ negatorOid = OperatorGetOrCreateValidNegator(negatorName,
+ oprForm->oid,
+ oprForm->oprname.data,
+ oprForm->oprnamespace,
+ oprForm->oprleft,
+ oprForm->oprright);
+ else
+ negatorOid = InvalidOid;
- if (oprForm->oprresult != BOOLOID)
+ /*
+ * check that we're not changing any existing values that might be depended
+ * on elsewhere and may be expected to never change, while allowing no-ops.
+ */
+ if (OidIsValid(commutatorOid) && OidIsValid(oprForm->oprcom)
+ && commutatorOid != oprForm->oprcom)
{
- if (OidIsValid(restrictionOid))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can have restriction selectivity")));
- if (OidIsValid(joinOid))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can have join selectivity")));
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator attribute \"commutator\" cannot be changed if it has already been set")));
+ }
+
+ if (OidIsValid(negatorOid) && OidIsValid(oprForm->oprnegate)
+ && negatorOid != oprForm->oprnegate)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator attribute \"negator\" cannot be changed if it has already been set")));
+ }
+
+ if (updateHashes && oprForm->oprcanhash && !canHash)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator attribute \"hashes\" cannot be changed if it has already been set")));
+ }
+
+ if (updateMerges && oprForm->oprcanmerge && !canMerge)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator attribute \"merges\" cannot be changed if it has already been set")));
}
+ /* Perform additional checks, like OperatorCreate does */
+ OperatorValidateParams(oprForm->oprleft,
+ oprForm->oprright,
+ oprForm->oprresult,
+ OidIsValid(commutatorOid),
+ OidIsValid(negatorOid),
+ OidIsValid(joinOid),
+ OidIsValid(restrictionOid),
+ updateMerges,
+ updateHashes);
+
/* Update the tuple */
for (i = 0; i < Natts_pg_operator; ++i)
{
@@ -539,6 +613,30 @@ AlterOperator(AlterOperatorStmt *stmt)
values[Anum_pg_operator_oprjoin - 1] = joinOid;
}
+ if (OidIsValid(commutatorOid))
+ {
+ replaces[Anum_pg_operator_oprcom - 1] = true;
+ values[Anum_pg_operator_oprcom - 1] = ObjectIdGetDatum(commutatorOid);
+ }
+
+ if (OidIsValid(negatorOid))
+ {
+ replaces[Anum_pg_operator_oprnegate - 1] = true;
+ values[Anum_pg_operator_oprnegate - 1] = ObjectIdGetDatum(negatorOid);
+ }
+
+ if (updateMerges)
+ {
+ replaces[Anum_pg_operator_oprcanmerge - 1] = true;
+ values[Anum_pg_operator_oprcanmerge - 1] = canMerge;
+ }
+
+ if (updateHashes)
+ {
+ replaces[Anum_pg_operator_oprcanhash - 1] = true;
+ values[Anum_pg_operator_oprcanhash - 1] = canHash;
+ }
+
tup = heap_modify_tuple(tup, RelationGetDescr(catalog),
values, nulls, replaces);
@@ -550,5 +648,8 @@ AlterOperator(AlterOperatorStmt *stmt)
table_close(catalog, NoLock);
+ if (OidIsValid(commutatorOid) || OidIsValid(negatorOid))
+ OperatorUpd(oprId, commutatorOid, negatorOid, false);
+
return address;
}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 39ab7eac0d..84017da3ee 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -10101,6 +10101,8 @@ operator_def_elem: ColLabel '=' NONE
{ $$ = makeDefElem($1, NULL, @1); }
| ColLabel '=' operator_def_arg
{ $$ = makeDefElem($1, (Node *) $3, @1); }
+ | ColLabel
+ { $$ = makeDefElem($1, NULL, @1); }
;
/* must be similar enough to def_arg to avoid reduce/reduce conflicts */
diff --git a/src/test/regress/expected/alter_operator.out b/src/test/regress/expected/alter_operator.out
index 71bd484282..50a0c61a39 100644
--- a/src/test/regress/expected/alter_operator.out
+++ b/src/test/regress/expected/alter_operator.out
@@ -25,7 +25,7 @@ ORDER BY 1;
(3 rows)
--
--- Reset and set params
+-- test reset and set restrict and join
--
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = NONE);
ALTER OPERATOR === (boolean, boolean) SET (JOIN = NONE);
@@ -106,34 +106,243 @@ ORDER BY 1;
schema public | n
(3 rows)
---
--- Test invalid options.
---
-ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = ====);
-ERROR: operator attribute "commutator" cannot be changed
-ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = ====);
-ERROR: operator attribute "negator" cannot be changed
+-- test cannot set non existant function
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = non_existent_func);
ERROR: function non_existent_func(internal, oid, internal, integer) does not exist
ALTER OPERATOR === (boolean, boolean) SET (JOIN = non_existent_func);
ERROR: function non_existent_func(internal, oid, internal, smallint, internal) does not exist
-ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = !==);
-ERROR: operator attribute "commutator" cannot be changed
-ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = !==);
-ERROR: operator attribute "negator" cannot be changed
--- invalid: non-lowercase quoted identifiers
+-- test non-lowercase quoted identifiers invalid
ALTER OPERATOR & (bit, bit) SET ("Restrict" = _int_contsel, "Join" = _int_contjoinsel);
ERROR: operator attribute "Restrict" not recognized
--
--- Test permission check. Must be owner to ALTER OPERATOR.
+-- test must be owner of operator to ALTER OPERATOR.
--
CREATE USER regress_alter_op_user;
SET SESSION AUTHORIZATION regress_alter_op_user;
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = NONE);
ERROR: must be owner of operator ===
--- Clean up
RESET SESSION AUTHORIZATION;
+--
+-- test set commutator, negator, hashes, and merges which can only be set if not
+-- already set
+--
+-- for these tests create operators with different left and right types so that
+-- we can validate that function signatures are handled correctly
+CREATE FUNCTION alter_op_test_fn_bool_real(boolean, real)
+RETURNS boolean AS $$ SELECT NULL::BOOLEAN; $$ LANGUAGE sql IMMUTABLE;
+CREATE FUNCTION alter_op_test_fn_real_bool(real, boolean)
+RETURNS boolean AS $$ SELECT NULL::BOOLEAN; $$ LANGUAGE sql IMMUTABLE;
+-- operator
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+-- commutator
+CREATE OPERATOR ==== (
+ LEFTARG = real,
+ RIGHTARG = boolean,
+ PROCEDURE = alter_op_test_fn_real_bool
+);
+-- negator
+CREATE OPERATOR !==== (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+-- test no-op setting already false hashes and merges to false works
+ALTER OPERATOR === (boolean, real) SET (HASHES = false);
+ALTER OPERATOR === (boolean, real) SET (MERGES = false);
+-- validate still false after no-op
+SELECT oprcanhash, oprcanmerge FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanhash | oprcanmerge
+------------+-------------
+ f | f
+(1 row)
+
+-- test cannot set commutator or negator without owning them
+SET SESSION AUTHORIZATION regress_alter_op_user;
+-- we need need a new operator owned by regress_alter_op_user so that we are
+-- allowed to alter it. ==== and !==== are owned by the test user so we expect
+-- the alters below to fail.
+CREATE OPERATOR ===@@@ (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+ALTER OPERATOR ===@@@ (boolean, real) SET (COMMUTATOR= ====);
+ERROR: must be owner of operator ====
+ALTER OPERATOR ===@@@ (boolean, real) SET (NEGATOR = !====);
+ERROR: must be owner of operator !====
+-- validate operator is unchanged and commutator and negator are unset
+SELECT oprcom, oprnegate FROM pg_operator WHERE oprname = '===@@@'
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcom | oprnegate
+--------+-----------
+ 0 | 0
+(1 row)
+
+DROP OPERATOR ===@@@ (boolean, real);
+RESET SESSION AUTHORIZATION;
+-- test cannot set self negator
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = ===);
+ERROR: operator cannot be its own negator
+-- validate no changes made
+SELECT oprcanmerge, oprcanhash, oprcom, oprnegate
+FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanmerge | oprcanhash | oprcom | oprnegate
+-------------+------------+--------+-----------
+ f | f | 0 | 0
+(1 row)
+
+-- test set hashes
+ALTER OPERATOR === (boolean, real) SET (HASHES);
+SELECT oprcanhash FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanhash
+------------
+ t
+(1 row)
+
+-- test set merges
+ALTER OPERATOR === (boolean, real) SET (MERGES);
+SELECT oprcanmerge FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanmerge
+-------------
+ t
+(1 row)
+
+-- test set commutator
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ====);
+-- validate that the commutator has been set on both the operator and commutator,
+-- that they reference each other, and that the operator used is the existing
+-- one we created and not a new shell operator
+SELECT op.oprname AS operator_name, com.oprname AS commutator_name,
+ com.oprcode AS commutator_func
+ FROM pg_operator op
+ INNER JOIN pg_operator com ON (op.oid = com.oprcom AND op.oprcom = com.oid)
+ WHERE op.oprname = '==='
+ AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype;
+ operator_name | commutator_name | commutator_func
+---------------+-----------------+----------------------------
+ === | ==== | alter_op_test_fn_real_bool
+(1 row)
+
+-- test set negator
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !====);
+-- validate that the negator has been set on both the operator and negator, that
+-- they reference each other, and that the operator used is the existing one we
+-- created and not a new shell operator
+SELECT op.oprname AS operator_name, neg.oprname AS negator_name,
+ neg.oprcode AS negator_func
+ FROM pg_operator op
+ INNER JOIN pg_operator neg ON (op.oid = neg.oprnegate AND op.oprnegate = neg.oid)
+ WHERE op.oprname = '==='
+ AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype;
+ operator_name | negator_name | negator_func
+---------------+--------------+----------------------------
+ === | !==== | alter_op_test_fn_bool_real
+(1 row)
+
+-- validate that the final state of the operator is as we expect
+SELECT oprcanmerge, oprcanhash,
+ pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator,
+ pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator
+ FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanmerge | oprcanhash | commutator | negator
+-------------+------------+-----------------------------+------------------------------
+ t | t | operator ====(real,boolean) | operator !====(boolean,real)
+(1 row)
+
+-- test no-op set 'succeeds'
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !====);
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ====);
+ALTER OPERATOR === (boolean, real) SET (HASHES);
+ALTER OPERATOR === (boolean, real) SET (MERGES);
+-- test cannot change commutator, negator, hashes, and merges when already set
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = =);
+ERROR: operator attribute "commutator" cannot be changed if it has already been set
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = =);
+ERROR: operator attribute "negator" cannot be changed if it has already been set
+ALTER OPERATOR === (boolean, real) SET (HASHES = false);
+ERROR: operator attribute "hashes" cannot be changed if it has already been set
+ALTER OPERATOR === (boolean, real) SET (MERGES = false);
+ERROR: operator attribute "merges" cannot be changed if it has already been set
+-- validate no changes made
+SELECT oprcanmerge, oprcanhash,
+ pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator,
+ pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator
+ FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanmerge | oprcanhash | commutator | negator
+-------------+------------+-----------------------------+------------------------------
+ t | t | operator ====(real,boolean) | operator !====(boolean,real)
+(1 row)
+
+--
+-- test setting undefined operator creates shell operator (matches the
+-- behaviour of CREATE OPERATOR)
+--
+DROP OPERATOR === (boolean, real);
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ===@@@);
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !===@@@);
+-- validate that shell operators are created for the commutator and negator with
+-- the commutator having reversed args and the negator matching the operator.
+-- The shell operators should have an empty function below.
+SELECT pg_describe_object('pg_operator'::regclass, op.oid, 0) AS operator,
+ pg_describe_object('pg_operator'::regclass, com.oid, 0) AS commutator,
+ com.oprcode AS commutator_func,
+ pg_describe_object('pg_operator'::regclass, neg.oid, 0) AS negator,
+ neg.oprcode AS negator_func
+ FROM pg_operator op
+ INNER JOIN pg_operator com ON (op.oid = com.oprcom AND op.oprcom = com.oid)
+ INNER JOIN pg_operator neg ON (op.oid = neg.oprnegate AND op.oprnegate = neg.oid)
+ WHERE op.oprname = '===' AND com.oprname = '===@@@' AND neg.oprname = '!===@@@'
+ AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype;
+ operator | commutator | commutator_func | negator | negator_func
+----------------------------+-------------------------------+-----------------+--------------------------------+--------------
+ operator ===(boolean,real) | operator ===@@@(real,boolean) | - | operator !===@@@(boolean,real) | -
+(1 row)
+
+--
+-- test setting self commutator
+--
+DROP OPERATOR === (boolean, boolean);
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = boolean,
+ PROCEDURE = alter_op_test_fn
+);
+ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = ===);
+-- validate that the oprcom is the operator oid
+SELECT oprname FROM pg_operator
+ WHERE oprname = '===' AND oid = oprcom
+ AND oprleft = 'boolean'::regtype AND oprright = 'boolean'::regtype;
+ oprname
+---------
+ ===
+(1 row)
+
+--
+-- Clean up
+--
DROP USER regress_alter_op_user;
DROP OPERATOR === (boolean, boolean);
+DROP OPERATOR === (boolean, real);
+DROP OPERATOR ==== (real, boolean);
+DROP OPERATOR !==== (boolean, real);
+DROP OPERATOR ===@@@ (real, boolean);
+DROP OPERATOR !===@@@(boolean, real);
DROP FUNCTION customcontsel(internal, oid, internal, integer);
DROP FUNCTION alter_op_test_fn(boolean, boolean);
+DROP FUNCTION alter_op_test_fn_bool_real(boolean, real);
+DROP FUNCTION alter_op_test_fn_real_bool(real, boolean);
diff --git a/src/test/regress/sql/alter_operator.sql b/src/test/regress/sql/alter_operator.sql
index fd40370165..978290be0c 100644
--- a/src/test/regress/sql/alter_operator.sql
+++ b/src/test/regress/sql/alter_operator.sql
@@ -22,7 +22,7 @@ WHERE classid = 'pg_operator'::regclass AND
ORDER BY 1;
--
--- Reset and set params
+-- test reset and set restrict and join
--
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = NONE);
@@ -71,30 +71,221 @@ WHERE classid = 'pg_operator'::regclass AND
objid = '===(bool,bool)'::regoperator
ORDER BY 1;
---
--- Test invalid options.
---
-ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = ====);
-ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = ====);
+-- test cannot set non existant function
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = non_existent_func);
ALTER OPERATOR === (boolean, boolean) SET (JOIN = non_existent_func);
-ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = !==);
-ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = !==);
--- invalid: non-lowercase quoted identifiers
+-- test non-lowercase quoted identifiers invalid
ALTER OPERATOR & (bit, bit) SET ("Restrict" = _int_contsel, "Join" = _int_contjoinsel);
--
--- Test permission check. Must be owner to ALTER OPERATOR.
+-- test must be owner of operator to ALTER OPERATOR.
--
CREATE USER regress_alter_op_user;
SET SESSION AUTHORIZATION regress_alter_op_user;
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = NONE);
--- Clean up
RESET SESSION AUTHORIZATION;
+
+--
+-- test set commutator, negator, hashes, and merges which can only be set if not
+-- already set
+--
+
+-- for these tests create operators with different left and right types so that
+-- we can validate that function signatures are handled correctly
+
+CREATE FUNCTION alter_op_test_fn_bool_real(boolean, real)
+RETURNS boolean AS $$ SELECT NULL::BOOLEAN; $$ LANGUAGE sql IMMUTABLE;
+
+CREATE FUNCTION alter_op_test_fn_real_bool(real, boolean)
+RETURNS boolean AS $$ SELECT NULL::BOOLEAN; $$ LANGUAGE sql IMMUTABLE;
+
+-- operator
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+
+-- commutator
+CREATE OPERATOR ==== (
+ LEFTARG = real,
+ RIGHTARG = boolean,
+ PROCEDURE = alter_op_test_fn_real_bool
+);
+
+-- negator
+CREATE OPERATOR !==== (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+
+-- test no-op setting already false hashes and merges to false works
+ALTER OPERATOR === (boolean, real) SET (HASHES = false);
+ALTER OPERATOR === (boolean, real) SET (MERGES = false);
+
+-- validate still false after no-op
+SELECT oprcanhash, oprcanmerge FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+-- test cannot set commutator or negator without owning them
+SET SESSION AUTHORIZATION regress_alter_op_user;
+
+-- we need need a new operator owned by regress_alter_op_user so that we are
+-- allowed to alter it. ==== and !==== are owned by the test user so we expect
+-- the alters below to fail.
+CREATE OPERATOR ===@@@ (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+
+ALTER OPERATOR ===@@@ (boolean, real) SET (COMMUTATOR= ====);
+ALTER OPERATOR ===@@@ (boolean, real) SET (NEGATOR = !====);
+
+-- validate operator is unchanged and commutator and negator are unset
+SELECT oprcom, oprnegate FROM pg_operator WHERE oprname = '===@@@'
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+DROP OPERATOR ===@@@ (boolean, real);
+
+RESET SESSION AUTHORIZATION;
+
+-- test cannot set self negator
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = ===);
+
+-- validate no changes made
+SELECT oprcanmerge, oprcanhash, oprcom, oprnegate
+FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+-- test set hashes
+ALTER OPERATOR === (boolean, real) SET (HASHES);
+SELECT oprcanhash FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+-- test set merges
+ALTER OPERATOR === (boolean, real) SET (MERGES);
+SELECT oprcanmerge FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+-- test set commutator
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ====);
+
+-- validate that the commutator has been set on both the operator and commutator,
+-- that they reference each other, and that the operator used is the existing
+-- one we created and not a new shell operator
+SELECT op.oprname AS operator_name, com.oprname AS commutator_name,
+ com.oprcode AS commutator_func
+ FROM pg_operator op
+ INNER JOIN pg_operator com ON (op.oid = com.oprcom AND op.oprcom = com.oid)
+ WHERE op.oprname = '==='
+ AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype;
+
+-- test set negator
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !====);
+
+-- validate that the negator has been set on both the operator and negator, that
+-- they reference each other, and that the operator used is the existing one we
+-- created and not a new shell operator
+SELECT op.oprname AS operator_name, neg.oprname AS negator_name,
+ neg.oprcode AS negator_func
+ FROM pg_operator op
+ INNER JOIN pg_operator neg ON (op.oid = neg.oprnegate AND op.oprnegate = neg.oid)
+ WHERE op.oprname = '==='
+ AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype;
+
+-- validate that the final state of the operator is as we expect
+SELECT oprcanmerge, oprcanhash,
+ pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator,
+ pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator
+ FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+-- test no-op set 'succeeds'
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !====);
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ====);
+ALTER OPERATOR === (boolean, real) SET (HASHES);
+ALTER OPERATOR === (boolean, real) SET (MERGES);
+
+-- test cannot change commutator, negator, hashes, and merges when already set
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = =);
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = =);
+ALTER OPERATOR === (boolean, real) SET (HASHES = false);
+ALTER OPERATOR === (boolean, real) SET (MERGES = false);
+
+-- validate no changes made
+SELECT oprcanmerge, oprcanhash,
+ pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator,
+ pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator
+ FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+--
+-- test setting undefined operator creates shell operator (matches the
+-- behaviour of CREATE OPERATOR)
+--
+
+DROP OPERATOR === (boolean, real);
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ===@@@);
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !===@@@);
+
+-- validate that shell operators are created for the commutator and negator with
+-- the commutator having reversed args and the negator matching the operator.
+-- The shell operators should have an empty function below.
+SELECT pg_describe_object('pg_operator'::regclass, op.oid, 0) AS operator,
+ pg_describe_object('pg_operator'::regclass, com.oid, 0) AS commutator,
+ com.oprcode AS commutator_func,
+ pg_describe_object('pg_operator'::regclass, neg.oid, 0) AS negator,
+ neg.oprcode AS negator_func
+ FROM pg_operator op
+ INNER JOIN pg_operator com ON (op.oid = com.oprcom AND op.oprcom = com.oid)
+ INNER JOIN pg_operator neg ON (op.oid = neg.oprnegate AND op.oprnegate = neg.oid)
+ WHERE op.oprname = '===' AND com.oprname = '===@@@' AND neg.oprname = '!===@@@'
+ AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype;
+
+
+--
+-- test setting self commutator
+--
+
+DROP OPERATOR === (boolean, boolean);
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = boolean,
+ PROCEDURE = alter_op_test_fn
+);
+
+ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = ===);
+
+-- validate that the oprcom is the operator oid
+SELECT oprname FROM pg_operator
+ WHERE oprname = '===' AND oid = oprcom
+ AND oprleft = 'boolean'::regtype AND oprright = 'boolean'::regtype;
+
+--
+-- Clean up
+--
+
DROP USER regress_alter_op_user;
+
DROP OPERATOR === (boolean, boolean);
+DROP OPERATOR === (boolean, real);
+DROP OPERATOR ==== (real, boolean);
+DROP OPERATOR !==== (boolean, real);
+DROP OPERATOR ===@@@ (real, boolean);
+DROP OPERATOR !===@@@(boolean, real);
+
DROP FUNCTION customcontsel(internal, oid, internal, integer);
DROP FUNCTION alter_op_test_fn(boolean, boolean);
+DROP FUNCTION alter_op_test_fn_bool_real(boolean, real);
+DROP FUNCTION alter_op_test_fn_real_bool(real, boolean);
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-07-02 14:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
@ 2023-09-25 10:52 ` jian he <jian.universality@gmail.com>
2023-09-25 14:55 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges jian he <jian.universality@gmail.com>
1 sibling, 1 reply; 25+ messages in thread
From: jian he @ 2023-09-25 10:52 UTC (permalink / raw)
To: Tommy Pavlicek <tommypav122@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>
/*
* AlterOperator
* routine implementing ALTER OPERATOR <operator> SET (option = ...).
*
* Currently, only RESTRICT and JOIN estimator functions can be changed.
*/
ObjectAddress
AlterOperator(AlterOperatorStmt *stmt)
The above comment needs to change, other than that, it passed the
test, works as expected.
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-07-02 14:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-09-25 10:52 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges jian he <jian.universality@gmail.com>
@ 2023-09-25 14:55 ` jian he <jian.universality@gmail.com>
0 siblings, 0 replies; 25+ messages in thread
From: jian he @ 2023-09-25 14:55 UTC (permalink / raw)
To: Tommy Pavlicek <tommypav122@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>
in doc/src/sgml/ref/alter_operator.sgml
<varlistentry>
<term><literal>HASHES</literal></term>
<listitem>
<para>
Indicates this operator can support a hash join. Can only be set
when the operator does not support a hash join.
</para>
</listitem>
</varlistentry>
<varlistentry>
<term><literal>MERGES</literal></term>
<listitem>
<para>
Indicates this operator can support a merge join. Can only be set
when the operator does not support a merge join.
</para>
</listitem>
</varlistentry>
------------------------
if the operator cannot support hash/merge join, it can't do ALTER
OPERATOR oprname(left_arg, right_arg) SET (HASHES/MERGES = false);
I think it means:
Can only be set when the operator does support a hash/merge join. Once
set to true, it cannot be reset to false.
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-07-02 14:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
@ 2023-09-28 20:18 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-10-10 20:12 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
1 sibling, 1 reply; 25+ messages in thread
From: Tom Lane @ 2023-09-28 20:18 UTC (permalink / raw)
To: Tommy Pavlicek <tommypav122@gmail.com>; +Cc: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>
Tommy Pavlicek <tommypav122@gmail.com> writes:
> I've attached a new version of the ALTER OPERATOR patch that allows
> no-ops. It should be ready to review now.
I got around to looking through this finally (sorry about the delay).
I'm mostly on board with the functionality, with the exception that
I don't see why we should allow ALTER OPERATOR to cause new shell
operators to be created. The argument for allowing that in CREATE
OPERATOR was mainly to allow a linked pair of operators to be created
without a lot of complexity (specifically, being careful to specify
the commutator or negator linkage only in the second CREATE, which
is a rule that requires another exception for a self-commutator).
However, if you're using ALTER OPERATOR then you might as well create
both operators first and then link them with an ALTER command.
In fact, I don't really see a use-case where the operators wouldn't
both exist; isn't this feature mainly to allow retrospective
correction of omitted linkages? So I think allowing ALTER to create a
second operator is more likely to allow mistakes to sneak by than to
do anything useful --- and they will be mistakes you can't correct
except by starting over. I'd even question whether we want to let
ALTER establish a linkage to an existing shell operator, rather than
insisting you turn it into a valid operator via CREATE first.
If we implement it with that restriction then I don't think the
refactorization done in 0001 is correct, or at least not ideal.
(In any case, it seems like a bad idea that the command reference
pages make no mention of this stuff about shell operators. It's
explained in 38.15. Operator Optimization Information, but it'd
be worth at least alluding to that section here. Or maybe we
should move that info to CREATE OPERATOR?)
More generally, you muttered something about 0001 fixing some
existing bugs, but if so I can't see those trees for the forest of
refactorization. I'd suggest splitting any bug fixes apart from
the pure-refactorization step.
regards, tom lane
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-07-02 14:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-09-28 20:18 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-10-10 20:12 ` Tommy Pavlicek <tommypav122@gmail.com>
2023-10-10 20:32 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 25+ messages in thread
From: Tommy Pavlicek @ 2023-10-10 20:12 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>; jian.universality@gmail.com
On Thu, Sep 28, 2023 at 9:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Tommy Pavlicek <tommypav122@gmail.com> writes:
> > I've attached a new version of the ALTER OPERATOR patch that allows
> > no-ops. It should be ready to review now.
>
> I got around to looking through this finally (sorry about the delay).
> I'm mostly on board with the functionality, with the exception that
> I don't see why we should allow ALTER OPERATOR to cause new shell
> operators to be created. The argument for allowing that in CREATE
> OPERATOR was mainly to allow a linked pair of operators to be created
> without a lot of complexity (specifically, being careful to specify
> the commutator or negator linkage only in the second CREATE, which
> is a rule that requires another exception for a self-commutator).
> However, if you're using ALTER OPERATOR then you might as well create
> both operators first and then link them with an ALTER command.
> In fact, I don't really see a use-case where the operators wouldn't
> both exist; isn't this feature mainly to allow retrospective
> correction of omitted linkages? So I think allowing ALTER to create a
> second operator is more likely to allow mistakes to sneak by than to
> do anything useful --- and they will be mistakes you can't correct
> except by starting over. I'd even question whether we want to let
> ALTER establish a linkage to an existing shell operator, rather than
> insisting you turn it into a valid operator via CREATE first.
>
> If we implement it with that restriction then I don't think the
> refactorization done in 0001 is correct, or at least not ideal.
>
> (In any case, it seems like a bad idea that the command reference
> pages make no mention of this stuff about shell operators. It's
> explained in 38.15. Operator Optimization Information, but it'd
> be worth at least alluding to that section here. Or maybe we
> should move that info to CREATE OPERATOR?)
>
> More generally, you muttered something about 0001 fixing some
> existing bugs, but if so I can't see those trees for the forest of
> refactorization. I'd suggest splitting any bug fixes apart from
> the pure-refactorization step.
>
> regards, tom lane
Thanks Tom.
The rationale behind the shell operator and that part of section 38.15
of the documentation had escaped me, but what you're saying makes
complete sense. Based on your comments, I've made some changes:
1. I've isolated the bug fixes (fixing the error message and
disallowing self negation when filling in a shell operator) into
0001-bug-fixes-v3.patch.
2. I've scaled back most of the refactoring as I agree it no longer makes sense.
3. I updated the logic to prevent the creation of or linking to shell operators.
4. I made further updates to the documentation including referencing
38.15 directly in the CREATE and ALTER pages (It's easy to miss if
only 38.14 is referenced) and moved the commentary about creating
commutators and negators into the CREATE section as with the the ALTER
changes it now seems specific to CREATE. I didn't move the rest of
38.15 as I think this applies to both CREATE and ALTER.
I did notice one further potential bug. When creating an operator and
adding a commutator, PostgreSQL only links the commutator back to the
operator if the commutator has no commutator of its own, but the
create operation succeeds regardless of whether this linkage happens.
In other words, if A and B are a pair of commutators and one creates
another operator, C, with A as its commutator, then C will link to A,
but A will still link to B (and B to A). It's not clear to me if this
in itself is a problem, but unless I've misunderstood something
operator C must be the same as B so it implies an error by the user
and there could also be issues if A was deleted since C would continue
to refer to the deleted A.
The same applies for negators and alter operations.
Do you know if this behaviour is intentional or if I've missed
something because it seems undesirable to me. If it is a bug, then I
think I can see how to fix it, but wanted to ask before making any
changes.
On Mon, Sep 25, 2023 at 11:52 AM jian he <jian.universality@gmail.com> wrote:
>
> /*
> * AlterOperator
> * routine implementing ALTER OPERATOR <operator> SET (option = ...).
> *
> * Currently, only RESTRICT and JOIN estimator functions can be changed.
> */
> ObjectAddress
> AlterOperator(AlterOperatorStmt *stmt)
>
> The above comment needs to change, other than that, it passed the
> test, works as expected.
Thanks, added a comment.
> Can only be set when the operator does support a hash/merge join. Once
> set to true, it cannot be reset to false.
Yes, I updated the wording. Is it clearer now?
Attachments: download all (3 files, .tar.gz)
0001-bug-fixes-v3.patch (application/octet-stream, 5.5K) download
diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index 95918a77a1..b0570c33b6 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -57,8 +57,7 @@ static Oid OperatorShellMake(const char *operatorName,
static Oid get_other_operator(List *otherOp,
Oid otherLeftTypeId, Oid otherRightTypeId,
const char *operatorName, Oid operatorNamespace,
- Oid leftTypeId, Oid rightTypeId,
- bool isCommutator);
+ Oid leftTypeId, Oid rightTypeId);
/*
@@ -442,8 +441,7 @@ OperatorCreate(const char *operatorName,
commutatorId = get_other_operator(commutatorName,
rightTypeId, leftTypeId,
operatorName, operatorNamespace,
- leftTypeId, rightTypeId,
- true);
+ leftTypeId, rightTypeId);
/* Permission check: must own other operator */
if (OidIsValid(commutatorId) &&
@@ -467,8 +465,19 @@ OperatorCreate(const char *operatorName,
negatorId = get_other_operator(negatorName,
leftTypeId, rightTypeId,
operatorName, operatorNamespace,
- leftTypeId, rightTypeId,
- false);
+ leftTypeId, rightTypeId);
+
+ /*
+ * Prevent self negation as it doesn't make sense. When self negating,
+ * get_other_operator returns InvalidOid when creating a new operator
+ * and the OID of the operator we are 'creating' when upgrading a
+ * shell operator.
+ */
+
+ if (!OidIsValid(negatorId) || operatorObjectId == negatorId)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator cannot be its own negator")));
/* Permission check: must own other operator */
if (OidIsValid(negatorId) &&
@@ -584,7 +593,7 @@ OperatorCreate(const char *operatorName,
static Oid
get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
const char *operatorName, Oid operatorNamespace,
- Oid leftTypeId, Oid rightTypeId, bool isCommutator)
+ Oid leftTypeId, Oid rightTypeId)
{
Oid other_oid;
bool otherDefined;
@@ -611,14 +620,7 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
otherLeftTypeId == leftTypeId &&
otherRightTypeId == rightTypeId)
{
- /*
- * self-linkage to this operator; caller will fix later. Note that
- * only self-linkage for commutation makes sense.
- */
- if (!isCommutator)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("operator cannot be its own negator or sort operator")));
+ /* self-linkage to this operator; caller will fix later. */
return InvalidOid;
}
diff --git a/src/test/regress/expected/create_operator.out b/src/test/regress/expected/create_operator.out
index f71b601f2d..49070ce90f 100644
--- a/src/test/regress/expected/create_operator.out
+++ b/src/test/regress/expected/create_operator.out
@@ -260,6 +260,42 @@ CREATE OPERATOR #*# (
);
ERROR: permission denied for type type_op6
ROLLBACK;
+-- Should fail. An operator cannot have a self negator.
+BEGIN TRANSACTION;
+CREATE FUNCTION create_op_test_fn(boolean, boolean)
+RETURNS boolean AS $$
+ SELECT NULL::BOOLEAN;
+$$ LANGUAGE sql IMMUTABLE;
+CREATE OPERATOR === (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ negator = ===
+);
+ERROR: operator cannot be its own negator
+ROLLBACK;
+-- Should fail. An operator cannot have a self negator. Here we test that when
+-- 'creating' an existing shell operator, it checks the negator is not self.
+BEGIN TRANSACTION;
+CREATE FUNCTION create_op_test_fn(boolean, boolean)
+RETURNS boolean AS $$
+ SELECT NULL::BOOLEAN;
+$$ LANGUAGE sql IMMUTABLE;
+-- create a shell operator for ===!!! by referencing it as a commutator
+CREATE OPERATOR === (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ commutator = ===!!!
+);
+CREATE OPERATOR ===!!! (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ negator = ===!!!
+);
+ERROR: operator cannot be its own negator
+ROLLBACK;
-- invalid: non-lowercase quoted identifiers
CREATE OPERATOR ===
(
diff --git a/src/test/regress/sql/create_operator.sql b/src/test/regress/sql/create_operator.sql
index f53e24db3c..168cad3814 100644
--- a/src/test/regress/sql/create_operator.sql
+++ b/src/test/regress/sql/create_operator.sql
@@ -210,6 +210,42 @@ CREATE OPERATOR #*# (
);
ROLLBACK;
+-- Should fail. An operator cannot have a self negator.
+BEGIN TRANSACTION;
+CREATE FUNCTION create_op_test_fn(boolean, boolean)
+RETURNS boolean AS $$
+ SELECT NULL::BOOLEAN;
+$$ LANGUAGE sql IMMUTABLE;
+CREATE OPERATOR === (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ negator = ===
+);
+ROLLBACK;
+
+-- Should fail. An operator cannot have a self negator. Here we test that when
+-- 'creating' an existing shell operator, it checks the negator is not self.
+BEGIN TRANSACTION;
+CREATE FUNCTION create_op_test_fn(boolean, boolean)
+RETURNS boolean AS $$
+ SELECT NULL::BOOLEAN;
+$$ LANGUAGE sql IMMUTABLE;
+-- create a shell operator for ===!!! by referencing it as a commutator
+CREATE OPERATOR === (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ commutator = ===!!!
+);
+CREATE OPERATOR ===!!! (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ negator = ===!!!
+);
+ROLLBACK;
+
-- invalid: non-lowercase quoted identifiers
CREATE OPERATOR ===
(
0003-alter_op-v3.patch (application/octet-stream, 39.1K) download
diff --git a/doc/src/sgml/ref/alter_operator.sgml b/doc/src/sgml/ref/alter_operator.sgml
index a4a1af564f..ce447e22b0 100644
--- a/doc/src/sgml/ref/alter_operator.sgml
+++ b/doc/src/sgml/ref/alter_operator.sgml
@@ -29,8 +29,12 @@ ALTER OPERATOR <replaceable>name</replaceable> ( { <replaceable>left_type</repla
ALTER OPERATOR <replaceable>name</replaceable> ( { <replaceable>left_type</replaceable> | NONE } , <replaceable>right_type</replaceable> )
SET ( { RESTRICT = { <replaceable class="parameter">res_proc</replaceable> | NONE }
- | JOIN = { <replaceable class="parameter">join_proc</replaceable> | NONE }
- } [, ... ] )
+ | JOIN = { <replaceable class="parameter">join_proc</replaceable> | NONE }
+ | COMMUTATOR = <replaceable class="parameter">com_op</replaceable>
+ | NEGATOR = <replaceable class="parameter">neg_op</replaceable>
+ | HASHES
+ | MERGES
+ } [, ... ] )
</synopsis>
</refsynopsisdiv>
@@ -121,9 +125,64 @@ ALTER OPERATOR <replaceable>name</replaceable> ( { <replaceable>left_type</repla
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><replaceable class="parameter">com_op</replaceable></term>
+ <listitem>
+ <para>
+ The commutator of this operator. Can only be set if the operator does not have an existing commutator.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><replaceable class="parameter">neg_op</replaceable></term>
+ <listitem>
+ <para>
+ The negator of this operator. Can only be set if the operator does not have an existing negator.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>HASHES</literal></term>
+ <listitem>
+ <para>
+ Indicates this operator can support a hash join. Can only be enabled and not disabled.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>MERGES</literal></term>
+ <listitem>
+ <para>
+ Indicates this operator can support a merge join. Can only be enabled and not disabled.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</refsect1>
+ <refsect1>
+ <title>Notes</title>
+
+ <para>
+ Refer to <xref linkend="xoper"/> and <xref linkend="xoper-optimization"/> for further information.
+ </para>
+
+ <para>
+ Since commutators come in pairs and are commutators of each other,
+ <productname>PostgreSQL</productname> will set the commutator of the
+ <replaceable class="parameter">com_op</replaceable> to the operator.
+ </para>
+
+ <para>
+ Like with commutators, <productname>PostgreSQL</productname> will set the
+ negator of the <replaceable class="parameter">neg_op</replaceable> to the operator.
+ </para>
+ </refsect1>
+
<refsect1>
<title>Examples</title>
diff --git a/doc/src/sgml/ref/create_operator.sgml b/doc/src/sgml/ref/create_operator.sgml
index e27512ff39..9801eb3ed0 100644
--- a/doc/src/sgml/ref/create_operator.sgml
+++ b/doc/src/sgml/ref/create_operator.sgml
@@ -231,9 +231,60 @@ COMMUTATOR = OPERATOR(myschema.===) ,
<title>Notes</title>
<para>
- Refer to <xref linkend="xoper"/> for further information.
+ Refer to <xref linkend="xoper"/> and <xref linkend="xoper-optimization"/> for further information.
</para>
+ <para>
+ When you are defining a self-commutative operator, you just do it.
+ When you are defining a pair of commutative operators, things are
+ a little trickier: how can the first one to be defined refer to the
+ other one, which you haven't defined yet? There are three solutions
+ to this problem:
+
+ <itemizedlist>
+ <listitem>
+ <para>
+ One way is to omit the <literal>COMMUTATOR</literal> clause in the first operator that
+ you define, and then provide one in the second operator's definition.
+ Since <productname>PostgreSQL</productname> knows that commutative
+ operators come in pairs, when it sees the second definition it will
+ automatically go back and fill in the missing <literal>COMMUTATOR</literal> clause in
+ the first definition.
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ Another, more straightforward way is just to include <literal>COMMUTATOR</literal> clauses
+ in both definitions. When <productname>PostgreSQL</productname> processes
+ the first definition and realizes that <literal>COMMUTATOR</literal> refers to a nonexistent
+ operator, the system will make a dummy entry for that operator in the
+ system catalog. This dummy entry will have valid data only
+ for the operator name, left and right operand types, and result type,
+ since that's all that <productname>PostgreSQL</productname> can deduce
+ at this point. The first operator's catalog entry will link to this
+ dummy entry. Later, when you define the second operator, the system
+ updates the dummy entry with the additional information from the second
+ definition. If you try to use the dummy operator before it's been filled
+ in, you'll just get an error message.
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ Alternatively, both operators can be defined without a <literal>COMMUTATOR</literal>
+ and then <command>ALTER OPERATOR</command> can be used to set the <literal>COMMUTATOR</literal>
+ for the operator.
+ </para>
+ </listitem>
+ </itemizedlist>
+ </para>
+
+ <para>
+ Pairs of negator operators can be defined using the same methods
+ explained above for commutator pairs.
+ </para>
+
<para>
It is not possible to specify an operator's lexical precedence in
<command>CREATE OPERATOR</command>, because the parser's precedence behavior
diff --git a/doc/src/sgml/xoper.sgml b/doc/src/sgml/xoper.sgml
index a929ced07d..31bc8df3de 100644
--- a/doc/src/sgml/xoper.sgml
+++ b/doc/src/sgml/xoper.sgml
@@ -146,44 +146,6 @@ SELECT (a + b) AS c FROM test_complex;
<literal>=</literal> operator must specify that it is valid, by marking the
operator with commutator information.
</para>
-
- <para>
- When you are defining a self-commutative operator, you just do it.
- When you are defining a pair of commutative operators, things are
- a little trickier: how can the first one to be defined refer to the
- other one, which you haven't defined yet? There are two solutions
- to this problem:
-
- <itemizedlist>
- <listitem>
- <para>
- One way is to omit the <literal>COMMUTATOR</literal> clause in the first operator that
- you define, and then provide one in the second operator's definition.
- Since <productname>PostgreSQL</productname> knows that commutative
- operators come in pairs, when it sees the second definition it will
- automatically go back and fill in the missing <literal>COMMUTATOR</literal> clause in
- the first definition.
- </para>
- </listitem>
-
- <listitem>
- <para>
- The other, more straightforward way is just to include <literal>COMMUTATOR</literal> clauses
- in both definitions. When <productname>PostgreSQL</productname> processes
- the first definition and realizes that <literal>COMMUTATOR</literal> refers to a nonexistent
- operator, the system will make a dummy entry for that operator in the
- system catalog. This dummy entry will have valid data only
- for the operator name, left and right operand types, and result type,
- since that's all that <productname>PostgreSQL</productname> can deduce
- at this point. The first operator's catalog entry will link to this
- dummy entry. Later, when you define the second operator, the system
- updates the dummy entry with the additional information from the second
- definition. If you try to use the dummy operator before it's been filled
- in, you'll just get an error message.
- </para>
- </listitem>
- </itemizedlist>
- </para>
</sect2>
<sect2 id="xoper-negator">
@@ -218,12 +180,7 @@ SELECT (a + b) AS c FROM test_complex;
<literal>NOT</literal> operations can be inserted as a consequence of other rearrangements.
</para>
- <para>
- Pairs of negator operators can be defined using the same methods
- explained above for commutator pairs.
- </para>
-
- </sect2>
+ </sect2>
<sect2 id="xoper-restrict">
<title><literal>RESTRICT</literal></title>
diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index cda543a03b..35f6e04b47 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -44,11 +44,6 @@ static Oid OperatorGet(const char *operatorName,
Oid rightObjectId,
bool *defined);
-static Oid OperatorLookup(List *operatorName,
- Oid leftObjectId,
- Oid rightObjectId,
- bool *defined);
-
static Oid OperatorShellMake(const char *operatorName,
Oid operatorNamespace,
Oid leftTypeId,
@@ -165,7 +160,7 @@ OperatorGet(const char *operatorName,
*
* *defined is set true if defined (not a shell)
*/
-static Oid
+Oid
OperatorLookup(List *operatorName,
Oid leftObjectId,
Oid rightObjectId,
diff --git a/src/backend/commands/operatorcmds.c b/src/backend/commands/operatorcmds.c
index cd7f83136f..e3d2619081 100644
--- a/src/backend/commands/operatorcmds.c
+++ b/src/backend/commands/operatorcmds.c
@@ -54,6 +54,9 @@
static Oid ValidateRestrictionEstimator(List *restrictionName);
static Oid ValidateJoinEstimator(List *joinName);
+static Oid ValidateOperatorDefined(List *name,
+ Oid leftTypeId,
+ Oid rightTypeId);
/*
* DefineOperator
@@ -359,6 +362,44 @@ ValidateJoinEstimator(List *joinName)
return joinOid;
}
+/*
+ * Looks up and returns an oid for an operator given a possibly-qualified name
+ * and left and right type IDs, verifies the operator is defined (non-shell),
+ * and owned by the current user. Raises an error otherwise.
+ */
+static Oid
+ValidateOperatorDefined(List *name,
+ Oid leftTypeId,
+ Oid rightTypeId)
+{
+ Oid oid;
+ bool defined;
+
+ oid = OperatorLookup(name,
+ leftTypeId,
+ rightTypeId,
+ &defined);
+
+ if (!OidIsValid(oid))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_FUNCTION),
+ errmsg("operator %s does not exist",
+ NameListToString(name))));
+
+ if (!defined)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_FUNCTION),
+ errmsg("operator %s is undefined shell operator",
+ NameListToString(name))));
+
+ if (!object_ownercheck(OperatorRelationId, oid, GetUserId()))
+ aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_OPERATOR,
+ NameListToString(name));
+
+ return oid;
+}
+
+
/*
* Guts of operator deletion.
*/
@@ -406,6 +447,8 @@ RemoveOperatorById(Oid operOid)
* routine implementing ALTER OPERATOR <operator> SET (option = ...).
*
* Currently, only RESTRICT and JOIN estimator functions can be changed.
+ * COMMUTATOR and NEGATOR, HASHES, and MERGES can be set if they have not been set
+ * previously.
*/
ObjectAddress
AlterOperator(AlterOperatorStmt *stmt)
@@ -426,6 +469,14 @@ AlterOperator(AlterOperatorStmt *stmt)
List *joinName = NIL; /* optional join sel. function */
bool updateJoin = false;
Oid joinOid;
+ List *commutatorName = NIL; /* optional commutator operator name */
+ Oid commutatorOid;
+ List *negatorName = NIL; /* optional negator operator name */
+ Oid negatorOid;
+ bool updateHashes = false;
+ bool canHash = false;
+ bool updateMerges = false;
+ bool canMerge = false;
/* Look up the operator */
oprId = LookupOperWithArgs(stmt->opername, false);
@@ -456,6 +507,24 @@ AlterOperator(AlterOperatorStmt *stmt)
joinName = param;
updateJoin = true;
}
+ else if (strcmp(defel->defname, "commutator") == 0)
+ {
+ commutatorName = defGetQualifiedName(defel);
+ }
+ else if (strcmp(defel->defname, "negator") == 0)
+ {
+ negatorName = defGetQualifiedName(defel);
+ }
+ else if (strcmp(defel->defname, "hashes") == 0)
+ {
+ canHash = defGetBoolean(defel);
+ updateHashes = true;
+ }
+ else if (strcmp(defel->defname, "merges") == 0)
+ {
+ canMerge = defGetBoolean(defel);
+ updateMerges = true;
+ }
/*
* The rest of the options that CREATE accepts cannot be changed.
@@ -464,11 +533,7 @@ AlterOperator(AlterOperatorStmt *stmt)
else if (strcmp(defel->defname, "leftarg") == 0 ||
strcmp(defel->defname, "rightarg") == 0 ||
strcmp(defel->defname, "function") == 0 ||
- strcmp(defel->defname, "procedure") == 0 ||
- strcmp(defel->defname, "commutator") == 0 ||
- strcmp(defel->defname, "negator") == 0 ||
- strcmp(defel->defname, "hashes") == 0 ||
- strcmp(defel->defname, "merges") == 0)
+ strcmp(defel->defname, "procedure") == 0)
{
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -488,7 +553,7 @@ AlterOperator(AlterOperatorStmt *stmt)
NameStr(oprForm->oprname));
/*
- * Look up restriction and join estimators if specified
+ * Look up Oid for any parameters specified
*/
if (restrictionName)
restrictionOid = ValidateRestrictionEstimator(restrictionName);
@@ -499,28 +564,85 @@ AlterOperator(AlterOperatorStmt *stmt)
else
joinOid = InvalidOid;
- /* Perform additional checks, like OperatorCreate does */
- if (!(OidIsValid(oprForm->oprleft) && OidIsValid(oprForm->oprright)))
+ if (commutatorName)
{
- /* If it's not a binary op, these things mustn't be set: */
- if (OidIsValid(joinOid))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only binary operators can have join selectivity")));
+ /* commutator has reversed arg types */
+ commutatorOid = ValidateOperatorDefined(commutatorName,
+ oprForm->oprright,
+ oprForm->oprleft);
+
+ /*
+ * we don't need to do anything extra for a self commutator as in
+ * OperatorCreate as there we have to create the operator before
+ * setting the commutator, but here the operator already exists and
+ * the commutatorOid above is valid (and is the operator oid).
+ */
}
+ else
+ commutatorOid = InvalidOid;
- if (oprForm->oprresult != BOOLOID)
+ if (negatorName)
{
- if (OidIsValid(restrictionOid))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can have restriction selectivity")));
- if (OidIsValid(joinOid))
+ negatorOid = ValidateOperatorDefined(negatorName,
+ oprForm->oprleft,
+ oprForm->oprright);
+
+ if (negatorOid == oprForm->oid)
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can have join selectivity")));
+ errmsg("operator cannot be its own negator")));
+ }
+ else
+ {
+ negatorOid = InvalidOid;
+ }
+
+ /*
+ * check that we're not changing any existing values that might be
+ * depended on elsewhere and may be expected to never change, while
+ * allowing no-ops.
+ */
+ if (OidIsValid(commutatorOid) && OidIsValid(oprForm->oprcom)
+ && commutatorOid != oprForm->oprcom)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator attribute \"commutator\" cannot be changed if it has already been set")));
+ }
+
+ if (OidIsValid(negatorOid) && OidIsValid(oprForm->oprnegate)
+ && negatorOid != oprForm->oprnegate)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator attribute \"negator\" cannot be changed if it has already been set")));
+ }
+
+ if (updateHashes && oprForm->oprcanhash && !canHash)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator attribute \"hashes\" cannot be disabled after it has been enabled")));
+ }
+
+ if (updateMerges && oprForm->oprcanmerge && !canMerge)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator attribute \"merges\" cannot be disabled after it has been enabled")));
}
+ /* Perform additional checks, like OperatorCreate does */
+ OperatorValidateParams(oprForm->oprleft,
+ oprForm->oprright,
+ oprForm->oprresult,
+ OidIsValid(commutatorOid),
+ OidIsValid(negatorOid),
+ OidIsValid(joinOid),
+ OidIsValid(restrictionOid),
+ updateMerges,
+ updateHashes);
+
/* Update the tuple */
for (i = 0; i < Natts_pg_operator; ++i)
{
@@ -539,6 +661,30 @@ AlterOperator(AlterOperatorStmt *stmt)
values[Anum_pg_operator_oprjoin - 1] = joinOid;
}
+ if (OidIsValid(commutatorOid))
+ {
+ replaces[Anum_pg_operator_oprcom - 1] = true;
+ values[Anum_pg_operator_oprcom - 1] = ObjectIdGetDatum(commutatorOid);
+ }
+
+ if (OidIsValid(negatorOid))
+ {
+ replaces[Anum_pg_operator_oprnegate - 1] = true;
+ values[Anum_pg_operator_oprnegate - 1] = ObjectIdGetDatum(negatorOid);
+ }
+
+ if (updateMerges)
+ {
+ replaces[Anum_pg_operator_oprcanmerge - 1] = true;
+ values[Anum_pg_operator_oprcanmerge - 1] = canMerge;
+ }
+
+ if (updateHashes)
+ {
+ replaces[Anum_pg_operator_oprcanhash - 1] = true;
+ values[Anum_pg_operator_oprcanhash - 1] = canHash;
+ }
+
tup = heap_modify_tuple(tup, RelationGetDescr(catalog),
values, nulls, replaces);
@@ -550,5 +696,8 @@ AlterOperator(AlterOperatorStmt *stmt)
table_close(catalog, NoLock);
+ if (OidIsValid(commutatorOid) || OidIsValid(negatorOid))
+ OperatorUpd(oprId, commutatorOid, negatorOid, false);
+
return address;
}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index fec9b2604f..8c9a35b5f3 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -10124,6 +10124,8 @@ operator_def_elem: ColLabel '=' NONE
{ $$ = makeDefElem($1, NULL, @1); }
| ColLabel '=' operator_def_arg
{ $$ = makeDefElem($1, (Node *) $3, @1); }
+ | ColLabel
+ { $$ = makeDefElem($1, NULL, @1); }
;
/* must be similar enough to def_arg to avoid reduce/reduce conflicts */
diff --git a/src/include/catalog/pg_operator.h b/src/include/catalog/pg_operator.h
index a670973ab2..13ebc592f2 100644
--- a/src/include/catalog/pg_operator.h
+++ b/src/include/catalog/pg_operator.h
@@ -85,6 +85,11 @@ typedef FormData_pg_operator *Form_pg_operator;
DECLARE_UNIQUE_INDEX_PKEY(pg_operator_oid_index, 2688, OperatorOidIndexId, pg_operator, btree(oid oid_ops));
DECLARE_UNIQUE_INDEX(pg_operator_oprname_l_r_n_index, 2689, OperatorNameNspIndexId, pg_operator, btree(oprname name_ops, oprleft oid_ops, oprright oid_ops, oprnamespace oid_ops));
+extern Oid
+ OperatorLookup(List *operatorName,
+ Oid leftObjectId,
+ Oid rightObjectId,
+ bool *defined);
extern ObjectAddress OperatorCreate(const char *operatorName,
Oid operatorNamespace,
diff --git a/src/test/regress/expected/alter_operator.out b/src/test/regress/expected/alter_operator.out
index 71bd484282..87bf0618fe 100644
--- a/src/test/regress/expected/alter_operator.out
+++ b/src/test/regress/expected/alter_operator.out
@@ -25,7 +25,7 @@ ORDER BY 1;
(3 rows)
--
--- Reset and set params
+-- test reset and set restrict and join
--
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = NONE);
ALTER OPERATOR === (boolean, boolean) SET (JOIN = NONE);
@@ -106,34 +106,264 @@ ORDER BY 1;
schema public | n
(3 rows)
---
--- Test invalid options.
---
-ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = ====);
-ERROR: operator attribute "commutator" cannot be changed
-ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = ====);
-ERROR: operator attribute "negator" cannot be changed
+-- test cannot set non existant function
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = non_existent_func);
ERROR: function non_existent_func(internal, oid, internal, integer) does not exist
ALTER OPERATOR === (boolean, boolean) SET (JOIN = non_existent_func);
ERROR: function non_existent_func(internal, oid, internal, smallint, internal) does not exist
-ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = !==);
-ERROR: operator attribute "commutator" cannot be changed
-ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = !==);
-ERROR: operator attribute "negator" cannot be changed
--- invalid: non-lowercase quoted identifiers
+-- test non-lowercase quoted identifiers invalid
ALTER OPERATOR & (bit, bit) SET ("Restrict" = _int_contsel, "Join" = _int_contjoinsel);
ERROR: operator attribute "Restrict" not recognized
--
--- Test permission check. Must be owner to ALTER OPERATOR.
+-- test must be owner of operator to ALTER OPERATOR.
--
CREATE USER regress_alter_op_user;
SET SESSION AUTHORIZATION regress_alter_op_user;
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = NONE);
ERROR: must be owner of operator ===
--- Clean up
RESET SESSION AUTHORIZATION;
+--
+-- test set commutator, negator, hashes, and merges which can only be set if not
+-- already set
+--
+-- for these tests create operators with different left and right types so that
+-- we can validate that function signatures are handled correctly
+CREATE FUNCTION alter_op_test_fn_bool_real(boolean, real)
+RETURNS boolean AS $$ SELECT NULL::BOOLEAN; $$ LANGUAGE sql IMMUTABLE;
+CREATE FUNCTION alter_op_test_fn_real_bool(real, boolean)
+RETURNS boolean AS $$ SELECT NULL::BOOLEAN; $$ LANGUAGE sql IMMUTABLE;
+-- operator
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+-- commutator
+CREATE OPERATOR ==== (
+ LEFTARG = real,
+ RIGHTARG = boolean,
+ PROCEDURE = alter_op_test_fn_real_bool
+);
+-- negator
+CREATE OPERATOR !==== (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+-- test no-op setting already false hashes and merges to false works
+ALTER OPERATOR === (boolean, real) SET (HASHES = false);
+ALTER OPERATOR === (boolean, real) SET (MERGES = false);
+-- validate still false after no-op
+SELECT oprcanhash, oprcanmerge FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanhash | oprcanmerge
+------------+-------------
+ f | f
+(1 row)
+
+-- test cannot set commutator or negator without owning them
+SET SESSION AUTHORIZATION regress_alter_op_user;
+-- we need need a new operator owned by regress_alter_op_user so that we are
+-- allowed to alter it. ==== and !==== are owned by the test user so we expect
+-- the alters below to fail.
+CREATE OPERATOR ===@@@ (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+ALTER OPERATOR ===@@@ (boolean, real) SET (COMMUTATOR= ====);
+ERROR: must be owner of operator ====
+ALTER OPERATOR ===@@@ (boolean, real) SET (NEGATOR = !====);
+ERROR: must be owner of operator !====
+-- validate operator is unchanged and commutator and negator are unset
+SELECT oprcom, oprnegate FROM pg_operator WHERE oprname = '===@@@'
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcom | oprnegate
+--------+-----------
+ 0 | 0
+(1 row)
+
+DROP OPERATOR ===@@@ (boolean, real);
+RESET SESSION AUTHORIZATION;
+-- test cannot set self negator
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = ===);
+ERROR: operator cannot be its own negator
+-- validate no changes made
+SELECT oprcanmerge, oprcanhash, oprcom, oprnegate
+FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanmerge | oprcanhash | oprcom | oprnegate
+-------------+------------+--------+-----------
+ f | f | 0 | 0
+(1 row)
+
+-- test set hashes
+ALTER OPERATOR === (boolean, real) SET (HASHES);
+SELECT oprcanhash FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanhash
+------------
+ t
+(1 row)
+
+-- test set merges
+ALTER OPERATOR === (boolean, real) SET (MERGES);
+SELECT oprcanmerge FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanmerge
+-------------
+ t
+(1 row)
+
+-- test set commutator
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ====);
+-- validate that the commutator has been set on both the operator and commutator,
+-- that they reference each other, and that the operator used is the existing
+-- one we created and not a new shell operator
+SELECT op.oprname AS operator_name, com.oprname AS commutator_name,
+ com.oprcode AS commutator_func
+ FROM pg_operator op
+ INNER JOIN pg_operator com ON (op.oid = com.oprcom AND op.oprcom = com.oid)
+ WHERE op.oprname = '==='
+ AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype;
+ operator_name | commutator_name | commutator_func
+---------------+-----------------+----------------------------
+ === | ==== | alter_op_test_fn_real_bool
+(1 row)
+
+-- test set negator
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !====);
+-- validate that the negator has been set on both the operator and negator, that
+-- they reference each other, and that the operator used is the existing one we
+-- created and not a new shell operator
+SELECT op.oprname AS operator_name, neg.oprname AS negator_name,
+ neg.oprcode AS negator_func
+ FROM pg_operator op
+ INNER JOIN pg_operator neg ON (op.oid = neg.oprnegate AND op.oprnegate = neg.oid)
+ WHERE op.oprname = '==='
+ AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype;
+ operator_name | negator_name | negator_func
+---------------+--------------+----------------------------
+ === | !==== | alter_op_test_fn_bool_real
+(1 row)
+
+-- validate that the final state of the operator is as we expect
+SELECT oprcanmerge, oprcanhash,
+ pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator,
+ pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator
+ FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanmerge | oprcanhash | commutator | negator
+-------------+------------+-----------------------------+------------------------------
+ t | t | operator ====(real,boolean) | operator !====(boolean,real)
+(1 row)
+
+-- test no-op set 'succeeds'
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !====);
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ====);
+ALTER OPERATOR === (boolean, real) SET (HASHES);
+ALTER OPERATOR === (boolean, real) SET (MERGES);
+-- test cannot change commutator, negator, hashes, and merges when already set
+CREATE OPERATOR @= (
+ LEFTARG = real,
+ RIGHTARG = boolean,
+ PROCEDURE = alter_op_test_fn_real_bool
+);
+CREATE OPERATOR @!= (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = @=);
+ERROR: operator attribute "commutator" cannot be changed if it has already been set
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = @!=);
+ERROR: operator attribute "negator" cannot be changed if it has already been set
+ALTER OPERATOR === (boolean, real) SET (HASHES = false);
+ERROR: operator attribute "hashes" cannot be disabled after it has been enabled
+ALTER OPERATOR === (boolean, real) SET (MERGES = false);
+ERROR: operator attribute "merges" cannot be disabled after it has been enabled
+-- validate no changes made
+SELECT oprcanmerge, oprcanhash,
+ pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator,
+ pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator
+ FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanmerge | oprcanhash | commutator | negator
+-------------+------------+-----------------------------+------------------------------
+ t | t | operator ====(real,boolean) | operator !====(boolean,real)
+(1 row)
+
+DROP OPERATOR @=(real, boolean);
+DROP OPERATOR @!=(boolean, real);
+--
+-- test setting commutator and negator to undefined or shell operator fails
+--
+DROP OPERATOR === (boolean, real);
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+-- test cannot set undefined
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ===@@@);
+ERROR: operator ===@@@ does not exist
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !===@@@);
+ERROR: operator !===@@@ does not exist
+-- create shell operators for ===@@@ and !===@@@
+CREATE OPERATOR ===@@ (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real,
+ COMMUTATOR = ===@@@,
+ NEGATOR = !===@@@
+);
+-- test cannot set shell operators
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ===@@@);
+ERROR: operator ===@@@ is undefined shell operator
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !===@@@);
+ERROR: operator !===@@@ is undefined shell operator
+-- validate no changes made
+SELECT oprcanmerge, oprcanhash,
+ pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator,
+ pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator
+ FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanmerge | oprcanhash | commutator | negator
+-------------+------------+------------+---------
+ f | f | |
+(1 row)
+
+DROP OPERATOR ===@@(boolean, real);
+DROP OPERATOR ===@@@(real, boolean);
+DROP OPERATOR !===@@@(boolean, real);
+--
+-- test setting self commutator
+--
+DROP OPERATOR === (boolean, boolean);
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = boolean,
+ PROCEDURE = alter_op_test_fn
+);
+ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = ===);
+-- validate that the oprcom is the operator oid
+SELECT oprname FROM pg_operator
+ WHERE oprname = '===' AND oid = oprcom
+ AND oprleft = 'boolean'::regtype AND oprright = 'boolean'::regtype;
+ oprname
+---------
+ ===
+(1 row)
+
+--
+-- Clean up
+--
DROP USER regress_alter_op_user;
DROP OPERATOR === (boolean, boolean);
+DROP OPERATOR === (boolean, real);
+DROP OPERATOR ==== (real, boolean);
+DROP OPERATOR !==== (boolean, real);
DROP FUNCTION customcontsel(internal, oid, internal, integer);
DROP FUNCTION alter_op_test_fn(boolean, boolean);
+DROP FUNCTION alter_op_test_fn_bool_real(boolean, real);
+DROP FUNCTION alter_op_test_fn_real_bool(real, boolean);
diff --git a/src/test/regress/sql/alter_operator.sql b/src/test/regress/sql/alter_operator.sql
index fd40370165..9a3b3f0cde 100644
--- a/src/test/regress/sql/alter_operator.sql
+++ b/src/test/regress/sql/alter_operator.sql
@@ -22,7 +22,7 @@ WHERE classid = 'pg_operator'::regclass AND
ORDER BY 1;
--
--- Reset and set params
+-- test reset and set restrict and join
--
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = NONE);
@@ -71,30 +71,243 @@ WHERE classid = 'pg_operator'::regclass AND
objid = '===(bool,bool)'::regoperator
ORDER BY 1;
---
--- Test invalid options.
---
-ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = ====);
-ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = ====);
+-- test cannot set non existant function
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = non_existent_func);
ALTER OPERATOR === (boolean, boolean) SET (JOIN = non_existent_func);
-ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = !==);
-ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = !==);
--- invalid: non-lowercase quoted identifiers
+-- test non-lowercase quoted identifiers invalid
ALTER OPERATOR & (bit, bit) SET ("Restrict" = _int_contsel, "Join" = _int_contjoinsel);
--
--- Test permission check. Must be owner to ALTER OPERATOR.
+-- test must be owner of operator to ALTER OPERATOR.
--
CREATE USER regress_alter_op_user;
SET SESSION AUTHORIZATION regress_alter_op_user;
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = NONE);
--- Clean up
RESET SESSION AUTHORIZATION;
+
+--
+-- test set commutator, negator, hashes, and merges which can only be set if not
+-- already set
+--
+
+-- for these tests create operators with different left and right types so that
+-- we can validate that function signatures are handled correctly
+
+CREATE FUNCTION alter_op_test_fn_bool_real(boolean, real)
+RETURNS boolean AS $$ SELECT NULL::BOOLEAN; $$ LANGUAGE sql IMMUTABLE;
+
+CREATE FUNCTION alter_op_test_fn_real_bool(real, boolean)
+RETURNS boolean AS $$ SELECT NULL::BOOLEAN; $$ LANGUAGE sql IMMUTABLE;
+
+-- operator
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+
+-- commutator
+CREATE OPERATOR ==== (
+ LEFTARG = real,
+ RIGHTARG = boolean,
+ PROCEDURE = alter_op_test_fn_real_bool
+);
+
+-- negator
+CREATE OPERATOR !==== (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+
+-- test no-op setting already false hashes and merges to false works
+ALTER OPERATOR === (boolean, real) SET (HASHES = false);
+ALTER OPERATOR === (boolean, real) SET (MERGES = false);
+
+-- validate still false after no-op
+SELECT oprcanhash, oprcanmerge FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+-- test cannot set commutator or negator without owning them
+SET SESSION AUTHORIZATION regress_alter_op_user;
+
+-- we need need a new operator owned by regress_alter_op_user so that we are
+-- allowed to alter it. ==== and !==== are owned by the test user so we expect
+-- the alters below to fail.
+CREATE OPERATOR ===@@@ (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+
+ALTER OPERATOR ===@@@ (boolean, real) SET (COMMUTATOR= ====);
+ALTER OPERATOR ===@@@ (boolean, real) SET (NEGATOR = !====);
+
+-- validate operator is unchanged and commutator and negator are unset
+SELECT oprcom, oprnegate FROM pg_operator WHERE oprname = '===@@@'
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+DROP OPERATOR ===@@@ (boolean, real);
+
+RESET SESSION AUTHORIZATION;
+
+-- test cannot set self negator
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = ===);
+
+-- validate no changes made
+SELECT oprcanmerge, oprcanhash, oprcom, oprnegate
+FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+-- test set hashes
+ALTER OPERATOR === (boolean, real) SET (HASHES);
+SELECT oprcanhash FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+-- test set merges
+ALTER OPERATOR === (boolean, real) SET (MERGES);
+SELECT oprcanmerge FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+-- test set commutator
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ====);
+
+-- validate that the commutator has been set on both the operator and commutator,
+-- that they reference each other, and that the operator used is the existing
+-- one we created and not a new shell operator
+SELECT op.oprname AS operator_name, com.oprname AS commutator_name,
+ com.oprcode AS commutator_func
+ FROM pg_operator op
+ INNER JOIN pg_operator com ON (op.oid = com.oprcom AND op.oprcom = com.oid)
+ WHERE op.oprname = '==='
+ AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype;
+
+-- test set negator
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !====);
+
+-- validate that the negator has been set on both the operator and negator, that
+-- they reference each other, and that the operator used is the existing one we
+-- created and not a new shell operator
+SELECT op.oprname AS operator_name, neg.oprname AS negator_name,
+ neg.oprcode AS negator_func
+ FROM pg_operator op
+ INNER JOIN pg_operator neg ON (op.oid = neg.oprnegate AND op.oprnegate = neg.oid)
+ WHERE op.oprname = '==='
+ AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype;
+
+-- validate that the final state of the operator is as we expect
+SELECT oprcanmerge, oprcanhash,
+ pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator,
+ pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator
+ FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+-- test no-op set 'succeeds'
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !====);
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ====);
+ALTER OPERATOR === (boolean, real) SET (HASHES);
+ALTER OPERATOR === (boolean, real) SET (MERGES);
+
+-- test cannot change commutator, negator, hashes, and merges when already set
+
+CREATE OPERATOR @= (
+ LEFTARG = real,
+ RIGHTARG = boolean,
+ PROCEDURE = alter_op_test_fn_real_bool
+);
+CREATE OPERATOR @!= (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = @=);
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = @!=);
+ALTER OPERATOR === (boolean, real) SET (HASHES = false);
+ALTER OPERATOR === (boolean, real) SET (MERGES = false);
+
+-- validate no changes made
+SELECT oprcanmerge, oprcanhash,
+ pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator,
+ pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator
+ FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+DROP OPERATOR @=(real, boolean);
+DROP OPERATOR @!=(boolean, real);
+
+--
+-- test setting commutator and negator to undefined or shell operator fails
+--
+
+DROP OPERATOR === (boolean, real);
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+
+-- test cannot set undefined
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ===@@@);
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !===@@@);
+
+-- create shell operators for ===@@@ and !===@@@
+CREATE OPERATOR ===@@ (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real,
+ COMMUTATOR = ===@@@,
+ NEGATOR = !===@@@
+);
+
+-- test cannot set shell operators
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ===@@@);
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !===@@@);
+
+-- validate no changes made
+SELECT oprcanmerge, oprcanhash,
+ pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator,
+ pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator
+ FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+DROP OPERATOR ===@@(boolean, real);
+DROP OPERATOR ===@@@(real, boolean);
+DROP OPERATOR !===@@@(boolean, real);
+
+--
+-- test setting self commutator
+--
+
+DROP OPERATOR === (boolean, boolean);
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = boolean,
+ PROCEDURE = alter_op_test_fn
+);
+
+ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = ===);
+
+-- validate that the oprcom is the operator oid
+SELECT oprname FROM pg_operator
+ WHERE oprname = '===' AND oid = oprcom
+ AND oprleft = 'boolean'::regtype AND oprright = 'boolean'::regtype;
+
+--
+-- Clean up
+--
+
DROP USER regress_alter_op_user;
+
DROP OPERATOR === (boolean, boolean);
+DROP OPERATOR === (boolean, real);
+DROP OPERATOR ==== (real, boolean);
+DROP OPERATOR !==== (boolean, real);
+
DROP FUNCTION customcontsel(internal, oid, internal, integer);
DROP FUNCTION alter_op_test_fn(boolean, boolean);
+DROP FUNCTION alter_op_test_fn_bool_real(boolean, real);
+DROP FUNCTION alter_op_test_fn_real_bool(real, boolean);
0002-refactor-v3.patch (application/octet-stream, 5.7K) download
diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index b0570c33b6..cda543a03b 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -360,53 +360,17 @@ OperatorCreate(const char *operatorName,
errmsg("\"%s\" is not a valid operator name",
operatorName)));
- if (!(OidIsValid(leftTypeId) && OidIsValid(rightTypeId)))
- {
- /* If it's not a binary op, these things mustn't be set: */
- if (commutatorName)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only binary operators can have commutators")));
- if (OidIsValid(joinId))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only binary operators can have join selectivity")));
- if (canMerge)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only binary operators can merge join")));
- if (canHash)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only binary operators can hash")));
- }
-
operResultType = get_func_rettype(procedureId);
- if (operResultType != BOOLOID)
- {
- /* If it's not a boolean op, these things mustn't be set: */
- if (negatorName)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can have negators")));
- if (OidIsValid(restrictionId))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can have restriction selectivity")));
- if (OidIsValid(joinId))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can have join selectivity")));
- if (canMerge)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can merge join")));
- if (canHash)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can hash")));
- }
+ OperatorValidateParams(leftTypeId,
+ rightTypeId,
+ operResultType,
+ commutatorName,
+ negatorName,
+ OidIsValid(joinId),
+ OidIsValid(restrictionId),
+ canMerge,
+ canHash);
operatorObjectId = OperatorGet(operatorName,
operatorNamespace,
@@ -763,6 +727,78 @@ OperatorUpd(Oid baseId, Oid commId, Oid negId, bool isDelete)
table_close(pg_operator_desc, RowExclusiveLock);
}
+/*
+ * OperatorValidateParams
+ *
+ * This function validates that an operator with arguments of leftTypeId and
+ * rightTypeId returning operResultType can have the attributes that are set
+ * to true. If any attributes set are not compatible with the operator then this
+ * function will raise an error, otherwise it simply returns.
+ *
+ * This function doesn't check for missing requirements so any attribute that's
+ * false is considered valid by this function. As such, if you are modifying an
+ * operator you can set any attributes you are not modifying to false to validate
+ * the changes you are making.
+ */
+
+void
+OperatorValidateParams(Oid leftTypeId,
+ Oid rightTypeId,
+ Oid operResultType,
+ bool hasCommutator,
+ bool hasNegator,
+ bool hasJoinSelectivity,
+ bool hasRestrictionSelectivity,
+ bool canMerge,
+ bool canHash)
+{
+ if (!(OidIsValid(leftTypeId) && OidIsValid(rightTypeId)))
+ {
+ /* If it's not a binary op, these things mustn't be set: */
+ if (hasCommutator)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only binary operators can have commutators")));
+ if (hasJoinSelectivity)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only binary operators can have join selectivity")));
+ if (canMerge)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only binary operators can merge join")));
+ if (canHash)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only binary operators can hash")));
+ }
+
+ if (operResultType != BOOLOID)
+ {
+ /* If it's not a boolean op, these things mustn't be set: */
+ if (hasNegator)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only boolean operators can have negators")));
+ if (hasRestrictionSelectivity)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only boolean operators can have restriction selectivity")));
+ if (hasJoinSelectivity)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only boolean operators can have join selectivity")));
+ if (canMerge)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only boolean operators can merge join")));
+ if (canHash)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only boolean operators can hash")));
+ }
+}
+
/*
* Create dependencies for an operator (either a freshly inserted
* complete operator, a new shell operator, a just-updated shell,
diff --git a/src/include/catalog/pg_operator.h b/src/include/catalog/pg_operator.h
index aff372b4bb..a670973ab2 100644
--- a/src/include/catalog/pg_operator.h
+++ b/src/include/catalog/pg_operator.h
@@ -104,4 +104,14 @@ extern ObjectAddress makeOperatorDependencies(HeapTuple tuple,
extern void OperatorUpd(Oid baseId, Oid commId, Oid negId, bool isDelete);
+extern void OperatorValidateParams(Oid leftTypeId,
+ Oid rightTypeId,
+ Oid operResultType,
+ bool hasCommutator,
+ bool hasNegator,
+ bool hasJoinSelectivity,
+ bool hasRestrictionSelectivity,
+ bool canMerge,
+ bool canHash);
+
#endif /* PG_OPERATOR_H */
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-07-02 14:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-09-28 20:18 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-10 20:12 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
@ 2023-10-10 20:32 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-10-11 15:11 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
0 siblings, 1 reply; 25+ messages in thread
From: Tom Lane @ 2023-10-10 20:32 UTC (permalink / raw)
To: Tommy Pavlicek <tommypav122@gmail.com>; +Cc: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>; jian.universality@gmail.com
Tommy Pavlicek <tommypav122@gmail.com> writes:
> I did notice one further potential bug. When creating an operator and
> adding a commutator, PostgreSQL only links the commutator back to the
> operator if the commutator has no commutator of its own, but the
> create operation succeeds regardless of whether this linkage happens.
> In other words, if A and B are a pair of commutators and one creates
> another operator, C, with A as its commutator, then C will link to A,
> but A will still link to B (and B to A). It's not clear to me if this
> in itself is a problem, but unless I've misunderstood something
> operator C must be the same as B so it implies an error by the user
> and there could also be issues if A was deleted since C would continue
> to refer to the deleted A.
Yeah, it'd make sense to tighten that up. Per the discussion so far,
we should not allow an operator's commutator/negator links to change
once set, so overwriting the existing link with a different value
would be wrong. But allowing creation of the new operator to proceed
with a different outcome than expected isn't good either. I think
we should start throwing an error for that.
regards, tom lane
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-07-02 14:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-09-28 20:18 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-10 20:12 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-10 20:32 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-10-11 15:11 ` Tommy Pavlicek <tommypav122@gmail.com>
2023-10-12 03:53 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges jian he <jian.universality@gmail.com>
2023-10-24 13:51 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
0 siblings, 2 replies; 25+ messages in thread
From: Tommy Pavlicek @ 2023-10-11 15:11 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>; jian.universality@gmail.com
On Tue, Oct 10, 2023 at 9:32 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Tommy Pavlicek <tommypav122@gmail.com> writes:
> > I did notice one further potential bug. When creating an operator and
> > adding a commutator, PostgreSQL only links the commutator back to the
> > operator if the commutator has no commutator of its own, but the
> > create operation succeeds regardless of whether this linkage happens.
>
> > In other words, if A and B are a pair of commutators and one creates
> > another operator, C, with A as its commutator, then C will link to A,
> > but A will still link to B (and B to A). It's not clear to me if this
> > in itself is a problem, but unless I've misunderstood something
> > operator C must be the same as B so it implies an error by the user
> > and there could also be issues if A was deleted since C would continue
> > to refer to the deleted A.
>
> Yeah, it'd make sense to tighten that up. Per the discussion so far,
> we should not allow an operator's commutator/negator links to change
> once set, so overwriting the existing link with a different value
> would be wrong. But allowing creation of the new operator to proceed
> with a different outcome than expected isn't good either. I think
> we should start throwing an error for that.
>
> regards, tom lane
Thanks.
I've added another patch (0002-require_unused_neg_com-v1.patch) that
prevents using a commutator or negator that's already part of a pair.
The only other changes from my email yesterday are that in the ALTER
command I moved the post alter hook to after OperatorUpd and the
addition of tests to verify that we can't use an existing commutator
or negator with the ALTER command.
I believe this can all be looked at again.
Cheers,
Tommy
Attachments: download all (4 files, .tar.gz)
0002-require_unused_neg_com-v1.patch (application/octet-stream, 6.2K) download
diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index b0570c33b6..15128235d4 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -557,11 +557,6 @@ OperatorCreate(const char *operatorName,
/* Add dependencies for the entry */
address = makeOperatorDependencies(tup, true, isUpdate);
- /* Post creation hook for new operator */
- InvokeObjectPostCreateHook(OperatorRelationId, operatorObjectId, 0);
-
- table_close(pg_operator_desc, RowExclusiveLock);
-
/*
* If a commutator and/or negator link is provided, update the other
* operator(s) to point at this one, if they don't already have a link.
@@ -579,6 +574,11 @@ OperatorCreate(const char *operatorName,
if (OidIsValid(commutatorId) || OidIsValid(negatorId))
OperatorUpd(operatorObjectId, commutatorId, negatorId, false);
+ /* Post creation hook for new operator */
+ InvokeObjectPostCreateHook(OperatorRelationId, operatorObjectId, 0);
+
+ table_close(pg_operator_desc, RowExclusiveLock);
+
return address;
}
@@ -692,8 +692,28 @@ OperatorUpd(Oid baseId, Oid commId, Oid negId, bool isDelete)
t->oprcom = InvalidOid;
update_commutator = true;
}
- else if (!isDelete && !OidIsValid(t->oprcom))
+ else if (!isDelete && baseId != t->oprcom)
{
+ /*
+ * When filling in a shell operator the commutator's oprcom field
+ * will already be set to the baseId so we don't need to make an
+ * update in that case.
+ */
+
+ if (OidIsValid(t->oprcom))
+ {
+ /*
+ * When commutator's oprcom is already filled in that means
+ * that it's part of a pair of commutators and by definition
+ * the existing oprcom must be equivalent to the baseId
+ * operator. It doesn't make sense to allow the baseId alter
+ * or create to succeed in this case so raise an error.
+ */
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("commutator commutates another operator")));
+ }
+
t->oprcom = baseId;
update_commutator = true;
}
@@ -737,8 +757,28 @@ OperatorUpd(Oid baseId, Oid commId, Oid negId, bool isDelete)
t->oprnegate = InvalidOid;
update_negator = true;
}
- else if (!isDelete && !OidIsValid(t->oprnegate))
+ else if (!isDelete && baseId != t->oprnegate)
{
+ /*
+ * When filling in a shell operator the negator's oprnegate field
+ * will already be set to the baseId so we don't need to make an
+ * update in that case.
+ */
+
+ if (OidIsValid(t->oprnegate))
+ {
+ /*
+ * When negator's oprnegate is already filled in that means
+ * that it's part of a pair of negators and by definition the
+ * existing oprnegate must be equivalent to the baseId
+ * operator. It doesn't make sense to allow the baseId alter
+ * or create to succeed in this case so raise an error.
+ */
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("negator negates another operator")));
+ }
+
t->oprnegate = baseId;
update_negator = true;
}
diff --git a/src/test/regress/expected/create_operator.out b/src/test/regress/expected/create_operator.out
index 49070ce90f..600699f407 100644
--- a/src/test/regress/expected/create_operator.out
+++ b/src/test/regress/expected/create_operator.out
@@ -296,6 +296,52 @@ CREATE OPERATOR ===!!! (
);
ERROR: operator cannot be its own negator
ROLLBACK;
+-- test that we can't use part of an existing commutator or negator pair as a
+-- commutator or negator
+CREATE FUNCTION create_op_test_fn(boolean, boolean)
+RETURNS boolean AS $$
+ SELECT NULL::BOOLEAN;
+$$ LANGUAGE sql IMMUTABLE;
+CREATE OPERATOR === (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ commutator = ===@@@,
+ negator = ===!!!
+);
+CREATE OPERATOR ===!!! (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ negator = ===
+);
+CREATE OPERATOR ===@@@ (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ commutator = ===
+);
+-- test trying to use === as a commutator fails
+CREATE OPERATOR ===@ (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ commutator = ===
+);
+ERROR: commutator commutates another operator
+-- test trying to use === as a negator fails
+CREATE OPERATOR ===@ (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ negator = ===
+);
+ERROR: negator negates another operator
+-- cleanup
+DROP OPERATOR ===@@@(bool, bool);
+DROP OPERATOR ===!!!(bool, bool);
+DROP OPERATOR ===(bool, bool);
+DROP FUNCTION create_op_test_fn;
-- invalid: non-lowercase quoted identifiers
CREATE OPERATOR ===
(
diff --git a/src/test/regress/sql/create_operator.sql b/src/test/regress/sql/create_operator.sql
index 168cad3814..03e2157a57 100644
--- a/src/test/regress/sql/create_operator.sql
+++ b/src/test/regress/sql/create_operator.sql
@@ -246,6 +246,51 @@ CREATE OPERATOR ===!!! (
);
ROLLBACK;
+-- test that we can't use part of an existing commutator or negator pair as a
+-- commutator or negator
+CREATE FUNCTION create_op_test_fn(boolean, boolean)
+RETURNS boolean AS $$
+ SELECT NULL::BOOLEAN;
+$$ LANGUAGE sql IMMUTABLE;
+CREATE OPERATOR === (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ commutator = ===@@@,
+ negator = ===!!!
+);
+CREATE OPERATOR ===!!! (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ negator = ===
+);
+CREATE OPERATOR ===@@@ (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ commutator = ===
+);
+-- test trying to use === as a commutator fails
+CREATE OPERATOR ===@ (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ commutator = ===
+);
+-- test trying to use === as a negator fails
+CREATE OPERATOR ===@ (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ negator = ===
+);
+-- cleanup
+DROP OPERATOR ===@@@(bool, bool);
+DROP OPERATOR ===!!!(bool, bool);
+DROP OPERATOR ===(bool, bool);
+DROP FUNCTION create_op_test_fn;
+
-- invalid: non-lowercase quoted identifiers
CREATE OPERATOR ===
(
0003-refactor-v3.patch (application/octet-stream, 5.7K) download
diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index b0570c33b6..cda543a03b 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -360,53 +360,17 @@ OperatorCreate(const char *operatorName,
errmsg("\"%s\" is not a valid operator name",
operatorName)));
- if (!(OidIsValid(leftTypeId) && OidIsValid(rightTypeId)))
- {
- /* If it's not a binary op, these things mustn't be set: */
- if (commutatorName)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only binary operators can have commutators")));
- if (OidIsValid(joinId))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only binary operators can have join selectivity")));
- if (canMerge)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only binary operators can merge join")));
- if (canHash)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only binary operators can hash")));
- }
-
operResultType = get_func_rettype(procedureId);
- if (operResultType != BOOLOID)
- {
- /* If it's not a boolean op, these things mustn't be set: */
- if (negatorName)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can have negators")));
- if (OidIsValid(restrictionId))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can have restriction selectivity")));
- if (OidIsValid(joinId))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can have join selectivity")));
- if (canMerge)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can merge join")));
- if (canHash)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can hash")));
- }
+ OperatorValidateParams(leftTypeId,
+ rightTypeId,
+ operResultType,
+ commutatorName,
+ negatorName,
+ OidIsValid(joinId),
+ OidIsValid(restrictionId),
+ canMerge,
+ canHash);
operatorObjectId = OperatorGet(operatorName,
operatorNamespace,
@@ -763,6 +727,78 @@ OperatorUpd(Oid baseId, Oid commId, Oid negId, bool isDelete)
table_close(pg_operator_desc, RowExclusiveLock);
}
+/*
+ * OperatorValidateParams
+ *
+ * This function validates that an operator with arguments of leftTypeId and
+ * rightTypeId returning operResultType can have the attributes that are set
+ * to true. If any attributes set are not compatible with the operator then this
+ * function will raise an error, otherwise it simply returns.
+ *
+ * This function doesn't check for missing requirements so any attribute that's
+ * false is considered valid by this function. As such, if you are modifying an
+ * operator you can set any attributes you are not modifying to false to validate
+ * the changes you are making.
+ */
+
+void
+OperatorValidateParams(Oid leftTypeId,
+ Oid rightTypeId,
+ Oid operResultType,
+ bool hasCommutator,
+ bool hasNegator,
+ bool hasJoinSelectivity,
+ bool hasRestrictionSelectivity,
+ bool canMerge,
+ bool canHash)
+{
+ if (!(OidIsValid(leftTypeId) && OidIsValid(rightTypeId)))
+ {
+ /* If it's not a binary op, these things mustn't be set: */
+ if (hasCommutator)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only binary operators can have commutators")));
+ if (hasJoinSelectivity)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only binary operators can have join selectivity")));
+ if (canMerge)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only binary operators can merge join")));
+ if (canHash)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only binary operators can hash")));
+ }
+
+ if (operResultType != BOOLOID)
+ {
+ /* If it's not a boolean op, these things mustn't be set: */
+ if (hasNegator)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only boolean operators can have negators")));
+ if (hasRestrictionSelectivity)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only boolean operators can have restriction selectivity")));
+ if (hasJoinSelectivity)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only boolean operators can have join selectivity")));
+ if (canMerge)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only boolean operators can merge join")));
+ if (canHash)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("only boolean operators can hash")));
+ }
+}
+
/*
* Create dependencies for an operator (either a freshly inserted
* complete operator, a new shell operator, a just-updated shell,
diff --git a/src/include/catalog/pg_operator.h b/src/include/catalog/pg_operator.h
index aff372b4bb..a670973ab2 100644
--- a/src/include/catalog/pg_operator.h
+++ b/src/include/catalog/pg_operator.h
@@ -104,4 +104,14 @@ extern ObjectAddress makeOperatorDependencies(HeapTuple tuple,
extern void OperatorUpd(Oid baseId, Oid commId, Oid negId, bool isDelete);
+extern void OperatorValidateParams(Oid leftTypeId,
+ Oid rightTypeId,
+ Oid operResultType,
+ bool hasCommutator,
+ bool hasNegator,
+ bool hasJoinSelectivity,
+ bool hasRestrictionSelectivity,
+ bool canMerge,
+ bool canHash);
+
#endif /* PG_OPERATOR_H */
0001-bug-fixes-v3.patch (application/octet-stream, 5.5K) download
diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index 95918a77a1..b0570c33b6 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -57,8 +57,7 @@ static Oid OperatorShellMake(const char *operatorName,
static Oid get_other_operator(List *otherOp,
Oid otherLeftTypeId, Oid otherRightTypeId,
const char *operatorName, Oid operatorNamespace,
- Oid leftTypeId, Oid rightTypeId,
- bool isCommutator);
+ Oid leftTypeId, Oid rightTypeId);
/*
@@ -442,8 +441,7 @@ OperatorCreate(const char *operatorName,
commutatorId = get_other_operator(commutatorName,
rightTypeId, leftTypeId,
operatorName, operatorNamespace,
- leftTypeId, rightTypeId,
- true);
+ leftTypeId, rightTypeId);
/* Permission check: must own other operator */
if (OidIsValid(commutatorId) &&
@@ -467,8 +465,19 @@ OperatorCreate(const char *operatorName,
negatorId = get_other_operator(negatorName,
leftTypeId, rightTypeId,
operatorName, operatorNamespace,
- leftTypeId, rightTypeId,
- false);
+ leftTypeId, rightTypeId);
+
+ /*
+ * Prevent self negation as it doesn't make sense. When self negating,
+ * get_other_operator returns InvalidOid when creating a new operator
+ * and the OID of the operator we are 'creating' when upgrading a
+ * shell operator.
+ */
+
+ if (!OidIsValid(negatorId) || operatorObjectId == negatorId)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator cannot be its own negator")));
/* Permission check: must own other operator */
if (OidIsValid(negatorId) &&
@@ -584,7 +593,7 @@ OperatorCreate(const char *operatorName,
static Oid
get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
const char *operatorName, Oid operatorNamespace,
- Oid leftTypeId, Oid rightTypeId, bool isCommutator)
+ Oid leftTypeId, Oid rightTypeId)
{
Oid other_oid;
bool otherDefined;
@@ -611,14 +620,7 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
otherLeftTypeId == leftTypeId &&
otherRightTypeId == rightTypeId)
{
- /*
- * self-linkage to this operator; caller will fix later. Note that
- * only self-linkage for commutation makes sense.
- */
- if (!isCommutator)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("operator cannot be its own negator or sort operator")));
+ /* self-linkage to this operator; caller will fix later. */
return InvalidOid;
}
diff --git a/src/test/regress/expected/create_operator.out b/src/test/regress/expected/create_operator.out
index f71b601f2d..49070ce90f 100644
--- a/src/test/regress/expected/create_operator.out
+++ b/src/test/regress/expected/create_operator.out
@@ -260,6 +260,42 @@ CREATE OPERATOR #*# (
);
ERROR: permission denied for type type_op6
ROLLBACK;
+-- Should fail. An operator cannot have a self negator.
+BEGIN TRANSACTION;
+CREATE FUNCTION create_op_test_fn(boolean, boolean)
+RETURNS boolean AS $$
+ SELECT NULL::BOOLEAN;
+$$ LANGUAGE sql IMMUTABLE;
+CREATE OPERATOR === (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ negator = ===
+);
+ERROR: operator cannot be its own negator
+ROLLBACK;
+-- Should fail. An operator cannot have a self negator. Here we test that when
+-- 'creating' an existing shell operator, it checks the negator is not self.
+BEGIN TRANSACTION;
+CREATE FUNCTION create_op_test_fn(boolean, boolean)
+RETURNS boolean AS $$
+ SELECT NULL::BOOLEAN;
+$$ LANGUAGE sql IMMUTABLE;
+-- create a shell operator for ===!!! by referencing it as a commutator
+CREATE OPERATOR === (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ commutator = ===!!!
+);
+CREATE OPERATOR ===!!! (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ negator = ===!!!
+);
+ERROR: operator cannot be its own negator
+ROLLBACK;
-- invalid: non-lowercase quoted identifiers
CREATE OPERATOR ===
(
diff --git a/src/test/regress/sql/create_operator.sql b/src/test/regress/sql/create_operator.sql
index f53e24db3c..168cad3814 100644
--- a/src/test/regress/sql/create_operator.sql
+++ b/src/test/regress/sql/create_operator.sql
@@ -210,6 +210,42 @@ CREATE OPERATOR #*# (
);
ROLLBACK;
+-- Should fail. An operator cannot have a self negator.
+BEGIN TRANSACTION;
+CREATE FUNCTION create_op_test_fn(boolean, boolean)
+RETURNS boolean AS $$
+ SELECT NULL::BOOLEAN;
+$$ LANGUAGE sql IMMUTABLE;
+CREATE OPERATOR === (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ negator = ===
+);
+ROLLBACK;
+
+-- Should fail. An operator cannot have a self negator. Here we test that when
+-- 'creating' an existing shell operator, it checks the negator is not self.
+BEGIN TRANSACTION;
+CREATE FUNCTION create_op_test_fn(boolean, boolean)
+RETURNS boolean AS $$
+ SELECT NULL::BOOLEAN;
+$$ LANGUAGE sql IMMUTABLE;
+-- create a shell operator for ===!!! by referencing it as a commutator
+CREATE OPERATOR === (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ commutator = ===!!!
+);
+CREATE OPERATOR ===!!! (
+ leftarg = boolean,
+ rightarg = boolean,
+ procedure = create_op_test_fn,
+ negator = ===!!!
+);
+ROLLBACK;
+
-- invalid: non-lowercase quoted identifiers
CREATE OPERATOR ===
(
0004-alter_op-v4.patch (application/octet-stream, 40.2K) download
diff --git a/doc/src/sgml/ref/alter_operator.sgml b/doc/src/sgml/ref/alter_operator.sgml
index a4a1af564f..ce447e22b0 100644
--- a/doc/src/sgml/ref/alter_operator.sgml
+++ b/doc/src/sgml/ref/alter_operator.sgml
@@ -29,8 +29,12 @@ ALTER OPERATOR <replaceable>name</replaceable> ( { <replaceable>left_type</repla
ALTER OPERATOR <replaceable>name</replaceable> ( { <replaceable>left_type</replaceable> | NONE } , <replaceable>right_type</replaceable> )
SET ( { RESTRICT = { <replaceable class="parameter">res_proc</replaceable> | NONE }
- | JOIN = { <replaceable class="parameter">join_proc</replaceable> | NONE }
- } [, ... ] )
+ | JOIN = { <replaceable class="parameter">join_proc</replaceable> | NONE }
+ | COMMUTATOR = <replaceable class="parameter">com_op</replaceable>
+ | NEGATOR = <replaceable class="parameter">neg_op</replaceable>
+ | HASHES
+ | MERGES
+ } [, ... ] )
</synopsis>
</refsynopsisdiv>
@@ -121,9 +125,64 @@ ALTER OPERATOR <replaceable>name</replaceable> ( { <replaceable>left_type</repla
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><replaceable class="parameter">com_op</replaceable></term>
+ <listitem>
+ <para>
+ The commutator of this operator. Can only be set if the operator does not have an existing commutator.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><replaceable class="parameter">neg_op</replaceable></term>
+ <listitem>
+ <para>
+ The negator of this operator. Can only be set if the operator does not have an existing negator.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>HASHES</literal></term>
+ <listitem>
+ <para>
+ Indicates this operator can support a hash join. Can only be enabled and not disabled.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>MERGES</literal></term>
+ <listitem>
+ <para>
+ Indicates this operator can support a merge join. Can only be enabled and not disabled.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</refsect1>
+ <refsect1>
+ <title>Notes</title>
+
+ <para>
+ Refer to <xref linkend="xoper"/> and <xref linkend="xoper-optimization"/> for further information.
+ </para>
+
+ <para>
+ Since commutators come in pairs and are commutators of each other,
+ <productname>PostgreSQL</productname> will set the commutator of the
+ <replaceable class="parameter">com_op</replaceable> to the operator.
+ </para>
+
+ <para>
+ Like with commutators, <productname>PostgreSQL</productname> will set the
+ negator of the <replaceable class="parameter">neg_op</replaceable> to the operator.
+ </para>
+ </refsect1>
+
<refsect1>
<title>Examples</title>
diff --git a/doc/src/sgml/ref/create_operator.sgml b/doc/src/sgml/ref/create_operator.sgml
index e27512ff39..9801eb3ed0 100644
--- a/doc/src/sgml/ref/create_operator.sgml
+++ b/doc/src/sgml/ref/create_operator.sgml
@@ -231,9 +231,60 @@ COMMUTATOR = OPERATOR(myschema.===) ,
<title>Notes</title>
<para>
- Refer to <xref linkend="xoper"/> for further information.
+ Refer to <xref linkend="xoper"/> and <xref linkend="xoper-optimization"/> for further information.
</para>
+ <para>
+ When you are defining a self-commutative operator, you just do it.
+ When you are defining a pair of commutative operators, things are
+ a little trickier: how can the first one to be defined refer to the
+ other one, which you haven't defined yet? There are three solutions
+ to this problem:
+
+ <itemizedlist>
+ <listitem>
+ <para>
+ One way is to omit the <literal>COMMUTATOR</literal> clause in the first operator that
+ you define, and then provide one in the second operator's definition.
+ Since <productname>PostgreSQL</productname> knows that commutative
+ operators come in pairs, when it sees the second definition it will
+ automatically go back and fill in the missing <literal>COMMUTATOR</literal> clause in
+ the first definition.
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ Another, more straightforward way is just to include <literal>COMMUTATOR</literal> clauses
+ in both definitions. When <productname>PostgreSQL</productname> processes
+ the first definition and realizes that <literal>COMMUTATOR</literal> refers to a nonexistent
+ operator, the system will make a dummy entry for that operator in the
+ system catalog. This dummy entry will have valid data only
+ for the operator name, left and right operand types, and result type,
+ since that's all that <productname>PostgreSQL</productname> can deduce
+ at this point. The first operator's catalog entry will link to this
+ dummy entry. Later, when you define the second operator, the system
+ updates the dummy entry with the additional information from the second
+ definition. If you try to use the dummy operator before it's been filled
+ in, you'll just get an error message.
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ Alternatively, both operators can be defined without a <literal>COMMUTATOR</literal>
+ and then <command>ALTER OPERATOR</command> can be used to set the <literal>COMMUTATOR</literal>
+ for the operator.
+ </para>
+ </listitem>
+ </itemizedlist>
+ </para>
+
+ <para>
+ Pairs of negator operators can be defined using the same methods
+ explained above for commutator pairs.
+ </para>
+
<para>
It is not possible to specify an operator's lexical precedence in
<command>CREATE OPERATOR</command>, because the parser's precedence behavior
diff --git a/doc/src/sgml/xoper.sgml b/doc/src/sgml/xoper.sgml
index a929ced07d..31bc8df3de 100644
--- a/doc/src/sgml/xoper.sgml
+++ b/doc/src/sgml/xoper.sgml
@@ -146,44 +146,6 @@ SELECT (a + b) AS c FROM test_complex;
<literal>=</literal> operator must specify that it is valid, by marking the
operator with commutator information.
</para>
-
- <para>
- When you are defining a self-commutative operator, you just do it.
- When you are defining a pair of commutative operators, things are
- a little trickier: how can the first one to be defined refer to the
- other one, which you haven't defined yet? There are two solutions
- to this problem:
-
- <itemizedlist>
- <listitem>
- <para>
- One way is to omit the <literal>COMMUTATOR</literal> clause in the first operator that
- you define, and then provide one in the second operator's definition.
- Since <productname>PostgreSQL</productname> knows that commutative
- operators come in pairs, when it sees the second definition it will
- automatically go back and fill in the missing <literal>COMMUTATOR</literal> clause in
- the first definition.
- </para>
- </listitem>
-
- <listitem>
- <para>
- The other, more straightforward way is just to include <literal>COMMUTATOR</literal> clauses
- in both definitions. When <productname>PostgreSQL</productname> processes
- the first definition and realizes that <literal>COMMUTATOR</literal> refers to a nonexistent
- operator, the system will make a dummy entry for that operator in the
- system catalog. This dummy entry will have valid data only
- for the operator name, left and right operand types, and result type,
- since that's all that <productname>PostgreSQL</productname> can deduce
- at this point. The first operator's catalog entry will link to this
- dummy entry. Later, when you define the second operator, the system
- updates the dummy entry with the additional information from the second
- definition. If you try to use the dummy operator before it's been filled
- in, you'll just get an error message.
- </para>
- </listitem>
- </itemizedlist>
- </para>
</sect2>
<sect2 id="xoper-negator">
@@ -218,12 +180,7 @@ SELECT (a + b) AS c FROM test_complex;
<literal>NOT</literal> operations can be inserted as a consequence of other rearrangements.
</para>
- <para>
- Pairs of negator operators can be defined using the same methods
- explained above for commutator pairs.
- </para>
-
- </sect2>
+ </sect2>
<sect2 id="xoper-restrict">
<title><literal>RESTRICT</literal></title>
diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index b75f7a9bc1..578c89b6f9 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -44,11 +44,6 @@ static Oid OperatorGet(const char *operatorName,
Oid rightObjectId,
bool *defined);
-static Oid OperatorLookup(List *operatorName,
- Oid leftObjectId,
- Oid rightObjectId,
- bool *defined);
-
static Oid OperatorShellMake(const char *operatorName,
Oid operatorNamespace,
Oid leftTypeId,
@@ -165,7 +160,7 @@ OperatorGet(const char *operatorName,
*
* *defined is set true if defined (not a shell)
*/
-static Oid
+Oid
OperatorLookup(List *operatorName,
Oid leftObjectId,
Oid rightObjectId,
diff --git a/src/backend/commands/operatorcmds.c b/src/backend/commands/operatorcmds.c
index cd7f83136f..6666bf476f 100644
--- a/src/backend/commands/operatorcmds.c
+++ b/src/backend/commands/operatorcmds.c
@@ -54,6 +54,9 @@
static Oid ValidateRestrictionEstimator(List *restrictionName);
static Oid ValidateJoinEstimator(List *joinName);
+static Oid ValidateOperatorDefined(List *name,
+ Oid leftTypeId,
+ Oid rightTypeId);
/*
* DefineOperator
@@ -359,6 +362,44 @@ ValidateJoinEstimator(List *joinName)
return joinOid;
}
+/*
+ * Looks up and returns an oid for an operator given a possibly-qualified name
+ * and left and right type IDs, verifies the operator is defined (non-shell),
+ * and owned by the current user. Raises an error otherwise.
+ */
+static Oid
+ValidateOperatorDefined(List *name,
+ Oid leftTypeId,
+ Oid rightTypeId)
+{
+ Oid oid;
+ bool defined;
+
+ oid = OperatorLookup(name,
+ leftTypeId,
+ rightTypeId,
+ &defined);
+
+ if (!OidIsValid(oid))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_FUNCTION),
+ errmsg("operator %s does not exist",
+ NameListToString(name))));
+
+ if (!defined)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_FUNCTION),
+ errmsg("operator %s is undefined shell operator",
+ NameListToString(name))));
+
+ if (!object_ownercheck(OperatorRelationId, oid, GetUserId()))
+ aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_OPERATOR,
+ NameListToString(name));
+
+ return oid;
+}
+
+
/*
* Guts of operator deletion.
*/
@@ -406,6 +447,8 @@ RemoveOperatorById(Oid operOid)
* routine implementing ALTER OPERATOR <operator> SET (option = ...).
*
* Currently, only RESTRICT and JOIN estimator functions can be changed.
+ * COMMUTATOR and NEGATOR, HASHES, and MERGES can be set if they have not been set
+ * previously.
*/
ObjectAddress
AlterOperator(AlterOperatorStmt *stmt)
@@ -426,6 +469,14 @@ AlterOperator(AlterOperatorStmt *stmt)
List *joinName = NIL; /* optional join sel. function */
bool updateJoin = false;
Oid joinOid;
+ List *commutatorName = NIL; /* optional commutator operator name */
+ Oid commutatorOid;
+ List *negatorName = NIL; /* optional negator operator name */
+ Oid negatorOid;
+ bool updateHashes = false;
+ bool canHash = false;
+ bool updateMerges = false;
+ bool canMerge = false;
/* Look up the operator */
oprId = LookupOperWithArgs(stmt->opername, false);
@@ -456,6 +507,24 @@ AlterOperator(AlterOperatorStmt *stmt)
joinName = param;
updateJoin = true;
}
+ else if (strcmp(defel->defname, "commutator") == 0)
+ {
+ commutatorName = defGetQualifiedName(defel);
+ }
+ else if (strcmp(defel->defname, "negator") == 0)
+ {
+ negatorName = defGetQualifiedName(defel);
+ }
+ else if (strcmp(defel->defname, "hashes") == 0)
+ {
+ canHash = defGetBoolean(defel);
+ updateHashes = true;
+ }
+ else if (strcmp(defel->defname, "merges") == 0)
+ {
+ canMerge = defGetBoolean(defel);
+ updateMerges = true;
+ }
/*
* The rest of the options that CREATE accepts cannot be changed.
@@ -464,11 +533,7 @@ AlterOperator(AlterOperatorStmt *stmt)
else if (strcmp(defel->defname, "leftarg") == 0 ||
strcmp(defel->defname, "rightarg") == 0 ||
strcmp(defel->defname, "function") == 0 ||
- strcmp(defel->defname, "procedure") == 0 ||
- strcmp(defel->defname, "commutator") == 0 ||
- strcmp(defel->defname, "negator") == 0 ||
- strcmp(defel->defname, "hashes") == 0 ||
- strcmp(defel->defname, "merges") == 0)
+ strcmp(defel->defname, "procedure") == 0)
{
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -488,7 +553,7 @@ AlterOperator(AlterOperatorStmt *stmt)
NameStr(oprForm->oprname));
/*
- * Look up restriction and join estimators if specified
+ * Look up Oid for any parameters specified
*/
if (restrictionName)
restrictionOid = ValidateRestrictionEstimator(restrictionName);
@@ -499,28 +564,85 @@ AlterOperator(AlterOperatorStmt *stmt)
else
joinOid = InvalidOid;
- /* Perform additional checks, like OperatorCreate does */
- if (!(OidIsValid(oprForm->oprleft) && OidIsValid(oprForm->oprright)))
+ if (commutatorName)
{
- /* If it's not a binary op, these things mustn't be set: */
- if (OidIsValid(joinOid))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only binary operators can have join selectivity")));
+ /* commutator has reversed arg types */
+ commutatorOid = ValidateOperatorDefined(commutatorName,
+ oprForm->oprright,
+ oprForm->oprleft);
+
+ /*
+ * we don't need to do anything extra for a self commutator as in
+ * OperatorCreate as there we have to create the operator before
+ * setting the commutator, but here the operator already exists and
+ * the commutatorOid above is valid (and is the operator oid).
+ */
}
+ else
+ commutatorOid = InvalidOid;
- if (oprForm->oprresult != BOOLOID)
+ if (negatorName)
{
- if (OidIsValid(restrictionOid))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can have restriction selectivity")));
- if (OidIsValid(joinOid))
+ negatorOid = ValidateOperatorDefined(negatorName,
+ oprForm->oprleft,
+ oprForm->oprright);
+
+ if (negatorOid == oprForm->oid)
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("only boolean operators can have join selectivity")));
+ errmsg("operator cannot be its own negator")));
+ }
+ else
+ {
+ negatorOid = InvalidOid;
+ }
+
+ /*
+ * check that we're not changing any existing values that might be
+ * depended on elsewhere and may be expected to never change, while
+ * allowing no-ops.
+ */
+ if (OidIsValid(commutatorOid) && OidIsValid(oprForm->oprcom)
+ && commutatorOid != oprForm->oprcom)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator attribute \"commutator\" cannot be changed if it has already been set")));
+ }
+
+ if (OidIsValid(negatorOid) && OidIsValid(oprForm->oprnegate)
+ && negatorOid != oprForm->oprnegate)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator attribute \"negator\" cannot be changed if it has already been set")));
+ }
+
+ if (updateHashes && oprForm->oprcanhash && !canHash)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator attribute \"hashes\" cannot be disabled after it has been enabled")));
+ }
+
+ if (updateMerges && oprForm->oprcanmerge && !canMerge)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator attribute \"merges\" cannot be disabled after it has been enabled")));
}
+ /* Perform additional checks, like OperatorCreate does */
+ OperatorValidateParams(oprForm->oprleft,
+ oprForm->oprright,
+ oprForm->oprresult,
+ OidIsValid(commutatorOid),
+ OidIsValid(negatorOid),
+ OidIsValid(joinOid),
+ OidIsValid(restrictionOid),
+ updateMerges,
+ updateHashes);
+
/* Update the tuple */
for (i = 0; i < Natts_pg_operator; ++i)
{
@@ -539,6 +661,30 @@ AlterOperator(AlterOperatorStmt *stmt)
values[Anum_pg_operator_oprjoin - 1] = joinOid;
}
+ if (OidIsValid(commutatorOid))
+ {
+ replaces[Anum_pg_operator_oprcom - 1] = true;
+ values[Anum_pg_operator_oprcom - 1] = ObjectIdGetDatum(commutatorOid);
+ }
+
+ if (OidIsValid(negatorOid))
+ {
+ replaces[Anum_pg_operator_oprnegate - 1] = true;
+ values[Anum_pg_operator_oprnegate - 1] = ObjectIdGetDatum(negatorOid);
+ }
+
+ if (updateMerges)
+ {
+ replaces[Anum_pg_operator_oprcanmerge - 1] = true;
+ values[Anum_pg_operator_oprcanmerge - 1] = canMerge;
+ }
+
+ if (updateHashes)
+ {
+ replaces[Anum_pg_operator_oprcanhash - 1] = true;
+ values[Anum_pg_operator_oprcanhash - 1] = canHash;
+ }
+
tup = heap_modify_tuple(tup, RelationGetDescr(catalog),
values, nulls, replaces);
@@ -546,6 +692,9 @@ AlterOperator(AlterOperatorStmt *stmt)
address = makeOperatorDependencies(tup, false, true);
+ if (OidIsValid(commutatorOid) || OidIsValid(negatorOid))
+ OperatorUpd(oprId, commutatorOid, negatorOid, false);
+
InvokeObjectPostAlterHook(OperatorRelationId, oprId, 0);
table_close(catalog, NoLock);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index e56cbe77cb..0c7a1f77e0 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -10151,6 +10151,8 @@ operator_def_elem: ColLabel '=' NONE
{ $$ = makeDefElem($1, NULL, @1); }
| ColLabel '=' operator_def_arg
{ $$ = makeDefElem($1, (Node *) $3, @1); }
+ | ColLabel
+ { $$ = makeDefElem($1, NULL, @1); }
;
/* must be similar enough to def_arg to avoid reduce/reduce conflicts */
diff --git a/src/include/catalog/pg_operator.h b/src/include/catalog/pg_operator.h
index a670973ab2..13ebc592f2 100644
--- a/src/include/catalog/pg_operator.h
+++ b/src/include/catalog/pg_operator.h
@@ -85,6 +85,11 @@ typedef FormData_pg_operator *Form_pg_operator;
DECLARE_UNIQUE_INDEX_PKEY(pg_operator_oid_index, 2688, OperatorOidIndexId, pg_operator, btree(oid oid_ops));
DECLARE_UNIQUE_INDEX(pg_operator_oprname_l_r_n_index, 2689, OperatorNameNspIndexId, pg_operator, btree(oprname name_ops, oprleft oid_ops, oprright oid_ops, oprnamespace oid_ops));
+extern Oid
+ OperatorLookup(List *operatorName,
+ Oid leftObjectId,
+ Oid rightObjectId,
+ bool *defined);
extern ObjectAddress OperatorCreate(const char *operatorName,
Oid operatorNamespace,
diff --git a/src/test/regress/expected/alter_operator.out b/src/test/regress/expected/alter_operator.out
index 71bd484282..ea5d79b054 100644
--- a/src/test/regress/expected/alter_operator.out
+++ b/src/test/regress/expected/alter_operator.out
@@ -25,7 +25,7 @@ ORDER BY 1;
(3 rows)
--
--- Reset and set params
+-- test reset and set restrict and join
--
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = NONE);
ALTER OPERATOR === (boolean, boolean) SET (JOIN = NONE);
@@ -106,34 +106,282 @@ ORDER BY 1;
schema public | n
(3 rows)
---
--- Test invalid options.
---
-ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = ====);
-ERROR: operator attribute "commutator" cannot be changed
-ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = ====);
-ERROR: operator attribute "negator" cannot be changed
+-- test cannot set non existant function
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = non_existent_func);
ERROR: function non_existent_func(internal, oid, internal, integer) does not exist
ALTER OPERATOR === (boolean, boolean) SET (JOIN = non_existent_func);
ERROR: function non_existent_func(internal, oid, internal, smallint, internal) does not exist
-ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = !==);
-ERROR: operator attribute "commutator" cannot be changed
-ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = !==);
-ERROR: operator attribute "negator" cannot be changed
--- invalid: non-lowercase quoted identifiers
+-- test non-lowercase quoted identifiers invalid
ALTER OPERATOR & (bit, bit) SET ("Restrict" = _int_contsel, "Join" = _int_contjoinsel);
ERROR: operator attribute "Restrict" not recognized
--
--- Test permission check. Must be owner to ALTER OPERATOR.
+-- test must be owner of operator to ALTER OPERATOR.
--
CREATE USER regress_alter_op_user;
SET SESSION AUTHORIZATION regress_alter_op_user;
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = NONE);
ERROR: must be owner of operator ===
--- Clean up
RESET SESSION AUTHORIZATION;
+--
+-- test set commutator, negator, hashes, and merges which can only be set if not
+-- already set
+--
+-- for these tests create operators with different left and right types so that
+-- we can validate that function signatures are handled correctly
+CREATE FUNCTION alter_op_test_fn_bool_real(boolean, real)
+RETURNS boolean AS $$ SELECT NULL::BOOLEAN; $$ LANGUAGE sql IMMUTABLE;
+CREATE FUNCTION alter_op_test_fn_real_bool(real, boolean)
+RETURNS boolean AS $$ SELECT NULL::BOOLEAN; $$ LANGUAGE sql IMMUTABLE;
+-- operator
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+-- commutator
+CREATE OPERATOR ==== (
+ LEFTARG = real,
+ RIGHTARG = boolean,
+ PROCEDURE = alter_op_test_fn_real_bool
+);
+-- negator
+CREATE OPERATOR !==== (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+-- test no-op setting already false hashes and merges to false works
+ALTER OPERATOR === (boolean, real) SET (HASHES = false);
+ALTER OPERATOR === (boolean, real) SET (MERGES = false);
+-- validate still false after no-op
+SELECT oprcanhash, oprcanmerge FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanhash | oprcanmerge
+------------+-------------
+ f | f
+(1 row)
+
+-- test cannot set commutator or negator without owning them
+SET SESSION AUTHORIZATION regress_alter_op_user;
+-- we need need a new operator owned by regress_alter_op_user so that we are
+-- allowed to alter it. ==== and !==== are owned by the test user so we expect
+-- the alters below to fail.
+CREATE OPERATOR ===@@@ (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+ALTER OPERATOR ===@@@ (boolean, real) SET (COMMUTATOR= ====);
+ERROR: must be owner of operator ====
+ALTER OPERATOR ===@@@ (boolean, real) SET (NEGATOR = !====);
+ERROR: must be owner of operator !====
+-- validate operator is unchanged and commutator and negator are unset
+SELECT oprcom, oprnegate FROM pg_operator WHERE oprname = '===@@@'
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcom | oprnegate
+--------+-----------
+ 0 | 0
+(1 row)
+
+DROP OPERATOR ===@@@ (boolean, real);
+RESET SESSION AUTHORIZATION;
+-- test cannot set self negator
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = ===);
+ERROR: operator cannot be its own negator
+-- validate no changes made
+SELECT oprcanmerge, oprcanhash, oprcom, oprnegate
+FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanmerge | oprcanhash | oprcom | oprnegate
+-------------+------------+--------+-----------
+ f | f | 0 | 0
+(1 row)
+
+-- test set hashes
+ALTER OPERATOR === (boolean, real) SET (HASHES);
+SELECT oprcanhash FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanhash
+------------
+ t
+(1 row)
+
+-- test set merges
+ALTER OPERATOR === (boolean, real) SET (MERGES);
+SELECT oprcanmerge FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanmerge
+-------------
+ t
+(1 row)
+
+-- test set commutator
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ====);
+-- validate that the commutator has been set on both the operator and commutator,
+-- that they reference each other, and that the operator used is the existing
+-- one we created and not a new shell operator
+SELECT op.oprname AS operator_name, com.oprname AS commutator_name,
+ com.oprcode AS commutator_func
+ FROM pg_operator op
+ INNER JOIN pg_operator com ON (op.oid = com.oprcom AND op.oprcom = com.oid)
+ WHERE op.oprname = '==='
+ AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype;
+ operator_name | commutator_name | commutator_func
+---------------+-----------------+----------------------------
+ === | ==== | alter_op_test_fn_real_bool
+(1 row)
+
+-- test set negator
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !====);
+-- validate that the negator has been set on both the operator and negator, that
+-- they reference each other, and that the operator used is the existing one we
+-- created and not a new shell operator
+SELECT op.oprname AS operator_name, neg.oprname AS negator_name,
+ neg.oprcode AS negator_func
+ FROM pg_operator op
+ INNER JOIN pg_operator neg ON (op.oid = neg.oprnegate AND op.oprnegate = neg.oid)
+ WHERE op.oprname = '==='
+ AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype;
+ operator_name | negator_name | negator_func
+---------------+--------------+----------------------------
+ === | !==== | alter_op_test_fn_bool_real
+(1 row)
+
+-- validate that the final state of the operator is as we expect
+SELECT oprcanmerge, oprcanhash,
+ pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator,
+ pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator
+ FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanmerge | oprcanhash | commutator | negator
+-------------+------------+-----------------------------+------------------------------
+ t | t | operator ====(real,boolean) | operator !====(boolean,real)
+(1 row)
+
+-- test no-op set 'succeeds'
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !====);
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ====);
+ALTER OPERATOR === (boolean, real) SET (HASHES);
+ALTER OPERATOR === (boolean, real) SET (MERGES);
+-- test cannot change commutator, negator, hashes, and merges when already set
+CREATE OPERATOR @= (
+ LEFTARG = real,
+ RIGHTARG = boolean,
+ PROCEDURE = alter_op_test_fn_real_bool
+);
+CREATE OPERATOR @!= (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = @=);
+ERROR: operator attribute "commutator" cannot be changed if it has already been set
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = @!=);
+ERROR: operator attribute "negator" cannot be changed if it has already been set
+ALTER OPERATOR === (boolean, real) SET (HASHES = false);
+ERROR: operator attribute "hashes" cannot be disabled after it has been enabled
+ALTER OPERATOR === (boolean, real) SET (MERGES = false);
+ERROR: operator attribute "merges" cannot be disabled after it has been enabled
+-- test cannot set an operator that aleady has a commutator as the commutator
+ALTER OPERATOR @=(real, boolean) SET (COMMUTATOR = ===);
+ERROR: commutator commutates another operator
+SELECT oprcom FROM pg_operator WHERE oprname = '@=';
+ oprcom
+--------
+ 0
+(1 row)
+
+-- test cannot set an operator that already has a negator as the negator
+ALTER OPERATOR @!=(boolean, real) SET (NEGATOR = ===);
+ERROR: negator negates another operator
+SELECT oprnegate FROM pg_operator WHERE oprname = '@!=';
+ oprnegate
+-----------
+ 0
+(1 row)
+
+-- validate no changes made
+SELECT oprcanmerge, oprcanhash,
+ pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator,
+ pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator
+ FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanmerge | oprcanhash | commutator | negator
+-------------+------------+-----------------------------+------------------------------
+ t | t | operator ====(real,boolean) | operator !====(boolean,real)
+(1 row)
+
+DROP OPERATOR @=(real, boolean);
+DROP OPERATOR @!=(boolean, real);
+--
+-- test setting commutator and negator to undefined or shell operator fails
+--
+DROP OPERATOR === (boolean, real);
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+-- test cannot set undefined
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ===@@@);
+ERROR: operator ===@@@ does not exist
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !===@@@);
+ERROR: operator !===@@@ does not exist
+-- create shell operators for ===@@@ and !===@@@
+CREATE OPERATOR ===@@ (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real,
+ COMMUTATOR = ===@@@,
+ NEGATOR = !===@@@
+);
+-- test cannot set shell operators
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ===@@@);
+ERROR: operator ===@@@ is undefined shell operator
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !===@@@);
+ERROR: operator !===@@@ is undefined shell operator
+-- validate no changes made
+SELECT oprcanmerge, oprcanhash,
+ pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator,
+ pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator
+ FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+ oprcanmerge | oprcanhash | commutator | negator
+-------------+------------+------------+---------
+ f | f | |
+(1 row)
+
+DROP OPERATOR ===@@(boolean, real);
+DROP OPERATOR ===@@@(real, boolean);
+DROP OPERATOR !===@@@(boolean, real);
+--
+-- test setting self commutator
+--
+DROP OPERATOR === (boolean, boolean);
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = boolean,
+ PROCEDURE = alter_op_test_fn
+);
+ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = ===);
+-- validate that the oprcom is the operator oid
+SELECT oprname FROM pg_operator
+ WHERE oprname = '===' AND oid = oprcom
+ AND oprleft = 'boolean'::regtype AND oprright = 'boolean'::regtype;
+ oprname
+---------
+ ===
+(1 row)
+
+--
+-- Clean up
+--
DROP USER regress_alter_op_user;
DROP OPERATOR === (boolean, boolean);
+DROP OPERATOR === (boolean, real);
+DROP OPERATOR ==== (real, boolean);
+DROP OPERATOR !==== (boolean, real);
DROP FUNCTION customcontsel(internal, oid, internal, integer);
DROP FUNCTION alter_op_test_fn(boolean, boolean);
+DROP FUNCTION alter_op_test_fn_bool_real(boolean, real);
+DROP FUNCTION alter_op_test_fn_real_bool(real, boolean);
diff --git a/src/test/regress/sql/alter_operator.sql b/src/test/regress/sql/alter_operator.sql
index fd40370165..85004d4191 100644
--- a/src/test/regress/sql/alter_operator.sql
+++ b/src/test/regress/sql/alter_operator.sql
@@ -22,7 +22,7 @@ WHERE classid = 'pg_operator'::regclass AND
ORDER BY 1;
--
--- Reset and set params
+-- test reset and set restrict and join
--
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = NONE);
@@ -71,30 +71,251 @@ WHERE classid = 'pg_operator'::regclass AND
objid = '===(bool,bool)'::regoperator
ORDER BY 1;
---
--- Test invalid options.
---
-ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = ====);
-ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = ====);
+-- test cannot set non existant function
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = non_existent_func);
ALTER OPERATOR === (boolean, boolean) SET (JOIN = non_existent_func);
-ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = !==);
-ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = !==);
--- invalid: non-lowercase quoted identifiers
+-- test non-lowercase quoted identifiers invalid
ALTER OPERATOR & (bit, bit) SET ("Restrict" = _int_contsel, "Join" = _int_contjoinsel);
--
--- Test permission check. Must be owner to ALTER OPERATOR.
+-- test must be owner of operator to ALTER OPERATOR.
--
CREATE USER regress_alter_op_user;
SET SESSION AUTHORIZATION regress_alter_op_user;
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = NONE);
--- Clean up
RESET SESSION AUTHORIZATION;
+
+--
+-- test set commutator, negator, hashes, and merges which can only be set if not
+-- already set
+--
+
+-- for these tests create operators with different left and right types so that
+-- we can validate that function signatures are handled correctly
+
+CREATE FUNCTION alter_op_test_fn_bool_real(boolean, real)
+RETURNS boolean AS $$ SELECT NULL::BOOLEAN; $$ LANGUAGE sql IMMUTABLE;
+
+CREATE FUNCTION alter_op_test_fn_real_bool(real, boolean)
+RETURNS boolean AS $$ SELECT NULL::BOOLEAN; $$ LANGUAGE sql IMMUTABLE;
+
+-- operator
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+
+-- commutator
+CREATE OPERATOR ==== (
+ LEFTARG = real,
+ RIGHTARG = boolean,
+ PROCEDURE = alter_op_test_fn_real_bool
+);
+
+-- negator
+CREATE OPERATOR !==== (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+
+-- test no-op setting already false hashes and merges to false works
+ALTER OPERATOR === (boolean, real) SET (HASHES = false);
+ALTER OPERATOR === (boolean, real) SET (MERGES = false);
+
+-- validate still false after no-op
+SELECT oprcanhash, oprcanmerge FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+-- test cannot set commutator or negator without owning them
+SET SESSION AUTHORIZATION regress_alter_op_user;
+
+-- we need need a new operator owned by regress_alter_op_user so that we are
+-- allowed to alter it. ==== and !==== are owned by the test user so we expect
+-- the alters below to fail.
+CREATE OPERATOR ===@@@ (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+
+ALTER OPERATOR ===@@@ (boolean, real) SET (COMMUTATOR= ====);
+ALTER OPERATOR ===@@@ (boolean, real) SET (NEGATOR = !====);
+
+-- validate operator is unchanged and commutator and negator are unset
+SELECT oprcom, oprnegate FROM pg_operator WHERE oprname = '===@@@'
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+DROP OPERATOR ===@@@ (boolean, real);
+
+RESET SESSION AUTHORIZATION;
+
+-- test cannot set self negator
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = ===);
+
+-- validate no changes made
+SELECT oprcanmerge, oprcanhash, oprcom, oprnegate
+FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+-- test set hashes
+ALTER OPERATOR === (boolean, real) SET (HASHES);
+SELECT oprcanhash FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+-- test set merges
+ALTER OPERATOR === (boolean, real) SET (MERGES);
+SELECT oprcanmerge FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+-- test set commutator
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ====);
+
+-- validate that the commutator has been set on both the operator and commutator,
+-- that they reference each other, and that the operator used is the existing
+-- one we created and not a new shell operator
+SELECT op.oprname AS operator_name, com.oprname AS commutator_name,
+ com.oprcode AS commutator_func
+ FROM pg_operator op
+ INNER JOIN pg_operator com ON (op.oid = com.oprcom AND op.oprcom = com.oid)
+ WHERE op.oprname = '==='
+ AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype;
+
+-- test set negator
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !====);
+
+-- validate that the negator has been set on both the operator and negator, that
+-- they reference each other, and that the operator used is the existing one we
+-- created and not a new shell operator
+SELECT op.oprname AS operator_name, neg.oprname AS negator_name,
+ neg.oprcode AS negator_func
+ FROM pg_operator op
+ INNER JOIN pg_operator neg ON (op.oid = neg.oprnegate AND op.oprnegate = neg.oid)
+ WHERE op.oprname = '==='
+ AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype;
+
+-- validate that the final state of the operator is as we expect
+SELECT oprcanmerge, oprcanhash,
+ pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator,
+ pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator
+ FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+-- test no-op set 'succeeds'
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !====);
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ====);
+ALTER OPERATOR === (boolean, real) SET (HASHES);
+ALTER OPERATOR === (boolean, real) SET (MERGES);
+
+-- test cannot change commutator, negator, hashes, and merges when already set
+
+CREATE OPERATOR @= (
+ LEFTARG = real,
+ RIGHTARG = boolean,
+ PROCEDURE = alter_op_test_fn_real_bool
+);
+CREATE OPERATOR @!= (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = @=);
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = @!=);
+ALTER OPERATOR === (boolean, real) SET (HASHES = false);
+ALTER OPERATOR === (boolean, real) SET (MERGES = false);
+
+-- test cannot set an operator that aleady has a commutator as the commutator
+ALTER OPERATOR @=(real, boolean) SET (COMMUTATOR = ===);
+SELECT oprcom FROM pg_operator WHERE oprname = '@=';
+
+-- test cannot set an operator that already has a negator as the negator
+ALTER OPERATOR @!=(boolean, real) SET (NEGATOR = ===);
+SELECT oprnegate FROM pg_operator WHERE oprname = '@!=';
+
+-- validate no changes made
+SELECT oprcanmerge, oprcanhash,
+ pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator,
+ pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator
+ FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+DROP OPERATOR @=(real, boolean);
+DROP OPERATOR @!=(boolean, real);
+
+--
+-- test setting commutator and negator to undefined or shell operator fails
+--
+
+DROP OPERATOR === (boolean, real);
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real
+);
+
+-- test cannot set undefined
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ===@@@);
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !===@@@);
+
+-- create shell operators for ===@@@ and !===@@@
+CREATE OPERATOR ===@@ (
+ LEFTARG = boolean,
+ RIGHTARG = real,
+ PROCEDURE = alter_op_test_fn_bool_real,
+ COMMUTATOR = ===@@@,
+ NEGATOR = !===@@@
+);
+
+-- test cannot set shell operators
+ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ===@@@);
+ALTER OPERATOR === (boolean, real) SET (NEGATOR = !===@@@);
+
+-- validate no changes made
+SELECT oprcanmerge, oprcanhash,
+ pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator,
+ pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator
+ FROM pg_operator WHERE oprname = '==='
+ AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype;
+
+DROP OPERATOR ===@@(boolean, real);
+DROP OPERATOR ===@@@(real, boolean);
+DROP OPERATOR !===@@@(boolean, real);
+
+--
+-- test setting self commutator
+--
+
+DROP OPERATOR === (boolean, boolean);
+CREATE OPERATOR === (
+ LEFTARG = boolean,
+ RIGHTARG = boolean,
+ PROCEDURE = alter_op_test_fn
+);
+
+ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = ===);
+
+-- validate that the oprcom is the operator oid
+SELECT oprname FROM pg_operator
+ WHERE oprname = '===' AND oid = oprcom
+ AND oprleft = 'boolean'::regtype AND oprright = 'boolean'::regtype;
+
+--
+-- Clean up
+--
+
DROP USER regress_alter_op_user;
+
DROP OPERATOR === (boolean, boolean);
+DROP OPERATOR === (boolean, real);
+DROP OPERATOR ==== (real, boolean);
+DROP OPERATOR !==== (boolean, real);
+
DROP FUNCTION customcontsel(internal, oid, internal, integer);
DROP FUNCTION alter_op_test_fn(boolean, boolean);
+DROP FUNCTION alter_op_test_fn_bool_real(boolean, real);
+DROP FUNCTION alter_op_test_fn_real_bool(real, boolean);
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-07-02 14:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-09-28 20:18 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-10 20:12 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-10 20:32 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-11 15:11 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
@ 2023-10-12 03:53 ` jian he <jian.universality@gmail.com>
2023-10-20 16:33 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 25+ messages in thread
From: jian he @ 2023-10-12 03:53 UTC (permalink / raw)
To: Tommy Pavlicek <tommypav122@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>
errmsg("operator attribute \"negator\" cannot be changed if it has
already been set")));
I feel like the above message is not very helpful.
Something like the following may be more helpful for diagnosis.
errmsg("operator %s's attribute \"negator\" cannot be changed if it
has already been set", operatorname)));
when I "git apply", I've noticed some minor whitespace warning.
Other than that, it looks fine.
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-07-02 14:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-09-28 20:18 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-10 20:12 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-10 20:32 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-11 15:11 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-12 03:53 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges jian he <jian.universality@gmail.com>
@ 2023-10-20 16:33 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 25+ messages in thread
From: Tom Lane @ 2023-10-20 16:33 UTC (permalink / raw)
To: jian he <jian.universality@gmail.com>; +Cc: Tommy Pavlicek <tommypav122@gmail.com>; Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>
jian he <jian.universality@gmail.com> writes:
> errmsg("operator attribute \"negator\" cannot be changed if it has
> already been set")));
> I feel like the above message is not very helpful.
I think it's okay to be concise about this as long as the operator
we're referring to is the target of the ALTER. I agree that when
we're complaining about some *other* operator, we'd better spell
out which one we mean, and I made some changes to the patch to
improve that.
Pushed after a round of editorialization -- mostly cosmetic
stuff, except for tweaking some error messages. I shortened the
test cases a bit too, as I thought they were somewhat excessive
to have as a permanent thing.
regards, tom lane
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-07-02 14:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-09-28 20:18 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-10 20:12 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-10 20:32 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-11 15:11 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
@ 2023-10-24 13:51 ` Christoph Berg <myon@debian.org>
2023-10-24 15:16 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-31 16:10 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
1 sibling, 2 replies; 25+ messages in thread
From: Christoph Berg @ 2023-10-24 13:51 UTC (permalink / raw)
To: Tommy Pavlicek <tommypav122@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>; jian.universality@gmail.com
Re: Tommy Pavlicek
> I've added another patch (0002-require_unused_neg_com-v1.patch) that
> prevents using a commutator or negator that's already part of a pair.
Hmm. I agree with the general idea of adding sanity checks, but this
might be overzealous:
This change is breaking pgsphere which has <@ @> operator pairs, but
for historical reasons also includes alternative spellings of these
operators (both called @ with swapped operand types) which now
explodes because we can't add them with the "proper" commutator and
negators declared (which point to the canonical <@ @> !<@ !@>
operators).
https://github.com/postgrespro/pgsphere/blob/master/pgs_moc_compat.sql.in
We might be able to simply delete the @ operators, but doesn't this
new check break the general possibility to have more than one spelling
for the same operator?
Christoph
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-07-02 14:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-09-28 20:18 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-10 20:12 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-10 20:32 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-11 15:11 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-24 13:51 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
@ 2023-10-24 15:16 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-10-24 15:31 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
1 sibling, 1 reply; 25+ messages in thread
From: Tom Lane @ 2023-10-24 15:16 UTC (permalink / raw)
To: Christoph Berg <myon@debian.org>; +Cc: Tommy Pavlicek <tommypav122@gmail.com>; Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>; jian.universality@gmail.com
Christoph Berg <myon@debian.org> writes:
> This change is breaking pgsphere which has <@ @> operator pairs, but
> for historical reasons also includes alternative spellings of these
> operators (both called @ with swapped operand types) which now
> explodes because we can't add them with the "proper" commutator and
> negators declared (which point to the canonical <@ @> !<@ !@>
> operators).
Should have guessed that somebody might be depending on the previous
squishy behavior. Still, I can't see how the above situation is a
good idea. Commutators/negators should come in pairs, not have
completely random links. I think it's only accidental that this
setup isn't triggering other strange behavior.
> We might be able to simply delete the @ operators, but doesn't this
> new check break the general possibility to have more than one spelling
> for the same operator?
You can have more than one operator atop the same function.
But why didn't you make the @ operators commutators of each other,
rather than this mess?
regards, tom lane
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-07-02 14:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-09-28 20:18 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-10 20:12 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-10 20:32 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-11 15:11 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-24 13:51 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
2023-10-24 15:16 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-10-24 15:31 ` Christoph Berg <myon@debian.org>
2023-10-24 15:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 25+ messages in thread
From: Christoph Berg @ 2023-10-24 15:31 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tommy Pavlicek <tommypav122@gmail.com>; Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>; jian.universality@gmail.com
Re: Tom Lane
> > We might be able to simply delete the @ operators, but doesn't this
> > new check break the general possibility to have more than one spelling
> > for the same operator?
>
> You can have more than one operator atop the same function.
> But why didn't you make the @ operators commutators of each other,
> rather than this mess?
Historical something.
You are right that the commutators could be fixed that way, but the
negators are a different question. There is no legacy spelling for
these.
Anyway, if this doesn't raise any "oh we didn't think of this"
concerns, we'll just remove the old operators in pgsphere.
Christoph
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-07-02 14:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-09-28 20:18 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-10 20:12 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-10 20:32 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-11 15:11 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-24 13:51 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
2023-10-24 15:16 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-24 15:31 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
@ 2023-10-24 15:42 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-10-31 16:15 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
2024-09-24 22:07 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Michael Banck <mbanck@gmx.net>
0 siblings, 2 replies; 25+ messages in thread
From: Tom Lane @ 2023-10-24 15:42 UTC (permalink / raw)
To: Christoph Berg <myon@debian.org>; +Cc: Tommy Pavlicek <tommypav122@gmail.com>; Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>; jian.universality@gmail.com
Christoph Berg <myon@debian.org> writes:
> Anyway, if this doesn't raise any "oh we didn't think of this"
> concerns, we'll just remove the old operators in pgsphere.
Well, the idea was exactly to forbid that sort of setup.
However, if we get sufficient pushback maybe we should
reconsider --- for example, maybe it'd be sane to enforce
the restriction in ALTER but not CREATE?
I'm inclined to wait and see if there are more complaints.
regards, tom lane
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-07-02 14:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-09-28 20:18 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-10 20:12 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-10 20:32 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-11 15:11 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-24 13:51 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
2023-10-24 15:16 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-24 15:31 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
2023-10-24 15:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-10-31 16:15 ` Christoph Berg <myon@debian.org>
2024-09-12 15:46 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
1 sibling, 1 reply; 25+ messages in thread
From: Christoph Berg @ 2023-10-31 16:15 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tommy Pavlicek <tommypav122@gmail.com>; Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>; jian.universality@gmail.com
Re: Tom Lane
> Well, the idea was exactly to forbid that sort of setup.
Fwiw, pgsphere has remove the problematic operators now:
https://github.com/postgrespro/pgsphere/commit/e810f5ddd827881b06a92a303c5c9fbf997b892e
> However, if we get sufficient pushback maybe we should
> reconsider --- for example, maybe it'd be sane to enforce
> the restriction in ALTER but not CREATE?
Hmm, that seems backwards, I'd expect that CREATE might have some
checks that could circumvent using ALTER if I really insisted. If
CREATE can create things that I can't reach by ALTERing existing other
things, that's weird.
Let's keep it like it is now in PG17.
Christoph
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-07-02 14:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-09-28 20:18 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-10 20:12 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-10 20:32 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-11 15:11 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-24 13:51 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
2023-10-24 15:16 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-24 15:31 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
2023-10-24 15:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-31 16:15 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
@ 2024-09-12 15:46 ` Christoph Berg <myon@debian.org>
0 siblings, 0 replies; 25+ messages in thread
From: Christoph Berg @ 2024-09-12 15:46 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tommy Pavlicek <tommypav122@gmail.com>; Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>; jian.universality@gmail.com
Re: To Tom Lane
> Let's keep it like it is now in PG17.
Late followup news: This feature has actually found a bug in
postgresql-debversion:
CREATE OPERATOR > (
LEFTARG = debversion,
RIGHTARG = debversion,
COMMUTATOR = <,
- NEGATOR = >=,
+ NEGATOR = <=,
RESTRICT = scalargtsel,
JOIN = scalargtjoinsel
);
https://salsa.debian.org/postgresql/postgresql-debversion/-/commit/8ef08ccbea1438468249b0e94048b1a8a...
So, thanks!
Christoph
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-07-02 14:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-09-28 20:18 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-10 20:12 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-10 20:32 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-11 15:11 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-24 13:51 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
2023-10-24 15:16 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-24 15:31 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
2023-10-24 15:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-09-24 22:07 ` Michael Banck <mbanck@gmx.net>
2024-09-26 14:51 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
1 sibling, 1 reply; 25+ messages in thread
From: Michael Banck @ 2024-09-24 22:07 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Christoph Berg <myon@debian.org>; Tommy Pavlicek <tommypav122@gmail.com>; Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>; jian.universality@gmail.com
Hi,
On Tue, Oct 24, 2023 at 11:42:15AM -0400, Tom Lane wrote:
> Christoph Berg <myon@debian.org> writes:
> > Anyway, if this doesn't raise any "oh we didn't think of this"
> > concerns, we'll just remove the old operators in pgsphere.
>
> Well, the idea was exactly to forbid that sort of setup.
> However, if we get sufficient pushback maybe we should
> reconsider --- for example, maybe it'd be sane to enforce
> the restriction in ALTER but not CREATE?
>
> I'm inclined to wait and see if there are more complaints.
FWIW, rdkit also fails, but that seems to be an ancient thing as well:
https://github.com/rdkit/rdkit/issues/7843
I guess there's no way to make that error a bit more helpful, like
printing out the offenbding SQL command, presumably because we are
loding an extension?
Michael
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-07-02 14:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-09-28 20:18 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-10 20:12 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-10 20:32 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-11 15:11 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-24 13:51 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
2023-10-24 15:16 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-24 15:31 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
2023-10-24 15:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2024-09-24 22:07 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Michael Banck <mbanck@gmx.net>
@ 2024-09-26 14:51 ` Christoph Berg <myon@debian.org>
2024-09-26 15:51 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 25+ messages in thread
From: Christoph Berg @ 2024-09-26 14:51 UTC (permalink / raw)
To: Michael Banck <mbanck@gmx.net>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Tommy Pavlicek <tommypav122@gmail.com>; Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>; jian.universality@gmail.com
Re: Michael Banck
> I guess there's no way to make that error a bit more helpful, like
> printing out the offenbding SQL command, presumably because we are
> loding an extension?
I wish there was. The error reporting from failing extension scripts
is really bad with no context at all, it has hit me a few times in the
past already.
Christoph
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-07-02 14:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-09-28 20:18 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-10 20:12 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-10 20:32 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-11 15:11 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-24 13:51 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
2023-10-24 15:16 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-24 15:31 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
2023-10-24 15:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2024-09-24 22:07 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Michael Banck <mbanck@gmx.net>
2024-09-26 14:51 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
@ 2024-09-26 15:51 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 25+ messages in thread
From: Tom Lane @ 2024-09-26 15:51 UTC (permalink / raw)
To: Christoph Berg <myon@debian.org>; +Cc: Michael Banck <mbanck@gmx.net>; Tommy Pavlicek <tommypav122@gmail.com>; Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>; jian.universality@gmail.com
Christoph Berg <myon@debian.org> writes:
> I wish there was. The error reporting from failing extension scripts
> is really bad with no context at all, it has hit me a few times in the
> past already.
Nobody's spent any work on that :-(. A really basic reporting
facility is not hard to add, as in the attached finger exercise.
The trouble with it can be explained by showing what I get after
intentionally breaking a script file command:
regression=# create extension cube;
ERROR: syntax error at or near "CREAT"
LINE 16: CREAT FUNCTION cube_send(cube)
^
QUERY: /* contrib/cube/cube--1.4--1.5.sql */
-- complain if script is sourced in psql, rather than via ALTER EXTENSION
-- Remove @ and ~
DROP OPERATOR @ (cube, cube);
DROP OPERATOR ~ (cube, cube);
-- Add binary input/output handlers
CREATE FUNCTION cube_recv(internal)
RETURNS cube
AS '$libdir/cube'
LANGUAGE C IMMUTABLE STRICT PARALLEL SAFE;
CREAT FUNCTION cube_send(cube)
RETURNS bytea
AS '$libdir/cube'
LANGUAGE C IMMUTABLE STRICT PARALLEL SAFE;
ALTER TYPE cube SET ( RECEIVE = cube_recv, SEND = cube_send );
CONTEXT: extension script file "/home/postgres/install/share/extension/cube--1.4--1.5.sql"
So the first part of that is great, but if your script file is
large you probably won't be happy about having the whole thing
repeated in the "QUERY" field. So this needs some work on
user-friendliness.
I'm inclined to think that maybe we'd be best off keeping the server
end of it straightforward, and trying to teach psql to abbreviate the
QUERY field in a useful way. IIRC you get this same type of problem
with very large SQL-language functions and suchlike.
Also, I believe this doesn't help much for non-syntax errors
(those that aren't reported with an error location). It might
be interesting to use the RawStmt.stmt_location/stmt_len fields
for the current parsetree to identify what to report, but
I've not dug further than this.
regards, tom lane
Attachments:
extension-error-reporting-poc.patch (text/x-diff, 2.8K) download
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index fab59ad5f6..6af72ff422 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -107,6 +107,13 @@ typedef struct ExtensionVersionInfo
struct ExtensionVersionInfo *previous; /* current best predecessor */
} ExtensionVersionInfo;
+/* Info structure for script_error_callback() */
+typedef struct
+{
+ const char *sql;
+ const char *filename;
+} script_error_callback_arg;
+
/* Local functions */
static List *find_update_path(List *evi_list,
ExtensionVersionInfo *evi_start,
@@ -670,9 +677,32 @@ read_extension_script_file(const ExtensionControlFile *control,
return dest_str;
}
+/*
+ * error context callback for failures in script-file execution
+ */
+static void
+script_error_callback(void *arg)
+{
+ script_error_callback_arg *callback_arg = (script_error_callback_arg *) arg;
+ int syntaxerrposition;
+
+ /* If it's a syntax error, convert to internal syntax error report */
+ syntaxerrposition = geterrposition();
+ if (syntaxerrposition > 0)
+ {
+ errposition(0);
+ internalerrposition(syntaxerrposition);
+ internalerrquery(callback_arg->sql);
+ }
+
+ errcontext("extension script file \"%s\"", callback_arg->filename);
+}
+
/*
* Execute given SQL string.
*
+ * The filename the string came from is also provided, for error reporting.
+ *
* Note: it's tempting to just use SPI to execute the string, but that does
* not work very well. The really serious problem is that SPI will parse,
* analyze, and plan the whole string before executing any of it; of course
@@ -682,12 +712,25 @@ read_extension_script_file(const ExtensionControlFile *control,
* could be very long.
*/
static void
-execute_sql_string(const char *sql)
+execute_sql_string(const char *sql, const char *filename)
{
+ script_error_callback_arg callback_arg;
+ ErrorContextCallback scripterrcontext;
List *raw_parsetree_list;
DestReceiver *dest;
ListCell *lc1;
+ /*
+ * Setup error traceback support for ereport().
+ */
+ callback_arg.sql = sql;
+ callback_arg.filename = filename;
+
+ scripterrcontext.callback = script_error_callback;
+ scripterrcontext.arg = (void *) &callback_arg;
+ scripterrcontext.previous = error_context_stack;
+ error_context_stack = &scripterrcontext;
+
/*
* Parse the SQL string into a list of raw parse trees.
*/
@@ -780,6 +823,8 @@ execute_sql_string(const char *sql)
/* Be sure to advance the command counter after the last script command */
CommandCounterIncrement();
+
+ error_context_stack = scripterrcontext.previous;
}
/*
@@ -1054,7 +1099,7 @@ execute_extension_script(Oid extensionOid, ExtensionControlFile *control,
/* And now back to C string */
c_sql = text_to_cstring(DatumGetTextPP(t_sql));
- execute_sql_string(c_sql);
+ execute_sql_string(c_sql, filename);
}
PG_FINALLY();
{
* Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-07-02 14:42 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-09-28 20:18 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-10 20:12 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-10 20:32 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tom Lane <tgl@sss.pgh.pa.us>
2023-10-11 15:11 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-10-24 13:51 ` Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Christoph Berg <myon@debian.org>
@ 2023-10-31 16:10 ` Tommy Pavlicek <tommypav122@gmail.com>
1 sibling, 0 replies; 25+ messages in thread
From: Tommy Pavlicek @ 2023-10-31 16:10 UTC (permalink / raw)
To: Christoph Berg <myon@debian.org>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>; pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>; jian.universality@gmail.com
On Fri, Oct 20, 2023 at 5:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Pushed after a round of editorialization -- mostly cosmetic
> stuff, except for tweaking some error messages. I shortened the
> test cases a bit too, as I thought they were somewhat excessive
> to have as a permanent thing.
Thanks Tom.
On Tue, Oct 24, 2023 at 2:51 PM Christoph Berg <myon@debian.org> wrote:
>
> Re: Tommy Pavlicek
> > I've added another patch (0002-require_unused_neg_com-v1.patch) that
> > prevents using a commutator or negator that's already part of a pair.
>
> Hmm. I agree with the general idea of adding sanity checks, but this
> might be overzealous:
I can't add much beyond what Tom said, but I think this does go a bit
beyond a sanity check. I forgot to mention it in my previous message,
but the main reason I noticed this was because the DELETE operator
code cleans up commutator and negator links to the operator being
deleted and that code expects each to be part of exactly a pair.
^ permalink raw reply [nested|flat] 25+ messages in thread
end of thread, other threads:[~2024-09-26 15:51 UTC | newest]
Thread overview: 25+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-06-22 16:35 [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges Tommy Pavlicek <tommypav122@gmail.com>
2023-06-22 16:47 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-06-22 16:54 ` Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
2023-06-22 17:36 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-06-23 10:34 ` Tommy Pavlicek <tommypav122@gmail.com>
2023-06-23 11:21 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-07-02 14:42 ` Tommy Pavlicek <tommypav122@gmail.com>
2023-09-25 10:52 ` jian he <jian.universality@gmail.com>
2023-09-25 14:55 ` jian he <jian.universality@gmail.com>
2023-09-28 20:18 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-10-10 20:12 ` Tommy Pavlicek <tommypav122@gmail.com>
2023-10-10 20:32 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-10-11 15:11 ` Tommy Pavlicek <tommypav122@gmail.com>
2023-10-12 03:53 ` jian he <jian.universality@gmail.com>
2023-10-20 16:33 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-10-24 13:51 ` Christoph Berg <myon@debian.org>
2023-10-24 15:16 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-10-24 15:31 ` Christoph Berg <myon@debian.org>
2023-10-24 15:42 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-10-31 16:15 ` Christoph Berg <myon@debian.org>
2024-09-12 15:46 ` Christoph Berg <myon@debian.org>
2024-09-24 22:07 ` Michael Banck <mbanck@gmx.net>
2024-09-26 14:51 ` Christoph Berg <myon@debian.org>
2024-09-26 15:51 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-10-31 16:10 ` Tommy Pavlicek <tommypav122@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox