Re: Implementing Incremental View Maintenance

> 2022年4月22日 下午1:58，Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp> 写道：
>
> On Fri, 22 Apr 2022 11:29:39 +0900
> Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp> wrote:
>
>> Hi,
>>
>> On Fri, 1 Apr 2022 11:09:16 -0400
>> Greg Stark <stark(at)mit(dot)edu> wrote:
>>
>>> This patch has bitrotted due to some other patch affecting trigger.c.
>>>
>>> Could you post a rebase?
>>>
>>> This is the last week of the CF before feature freeze so time is of the essence.
>>
>> I attached a rebased patch-set.
>>
>> Also, I made the folowing changes from the previous.
>>
>> 1. Fix to not use a new deptye
>>
>> In the previous patch, we introduced a new deptye 'm' into pg_depend.
>> This deptype was used for looking for IVM triggers to be removed at
>> REFRESH WITH NO DATA. However, we decided to not use it for reducing
>> unnecessary change in the core code. Currently, the trigger name and
>> dependent objclass are used at that time instead of it.
>>
>> As a result, the number of patches are reduced to nine from ten.
>
>
>> 2. Bump the version numbers in psql and pg_dump
>>
>> This feature's target is PG 16 now.
>
> Sorry, I revert this change. It was too early to bump up the
> version number.
>
> --
> Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp>
>
> <v27-0001-Add-a-syntax-to-create-Incrementally-Maintainabl.patch><v27-0002-Add-relisivm-column-to-pg_class-system-catalog.patch><v27-0003-Allow-to-prolong-life-span-of-transition-tables-.patch><v27-0004-Add-Incremental-View-Maintenance-support-to-pg_d.patch><v27-0005-Add-Incremental-View-Maintenance-support-to-psql.patch><v27-0006-Add-Incremental-View-Maintenance-support.patch><v27-0007-Add-aggregates-support-in-IVM.patch><v27-0008-Add-regression-tests-for-Incremental-View-Mainte.patch><v27-0009-Add-documentations-about-Incremental-View-Mainte.patch>

Hi, Nagata-san
I read your patch with v27 version and has some new comments,I want to discuss with you.

1. How about use DEPENDENCY_INTERNAL instead of DEPENDENCY_AUTO
when record dependence on trigger created by IMV.( related code is in the end of CreateIvmTrigger)
Otherwise, User can use sql to drop trigger and corrupt IVM, DEPENDENCY_INTERNAL is also semantically more correct
Crash case like:
create table t( a int);
create incremental materialized view s as select * from t;
drop trigger "IVM_trigger_XXXX”;
Insert into t values(1);

2. In get_matching_condition_string, Considering NULL values, we can not use simple = operator.
But how about 'record = record', record_eq treat NULL = NULL
it should fast than current implementation for only one comparation
Below is my simple implementation with this, Variables are named arbitrarily..
I test some cases it’s ok

static char *
get_matching_condition_string(List *keys)
{
StringInfoData match_cond;
ListCell *lc;

/* If there is no key columns, the condition is always true. */
if (keys == NIL)
return "true";
else
{
StringInfoData s1;
StringInfoData s2;
initStringInfo(&match_cond);
initStringInfo(&s1);
initStringInfo(&s2);
/* Considering NULL values, we can not use simple = operator. */
appendStringInfo(&s1, "ROW(");
appendStringInfo(&s2, "ROW(");
foreach (lc, keys)
{
Form_pg_attribute attr = (Form_pg_attribute) lfirst(lc);
char *resname = NameStr(attr->attname);
char *mv_resname = quote_qualified_identifier("mv", resname);
char *diff_resname = quote_qualified_identifier("diff", resname);

appendStringInfo(&s1, "%s", mv_resname);
appendStringInfo(&s2, "%s", diff_resname);

if (lnext(lc))
{
appendStringInfo(&s1, ", ");
appendStringInfo(&s2, ", ");
}
}
appendStringInfo(&s1, ")::record");
appendStringInfo(&s2, ")::record");
appendStringInfo(&match_cond, "%s operator(pg_catalog.=) %s", s1.data, s2.data);
return match_cond.data;
}
}

3. Consider truncate base tables, IVM will not refresh, maybe raise an error will be better

4. In IVM_immediate_before,I know Lock base table with ExclusiveLock is
for concurrent updates to the IVM correctly, But how about to Lock it when actually
need to maintain MV which in IVM_immediate_maintenance
In this way you don't have to lock multiple times.

5. Why we need CreateIndexOnIMMV, is it a optimize?
It seems like when maintenance MV,
the index may not be used because of our match conditions can’t use simple = operator

Looking forward to your early reply to answer my above doubts, thank you a lot!
Regards,
Yajun Hu