Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
On 07/14/2015 07:28 PM, Jeff Janes wrote: I'm getting some compiler warnings now: operatorcmds.c: In function 'AlterOperator': operatorcmds.c:504: warning: 'address.objectSubId' may be used uninitialized in this function operatorcmds.c:365: note: 'address.objectSubId' was declared here operatorcmds.c:504: warning: 'address.objectId' may be used uninitialized in this function operatorcmds.c:365: note: 'address.objectId' was declared here operatorcmds.c:504: warning: 'address.classId' may be used uninitialized in this function operatorcmds.c:365: note: 'address.classId' was declared here gcc version 4.4.7 20120313 (Red Hat 4.4.7-11) (GCC) Ah, thanks, fixed. I was apparently compiling with -O0. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
On Tue, Jul 14, 2015 at 8:22 AM, Heikki Linnakangas wrote: > On 07/13/2015 03:43 PM, Uriy Zhuravlev wrote: > >> Hello hackers. >> >> Attached is a new version of patch: >> * port syntax from NULL to truth NONE >> * fix documentation (thanks Heikki) >> * rebase to master >> > > Thanks, committed after some editing. I put all the logic into > AlterOperator function, in operatorcmds.c, rather than pg_operator.c. I > think that's how we've lately organized commands. I also simplified the > code quite a bit - I think you had copy-pasted from one of the generic > ALTER variants, like AlterOwnerStmt, which was overly generic for ALTER > OPERATOR. No need to look up the owner/name attributes dynamically, etc. > > There was one genuine bug that I fixed: the ALTER OPERATOR command didn't > check all the same conditions that CREATE OPERATOR did, namely that only > binary operators can have join selectivity, and that only boolean operators > can have restriction/join selectivity. I'm getting some compiler warnings now: operatorcmds.c: In function 'AlterOperator': operatorcmds.c:504: warning: 'address.objectSubId' may be used uninitialized in this function operatorcmds.c:365: note: 'address.objectSubId' was declared here operatorcmds.c:504: warning: 'address.objectId' may be used uninitialized in this function operatorcmds.c:365: note: 'address.objectId' was declared here operatorcmds.c:504: warning: 'address.classId' may be used uninitialized in this function operatorcmds.c:365: note: 'address.classId' was declared here gcc version 4.4.7 20120313 (Red Hat 4.4.7-11) (GCC) Thanks, Jeff
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
On Tuesday 14 July 2015 18:22:26 you wrote: > I think you had copy-pasted from one of the generic > ALTER variants, like AlterOwnerStmt, which was overly generic for ALTER > OPERATOR. You right. > Thanks, committed after some editing. Thanks you. And it BIG editing. ;) After that, can you view my next patch?^_^ https://commitfest.postgresql.org/5/253/ -- Uriy Zhuravlev Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
On 07/13/2015 03:43 PM, Uriy Zhuravlev wrote: Hello hackers. Attached is a new version of patch: * port syntax from NULL to truth NONE * fix documentation (thanks Heikki) * rebase to master Thanks, committed after some editing. I put all the logic into AlterOperator function, in operatorcmds.c, rather than pg_operator.c. I think that's how we've lately organized commands. I also simplified the code quite a bit - I think you had copy-pasted from one of the generic ALTER variants, like AlterOwnerStmt, which was overly generic for ALTER OPERATOR. No need to look up the owner/name attributes dynamically, etc. There was one genuine bug that I fixed: the ALTER OPERATOR command didn't check all the same conditions that CREATE OPERATOR did, namely that only binary operators can have join selectivity, and that only boolean operators can have restriction/join selectivity. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
Hello hackers.
Attached is a new version of patch:
* port syntax from NULL to truth NONE
* fix documentation (thanks Heikki)
* rebase to master
Thanks.
--
Uriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companydiff --git a/doc/src/sgml/ref/alter_operator.sgml b/doc/src/sgml/ref/alter_operator.sgml
index bdb2d02..237e4f1 100644
--- a/doc/src/sgml/ref/alter_operator.sgml
+++ b/doc/src/sgml/ref/alter_operator.sgml
@@ -26,6 +26,11 @@ ALTER OPERATOR name ( { left_typename ( { left_type | NONE } , { right_type | NONE } )
SET SCHEMA new_schema
+
+ALTER OPERATOR name ( { left_type | NONE } , { right_type | NONE } )
+SET ( { RESTRICT = { res_proc | NONE }
+ | JOIN = { join_proc | NONE }
+ } [, ... ] )
@@ -34,8 +39,7 @@ ALTER OPERATOR name ( { left_type
ALTER OPERATOR changes the definition of
- an operator. The only currently available functionality is to change the
- owner of the operator.
+ an operator.
@@ -98,6 +102,25 @@ ALTER OPERATOR name ( { left_type
+
+
+ res_proc
+
+
+ The restriction selectivity estimator function for this operator; write NONE to remove existing selectivity estimator.
+
+
+
+
+
+ join_proc
+
+
+ The join selectivity estimator function for this operator; write NONE to remove existing selectivity estimator.
+
+
+
+
@@ -109,6 +132,13 @@ ALTER OPERATOR name ( { left_type
ALTER OPERATOR @@ (text, text) OWNER TO joe;
+
+
+Change the restriction and join selectivity estimator function of a custom operator a && b for type int[]:
+
+ALTER OPERATOR && (_int4, _int4) SET (RESTRICT = _int_contsel, JOIN = _int_contjoinsel);
+
+
diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index 072f530..4c5c9c6 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -28,8 +28,10 @@
#include "catalog/pg_operator.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
+#include "commands/defrem.h"
#include "miscadmin.h"
#include "parser/parse_oper.h"
+#include "parser/parse_func.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
@@ -53,7 +55,7 @@ static Oid OperatorShellMake(const char *operatorName,
Oid leftTypeId,
Oid rightTypeId);
-static void OperatorUpd(Oid baseId, Oid commId, Oid negId);
+static void ShellOperatorUpd(Oid baseId, Oid commId, Oid negId);
static Oid get_other_operator(List *otherOp,
Oid otherLeftTypeId, Oid otherRightTypeId,
@@ -563,7 +565,7 @@ OperatorCreate(const char *operatorName,
commutatorId = operatorObjectId;
if (OidIsValid(commutatorId) || OidIsValid(negatorId))
- OperatorUpd(operatorObjectId, commutatorId, negatorId);
+ ShellOperatorUpd(operatorObjectId, commutatorId, negatorId);
return address;
}
@@ -633,7 +635,7 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
}
/*
- * OperatorUpd
+ * ShellOperatorUpd
*
* For a given operator, look up its negator and commutator operators.
* If they are defined, but their negator and commutator fields
@@ -642,7 +644,7 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
* which are the negator or commutator of each other.
*/
static void
-OperatorUpd(Oid baseId, Oid commId, Oid negId)
+ShellOperatorUpd(Oid baseId, Oid commId, Oid negId)
{
int i;
Relation pg_operator_desc;
@@ -864,3 +866,154 @@ makeOperatorDependencies(HeapTuple tuple)
return myself;
}
+
+/*
+ * Operator update aka ALTER OPERATOR for RESTRICT, JOIN
+ */
+void OperatorUpd(Oid classId,
+ Oid baseId,
+ List *operator_params)
+{
+ int i;
+ ListCell *pl;
+ Relation catalog;
+ HeapTuple tup;
+ Oid operator_param_id = 0;
+ bool nulls[Natts_pg_operator];
+ bool replaces[Natts_pg_operator];
+ Datum values[Natts_pg_operator];
+
+ for (i = 0; i < Natts_pg_operator; ++i)
+ {
+ values[i] = (Datum) 0;
+ replaces[i] = false;
+ nulls[i] = false;
+ }
+
+ catalog = heap_open(classId, RowExclusiveLock);
+ tup = SearchSysCacheCopy1(OPEROID, ObjectIdGetDatum(baseId));
+
+ if (HeapTupleIsValid(tup))
+ {
+ /*
+ * loop over the definition list and extract the information we need.
+ */
+ foreach(pl, operator_params)
+ {
+ DefElem*defel = (DefElem *) lfirst(pl);
+ List *param;
+ int param_type;
+
+ if (defel->arg == NULL)
+param = NULL;
+ else
+param = defGetQualifiedName(defel);
+
+ if (pg_strcasecmp(defel->defname, "restrict") == 0)
+param_type = Anum_pg_operator_oprrest;
+ else if (pg_strcasecmp(defel->defname, "join") == 0)
+param_type = Anum_pg_operator_oprjoin;
+ else
+ {
+ereport(WARNING,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("operator attribute \"%s\" not recognized",
+defel->defname)));
+continue;
+ }
+
+ /* Resets if written NONE */
+
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
Hello. On Friday 10 July 2015 15:46:35 you wrote: > * I think it'd be better to use NONE instead of NULL above, to remove > the estimator. It seems inconsistent when we've used NONE to mean > "missing" earlier in the same statement. Ok, you right. > * The way you check for the NULL in OperatorUpd is wrong. It cannot > distinguish between a quoted "null", meaning a function called "null", > and a NULL, meaning no function. With "none" has a similar problem. I need to correct the grammar. > > Attached is a new version of your patch, rebased over current master, > and the docs formatting fixes. Thanks. I hope to send the new patch on Monday. -- Uriy Zhuravlev Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
On 07/06/2015 07:21 PM, Uriy Zhuravlev wrote:
Hello hackers.
This is the fifth version of the patch (the fourth was unsuccessful :)).
I added documentation and was held a small refactoring.
I edited the formatting of the syntax page a bit, and came up with this:
ALTER OPERATOR name ( { left_type | NONE } , { right_type | NONE } )
SET ( { RESTRICT = { res_proc | NULL }
| JOIN = { join_proc | NULL }
} [, ... ] )
A couple of minor issues with the syntax itself:
* I think it'd be better to use NONE instead of NULL above, to remove
the estimator. It seems inconsistent when we've used NONE to mean
"missing" earlier in the same statement.
* The way you check for the NULL in OperatorUpd is wrong. It cannot
distinguish between a quoted "null", meaning a function called "null",
and a NULL, meaning no function. (You can argue that you're just asking
for trouble if you name any function "null", but we're careful to deal
with that correctly everywhere else.) You don't have the information
about quoting once you leave the parser, so you'll have to distinguish
those in the grammar.
Attached is a new version of your patch, rebased over current master,
and the docs formatting fixes.
- Heikki
diff --git a/doc/src/sgml/ref/alter_operator.sgml b/doc/src/sgml/ref/alter_operator.sgml
index 8a7af50..b8c353f 100644
--- a/doc/src/sgml/ref/alter_operator.sgml
+++ b/doc/src/sgml/ref/alter_operator.sgml
@@ -26,6 +26,11 @@ ALTER OPERATOR name ( { left_typename ( { left_type | NONE } , { right_type | NONE } )
SET SCHEMA new_schema
+
+ALTER OPERATOR name ( { left_type | NONE } , { right_type | NONE } )
+SET ( { RESTRICT = { res_proc | NONE }
+ | JOIN = { join_proc | NONE }
+ } [, ... ] )
@@ -34,8 +39,7 @@ ALTER OPERATOR name ( { left_type
ALTER OPERATOR changes the definition of
- an operator. The only currently available functionality is to change the
- owner of the operator.
+ an operator.
@@ -98,6 +102,25 @@ ALTER OPERATOR name ( { left_type
+
+
+ res_proc
+
+
+ The restriction selectivity estimator function for this operator; write NONE to remove existing selectivity estimator.
+
+
+
+
+
+ join_proc
+
+
+ The join selectivity estimator function for this operator; write NONE to remove existing selectivity estimator.
+
+
+
+
@@ -109,6 +132,13 @@ ALTER OPERATOR name ( { left_type
ALTER OPERATOR @@ (text, text) OWNER TO joe;
+
+
+Change the restriction and join selectivity estimator function of a custom operator a && b for type int[]:
+
+ALTER OPERATOR && (_int4, _int4) SET (RESTRICT = _int_contsel, JOIN = _int_contjoinsel);
+
+
diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index 072f530..b981a44 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -28,8 +28,10 @@
#include "catalog/pg_operator.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
+#include "commands/defrem.h"
#include "miscadmin.h"
#include "parser/parse_oper.h"
+#include "parser/parse_func.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
@@ -53,7 +55,7 @@ static Oid OperatorShellMake(const char *operatorName,
Oid leftTypeId,
Oid rightTypeId);
-static void OperatorUpd(Oid baseId, Oid commId, Oid negId);
+static void ShellOperatorUpd(Oid baseId, Oid commId, Oid negId);
static Oid get_other_operator(List *otherOp,
Oid otherLeftTypeId, Oid otherRightTypeId,
@@ -563,7 +565,7 @@ OperatorCreate(const char *operatorName,
commutatorId = operatorObjectId;
if (OidIsValid(commutatorId) || OidIsValid(negatorId))
- OperatorUpd(operatorObjectId, commutatorId, negatorId);
+ ShellOperatorUpd(operatorObjectId, commutatorId, negatorId);
return address;
}
@@ -633,7 +635,7 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
}
/*
- * OperatorUpd
+ * ShellOperatorUpd
*
* For a given operator, look up its negator and commutator operators.
* If they are defined, but their negator and commutator fields
@@ -642,7 +644,7 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
* which are the negator or commutator of each other.
*/
static void
-OperatorUpd(Oid baseId, Oid commId, Oid negId)
+ShellOperatorUpd(Oid baseId, Oid commId, Oid negId)
{
int i;
Relation pg_operator_desc;
@@ -864,3 +866,149 @@ makeOperatorDependencies(HeapTuple tuple)
return myself;
}
+
+/*
+ * Operator update aka ALTER OPERATOR for RESTRICT, JOIN
+ */
+void
+OperatorUpd(Oid classId, Oid baseId, List *operator_params)
+{
+ int i;
+ ListCell *pl;
+ Relation catalog;
+ HeapTuple tup;
+ Oid operator_param_id = 0;
+ bool nulls[Natts_pg_operator];
+ bool replaces[Natts_pg_operator];
+ Datum values[Natts_pg_operator];
+
+ for (i = 0; i < Natts_pg_operator; ++i)
+
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
Hello hackers.
This is the fifth version of the patch (the fourth was unsuccessful :)).
I added documentation and was held a small refactoring.
Thanks.
--
Uriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companydiff --git a/doc/src/sgml/ref/alter_operator.sgml b/doc/src/sgml/ref/alter_operator.sgml
index bdb2d02..4aaeed0 100644
--- a/doc/src/sgml/ref/alter_operator.sgml
+++ b/doc/src/sgml/ref/alter_operator.sgml
@@ -26,6 +26,11 @@ ALTER OPERATOR name ( { left_typename ( { left_type | NONE } , { right_type | NONE } )
SET SCHEMA new_schema
+
+ALTER OPERATOR name ( { left_type | NONE } , { right_type | NONE } ) SET (
+[, RESTRICT = { res_proc | NULL } ]
+[, JOIN = { join_proc | NULL } ]
+)
@@ -34,8 +39,7 @@ ALTER OPERATOR name ( { left_type
ALTER OPERATOR changes the definition of
- an operator. The only currently available functionality is to change the
- owner of the operator.
+ an operator.
@@ -98,6 +102,25 @@ ALTER OPERATOR name ( { left_type
+
+
+ res_proc
+
+
+ The restriction selectivity estimator function for this operator.
+
+
+
+
+
+ join_proc
+
+
+ The join selectivity estimator function for this operator.
+
+
+
+
@@ -109,6 +132,13 @@ ALTER OPERATOR name ( { left_type
ALTER OPERATOR @@ (text, text) OWNER TO joe;
+
+
+Change the restriction and join selectivity estimator function of a custom operator a && b for type int[]:
+
+ALTER OPERATOR && (_int4, _int4) SET (RESTRICT = _int_contsel, JOIN = _int_contjoinsel);
+
+
diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index 072f530..f5ff381 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -28,8 +28,10 @@
#include "catalog/pg_operator.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
+#include "commands/defrem.h"
#include "miscadmin.h"
#include "parser/parse_oper.h"
+#include "parser/parse_func.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
@@ -53,7 +55,7 @@ static Oid OperatorShellMake(const char *operatorName,
Oid leftTypeId,
Oid rightTypeId);
-static void OperatorUpd(Oid baseId, Oid commId, Oid negId);
+static void ShellOperatorUpd(Oid baseId, Oid commId, Oid negId);
static Oid get_other_operator(List *otherOp,
Oid otherLeftTypeId, Oid otherRightTypeId,
@@ -563,7 +565,7 @@ OperatorCreate(const char *operatorName,
commutatorId = operatorObjectId;
if (OidIsValid(commutatorId) || OidIsValid(negatorId))
- OperatorUpd(operatorObjectId, commutatorId, negatorId);
+ ShellOperatorUpd(operatorObjectId, commutatorId, negatorId);
return address;
}
@@ -633,7 +635,7 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
}
/*
- * OperatorUpd
+ * ShellOperatorUpd
*
* For a given operator, look up its negator and commutator operators.
* If they are defined, but their negator and commutator fields
@@ -642,7 +644,7 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
* which are the negator or commutator of each other.
*/
static void
-OperatorUpd(Oid baseId, Oid commId, Oid negId)
+ShellOperatorUpd(Oid baseId, Oid commId, Oid negId)
{
int i;
Relation pg_operator_desc;
@@ -864,3 +866,150 @@ makeOperatorDependencies(HeapTuple tuple)
return myself;
}
+
+/*
+ * Operator update aka ALTER OPERATOR for RESTRICT, JOIN
+ */
+void OperatorUpd(Oid classId,
+ Oid baseId,
+ List *operator_params)
+{
+ int i;
+ ListCell *pl;
+ Relation catalog;
+ HeapTuple tup;
+ Oid operator_param_id = 0;
+ bool nulls[Natts_pg_operator];
+ bool replaces[Natts_pg_operator];
+ Datum values[Natts_pg_operator];
+
+ for (i = 0; i < Natts_pg_operator; ++i)
+ {
+ values[i] = (Datum) 0;
+ replaces[i] = false;
+ nulls[i] = false;
+ }
+
+ catalog = heap_open(classId, RowExclusiveLock);
+ tup = SearchSysCacheCopy1(OPEROID, ObjectIdGetDatum(baseId));
+ if (HeapTupleIsValid(tup))
+ {
+ /*
+ * loop over the definition list and extract the information we need.
+ */
+ foreach(pl, operator_params)
+ {
+ DefElem*defel = (DefElem *) lfirst(pl);
+ List *param = defGetQualifiedName(defel);
+ int param_type;
+
+ if (pg_strcasecmp(defel->defname, "restrict") == 0)
+param_type = Anum_pg_operator_oprrest;
+ else if (pg_strcasecmp(defel->defname, "join") == 0)
+param_type = Anum_pg_operator_oprjoin;
+ else
+ {
+ereport(WARNING,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("operator attribute \"%s\" not recognized",
+defel->defname)));
+continue;
+ }
+
+ /* Resets if written NULL */
+ if (PointerIsValid(param) && pg_strcasecmp(NameListToString(param), "null") == 0)
+ {
+values[param_type - 1] = ObjectIdGetDatum(InvalidOid);
+replaces[param_type - 1] = true;
+continue;
+
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
On Wed, Jun 24, 2015 at 7:30 AM, Uriy Zhuravlev wrote: > Because change the commutator and negator raised questions I suggest 3 version > of the patch without them. In addition, for us now is much more important > restrict and join (for "Selectivity estimation for intarray" patch). Seems broadly sensible. You will need to write docs. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
Hello Hackers.
Because change the commutator and negator raised questions I suggest 3 version
of the patch without them. In addition, for us now is much more important
restrict and join (for "Selectivity estimation for intarray" patch).
What do you think?
Thanks!
--
Uriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companydiff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index 072f530..f7770fd 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -28,8 +28,10 @@
#include "catalog/pg_operator.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
+#include "commands/defrem.h"
#include "miscadmin.h"
#include "parser/parse_oper.h"
+#include "parser/parse_func.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
@@ -53,7 +55,7 @@ static Oid OperatorShellMake(const char *operatorName,
Oid leftTypeId,
Oid rightTypeId);
-static void OperatorUpd(Oid baseId, Oid commId, Oid negId);
+static void ShellOperatorUpd(Oid baseId, Oid commId, Oid negId);
static Oid get_other_operator(List *otherOp,
Oid otherLeftTypeId, Oid otherRightTypeId,
@@ -563,7 +565,7 @@ OperatorCreate(const char *operatorName,
commutatorId = operatorObjectId;
if (OidIsValid(commutatorId) || OidIsValid(negatorId))
- OperatorUpd(operatorObjectId, commutatorId, negatorId);
+ ShellOperatorUpd(operatorObjectId, commutatorId, negatorId);
return address;
}
@@ -633,7 +635,7 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
}
/*
- * OperatorUpd
+ * ShellOperatorUpd
*
* For a given operator, look up its negator and commutator operators.
* If they are defined, but their negator and commutator fields
@@ -642,7 +644,7 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
* which are the negator or commutator of each other.
*/
static void
-OperatorUpd(Oid baseId, Oid commId, Oid negId)
+ShellOperatorUpd(Oid baseId, Oid commId, Oid negId)
{
int i;
Relation pg_operator_desc;
@@ -864,3 +866,158 @@ makeOperatorDependencies(HeapTuple tuple)
return myself;
}
+
+/*
+ * Operator update aka ALTER OPERATOR for RESTRICT, JOIN
+ */
+void OperatorUpd(Oid classId,
+ Oid baseId,
+ List *operator_params)
+{
+ int i;
+ ListCell *pl;
+ Relation catalog;
+ HeapTuple tup;
+ Oid operator_param_id = 0;
+ bool nulls[Natts_pg_operator];
+ bool replaces[Natts_pg_operator];
+ Datum values[Natts_pg_operator];
+
+ for (i = 0; i < Natts_pg_operator; ++i)
+ {
+ values[i] = (Datum) 0;
+ replaces[i] = false;
+ nulls[i] = false;
+ }
+
+ catalog = heap_open(classId, RowExclusiveLock);
+ tup = SearchSysCacheCopy1(OPEROID, ObjectIdGetDatum(baseId));
+ if (HeapTupleIsValid(tup))
+ {
+ /*
+ * loop over the definition list and extract the information we need.
+ */
+ foreach(pl, operator_params)
+ {
+ DefElem*defel = (DefElem *) lfirst(pl);
+ List *param = defGetQualifiedName(defel);
+ int param_type;
+
+ if (pg_strcasecmp(defel->defname, "restrict") == 0)
+param_type = Anum_pg_operator_oprrest;
+ else if (pg_strcasecmp(defel->defname, "join") == 0)
+param_type = Anum_pg_operator_oprjoin;
+ else
+ {
+ereport(WARNING,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("operator attribute \"%s\" not recognized",
+defel->defname)));
+continue;
+ }
+
+ /* Resets if written NULL */
+ if (pg_strcasecmp(NameListToString(param), "null") == 0)
+ {
+values[param_type - 1] = ObjectIdGetDatum(InvalidOid);
+replaces[param_type - 1] = true;
+continue;
+ }
+
+ if (param_type == Anum_pg_operator_oprrest)
+ {
+if (PointerIsValid(param))
+{
+ Oid typeId[5];
+ AclResult aclresult;
+ typeId[0] = INTERNALOID; /* PlannerInfo */
+ typeId[1] = OIDOID; /* operator OID */
+ typeId[2] = INTERNALOID; /* args list */
+ typeId[3] = INT4OID; /* varRelid */
+
+ operator_param_id = LookupFuncName(param, 4, typeId, false);
+
+ /* estimators must return float8 */
+ if (get_func_rettype(operator_param_id) != FLOAT8OID)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("restriction estimator function %s must return type \"float8\"",
+ NameListToString(param;
+
+ /* Require EXECUTE rights for the estimator */
+ aclresult = pg_proc_aclcheck(operator_param_id, GetUserId(), ACL_EXECUTE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, ACL_KIND_PROC,
+ NameListToString(param));
+}
+else
+ operator_param_id = 0;
+ }
+ else if (param_type == Anum_pg_operator_oprjoin)
+ {
+if (PointerIsValid(param))
+{
+ Oid typeId[5];
+ AclResult aclresult;
+ typeId[0] = INTERNALOID; /* PlannerInfo */
+ typeId[1] = OIDOID; /* operator OID */
+ typeId[2] = INTERNALOID; /* args list */
+ typeId[3]
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
On Fri, May 29, 2015 at 4:28 AM, Alexander Korotkov wrote: > On Thu, May 28, 2015 at 6:43 PM, Tom Lane wrote: >> >> Alexander Korotkov writes: >> > Could we address both this problems by denying changing existing >> > commutators and negator? ISTM that setting absent commutator and negator >> > is >> > quite enough for ALTER OPERATOR. User extensions could need setting of >> > commutator and negator because they could add new operators which don't >> > exist before. But it's rather uncommon to unset or change commutator or >> > negator. >> >> Note that this functionality is already covered, in that you can specify >> the commutator/negator linkage when you create the second operator. >> I'm not particularly convinced that we need to have it in ALTER OPERATOR. > > > Yeah, I didn't notice that CREATE OPERATOR sets commutator and negator on > opposite side as well. Then we probably can leave ALTER OPERATOR without > altering commutator and negator. > > BTW, does DROP OPERATOR works correctly? > > # create operator == (procedure = int8eq, leftarg = bigint, rightarg = > bigint); > CREATE OPERATOR > # create operator !== (procedure = int8ne, leftarg = bigint, rightarg = > bigint, negator = ==); > CREATE OPERATOR > # select oid, * from pg_operator where oprnamespace = (select oid from > pg_namespace where nspname = 'public'); > oid | oprname | oprnamespace | oprowner | oprkind | oprcanmerge | > oprcanhash | oprleft | oprright | oprresult | oprcom | oprnegate | oprcode | > oprrest | oprjoin > ---+-+--+--+-+-++-+--+---++---+-+-+- > 46355 | !== | 2200 | 10 | b | f | f > | 20 | 20 |16 | 0 | 46354 | int8ne | - | > - > 46354 | == | 2200 | 10 | b | f | f > | 20 | 20 |16 | 0 | 46355 | int8eq | - | > - > (2 rows) > # create table test (id bigint); > CREATE TABLE > # explain verbose select * from test where not id == 10::bigint; > QUERY PLAN > --- > Seq Scan on public.test (cost=0.00..38.25 rows=1130 width=8) >Output: id >Filter: (test.id !== '10'::bigint) > (3 rows) > # drop operator !== (int8, int8); > DROP OPERATOR > # select oid, * from pg_operator where oprnamespace = (select oid from > pg_namespace where nspname = 'public'); > oid | oprname | oprnamespace | oprowner | oprkind | oprcanmerge | > oprcanhash | oprleft | oprright | oprresult | oprcom | oprnegate | oprcode | > oprrest | oprjoin > ---+-+--+--+-+-++-+--+---++---+-+-+- > 46354 | == | 2200 | 10 | b | f | f > | 20 | 20 |16 | 0 | 46355 | int8eq | - | > - > (1 row) > # explain verbose select * from test where not id == 10::bigint; > ERROR: cache lookup failed for function 0 > > DROP OPERATOR leaves broken reference in oprnegate. Should we fix it? Yeah, that doesn't seem good. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
On Thu, May 28, 2015 at 6:43 PM, Tom Lane wrote: > Alexander Korotkov writes: > > Could we address both this problems by denying changing existing > > commutators and negator? ISTM that setting absent commutator and negator > is > > quite enough for ALTER OPERATOR. User extensions could need setting of > > commutator and negator because they could add new operators which don't > > exist before. But it's rather uncommon to unset or change commutator or > > negator. > > Note that this functionality is already covered, in that you can specify > the commutator/negator linkage when you create the second operator. > I'm not particularly convinced that we need to have it in ALTER OPERATOR. > Yeah, I didn't notice that CREATE OPERATOR sets commutator and negator on opposite side as well. Then we probably can leave ALTER OPERATOR without altering commutator and negator. BTW, does DROP OPERATOR works correctly? # create operator == (procedure = int8eq, leftarg = bigint, rightarg = bigint); CREATE OPERATOR # create operator !== (procedure = int8ne, leftarg = bigint, rightarg = bigint, negator = ==); CREATE OPERATOR # select oid, * from pg_operator where oprnamespace = (select oid from pg_namespace where nspname = 'public'); oid | oprname | oprnamespace | oprowner | oprkind | oprcanmerge | oprcanhash | oprleft | oprright | oprresult | oprcom | oprnegate | oprcode | oprrest | oprjoin ---+-+--+--+-+-++-+--+---++---+-+-+- 46355 | !== | 2200 | 10 | b | f | f | 20 | 20 |16 | 0 | 46354 | int8ne | - | - 46354 | == | 2200 | 10 | b | f | f | 20 | 20 |16 | 0 | 46355 | int8eq | - | - (2 rows) # create table test (id bigint); CREATE TABLE # explain verbose select * from test where not id == 10::bigint; QUERY PLAN --- Seq Scan on public.test (cost=0.00..38.25 rows=1130 width=8) Output: id Filter: (test.id !== '10'::bigint) (3 rows) # drop operator !== (int8, int8); DROP OPERATOR # select oid, * from pg_operator where oprnamespace = (select oid from pg_namespace where nspname = 'public'); oid | oprname | oprnamespace | oprowner | oprkind | oprcanmerge | oprcanhash | oprleft | oprright | oprresult | oprcom | oprnegate | oprcode | oprrest | oprjoin ---+-+--+--+-+-++-+--+---++---+-+-+- 46354 | == | 2200 | 10 | b | f | f | 20 | 20 |16 | 0 | 46355 | int8eq | - | - (1 row) # explain verbose select * from test where not id == 10::bigint; ERROR: cache lookup failed for function 0 DROP OPERATOR leaves broken reference in oprnegate. Should we fix it? -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
Alexander Korotkov writes: > Could we address both this problems by denying changing existing > commutators and negator? ISTM that setting absent commutator and negator is > quite enough for ALTER OPERATOR. User extensions could need setting of > commutator and negator because they could add new operators which don't > exist before. But it's rather uncommon to unset or change commutator or > negator. Note that this functionality is already covered, in that you can specify the commutator/negator linkage when you create the second operator. I'm not particularly convinced that we need to have it in ALTER OPERATOR. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
On Wed, May 27, 2015 at 9:28 PM, Robert Haas wrote: > On Mon, May 18, 2015 at 10:21 AM, Tom Lane wrote: > > Uriy Zhuravlev writes: > >> I have attached a patch that extends ALTER OPERATOR to support > COMMUTATOR, > >> NEGATOR, RESTRICT and JOIN. > > > > There are fairly significant reasons why we have not done this, based > > on the difficulty of updating existing cached plans that might have > > incidentally depended on those operator properties during creation. > > Perhaps it's all right to simply ignore such concerns, but I would like > > to see a defense of why. > > I don't think there's a direct problem with cached plans, because it > looks like plancache.c blows away the entire plan cache for any change > to pg_operator. OperatorUpd() can already update oprcom and > oprnegate, which seems to stand for the proposition that it's OK to > set those from InvalidOid to something else. But that doesn't prove > that other kinds of changes are safe. > > A search of other places where oprcom is used in the code led me to > ComputeIndexAttrs(). If an operator whose commutator is itself were > changed so that the commutator was anything else, then we'd end up > with a broken exclusion constraint. That's a problem. > targetIsInSortList is run during parse analysis, and that too tests > whether sortop == get_commutator(scl->sortop). Those decisions > wouldn't get reevaluated if the truth of that expression changed after > the fact, which I suspect is also a problem. > Could we address both this problems by denying changing existing commutators and negator? ISTM that setting absent commutator and negator is quite enough for ALTER OPERATOR. User extensions could need setting of commutator and negator because they could add new operators which don't exist before. But it's rather uncommon to unset or change commutator or negator. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
On Mon, May 18, 2015 at 10:21 AM, Tom Lane wrote: > Uriy Zhuravlev writes: >> I have attached a patch that extends ALTER OPERATOR to support COMMUTATOR, >> NEGATOR, RESTRICT and JOIN. > > There are fairly significant reasons why we have not done this, based > on the difficulty of updating existing cached plans that might have > incidentally depended on those operator properties during creation. > Perhaps it's all right to simply ignore such concerns, but I would like > to see a defense of why. I don't think there's a direct problem with cached plans, because it looks like plancache.c blows away the entire plan cache for any change to pg_operator. OperatorUpd() can already update oprcom and oprnegate, which seems to stand for the proposition that it's OK to set those from InvalidOid to something else. But that doesn't prove that other kinds of changes are safe. A search of other places where oprcom is used in the code led me to ComputeIndexAttrs(). If an operator whose commutator is itself were changed so that the commutator was anything else, then we'd end up with a broken exclusion constraint. That's a problem. targetIsInSortList is run during parse analysis, and that too tests whether sortop == get_commutator(scl->sortop). Those decisions wouldn't get reevaluated if the truth of that expression changed after the fact, which I suspect is also a problem. On a quick survey, I did not find similar hazards related to oprnegate, oprrest, or oprjoin. AFAICS, they are used only by the planner. I *think* that means that if our plan invalidation code is smart enough, those instances will be OK. But I haven't audited them in detail. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
Hello Hackers.
I reworked the patch. Now, the syntax is similar to CREATE OPERATOR.
And as I wrote earlier problems with the cache I have not found. If someone
can suggest how it could be verified that would be happy.
New syntax example:
ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = =);
ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = =);
ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = example_func);
ALTER OPERATOR === (boolean, boolean) SET (JOIN = example_func);
ALTER OPERATOR === (boolean, boolean) SET (
COMMUTATOR = NULL,
NEGATOR = NULL,
RESTRICT = NULL,
JOIN = NULL
);
Thanks!
--
Uriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companydiff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index 072f530..83e020b 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -28,8 +28,10 @@
#include "catalog/pg_operator.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
+#include "commands/defrem.h"
#include "miscadmin.h"
#include "parser/parse_oper.h"
+#include "parser/parse_func.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
@@ -53,7 +55,7 @@ static Oid OperatorShellMake(const char *operatorName,
Oid leftTypeId,
Oid rightTypeId);
-static void OperatorUpd(Oid baseId, Oid commId, Oid negId);
+static void ShellOperatorUpd(Oid baseId, Oid commId, Oid negId);
static Oid get_other_operator(List *otherOp,
Oid otherLeftTypeId, Oid otherRightTypeId,
@@ -563,7 +565,7 @@ OperatorCreate(const char *operatorName,
commutatorId = operatorObjectId;
if (OidIsValid(commutatorId) || OidIsValid(negatorId))
- OperatorUpd(operatorObjectId, commutatorId, negatorId);
+ ShellOperatorUpd(operatorObjectId, commutatorId, negatorId);
return address;
}
@@ -633,7 +635,7 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
}
/*
- * OperatorUpd
+ * ShellOperatorUpd
*
* For a given operator, look up its negator and commutator operators.
* If they are defined, but their negator and commutator fields
@@ -642,7 +644,7 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
* which are the negator or commutator of each other.
*/
static void
-OperatorUpd(Oid baseId, Oid commId, Oid negId)
+ShellOperatorUpd(Oid baseId, Oid commId, Oid negId)
{
int i;
Relation pg_operator_desc;
@@ -864,3 +866,187 @@ makeOperatorDependencies(HeapTuple tuple)
return myself;
}
+
+/*
+ * Operator update aka ALTER OPERATOR for COMMUTATOR, NEGATOR, RESTRICT, JOIN
+ */
+void OperatorUpd(Oid classId,
+ Oid baseId,
+ List *operator_params)
+{
+ int i;
+ ListCell *pl;
+ Relation catalog;
+ HeapTuple tup;
+ Oid operator_param_id = 0;
+ Form_pg_operator DstOperatorData;
+ bool otherDefined;
+ bool nulls[Natts_pg_operator];
+ bool replaces[Natts_pg_operator];
+ Datum values[Natts_pg_operator];
+
+ for (i = 0; i < Natts_pg_operator; ++i)
+ {
+ values[i] = (Datum) 0;
+ replaces[i] = false;
+ nulls[i] = false;
+ }
+
+ catalog = heap_open(classId, RowExclusiveLock);
+ tup = SearchSysCacheCopy1(OPEROID, ObjectIdGetDatum(baseId));
+ if (HeapTupleIsValid(tup))
+ {
+ DstOperatorData = (Form_pg_operator) GETSTRUCT(tup);
+
+ /*
+ * loop over the definition list and extract the information we need.
+ */
+ foreach(pl, operator_params)
+ {
+ DefElem*defel = (DefElem *) lfirst(pl);
+ List *param = defGetQualifiedName(defel);
+ int param_type;
+
+ if (pg_strcasecmp(defel->defname, "commutator") == 0)
+param_type = Anum_pg_operator_oprcom;
+ else if (pg_strcasecmp(defel->defname, "negator") == 0)
+param_type = Anum_pg_operator_oprnegate;
+ else if (pg_strcasecmp(defel->defname, "restrict") == 0)
+param_type = Anum_pg_operator_oprrest;
+ else if (pg_strcasecmp(defel->defname, "join") == 0)
+param_type = Anum_pg_operator_oprjoin;
+ else
+ {
+ereport(WARNING,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("operator attribute \"%s\" not recognized",
+defel->defname)));
+continue;
+ }
+
+ /* Resets if written NULL */
+ if (pg_strcasecmp(NameListToString(param), "null") == 0)
+ {
+values[param_type - 1] = ObjectIdGetDatum(InvalidOid);
+replaces[param_type - 1] = true;
+continue;
+ }
+
+ /*
+ * Prepare tuple to upgrade the operator
+ * considering the type of the parameter.
+ */
+ if (param_type == Anum_pg_operator_oprcom ||
+param_type == Anum_pg_operator_oprnegate)
+ {
+otherDefined = true;
+if (PointerIsValid(param))
+ operator_param_id = OperatorLookup(param,
+DstOperatorData->oprleft,
+DstOperatorData->oprright,
+&otherDefined);
+else
+ operator_param_id = InvalidOid;
+
+if (!otherDefined && OidIsValid(operator_param_id)) {
+ ereport(ERROR,
+ (errmsg_internal("You can't s
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
On Wednesday 20 May 2015 20:50:41 Andres Freund wrote: > Note that this very likely wasn't actually using a prepared plan. Due to > the custom plan infrastructure the first few invocations are going to be > replanned. Hello. I tested it on 30 and 50 iterations, and it feels good. Thanks. -- Uriy Zhuravlev Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
Hi, On 2015-05-20 12:22:34 +0300, Uriy Zhuravlev wrote: > On Monday 18 May 2015 10:21:10 you wrote: > > difficulty of updating existing cached plans > Could you specify more precisely about some caches we talking about? PREPARE > working correctly: > > CREATE TABLE test_ints(i int4); > CREATE TABLE > CREATE INDEX idx ON test_ints(i); > CREATE INDEX > set enable_bitmapscan=off; > SET > set enable_seqscan=off; > SET > PREPARE test_plan (int) AS > SELECT * FROM test_ints WHERE $1::int4 > i; > PREPARE > EXPLAIN (COSTS OFF) > EXECUTE test_plan(5); >QUERY PLAN > > Index Only Scan using idx on test_ints >Index Cond: (i < 5) > > ALTER OPERATOR > (int4, int4) SET COMMUTATOR NONE; > ALTER OPERATOR > EXPLAIN (COSTS OFF) > EXECUTE test_plan(5); >QUERY PLAN > > Index Only Scan using idx on test_ints >Filter: (5 > i) Note that this very likely wasn't actually using a prepared plan. Due to the custom plan infrastructure the first few invocations are going to be replanned. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
Alvaro Herrera writes: > Uriy Zhuravlev wrote: >> And can you explain more about the syntax? > I think he means to treat COMMUTATOR etc like a generic element list, > i.e. don't define new keywords in kwlist.h/gram.y at all but rather pass > the names as strings (probably using a list of DefElem) and strcmp() > them in OperatorUpd() or something. Yeah. If they aren't keywords in CREATE OPERATOR, I don't think they should be in ALTER OPERATOR either. Indeed, the syntax of the two commands probably ought to be similar. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
Uriy Zhuravlev wrote: > And can you explain more about the syntax? I think he means to treat COMMUTATOR etc like a generic element list, i.e. don't define new keywords in kwlist.h/gram.y at all but rather pass the names as strings (probably using a list of DefElem) and strcmp() them in OperatorUpd() or something. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
On Monday 18 May 2015 10:21:10 you wrote: > difficulty of updating existing cached plans Could you specify more precisely about some caches we talking about? PREPARE working correctly: CREATE TABLE test_ints(i int4); CREATE TABLE CREATE INDEX idx ON test_ints(i); CREATE INDEX set enable_bitmapscan=off; SET set enable_seqscan=off; SET PREPARE test_plan (int) AS SELECT * FROM test_ints WHERE $1::int4 > i; PREPARE EXPLAIN (COSTS OFF) EXECUTE test_plan(5); QUERY PLAN Index Only Scan using idx on test_ints Index Cond: (i < 5) ALTER OPERATOR > (int4, int4) SET COMMUTATOR NONE; ALTER OPERATOR EXPLAIN (COSTS OFF) EXECUTE test_plan(5); QUERY PLAN Index Only Scan using idx on test_ints Filter: (5 > i) And can you explain more about the syntax? Thanks. -- Uriy Zhuravlev Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
On Mon, May 18, 2015 at 5:44 PM, Uriy Zhuravlev wrote: > On Monday 18 May 2015 10:21:10 Tom Lane wrote: > > There are fairly significant reasons why we have not done this, based > > on the difficulty of updating existing cached plans that might have > > incidentally depended on those operator properties during creation. > > Perhaps it's all right to simply ignore such concerns, but I would like > > to see a defense of why. > > Then need to prohibit the use of shells operators (stub operators) to > create > self-linkage. Implicitly changing commutators and negators working for a > long > time through the shell operators. > I could give another motivation. AFAICS, typically ALTER OPERATOR should introduce enchancements. For instance, some version of extension didn't have negator for operator. In the next version extension introduce such negator. Or the same situation with selectivity estimation. If ALTER OPERATOR introduce only enchancements then old plans could be not optimal but they don't lead to invalid query answers. From this point of view cache invalidation after ALTER OPERATOR is term of optimization. We could include into the patch documentation statement about possible side effects with cached query plans. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
On Monday 18 May 2015 10:21:10 Tom Lane wrote: > There are fairly significant reasons why we have not done this, based > on the difficulty of updating existing cached plans that might have > incidentally depended on those operator properties during creation. > Perhaps it's all right to simply ignore such concerns, but I would like > to see a defense of why. Then need to prohibit the use of shells operators (stub operators) to create self-linkage. Implicitly changing commutators and negators working for a long time through the shell operators. > On a more practical level, please change the syntax so that it does > not require making all those things into keywords. Do you say about CREATE OPERATOR like syntax? Thanks. -- Uriy Zhuravlev Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Enhanced ALTER OPERATOR
Uriy Zhuravlev writes: > I have attached a patch that extends ALTER OPERATOR to support COMMUTATOR, > NEGATOR, RESTRICT and JOIN. There are fairly significant reasons why we have not done this, based on the difficulty of updating existing cached plans that might have incidentally depended on those operator properties during creation. Perhaps it's all right to simply ignore such concerns, but I would like to see a defense of why. On a more practical level, please change the syntax so that it does not require making all those things into keywords. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] WIP: Enhanced ALTER OPERATOR
Hello Hackers.
I have attached a patch that extends ALTER OPERATOR to support COMMUTATOR,
NEGATOR, RESTRICT and JOIN. This patch is based on master. It is small patch
with regression tests.
Why do it?
The operator has four important parameters that can be set only during the
creation. These are: commutator (oprcom), negator (oprnegate), restrict
(oprrest), join (oprjoin). For example, you created operator with RESTRICT =
contsel . After a while you began to actively use your new operator. Then you
develop a new function for RESTRICT (my_restrict_func). To change the
RESTRICT operator you have to create a new database and to migrate there
because operator is used and you can't DROP OPERATOR and CREATE OPERATOR
again. The fact that it is extremely difficult sometimes almost think it is
clear to all.
It is interesting that the change in the parameters of the operator takes
place periodically for the built-in operators (when changing major version),
but it is impossible for users defined operators.
Real life example is intarray (
http://www.postgresql.org/message-id/capphfdssy+qepdcovxx-b4lp3ybr+qs04m6-arggknfk3fr...@mail.gmail.com
).
Also using ALTER OPERATOR for self-linkage more logical than make operator
shells.
Simple syntax example:
ALTER OPERATOR === (boolean, boolean) SET COMMUTATOR =;
ALTER OPERATOR === (boolean, boolean) SET NEGATOR =;
ALTER OPERATOR === (boolean, boolean) SET RESTRICT example_func;
ALTER OPERATOR === (boolean, boolean) SET JOIN example_func;
ALTER OPERATOR === (boolean, boolean) SET COMMUTATOR NONE;
ALTER OPERATOR === (boolean, boolean) SET NEGATOR NONE;
ALTER OPERATOR === (boolean, boolean) SET RESTRICT NONE;
ALTER OPERATOR === (boolean, boolean) SET JOIN NONE;
It seems to me a syntax similar to the classic ALTER will be better than what
was used in the CREATE OPERATOR.
In this patch I am:
1. renamed OperatorUpd to ShellOperatorUpd. It more right name.
2. created AlterOperatorStmt struct for parsing command.
3. created ExecAlterOperatorStmt function for check user rights and select
parameter for edit.
4. recreated OperatorUpd for update params of operator in catalog.
5. And other small fix for extend parser.
In AlterOperatorStmt confuses me to use const char for cmd_name. In addition,
I clean only the catalog cache but judging by how works shell operators,
nothing more is needed.
Thanks!
--
Uriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companydiff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index 072f530..7ff95c1 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -30,6 +30,7 @@
#include "catalog/pg_type.h"
#include "miscadmin.h"
#include "parser/parse_oper.h"
+#include "parser/parse_func.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
@@ -53,7 +54,7 @@ static Oid OperatorShellMake(const char *operatorName,
Oid leftTypeId,
Oid rightTypeId);
-static void OperatorUpd(Oid baseId, Oid commId, Oid negId);
+static void ShellOperatorUpd(Oid baseId, Oid commId, Oid negId);
static Oid get_other_operator(List *otherOp,
Oid otherLeftTypeId, Oid otherRightTypeId,
@@ -563,7 +564,7 @@ OperatorCreate(const char *operatorName,
commutatorId = operatorObjectId;
if (OidIsValid(commutatorId) || OidIsValid(negatorId))
- OperatorUpd(operatorObjectId, commutatorId, negatorId);
+ ShellOperatorUpd(operatorObjectId, commutatorId, negatorId);
return address;
}
@@ -633,7 +634,7 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
}
/*
- * OperatorUpd
+ * ShellOperatorUpd
*
* For a given operator, look up its negator and commutator operators.
* If they are defined, but their negator and commutator fields
@@ -642,7 +643,7 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
* which are the negator or commutator of each other.
*/
static void
-OperatorUpd(Oid baseId, Oid commId, Oid negId)
+ShellOperatorUpd(Oid baseId, Oid commId, Oid negId)
{
int i;
Relation pg_operator_desc;
@@ -864,3 +865,164 @@ makeOperatorDependencies(HeapTuple tuple)
return myself;
}
+
+/*
+ * Operator update aka ALTER OPERATOR for COMMUTATOR, NEGATOR, RESTRICT, JOIN
+ */
+void OperatorUpd(Oid classId,
+ Oid baseId,
+ List *operator_param,
+ unsigned int operator_param_type)
+{
+ int i;
+ Relation catalog;
+ HeapTuple tup;
+ Oid operator_param_id = 0;
+ Form_pg_operator DstOperatorData;
+ bool otherDefined;
+ bool nulls[Natts_pg_operator];
+ bool replaces[Natts_pg_operator];
+ Datum values[Natts_pg_operator];
+
+ for (i = 0; i < Natts_pg_operator; ++i)
+ {
+ values[i] = (Datum) 0;
+ replaces[i] = false;
+ nulls[i] = false;
+ }
+
+ catalog = heap_open(classId, RowExclusiveLock);
+ tup = SearchSysCacheCopy1(OPEROID, ObjectIdGetDatum(baseId));
+ if (HeapTupleIsValid(tup))
+ {
+ DstOperatorData = (Form_pg_operator)
