On 2013-07-23 00:01:50 +0200, Andres Freund wrote:
> On 2013-07-17 10:11:28 -0400, Tom Lane wrote:
> > Kevin Grittner <kgrittn@postgresql.org> writes:
> > > Add support for REFRESH MATERIALIZED VIEW CONCURRENTLY.
> >
> > The buildfarm members that use -DCLOBBER_CACHE_ALWAYS say this patch
> > is broken.
>
> Jagarundi still tells that story. At least something like the following
> patch seems to be required.
Hm. There seems to be more things that need some more improvement from a
quick look.
First, I have my doubts of the general modus operandy of CONCURRENTLY
here. I think in many, many cases it would be faster to simply swap in
the newly built heap + indexes in concurrently. I think I have seen that
mentioned in the review while mostly skipping the discussion, but still.
That would be easy enough as of Robert's mvcc patch.
* Shouldn't the GetUserIdAndSecContext(), NewGUCNestLevel() dance be done a good bit earlier? We're executing queries
beforethat.
* The loop over indexes in refresh_by_match_merge should index_open(ExclusiveLock) the indexes initially instead of
searchingthe syscache manually. They are opened inside the loop in many cases anyway. There probably aren't any hazards
currently,but I am not even sure about that. The index_open() in the loop at the very least processes the invalidation
messagesother backends send... I'd even suggest using BuildIndexInfo() or such on the indexes, then you could use
->ii_Expressionset al instead of peeking into indkeys by hand.
* All not explicitly looked up operators should be qualified using OPERATOR(). It's easy to define your own = operator
fortid and set the search_path to public,pg_catalog. Now, the whole thing is restricted to talbe owners afaics, making
thisdecidedly less dicey, but still. But if anyobdy actually uses a search_path like the above you can easily hurt
them.
* Same seems to hold true for the y = x and y.* IS DISTINCT FROM x.* operations.
* (y.*) = (x.*) also needs to do proper operator lookups.
* OpenMatViewIncrementalMaintenance() et al seem to be underdocumented.
* why is it even allowed that matview_maintenance_depth is > 1? Unless I miss something there doesn't seem to be
supportfor a recursive refresh whatever that would mean?
* INSERT and DELETE should also alias the table names, to make sure there cannot be issues with the nested queries.
* I'd strongly suggest more descriptive table aliases than x, y, d. Those make the statements rather hard to parse.
* SPI_exec() is deprecated in favor of SPI_execute()
* ISTM deferred uniqueness checks and similar things should be enforced to run ub refresh_by_match_merge()
I think this patch/feature needs a good bit of work...
Greetings,
Andres Freund
-- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services
