Thread: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
[PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
Tommy Pavlicek
Date:
Hi All,
I've attached a couple of patches to allow ALTER OPERATOR to add commutators, negators, hashes and merges to operators that lack them.
The need for this arose adding hash functions to the ltree type after the operator had been created without hash support[1]. There are potential issues with modifying these attributes that have been discussed previously[2], but I understand that setting them, if they have not been set before, is ok.
I belatedly realised that it may not be desirable or necessary to allow adding commutators and negators in ALTER OPERATOR because the linkage can already be added when creating a new operator. I don't know what's best, so I thought I'd post this here and get feedback before removing anything.
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
The second patch is alter_op_v1.patch which contains the changes to ALTER OPERATOR and depends on create_op_fixes_v1.patch.
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.
Comments appreciated.
Thanks,
Tommy
[1] https://www.postgresql.org/message-id/flat/CAEhP-W9ZEoHeaP_nKnPCVd_o1c3BAUvq1gWHrq8EbkNRiS9CvQ%40mail.gmail.com
[2] https://www.postgresql.org/message-id/flat/3348985.V7xMLFDaJO@dinodell
I've attached a couple of patches to allow ALTER OPERATOR to add commutators, negators, hashes and merges to operators that lack them.
The need for this arose adding hash functions to the ltree type after the operator had been created without hash support[1]. There are potential issues with modifying these attributes that have been discussed previously[2], but I understand that setting them, if they have not been set before, is ok.
I belatedly realised that it may not be desirable or necessary to allow adding commutators and negators in ALTER OPERATOR because the linkage can already be added when creating a new operator. I don't know what's best, so I thought I'd post this here and get feedback before removing anything.
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
The second patch is alter_op_v1.patch which contains the changes to ALTER OPERATOR and depends on create_op_fixes_v1.patch.
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.
Comments appreciated.
Thanks,
Tommy
[1] https://www.postgresql.org/message-id/flat/CAEhP-W9ZEoHeaP_nKnPCVd_o1c3BAUvq1gWHrq8EbkNRiS9CvQ%40mail.gmail.com
[2] https://www.postgresql.org/message-id/flat/3348985.V7xMLFDaJO@dinodell
Attachment
Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
Tom Lane
Date:
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/
Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
Dagfinn Ilmari Mannsåker
Date:
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
Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
Tom Lane
Date:
=?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
Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
Tommy Pavlicek
Date:
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.
Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
Tom Lane
Date:
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
Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
Tommy Pavlicek
Date:
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.
Attachment
Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
jian he
Date:
/* * 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.
Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
jian he
Date:
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.
Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
Tom Lane
Date:
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
Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
Tommy Pavlicek
Date:
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?
Attachment
Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
Tom Lane
Date:
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
Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
Tommy Pavlicek
Date:
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
Attachment
Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
jian he
Date:
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.
Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
Tom Lane
Date:
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: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
Christoph Berg
Date:
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
Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
Tom Lane
Date:
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: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
Christoph Berg
Date:
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
Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
Tom Lane
Date:
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
Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
Tommy Pavlicek
Date:
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: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
Christoph Berg
Date:
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
Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
From
Christoph Berg
Date:
Re: To Tom Lane > Let's keep it like it is now in PG17. Late followup news: This feature has actually found a bug in postgresql-debversion: CREATE OPERATOR > ( LEFTARG = debversion, RIGHTARG = debversion, COMMUTATOR = <, - NEGATOR = >=, + NEGATOR = <=, RESTRICT = scalargtsel, JOIN = scalargtjoinsel ); https://salsa.debian.org/postgresql/postgresql-debversion/-/commit/8ef08ccbea1438468249b0e94048b1a8a25fc625#000e84a71f8a28b762658375c194b25d529336f3 So, thanks! Christoph