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 <nag...@sraoss.co.jp> wrote:
>
> On Tue, 2 Jul 2024 17:03:11 +0900
> Yugo NAGATA <nag...@sraoss.co.jp> wrote:
>
> > On Sun, 31 Mar 2024 22:59:31 +0900
> > Yugo NAGATA <nag...@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 <nag...@sraoss.co.jp>
>
>
> --
> Yugo NAGATA <nag...@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
