Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

On Thu, Sep 28, 2023 at 9:18 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Tommy Pavlicek <tommypav122(at)gmail(dot)com> writes:
> > I've attached a new version of the ALTER OPERATOR patch that allows
> > no-ops. It should be ready to review now.
>
> I got around to looking through this finally (sorry about the delay).
> I'm mostly on board with the functionality, with the exception that
> I don't see why we should allow ALTER OPERATOR to cause new shell
> operators to be created. The argument for allowing that in CREATE
> OPERATOR was mainly to allow a linked pair of operators to be created
> without a lot of complexity (specifically, being careful to specify
> the commutator or negator linkage only in the second CREATE, which
> is a rule that requires another exception for a self-commutator).
> However, if you're using ALTER OPERATOR then you might as well create
> both operators first and then link them with an ALTER command.
> In fact, I don't really see a use-case where the operators wouldn't
> both exist; isn't this feature mainly to allow retrospective
> correction of omitted linkages? So I think allowing ALTER to create a
> second operator is more likely to allow mistakes to sneak by than to
> do anything useful --- and they will be mistakes you can't correct
> except by starting over. I'd even question whether we want to let
> ALTER establish a linkage to an existing shell operator, rather than
> insisting you turn it into a valid operator via CREATE first.
>
> If we implement it with that restriction then I don't think the
> refactorization done in 0001 is correct, or at least not ideal.
>
> (In any case, it seems like a bad idea that the command reference
> pages make no mention of this stuff about shell operators. It's
> explained in 38.15. Operator Optimization Information, but it'd
> be worth at least alluding to that section here. Or maybe we
> should move that info to CREATE OPERATOR?)
>
> More generally, you muttered something about 0001 fixing some
> existing bugs, but if so I can't see those trees for the forest of
> refactorization. I'd suggest splitting any bug fixes apart from
> the pure-refactorization step.
>
> regards, tom lane

Thanks Tom.

The rationale behind the shell operator and that part of section 38.15
of the documentation had escaped me, but what you're saying makes
complete sense. Based on your comments, I've made some changes:

1. I've isolated the bug fixes (fixing the error message and
disallowing self negation when filling in a shell operator) into
0001-bug-fixes-v3.patch.
2. I've scaled back most of the refactoring as I agree it no longer makes sense.
3. I updated the logic to prevent the creation of or linking to shell operators.
4. I made further updates to the documentation including referencing
38.15 directly in the CREATE and ALTER pages (It's easy to miss if
only 38.14 is referenced) and moved the commentary about creating
commutators and negators into the CREATE section as with the the ALTER
changes it now seems specific to CREATE. I didn't move the rest of
38.15 as I think this applies to both CREATE and ALTER.

I did notice one further potential bug. When creating an operator and
adding a commutator, PostgreSQL only links the commutator back to the
operator if the commutator has no commutator of its own, but the
create operation succeeds regardless of whether this linkage happens.

In other words, if A and B are a pair of commutators and one creates
another operator, C, with A as its commutator, then C will link to A,
but A will still link to B (and B to A). It's not clear to me if this
in itself is a problem, but unless I've misunderstood something
operator C must be the same as B so it implies an error by the user
and there could also be issues if A was deleted since C would continue
to refer to the deleted A.

The same applies for negators and alter operations.

Do you know if this behaviour is intentional or if I've missed
something because it seems undesirable to me. If it is a bug, then I
think I can see how to fix it, but wanted to ask before making any
changes.

On Mon, Sep 25, 2023 at 11:52 AM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> /*
> * AlterOperator
> * routine implementing ALTER OPERATOR <operator> SET (option = ...).
> *
> * Currently, only RESTRICT and JOIN estimator functions can be changed.
> */
> ObjectAddress
> AlterOperator(AlterOperatorStmt *stmt)
>
> The above comment needs to change, other than that, it passed the
> test, works as expected.

Thanks, added a comment.

> Can only be set when the operator does support a hash/merge join. Once
> set to true, it cannot be reset to false.

Yes, I updated the wording. Is it clearer now?