Re: [PATCH] fix DROP OPERATOR to reset links to itself on commutator and negator

Hi,

On 03/23/2016 12:50 AM, Roma Sokolov wrote:

Hi,

Tomas, thanks for review and comments!

I have to admit I find the existing code a bit convoluted,
particularly the part that deals with the (commId == negId) case.
And the patch does not really improve the situation, quite the
contrary. Perhaps it’s time to get rid of this optimization?

Indeed, code in OperatorUpd is not very easy to read, due to
handling this case. However we can achieve the same results without
too much duplication. I have changed OperatorUpd to perform tuple
modification in "lazy" way. Please, check it out in v4.patch
(attached).

OK, the new code seems more comprehensible to me.

Also, maybe I'm missing something obvious, but it's not
immediately obvious to me why we're only checking oprcom and not
oprnegate? I.e. why shouldn’t the code be

We do not need to check for operOid != op->oprnegate, since we can't
create operator that is negator to itself. Thus, opnergate either
present and differs from operator being deleted, or is InvalidOid. I
have added some clarification in the comment for future readers.

Ah, I see. Thanks for the clarification.

Fixed style issues as well.

I've noticed some whitespace issues in the OperatorUpd function - there
are two or three lines with just a tabulator at the beginning, and one
comment mixes indentation by tabs with spaces.

Also, it's generally recommended no to tweak formatting when not
necessary, so perhaps don't remove the empty line at the end of the
function (before simple_heap_update).

I think the comments will need rewording, but I'll leave that to a
native speaker.

regards
Tomas

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Mar 22, 2016 at 9:25 PM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:

OK, the new code seems more comprehensible to me.

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?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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.

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.

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.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Mar 23, 2016 at 12:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

I would not be worried except that it breaks a case that used to work,
as your test below demonstrates.

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.

Odd. If you post the revised patch, I'll try to chase down what's wrong.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Mar 23, 2016 at 12:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

Odd. If you post the revised patch, I'll try to chase down what's wrong.

After playing with this, I'll bet you forgot that RemoveOperatorById would
need to re-fetch the target tuple if it got updated. We could
alternatively fix that by skipping updates on the tuple due to be deleted,
but that would convolute the logic in OperatorUpd, which didn't seem
worthwhile to me.

I found some other stuff needing fixing (mostly typos in comments) and
also realized that we don't really need to bother with heap_modify_tuple
at all. I pushed it with those fixes.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Mar 25, 2016 at 12:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Mar 23, 2016 at 12:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

Odd. If you post the revised patch, I'll try to chase down what's wrong.

After playing with this, I'll bet you forgot that RemoveOperatorById would
need to re-fetch the target tuple if it got updated. We could
alternatively fix that by skipping updates on the tuple due to be deleted,
but that would convolute the logic in OperatorUpd, which didn't seem
worthwhile to me.

I found some other stuff needing fixing (mostly typos in comments) and
also realized that we don't really need to bother with heap_modify_tuple
at all. I pushed it with those fixes.

Cool.

--
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