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

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