On Wed, 16 Feb 2022 22:34:18 +0800
huyajun <[email protected]> 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 <[email protected]>
