MIME-Version: 1.0
References: <20230828115252.c1b018605b9a0756a30c3382@sraoss.co.jp>
 <20230828160530.adde1e20f257d7d345989163@sraoss.co.jp> <CACJufxEoCCJE1vntJp1SWjen8vBUa3vZLgL=swPwar4zim976g@mail.gmail.com>
 <20230902.204634.955758704959569058.t-ishii@sranhm.sra.co.jp>
 <CACJufxFjankFQDNppOfqCTpY=zW4Q0+2WCmKjT95kggiT978Lw@mail.gmail.com>
 <CAHut+PsDpBTxZ7bLhko7_E-C7khMhoNJcriNQ_p_gWjADn01vg@mail.gmail.com>
 <20240123162327.c2803162619dd7634cca0b6c@sraoss.co.jp> <20240304115846.2275fb44fd904e8789d43590@sraoss.co.jp>
 <20240329234700.73ff2e28c9248d29f8fa6a66@sraoss.co.jp> <20240331225931.712683cecb26862b73b2b822@sraoss.co.jp>
 <20240702170311.1ddb417759a48ff12c555b92@sranhm.sraoss.co.jp.sranhm> <20240711132357.fe3f78c184cfa99159208178@sranhm.sraoss.co.jp>
In-Reply-To: <20240711132357.fe3f78c184cfa99159208178@sranhm.sraoss.co.jp>
From: Kirill Reshke <reshkekirill@gmail.com>
Date: Thu, 8 Aug 2024 14:37:54 +0500
Message-ID: <CALdSSPgdkrqZjA7Wb11essc6_=ZgSxjs+frUE3w=bJDu_0mf6A@mail.gmail.com>
Subject: Re: Incremental View Maintenance, take 2
To: Yugo NAGATA <nagata@sraoss.co.jp>
Cc: Peter Smith <smithpb2250@gmail.com>, jian he <jian.universality@gmail.com>, 
	Tatsuo Ishii <ishii@sraoss.co.jp>, pgsql-hackers@postgresql.org
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CALdSSPgdkrqZjA7Wb11essc6_%3DZgSxjs%2BfrUE3w%3DbJDu_0mf6A%40mail.gmail.com>
Precedence: bulk

I am really sorry for splitting my review comments into multiple
emails. I'll try to do a better review in a future, all-in-one.

On Thu, 11 Jul 2024 at 09:24, Yugo NAGATA <nagata@sraoss.co.jp> wrote:
>
> On Tue, 2 Jul 2024 17:03:11 +0900
> Yugo NAGATA <nagata@sraoss.co.jp> wrote:
>
> > On Sun, 31 Mar 2024 22:59:31 +0900
> > Yugo NAGATA <nagata@sraoss.co.jp> wrote:
> > > >
> > > > Also, I added a comment on RelationIsIVM() macro persuggestion from jian he.
> > > > In addition, I fixed a failure reported from cfbot on FreeBSD build caused by;
> > > >
> > > >  WARNING:  outfuncs/readfuncs failed to produce an equal rewritten parse tree
> > > >
> > > > This warning was raised since I missed to modify outfuncs.c for a new field.
> > >
> > > I found cfbot on FreeBSD still reported a failure due to
> > > ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS because the regression test used
> > > wrong role names. Attached is a fixed version, v32.
> >
> > Attached is a rebased version, v33.
>
> I updated the patch to bump up the version numbers in psql and pg_dump codes
> from 17 to 18.
>
> Regards,
> Yugo Nagata
>
> >
> > Regards,
> > Yugo Nagata
> >
> >
> > --
> > Yugo NAGATA <nagata@sraoss.co.jp>
>
>
> --
> Yugo NAGATA <nagata@sraoss.co.jp>

1) Provided patches do not set process title correctly:
```
reshke   2602433 18.7  0.1 203012 39760 ?        Rs   20:41   1:58
postgres: reshke ivm [local] CREATE MATERIALIZED VIEW
```
2) We allow to REFRESH IMMV. Why? IMMV should be always up to date.
Well, I can see that this utility command may be useful in case of
corruption of some base relation/view itself, so there will be a need
to rebuild the whole from scratch.
But we already have VACUUM FULL for this, aren't we?

3) Triggers created for IMMV are not listed via \dS [tablename]

4) apply_old_delta_with_count executes non-trivial SQL statements for
IMMV. It would be really helpful to see this in EXPLAIN ANALYZE.

5)
> + "DELETE FROM %s WHERE ctid IN ("
> + "SELECT tid FROM (SELECT pg_catalog.row_number() over (partition by %s) AS \"__ivm_row_number__\","
> +  "mv.ctid AS tid,"
> +  "diff.\"__ivm_count__\""
> + "FROM %s AS mv, %s AS diff "
> + "WHERE %s) v "
> + "WHERE v.\"__ivm_row_number__\" OPERATOR(pg_catalog.<=) v.\"__ivm_count__\")",
> + matviewname,
> + keysbuf.data,
> + matviewname, deltaname_old,
> + match_cond);

`SELECT pg_catalog.row_number()` is too generic to my taste. Maybe
pg_catalog.immv_row_number() /  pg_catalog.get_immv_row_number() ?

6)

> +static void
> +apply_new_delta(const char *matviewname, const char *deltaname_new,
> + StringInfo target_list)
> +{
> + StringInfoData querybuf;
>+
> + /* Search for matching tuples from the view and update or delete if found. */

Is this comment correct? we only insert tuples here?

7)

 During patch development, one should pick OIDs from range 8000-9999
> +# IVM
> +{ oid => '786', descr => 'ivm trigger (before)',
> +  proname => 'IVM_immediate_before', provolatile => 'v', prorettype => 'trigger',
> +  proargtypes => '', prosrc => 'IVM_immediate_before' },
> +{ oid => '787', descr => 'ivm trigger (after)',
> +  proname => 'IVM_immediate_maintenance', provolatile => 'v', prorettype => 'trigger',
> +  proargtypes => '', prosrc => 'IVM_immediate_maintenance' },
> +{ oid => '788', descr => 'ivm filetring ',
> +  proname => 'ivm_visible_in_prestate', provolatile => 's', prorettype => 'bool',
> +  proargtypes => 'oid tid oid', prosrc => 'ivm_visible_in_prestate' },
> ]


--
Best regards,
Kirill Reshke