Re: WIP: Enhanced ALTER OPERATOR

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