On Tue, 30 Jul 2024 at 03:32, Kirill Reshke <reshkekir...@gmail.com> wrote:
>
> On Sat, 27 Jul 2024 at 13:26, Kirill Reshke <reshkekir...@gmail.com> wrote:
> >
> > Hi!
> > Cloudberry DB (Greenplum fork) uses IMMV feature for AQUMV (auto query
> > use matview) feature, so i got interested in how it is implemented.
> >
> > On Thu, 11 Jul 2024 at 09:24, Yugo NAGATA <nag...@sraoss.co.jp> wrote:
> > >
> > > I updated the patch to bump up the version numbers in psql and pg_dump
> > > codes
> > > from 17 to 18.
> >
> > Few suggestions:
> >
> > 1) `Add-relisivm-column-to-pg_class-system-catalog` commit message
> > should be fixed, there is "isimmv" in the last line.
> > 2) I dont get why `Add-Incremental-View-Maintenance-support.patch`
> > goes after 0005 & 0004. Shoulndt we first implement feature server
> > side, only when client (psql & pg_dump) side?
> > 3) Can we provide regression tests for each function separately? Test
> > for main feature in main patch, test for DISTINCT support in
> > v34-0007-Add-DISTINCT-support-for-IVM.patch etc? This way the patchset
> > will be easier to review, and can be committed separelety.
> > 4) v34-0006-Add-Incremental-View-Maintenance-support.patch no longer
> > applies due to 4b74ebf726d444ba820830cad986a1f92f724649. After
> > resolving issues manually, it does not compile, because
> > 4b74ebf726d444ba820830cad986a1f92f724649 also removes
> > save_userid/save_sec_context fields from ExecCreateTableAs.
> >
> > > if (RelationIsIVM(matviewRel) && stmt->skipData)
> > Now this function accepts skipData param.
> >
> > 5) For DISTINCT support patch uses hidden __ivm* columns. Is this
> > design discussed anywhere? I wonder if this is a necessity (only
> > solution) or if there are alternatives.
> > 6)
> > What are the caveats of supporting some simple cases for aggregation
> > funcs like in example?
> > ```
> > regress=# CREATE INCREMENTAL MATERIALIZED VIEW mv_ivm_2 AS SELECT
> > sum(j) + sum(i) from mv_base_a;
> > ERROR: expression containing an aggregate in it is not supported on
> > incrementally maintainable materialized view
> > ```
> > I can see some difficulties with division CREATE IMMV .... AS SELECT
> > 1/sum(i) from mv_base_a; (sum(i) == 0 case), but adding &
> > multiplication should be ok, aren't they?
> >
> >
> > Overall, patchset looks mature, however it is far from being
> > committable due to lack of testing/feedback/discussion. There is only
> > one way to fix this... Test and discuss it!
> >
> >
> > [1] https://github.com/cloudberrydb/cloudberrydb
>
> Hi! Small update: I tried to run a regression test and all
> IMMV-related tests failed on my vm. Maybe I'm doing something wrong, I
> will try to investigate.
>
> Another suggestion: support for \d and \d+ commands in psql. With v34
> patchset applied, psql does not show anything IMMV-related in \d mode.
>
> ```
> reshke=# \d m1
> Materialized view "public.m1"
> Column | Type | Collation | Nullable | Default
> --------+---------+-----------+----------+---------
> i | integer | | |
> Distributed by: (i)
>
>
> reshke=# \d+ m1
> Materialized view "public.m1"
> Column | Type | Collation | Nullable | Default | Storage |
> Compression | Stats target | Description
> --------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
> i | integer | | | | plain |
> | |
> View definition:
> SELECT t1.i
> FROM t1;
> Distributed by: (i)
> Access method: heap
>
> ```
>
> Output should be 'Incrementally materialized view "public.m1"' IMO.
And one more thing, noticed today while playing with patchset:
I believe non-terminal incremental should be OptIncremental
Im talking about this:
```
incremental: INCREMENTAL { $$ = true; }
| /*EMPTY*/ { $$ = false; }
;
```
