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
outwhere we can document them.  It it means we have to section describing the equal operator for records as well then
maybewe 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
thesefunctions into a common function (that isn't exposed via SQL) that then behaves differently  in 2 or 3 spots based
ona 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
ofthe data in the column.  If I'm a user testing code that compares two records with this operator and I test my query
with1 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
recordtype.