Обсуждение: operator dependency of commutator and negator, redux

Bug #7758 seems to be a rediscovery of the behavior that Itagaki-san
complained of a couple years ago:
http://archives.postgresql.org/pgsql-hackers/2010-09/msg02035.php

While reconsidering the various not-too-satisfactory fixes we thought of
back then, I had a sudden thought.  Instead of having a COMMUTATOR or
NEGATOR forward reference create a "shell" operator and link to it,
why not simply *ignore* such references?  Then when the second operator
is defined, go ahead and fill in both links?

The only case where this could result in an unsatisfactory outcome is
if the second operator's CREATE command fails to include the converse
COMMUTATOR or NEGATOR reference ... but that doesn't work very nicely
today anyway, as you end up with a unidirectional reference, hardly a
desirable state of affairs.

Not only does this solve the problem complained of, but it allows for
much stronger error checking, as there is no longer any need to allow
inconsistent catalog states even transiently.  We could start treating
commutator/negator references as true dependencies, permanently
preventing dangling references.  We could probably even get rid of the
notion of shell operators altogether.

Thoughts?
        regards, tom lane

On 20 December 2012 11:51, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> While reconsidering the various not-too-satisfactory fixes we thought of
> back then, I had a sudden thought.  Instead of having a COMMUTATOR or
> NEGATOR forward reference create a "shell" operator and link to it,
> why not simply *ignore* such references?  Then when the second operator
> is defined, go ahead and fill in both links?


Ignore with warning sounds pretty good.  So it would go something like this?

# CREATE OPERATOR < (... COMMUTATOR >);
WARNING: COMMUTATOR > (foo, foo) undefined, ignoring.
CREATE OPERATOR

# CREATE OPERATOR > (... COMMUTATOR <);
CREATE OPERATOR


Cheers,
BJ

Brendan Jurd <direvus@gmail.com> writes:
> On 20 December 2012 11:51, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> While reconsidering the various not-too-satisfactory fixes we thought of
>> back then, I had a sudden thought.  Instead of having a COMMUTATOR or
>> NEGATOR forward reference create a "shell" operator and link to it,
>> why not simply *ignore* such references?  Then when the second operator
>> is defined, go ahead and fill in both links?

> Ignore with warning sounds pretty good.  So it would go something like this?

> # CREATE OPERATOR < (... COMMUTATOR >);
> WARNING: COMMUTATOR > (foo, foo) undefined, ignoring.
> CREATE OPERATOR

> # CREATE OPERATOR > (... COMMUTATOR <);
> CREATE OPERATOR

I was thinking a NOTICE at most.  If it's a WARNING then restoring
perfectly valid pg_dump files will result in lots of scary-looking
chatter.  You could make an argument for printing nothing at all,
but that would probably mislead people who'd fat-fingered their
COMMUTATOR entries.
        regards, tom lane

On Thu, Dec 20, 2012 at 10:52 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Brendan Jurd <direvus@gmail.com> writes:
>> On 20 December 2012 11:51, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> While reconsidering the various not-too-satisfactory fixes we thought of
>>> back then, I had a sudden thought.  Instead of having a COMMUTATOR or
>>> NEGATOR forward reference create a "shell" operator and link to it,
>>> why not simply *ignore* such references?  Then when the second operator
>>> is defined, go ahead and fill in both links?
>
>> Ignore with warning sounds pretty good.  So it would go something like this?
>
>> # CREATE OPERATOR < (... COMMUTATOR >);
>> WARNING: COMMUTATOR > (foo, foo) undefined, ignoring.
>> CREATE OPERATOR
>
>> # CREATE OPERATOR > (... COMMUTATOR <);
>> CREATE OPERATOR
>
> I was thinking a NOTICE at most.  If it's a WARNING then restoring
> perfectly valid pg_dump files will result in lots of scary-looking
> chatter.  You could make an argument for printing nothing at all,
> but that would probably mislead people who'd fat-fingered their
> COMMUTATOR entries.

What about jiggering the dump so that only the second of the two
operators to be dumped includes the COMMUTATOR clause?  Or even adding
a separate ALTER OPERATOR < COMMUTATOR > statement (or something of
the sort) that pg_dump can emit as a separate item.  Even a NOTICE in
pg_dump seems like too much chatter (witness recent quieting of some
other NOTICE messages we've all grown tired of) but silently ignoring
the problem doesn't seem smart either, for the reason you mention.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Dec 20, 2012 at 10:52 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I was thinking a NOTICE at most.  If it's a WARNING then restoring
>> perfectly valid pg_dump files will result in lots of scary-looking
>> chatter.  You could make an argument for printing nothing at all,
>> but that would probably mislead people who'd fat-fingered their
>> COMMUTATOR entries.

> What about jiggering the dump so that only the second of the two
> operators to be dumped includes the COMMUTATOR clause?

Seems messy and fragile.  In particular this'd represent a lot of work
in order to make it more likely that the restore malfunctions if someone
makes use of pg_restore's -l switch to reorder the entries.  Also it
would not retroactively fix the problem for restoring dumps made with
existing pg_dump versions.

> Even a NOTICE in
> pg_dump seems like too much chatter (witness recent quieting of some
> other NOTICE messages we've all grown tired of)

pg_dump has included "set client_min_messages = warning" in its output
for quite some time now.  So as long as we don't insist on making this
a WARNING, people won't see it in that usage.
        regards, tom lane

Robert Haas <robertmhaas@gmail.com> writes:
> a separate ALTER OPERATOR < COMMUTATOR > statement (or something of
> the sort) that pg_dump can emit as a separate item.  Even a NOTICE in

I like that capability, but it's not helping us in the backward
compatibility section where we will still read commutator declarations
as operator "properties". And maintaining an extension with different
syntax for CREATE OPERATOR depending on the major version would be a
pain (it's the case already for create type by the way, painfully so).

So I think Tom's idea is better to fix the problem at hand.


About dropping the Operator Shell, we've been talking in the past about
adding more properties to our operators to allow for some more optimizer
tricks (reducing expressions to constants or straigth variable
references at parse time, reducing joins, adding parametrized paths
etc).

I can think about assiociativity and neutral element, but that's a
property of one operator only. Now there's the distributive property
that happens in between two different operators and that maybe would
better be added as an ALTER OPERATOR statement rather than a possibly
forward reference when we come to that.

I'm not too sure about other concepts that we might want to tackle down
the road here, another angle here would be about support for parallelism
where maybe operators property could tell the planner how to spread a
complex where clause or output column computation…


All in all, it looks to me like the current proposals on the table would
allow us to dispose of the Operator Shell idea entirely. If we ever need
it back, the ALTER OPERATOR trick looks like a better tool.


Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support