On 2013-07-23 00:01:50 +0200, Andres Freund wrote: > On 2013-07-17 10:11:28 -0400, Tom Lane wrote: > > Kevin Grittner <kgri...@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 before that. * The loop over indexes in refresh_by_match_merge should index_open(ExclusiveLock) the indexes initially instead of searching the 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 messages other backends send... I'd even suggest using BuildIndexInfo() or such on the indexes, then you could use ->ii_Expressions et 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 for tid and set the search_path to public,pg_catalog. Now, the whole thing is restricted to talbe owners afaics, making this decidedly 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 support for 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
