On Wed, Mar 23, 2016 at 12:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> I did not find this version very clear. It wasn't consistent about
>> using ObjectIdGetDatum() where needed, but the bigger problem was that
>> I found the logic unnecessarily convoluted. I rewrote it - I believe
>> more straightforwardly - as attached. How does this look?
>
> I'd suggest that we save some code by always doing separate updates for
> the commutator and negator entries. We can handle the corner case where
> they're the same by doing a CommandCounterIncrement between the updates,
> instead of having convoluted and probably-never-yet-tested logic.
Sure, we could do that, but it isn't necessary. If the logic never
gets hit, the question of whether it has bugs isn't that important.
And I'd rather not rejigger things more than necessary in something
that's going to be back-patched.
> I'm also a bit dubious of the assumption in RemoveOperatorById that an
> operator can't be its own negator. Yeah, that should not be the case,
> but if it is the case the deletion will fail outright.
So what? We've never guaranteed that things are going to work if you
start by corrupting the catalogs, and I wouldn't pick this as a place
to start.
> We could resolve both of these issues by changing the semantics of
> OprUpdate so that it unconditionally does a CommandCounterIncrement
> after each update that it performs. IMO that would be a lot simpler
> and more bulletproof; it'd allow removal of a lot of these
> overly-tightly-reasoned cases.
I tried this, but it did not seem to work. With the command counter
increments added and the conditional logic removed, I got:
rhaas=# CREATE OPERATOR === (PROCEDURE = int8eq, LEFTARG = bigint,
RIGHTARG = bigint);
CREATE OPERATOR
rhaas=# update pg_operator set oprnegate = oid where oprname = '===';
UPDATE 1
rhaas=# drop operator === (bigint, bigint);
ERROR: attempted to delete invisible tuple
The same test case without those changes fails with:
ERROR: tuple already updated by self
Interestingly, that test case passes on unpatched master.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company