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
