On Wed, Jun 26, 2013 at 1:38 AM, Kevin Grittner <kgri...@ymail.com> wrote:
> Hitoshi Harada <umi.tan...@gmail.com> wrote: > > > I spent a few hours to review the patch. > > Thanks! > > > As far as I can tell, the overall approach is as follows. > > > > - create a new temp heap as non-concurrent does, but with > > ExclusiveLock on the matview, so that reader wouldn't be blocked > > Non-concurrent creates the heap in the matview's tablespace and > namespace, so the "temp" part is different in concurrent > generation. This difference is why concurrent can be faster when > few rows change. > > It's still not clear to me why you need temp in concurrent and not in non-concurrent. If this type of operations is always creating "temp" table and just swap it with existing one, why can't we just make it temp always? And if the performance is the only concern, is temp better than just turning off WAL for the table or UNLOGGED table? > Also, before the next step there is an ANALYZE of the temp table, > so the planner can make good choices in the next step. > > > - with this temp table and the matview, query FULL JOIN and > > extract difference between original matview and temp heap (via SPI) > > Right; into another temp table. > > > - this operation requires unique index for performance reason (or > > correctness reason too) > > It is primarily for correctness in the face of duplicate rows which > have no nulls. Do you think the reasons need to be better > documented with comments? > > Ah, yes, even after looking at patch I was confused if it was for performance or correctness. It's a shame we cannot refresh it concurrently if we have duplicate rows in the matview. I thought it would make sense to allow it without unique key if it was only performance tradeoffs. > I also modified the confusing error message to something close to > the suggestion from Robert. > > What to do about the Assert that the matview is not a system > relation seems like material for a separate patch. After review, > I'm inclined to remove the test altogether, so that extensions can > create matviews in pg_catalog. > > I like this better. > New version attached. > > > Will take another look. Thanks, -- Hitoshi Harada
