operator dependency of commutator and negator

When we drop an operator used by other operators as COMMUTATOR or NEGATOR,
pg_dump generates an invalid SQL command for the operators depending on
the dropped one. Is it an unavoidable restriction?

CREATE OPERATOR <<< (
PROCEDURE = text_lt, LEFTARG = text, RIGHTARG = text, COMMUTATOR = >>>
);
CREATE OPERATOR >>> (
PROCEDURE = text_gt, LEFTARG = text, RIGHTARG = text, COMMUTATOR = <<<
);
DROP OPERATOR >>> (text, text);

$ pg_dump
--
-- Name: <<<; Type: OPERATOR; Schema: public; Owner: postgres
--
CREATE OPERATOR <<< (
PROCEDURE = text_lt,
LEFTARG = text,
RIGHTARG = text,
COMMUTATOR = 16395 <== HERE
);

--
Itagaki Takahiro

Excerpts from Itagaki Takahiro's message of mié sep 29 03:56:33 -0400 2010:
> When we drop an operator used by other operators as COMMUTATOR or NEGATOR,
> pg_dump generates an invalid SQL command for the operators depending on
> the dropped one. Is it an unavoidable restriction?

Maybe we need a pg_depend entry from each pg_operator entry to the other
one. The problem is that this creates a cycle in the depends graph; not
sure how well these are handled in the code, if at all.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Excerpts from Itagaki Takahiro's message of mi sep 29 03:56:33 -0400 2010:
>> When we drop an operator used by other operators as COMMUTATOR or NEGATOR,
>> pg_dump generates an invalid SQL command for the operators depending on
>> the dropped one. Is it an unavoidable restriction?

> Maybe we need a pg_depend entry from each pg_operator entry to the other
> one. The problem is that this creates a cycle in the depends graph; not
> sure how well these are handled in the code, if at all.

See the comment in catalog/pg_operator.c:

/*
* NOTE: we do not consider the operator to depend on the associated
* operators oprcom and oprnegate. We would not want to delete this
* operator if those go away, but only reset the link fields; which is not
* a function that the dependency code can presently handle. (Something
* could perhaps be done with objectSubId though.) For now, it's okay to
* let those links dangle if a referenced operator is removed.
*/

I'm not sure that fixing this case is worth the amount of work it'd
take. How often do you drop just one member of a commutator pair?

regards, tom lane

On Wed, Sep 29, 2010 at 11:56 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I'm not sure that fixing this case is worth the amount of work it'd
> take.  How often do you drop just one member of a commutator pair?

I found the issue when an user tries to write a "safe" installer
script under "DROP before CREATE" coding rule:

1. DROP OPERATOR IF EXISTS <<< ... ;
2. CREATE OPERATOR <<< (... COMMUTATOR >>>);
3. DROP OPERATOR IF EXISTS >>> ... ;
4. CREATE OPERATOR >>> (... COMMUTATOR <<<);

3 drops catalog-only >>> added at 2, and 4 adds a operator that
has a different oid from <<<'s commutator. The operator <<<
becomes broken state in system catalog.

Anyway, it must be a rare case, and we can just avoid the usage.

--
Itagaki Takahiro

Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
> On Wed, Sep 29, 2010 at 11:56 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I'm not sure that fixing this case is worth the amount of work it'd
>> take. How often do you drop just one member of a commutator pair?

> I found the issue when an user tries to write a "safe" installer
> script under "DROP before CREATE" coding rule:

> 1. DROP OPERATOR IF EXISTS <<< ... ;
> 2. CREATE OPERATOR <<< (... COMMUTATOR >>>);
> 3. DROP OPERATOR IF EXISTS >>> ... ;
> 4. CREATE OPERATOR >>> (... COMMUTATOR <<<);

> 3 drops catalog-only >>> added at 2, and 4 adds a operator that
> has a different oid from <<<'s commutator. The operator <<<
> becomes broken state in system catalog.

> Anyway, it must be a rare case, and we can just avoid the usage.

Yeah. The above script seems incorrect anyway: if we did clean
up the commutator links fully then step 3 would undo the effect
of step 2. So really you should drop all the operators first
and then start creating new ones.

On the other hand ... the above script pattern would do the right thing
if OperatorUpd() were willing to overwrite existing nonzero values in
the referenced operators' entries. I'm not sure if this is a good idea
though. I think that the reason it doesn't do it now is so that you
don't accidentally damage the links in an existing unrelated operator.
But AFAICS there are no cases where commutator and negator pairs
shouldn't be symmetrical, so simply doing nothing doesn't seem like the
right thing either: if you don't modify the other operator then you're
definitely leaving an inconsistent state in the catalogs. Maybe what we
should do is require the user to own the referenced operator and then
unconditionally force the referenced operator's link to match.

regards, tom lane