Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1o6Vfy-00080j-JU for pgsql-hackers@arkaria.postgresql.org; Wed, 29 Jun 2022 11:16:07 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1o6Vfx-00039T-HD for pgsql-hackers@arkaria.postgresql.org; Wed, 29 Jun 2022 11:16:05 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1o6UXq-0000xH-WD for pgsql-hackers@lists.postgresql.org; Wed, 29 Jun 2022 10:03:40 +0000 Received: from out162-62-57-87.mail.qq.com ([162.62.57.87]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1o6UXm-0001DK-Gc for pgsql-hackers@lists.postgresql.org; Wed, 29 Jun 2022 10:03:38 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=qq.com; s=s201512; t=1656497008; bh=bKOGZdabLSN9yKOkajp3//eg1SmS098eo0LDaE5sOD0=; h=From:Subject:Date:In-Reply-To:Cc:To:References; b=EW74y98S9qJcJ3jHyj2QEfw5azj3QTJIYQXcT30h7epP6Hilj2aT3ovsszF19SVB8 9YHQSLUGbJzCqyuYg8MaQBXE8eqfClXc+uVQMXtloUQkG4YBtMgqysGzLunSz2MFbe 4Av2Rxc4BHceVQWV/hNux4a2MbhquU3uSfkvzvfg= Received: from smtpclient.apple ([2409:8928:810:363a:791c:b181:27f9:b88e]) by newxmesmtplogicsvrszb7.qq.com (NewEsmtp) with SMTP id D7ABE83; Wed, 29 Jun 2022 18:03:23 +0800 X-QQ-mid: xmsmtpt1656497003tcqyrecn9 Message-ID: X-QQ-XMAILINFO: MRw/zKT/0BpPXN4OyhOEEyUuHJJ/ETeMLuxGM3EF3vW7ta7UDFYndEyEWIkIQU KCuA8TxukXylsfWnOp3kv3vYA00VRfFUUs7Jq+Gpse95Sl1h2qHFeYc8L0/1aK1zdHTgEh2kmUk3 PTxACsqtzO4nbumHHU+BJfgwHd0eEMoe/joWTpaItEo5v3w/i/7boYnh8h0zYPm9qV8xx/JBQpkU a+hChDFiuvCWC7CnbYtzp+S8iH/i7W1GIVDdQnKv7dFdbrMy+laOdA8kFFqMDN5BnY0FBKzWnsvT RqCSTiZ6s5WkFPzzd0/QpEFjeBhtoonF5Q3hQ4xQInO8Ld9rXtMG45AP1Y13B4r8feLyVS84mAaT aDu6YVJ6QQEO4k64iucZ5R47HOd2Htvawd9F2C+HfYQDVQ5tnoHPZ3dBj2yMgafN0zfFoIq2jyAA XU//7DWDUYChWqXAyHXiAha4N+kzUPiqUlnOpYwHrjOHqjfr/DtImzEFNq0tY1PlhuSlb6dnTGHP yngVRLJLF9L3xx1VRFUptB+OwbCIA3Rnk3XxvFK0kqzoAhMn6gTClLYzMWBuNiqX3fBYdS/Pll8S pJtFbxwFqDnIrY5Lv98tJCkRbp5cqq9HBF2GOsfsdSSOPvaMhiqFegPuJSaU6I6s8lczbNqffqih hWWgbfbcXJZjL5pDMr7m2UDn/6N9Z3xzX7AxXAn7JtGSF1hlwGrgt7pJFNtFTvruBgj/x/vftakK uaCN23CzM/9UfuzGEMDTWTDLqGcIlhhSKQxnVz0K+k4z0GBFtr+Z7tJapo/kLWcAh8eS8H6Y9egB C5e3hFoxRY7/Yq0IgkAYibMWMWR65MfO9qTSHV2S65CZ3OaP3SjMVv0dKVLyxhJRpB2yOK3wWIAZ JIDExcth2HGGe5xYgb+0Ip85zBmvLj/nU70/MnzL1RLoKoZ1YyfM9URdsiq3RLTlNVT1bjSziLZq C7hmFrsXQ= From: huyajun X-OQ-MSGID: Content-Type: multipart/alternative; boundary="Apple-Mail=_66FB6202-BA9F-4A3D-B4C2-AEBF52AD099C" Mime-Version: 1.0 (Mac OS X Mail 15.0 \(3693.20.0.1.32\)) Subject: Re: Implementing Incremental View Maintenance Date: Wed, 29 Jun 2022 17:56:39 +0800 In-Reply-To: <20220422145801.2a1ded1096bbe5cec983cfdc@sraoss.co.jp> Cc: PostgreSQL Hackers , Greg Stark , Zhihong Yu , Julien Rouhaud , "r.takahashi_2@fujitsu.com" , vignesh C , Tom Lane , Andrew Dunstan , Andy Fan , Tatsuo Ishii , Thomas Munro To: Yugo NAGATA References: <20210517133646.04c3d9ed232af22c0f290ebc@sraoss.co.jp> <20210922191227.09a9891ec9da5ebe67475f1f@sraoss.co.jp> <20211125163710.2f32ae3d4be5d5f9ade020b6@sraoss.co.jp> <20220113102342.mlhknuy6ppypflq3@jrouhaud> <20220204012548.788837b8b3d76aa57022ba9c@sraoss.co.jp> <20220204014806.d29d5d923a0c51ad723cedb6@sraoss.co.jp> <20220314192616.8d15d05d71b1e426c4bfaf96@sraoss.co.jp> <20220422112939.dc2a3a4e559defc9b43024d0@sraoss.co.jp> <20220422145801.2a1ded1096bbe5cec983cfdc@sraoss.co.jp> X-Mailer: Apple Mail (2.3693.20.0.1.32) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --Apple-Mail=_66FB6202-BA9F-4A3D-B4C2-AEBF52AD099C Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > 2022=E5=B9=B44=E6=9C=8822=E6=97=A5 =E4=B8=8B=E5=8D=881:58=EF=BC=8CYugo = NAGATA =E5=86=99=E9=81=93=EF=BC=9A >=20 > On Fri, 22 Apr 2022 11:29:39 +0900 > Yugo NAGATA wrote: >=20 >> Hi, >>=20 >> On Fri, 1 Apr 2022 11:09:16 -0400 >> Greg Stark wrote: >>=20 >>> This patch has bitrotted due to some other patch affecting = trigger.c. >>>=20 >>> Could you post a rebase? >>>=20 >>> This is the last week of the CF before feature freeze so time is of = the essence. >>=20 >> I attached a rebased patch-set. >>=20 >> Also, I made the folowing changes from the previous. >>=20 >> 1. Fix to not use a new deptye >>=20 >> 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. >>=20 >> As a result, the number of patches are reduced to nine from ten. >=20 >=20 >> 2. Bump the version numbers in psql and pg_dump >>=20 >> This feature's target is PG 16 now. >=20 > Sorry, I revert this change. It was too early to bump up the > version number. >=20 > --=20 > Yugo NAGATA >=20 > = 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=E2=80=9D; Insert into t values(1); 2. In get_matching_condition_string, Considering NULL values, we can not = use simple =3D operator. But how about 'record =3D record', record_eq treat NULL =3D 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=E2=80=99s 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 =3D=3D NIL) return "true"; else { StringInfoData s1; StringInfoData s2; initStringInfo(&match_cond); initStringInfo(&s1); initStringInfo(&s2); /* Considering NULL values, we can not use simple =3D operator. = */ appendStringInfo(&s1, "ROW("); appendStringInfo(&s2, "ROW("); foreach (lc, keys) { Form_pg_attribute attr =3D (Form_pg_attribute) lfirst(lc); char *resname =3D NameStr(attr->attname); char *mv_resname =3D quote_qualified_identifier("mv", = resname); char *diff_resname =3D quote_qualified_identifier("diff", = resname); =20 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.=3D) %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=20= for concurrent updates to the IVM correctly, But how about to Lock it = when actually=20 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?=20 It seems like when maintenance MV, the index may not be used because of our match conditions can=E2=80=99= t use simple =3D operator Looking forward to your early reply to answer my above doubts, thank you = a lot! Regards, Yajun Hu --Apple-Mail=_66FB6202-BA9F-4A3D-B4C2-AEBF52AD099C Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
2022=E5=B9=B44=E6=9C=8822=E6=97=A5 =E4=B8=8B=E5=8D=881:58=EF=BC= =8CYugo NAGATA <nagata@sraoss.co.jp> =E5=86=99=E9=81=93=EF=BC=9A
On = Fri, 22 Apr 2022 11:29:39 +0900
Yugo NAGATA <nagata@sraoss.co.jp>= wrote:

Hi,

On Fri, 1 Apr 2022 11:09:16 = -0400
Greg Stark <stark@mit.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@sraoss.co.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-prol= ong-life-span-of-transition-tables-.patch><v27-0004-Add-Increment= al-View-Maintenance-support-to-pg_d.patch><v27-0005-Add-Increment= al-View-Maintenance-support-to-psql.patch><v27-0006-Add-Increment= al-View-Maintenance-support.patch><v27-0007-Add-aggregate= s-support-in-IVM.patch><v27-0008-Add-regressio= n-tests-for-Incremental-View-Mainte.patch><v27-0009-Add-documenta= tions-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=E2=80=9D;
   Insert into t values(1);

2. In get_matching_condition_string, Considering NULL values, we can not use simple =3D = operator.
But = how about 'record =3D record', record_eq treat NULL =3D = 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=E2=80=99s 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 =3D=3D NIL)
= return "true";
else
= {
StringInfoData s1;
StringInfoData s2;
= initStringInfo(&match_cond);
= initStringInfo(&s1);
= initStringInfo(&s2);
= /* Considering NULL values, = we can not use simple =3D operator. */
appendStringInfo(&s1, "ROW(");
= appendStringInfo(&s2, "ROW(");
= foreach (lc, keys)
= {
Form_pg_attribute attr =3D (Form_pg_attribute) lfirst(lc);
= char = *resname =3D NameStr(attr->attname);
= char = *mv_resname =3D quote_qualified_identifier("mv", resname);
= char = *diff_resname =3D = 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.=3D) %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=E2=80=99t use simple =3D operator

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


= --Apple-Mail=_66FB6202-BA9F-4A3D-B4C2-AEBF52AD099C--