Re: Incremental View Maintenance, take 2

On Fri, 1 Sep 2023 15:42:17 +0800
jian he <jian(dot)universality(at)gmail(dot)com> wrote:

I apologize for this late reply.

> I added a new function append_update_set_caluse, and deleted
> functions: {append_set_clause_for_count, append_set_clause_for_sum,
> append_set_clause_for_avg, append_set_clause_for_minmax}
>
> I guess this way is more extensible/generic than yours.

Do you mean that consolidating such functions to a general function
make easier to support a new aggregate function in future? I'm not
convinced completely yet it because your suggestion seems that every
functions' logic are just put into a new function, but providing a
common interface might make a sense a bit.

By the way, when you attach files other than updated patches that
can be applied to master branch, using ".patch" or ".diff" as the
file extension help to avoid to confuse cfbot (for example, like
basedon_v29_matview_c_refactor_update_set_clause.patch.txt).

> src/backend/commands/matview.c
> 2268: /* For tuple deletion */
> maybe "/* For tuple deletion and update*/" is more accurate?

This "deletion" means deletion of tuple from the view rather
than DELETE statement, so I think this is ok.

> Since the apply delta query is quite complex, I feel like adding some
> "if debug then print out the final querybuf.data end if" would be a
> good idea.

Agreed, it would be helpful for debugging. I think it would be good
to add a debug macro that works if DEBUG_IVM is defined rather than
adding GUC like debug_print_..., how about it?

> we add hidden columns somewhere, also to avoid corner cases, so maybe
> somewhere we should assert total attribute number is sane.

The number of hidden columns to be added depends on the view definition
query, so I wonder the Assert condition would be a bit complex. Could
you explain what are you assume about like for example?

Regards,
Yugo Nagata

--
Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp>