Обсуждение: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

Tommy Pavlicek <tommypav122@gmail.com> writes:
> I've attached a couple of patches to allow ALTER OPERATOR to add
> commutators, negators, hashes and merges to operators that lack them.

Please add this to the upcoming commitfest [1], to ensure we don't
lose track of it.

> The first patch is create_op_fixes_v1.patch and it includes some
> refactoring in preparation for the ALTER OPERATOR changes and fixes a
> couple of minor bugs in CREATE OPERATOR:
> - prevents self negation when filling in/creating an existing shell operator
> - remove reference to sort operator in the self negation error message as
> the sort attribute appears to be deprecated in Postgres 8.3

Hmm, yeah, I bet nobody has looked at those edge cases in awhile.

> Additionally, I wasn't sure whether it was preferred to fail or succeed on
> ALTERs that have no effect, such as adding hashes on an operator that
> already allows them or disabling hashes on one that does not. I chose to
> raise an error when this happens, on the thinking it was more explicit and
> made the code simpler, even though the end result would be what the user
> wanted.

You could argue that both ways I guess.  We definitely need to raise error
if the command tries to change an existing nondefault setting, since that
might break things as per previous discussion.  But perhaps rejecting
an attempt to set the existing setting is overly nanny-ish.  Personally
I think I'd lean to "don't throw an error if we don't have to", but I'm
not strongly set on that position.

(Don't we have existing precedents that apply here?  I can't offhand
think of any existing ALTER commands that would reject no-op requests,
but maybe that's not a direct precedent.)

            regards, tom lane

[1] https://commitfest.postgresql.org/43/

Tom Lane <tgl@sss.pgh.pa.us> writes:

> Tommy Pavlicek <tommypav122@gmail.com> writes:
>
>> Additionally, I wasn't sure whether it was preferred to fail or succeed on
>> ALTERs that have no effect, such as adding hashes on an operator that
>> already allows them or disabling hashes on one that does not. I chose to
>> raise an error when this happens, on the thinking it was more explicit and
>> made the code simpler, even though the end result would be what the user
>> wanted.
>
> You could argue that both ways I guess.  We definitely need to raise error
> if the command tries to change an existing nondefault setting, since that
> might break things as per previous discussion.  But perhaps rejecting
> an attempt to set the existing setting is overly nanny-ish.  Personally
> I think I'd lean to "don't throw an error if we don't have to", but I'm
> not strongly set on that position.
>
> (Don't we have existing precedents that apply here?  I can't offhand
> think of any existing ALTER commands that would reject no-op requests,
> but maybe that's not a direct precedent.)

Since it only supports adding these operations if they don't already
exist, should it not be ALTER OPERATOR ADD <thing>, not SET <thing>?

That makes it natural to add an IF NOT EXISTS clause, like ALTER TABLE
ADD COLUMN has, to make it a no-op instead of an error.

>             regards, tom lane

- ilmari

=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> (Don't we have existing precedents that apply here?  I can't offhand
>> think of any existing ALTER commands that would reject no-op requests,
>> but maybe that's not a direct precedent.)

> Since it only supports adding these operations if they don't already
> exist, should it not be ALTER OPERATOR ADD <thing>, not SET <thing>?
> That makes it natural to add an IF NOT EXISTS clause, like ALTER TABLE
> ADD COLUMN has, to make it a no-op instead of an error.

Hmm, maybe.  But it feels like choosing syntax and semantics based
on what might be only a temporary implementation restriction.  We
certainly don't handle any other property-setting commands that way.

Admittedly, "can't change an existing setting" is likely to be pretty
permanent in this case, just because I don't see a use-case for it
that'd justify the work involved.  (My wife recently gave me a coffee
cup that says "Nothing is as permanent as a temporary fix.")  But
still, if someone did show up and do that work, we'd regret this
choice of syntax because it'd then be uselessly unlike every other
ALTER command.

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

> Please add this to the upcoming commitfest [1], to ensure we don't
> lose track of it.

I've added a single patch here: https://commitfest.postgresql.org/43/4389/

It wasn't obvious whether I should create a second commitfest entry
because I've included 2 patches so I've just done 1 to begin with. On
that note, is it preferred here to split patches of this size into
separate patches, and if so, additionally, separate threads?

Tom Lane <tgl@sss.pgh.pa.us> writes:

> > Additionally, I wasn't sure whether it was preferred to fail or succeed on
> > ALTERs that have no effect, such as adding hashes on an operator that
> > already allows them or disabling hashes on one that does not. I chose to
> > raise an error when this happens, on the thinking it was more explicit and
> > made the code simpler, even though the end result would be what the user
> > wanted.
>
> You could argue that both ways I guess.  We definitely need to raise error
> if the command tries to change an existing nondefault setting, since that
> might break things as per previous discussion.  But perhaps rejecting
> an attempt to set the existing setting is overly nanny-ish.  Personally
> I think I'd lean to "don't throw an error if we don't have to", but I'm
> not strongly set on that position.
>
> (Don't we have existing precedents that apply here?  I can't offhand
> think of any existing ALTER commands that would reject no-op requests,
> but maybe that's not a direct precedent.)

My initial thinking behind the error for a no-op was largely driven by
the existence of 'DROP.. IF EXISTS'. However, I did some ad hoc
testing on ALTER commands and it does seem that they mostly allow
no-ops. I did find that renaming an object to the same name will fail
due to the object already existing, but that seems to be more of a
coincidence than a design decision to me. Given this, I also lean
towards allowing the no-ops and will change it unless there are
objections.

Tommy Pavlicek <tommypav122@gmail.com> writes:
> I've added a single patch here: https://commitfest.postgresql.org/43/4389/

> It wasn't obvious whether I should create a second commitfest entry
> because I've included 2 patches so I've just done 1 to begin with. On
> that note, is it preferred here to split patches of this size into
> separate patches, and if so, additionally, separate threads?

No, our commitfest infrastructure is unable to deal with patches that have
interdependencies unless they're presented in a single email.  So just use
one thread, and be sure to attach all the patches each time.

(BTW, while you seem to have gotten away with it so far, it's usually
advisable to name the patch files like 0001-foo.patch, 0002-bar.patch,
etc, to make sure the cfbot understands what order to apply them in.)

            regards, tom lane

On Fri, Jun 23, 2023 at 12:21 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Tommy Pavlicek <tommypav122@gmail.com> writes:
> > I've added a single patch here: https://commitfest.postgresql.org/43/4389/
>
> > It wasn't obvious whether I should create a second commitfest entry
> > because I've included 2 patches so I've just done 1 to begin with. On
> > that note, is it preferred here to split patches of this size into
> > separate patches, and if so, additionally, separate threads?
>
> No, our commitfest infrastructure is unable to deal with patches that have
> interdependencies unless they're presented in a single email.  So just use
> one thread, and be sure to attach all the patches each time.
>
> (BTW, while you seem to have gotten away with it so far, it's usually
> advisable to name the patch files like 0001-foo.patch, 0002-bar.patch,
> etc, to make sure the cfbot understands what order to apply them in.)
>
>                         regards, tom lane

Thanks.

I've attached a new version of the ALTER OPERATOR patch that allows
no-ops. It should be ready to review now.

/*
 * 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.

in doc/src/sgml/ref/alter_operator.sgml

   <varlistentry>
    <term><literal>HASHES</literal></term>
    <listitem>
     <para>
     Indicates this operator can support a hash join. Can only be set
when the operator does not support a hash join.
     </para>
    </listitem>
   </varlistentry>

   <varlistentry>
    <term><literal>MERGES</literal></term>
    <listitem>
     <para>
     Indicates this operator can support a merge join. Can only be set
when the operator does not support a merge join.
     </para>
    </listitem>
   </varlistentry>
------------------------
if the operator cannot support hash/merge join, it can't do ALTER
OPERATOR oprname(left_arg, right_arg) SET (HASHES/MERGES = false);

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

Tommy Pavlicek <tommypav122@gmail.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

On Thu, Sep 28, 2023 at 9:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Tommy Pavlicek <tommypav122@gmail.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.universality@gmail.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?

Tommy Pavlicek <tommypav122@gmail.com> writes:
> 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.

Yeah, it'd make sense to tighten that up.  Per the discussion so far,
we should not allow an operator's commutator/negator links to change
once set, so overwriting the existing link with a different value
would be wrong.  But allowing creation of the new operator to proceed
with a different outcome than expected isn't good either.  I think
we should start throwing an error for that.

            regards, tom lane

On Tue, Oct 10, 2023 at 9:32 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Tommy Pavlicek <tommypav122@gmail.com> writes:
> > 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.
>
> Yeah, it'd make sense to tighten that up.  Per the discussion so far,
> we should not allow an operator's commutator/negator links to change
> once set, so overwriting the existing link with a different value
> would be wrong.  But allowing creation of the new operator to proceed
> with a different outcome than expected isn't good either.  I think
> we should start throwing an error for that.
>
>                         regards, tom lane

Thanks.

I've added another patch (0002-require_unused_neg_com-v1.patch) that
prevents using a commutator or negator that's already part of a pair.
The only other changes from my email yesterday are that in the ALTER
command I moved the post alter hook to after OperatorUpd and the
addition of tests to verify that we can't use an existing commutator
or negator with the ALTER command.

I believe this can all be looked at again.

Cheers,
Tommy

errmsg("operator attribute \"negator\" cannot be changed if it has
already been set")));
I feel like the above message is not very helpful.

Something like the following may be more helpful for diagnosis.
errmsg("operator %s's attribute \"negator\" cannot be changed if it
has already been set", operatorname)));

when I "git apply", I've noticed some minor whitespace  warning.

Other than that, it looks fine.

jian he <jian.universality@gmail.com> writes:
> errmsg("operator attribute \"negator\" cannot be changed if it has
> already been set")));
> I feel like the above message is not very helpful.

I think it's okay to be concise about this as long as the operator
we're referring to is the target of the ALTER.  I agree that when
we're complaining about some *other* operator, we'd better spell
out which one we mean, and I made some changes to the patch to
improve that.

Pushed after a round of editorialization -- mostly cosmetic
stuff, except for tweaking some error messages.  I shortened the
test cases a bit too, as I thought they were somewhat excessive
to have as a permanent thing.

            regards, tom lane

Re: Tommy Pavlicek
> I've added another patch (0002-require_unused_neg_com-v1.patch) that
> prevents using a commutator or negator that's already part of a pair.

Hmm. I agree with the general idea of adding sanity checks, but this
might be overzealous:

This change is breaking pgsphere which has <@ @> operator pairs, but
for historical reasons also includes alternative spellings of these
operators (both called @ with swapped operand types) which now
explodes because we can't add them with the "proper" commutator and
negators declared (which point to the canonical <@ @> !<@ !@>
operators).

https://github.com/postgrespro/pgsphere/blob/master/pgs_moc_compat.sql.in

We might be able to simply delete the @ operators, but doesn't this
new check break the general possibility to have more than one spelling
for the same operator?

Christoph

Christoph Berg <myon@debian.org> writes:
> This change is breaking pgsphere which has <@ @> operator pairs, but
> for historical reasons also includes alternative spellings of these
> operators (both called @ with swapped operand types) which now
> explodes because we can't add them with the "proper" commutator and
> negators declared (which point to the canonical <@ @> !<@ !@>
> operators).

Should have guessed that somebody might be depending on the previous
squishy behavior.  Still, I can't see how the above situation is a
good idea.  Commutators/negators should come in pairs, not have
completely random links.  I think it's only accidental that this
setup isn't triggering other strange behavior.

> We might be able to simply delete the @ operators, but doesn't this
> new check break the general possibility to have more than one spelling
> for the same operator?

You can have more than one operator atop the same function.
But why didn't you make the @ operators commutators of each other,
rather than this mess?

            regards, tom lane

Re: Tom Lane
> > We might be able to simply delete the @ operators, but doesn't this
> > new check break the general possibility to have more than one spelling
> > for the same operator?
> 
> You can have more than one operator atop the same function.
> But why didn't you make the @ operators commutators of each other,
> rather than this mess?

Historical something.

You are right that the commutators could be fixed that way, but the
negators are a different question. There is no legacy spelling for
these.

Anyway, if this doesn't raise any "oh we didn't think of this"
concerns, we'll just remove the old operators in pgsphere.

Christoph

Christoph Berg <myon@debian.org> writes:
> Anyway, if this doesn't raise any "oh we didn't think of this"
> concerns, we'll just remove the old operators in pgsphere.

Well, the idea was exactly to forbid that sort of setup.
However, if we get sufficient pushback maybe we should
reconsider --- for example, maybe it'd be sane to enforce
the restriction in ALTER but not CREATE?

I'm inclined to wait and see if there are more complaints.

            regards, tom lane

On Fri, Oct 20, 2023 at 5:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Pushed after a round of editorialization -- mostly cosmetic
> stuff, except for tweaking some error messages.  I shortened the
> test cases a bit too, as I thought they were somewhat excessive
> to have as a permanent thing.

Thanks Tom.

On Tue, Oct 24, 2023 at 2:51 PM Christoph Berg <myon@debian.org> wrote:
>
> Re: Tommy Pavlicek
> > I've added another patch (0002-require_unused_neg_com-v1.patch) that
> > prevents using a commutator or negator that's already part of a pair.
>
> Hmm. I agree with the general idea of adding sanity checks, but this
> might be overzealous:

I can't add much beyond what Tom said, but I think this does go a bit
beyond a sanity check. I forgot to mention it in my previous message,
but the main reason I noticed this was because the DELETE operator
code cleans up commutator and negator links to the operator being
deleted and that code expects each to be part of exactly a pair.

Re: Tom Lane
> Well, the idea was exactly to forbid that sort of setup.

Fwiw, pgsphere has remove the problematic operators now:

https://github.com/postgrespro/pgsphere/commit/e810f5ddd827881b06a92a303c5c9fbf997b892e

> However, if we get sufficient pushback maybe we should
> reconsider --- for example, maybe it'd be sane to enforce
> the restriction in ALTER but not CREATE?

Hmm, that seems backwards, I'd expect that CREATE might have some
checks that could circumvent using ALTER if I really insisted. If
CREATE can create things that I can't reach by ALTERing existing other
things, that's weird.

Let's keep it like it is now in PG17.

Christoph