On Wed, Mar 23, 2016 at 12:27 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > Robert Haas <robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
