Thread: 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-ARgGKNFk3FrFow@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 Company

Uriy Zhuravlev <u.zhuravlev@postgrespro.ru> 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

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

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

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> 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

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

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

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 Company

On Mon, May 18, 2015 at 10:21 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Uriy Zhuravlev <u.zhuravlev@postgrespro.ru> 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

Alexander Korotkov <a.korotkov@postgrespro.ru> 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

On Fri, May 29, 2015 at 4:28 AM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
> On Thu, May 28, 2015 at 6:43 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Alexander Korotkov <a.korotkov@postgrespro.ru> 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

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 Company

On Wed, Jun 24, 2015 at 7:30 AM, Uriy Zhuravlev
<u.zhuravlev@postgrespro.ru> 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

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 Company

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

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

 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 Company

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

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

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