On Wed, 16 Feb 2022 22:34:18 +0800 huyajun <hu_ya...@qq.com> wrote: > Hi, Nagata-san > I am very interested in IMMV and read your patch but have some comments in > v25-0007-Add-Incremental-View-Maintenance-support.patch and want to discuss > with you.
Thank you for your review! > > + /* For IMMV, we need to rewrite matview query */ > + query = rewriteQueryForIMMV(query, into->colNames); > + query_immv = copyObject(query); > > /* Create triggers on incremental maintainable materialized view */ > + Assert(query_immv != NULL); > + CreateIvmTriggersOnBaseTables(query_immv, > matviewOid, true); > 1. Do we need copy query?Is it okay that CreateIvmTriggersOnBaseTables > directly use (Query *) into->viewQuery instead of query_immv like > CreateIndexOnIMMV? It seems only planner may change query, but it shouldn't > affect us finding the correct base table in CreateIvmTriggersOnBaseTables . The copy to query_immv was necessary for supporting sub-queries in the view definition. However, we excluded the fueature from the current patch to reduce the patch size, so it would be unnecessary. I'll fix it. > > +void > +CreateIndexOnIMMV(Query *query, Relation matviewRel) > +{ > + Query *qry = (Query *) copyObject(query); > 2. Also, is it okay to not copy query in CreateIndexOnIMMV? It seems we only > read query in CreateIndexOnIMMV. This was also necessary for supporting CTEs, but unnecessary in the current patch, so I'll fix it, too. Regards, Yugo Nagata -- Yugo NAGATA <nag...@sraoss.co.jp>