WIP: Enhanced ALTER OPERATOR

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

On Mon, May 18, 2015 at 5:44 PM, Uriy Zhuravlev <u.zhuravlev@postgrespro.ru>
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

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, May 27, 2015 at 9:28 PM, Robert Haas <robertmhaas@gmail.com> wrote:

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.

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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?

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers