Re: REPLICA IDENTITY FULL

On Fri, Jun 23, 2017 at 03:45:48PM -0400, Peter Eisentraut wrote:
> On 6/23/17 13:14, Alvaro Herrera wrote:
> > Andres Freund wrote:
> >> On 2017-06-23 13:05:21 -0400, Alvaro Herrera wrote:
> >>> Tom Lane wrote:
> >>>> Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
> >>>>> Any thoughts about keeping datumAsEqual() as a first check? I did some
> >>>>> light performance tests, but it was inconclusive.
> >>>>
> >>>> Seems like it would tend to be a win if, in fact, the values are
> >>>> usually equal. If they're usually not, then it's a loser. Do
> >>>> we have any feeling for which case is more common?
> >>
> >> Seems like a premature optimization to me - if you care about
> >> performance and do this frequently, you're not going to end up using
> >> FULL. If we want to performance optimize, it'd probably better to
> >> lookup candidate keys and use those if available.
> >
> > I can get behind that argument.
>
> Thanks for the feedback. I have committed it after removing the
> datumIsEqual() call.

While reviewing this patch, I noticed a couple of nearby defects:

- RelationFindReplTupleSeq() says "Note that this stops on the first matching
tuple.", but that's not the case. It visits every row in the table, and it
uses the last match. The claimed behavior sounds more attractive.

- RelationFindReplTupleSeq() has comment "/* Start an index scan. */", an
inapplicable copy-paste from RelationFindReplTupleByIndex().