Re: record identical operator

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.

The idea is that after RMVC or incremental maintenance, the matview
should not be visibly different that it would have been at creation
or on a non-concurrent REFRESH.  The issue is easy to demonstrate
with citext, but anywhere that the = operator allows user-visible
differences between "equal" values it can be an issue.

test=# CREATE TABLE citext_table (
test(#   id serial primary key,
test(#   name citext
test(# );
CREATE TABLE
test=# INSERT INTO citext_table (name)
test-#   VALUES ('one'), ('two'), ('three'), (NULL), (NULL);
INSERT 0 5
test=# CREATE MATERIALIZED VIEW citext_matview AS
test-#   SELECT * FROM citext_table;
SELECT 5
test=# CREATE UNIQUE INDEX citext_matview_id
test-#   ON citext_matview (id);
CREATE INDEX
test=# UPDATE citext_table SET name = 'Two' WHERE name = 'TWO';
UPDATE 1

At this point, the table and the matview have visibly different
values, yet without the patch the query used to find differences
for RMVC would be essentially like this (slightly simplified for
readability):

test=# SELECT *
  FROM citext_matview m
  FULL JOIN citext_table t ON (t.id = m.id AND t = m)
  WHERE t IS NULL OR m IS NULL;
 id | name | id | name
----+------+----+------
(0 rows)

No differences were found, so without this patch, the matview would
remain visibly different from the results generated by a run of its
defining query.

The patch adds an "identical" operator (===) for the record type:

test=# SELECT *
  FROM citext_matview m
  FULL JOIN citext_table t ON (t.id = m.id AND t === m)
  WHERE t IS NULL OR m IS NULL;
 id | name | id | name
----+------+----+------
    |      |  2 | Two
  2 | two  |    |
(2 rows)

The difference is now found, so RMVC makes the appropriate change.

test=# REFRESH MATERIALIZED VIEW CONCURRENTLY citext_matview;
REFRESH MATERIALIZED VIEW
test=# SELECT * FROM citext_matview ORDER BY id;
 id | name  
----+-------
  1 | one
  2 | Two
  3 | three
  4 |
  5 |
(5 rows)

The patch adds all of the functions, operators, and catalog
information to support merge joins using the "identical" operator.

The new operator is logically similar to IS NOT DISTINCT FROM for a
record, although its implementation is very different.  For one
thing, it doesn't replace the operation with column level operators
in the parser.  For another thing, it doesn't look up operators for
each type, so the "identical" operator does not need to be
implemented for each type to use it as shown above.  It compares
values byte-for-byte, after detoasting.  The test for identical
records can avoid the detoasting altogether for any values with
different lengths, and it stops when it finds the first column with
a difference.

I toyed with the idea of supporting hashing of records using this
operator, but could not see how that would be a performance win.

The identical (===) and not identical (!==) operator names were
chosen because of a vague similarity to the "exactly equals"
concepts in JavaScript and PHP, which use that name.  The semantics
aren't quite the same, but it seemed close enough not to be too
surprising.  The additional operator names seemed natural to me
based on the first two, but I'm not really that attached to these
names for the operators if someone has a better idea.

Since the comparison of record values is not documented (only
comparisons involving row value constructors), it doesn't seem like
we should document this special case.  It is intended primarily for
support of matview refresh and maintenance, and it seems likely
that record comparison was not documented on the basis that it is
intended primarily for support of such things as indexing and merge
joins -- so leaving the new operators undocumented seems consistent
with existing policy.  I'm open to arguments that the policy should
change.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Sep 12, 2013 at 11:27 PM, Kevin Grittner <kgrittn(at)ymail(dot)com> 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.
> [...]
> The patch adds an "identical" operator (===) for the record type:
>

[...]

> The new operator is logically similar to IS NOT DISTINCT FROM for a
> record, although its implementation is very different. For one
> thing, it doesn't replace the operation with column level operators
> in the parser. For another thing, it doesn't look up operators for
> each type, so the "identical" operator does not need to be
> implemented for each type to use it as shown above. It compares
> values byte-for-byte, after detoasting. The test for identical
> records can avoid the detoasting altogether for any values with
> different lengths, and it stops when it finds the first column with
> a difference.
>
> I toyed with the idea of supporting hashing of records using this
> operator, but could not see how that would be a performance win.
>
> The identical (===) and not identical (!==) operator names were
> chosen because of a vague similarity to the "exactly equals"
> concepts in JavaScript and PHP, which use that name. The semantics
> aren't quite the same, but it seemed close enough not to be too
> surprising. The additional operator names seemed natural to me
> based on the first two, but I'm not really that attached to these
> names for the operators if someone has a better idea.
>
> Since the comparison of record values is not documented (only
> comparisons involving row value constructors), it doesn't seem like
> we should document this special case. It is intended primarily for
> support of matview refresh and maintenance, and it seems likely
> that record comparison was not documented on the basis that it is
> intended primarily for support of such things as indexing and merge
> joins -- so leaving the new operators undocumented seems consistent
> with existing policy. I'm open to arguments that the policy should
> change.
>
> -
>

Wouldn't it be slightly less surprising / magical to not declare new
operators
but just new primitive functions? In the least invasive version they could
even
be called matview_is_record_identical or similar.

cheers,

Bene

Benedikt Grundmann <bgrundmann(at)janestreet(dot)com> wrote:

> Kevin Grittner <kgrittn(at)ymail(dot)com> 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.
>> [...]
>> The patch adds an "identical" operator (===) for the record
>> type:

> Wouldn't it be slightly less surprising / magical to not declare
> new operators but just new primitive functions?  In the least
> invasive version they could even be called
> matview_is_record_identical or similar.

I'm not sure what is particularly surprising or magical about new
operators, but it is true that the patch could be smaller if
operators were not added.  The SQL functions added by the current
patch to support the operator approach are:

extern Datum record_image_eq(PG_FUNCTION_ARGS);
extern Datum record_image_ne(PG_FUNCTION_ARGS);
extern Datum record_image_lt(PG_FUNCTION_ARGS);
extern Datum record_image_gt(PG_FUNCTION_ARGS);
extern Datum record_image_le(PG_FUNCTION_ARGS);
extern Datum record_image_ge(PG_FUNCTION_ARGS);
extern Datum btrecordimagecmp(PG_FUNCTION_ARGS);

All take two record arguments and all but the last return a
boolean.  The last one returns -1, 0, or 1 depending on how the
values compare.  All comparisons are based on memcmp() of the data
in its storage format (after detoasting, where applicable).

As currently written, the patch still requires a unique index on
the matview in order to allow RMVC, but this patch was intended to
support removal of that restriction, which is something some people
were saying they wanted.  It just seemed best to do that with a
separate patch once we had the mechanism to support it.

RMVC currently generates its "diff" data with a query similar to
this:

test=# explain analyze
test-# SELECT *
test-#   FROM citext_matview m
test-#   FULL JOIN citext_table t ON (t === m)
test-#   WHERE t.id IS NULL OR m.id IS NULL;
test=# \pset pager off
Pager usage is off.
test=# explain analyze
SELECT *
  FROM citext_matview m
  FULL JOIN citext_table t ON (t.id = m.id AND t === m)
  WHERE t.id IS NULL OR m.id IS NULL;
                                                     QUERY PLAN                                                      
---------------------------------------------------------------------------------------------------------------------
 Hash Full Join  (cost=1.11..2.24 rows=1 width=16) (actual time=0.056..0.056 rows=0 loops=1)
   Hash Cond: (m.id = t.id)
   Join Filter: (t.* === m.*)
   Filter: ((t.id IS NULL) OR (m.id IS NULL))
   Rows Removed by Filter: 5
   ->  Seq Scan on citext_matview m  (cost=0.00..1.05 rows=5 width=40) (actual time=0.002..0.006 rows=5 loops=1)
   ->  Hash  (cost=1.05..1.05 rows=5 width=40) (actual time=0.023..0.023 rows=5 loops=1)
         Buckets: 1024  Batches: 1  Memory Usage: 1kB
         ->  Seq Scan on citext_table t  (cost=0.00..1.05 rows=5 width=40) (actual time=0.010..0.016 rows=5 loops=1)
 Total runtime: 0.102 ms
(10 rows)

With the operator support, we can remove the primary key columns
from the join conditions, and it still works, albeit with a slower
plan:

test=# explain analyze
SELECT *
  FROM citext_matview m
  FULL JOIN citext_table t ON (t === m)
  WHERE t.id IS NULL OR m.id IS NULL;
                                                      QUERY PLAN                                                       
-----------------------------------------------------------------------------------------------------------------------
 Merge Full Join  (cost=2.22..2.32 rows=1 width=16) (actual time=0.072..0.072 rows=0 loops=1)
   Merge Cond: (m.* === t.*)
   Filter: ((t.id IS NULL) OR (m.id IS NULL))
   Rows Removed by Filter: 5
   ->  Sort  (cost=1.11..1.12 rows=5 width=40) (actual time=0.035..0.035 rows=5 loops=1)
         Sort Key: m.*
         Sort Method: quicksort  Memory: 25kB
         ->  Seq Scan on citext_matview m  (cost=0.00..1.05 rows=5 width=40) (actual time=0.012..0.016 rows=5 loops=1)
   ->  Sort  (cost=1.11..1.12 rows=5 width=40) (actual time=0.014..0.014 rows=5 loops=1)
         Sort Key: t.*
         Sort Method: quicksort  Memory: 25kB
         ->  Seq Scan on citext_table t  (cost=0.00..1.05 rows=5 width=40) (actual time=0.003..0.003 rows=5 loops=1)
 Total runtime: 0.128 ms
(13 rows)

So, if the operators are included it will be a very small and
simple patch to relax the requirement for a unique index.
Currently we generate an error if no index columns were found, but
with the new operators we could leave them out and the query would
still work.  Adding indexes to matviews would then be just a matter
of optimization, not a requirement to be able to use the RMVC
feature.

Using the functions instead of the operators things work just as
well as long as we use columns in the join conditions, which is
currently based on indexed columns:

test=# explain analyze
test-# SELECT *
test-#   FROM citext_matview m
test-#   FULL JOIN citext_table t ON (t.id = m.id AND record_image_eq(t, m))
test-#   WHERE t.id IS NULL OR m.id IS NULL;
                                                     QUERY PLAN                                                      
---------------------------------------------------------------------------------------------------------------------
 Hash Full Join  (cost=1.11..2.24 rows=1 width=16) (actual time=0.056..0.056 rows=0 loops=1)
   Hash Cond: (m.id = t.id)
   Join Filter: record_image_eq(t.*, m.*)
   Filter: ((t.id IS NULL) OR (m.id IS NULL))
   Rows Removed by Filter: 5
   ->  Seq Scan on citext_matview m  (cost=0.00..1.05 rows=5 width=40) (actual time=0.003..0.006 rows=5 loops=1)
   ->  Hash  (cost=1.05..1.05 rows=5 width=40) (actual time=0.023..0.023 rows=5 loops=1)
         Buckets: 1024  Batches: 1  Memory Usage: 1kB
         ->  Seq Scan on citext_table t  (cost=0.00..1.05 rows=5 width=40) (actual time=0.011..0.015 rows=5 loops=1)
 Total runtime: 0.101 ms
(10 rows)

Without columns in the FULL JOIN conditions, the function fails
entirely, because it is the operator information which lets the
planner know that a FULL JOIN is even possible:

test=# explain analyze
test-# SELECT *
test-#   FROM citext_matview m
test-#   FULL JOIN citext_table t ON (record_image_eq(t, m))
test-#   WHERE t.id IS NULL OR m.id IS NULL;
ERROR:  FULL JOIN is only supported with merge-joinable or hash-joinable join conditions

So, either the indexes would continue to be required, or we would
need some other criteria for deciding which columns to use for the
additional FULL JOIN criteria.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi Kevin,

On 2013-09-12 15:27:27 -0700, Kevin Grittner wrote:
> The new operator is logically similar to IS NOT DISTINCT FROM for a
> record, although its implementation is very different.  For one
> thing, it doesn't replace the operation with column level operators
> in the parser.  For another thing, it doesn't look up operators for
> each type, so the "identical" operator does not need to be
> implemented for each type to use it as shown above.  It compares
> values byte-for-byte, after detoasting.  The test for identical
> records can avoid the detoasting altogether for any values with
> different lengths, and it stops when it finds the first column with
> a difference.

In the general case, that operator sounds dangerous to me. We don't
guarantee that a Datum containing the same data always has the same
binary representation. E.g. array can have a null bitmap or may not have
one, depending on how they were created.

I am not actually sure whether that's a problem for your usecase, but I
get headaches when we try circumventing the type abstraction that way.

Yes, we do such tricks in other places already, but afaik in all those
places errorneously believing two Datums are distinct is not error, just
a missed optimization. Allowing a general operator with such a murky
definition to creep into something SQL exposed... Hm. Not sure.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-09-12 15:27:27 -0700, Kevin Grittner wrote:
>> The new operator is logically similar to IS NOT DISTINCT FROM for a
>> record, although its implementation is very different.  For one
>> thing, it doesn't replace the operation with column level operators
>> in the parser.  For another thing, it doesn't look up operators for
>> each type, so the "identical" operator does not need to be
>> implemented for each type to use it as shown above.  It compares
>> values byte-for-byte, after detoasting.  The test for identical
>> records can avoid the detoasting altogether for any values with
>> different lengths, and it stops when it finds the first column with
>> a difference.
>
> In the general case, that operator sounds dangerous to me. We don't
> guarantee that a Datum containing the same data always has the same
> binary representation. E.g. array can have a null bitmap or may not have
> one, depending on how they were created.
>
> I am not actually sure whether that's a problem for your usecase, but I
> get headaches when we try circumventing the type abstraction that way.
>
> Yes, we do such tricks in other places already, but afaik in all those
> places errorneously believing two Datums are distinct is not error, just
> a missed optimization. Allowing a general operator with such a murky
> definition to creep into something SQL exposed... Hm. Not sure.

Well, the only two alternatives I could see were to allow
user-visible differences not to be carried to the matview if they
old and new values were considered "equal", or to implement an
"identical" operator or function in every type that was to be
allowed in a matview.  Given those options, what's in this patch
seemed to me to be the least evil.

It might be worth noting that this scheme doesn't have a problem
with correctness if there are multiple equal values which are not
identical, as long as any two identical values are equal.  If the
query which generates contents for a matview generates
non-identical but equal values from one run to the next without any
particular reason, that might cause performance problems.

To mangle Orwell: "Among pairs of equal values, some pairs are more
equal than others."

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-09-13 14:36:27 -0700, Kevin Grittner wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2013-09-12 15:27:27 -0700, Kevin Grittner wrote:
> >> The new operator is logically similar to IS NOT DISTINCT FROM for a
> >> record, although its implementation is very different.  For one
> >> thing, it doesn't replace the operation with column level operators
> >> in the parser.  For another thing, it doesn't look up operators for
> >> each type, so the "identical" operator does not need to be
> >> implemented for each type to use it as shown above.  It compares
> >> values byte-for-byte, after detoasting.  The test for identical
> >> records can avoid the detoasting altogether for any values with
> >> different lengths, and it stops when it finds the first column with
> >> a difference.
> >
> > In the general case, that operator sounds dangerous to me. We don't
> > guarantee that a Datum containing the same data always has the same
> > binary representation. E.g. array can have a null bitmap or may not have
> > one, depending on how they were created.
> >
> > I am not actually sure whether that's a problem for your usecase, but I
> > get headaches when we try circumventing the type abstraction that way.
> >
> > Yes, we do such tricks in other places already, but afaik in all those
> > places errorneously believing two Datums are distinct is not error, just
> > a missed optimization. Allowing a general operator with such a murky
> > definition to creep into something SQL exposed... Hm. Not sure.
>
> Well, the only two alternatives I could see were to allow
> user-visible differences not to be carried to the matview if they
> old and new values were considered "equal", or to implement an
> "identical" operator or function in every type that was to be
> allowed in a matview.  Given those options, what's in this patch
> seemed to me to be the least evil.
>
> It might be worth noting that this scheme doesn't have a problem
> with correctness if there are multiple equal values which are not
> identical, as long as any two identical values are equal.  If the
> query which generates contents for a matview generates
> non-identical but equal values from one run to the next without any
> particular reason, that might cause performance problems.

I am not actually that concerned with MVCs using this, you're quite
capable of analyzing the dangers. What I am wary of is exposing an
operator that's basically broken from the get go to SQL.
Now, the obvious issue there is that matviews use SQL to refresh :(

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

> I am not actually that concerned with MVCs using this, you're quite
> capable of analyzing the dangers. What I am wary of is exposing an
> operator that's basically broken from the get go to SQL.
> Now, the obvious issue there is that matviews use SQL to refresh :(

I'm not sure why these operators are more broken or dangerous than
those which already exist to support the text_pattern_ops and
bpchar_pattern_ops operator families.  I could overload those
operator names as much as possible if that seems better.  As I said
at the start of the thread, I have no particular attachment to
these operator names.  For example, that would mean using ~>=~ as
the operator for record_image_ge() instead of using >==.  It would
be trivial to make that adjustment to the patch.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-09-13 15:13:20 -0700, Kevin Grittner wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>
> > I am not actually that concerned with MVCs using this, you're quite
> > capable of analyzing the dangers. What I am wary of is exposing an
> > operator that's basically broken from the get go to SQL.
> > Now, the obvious issue there is that matviews use SQL to refresh :(
>
> I'm not sure why these operators are more broken or dangerous than
> those which already exist to support the text_pattern_ops and
> bpchar_pattern_ops operator families.  I could overload those
> operator names as much as possible if that seems better.  As I said
> at the start of the thread, I have no particular attachment to
> these operator names.  For example, that would mean using ~>=~ as
> the operator for record_image_ge() instead of using >==.  It would
> be trivial to make that adjustment to the patch.

Hm. I don't see the similarity. Those have pretty clearly defined
behaviour. Not one that's dependendant on padding bytes, null bitmaps
that can or cannot be present and such.

I guess we need input from others here.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

> Not one that's dependendant on padding bytes, null bitmaps that
> can or cannot be present and such.

Can you provide an example of where that's an issue with this
patch?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-09-13 19:20:11 -0700, Kevin Grittner wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>
> > Not one that's dependendant on padding bytes, null bitmaps that
> > can or cannot be present and such.
>
> Can you provide an example of where that's an issue with this
> patch?

I haven't yet tested your patch, but what I am talking about is that
e.g.: SELECT (ARRAY[1,2,3,NULL])[1:3] = ARRAY[1,2,3];
obviously should be true. But both arrays don't have the same binary
representation since the former has a null bitmap, the latter not.
So, if you had a composite type like (int4[]) and would compare that
without invoking operators you'd return something false in some cases
because of the null bitmaps.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

> what I am talking about is that
> e.g.: SELECT (ARRAY[1,2,3,NULL])[1:3] = ARRAY[1,2,3];
> obviously should be true.

The patch does not change the behavior of the = operator for any
type under any circumstances.

> But both arrays don't have the same binary representation since
> the former has a null bitmap, the latter not. So, if you had a
> composite type like (int4[]) and would compare that without
> invoking operators you'd return something false in some cases
> because of the null bitmaps.

Not for the = operator.  The new "identical" operator would find
them to not be identical, though.

Since the new operator is only for the record type, I need to wrap
the values in your example:

test=# SELECT row ((ARRAY[1,2,3,NULL])[1:3])::record
test-#      = row (ARRAY[1,2,3])::record;
 ?column?
----------
 t
(1 row)

test=# SELECT row ((ARRAY[1,2,3,NULL])[1:3])::record
test-#    === row (ARRAY[1,2,3])::record;
 ?column?
----------
 f
(1 row)

Or, to borrow from the citext example using arrays:

test=# CREATE TABLE array_table (
test(#   id serial primary key,
test(#   nums int4[]
test(# );
CREATE TABLE
test=# INSERT INTO array_table (nums)
test-#   VALUES ((ARRAY[1,2,3,NULL])[1:3]), (ARRAY[1,2,3]),
test-#          (ARRAY[1,2,3]), (NULL), (NULL);
INSERT 0 5
test=# CREATE MATERIALIZED VIEW array_matview AS
test-#   SELECT * FROM array_table;
SELECT 5
test=# CREATE UNIQUE INDEX array_matview_id
test-#   ON array_matview (id);
CREATE INDEX
test=# select * from array_matview;
 id |  nums   
----+---------
  1 | {1,2,3}
  2 | {1,2,3}
  3 | {1,2,3}
  4 |
  5 |
(5 rows)

Note that the on-disk representation of the row where id = 1
differs from the on-disk representation where id in (2,3), both in
the table and the matview.

test=# SELECT *
test-#   FROM array_matview m
test-#   FULL JOIN array_table t ON (t.id = m.id AND t === m)
test-#   WHERE t.id IS NULL OR m.id IS NULL;
 id | nums | id | nums
----+------+----+------
(0 rows)

... so the query looking for work for the RMVC statement finds
nothing to do.

test=# UPDATE array_table SET nums = (ARRAY[1,2,3,NULL])[1:3]
test-#   WHERE id between 1 and 2;
UPDATE 2

Now we have added an unnecessary bitmap to the on-disk storage of
the value where id = 2.

test=# SELECT *
test-#   FROM array_matview m
test-#   FULL JOIN array_table t ON (t.id = m.id AND t === m)
test-#   WHERE t.id IS NULL OR m.id IS NULL;
 id |  nums   | id |  nums   
----+---------+----+---------
    |         |  2 | {1,2,3}
  2 | {1,2,3} |    |
(2 rows)

... and the query sees that they differ.

test=# REFRESH MATERIALIZED VIEW CONCURRENTLY array_matview;
REFRESH MATERIALIZED VIEW
test=# SELECT *
test-#   FROM array_matview m
test-#   FULL JOIN array_table t ON (t.id = m.id AND t === m)
test-#   WHERE t.id IS NULL OR m.id IS NULL;
 id | nums | id | nums
----+------+----+------
(0 rows)

The REFRESH causes them to match again, and later REFRESH runs
won't see a need to do any work there unless the on-disk
representation changes again.

As far as I can see, we have four choices:

(1)  Never update values that are "equal", even if they appear
different to the users, as was demonstrated with the citext
example.

(2)  Require every data type which can be used in a matview to
implement some new operator or function for "identical".  Perhaps
that could be mitigated to only implementat it if equal values can
have user-visible differences.

(3)  Embed special cases into record identical tests for types
known to allow multiple on-disk representations which have no
user-visible differences.

(4)  Base the need to update a matview column on whether its
on-disk representation is identical to what a new run of the
defining query would generate.  If this causes performance problems
for use of a given type in a matview, one possible solution would
be to modify that particular type to use a canonical format when
storing a value into a record.  For example, storing an array which
has a bitmap of null values even though there are no nulls in the
array could strip the bitmap as it is stored to the record.

Currently we are using (4).  I only included (3) for completeness;
even just typing it as a hypothetical made me want to take a
shower.  (1) seems pretty horrid, too.  (2) isn't evil, exactly,
but any types which allowed user-visible differences in equal
values would not exhibit correct behavior in matviews until and
unless an "identical" operator was added.  It might also perform
noticeably worse than (4).

Option (4) has the advantage of showing correct logical behavior in
all types immediately, and restricting any performance fixes to the
types with the inconsistent storage formats.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-09-14 11:25:52 -0700, Kevin Grittner wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>
> > what I am talking about is that
> > e.g.: SELECT (ARRAY[1,2,3,NULL])[1:3] = ARRAY[1,2,3];
> > obviously should be true.
>
> The patch does not change the behavior of the = operator for any
> type under any circumstances.

Yes, sure. I wasn't thinking you would.

> > But both arrays don't have the same binary representation since
> > the former has a null bitmap, the latter not. So, if you had a
> > composite type like (int4[]) and would compare that without
> > invoking operators you'd return something false in some cases
> > because of the null bitmaps.
>
> Not for the = operator.  The new "identical" operator would find
> them to not be identical, though.

Yep. And I think that's a problem if exposed to SQL. People won't
understand the hazards and end up using it because its faster or
somesuch.

> Since the new operator is only for the record type, I need to wrap
> the values in your example:

Yes.

> The REFRESH causes them to match again, and later REFRESH runs
> won't see a need to do any work there unless the on-disk
> representation changes again.

Yes, I understand that the matview code itself will just perform
superflous work. We use such comparisons in other parts of the code
similarly.

> As far as I can see, we have four choices:
>
> (1)  Never update values that are "equal", even if they appear
> different to the users, as was demonstrated with the citext
> example.

I think, introducing a noticeable amount of infrastructure for this just
because of citext is a bad idea.
At some point we need to replace citext with proper case-insensitive
collation support - then it really might become necessary.

> (2)  Require every data type which can be used in a matview to
> implement some new operator or function for "identical".  Perhaps
> that could be mitigated to only implementat it if equal values can
> have user-visible differences.

That basically would require adding a new member to btree opclasses that
btrees don't need themselves... Hm.

> (3)  Embed special cases into record identical tests for types
> known to allow multiple on-disk representations which have no
> user-visible differences.

I think this is a complete nogo. a) I don't forsee we know of all these
cases b) it wouldn't be extensible.

Oh. Now that I've read further, I see you feel the same. Good ;)

> (4)  Base the need to update a matview column on whether its
> on-disk representation is identical to what a new run of the
> defining query would generate.  If this causes performance problems
> for use of a given type in a matview, one possible solution would
> be to modify that particular type to use a canonical format when
> storing a value into a record.  For example, storing an array which
> has a bitmap of null values even though there are no nulls in the
> array could strip the bitmap as it is stored to the record.

If matview refreshs weren't using plain SQL and thus wouldn't require
exposing that operator to SQL I wouldn't have a problem with this...

There's the ungodly ugly choice of having an matview_equal function (or
operator) that checks if we're doing a refresh atm...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

> If matview refreshs weren't using plain SQL and thus wouldn't
> require exposing that operator to SQL I wouldn't have a problem
> with this...

If RMVC were the end of the story, it might be worth building up a
mass of execution nodes directly, although it would be hard to see
how we could make the right planning choices (e.g., MergeJoin
versus HashJoin) that way.  But the whole incremental maintenance
area, to have any chance of working accurately and without an
endless stream of bugs, needs to be based on relational algebra.
There needs to be a way to express that in a much higher level
language than execution node creation.  If it doesn't use SQL we
would need to invent a relational language very much like it, which
would be silly when we have a perfectly good language we can
already use.  The sky is blue; let's move on.

The test for identical records will be needed in SQL if we want to
have these matview features.  We could limit use of that to
contexts where MatViewIncrementalMaintenanceIsEnabled(), but I
don't see the point.  If someone uses an undocumented operator, and
uses it inappropriately, they may get a surprising result.  We
already have undocumented operators to support record comparison
and pattern opclasses, and if people use those they may get
surprising results.  I don't recall any reports of problems from
people actually trying to do so.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Sep 14, 2013 at 08:58:32PM +0200, Andres Freund wrote:
> On 2013-09-14 11:25:52 -0700, Kevin Grittner wrote:
> > Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > > But both arrays don't have the same binary representation since
> > > the former has a null bitmap, the latter not. So, if you had a
> > > composite type like (int4[]) and would compare that without
> > > invoking operators you'd return something false in some cases
> > > because of the null bitmaps.
> >
> > Not for the = operator. The new "identical" operator would find
> > them to not be identical, though.
>
> Yep. And I think that's a problem if exposed to SQL. People won't
> understand the hazards and end up using it because its faster or
> somesuch.

The important question is whether to document the new operator and/or provide
it under a guessable name. If we give the operator a weird name, don't
document it, and put an "internal use only" comment in the catalogs, that is
essentially as good as hiding this feature at the SQL level.

I'm of two minds on that question. On the one hand, MV maintenance is hardly
the first use case for an identity operator. Any replication system or user
space materialized view implementation might want this. On the other hand,
offering it for the record type exclusively is surprising. It's also
surprising how records with different numbers of dropped columns can be found
identical, even though a record column within the top-level record is not
permitted to vary that way.

Supposing a decision to document the operator, a second question is whether
"===" is the right name:

On Thu, Sep 12, 2013 at 03:27:27PM -0700, Kevin Grittner wrote:
> The identical (===) and not identical (!==) operator names were
> chosen because of a vague similarity to the "exactly equals"
> concepts in JavaScript and PHP, which use that name.  The semantics
> aren't quite the same, but it seemed close enough not to be too
> surprising.

Maybe. If we were mimicking the JavaScript/PHP operator of the same name,
'1.0'::numeric === '1.00'::numeric would return true, and '1'::int4 ===
'1'::int2 would return false. The patch submitted returns false for the first
example and raises a "cannot compare dissimilar column types" error when
comparing an int4 record column to an int2 record column.

> I think, introducing a noticeable amount of infrastructure for this just
> because of citext is a bad idea.
> At some point we need to replace citext with proper case-insensitive
> collation support - then it really might become necessary.

citext is just one example. Others include '1.0'::numeric = '1.00'::numeric
and '30 day'::interval = '1 mon'::interval.

> > (2) Require every data type which can be used in a matview to
> > implement some new operator or function for "identical". Perhaps
> > that could be mitigated to only implementat it if equal values can
> > have user-visible differences.
>
> That basically would require adding a new member to btree opclasses that
> btrees don't need themselves... Hm.

That's the wrong way to model it. Identity is a particular kind of equality,
which is to say a particular hash opclass. It so happens that memcmp() is the
logical way to implement most/all identity operators, so we can get a btree
opclass as well.

Type-specific identity operators seem like overkill, anyway. If we find that
meaningless variations in a particular data type are causing too many false
non-matches for the generic identity operator, the answer is to make the
functions generating datums of that type settle on a canonical form. That
would be the solution for your example involving array null bitmaps.

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

On 09/15/2013 01:35 PM, Kevin Grittner wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>
>> If matview refreshs weren't using plain SQL and thus wouldn't
>> require exposing that operator to SQL I wouldn't have a problem
>> with this...
> If RMVC were the end of the story, it might be worth building up a
> mass of execution nodes directly, although it would be hard to see
> how we could make the right planning choices (e.g., MergeJoin
> versus HashJoin) that way. But the whole incremental maintenance
> area, to have any chance of working accurately and without an
> endless stream of bugs, needs to be based on relational algebra.
> There needs to be a way to express that in a much higher level
> language than execution node creation. If it doesn't use SQL we
> would need to invent a relational language very much like it, which
> would be silly when we have a perfectly good language we can
> already use. The sky is blue; let's move on.
>
> The test for identical records will be needed in SQL if we want to
> have these matview features. We could limit use of that to
> contexts where MatViewIncrementalMaintenanceIsEnabled(), but I
> don't see the point. If someone uses an undocumented operator, and
> uses it inappropriately, they may get a surprising result.
Just remember to document it as "undocumented" so
people will know not to use them ;)

Lots of people were bitten when (undocumented) hash
functions were changed thus breaking hash-based partitioning.

Cheers

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

Hannu Krosing <hannu(at)2ndQuadrant(dot)com> wrote:

> Lots of people were bitten when (undocumented) hash
> functions were changed thus breaking hash-based partitioning.

Nobody can be affected by the new operators in this patch unless
they choose to use them to compare two records.  They don't work
for any other type and they don't come into play unless you
specifically request them.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-09-15 19:49:26 -0400, Noah Misch wrote:
> On Sat, Sep 14, 2013 at 08:58:32PM +0200, Andres Freund wrote:
> > On 2013-09-14 11:25:52 -0700, Kevin Grittner wrote:
> > > Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > > > But both arrays don't have the same binary representation since
> > > > the former has a null bitmap, the latter not. So, if you had a
> > > > composite type like (int4[]) and would compare that without
> > > > invoking operators you'd return something false in some cases
> > > > because of the null bitmaps.
> > >
> > > Not for the = operator. The new "identical" operator would find
> > > them to not be identical, though.
> >
> > Yep. And I think that's a problem if exposed to SQL. People won't
> > understand the hazards and end up using it because its faster or
> > somesuch.
>
> The important question is whether to document the new operator and/or provide
> it under a guessable name. If we give the operator a weird name, don't
> document it, and put an "internal use only" comment in the catalogs, that is
> essentially as good as hiding this feature at the SQL level.

Doesn't match my experience.

> Type-specific identity operators seem like overkill, anyway. If we find that
> meaningless variations in a particular data type are causing too many false
> non-matches for the generic identity operator, the answer is to make the
> functions generating datums of that type settle on a canonical form. That
> would be the solution for your example involving array null bitmaps.

I think that's pretty much unrealistic. I am pretty sure that if either
of us starts looking we will find at about a dozen of such cases and
miss the other dozen. Not to speak about external code which is damn
likely to contain such cases.
And I think that efficiency will often make such normalization expensive
(consider postgis where Datums afaik can exist with an internal bounding
box or without).

I think it's far more realistic to implement an identity operator that
will fall back to a type specific operator iff equals has "strange"
properties.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

> I think it's far more realistic to implement an identity operator
> that will fall back to a type specific operator iff equals has
> "strange" properties.

My biggest problem with that approach is that it defaults to
incorrect behavior for types for which user-visible differences
among "equal" values are possible, and requires per-type fixes to
get correct behavior.  The approach in this patch provides correct
behavior as the default, with possible per-type changes for
optimization, as people find that desirable.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sun, Sep 15, 2013 at 6:49 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Sat, Sep 14, 2013 at 08:58:32PM +0200, Andres Freund wrote:
>> On 2013-09-14 11:25:52 -0700, Kevin Grittner wrote:
>> > Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > > But both arrays don't have the same binary representation since
>> > > the former has a null bitmap, the latter not. So, if you had a
>> > > composite type like (int4[]) and would compare that without
>> > > invoking operators you'd return something false in some cases
>> > > because of the null bitmaps.
>> >
>> > Not for the = operator. The new "identical" operator would find
>> > them to not be identical, though.
>>
>> Yep. And I think that's a problem if exposed to SQL. People won't
>> understand the hazards and end up using it because its faster or
>> somesuch.
>
> The important question is whether to document the new operator and/or provide
> it under a guessable name. If we give the operator a weird name, don't
> document it, and put an "internal use only" comment in the catalogs, that is
> essentially as good as hiding this feature at the SQL level.
>
> I'm of two minds on that question. On the one hand, MV maintenance is hardly
> the first use case for an identity operator. Any replication system or user
> space materialized view implementation might want this. On the other hand,
> offering it for the record type exclusively is surprising. It's also
> surprising how records with different numbers of dropped columns can be found
> identical, even though a record column within the top-level record is not
> permitted to vary that way.
>
> Supposing a decision to document the operator, a second question is whether
> "===" is the right name:

I vote to reserve '===' as shorthand for 'IS NOT DISTINCT FROM' and
give the binary equality operator a funky name. I would document the
operator though.

merlin

On 09/16/2013 04:01 PM, Kevin Grittner wrote:
> Hannu Krosing <hannu(at)2ndQuadrant(dot)com> wrote:
>
>> Lots of people were bitten when (undocumented) hash
>> functions were changed thus breaking hash-based partitioning.
> Nobody can be affected by the new operators in this patch unless
> they choose to use them to compare two records. They don't work
> for any other type and they don't come into play unless you
> specifically request them.
What I meant is that rather than leave it really undocumented,
document it as "system function for specific usage, has caveats
and may change in future versions. use at your own risk and
make sure you know what you are doing"

PostgreSQL has good enough introspection features that people
tend to find functions and operators using psql-s \d ...
>
> --
> Kevin Grittner
> EDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

Hannu Krosing <hannu(at)2ndQuadrant(dot)com> wrote:

> What I meant is that rather than leave it really undocumented,
> document it as "system function for specific usage, has caveats
> and may change in future versions. use at your own risk and
> make sure you know what you are doing"

Well, that was my original assumption and intention; but when I
went to look for where the operators for record *equals* were
defined, I found that we had apparently chosen to leave them
undocumented.  Oddly, under a section titled "Row-wise Comparison"
we only document the behavior of comparisons involving what the SQL
spec calls <row value constructor>.   I asked whether that was
intentional, and heard only the chirping of crickets:

http://www.postgresql.org/message-id/1378848776.70700.YahooMailNeo@web162902.mail.bf1.yahoo.com

If we choose not to document the equals operator for records, it
hardly makes sense to document the identical operator for records.

> PostgreSQL has good enough introspection features that people
> tend to find functions and operators using psql-s \d ...

One would think so, yet I don't recall seeing anyone posting
regarding the existing undocumented record comparison operators.
Nor do I recall seeing anyone posting about the undocumented
pattern comparison operators.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-09-16 10:46:53 -0700, Kevin Grittner wrote:
> One would think so, yet I don't recall seeing anyone posting
> regarding the existing undocumented record comparison operators.
> Nor do I recall seeing anyone posting about the undocumented
> pattern comparison operators.

I've used and have seen them being used in client code...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-09-16 10:46:53 -0700, Kevin Grittner wrote:
>> I don't recall seeing anyone posting
>> regarding the existing undocumented record comparison operators.
>> Nor do I recall seeing anyone posting about the undocumented
>> pattern comparison operators.
>
> I've used and have seen them being used in client code...

Which, the record operators or the text pattern operators (which
ignore collations and multi-byte character considerations and just
use memcmp())?

http://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/utils/adt/varlena.c;h=5e2c2ddc532c604a05f365f0cf6761033a35be76;hb=master#l1719

Is the fact that you have seen them used in client code even though
they are not documented an argument for or against adding
documentation for them?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-09-16 10:58:01 -0700, Kevin Grittner wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2013-09-16 10:46:53 -0700, Kevin Grittner wrote:
> >> I don't recall seeing anyone posting
> >> regarding the existing undocumented record comparison operators.
> >> Nor do I recall seeing anyone posting about the undocumented
> >> pattern comparison operators.
> >
> > I've used and have seen them being used in client code...
>
> Which, the record operators or the text pattern operators (which
> ignore collations and multi-byte character considerations and just
> use memcmp())?

I was referring to the text pattern ops. But I've seen the record ones
as well.

> http://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/utils/adt/varlena.c;h=5e2c2ddc532c604a05f365f0cf6761033a35be76;hb=master#l1719
>
> Is the fact that you have seen them used in client code even though
> they are not documented an argument for or against adding
> documentation for them?

Neither. It's an argument that providing operators with confusing
behaviours will not go unnoticed/unused.

I don't really think the record identical and pattern ops situations are
comparable. The latter has a well defined behaviour (using the C locale
basically) and is only useable for types where it's well defined. The
proposed identical operator returns false for comparisons of actually
equal data, that's quite different imo.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Sep 16, 2013 at 12:46 PM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
> Hannu Krosing <hannu(at)2ndQuadrant(dot)com> wrote:
>
>> What I meant is that rather than leave it really undocumented,
>> document it as "system function for specific usage, has caveats
>> and may change in future versions. use at your own risk and
>> make sure you know what you are doing"
>
> Well, that was my original assumption and intention; but when I
> went to look for where the operators for record *equals* were
> defined, I found that we had apparently chosen to leave them
> undocumented. Oddly, under a section titled "Row-wise Comparison"
> we only document the behavior of comparisons involving what the SQL
> spec calls <row value constructor>. I asked whether that was
> intentional, and heard only the chirping of crickets:
>
> http://www.postgresql.org/message-id/1378848776.70700.YahooMailNeo@web162902.mail.bf1.yahoo.com
>
> If we choose not to document the equals operator for records, it
> hardly makes sense to document the identical operator for records.
>
>> PostgreSQL has good enough introspection features that people
>> tend to find functions and operators using psql-s \d ...
>
> One would think so, yet I don't recall seeing anyone posting
> regarding the existing undocumented record comparison operators.
> Nor do I recall seeing anyone posting about the undocumented
> pattern comparison operators.

This behavior came about via a gripe of mine (but mostly courtesy Tom
Lane_: http://www.postgresql.org/message-id/6EE64EF3AB31D5448D0007DD34EEB34101AEF5@Herge.rcsinc.local

It brought row-wise comparon closer- (if not exactly to-) SQL spec.
The underlying use-case is to do ISAM-like record iteration over the
index. "index" being the operative word.

merlin

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-09-16 10:58:01 -0700, Kevin Grittner wrote:
>> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>> On 2013-09-16 10:46:53 -0700, Kevin Grittner wrote:
>>>> I don't recall seeing anyone posting regarding the existing
>>>> undocumented record comparison operators.  Nor do I recall
>>>> seeing anyone posting about the undocumented pattern
>>>> comparison operators.
>>>
>>> I've used and have seen them being used in client code...
>>
>> Which, the record operators or the text pattern operators (which
>> ignore collations and multi-byte character considerations and
>> just use memcmp())?
>
> I was referring to the text pattern ops. But I've seen the record
> ones as well.

>> Is the fact that you have seen them used in client code even
>> though they are not documented an argument for or against adding
>> documentation for them?
>
> Neither. It's an argument that providing operators with confusing
> behaviours will not go unnoticed/unused.

It is true that there are likely to be some who find the new
operators useful and will read the code to determine how to use
them, just as they apparently did for the undocumented operators
which already exist.

> I don't really think the record identical and pattern ops
> situations are comparable. The latter has a well defined
> behaviour (using the C locale basically) and is only useable for
> types where it's well defined.

But it is *not* restricted to text in the C locale.  It uses
memcmp() on text values even if they contain multi-byte characters
and use collations which require different sequencing.

> The proposed identical operator returns false for comparisons of
> actually equal data,

That is the point of it.  It tells you whether the record images
are byte-for-byte identical for all values within the record.  In
some cases a detected difference clearly causes a user-visible
difference; in others it is merely a performance or storage-space
difference.  For example in your array example the array with the
unnecessary bitmap attached to it takes an extra eight bytes on
disk, for no apparent benefit.  The point is that code which wants
to know whether those are identical images can detect that they are
not.  This works by default for all types, without having to track
down which types are capable of doing this for "equal" values or in
which cases the differences are user-visible.

> that's quite different imo.

Sorting a string based on its byte image, with no regard to its
collation or character boundaries, is what these text pattern
operators do, and it is exactly what my patch does.  My patch just
provides such a test for all data types within a record by default,
rather than requiring a new opfamily for every type.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:

>> I don't recall seeing anyone posting regarding the existing
>> undocumented record comparison operators.

> This behavior came about via a gripe of mine (but mostly courtesy
> Tom Lane_:
> http://www.postgresql.org/message-id/6EE64EF3AB31D5448D0007DD34EEB34101AEF5@Herge.rcsinc.local
>
> It brought row-wise comparon closer- (if not exactly to-) SQL
> spec.  The underlying use-case is to do ISAM-like record
> iteration over the index.  "index" being the operative word.

Indeed.  The documented behavior complies with the spec (and is
extremely useful!), but only fully matches the behavior when one of
the arguments is a row value constructor.  The documentation sort
of acknowledges this by describing the arguments not as "row" or
"record", but as "row_constructor".  Comparison operators which
followed those exact rules would not be usable for indexing, so we
have slightly different semantics to support that.  I have no gripe
whatsoever with any of that.  I'm even OK with not documenting this
behavior, since it might do more to confuse than elucidate.  I did
ask the question, because if we choose not to document the behavior
of record comparisons based around the concept of "record equals",
I don't think it makes sense to document record comparisons based
around the concept of "record image equals".  That is the
terminology used in the function names and opfamily, but I have
been using "identical" in discussion, which perhaps has confused
the issue.

If we take documentation of record comparisons farther, we need to
be prepared to explain why this is correct (because it is required
by spec):

test=# select row(1, null::text) = row(1, null::text);
 ?column?
----------
 
(1 row)

... but this is also correct (because otherwise indexes don't
work):

test=# select row(1, null::text) = row(1, null::text)::record;
 ?column?
----------
 t
(1 row)

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Sep 16, 2013 at 04:28:23PM +0200, Andres Freund wrote:
> On 2013-09-15 19:49:26 -0400, Noah Misch wrote:
> > Type-specific identity operators seem like overkill, anyway. If we find that
> > meaningless variations in a particular data type are causing too many false
> > non-matches for the generic identity operator, the answer is to make the
> > functions generating datums of that type settle on a canonical form. That
> > would be the solution for your example involving array null bitmaps.
>
> I think that's pretty much unrealistic. I am pretty sure that if either
> of us starts looking we will find at about a dozen of such cases and
> miss the other dozen. Not to speak about external code which is damn
> likely to contain such cases.

It wouldn't be a problem if we missed cases or declined to update known cases.
The array example probably isn't worth changing. Who's going to repeatedly
flip-flop an array column value between the two representations and then
bemoan the resulting MV write traffic?

> And I think that efficiency will often make such normalization expensive
> (consider postgis where Datums afaik can exist with an internal bounding
> box or without).

If it's so difficult to canonicalize between two supposedly-identical
variations, it's likewise a stretch to trust that all credible operations will
fail to distinguish between those variations.

> I think it's far more realistic to implement an identity operator that
> will fall back to a type specific operator iff equals has "strange"
> properties.

Complicating such efforts, the author of a custom identity operator doesn't
have the last word on functions that process the data type. Take your postgis
example; if a second party adds a has_internal_bounding_box() function, an
identity operator ignoring that facet is no longer valid.

memcmp() has served well for HOT and for _equalConst(); why would it suddenly
fall short for MV maintenance?

nm

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

On 2013-09-16 16:58:21 -0400, Noah Misch wrote:
> memcmp() has served well for HOT and for _equalConst(); why would it suddenly
> fall short for MV maintenance?

I don't have a problem using it internally, I have a problem exposing
the capability to sql. Don't tell me that's the same.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-09-16 16:58:21 -0400, Noah Misch wrote:
>> memcmp() has served well for HOT and for _equalConst(); why
>> would it suddenly fall short for MV maintenance?
>
> I don't have a problem using it internally, I have a problem
> exposing the capability to sql.

... like we do in *pattern ops and the
suppress_redundant_updates_trigger() function?

> Don't tell me that's the same.

No, this gives users a way to make the same test that HOT uses for
whether values match, albeit undocumented.  Well, not exactly the
same test, because this patch detoasts before comparing -- but
pretty close.  The question is, if it's unsafe for a user to make
this test, why would it be safe for HOT to use it?

I'm really having trouble understanding what problem you have with
this.  Can you describe a scenario where someone shoots themselves
in the foot with it, because I'm not seeing any?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-09-16 14:39:30 -0700, Kevin Grittner wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2013-09-16 16:58:21 -0400, Noah Misch wrote:
> >> memcmp() has served well for HOT and for _equalConst(); why
> >> would it suddenly fall short for MV maintenance?
> >
> > I don't have a problem using it internally, I have a problem
> > exposing the capability to sql.
>
> ... like we do in *pattern ops and the

That's still an absurd comparison. pattern ops have a defined behaviour,
even for multibyte characters. After all, you can simply have a UTF-8
database with C collation. Remember, in a C collation pattern ops can
use normal indexes.
The only thing that happens is that multibyte chars are sorted
differently. They aren't sorted basically randomly or anything.

> suppress_redundant_updates_trigger() function?

You get superflous trigger calls. So what. It's not usable for anything
but a trigger.

> I'm really having trouble understanding what problem you have with
> this.  Can you describe a scenario where someone shoots themselves
> in the foot with it, because I'm not seeing any?

It certainly will be surprising to just about anyone that something like:

SELECT * FROM foo WHERE whatever_composite_or_row === '(...)';

may not return rows where there's no SQL level discernible difference
(even by looking at the text conversion) between
whatever_composite_or_row and '(...)' because e.g. of the aforementioned
array null bitmaps.

I can understand claiming that the risk is acceptable but arguing it's
not there seems extremly strange to me.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2013-09-16 23:58:46 +0200, Andres Freund wrote:
> > suppress_redundant_updates_trigger() function?
>
> You get superflous trigger calls. So what. It's not usable for anything
> but a trigger.

Primarily unneccesary IO, not unneccessary trigger calls (which can also
happen).

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-09-16 14:39:30 -0700, Kevin Grittner wrote:
>> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>> On 2013-09-16 16:58:21 -0400, Noah Misch wrote:
>>>> memcmp() has served well for HOT and for _equalConst(); why
>>>> would it suddenly fall short for MV maintenance?
>>>
>>> I don't have a problem using it internally, I have a problem
>>> exposing the capability to sql.
>>
>> ... like we do in *pattern ops and the
>
> That's still an absurd comparison. pattern ops have a defined
> behaviour, even for multibyte characters. After all, you can
> simply have a UTF-8 database with C collation. Remember, in a C
> collation pattern ops can use normal indexes.
> The only thing that happens is that multibyte chars are sorted
> differently. They aren't sorted basically randomly or anything.

Neither are records using the new operator -- there is a
deterministic sort order based on the bytes representing the
record's values.

>> suppress_redundant_updates_trigger() function?
>
> You get superflous trigger calls. So what.

My feeling exactly.

>> I'm really having trouble understanding what problem you have
>> with this.  Can you describe a scenario where someone shoots
>> themselves in the foot with it, because I'm not seeing any?
>
> It certainly will be surprising to just about anyone that
> something like:
>
> SELECT * FROM foo WHERE whatever_composite_or_row === '(...)';
>
> may not return rows where there's no SQL level discernible
> difference (even by looking at the text conversion) between
> whatever_composite_or_row and '(...)' because e.g. of the
> aforementioned array null bitmaps.

Since the operator is bound to a function named record_image_eq(),
it seems to me to be a feature that if the image isn't equal due to
a bitmap being present in one and not the other it says they
differ.  It's not a bug or a problem.  It gives you a way to *tell*
whether two rows actually have the same representation, which could
be valuable for debugging.  The closest you can easily come without
this is to see that pg_column_size() yields a different result for
them, where that is true (as in the array bitmap case).

> I can understand claiming that the risk is acceptable but arguing
> it's not there seems extremly strange to me.

It's not a risk.  It's why the operator exists.  Perhaps the name
of the operator (===) or the fact that I've been using the
shorthand description of "identical" instead of writing out "record
image equals" in these emails has confused you.  I can stop using
the shorter description and have absolutely no attachment to the
operator name, if that helps.

The point of the operator is to determine whether two records,
which may compare as "equal" (but which don't necessarily appear
the same to a user), have the same byte image for each
corresponding value.  The point of the opfamily is to be able to do
a MergeJoin on this basis.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-09-16 15:26:08 -0700, Kevin Grittner wrote:
> > I can understand claiming that the risk is acceptable but arguing
> > it's not there seems extremly strange to me.
>
> It's not a risk.  It's why the operator exists. 

Pft. It's fine if the materialized view code uses it. I don't see the
danger there.
It's about users discovering it. If they notice it, they will use it
because "its a crapload faster" than normal row comparisons. And deals
with NULLs in a "simpler" way.

> Perhaps the name
> of the operator (===) or the fact that I've been using the
> shorthand description of "identical" instead of writing out "record
> image equals" in these emails has confused you.

If you really think that "confusing you" is the correct description for
concerns about users not understanding limitations (which you didn't
seem to know about), go ahead. Way to go to solicit feedback.

> I can stop using
> the shorter description and have absolutely no attachment to the
> operator name, if that helps.

You're unfortunately going to need operators if you want mergejoins to
work, right? If not I'd have said name it matview_needs_update() or
something like that... But yes, === certainly seems like a bad idea.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 09/16/2013 04:01 PM, Kevin Grittner wrote:
> Hannu Krosing <hannu(at)2ndQuadrant(dot)com> wrote:
>
>> Lots of people were bitten when (undocumented) hash
>> functions were changed thus breaking hash-based partitioning.
> Nobody can be affected by the new operators in this patch unless
> they choose to use them to compare two records. They don't work
> for any other type and they don't come into play unless you
> specifically request them.
>
Maybe the binary equality operator should be named ====
for "really deeply equal"

to distinguish it from === which would be merely NOT DISTINCT FROM

we could even go one step further and define ===== to mean "the same".

?

This could fit better in conceptual sequence of operator 'strength'

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-09-16 15:26:08 -0700, Kevin Grittner wrote:
>>> I can understand claiming that the risk is acceptable but
>>> arguing it's not there seems extremly strange to me.
>>
>> It's not a risk.  It's why the operator exists.
>
> Pft. It's fine if the materialized view code uses it. I don't see
> the danger there.
> It's about users discovering it. If they notice it, they will use
> it because "its a crapload faster" than normal row comparisons.

To have clean semantics, I think the operator should mean that the
stored format of the row is the same.  Regarding the array null
bitmap example, I think it would be truly weird if the operator
said that the stored format was the same, but this happened:

test=# select pg_column_size(ARRAY[1,2,3]);
 pg_column_size
----------------
             36
(1 row)

test=# select pg_column_size((ARRAY[1,2,3,NULL])::int4[3]);
 pg_column_size
----------------
             44
(1 row)

They have the same stored format, but a different number of
bytes?!?

> And deals with NULLs in a "simpler" way.

That might be addressed by leaving room to implement IS NOT
DISTINCT FROM as an operator.  See below.

>> Perhaps the name of the operator (===) or the fact that I've
>> been using the shorthand description of "identical" instead of
>> writing out "record image equals" in these emails has confused
>> you.
>
> If you really think that "confusing you" is the correct
> description for concerns about users not understanding
> limitations (which you didn't seem to know about), go ahead. Way
> to go to solicit feedback.

I see how that could be taken in a pejorative way; that was not
intended.  I apologize.  I was rushing a bit on that email to make
an appointment shortly afterward.  The point was to suggest that
bad communication on my part about the intent and concept of the
operator may have contributed to you saying that there was a risk
of it working as intended.  There is perhaps a risk that someone
will think that it does something other than what is intended, but
when you say that there is a "risk" that it will behave exactly as
intended, it does suggest that we're talking past each other.

>> I can stop using the shorter description and have absolutely no
>> attachment to the operator name, if that helps.
>
> You're unfortunately going to need operators if you want
> mergejoins to work, right?

Yes, operators are needed for that.

> If not I'd have said name it matview_needs_update() or something
> like that... But yes, === certainly seems like a bad idea.

I've come to agree with that.  The appointment was to meet with a
local PostgreSQL user who I've talked to before.  He reminded me
that his pet peeve was that he couldn't use IS [ NOT ] DISTINCT
FROM with the ALL | ANY construct because the IS [ NOT ] DISTINCT
FROM predicate isn't an operator.

Hannu Krosing <hannu(at)2ndQuadrant(dot)com> wrote:

> Maybe the binary equality operator should be named ====
> for "really deeply equal"
>
> to distinguish it from === which would be merely NOT DISTINCT FROM
>
> we could even go one step further and define ===== to mean "the same".
>
> ?
>
> This could fit better in conceptual sequence of operator 'strength'

I'm inclined to go with this.  It would leave the door open to
implementing IS NOT DISTINCT FROM on a type-by-type basis.  My one
concern is that it doesn't make room for a "has no user-visible
differences from" operator; but I'm not sure whether that is
something that there really is any use for.  But if we want to
reserve name space for it "just in case" someone has a use for it,
that would be between IS NOT DISTINCT FROM and "has the same
storage representation as", so that would leave:

  =      equals (but doesn't necessarily look the same as)
  ===    IS NOT DISTINCT FROM as an operator
  ====   reserved for "has no user-visible differences from"
  =====  stored image is the same

Of course, that begs the question of whether == is already "taken".
If not, we could knock one '=' off of everything above except for
"equals".  What existing uses are known for == ?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Sep 17, 2013 at 8:23 AM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:

>
> Of course, that begs the question of whether == is already "taken".
> If not, we could knock one '=' off of everything above except for
> "equals". What existing uses are known for == ?
>

== is already taken as a common typo in plpgsql scripts. I strongly prefer
if this remained an error.

IF foo == bar THEN ...

On 09/17/2013 02:46 PM, Rod Taylor wrote:
> On Tue, Sep 17, 2013 at 8:23 AM, Kevin Grittner <kgrittn(at)ymail(dot)com
> <mailto:kgrittn(at)ymail(dot)com>> wrote:
>
>
> Of course, that begs the question of whether == is already "taken".
> If not, we could knock one '=' off of everything above except for
> "equals". What existing uses are known for == ?
>
>
> == is already taken as a common typo in plpgsql scripts. I strongly
> prefer if this remained an error.
>
> IF foo == bar THEN ...
That was also my reason for not suggesting == .
It is too widely used in other systems for simple equality check.

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

On 09/17/2013 12:52 AM, Andres Freund wrote:
> On 2013-09-16 15:26:08 -0700, Kevin Grittner wrote:
>>> I can understand claiming that the risk is acceptable but arguing
>>> it's not there seems extremly strange to me.
>> It's not a risk. It's why the operator exists.
> Pft. It's fine if the materialized view code uses it. I don't see the
> danger there.
> It's about users discovering it. If they notice it, they will use it
> because "its a crapload faster" than normal row comparisons. And deals
> with NULLs in a "simpler" way.
This is why I suggested naming the operator ==== with four '='s
as this should make the user ponder why there are so many equal signs.
And there are other languages where more equal signs mean "stronger"
equality.

Basically it is "equality with possible false negatives".

If a user wants to use it for speeding up simple equality, it should be
used as
"WHERE t.* ==== sample AND t.* = sample"

>
>> Perhaps the name
>> of the operator (===) or the fact that I've been using the
>> shorthand description of "identical" instead of writing out "record
>> image equals" in these emails has confused you.
> If you really think that "confusing you" is the correct description for
> concerns about users not understanding limitations (which you didn't
> seem to know about),
IIRC he did start with a visible limitation of = being too weak equality
for some fields
> go ahead. Way to go to solicit feedback.
>
>> I can stop using
>> the shorter description and have absolutely no attachment to the
>> operator name, if that helps.
> You're unfortunately going to need operators if you want mergejoins to
> work, right? If not I'd have said name it matview_needs_update() or
> something like that... But yes, === certainly seems like a bad idea.
but having === as an operator form of IS NOT DISTINCT FROM
seems like a good idea to me.

We will get an operator which most people expect, with sufficiently
strange name to make people look up the docs and we will not be
violating SQL spec.

And it prepares their heads for stronger semantics of ==== :)
>
> Greetings,
>
> Andres Freund
>

Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
>   =      equals (but doesn't necessarily look the same as)
>   ===    IS NOT DISTINCT FROM as an operator
>   ====   reserved for "has no user-visible differences from"
>   =====  stored image is the same

I understand the need for more than one equality operator and my
programming language of choice exposes eq, eql, equal and equalp for
solving a very similar situation. Oh, and also offers = and string= and
string-equal, among others.

My vote goes against using a different number of the same character to
name the operators though, as that's not visually helpful enough IMO.

In yet another language whose grammar took inspiration with Prolog,
dealing with binary is made with explicitely using the bit syntax
expression where any datum is boxed into << and >>.

http://erlang.org/doc/reference_manual/expressions.html#id78513

It might not be practical to do the same in SQL and have either the
following syntax example or even a unary "bit representation" operator
to do the same:

SELECT <<row(expression, here)>> = <<column_name>> …

That would probably prevent using indexes?

It might be inspiration to name the bit-comparison operator something
like <<=>> though, or =><= or something else. If only we could force the
usage of unicode for the SQL input itself… I'm sure there's a unicode
glyph perfect for what we want here…

About IS NOT DISTINCT FROM, I would propose something where NOT and
DISTINCT are kept as concepts. Not usually is expressed with ! and we
already have a bunch of "same as" or "matches" operators with ~, so:

<~> IS DISTINCT FROM
!<~> IS NOT DISTINCT FROM

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Tue, Sep 17, 2013 at 8:23 AM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
> To have clean semantics, I think the operator should mean that the
> stored format of the row is the same. Regarding the array null
> bitmap example, I think it would be truly weird if the operator
> said that the stored format was the same, but this happened:
>
> test=# select pg_column_size(ARRAY[1,2,3]);
> pg_column_size
> ----------------
> 36
> (1 row)
>
> test=# select pg_column_size((ARRAY[1,2,3,NULL])::int4[3]);
> pg_column_size
> ----------------
> 44
> (1 row)
>
> They have the same stored format, but a different number of
> bytes?!?

Hmm. For most of this thread, I was leaning toward the view that
comparing the binary representations was the wrong concept, and that
we actually needed to have type-specific operators that understand the
semantics of the data type.

But I think this example convinces me otherwise. What we really want
to do here is test whether two values are the same, and if you can
feed two values that are supposedly the same to some function and get
two different answers, well then they're not really the same, are
they?

Now, I grant that the array case is pretty weird. An array with an
all-zeroes null bitmap is basically semantically identical to one with
no null bitmap at all. But there are other such cases as well. You
can have two floats that print the same way except when
extra_float_digits=3, for example, and I think that's probably a
difference that we *wouldn't* want to paper over. You can have a
long-form numeric that represents a value that could have been
represented as a short-form numeric, which is similar to the array
case. There are probably other examples as well. But in each of
those cases, the point is that there *is* some operation which will
distinguish between the two supposedly-identical values, and therefore
they are not identical for all purposes. Therefore, I see no harm in
having an operator that tests for
are-these-values-identical-for-all-purposes. If that's useful for
RMVC, then there may be other legitimate uses for it as well.

And once we decide that's OK, I think we ought to document it. Sure,
it's a little confusing, but we can explain it, I think. It's a good
opportunity to point out to people that, most of the time, they really
want something else, like the equality operator for the default btree
opclass.

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

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> Therefore, I see no harm in
> having an operator that tests for
> are-these-values-identical-for-all-purposes. If that's useful for
> RMVC, then there may be other legitimate uses for it as well.
>
> And once we decide that's OK, I think we ought to document it. Sure,
> it's a little confusing, but we can explain it, I think. It's a good
> opportunity to point out to people that, most of the time, they really
> want something else, like the equality operator for the default btree
> opclass.

For my 2c on this, while this can be useful for *us*, and maybe folks
hacking pretty close to PG, I can't get behind introducing this as an
'===' or some such operator. I've missed why this can't be a simple
function and why in the world we would want to encourage users to use
this by making it look like a normal language construct of SQL, which
damn well better consider numbers which are equal in value to be equal,
regardless of their representation.

What the heck is the use case for this being a user-oriented, SQL
operator..?

Thanks,

Stephen

On 2013-09-18 11:50:23 -0400, Stephen Frost wrote:
> For my 2c on this, while this can be useful for *us*, and maybe folks
> hacking pretty close to PG, I can't get behind introducing this as an
> '===' or some such operator. I've missed why this can't be a simple
> function and why in the world we would want to encourage users to use
> this by making it look like a normal language construct of SQL, which
> damn well better consider numbers which are equal in value to be equal,
> regardless of their representation.

I certainly understand the feeling...

I think this really needs to have an obscure name. Like ==!!== or
somesuch (is equal very much, but doesn't actually test for equality ;))

> What the heck is the use case for this being a user-oriented, SQL
> operator..?

The materalized view code uses generated SQL, so it has to be SQL
accessible. And it needs to be an operator because the join planning
code requires that :(

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
>> To have clean semantics, I think the operator should mean that the
>> stored format of the row is the same.  Regarding the array null
>> bitmap example, I think it would be truly weird if the operator
>> said that the stored format was the same, but this happened:
>>
>> test=# select pg_column_size(ARRAY[1,2,3]);
>>   pg_column_size
>> ----------------
>>               36
>> (1 row)
>>
>> test=# select pg_column_size((ARRAY[1,2,3,NULL])::int4[3]);
>>   pg_column_size
>> ----------------
>>               44
>> (1 row)
>>
>> They have the same stored format, but a different number of
>> bytes?!?
>
> Hmm.  For most of this thread, I was leaning toward the view that
> comparing the binary representations was the wrong concept, and that
> we actually needed to have type-specific operators that understand the
> semantics of the data type.
>
> But I think this example convinces me otherwise.  What we really want
> to do here is test whether two values are the same, and if you can
> feed two values that are supposedly the same to some function and get
> two different answers, well then they're not really the same, are
> they?

Right.  Not only would the per-type solution make materialized views
maintenance broken by default, requiring per-type work to make it
work reasonably, with silent failures for any type you didn't know
about, but "no user-visible differences" is a pretty slippery
concept.  Did you really think of all the functions someone might
use to look at a value?  Might there be performance differences we
care about that should be handled, even if the user has no way to
dig out the difference?  Will that change in a future release?

> Now, I grant that the array case is pretty weird.  An array with an
> all-zeroes null bitmap is basically semantically identical to one with
> no null bitmap at all.  But there are other such cases as well.  You
> can have two floats that print the same way except when
> extra_float_digits=3, for example, and I think that's probably a
> difference that we *wouldn't* want to paper over.  You can have a
> long-form numeric that represents a value that could have been
> represented as a short-form numeric, which is similar to the array
> case.  There are probably other examples as well.  But in each of
> those cases, the point is that there *is* some operation which will
> distinguish between the two supposedly-identical values, and therefore
> they are not identical for all purposes.  Therefore, I see no harm in
> having an operator that tests for
> are-these-values-identical-for-all-purposes.  If that's useful for
> RMVC, then there may be other legitimate uses for it as well.
>
> And once we decide that's OK, I think we ought to document it.

That seems to be the consensus.  I don't think we can really
document this form of record comparison without also documenting
how equality works.  I'll work something up for the next version of
the patch.

> Sure, it's a little confusing, but we can explain it, I think.  It's a good
> opportunity to point out to people that, most of the time, they really
> want something else, like the equality operator for the default btree
> opclass.

I think the hardest part will be documenting the difference between
the row value constructor semantics (which are all that is
currently documented) and the record equality semantics (used for
sorting and building indexes).  In a green field I think I would
have argued for having just the standard semantics we have
documented, and modifying our sort execution nodes and index builds
to deal with that.  This is one of those cases where the breakage
from changing to that is hard to justify on a cleaner conceptual
semantics basis.

There also seems to be universal agreement that the operator names
should be something other than what I put in the v1 patch, but we
don't have agreement on what should be used instead.  We need six
operators, to support the btree am requirements.  Currently the
patch has:

=== !== >== >>> <== <<<

Suggested "same as" operators so far are:

====
=====
<<=>>
=><=

Anyone want to champion one of those, or something else?  How about
the other five operators to go with your favorite?

Keep in mind that this thread has also turned up strong support for
an operator to express IS NOT DISTINCT FROM -- so that it can be
used with ANY/ALL, among other things.  Long term, having an
opfamily for that might help us clean up the semantics of record
comparison when there are NULLs involved.  Currently we use the =
operator but act as though IS NOT DISTINCT FROM was specified
(except for some cases involving a row value constructor).  Any
serious discussion of that should probably move to a new thread,
but I mention it here because some people wanted to reserve
operator space for that, which could conflict with "same as"
operators.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> I think this really needs to have an obscure name. Like ==!!== or
> somesuch (is equal very much, but doesn't actually test for equality ;))

hah.

> > What the heck is the use case for this being a user-oriented, SQL
> > operator..?
>
> The materalized view code uses generated SQL, so it has to be SQL
> accessible. And it needs to be an operator because the join planning
> code requires that :(

Ugh. This feels like a pretty ugly hack to deal with that. I haven't
got any magical wand to address it, but making an SQL operator for 'are
these really the same bytes' to deal with what is essentially
implementation detail is _very_ grotty.

Thanks,

Stephen

* Kevin Grittner (kgrittn(at)ymail(dot)com) wrote:
> Right.  Not only would the per-type solution make materialized views
> maintenance broken by default, requiring per-type work to make it
> work reasonably, with silent failures for any type you didn't know
> about, but "no user-visible differences" is a pretty slippery
> concept. 

I don't like those possibilities, of course, but I'm starting to wonder
about this whole concept of looking at it byte-wise. If I'm following
correctly, what we're looking at here is having a way for matviews to
tell if these bytes are the same as those bytes, for the purpose of
deciding to update the matview, right? Yet we can then have cases where
the row isn't *actually* different from a value perspective, yet we're
going to update it anyway because it's represented slightly differently?

What happens if we later want to add support for users to have a matview
trigger that's called when a matview row *actually* changes? We'd end
up calling it on what are essentially false positives, or having to do
some double-check later on "well, did it *really* change?", neither of
which is good at all. If we had the IS NOT DISTINCT FROM operators
discussed, would that work for this even if it isn't as performant? Or
is there an issue with that?

Thanks,

Stephen

On Wed, Sep 18, 2013 at 10:59 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
>> I think this really needs to have an obscure name. Like ==!!== or
>> somesuch (is equal very much, but doesn't actually test for equality ;))
>
> hah.
>
>> > What the heck is the use case for this being a user-oriented, SQL
>> > operator..?
>>
>> The materalized view code uses generated SQL, so it has to be SQL
>> accessible. And it needs to be an operator because the join planning
>> code requires that :(
>
> Ugh. This feels like a pretty ugly hack to deal with that. I haven't
> got any magical wand to address it, but making an SQL operator for 'are
> these really the same bytes' to deal with what is essentially
> implementation detail is _very_ grotty.

Having matviews using SQL expressible features is a *good* thing.
Having a user accessible operator is nice to have (if for no other
reason than to allow testing for which matview rows would be
refreshed). I just don't understand what all the fuss is about except
to make sure not to utilize an operator name that is better suited for
other purposes.

merlin

Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> making an SQL operator for 'are these really the same bytes' to
> deal with what is essentially implementation detail is _very_
> grotty.

We already have some such operators, although Andres argues that
comparing to that isn't fair because we at least know it is a
string of characters; we're just ignoring character boundaries and
collations.  Some of the operators use for the existing byte
comparison opclasses are:

~<~ ~<=~ ~>=~ ~>~

Go ahead and try them out with existing text values.  Andres has
said that he has seen these used in production systems.

= and <> aren't listed above even though they do a byte-for-byte
comparison because, well, I guess because we have chosen to treat
two UTF8 strings which produce the same set of glyphs using
different bytes as unequal.  :-/

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-09-18 11:06:13 -0500, Merlin Moncure wrote:
> > Ugh. This feels like a pretty ugly hack to deal with that. I haven't
> > got any magical wand to address it, but making an SQL operator for 'are
> > these really the same bytes' to deal with what is essentially
> > implementation detail is _very_ grotty.

I know the feeling, but I don't have a better suggestion either, so...

> Having matviews using SQL expressible features is a *good* thing.
> Having a user accessible operator is nice to have (if for no other
> reason than to allow testing for which matview rows would be
> refreshed). I just don't understand what all the fuss is about except
> to make sure not to utilize an operator name that is better suited for
> other purposes.

It's an externally exposed API with not easily understandable semantics
that's not actually all that useful outside specific usecases. If we
decide to change it we're creating an API breakage. And we get to deal
with people saying it's broken because they don't understand the
semantics.

That said, I am ok with this if we use strange operator names and
document that the semantics are complicated...

==!!<
==!!<=
...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

* Merlin Moncure (mmoncure(at)gmail(dot)com) wrote:
> Having matviews using SQL expressible features is a *good* thing.

Fine, then it should be implemented *using SQL*, which is based on
*values*, not on how the value is represented in bits and bytes.

> Having a user accessible operator is nice to have (if for no other
> reason than to allow testing for which matview rows would be
> refreshed).

If it's not actually *changing* (wrt its value), then I'm not at all
impressed with the notion that it's going to get updated anyway.

Thanks,

Stephen

* Kevin Grittner (kgrittn(at)ymail(dot)com) wrote:
> = and <> aren't listed above even though they do a byte-for-byte
> comparison because, well, I guess because we have chosen to treat
> two UTF8 strings which produce the same set of glyphs using
> different bytes as unequal.  :-/

I tend to side with Andres on this case actually- we're being asked to
store specific UTF8 bytes by the end user. That is not the same as
treating two different numerics which are the same *number* as
different because they have different binary representations, which is
entirely an internal-to-postgres consideration.

Thanks,

Stephen

On 09/18/2013 11:39 AM, Stephen Frost wrote:
> * Kevin Grittner (kgrittn(at)ymail(dot)com) wrote:
>> = and <> aren't listed above even though they do a byte-for-byte
>> comparison because, well, I guess because we have chosen to treat
>> two UTF8 strings which produce the same set of glyphs using
>> different bytes as unequal. :-/
> I tend to side with Andres on this case actually- we're being asked to
> store specific UTF8 bytes by the end user. That is not the same as
> treating two different numerics which are the same *number* as
> different because they have different binary representations, which is
> entirely an internal-to-postgres consideration.

The problem is that there are datatypes (citext, postgis,...) that have
defined = to return true when comparing two values that are different
not just stored differently. Are you saying that matview's should
update only when the = operator of the datatype returns false and if
people don't like this behaviour they should fix the datatypes?

> Thanks,
>
> Stephen

* Steve Singer (steve(at)ssinger(dot)info) wrote:
> The problem is that there are datatypes (citext, postgis,...) that
> have defined = to return true when comparing two values that are
> different not just stored differently.

If the definition of the type is that they're equal, then they're equal.
Certainly there are cases where this is really rather broken
(particularly in the postgis case that you mention), but I don't think
that means we should change our definition of equality to generally be
"are the bytes the same"- clearly that'd lead to incorrect behavior in
the NUMERIC case.

> Are you saying that
> matview's should update only when the = operator of the datatype
> returns false and if people don't like this behaviour they should
> fix the datatypes?

imv, we are depending on the "=" operator to tell us when the
values are equal, regardless of type. I have a hard time seeing how we
can do anything else. The PostGIS situation is already broken when you
consider UNIQUE constraints and, while it's unfortunate that they'd need
to change their data type to fix that, I do feel it's on them to deal
with it.

Anyone can create an extension with their own data type which returns
wrong data and results, it's not on us to figure out how to make those
work even in the face of blatent violations like making "=" not actually
mean "these values are the same".

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> If it's not actually *changing* (wrt its value), then I'm not at
> all impressed with the notion that it's going to get updated
> anyway.

But PostgreSQL very specifically (and as far as I can tell
*intentionally*) allows you to *change* a value and have it still
be considered *equal*.  The concept of equal values really means
more like "equivalent" or "close enough" for common purposes.  It
very specifically does *not* mean the same value.

As just one example, think how much easier the citext type would be
to implement if it folded all values to lower case as they were
input, rather than preserving the data as entered and considering
different capitalizations as "equal".

The notion that in PostgreSQL a value has not changed if the new
value is equal to the old is just flat out wrong.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Kevin,

On Wednesday, September 18, 2013, Kevin Grittner wrote:

> Stephen Frost <sfrost(at)snowman(dot)net <javascript:;>> wrote:
>
> > If it's not actually *changing* (wrt its value), then I'm not at
> > all impressed with the notion that it's going to get updated
> > anyway.
>
> But PostgreSQL very specifically (and as far as I can tell
> *intentionally*) allows you to *change* a value and have it still
> be considered *equal*.

I'm curious where you're going with that- of course you can update a value
and have the same value (and possibly the same byte representation) stored
over the old.

> The concept of equal values really means
> more like "equivalent" or "close enough" for common purposes. It
> very specifically does *not* mean the same value.

I'm really curious about your thoughts on unique indexes then. Should two
numerics which are the same value but different byte representations be
allowed in a unique index?

> As just one example, think how much easier the citext type would be
> to implement if it folded all values to lower case as they were
> input, rather than preserving the data as entered and considering
> different capitalizations as "equal".

If the type operator says they're equal, then I think we need to consider
them as equal. If an update happens with a conditional of:

where col1 = 'Abc'

When col1 is 'ABC' using citext, should we still issue the update?

The notion that in PostgreSQL a value has not changed if the new
> value is equal to the old is just flat out wrong.

The value *can* be changed to be equal to the existing value but that
doesn't make the two values *not equal*.

Thanks,

Stephen

On 09/18/2013 05:53 PM, Andres Freund wrote:
> On 2013-09-18 11:50:23 -0400, Stephen Frost wrote:
>> For my 2c on this, while this can be useful for *us*, and maybe folks
>> hacking pretty close to PG, I can't get behind introducing this as an
>> '===' or some such operator. I've missed why this can't be a simple
>> function and why in the world we would want to encourage users to use
>> this by making it look like a normal language construct of SQL, which
>> damn well better consider numbers which are equal in value to be equal,
>> regardless of their representation.
> I certainly understand the feeling...
>
> I think this really needs to have an obscure name. Like ==!!== or
> somesuch (is equal very much, but doesn't actually test for equality ;))
In PostgreSQL equality can be "anything" :)

In other words, we have "pluggable equality", so it is entirely
feasible to have an opclass where binary equality is *the* equality

the problem started with some "opclass equality" (case insensitive
comparison) missing user-visible changes.

Cheers

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> I don't think that means we should change our definition of
> equality to generally be "are the bytes the same"- clearly that'd
> lead to incorrect behavior in the NUMERIC case.

Nobody is talking in any way, shape, or form about changing our
concept of what is "equal".  We're talking about recognizing that
in PostgreSQL "equal" does *not* mean "the same".  If we used the
equal concept for determining what has changed, if someone was
tracking numeric data without precision and scale so that they
could track accuracy (by storing the correct number of decimal
positions) the accuracy could not be replicated to a materialized
view.  Of course, streaming replication would replicate the
change, but if '1.4' was stored in a column copied into a matview
and they later updated the source to '1.40' the increase in
accuracy would not flow to the matview.  That would be a bug, not a
feature.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Stephen Frost <sfrost(at)snowman(dot)net>
> Kevin Grittner  wrote:
>> Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>>
>>> If it's not actually *changing* (wrt its value), then I'm not
>>> at all impressed with the notion that it's going to get updated
>>> anyway.
>>
>> But PostgreSQL very specifically (and as far as I can tell
>> *intentionally*) allows you to *change* a value and have it
>> still be considered *equal*.  
>
> I'm curious where you're going with that- of course you can
> update a value and have the same value (and possibly the same
> byte representation) stored over the old.

The way I see it, you can update a column to a different value
which will compare as equal.  That's fine.  Nobody wants to change
that.  But it is still not the same value.

>> The concept of equal values really means
>> more like "equivalent" or "close enough" for common purposes.  It
>> very specifically does *not* mean the same value.
>
> I'm really curious about your thoughts on unique indexes then.
> Should two numerics which are the same value but different byte
> representations be allowed in a unique index?

Not if it is defined with the default opclass, which will use an
equal operator.  Of course, this patch would allow an index on a
record to be defined with record_image_ops, in which case it would
sort by the raw bytes in the values of the record.  That's not
going to be useful in very many places, which is why it would not
be the default.  You don't get that behavior unless you ask for it.

See this docs page for a similar example related to complex numbers:

http://www.postgresql.org/docs/current/interactive/xindex.html#XINDEX-EXAMPLE

> If the type operator says they're equal, then I think we need to
> consider them as equal.

Absolutely.  Two different values may be equal within an opclass.

> If an update happens with a conditional of:
>
> where col1 = 'Abc'
>
> When col1 is 'ABC' using citext, should we still issue the
> update?

Absolutely not, because the update was requested in the case that
the equality test was true.  Yet if a row is updated to replace
'Abc' with 'ABC', then streaming replication should copy the
"different but equal value" (it does), a normal view should now
show 'ABC' (it does), and a refresh of a matview should cause the
matview to show 'ABC' (it doesn't in git, but this patch would make
that work).

> The value *can* be changed to be equal to the existing value but
> that doesn't make the two values *not equal*.

Nobody has ever argued that they should be considered *not equal*.
It's just about providing a way to recognize when two equal values
*are not the same*.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:

>> If an update happens with a conditional of:
>>
>> where col1 = 'Abc'
>>
>> When col1 is 'ABC' using citext, should we still issue the
>> update?
>
> Absolutely not, because the update was requested in the case that
> the equality test was true.

Sorry, as if this thread were not long enough, I misread that and
gave the wrong answer.  Yes, the equal operator was used and the
equal operator for two citext values says those are equal, so the
row *should* be updated.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
> change, but if '1.4' was stored in a column copied into a matview
> and they later updated the source to '1.40' the increase in
> accuracy would not flow to the matview.  That would be a bug, not a
> feature.

Maybe the answer to that use case is to use the seg extension?

http://www.postgresql.org/docs/9.3/interactive/seg.html

IOW, colour me unconvinced about that binary-equality opclass use case
in MatViews. We are trusting the btree equality operator about
everywhere in PostgreSQL and it's quite disturbing to be told that in
fact we should not trust it.

Would it make sense for you to produce a patch without the extra
operators, behavior, opclass and opfamily here so that we can focus on
the MatView parts of it?

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
>> change, but if '1.4' was stored in a column copied into a matview
>> and they later updated the source to '1.40' the increase in
>> accuracy would not flow to the matview.  That would be a bug, not a
>> feature.
>
> Maybe the answer to that use case is to use the seg extension?
>
>   http://www.postgresql.org/docs/9.3/interactive/seg.html

You are arguing that we should provide lesser support for numeric
columns (and who knows how many other types) in materialized views
than we do in streaming replication, pg_dump,
suppress_redundant_updates_trigger(), and other places?  Why?

> IOW, colour me unconvinced about that binary-equality opclass use case
> in MatViews. We are trusting the btree equality operator about
> everywhere in PostgreSQL and it's quite disturbing to be told that in
> fact we should not trust it.

Who said we should not trust it?  I have said that equality in
PostgreSQL does not mean the two values appear the same or behave
the same in all cases -- the definer of the class gets to determine
how many definitions of equality there are, and which (if any) is
tied to the = operator name.  That should not be news to anybody;
it's in the documentation.  I'm proposing a second definition of
equality with a different operator name for comparing two records,
without in any way disturbing the existing definition.  I am taken
completely by surprise that in this case creating a second opclass
for something is somehow controversial.  The documentation I cited
previously provides a clear example of another case where two
completely different concepts of equality for a type are useful.

We have, as a community, gone to a fair amount of trouble  to make
the concept of equality pluggable and allow multiple types of
equality per type.  To me it seems the perfect tool to solve this
problem.  Why the fuss?

> Would it make sense for you to produce a patch without the extra
> operators, behavior, opclass and opfamily here so that we can focus on
> the MatView parts of it?

No, matviews cannot be fixed without the new operators.  Here are
the stats on the patch:

kgrittn(at)Kevin-Desktop:~/pg/master$ git diff --stat master..matview
 contrib/citext/expected/citext.out       |   41 +++
 contrib/citext/expected/citext_1.out     |   41 +++
 contrib/citext/sql/citext.sql            |   23 ++
 src/backend/commands/matview.c           |    7 +-
 src/backend/utils/adt/rowtypes.c         |  482 ++++++++++++++++++++++++++
 src/include/catalog/pg_amop.h            |   10 +
 src/include/catalog/pg_amproc.h          |    1 +
 src/include/catalog/pg_opclass.h         |    1 +
 src/include/catalog/pg_operator.h        |   14 +
 src/include/catalog/pg_opfamily.h        |    1 +
 src/include/catalog/pg_proc.h            |   12 +-
 src/include/utils/builtins.h             |    7 +
 src/test/regress/expected/opr_sanity.out |    7 +-
 13 files changed, 642 insertions(+), 5 deletions(-)

The changes to matview.c are the only ones that are
matview-specific.  Basically, that consists of using the new
operator instead of = in a couple places.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 09/18/2013 09:19 PM, Dimitri Fontaine wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
>> change, but if '1.4' was stored in a column copied into a matview
>> and they later updated the source to '1.40' the increase in
>> accuracy would not flow to the matview. That would be a bug, not a
>> feature.
> Maybe the answer to that use case is to use the seg extension?
>
> http://www.postgresql.org/docs/9.3/interactive/seg.html
>
> IOW, colour me unconvinced about that binary-equality opclass use case
> in MatViews. We are trusting the btree equality operator about
> everywhere in PostgreSQL and it's quite disturbing to be told that in
> fact we should not trust it.
The problem is, that in this case the simple VIEW and MATVIEW
would yield different results.

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

On 09/18/2013 09:41 PM, Kevin Grittner wrote:
> Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> wrote:
>> Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
>>> change, but if '1.4' was stored in a column copied into a matview
>>> and they later updated the source to '1.40' the increase in
>>> accuracy would not flow to the matview. That would be a bug, not a
>>> feature.
>> Maybe the answer to that use case is to use the seg extension?
>>
>> http://www.postgresql.org/docs/9.3/interactive/seg.html
> You are arguing that we should provide lesser support for numeric
> columns (and who knows how many other types) in materialized views
> than we do in streaming replication, pg_dump,
> suppress_redundant_updates_trigger(), and other places? Why?
>
>> IOW, colour me unconvinced about that binary-equality opclass use case
>> in MatViews. We are trusting the btree equality operator about
>> everywhere in PostgreSQL and it's quite disturbing to be told that in
>> fact we should not trust it.
> Who said we should not trust it? I have said that equality in
> PostgreSQL does not mean the two values appear the same or behave
> the same in all cases -- the definer of the class gets to determine
> how many definitions of equality there are, and which (if any) is
> tied to the = operator name. That should not be news to anybody;
> it's in the documentation. I'm proposing a second definition of
> equality with a different operator name for comparing two records,
> without in any way disturbing the existing definition.
Basically what proposed === does is "is guaranteed to be equal".
If it is not *guaranteed* it is safe to re-evaluate, either using
equal or something else.

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

On 09/18/2013 06:05 PM, Stephen Frost wrote:
> * Kevin Grittner (kgrittn(at)ymail(dot)com) wrote:
>> Right. Not only would the per-type solution make materialized views
>> maintenance broken by default, requiring per-type work to make it
>> work reasonably, with silent failures for any type you didn't know
>> about, but "no user-visible differences" is a pretty slippery
>> concept.
> I don't like those possibilities, of course, but I'm starting to wonder
> about this whole concept of looking at it byte-wise. If I'm following
> correctly, what we're looking at here is having a way for matviews to
> tell if these bytes are the same as those bytes, for the purpose of
> deciding to update the matview, right?
Basically what is needed is to check if the rowm *might* have
changed, so the new value, which may or may not be "equal"
would be refreshed into matview.
> Yet we can then have cases where
> the row isn't *actually* different from a value perspective, yet we're
> going to update it anyway because it's represented slightly differently?
>
> What happens if we later want to add support for users to have a matview
> trigger that's called when a matview row *actually* changes? We'd end
> up calling it on what are essentially false positives, or having to do
> some double-check later on "well, did it *really* change?", neither of
> which is good at all. If we had the IS NOT DISTINCT FROM operators
> discussed, would that work for this even if it isn't as performant? Or
> is there an issue with that?
IS NOT DISTINCT solves the problem with weird equality of NULLS
and nothing else, so it would not help here.
What the proposed operator family solves is "possiby changed"

Cheers

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

On 09/18/2013 05:54 PM, Kevin Grittner wrote:
> ...
> I think the hardest part will be documenting the difference between
> the row value constructor semantics (which are all that is
> currently documented) and the record equality semantics (used for
> sorting and building indexes). In a green field I think I would
> have argued for having just the standard semantics we have
> documented, and modifying our sort execution nodes and index builds
> to deal with that. This is one of those cases where the breakage
> from changing to that is hard to justify on a cleaner conceptual
> semantics basis.
>
> There also seems to be universal agreement that the operator names
> should be something other than what I put in the v1 patch, but we
> don't have agreement on what should be used instead. We need six
> operators, to support the btree am requirements. Currently the
> patch has:
>
> === !== >== >>> <== <<<
>
> Suggested "same as" operators so far are:
>
> ====
> =====
> <<=>>
> =><=
>
> Anyone want to champion one of those, or something else? How about
> the other five operators to go with your favorite?
ANother take would be using "possibly unequal" for this with operator
defined as

*==* (*definitely* equal , or guaranteed to be equal)

the inequality operator would thus become !== (may be not equal)

and ordering ops would be ?<==(maybe smaller or equal), ?<(maybe smaller)
and same for larger ?>== and ?>

as a table

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

Cheers

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

On 09/18/2013 07:53 PM, Stephen Frost wrote:
>
> I'm really curious about your thoughts on unique indexes then. Should
> two numerics which are the same value but different byte
> representations be allowed in a unique index?
You could have multiple btree opclasses defined which would enforce
different kind of "uniqueness"

For example you could have an opclass which considers two strings
"equal" if four first bytes are equal.

If you would create an unique index using that opclass you could not
have both "industrial" and
"induction" as primary keys as the same time, as the unique index would
consider them equal.

But you would still want to see the change in your matview after you do

UPDATE mytable set id = 'industrial' where id = 'induction';

Cheers

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
> You are arguing that we should provide lesser support for numeric
> columns (and who knows how many other types) in materialized views
> than we do in streaming replication, pg_dump,
> suppress_redundant_updates_trigger(), and other places?  Why?

Because you're saying that you need SQL semantics, and probably because
I'm not understanding well enough the problem you're trying to solve.

> We have, as a community, gone to a fair amount of trouble  to make
> the concept of equality pluggable and allow multiple types of
> equality per type.  To me it seems the perfect tool to solve this
> problem.  Why the fuss?

Because I don't understand why you need another equality than the
default btree one, certainly. The other opclass, to my knowledge, are
only used in relation with index searches, that is when comparing heap
or input values with indexed values, right?

> No, matviews cannot be fixed without the new operators.  Here are
> the stats on the patch:

Ok, then someone (preferably a commiter) need to understand the problem
at hand in a better way than I do now, I guess. If possible I will read
through your patch, I'm curious now.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On 09/19/2013 12:55 AM, Dimitri Fontaine wrote:
>> We have, as a community, gone to a fair amount of trouble to make
>> > the concept of equality pluggable and allow multiple types of
>> > equality per type. To me it seems the perfect tool to solve this
>> > problem. Why the fuss?
> Because I don't understand why you need another equality than the
> default btree one, certainly.

Basically because 'word'::citext and 'Word'::citext are the same to your
brain but not to your eyeballs.

Unique indexes, for example, only need to please your brain. Matviews
need to please your eyeballs.

--
Vik

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
>> You are arguing that we should provide lesser support for numeric
>> columns (and who knows how many other types) in materialized views
>> than we do in streaming replication, pg_dump,
>> suppress_redundant_updates_trigger(), and other places?  Why?
>
> Because you're saying that you need SQL semantics, and probably because
> I'm not understanding well enough the problem you're trying to solve.

There are examples in the patch and this thread, but rather than
reference back to those I'll add a new one.  Without the patch:

test=# CREATE TABLE nt (id serial PRIMARY KEY, grp citext, num numeric);
CREATE TABLE
test=# INSERT INTO nt (grp, num) VALUES
test-#   ('one', '1.0'),
test-#   ('one', '2.0'),
test-#   ('two', '123.000');
INSERT 0 3
test=# CREATE VIEW nv AS SELECT grp, sum(num) AS num FROM nt GROUP BY grp;
CREATE VIEW
test=# SELECT * FROM nv ORDER BY grp;
 grp |   num  
-----+---------
 one |     3.0
 two | 123.000
(2 rows)

test=# CREATE MATERIALIZED VIEW nm AS SELECT grp, sum(num) AS num FROM nt GROUP BY grp;
SELECT 2
test=# CREATE UNIQUE INDEX nm_id ON nm (grp);
CREATE INDEX
test=# SELECT * FROM nm ORDER BY grp;
 grp |   num  
-----+---------
 one |     3.0
 two | 123.000
(2 rows)

test=# UPDATE nt SET grp = 'Two', num = '123.0000' WHERE id = 3;
UPDATE 1
test=# REFRESH MATERIALIZED VIEW CONCURRENTLY nm;
REFRESH MATERIALIZED VIEW
test=# SELECT * FROM nv ORDER BY grp;
 grp |   num   
-----+----------
 one |      3.0
 Two | 123.0000
(2 rows)

test=# SELECT * FROM nm ORDER BY grp;
 grp |   num  
-----+---------
 one |     3.0
 two | 123.000
(2 rows)

The problem, as I see it, is that the view and the concurrently
refreshed materialized view don't yield the same results for the
same query.  The rows are equal, but they are not the same.  With
the patch the matview, after RMVC, looks just the same as the view.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
> There are examples in the patch and this thread, but rather than
> reference back to those I'll add a new one.  Without the patch:

Thanks much for doing that.

> The problem, as I see it, is that the view and the concurrently
> refreshed materialized view don't yield the same results for the
> same query.  The rows are equal, but they are not the same.  With
> the patch the matview, after RMVC, looks just the same as the view.

My understanding is that if you choose citext then you don't care at all
about the case, so that the two relations actually yield the same
results for the right definition of "same" here: the citext one.

In other words, the results only look different in ways that don't
matter for the datatype involved, and I think that if it matters to the
user then he needs to review is datatype choices or view definition.

So my position on that is that your patch is only adding confusion for
no benefits that I'm able to understand.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On 09/19/2013 10:54 AM, Dimitri Fontaine wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
>> There are examples in the patch and this thread, but rather than
>> reference back to those I'll add a new one. Without the patch:
> Thanks much for doing that.
>
>> The problem, as I see it, is that the view and the concurrently
>> refreshed materialized view don't yield the same results for the
>> same query. The rows are equal, but they are not the same. With
>> the patch the matview, after RMVC, looks just the same as the view.
> My understanding is that if you choose citext then you don't care at all
> about the case, so that the two relations actually yield the same
> results for the right definition of "same" here: the citext one.
>
> In other words, the results only look different in ways that don't
> matter for the datatype involved, and I think that if it matters to the
> user then he needs to review is datatype choices or view definition.
>
> So my position on that is that your patch is only adding confusion for
> no benefits that I'm able to understand.
The aim is to have a view and materialized view return the same results.

If they do not, the user is guaranteed to be confused and to consider
the matview implementation broken

the patch solves the general problem of "when the table changes, refresh"

After saying it like this, the problem could also be solved by including
xmin(s) for rows from underlying table(s)in the matview.

Would this be a better approach ?

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)com> writes:

>> The problem, as I see it, is that the view and the concurrently
>> refreshed materialized view don't yield the same results for the
>> same query.  The rows are equal, but they are not the same.
>> With the patch the matview, after RMVC, looks just the same as
>> the view.
>
> My understanding is that if you choose citext then you don't care
> at all about the case

That's not my understanding.  If that was what citext was for it
would be much simpler to force the case in creating each value.  It
*preserves* the case for display, but ignores it for comparisons.
That's the contract of the type, like it or not.  "Equal" does not
mean "the same".  They clearly want to preserve and display
differences among equal values.

Streaming replication would preserve the differences.  pg_dump and
restore of the data would preserve the differences.  SELECTing the
data shows the differences.  suppress_redundant_updates_trigger()
will not suppress an UPDATE setting an "equal" value unless it is
also *the same*.  A normal VIEW shows the differences. RMV without
CONCURRENTLY will show the difference.  I'm suggesting that RMVC
and the upcoming incremental update of matviews should join this
crowd.

A matview which is being refreshed with the CONCURRENTLY option, or
maintained by incremental maintenance should not look different
from a regular VIEW, and should not suddenly look completely
different after a non-concurrent REFRESH.  If things are left that
way, people will quite justifiably feel that matviews are broken.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-09-19 05:33:22 -0700, Kevin Grittner wrote:
> Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> wrote:
> > Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
>
> >> The problem, as I see it, is that the view and the concurrently
> >> refreshed materialized view don't yield the same results for the
> >> same query.  The rows are equal, but they are not the same.
> >> With the patch the matview, after RMVC, looks just the same as
> >> the view.
> >
> > My understanding is that if you choose citext then you don't care
> > at all about the case
>
> That's not my understanding.  If that was what citext was for it
> would be much simpler to force the case in creating each value.  It
> *preserves* the case for display, but ignores it for comparisons.
> That's the contract of the type, like it or not.  "Equal" does not
> mean "the same".  They clearly want to preserve and display
> differences among equal values.

I agree.

I am not 100% sure if the can of worms this opens is worth the trouble,
but from my POV it's definitely an understandable and sensible goal.

My complaints about this "subfeature" were never about trying to get
that right for matviews...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hannu Krosing <hannu(at)2ndQuadrant(dot)com> wrote:

> the patch solves the general problem of "when the table changes,
> refresh"
>
> After saying it like this, the problem could also be solved by
> including xmin(s) for rows from underlying table(s)in the
> matview.
>
> Would this be a better approach ?

Now you're moving from REFRESH territory into incremental
maintenance.  There have been a very large number of papers written
on that topic, I have reviewed the literature, and I have picked a
technique which looks like it will be fast and reliable -- based on
relational algebra.  Let's save discussion of alternatives such as
you're suggesting here, for when I get past the easy stuff ... like
just refreshing a view so that the new contents are the same as
what you would see if you re-ran the query defining the matview.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 09/19/2013 02:41 PM, Kevin Grittner wrote:
> Hannu Krosing <hannu(at)2ndQuadrant(dot)com> wrote:
>
>> the patch solves the general problem of "when the table changes,
>> refresh"
>>
>> After saying it like this, the problem could also be solved by
>> including xmin(s) for rows from underlying table(s)in the
>> matview.
>>
>> Would this be a better approach ?
> Now you're moving from REFRESH territory into incremental
> maintenance. There have been a very large number of papers written
> on that topic, I have reviewed the literature, and I have picked a
> technique which looks like it will be fast and reliable -- based on
> relational algebra. Let's save discussion of alternatives such as
> you're suggesting here, for when I get past the easy stuff ... like
> just refreshing a view so that the new contents are the same as
> what you would see if you re-ran the query defining the matview.
I'm sure that comparing xmin records would work exactly
similar to binary comparisons of records for detecting
possible change in 99.999% of real-world cases.

I am also pretty sure it would have its own cans of worms
all over again when trying to actually implement this in
matviews :)

Cheers

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

On Wed, Sep 18, 2013 at 7:29 PM, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> wrote:
> On 09/19/2013 12:55 AM, Dimitri Fontaine wrote:
>>> We have, as a community, gone to a fair amount of trouble to make
>>> > the concept of equality pluggable and allow multiple types of
>>> > equality per type. To me it seems the perfect tool to solve this
>>> > problem. Why the fuss?
>> Because I don't understand why you need another equality than the
>> default btree one, certainly.
>
> Basically because 'word'::citext and 'Word'::citext are the same to your
> brain but not to your eyeballs.
>
> Unique indexes, for example, only need to please your brain. Matviews
> need to please your eyeballs.

Right, and well said.

It's perfectly reasonable to want a "unique" index that doesn't allow
both "Kevin O'leary" and "Kevin O'Leary" to be listed in the
"irish_names" table, but it's also perfectly reasonable to want to
remember that the user who entered the data spelled it the second way
and not the first. And it's also reasonable to want any corrections
made to the table to propagate to any materialized views defined over
it.

The fact that we have multiple concepts of equality can be confusing,
but it's not for no reason, and we're not the only database or
programming language to have such a concept.

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

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.

On Thu, Sep 19, 2013 at 06:31:38PM -0400, Steve Singer wrote:
> 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"

It's a pity operators must be non-alpha and can't be named. Something
like:

SELECT foo OPERATOR("byte_equivalent") bar;

is simultaneously obscure, yet clear.

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> He who writes carelessly confesses thereby at the very outset that he does
> not attach much importance to his own thoughts.
-- Arthur Schopenhauer

* Kevin Grittner (kgrittn(at)ymail(dot)com) wrote:
> ... like
> just refreshing a view so that the new contents are the same as
> what you would see if you re-ran the query defining the matview.

I've heard this notion a few times of wanting to make sure that what you
get from running the query matches the matview. While that makes sense
when the equality operator and what-you-see actually match, it doesn't
when they don't because the what-you-see ends up being non-deterministic
and can change based on the order the datums are seen in during the
query processing which can change with different data ordering on disk
and even due to simply different plans for the same data, I believe.

Consider a GROUP BY with a citext column as one of the key fields.
You're going to get whatever value the aggregate happened to come across
first when building the HashAgg. Having the binary equality operator
doesn't help that and seems like it could even result in change storms
happening due to a different plan when the actual data didn't change.

Thanks,

Stephen

Steve Singer <steve(at)ssinger(dot)info> wrote:
> 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.

Thanks for looking at it.

> There has been a heated discussion on list about if we even want this
> type of operator.

It's not a question of whether we want to allow operators which
define comparisons between objects in a non-standard way.  We
already have 11 such cases; this would add one more to make 12.  In
all cases there are different operators for making a comparison
that return potentially different results from the default
operators, to support uses that are specific to that type.

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

I do.  The issue is that there are a great many places that there
are multiple definitions of equality.  We generally try to use
something which tries to convey something about the nature of the
operation, since the fact that it can have different results is a
given.  There is nothing in those operators that says "binary image
comparison".  I thought about prepending a hash character in front
of normal operators, because that somehow suggests binary
operations to my eye (although I have no idea whether it does so
for anyone else), but those operators are already used for an
alternative set of comparisons for intervals, with a different
meaning.  I'm still trying to come up with something I really like.

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

Did you look at the record_eq and record_cmp functions which exist
before this patch?  Is there a reason we should do it one way for
the default operators and another way for this alternative?  Do you
think we should change record_eq and record_cmp to do things the
way you suggest?

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

Again, this is a result of following the precedent set by the
existing record comparison operators.

test=# create table r1 (c1 int, c2 int);
CREATE TABLE
test=# create table r2 (c1 int, c2 int, c3 int);
CREATE TABLE
test=# insert into r1 values (1, 2);
INSERT 0 1
test=# insert into r2 values (3, 4, 5);
INSERT 0 1
test=# select * from r1 join r2 on r1 < r2;
 c1 | c2 | c1 | c2 | c3
----+----+----+----+----
  1 |  2 |  3 |  4 |  5
(1 row)

test=# update r1 set c1 = 3, c2 = 4;
UPDATE 1
test=# select * from r1 join r2 on r1 < r2;
ERROR:  cannot compare record types with different numbers of columns

Would be in favor of forcing a change to the record comparison
operators which have been in use for years?  If not, is there a
good reason to have them behave differently in this regard?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Steve,

Thanks for providing a summary.

* Steve Singer (steve(at)ssinger(dot)info) wrote:
> 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'.

With the cases where the equality operator doesn't match the 'to the
eyeball' appearance, this really seems to be a pipe dream to me, unless
we *also* define an ordering or similar which would then change the
existing semantics for end users (which might be reasonable), but I'm
not sure how we could do that without input from the type- iow, I don't
think we can say "well, we'll order by byte representation". It's also
going to possibly be a performance hit since we're going to have to do
both an equality op call during a HashAgg/HashJoin/whatever and have to
do a "which one is bigger of these two equal things", and then I wonder
if we'd need to allow users to somehow specify "I don't want the bigger
of the equal things, I want the smaller, in this query for this equality
check." I'd really like to see how we're going to provide for that when
the user is doing a GROUP BY without breaking something else or causing
problems with later SQL spec revisions.

> * Supporting the materialized view refresh check via SQL makes a lot
> of sense but doing that requires exposing something via SQL

... which we don't currently expose for the queries that we already
support users running today. Users seem to generally be accepting of
that too, even though what they end up with in the output isn't
necessairly consistent from query to query. The issue here is that
we're trying to make the mat view look like what the query would do when
run at the same time, which is a bit of a losing battle, imv.

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

That's a bit over-simplistic, really. We do try to keep to the POLA
(principle of least astonishment) and that's not going to be easy here.

> 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

I'm a bit on the fence about this, after having discussed my concerns
with Robert at PostgresOpen. If we're going to expose and support
these at the SQL level, and we can figure out some semantics and
consistency for using them that isn't totally baffling, then perhaps
having them as simple and clear operator names would be reasonable. One
concern here is if the SQL spec might decide some day that '===' is a
useful operator for something else (perhaps even "they look the same
when cast to text").

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

Those look alright to me also though, but we'd need to work out the
other operation names, right? And then figure out if we want to use
those other operators (less-than, greater-than, etc) when doing equality
operations to figure out which equal value to return to the user.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Kevin Grittner (kgrittn(at)ymail(dot)com) wrote:
>> ... like
>> just refreshing a view so that the new contents are the same as
>> what you would see if you re-ran the query defining the matview.
>
> I've heard this notion a few times of wanting to make sure that what you
> get from running the query matches the matview.  While that makes sense
> when the equality operator and what-you-see actually match, it doesn't
> when they don't because the what-you-see ends up being non-deterministic
> and can change based on the order the datums are seen in during the
> query processing which can change with different data ordering on disk
> and even due to simply different plans for the same data, I believe.

That's a fair point to some extent.  Where notions of equality
differ, it is not always non-deterministic, but it can be.  For
citext you are correct.  For a sum() of numeric data, the number of
decimal positions will be the largest value seen; the value present
in the query results will not vary by order of rows scanned or by
plan.

The result of this is that with the patch, an incremental refresh
of a matview will always match what the query returned at the time
it was run (there is no *correctness* problem) but if someone uses
a query with non-deterministic results they may have a lot of
activity on a concurrent refresh even if there was no change to the
underlying data -- so you could have a *performance* penalty in
cases where the query returns something different, compared to
leaving the old "equal but not the same" results.

> Consider a GROUP BY with a citext column as one of the key fields.
> You're going to get whatever value the aggregate happened to come across
> first when building the HashAgg.  Having the binary equality operator
> doesn't help that and seems like it could even result in change storms
> happening due to a different plan when the actual data didn't change.

Yup.  A person who wants to GROUP BY a citext value for a matview
might want to fold it to a consistent capitalization to avoid that
issue.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> The issue here is that we're trying to make the mat view look
> like what the query would do when run at the same time, which is
> a bit of a losing battle, imv.

If we're not doing that, I don't see the point of matviews.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Friday, September 20, 2013, Kevin Grittner wrote:

> Stephen Frost <sfrost(at)snowman(dot)net <javascript:;>> wrote:
>
> > The issue here is that we're trying to make the mat view look
> > like what the query would do when run at the same time, which is
> > a bit of a losing battle, imv.
>
> If we're not doing that, I don't see the point of matviews.
>

When taken out of context, I can see how that might not come across
correctly. I was merely pointing out that it's a losing battle in the
context of types which have equality operators which claim equalness but
cast to text with results that don't match there.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> On Friday, September 20, 2013, Kevin Grittner  wrote:
>> Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>>
>>> The issue here is that we're trying to make the mat view look
>>> like what the query would do when run at the same time, which
>>> is a bit of a losing battle, imv.
>>
>> If we're not doing that, I don't see the point of matviews.
>
> When taken out of context, I can see how that might not come
> across correctly. I was merely pointing out that it's a losing
> battle in the context of types which have equality operators
> which claim equalness but cast to text with results that don't
> match there.

I think the patch I've submitted wins that battle.  The only oddity
is that if someone uses a query for a matview which can provide
results with rows which are equal to previous result rows under the
default record opclass but different under this patch's opclass,
RMVC could update to the latest query results when someone thinks
that might not be necessary.  Workarounds would include using a
query with deterministic results (like using the upper() or lower()
function on a grouped citext column in the result set) or not using
the CONCURRENTLY option.  Neither seems too onerous.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Kevin Grittner (kgrittn(at)ymail(dot)com) wrote:
> The result of this is that with the patch, an incremental refresh
> of a matview will always match what the query returned at the time
> it was run (there is no *correctness* problem) but if someone uses
> a query with non-deterministic results they may have a lot of
> activity on a concurrent refresh even if there was no change to the
> underlying data -- so you could have a *performance* penalty in
> cases where the query returns something different, compared to
> leaving the old "equal but not the same" results.

You mean 'at the time of the incremental refresh', right? Yet that may
or may not match running that query directly by an end-user as the plan
that a user might get for the entire query could be different than what
is run for an incremental update, or due to statistics updates, etc.

> > Consider a GROUP BY with a citext column as one of the key fields.
> > You're going to get whatever value the aggregate happened to come across
> > first when building the HashAgg.  Having the binary equality operator
> > doesn't help that and seems like it could even result in change storms
> > happening due to a different plan when the actual data didn't change.
>
> Yup.  A person who wants to GROUP BY a citext value for a matview
> might want to fold it to a consistent capitalization to avoid that
> issue.

I'm trying to figure out why that's a perfectly acceptable solution for
users running views with GROUP BYs, but apparently it isn't sufficient
for mat views? In other words, you would suggest telling users "sorry,
you can't rely on the value returned by a GROUP BY on that citext column
using a normal view, but we're going to try and do better for mat
views".

I don't intend the above to imply that we should never update values in
mat views when we can do so in a reliable way to ensure that the value
matches what a view would return. This matches our notion of UPDATE,
where we will still UPDATE a value even if the old value and the new
value are equal according to the type's equality operator, when the
conditional for the UPDATE is using a reliable type (eg: integer).

Thanks,

Stephen

On 2013-09-20 10:51:46 -0400, Stephen Frost wrote:
> I'm trying to figure out why that's a perfectly acceptable solution for
> users running views with GROUP BYs, but apparently it isn't sufficient
> for mat views?

Err, because users wrote a GROUP BY? They haven't (neccessarily) in the
cases of the matviews we're talking about?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Kevin Grittner (kgrittn(at)ymail(dot)com) wrote:
>> The result of this is that with the patch, an incremental refresh
>> of a matview will always match what the query returned at the time
>> it was run (there is no *correctness* problem) but if someone uses
>> a query with non-deterministic results they may have a lot of
>> activity on a concurrent refresh even if there was no change to the
>> underlying data -- so you could have a *performance* penalty in
>> cases where the query returns something different, compared to
>> leaving the old "equal but not the same" results.
>
> You mean 'at the time of the incremental refresh', right?  Yet that may
> or may not match running that query directly by an end-user as the plan
> that a user might get for the entire query could be different than what
> is run for an incremental update, or due to statistics updates, etc.

I'm confused.  The refresh *does* run the query.  Sure, if the
query is run again it could return different results.  I'm missing
the point here.

>>> Consider a GROUP BY with a citext column as one of the key
>>> fields.  You're going to get whatever value the aggregate
>>> happened to come across first when building the HashAgg.
>>> Having the binary equality operator doesn't help that and seems
>>> like it could even result in change storms happening due to a
>>> different plan when the actual data didn't change.
>>
>> Yup.  A person who wants to GROUP BY a citext value for a matview
>> might want to fold it to a consistent capitalization to avoid that
>> issue.
>
> I'm trying to figure out why that's a perfectly acceptable solution for
> users running views with GROUP BYs, but apparently it isn't sufficient
> for mat views?  In other words, you would suggest telling users "sorry,
> you can't rely on the value returned by a GROUP BY on that citext column
> using a normal view, but we're going to try and do better for mat
> views".

Again, I'm lost.  If they don't do something to make the result
deterministic, it could be different on each run of the VIEW, and
on each REFRESH of the matview.  I don't see why that is an
argument for trying to suppress the effort needed make the matview
match the latest run of the query.

> I don't intend the above to imply that we should never update values in
> mat views when we can do so in a reliable way to ensure that the value
> matches what a view would return.  This matches our notion of UPDATE,
> where we will still UPDATE a value even if the old value and the new
> value are equal according to the type's equality operator, when the
> conditional for the UPDATE is using a reliable type (eg: integer).

Well, we provide a trigger function to suppress the UPDATE
operation if the old and new values are identical -- in terms of
what is stored.  We do not attempt to use the default btree equals
operator to suppress updates to different values in some
circumstances.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> On 2013-09-20 10:51:46 -0400, Stephen Frost wrote:
> > I'm trying to figure out why that's a perfectly acceptable solution for
> > users running views with GROUP BYs, but apparently it isn't sufficient
> > for mat views?
>
> Err, because users wrote a GROUP BY? They haven't (neccessarily) in the
> cases of the matviews we're talking about?

Sure; my thinking was going back to what Hannu had suggested where we
have a mechanism to see if the value was updated (using xmin or similar)
and then update it in the mat view in that case, without actually doing
a comparison at all.

That wouldn't necessairly work for the GROUP BY case, but that situation
doesn't work reliably today anyway. If we solve the GROUP BY case in
SQL for these types then we could use the same for mat views, but we've
been happy enough to ignore the issue thus far.

Thanks,

Stephen

On 2013-09-20 11:05:06 -0400, Stephen Frost wrote:
> * Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> > On 2013-09-20 10:51:46 -0400, Stephen Frost wrote:
> > > I'm trying to figure out why that's a perfectly acceptable solution for
> > > users running views with GROUP BYs, but apparently it isn't sufficient
> > > for mat views?
> >
> > Err, because users wrote a GROUP BY? They haven't (neccessarily) in the
> > cases of the matviews we're talking about?
>
> Sure; my thinking was going back to what Hannu had suggested where we
> have a mechanism to see if the value was updated (using xmin or similar)
> and then update it in the mat view in that case, without actually doing
> a comparison at all.

VACUUM, HOT pruning. Have fun.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

* Kevin Grittner (kgrittn(at)ymail(dot)com) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > You mean 'at the time of the incremental refresh', right?  Yet that may
> > or may not match running that query directly by an end-user as the plan
> > that a user might get for the entire query could be different than what
> > is run for an incremental update, or due to statistics updates, etc.
>
> I'm confused.  The refresh *does* run the query.  Sure, if the
> query is run again it could return different results.  I'm missing
> the point here.

Perhaps I'm assuming things are farther along than they are.. I was
assumed that 'incremental mat view' updates were actuallly 'partial'-
iow, that it keeps track of the rows in the mat view which are
impacted by the set of rows in the base relations and then runs specific
queries to pull out and operate on the base-rel set-of-rows to update
the specific mat-view-rows. If it's running the whole query again then
it's certainly more likely to get the same results that the user did,
but it's not a guarantee and that's only a happy coincidence while we're
still doing the whole query approach (which I hope we'd eventually
progress beyond..).

> > I'm trying to figure out why that's a perfectly acceptable solution for
> > users running views with GROUP BYs, but apparently it isn't sufficient
> > for mat views?  In other words, you would suggest telling users "sorry,
> > you can't rely on the value returned by a GROUP BY on that citext column
> > using a normal view, but we're going to try and do better for mat
> > views".
>
> Again, I'm lost. 

Perhaps my reply to Andres will help clear that up?

> If they don't do something to make the result
> deterministic, it could be different on each run of the VIEW, and
> on each REFRESH of the matview.  I don't see why that is an
> argument for trying to suppress the effort needed make the matview
> match the latest run of the query.

Is this really just "run the new query and look if the old and new row
are different" wrt 'incremental' view updates? If so, I think we're
trying to design something here that we're going to totally break very
shortly when we move beyond that kind of sledgehammer approach to
incremental mat view updates and I'd rather we figure out a solution
which will work beyond that..

> > I don't intend the above to imply that we should never update values in
> > mat views when we can do so in a reliable way to ensure that the value
> > matches what a view would return.  This matches our notion of UPDATE,
> > where we will still UPDATE a value even if the old value and the new
> > value are equal according to the type's equality operator, when the
> > conditional for the UPDATE is using a reliable type (eg: integer).
>
> Well, we provide a trigger function to suppress the UPDATE
> operation if the old and new values are identical -- in terms of
> what is stored.  We do not attempt to use the default btree equals
> operator to suppress updates to different values in some
> circumstances.

This feels like we're trying to figure out how to create a key for a
whole row based on the binary representation of that row and then use
that to check if we should update a given row or not. This seems a bit
radical, but perhaps that's what we should just do then, rather than
invent binary equivilance operators for what-things-look-like-now for
this- extract the row columns out using their binary _send functions
and then hash the result to build a key that you can use. At least then
we're using a documented (well, we could probably improve on this, but
still) and stable representation of the data that at least some of our
users already deal with.

Thanks,

Stephen

* Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> On 2013-09-20 11:05:06 -0400, Stephen Frost wrote:
> > Sure; my thinking was going back to what Hannu had suggested where we
> > have a mechanism to see if the value was updated (using xmin or similar)
> > and then update it in the mat view in that case, without actually doing
> > a comparison at all.
>
> VACUUM, HOT pruning. Have fun.

Yea, clearly oversimplified, but I do expect that we're going to reach a
point where we're looking at the rows being updated in the base rels and
which rows they map to in the view and then marking those rows as
needing to be updated. That whole mechanism doesn't depend on this
"are-they-binary-equal" approach and is what I had anticipated as the
path we'd be going down in the future.

The above is also what I recall had been discussed at the hackers
meeting, along with some ideas/papers about how to specifically
implement partial updates, hence my assumption that was what we were
talking about..

Thanks,

Stephen

On 09/20/2013 08:38 AM, Kevin Grittner wrote:
> Did you look at the record_eq and record_cmp functions which exist
> before this patch? Is there a reason we should do it one way for the
> default operators and another way for this alternative?
> Do you think we should change record_eq and record_cmp to do things
> the way you suggest?

I think the record_eq and record_cmp functions would be better if they
shared code as well, but I don't think changing that should be part of
this patch, you aren't otherwise touching those functions. I know some
people dislike code that is switch based and prefer duplication, my
preference is to avoid duplication.

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.

> Again, this is a result of following the precedent set by the
> existing record comparison operators.
>
> test=# create table r1 (c1 int, c2 int);
> CREATE TABLE
> test=# create table r2 (c1 int, c2 int, c3 int);
> CREATE TABLE
> test=# insert into r1 values (1, 2);
> INSERT 0 1
> test=# insert into r2 values (3, 4, 5);
> INSERT 0 1
> test=# select * from r1 join r2 on r1 < r2;
> c1 | c2 | c1 | c2 | c3
> ----+----+----+----+----
> 1 | 2 | 3 | 4 | 5
> (1 row)
>
> test=# update r1 set c1 = 3, c2 = 4;
> UPDATE 1
> test=# select * from r1 join r2 on r1 < r2;
> ERROR: cannot compare record types with different numbers of columns
>
> Would be in favor of forcing a change to the record comparison
> operators which have been in use for years? If not, is there a
> good reason to have them behave differently in this regard?
>
> --
I hadn't picked up on that you copied that part of the behaviour from
the existing comparison operators.
No we would need a really good reason for changing the behaviour of the
comparison operators and I don't think we have that. I agree that the
binary identical operators should behave similarly to the existing
comparison operators and error out on the column number mismatch after
comparing the columns that are present in both.

Steve

> Kevin Grittner
> EDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>

Dimitri Fontaine escribió:

> My understanding is that if you choose citext then you don't care at all
> about the case, so that the two relations actually yield the same
> results for the right definition of "same" here: the citext one.

For the record, I don't think citext means that the user doesn't care"
about the case; it only means they want the comparisons to be
case-insensitive, but case should nonetheless be preserved. That is,
"case-insensitive, case-preserving". A parallel is MS-DOS file name
semantics.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Sep 20, 2013 at 11:23 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Perhaps I'm assuming things are farther along than they are.. I was
> assumed that 'incremental mat view' updates were actuallly 'partial'-
> iow, that it keeps track of the rows in the mat view which are
> impacted by the set of rows in the base relations and then runs specific
> queries to pull out and operate on the base-rel set-of-rows to update
> the specific mat-view-rows. If it's running the whole query again then
> it's certainly more likely to get the same results that the user did,
> but it's not a guarantee and that's only a happy coincidence while we're
> still doing the whole query approach (which I hope we'd eventually
> progress beyond..).

It seems odd to me that you have such strong opinions about what is
and is not acceptable here given that you don't seem to familiar with
the current state of this work. I will attempt to summarize my
understanding. In 9.3, we can refresh a materialized view by taking
an access exclusive lock on the relation, rerunning the query, and
replacing the contents wholesale. In master, there is a new option to
perform the refresh concurrently, which is documented here:

http://www.postgresql.org/docs/devel/static/sql-refreshmaterializedview.html

It reruns the query in its entirety and then figures out what inserts,
updates, and deletes are needed to make the matview match the output
of the query (see refresh_by_match_merge). This is an improvement
over what is available in 9.3, because even though you still have to
rerun the full query, you don't have to lock out access to the table
in order to apply the changes. However, currently, it sometimes fails
to perform updates that are needed to make the contents of the view
match the query output, because it relies on a notion of equality
other than exact equality. Kevin is proposing to fix this problem via
this patch.

Now, the next project Kevin's going to work on, and that he was
working on when he discovered this problem, is incremental
maintenance: that is, allowing us to update the view *without* needing
to rerun the entire query. This record comparison operator will be
just as important in that context. The *only* strategy refreshing a
materialized view that *doesn't* need this operator is the only we
have in 9.3: through out all the old data and replace it with the
result of re-executing the query. If you want anything smarter than
that, you MUST compare old and new rows for equality so that you can
update only those rows that have been changed. And if you compare
them *any strategy other than the one Kevin is proposing*, namely
binary equality, then you may sometimes decide that a row has not been
changed when it really has, and then you won't update the row, and
then incremental maintenance will be enabled to produce *wrong
answers*. So to me this has come to seem pretty much open and shut.

We all know that materialized views are not going to always match the
data returned by the underlying query. Perhaps the canonical example
is SELECT random(). As you pointed out upthread, any query that is
nondeterministic is a potential problem for materialized views. When
you write a query that can return different output based on the order
in which input rows are scanned, or based on any sort of external
state such as a random-number generator, each refresh of a
materialized view based on that query may produce different answers.
There's not a lot we can do about that, except tell people to avoid
using such queries in materialized views that they expect to be
stable. However, what we're talking about here is a completely
separate problem. Even if the query is 100% deterministic, without
this patch, the materialized view can get out of sync with the query
results.

Granted, most of the ways in which it can get out of sync are fairly
subtle: failing to preserve case in a data type where comparisons are
text-insensitive; gaining or loosing an all-zeroes null bitmap on an
array type; adding or removing trailing zeroes after the decimal point
in a numeric. If the materialized view sometimes said "1" when the
query was returning "0", we'd presumably all say "that's a bug, let's
fix it". But what we're actually talking about is that the query
returns "0.00" and the view still says zero. So we're doing a lot of
soul-searching about whether that's unequal enough to justify updating
the row. Personally, though, there's not a lot of doubt in my mind.
If I create a table and I put 0 into a column of that table and then
create a materialized view and that 0 ends up in the materialized
view, and then I update the 0 to 0.00 and refresh the view, I expect
that change to propagate through to the materialized view. It works
that way if I select from a regular, non-materialized view; and it
also works that way if I select from the table itself. The idea that
materialized views should somehow be exempt from reflecting changes to
the underlying data in certain corner cases seems odd and indefensible
to me, and I can't understand why anyone's arguing that we should do
that.

If we're going to avoid that, we need this operator. We can argue
about how it should be named and whether it should be documented, and
we can have all those arguments and still fix the problem. But if we
decide we're not going to add this operator, then that seems to be
basically saying that we don't want to allow materialized views to
accurately reflect the results of the underlying queries. And I think
that would be an extremely poor decision.

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

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> It seems odd to me that you have such strong opinions about what is
> and is not acceptable here given that you don't seem to familiar with
> the current state of this work.

That'd be because the arguments that I've been trying to make around
this havn't had terribly much to do with the current state of this work
but rather the higher level concepts- which are more important anyway,
imv.

> I will attempt to summarize my
> understanding. In 9.3, we can refresh a materialized view by taking
> an access exclusive lock on the relation, rerunning the query, and
> replacing the contents wholesale. In master, there is a new option to
> perform the refresh concurrently, which is documented here:
>
> http://www.postgresql.org/docs/devel/static/sql-refreshmaterializedview.html
>
> It reruns the query in its entirety and then figures out what inserts,
> updates, and deletes are needed to make the matview match the output
> of the query (see refresh_by_match_merge).

So I've gathered and I'm not terribly surprised that it's broken. It
was unfortunate that we committed it as such, imv. I'm *not* convinced
that means we should build on that in a direction that doesn't appear to
be getting us any closer to real matviews. Being in master doesn't make
it released.

> This is an improvement
> over what is available in 9.3, because even though you still have to
> rerun the full query, you don't have to lock out access to the table
> in order to apply the changes.

I see how you can view it as an improvement, but consider what you'd say
if a user showed up on IRC wanting to implement *exactly* this in their
DB using a cronjob and a bit of plpgsql. Would we encourage him/her and
say "gee, that's a great approach, just compare the whole row!" Heck
no, we'd say "uhh, you need a key there that you can depend on when
you're doing your updates." We'd probably even suggest that they, I
dunno, make that key their *primary key* for the matview. If they asked
what would happen with their little plpgsql code when using citext or
other, similar, type, we'd tell them exactly what would happen when
using such a type with regular updates, and maybe even describe how they
could get themselves into trouble if they tried to issue updates which
changed those key fields and therefore run into possible lock contention
from the unique index on values-that-are-not-really-unique-to-them.

I can't see anyone encouraging that and here we are building a major new
feature on it! To be honest, I'm amazed that I appear to be alone with
this.

> Now, the next project Kevin's going to work on, and that he was
> working on when he discovered this problem, is incremental
> maintenance: that is, allowing us to update the view *without* needing
> to rerun the entire query. This record comparison operator will be
> just as important in that context.

You state this but I don't see where you justify this claim..

> The *only* strategy refreshing a
> materialized view that *doesn't* need this operator is the only we
> have in 9.3: through out all the old data and replace it with the
> result of re-executing the query.

I really don't agree with this, but allow me to play along a bit. What
is going to happen with this incremental updates when you want to use an
index to figure out what to update? Are you going to build the index
using your binary equality operator, which could allow *duplicates that
are not really duplicates*, even in the externally-visible world? And
then we'll be returning multiple rows to the user when we shouldn't be?
There's a whole slew of evil examples of this- it's called unnecessary
DISTINCT's. Or perhaps you'll simply look up based on the actual
equality answer and then *pick one* to use, perhaps the most recent, but
then that may or may not equal what the actual query would generate
*anyway* because of costing, join ordering, etc, etc, as I already
pointed out. You're trying to guarantee something here that you can't.
Trying to fake it and tell the users "oh, you'll be ok" is actually
worse because it means they'll try and depend on it and then get all
kinds of upset when it ends up *not* working.

I'd much rather tell users "look, if you want to use these for *data*
fields, that's fine, because our smart matview will figure out the keys
associated with a GROUP BY and will update based on those, but if you
GROUP BY a column with a type whose equality operator allows things to
be different, you'll get *the same behavior you would get from the
query*, which is to say, it won't be deterministic."

> If you want anything smarter than
> that, you MUST compare old and new rows for equality so that you can
> update only those rows that have been changed. And if you compare
> them *any strategy other than the one Kevin is proposing*, namely
> binary equality, then you may sometimes decide that a row has not been
> changed when it really has, and then you won't update the row, and
> then incremental maintenance will be enabled to produce *wrong
> answers*. So to me this has come to seem pretty much open and shut.

The incremental maintenance approach, I'd hope, would be keeping track
of what changed and what rows in the view need to be regenerated due to
those underlying rows changing. Doing that, you *would* know what
needed to be updated, and how. If you're not keeping track of the
specific rows that changed underneath, then I'm curious how these
incremental matviews will work differently from what's being described
here, where we're trying to make the whole freakin' row the key.

> We all know that materialized views are not going to always match the
> data returned by the underlying query. Perhaps the canonical example
> is SELECT random(). As you pointed out upthread, any query that is
> nondeterministic is a potential problem for materialized views. When
> you write a query that can return different output based on the order
> in which input rows are scanned, or based on any sort of external
> state such as a random-number generator, each refresh of a
> materialized view based on that query may produce different answers.

Of course.

> There's not a lot we can do about that, except tell people to avoid
> using such queries in materialized views that they expect to be
> stable. However, what we're talking about here is a completely
> separate problem. Even if the query is 100% deterministic, without
> this patch, the materialized view can get out of sync with the query
> results.

Only a subset of the queries involving these types are 100%
deterministic and those are the stupid simple cases where the view is
just being used as a synonym. As soon as you start doing anything that
actual uses the equality operator (and we have a *lot* of things that
do!), you're going to get nondeterministic behavior. You're trying to
paper over this and I'm not impressed.

Or are you telling me that a matview using the binary-equality technique
will be deterministic when your GROUP BY a citext column in the view? I
don't buy it.

> Granted, most of the ways in which it can get out of sync are fairly
> subtle: failing to preserve case in a data type where comparisons are
> text-insensitive; gaining or loosing an all-zeroes null bitmap on an
> array type; adding or removing trailing zeroes after the decimal point
> in a numeric. If the materialized view sometimes said "1" when the
> query was returning "0", we'd presumably all say "that's a bug, let's
> fix it". But what we're actually talking about is that the query
> returns "0.00" and the view still says zero. So we're doing a lot of
> soul-searching about whether that's unequal enough to justify updating
> the row. Personally, though, there's not a lot of doubt in my mind.

If it was as open-and-shut as that, and we *could* be 100%
deterministic in *all* cases with these special types when going through
views, I'd be a lot more on-board. However, we can't; not without
insisting that the users use some new set of "binary-aggregation"
operators which are able to *pick* which of the equal-but-not-really
options they want; and by-the-way, you can *bet* that's going to need to
be type-specific.

> If I create a table and I put 0 into a column of that table and then
> create a materialized view and that 0 ends up in the materialized
> view, and then I update the 0 to 0.00 and refresh the view, I expect
> that change to propagate through to the materialized view. It works
> that way if I select from a regular, non-materialized view; and it
> also works that way if I select from the table itself.

Sure- and then you try to do the same thing with a join or a group by.
Just because it works in the simple SELECT case doesn't mean we get to
tell users "you can depend on this!"

> The idea that
> materialized views should somehow be exempt from reflecting changes to
> the underlying data in certain corner cases seems odd and indefensible
> to me, and I can't understand why anyone's arguing that we should do
> that.

*This* implementation of matviews, which doesn't actually track the
changes to the underlying data and *also* doesn't regenerate everything
is the *only* case where this concern is actually realized- and that's
because it's trying to make a key column out of an entire row, which is,
I'd argue, generally accepted in the DB world as a *bad idea*, with good
reason.

> If we're going to avoid that, we need this operator. We can argue
> about how it should be named and whether it should be documented, and
> we can have all those arguments and still fix the problem. But if we
> decide we're not going to add this operator, then that seems to be
> basically saying that we don't want to allow materialized views to
> accurately reflect the results of the underlying queries. And I think
> that would be an extremely poor decision.

I don't want this implementation of matviews, which I think we can all
agree isn't the end-all, be-all wrt what we want matvies in PG to look
like anyway, to introduce things that will cause problems for us and
confuse our users.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Robert Haas (robertmhaas(at)gmail(dot)com) wrote:

>> Now, the next project Kevin's going to work on, and that he was
>> working on when he discovered this problem, is incremental
>> maintenance: that is, allowing us to update the view *without*
>> needing to rerun the entire query.  This record comparison
>> operator will be just as important in that context.
>
> You state this but I don't see where you justify this claim..

Unless we can tell whether there are any differences between two
versions of a row, we can't accurately generate the delta to drive
the incremental maintenance.  The initial thread discussing how
incremental maintenance would be done is here:

http://www.postgresql.org/message-id/flat/1368561126(dot)64093(dot)YahooMailNeo(at)web162904(dot)mail(dot)bf1(dot)yahoo(dot)com#1368561126(dot)64093(dot)YahooMailNeo@web162904.mail.bf1.yahoo.com

The thread on the initial patch for REFRESH MATERIALIZED VIEW
CONCURRENTLY started with:

| Attached is a patch for REFRESH MATERIALIZED VIEW CONCURRENTLY
| for 9.4 CF1.  The goal of this patch is to allow a refresh
| without interfering with concurrent reads, using transactional
| semantics.
|
| It is my hope to get this committed during this CF to allow me to
| focus on incremental maintenance for the rest of the release cycle.

There was much discussion, testing, and revision then, which is
here:

http://www.postgresql.org/message-id/flat/1371225929(dot)28496(dot)YahooMailNeo(at)web162905(dot)mail(dot)bf1(dot)yahoo(dot)com#1371225929(dot)28496(dot)YahooMailNeo@web162905.mail.bf1.yahoo.com

I think pretty much every concern raised was addressed except for a
lingering doubt expressed by Noah over whether IS NOT DISTINCT FROM
semantics were really the right basis for matching rows.  Based on
that feedback, I spent a lot of time looking at why that might or
might not be correct, and decided that I had been wrong to base the
behavior on that, for the reasons Robert expressed so clearly a
couple messages back.  Hence this patch.

I had made the mistake of using an operator which used a
column-by-column comparison based on the equality operator for the
default opclass for comparing two values of each respective column
type.  I had been challenged on that in the review process, and was
responding to it with the fix contained in this patch.

The first post on this thread starts with:

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

I think it is fairly obvious that REFRESH should REgenerate a FRESH
copy of the data, versus incremental maintenance -- which attempts
to keep the matview up-to-date without regenerating the full set of
data.  Whenever there is logical replication (and materialized
views are, conceptually, one form of that -- within the database) I
feel it is important to be able to correct any possible "drift".
With matviews, I see the way to do that as the REFRESH command, and
I feel that it is important to be able to do that in a way that can
run concurrently with readers of the matview -- without blocking
them or being blocked by them.

Discussion of incremental maintenance really belongs on a different
thread.  Since I have gone to the trouble to read a lot of papers
on the topic, and select one that I think is a good basis for our
implementation, I hope everyone will frame discussion in terms of
either:
  -  how best to implement the techniques from that paper, or
  -  why some other paper presents a better technique.

I think it would be madness to approach implementing incremental
maintenance based on an ad hoc set of thoughts rather than a
peer-reviewed paper.  I know how tempting it is it start from zero
and think, "if we just do X we can cover this sort of query."  I
had a few thoughts like that before I read all those papers and
discovered that there were many subtle issues to cover.  We will
have plenty of time to get creative with alternatives when we get
past the query types specifically addressed in the paper and begin
to move into, for example, CTEs and window functions.  I certainly
expect robust discussion around those areas, or even some of the
infrastructure for capturing deltas and triggering the incremental
maintenance.  I really didn't expect to have to burn so much time
and energy arguing over whether a REFRESH should leave the matview
accurately containing the results of the matview's query.

>> We can argue about how it should be named

After looking at the existing suggestions and thinking about it I'm
leaning toward these operators (based on a star in front of the
usual default comparison operators):

  *=  *<>  *>  *>=  *<  *<=

>> and whether it should be documented

I thought we had a consensus to document both the existing record
comparison operators and these new ones, and I'm fine with that.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Sep 23, 2013 at 1:19 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> So I've gathered and I'm not terribly surprised that it's broken. It
> was unfortunate that we committed it as such, imv. I'm *not* convinced
> that means we should build on that in a direction that doesn't appear to
> be getting us any closer to real matviews. Being in master doesn't make
> it released.
>
>> This is an improvement
>> over what is available in 9.3, because even though you still have to
>> rerun the full query, you don't have to lock out access to the table
>> in order to apply the changes.
>
> I see how you can view it as an improvement, but consider what you'd say
> if a user showed up on IRC wanting to implement *exactly* this in their
> DB using a cronjob and a bit of plpgsql. Would we encourage him/her and
> say "gee, that's a great approach, just compare the whole row!" Heck
> no, we'd say "uhh, you need a key there that you can depend on when
> you're doing your updates." We'd probably even suggest that they, I
> dunno, make that key their *primary key* for the matview. If they asked
> what would happen with their little plpgsql code when using citext or
> other, similar, type, we'd tell them exactly what would happen when
> using such a type with regular updates, and maybe even describe how they
> could get themselves into trouble if they tried to issue updates which
> changed those key fields and therefore run into possible lock contention
> from the unique index on values-that-are-not-really-unique-to-them.
>
> I can't see anyone encouraging that and here we are building a major new
> feature on it! To be honest, I'm amazed that I appear to be alone with
> this.

I've written various scripts over the years for this kind of thing,
and they all compared the whole row. I used a PK comparison to
determine which row needed to be updated and then compared the
remaining fields to figure out which values needed to be updated. I
had assumed that's how most people did it; what do you do?

Anyway, that is exactly what Kevin is proposing to do here and, to be
clear, he's NOT proposing to use the binary-identical semantics to
identify the row to be updated. That will happen using the semantics
of whatever index the user chooses to create on the PK column.
Rather, he's only using the binary-identical to decide which rows are
completely unchanged. I might be wrong here, but it seems to me that
that renders most of your argument here moot. Under Kevin's proposal,
changing a citext column that acts as a PK for the matview WON'T cause
the row to be deleted and reinserted, but what it will do is say, oh,
it's the same row (the values are equal) but the case is different
(the rows are not binary-equal), let me go update the PK column with
the new value. From where I stand, that seems like exactly the right
behavior. What do you think should happen instead?

>> Now, the next project Kevin's going to work on, and that he was
>> working on when he discovered this problem, is incremental
>> maintenance: that is, allowing us to update the view *without* needing
>> to rerun the entire query. This record comparison operator will be
>> just as important in that context.
>
> You state this but I don't see where you justify this claim..

The next sentence was intended as justification.

>> The *only* strategy refreshing a
>> materialized view that *doesn't* need this operator is the only we
>> have in 9.3: through out all the old data and replace it with the
>> result of re-executing the query.
>
> I really don't agree with this, but allow me to play along a bit. What
> is going to happen with this incremental updates when you want to use an
> index to figure out what to update?

We already do use an index to figure out what to update with
concurrent refresh; or at least we can, when the query planner thinks
that is the cheapest strategy. I don't imagine that that part will
change significantly for incremental updates.

> Are you going to build the index
> using your binary equality operator, which could allow *duplicates that
> are not really duplicates*, even in the externally-visible world? And
> then we'll be returning multiple rows to the user when we shouldn't be?

No, see above. There's no intention that the user should need to use
this new opclass to make materialized views work correctly. The user
is required to define a unique index in order to use REFRESH
MATERIALIZED VIEW CONCURRENTLY, and that unique index defines the
semantics used to compare existing rows to new candidate rows. Only
after we've matched up the old and new rows do we use the
exact-equality semantics to decide whether to update the existing row
or do nothing.

> There's a whole slew of evil examples of this- it's called unnecessary
> DISTINCT's. Or perhaps you'll simply look up based on the actual
> equality answer and then *pick one* to use, perhaps the most recent,

Sure, all of that would suck, but nobody's proposing anything like that.

> but
> then that may or may not equal what the actual query would generate
> *anyway* because of costing, join ordering, etc, etc, as I already
> pointed out.

Most queries I write produce the same results every time. The order
of the rows may vary when I don't care, but I take care to control the
query so that the contents of the rows don't vary. For example, if I
use string_agg() to group rows, I make sure the rows always arrive in
the same order, using ORDER BY. The reason I do this is because, even
today, failing to do so causes too many user-visible artifacts.
However, I admit that people who don't adhere to that practice will
see additional anomalies with materialized views just as they do when
they run the same query repeatedly. However, that will be true in any
materialized view implementation and has little or nothing to do with
the present proposal.

> You're trying to guarantee something here that you can't.
> Trying to fake it and tell the users "oh, you'll be ok" is actually
> worse because it means they'll try and depend on it and then get all
> kinds of upset when it ends up *not* working.

I don't know what this is referring to.

> I'd much rather tell users "look, if you want to use these for *data*
> fields, that's fine, because our smart matview will figure out the keys
> associated with a GROUP BY and will update based on those, but if you
> GROUP BY a column with a type whose equality operator allows things to
> be different, you'll get *the same behavior you would get from the
> query*, which is to say, it won't be deterministic."

Again, Kevin is proposing something strictly BETTER than this. We
*will* figure out the keys associated with the GROUP BY and we *will*
update based on those, assuming you define the unique index on the
materialized view with the same opclass used for the GROUP BY.
However, if you GROUP BY a column with a type whose equality operator
allows things to be different, then the materialized view will
reference the actual value produced by the last execution of the
query, rather than possibly producing something else.

>> If you want anything smarter than
>> that, you MUST compare old and new rows for equality so that you can
>> update only those rows that have been changed. And if you compare
>> them *any strategy other than the one Kevin is proposing*, namely
>> binary equality, then you may sometimes decide that a row has not been
>> changed when it really has, and then you won't update the row, and
>> then incremental maintenance will be enabled to produce *wrong
>> answers*. So to me this has come to seem pretty much open and shut.
>
> The incremental maintenance approach, I'd hope, would be keeping track
> of what changed and what rows in the view need to be regenerated due to
> those underlying rows changing. Doing that, you *would* know what
> needed to be updated, and how. If you're not keeping track of the
> specific rows that changed underneath, then I'm curious how these
> incremental matviews will work differently from what's being described
> here, where we're trying to make the whole freakin' row the key.

Kevin posted an email discussing the algorithm that he was proposing
to implement back in May.

http://www.postgresql.org/message-id/1368561126.64093.YahooMailNeo@web162904.mail.bf1.yahoo.com

I would suggest that you read the referenced papers for details as to
how the algorithm works. To make a long story short, they do need to
keep track of what's changed, and how. However, that still seems
largely orthogonal to the present discussion.

>> We all know that materialized views are not going to always match the
>> data returned by the underlying query. Perhaps the canonical example
>> is SELECT random(). As you pointed out upthread, any query that is
>> nondeterministic is a potential problem for materialized views. When
>> you write a query that can return different output based on the order
>> in which input rows are scanned, or based on any sort of external
>> state such as a random-number generator, each refresh of a
>> materialized view based on that query may produce different answers.
>
> Of course.
>
>> There's not a lot we can do about that, except tell people to avoid
>> using such queries in materialized views that they expect to be
>> stable. However, what we're talking about here is a completely
>> separate problem. Even if the query is 100% deterministic, without
>> this patch, the materialized view can get out of sync with the query
>> results.
>
> Only a subset of the queries involving these types are 100%
> deterministic and those are the stupid simple cases where the view is
> just being used as a synonym. As soon as you start doing anything that
> actual uses the equality operator (and we have a *lot* of things that
> do!), you're going to get nondeterministic behavior. You're trying to
> paper over this and I'm not impressed.

This does not match my experience. I have written many complex views
over the years which it would have been beneficial to materialize.
For example, I had a 34-table join involving multiple aggregates in
one web app I deployed. I would not consider that to be using the
view "just as a synonym", but I can assure you that the results were
quite deterministic. That view involved both text and numeric data,
as I recall.

> Or are you telling me that a matview using the binary-equality technique
> will be deterministic when your GROUP BY a citext column in the view? I
> don't buy it.

No, I'm not telling you that. That would be a stupid thing to say, so
I'm glad I'm not saying it.

>> Granted, most of the ways in which it can get out of sync are fairly
>> subtle: failing to preserve case in a data type where comparisons are
>> text-insensitive; gaining or loosing an all-zeroes null bitmap on an
>> array type; adding or removing trailing zeroes after the decimal point
>> in a numeric. If the materialized view sometimes said "1" when the
>> query was returning "0", we'd presumably all say "that's a bug, let's
>> fix it". But what we're actually talking about is that the query
>> returns "0.00" and the view still says zero. So we're doing a lot of
>> soul-searching about whether that's unequal enough to justify updating
>> the row. Personally, though, there's not a lot of doubt in my mind.
>
> If it was as open-and-shut as that, and we *could* be 100%
> deterministic in *all* cases with these special types when going through
> views, I'd be a lot more on-board. However, we can't; not without
> insisting that the users use some new set of "binary-aggregation"
> operators which are able to *pick* which of the equal-but-not-really
> options they want; and by-the-way, you can *bet* that's going to need to
> be type-specific.

We don't have to control which operators users can select. The
existing implementation, as committed, lets the user control that by
choosing which unique indexes to define. It's actually a little bit
clunky at the moment; if you define multiple unique indexes on the
materialized view, it seems that we consider a row to be "the same
row" only if the equality operators for all columns in all such
indexes think that the rows hasn't been updated. We might want to
revisit that; I can see insisting that the user pick exactly one
unique index to define the comparison semantics, and giving them
explicit syntax for doing so, precisely to eliminate some of the lock
contention and other problems that might occur from excess updates. I
guess the importance of such a change depends largely on how likely
you think it is that someone might put a unique index on a
materialized view for some reason other than to define the comparison
semantics. But whether we change that or not, the basic point you are
concerned about here is already covered.

>> If I create a table and I put 0 into a column of that table and then
>> create a materialized view and that 0 ends up in the materialized
>> view, and then I update the 0 to 0.00 and refresh the view, I expect
>> that change to propagate through to the materialized view. It works
>> that way if I select from a regular, non-materialized view; and it
>> also works that way if I select from the table itself.
>
> Sure- and then you try to do the same thing with a join or a group by.
> Just because it works in the simple SELECT case doesn't mean we get to
> tell users "you can depend on this!"

No, but I think the case you're concerned about will work
mostly-acceptably today and even better after the proposed patch. If
you disagree, perhaps you could prepare a test case demonstrating the
problem. I don't think that either Kevin or myself are trying to ram
through a feature that is half-baked here, and I certainly agree that
it is very important to get as much of this possible worked out before
9.4 arrives in stores everywhere.

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

* Kevin Grittner (kgrittn(at)ymail(dot)com) wrote:
> Unless we can tell whether there are any differences between two
> versions of a row, we can't accurately generate the delta to drive
> the incremental maintenance.

This is predicated on the assumption that you simply generate the new
view and then try to figure out what to go update. I'm trying to
explain that using that methodology is what landed us in this situation
to begin with.

>   The initial thread discussing how
> incremental maintenance would be done is here:

My apologies for not paying quite as close attention to that thread as I
should have.

> I think it is fairly obvious that REFRESH should REgenerate a FRESH
> copy of the data, versus incremental maintenance -- which attempts
> to keep the matview up-to-date without regenerating the full set of
> data.

Having 'REFRESH' regenerate a fresh copy of the data makes sense to me,
and is what we have now, no? The only issue there is that it takes out
a big lock, which I appreciate that you're trying to get rid of.

>   Whenever there is logical replication (and materialized
> views are, conceptually, one form of that -- within the database) I
> feel it is important to be able to correct any possible "drift".
> With matviews, I see the way to do that as the REFRESH command, and
> I feel that it is important to be able to do that in a way that can
> run concurrently with readers of the matview -- without blocking
> them or being blocked by them.

Of course.

> Discussion of incremental maintenance really belongs on a different
> thread. 

I'm really getting tired of everyone saying "this is the only way to do
it" (or perhaps "well, this is already committed, therefore it must be
what we're gonna do") when a) we're already planning to rip this out
and change it, or so I thought, and b) we're trying to make promises we
can't keep with this approach.

> Since I have gone to the trouble to read a lot of papers
> on the topic, and select one that I think is a good basis for our
> implementation, I hope everyone will frame discussion in terms of
> either:
>   -  how best to implement the techniques from that paper, or
>   -  why some other paper presents a better technique.

My recollection from the hackers meeting is that I'm trying to simply
paraphrase what you had said was in the paper wrt keeping track of what
rows are changed underneath and using that as a basis to implement the
changes necessary in the view. Does the paper you're referring to
describe rerunning the whole query and then trying to figure out what's
been changed..? That's really what I'm having trouble understanding
why anyone would want to implement. I'll try and find time to hunt down
the threads and papers on it, but I really could have sworn this was
gone over at the hacker meeting- and it made a lot of sense to me, then.

> I really didn't expect to have to burn so much time
> and energy arguing over whether a REFRESH should leave the matview
> accurately containing the results of the matview's query.

I appreciate you bringing me up to speed on where things actually are
here- again, sorry for not realizing the direction that this was going
in earlier; it really didn't even occur to me that it would have gone
down this road. I, also, didn't expect to spend so much time on this.

> >> We can argue about how it should be named

Really, I'm back to trying to figure out why we want to go down this
road at all.

> >> and whether it should be documented
>
> I thought we had a consensus to document both the existing record
> comparison operators and these new ones, and I'm fine with that.

If it gets added, it certainly should be documented, and heavily
caveated.

Thanks,

Stephen

On Mon, Sep 23, 2013 at 2:46 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Anyway, that is exactly what Kevin is proposing to do here and, to be
> clear, he's NOT proposing to use the binary-identical semantics to
> identify the row to be updated. That will happen using the semantics
> of whatever index the user chooses to create on the PK column.
> Rather, he's only using the binary-identical to decide which rows are
> completely unchanged. I might be wrong here, but it seems to me that
> that renders most of your argument here moot. Under Kevin's proposal,
> changing a citext column that acts as a PK for the matview WON'T cause
> the row to be deleted and reinserted, but what it will do is say, oh,
> it's the same row (the values are equal) but the case is different
> (the rows are not binary-equal), let me go update the PK column with
> the new value. From where I stand, that seems like exactly the right
> behavior. What do you think should happen instead?

Ah, I'm wrong here. It is true that we use the unique index semantics
to compare the results of rerunning the query to what's in the view,
but it's not true (currently) that we ever do updates on the view as
opposed to delete-and-reinsert. Apparently that code got (mostly)
ripped out at some point, but I was confused by a comment that wasn't
fully updated to reflect the new reality.

Still, I believe that most of the points I'm making here remain valid,
because the key assumption you seem to be making is that Kevin is
proposing to use binary-identical semantics throughout, and that's not
true. Old rows and new candidate rows are matched up using the
user-specified opclass, but binary-identical. Binary-identical just
determines whether to replace the rows.

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

Robert, all,

Skipping out on much of the side-discussion to try and drive at the
heart of this..

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> I would suggest that you read the referenced papers for details as to
> how the algorithm works. To make a long story short, they do need to
> keep track of what's changed, and how.

I suppose it's good to know that I wasn't completely misunderstanding
the discussion in Ottawa.

> However, that still seems
> largely orthogonal to the present discussion.

It *solves* this issue, from where I'm sitting, without any binary
operators at all.

rows 4, 5, 6 are used to compose matrow 1. When 4, 5, or 6 are updated,
matrow 1 gets updated.

When the update happens, and it *will* happen (there's no question about
"oh, should we actually update this record?"), it's a normal PG update
and all of the values in the row get set to whatever the new values are.

The only reason we've having any of this discussion is because, in the
current implementation, aiui anyway, we're saying "oh, the user wants us
to update the matview, but we have *no clue* what actually changed, so
we're just gonna try and guess.." This is changing that from "we're
gonna try and guess.." to "well, if they aren't *binary identical*,
we'll change them." However, if you're actually tracking what's
changing and rebuilding the rows based on that, there's no question
about binary equality, and you're still only updating the rows that
actually need to be updated.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> I'm trying to explain that using that methodology is what landed
> us in this situation to begin with.

I'm trying to figure out what situation you think we're in.
Seriously, if you could apply the patch and show one example that
demonstrates what you see to be a problem, that would be great.

>> I think it is fairly obvious that REFRESH should REgenerate a FRESH
>> copy of the data, versus incremental maintenance -- which attempts
>> to keep the matview up-to-date without regenerating the full set of
>> data.
>
> Having 'REFRESH' regenerate a fresh copy of the data makes sense to me,
> and is what we have now, no?  The only issue there is that it takes out
> a big lock, which I appreciate that you're trying to get rid of.
>
>>   Whenever there is logical replication (and materialized
>> views are, conceptually, one form of that -- within the database) I
>> feel it is important to be able to correct any possible "drift".
>> With matviews, I see the way to do that as the REFRESH command, and
>> I feel that it is important to be able to do that in a way that can
>> run concurrently with readers of the matview -- without blocking
>> them or being blocked by them.
>
> Of course.
>
>> Discussion of incremental maintenance really belongs on a different
>> thread.
>
> I'm really getting tired of everyone saying "this is the only way to do
> it" (or perhaps "well, this is already committed, therefore it must be
> what we're gonna do")