From: Tom Lane <tgl@sss.pgh.pa.us>
To: Tommy Pavlicek <tommypav122@gmail.com>
Cc: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
Cc: pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>
Subject: Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
Date: Thu, 28 Sep 2023 16:18:34 -0400
Message-ID: <1092363.1695932314@sss.pgh.pa.us> (raw)
In-Reply-To: <CAEhP-W-bY5CTXgrsH_J=NyKJpqMOgUPqMQ9-oTtx2N+zND1ZuA@mail.gmail.com>
References: <CAEhP-W-vGVzf4udhR5M8Bdv88UYnPrhoSkj3ieR3QNrsGQoqdg@mail.gmail.com>
<4077062.1687452468@sss.pgh.pa.us>
<87r0q3qvxd.fsf@wibble.ilmari.org>
<4082750.1687455374@sss.pgh.pa.us>
<CAEhP-W_xGQVmiEJE_x0x5bbDSwsmaN1TQEawNadiMvQj2XK2Sg@mail.gmail.com>
<15382.1687519306@sss.pgh.pa.us>
<CAEhP-W-bY5CTXgrsH_J=NyKJpqMOgUPqMQ9-oTtx2N+zND1ZuA@mail.gmail.com>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, tommypav122@gmail.com, ilmari@ilmari.org, tomas.vondra@enterprisedb.com
Subject: Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
In-Reply-To: <1092363.1695932314@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox