Re: record identical operator

* Robert Haas (robertmhaas@gmail.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