Re: record identical operator - Review

On 09/12/2013 06:27 PM, Kevin Grittner wrote:
> Attached is a patch for a bit of infrastructure I believe to be
> necessary for correct behavior of REFRESH MATERIALIZED VIEW
> CONCURRENTLY as well as incremental maintenance of matviews.

Here is attempt at a review of the v1 patch.

There has been a heated discussion on list about if we even want this
type of operator. I will try to summarize it here

The arguments against it are
* that a record_image_identical call on two records that returns false
can still return true under the equality operator, and the records might
(or might not) appear to be the same to users
* Once we expose this as an operator via SQL someone will find it and
use it and then complain when we change things such as the internal
representation of a datatype which might
break there queries
* The differences between = and record_image_identical might not be
understood by everywhere who finds the operator leading to confusion
* Using a criteria other than equality (the = operator provided by the
datatype) might cause problems if we later provide 'on change' triggers
for materialized views

The arguments for this patch are
* We want the materialized view to return the same value as would be
returned if the query were executed directly. This not only means that
the values should be the same according to a datatypes = operator but
that they should appear the same 'to the eyeball'.
* Supporting the materialized view refresh check via SQL makes a lot of
sense but doing that requires exposing something via SQL
* If we adequately document what we mean by record_image_identical and
the operator we pick for this then users shouldn't be surprised at what
they get if they use this

I think there is agreement that better (as in more obscure) operators
than === and !== need to be picked and we need to find a place in the
user-docs to warn users of the behaviour of this operators. Hannu has
proposed

*==* "binary equal, surely very equal by any other definition as wall"
!==? "maybe not equal" -- "binary inequal, may still be equal by
other comparison methods"

and no one has yet objected to this. My vote would be to update the patch to use those operator names and then figure out where we can document them. It it means we have to section describing the equal operator for records as well then maybe we should do that so we can get on with things.

Code Review
--------------------

The record_image_eq and record_image_cmp functions are VERY similar. It seems to me that you could have the meet of these functions into a common function (that isn't exposed via SQL) that then behaves differently in 2 or 3 spots based on a parameter that controls if it detoasts the tuple for the compare or returns false on the equals.

Beyond that I don't see any issues with the code.

You call out a question about two records being compared have a different number of columns should it always be an error, or only an error when they match on the columns they do have in common.

The current behaviour is

select * FROM r1,r2 where r1<==r2;
a | b | a | b | c
---+---+---+---+---
1 | 1 | 1 | 2 | 1
(1 row)

update r2 set b=1;
UPDATE 1
test=# select * FROM r1,r2 where r2<==r1;
ERROR: cannot compare record types with different numbers of columns

This seems like a bad idea to me. I don't like that I get a type comparison error only sometimes based on the values of the data in the column. If I'm a user testing code that compares two records with this operator and I test my query with 1 value pair then and it works then I'd naively expect to get a true or false on all other value sets of the same record type.