Re: Incremental View Maintenance, take 2 - Mailing list pgsql-hackers

On Mon, 4 Sep 2023 16:48:02 +0800
jian he <jian.universality@gmail.com> wrote:
> other ideas based on v29.
> 
> src/include/utils/rel.h
> 680: #define RelationIsIVM(relation) ((relation)->rd_rel->relisivm)
> I guess it would be better to add some comments to address the usage.
> Since all peer macros all have some comments.

OK. I will add comments on this macro.

> pg_class change, I guess we need bump CATALOG_VERSION_NO?

CATALOG_VERSION_NO is frequently bumped up when new features are
committed, so including it in the patch causes frequent needs for
rebase during the review of the patch even if no meaningful change
is made. Therefore, I wonder we don't have to included it in the
patch at this time.  

> small issue. makeIvmAggColumn and calc_delta need to add an empty
> return statement?

I'm sorry but I could not understand what you suggested, so could
you give me more explanation?

> style issue. in gram.y, "incremental" upper case?
> +               CREATE OptNoLog incremental MATERIALIZED VIEW
> create_mv_target AS SelectStmt opt_with_data

This "incremental" is defined as INCREMENTAL or empty, as below.

incremental:    INCREMENTAL             { $$ = true; }
                 | /*EMPTY*/             { $$ = false; }


> I don't know how pgident works, do you need to add some keywords to
> src/tools/pgindent/typedefs.list to make indentation work?

I'm not sure typedefs.list should be updated in each patch, because
tools/pgindent/README said that the latest typedef file is downloaded
from the buildfarm when pgindent is run.

> in
>     /* If this is not the last AFTER trigger call, immediately exit. */
>     Assert (entry->before_trig_count >= entry->after_trig_count);
>     if (entry->before_trig_count != entry->after_trig_count)
>         return PointerGetDatum(NULL);
> 
> before returning NULL, do you also need clean_up_IVM_hash_entry? (I
> don't know when this case will happen)

No, clean_up_IVM_hash_entry is not necessary in this case.
When multiple tables are updated in a statement, statement-level AFTER
triggers collects every information of the tables, and the last AFTER
trigger have to perform the actual maintenance of the view. To make sure
this, the number that BEFORE and AFTER trigger is fired is counted
respectively, and when they match it is regarded the last AFTER trigger
call performing the maintenance. Until this, collected information have
to keep, so we cannot call clean_up_IVM_hash_entry. 

> in
>         /* Replace the modified table with the new delta table and
> calculate the new view delta*/
>         replace_rte_with_delta(rte, table, true, queryEnv);
>         refresh_matview_datafill(dest_new, query, queryEnv, tupdesc_new, "");
> 
> replace_rte_with_delta does not change the argument: table, argument:
> queryEnv. refresh_matview_datafill just uses the partial argument of
> the function calc_delta. So I guess, I am confused by the usage of
> replace_rte_with_delta. also I think it should return void, since you
> just modify the input argument. Here refresh_matview_datafill is just
> persisting new delta content to dest_new?

Yes, refresh_matview_datafill executes the query and the result rows to
"dest_new". And, replace_rte_with_delta updates the input argument "rte"
and returns the result to it, so it may be better that this returns void,
as you suggested.

Regards,
Yugo Nagata

-- 
Yugo NAGATA <nagata@sraoss.co.jp>