Re: EXPLAIN/EXPLAIN ANALYZE REFRESH MATERIALIZED VIEW

On Thu, 07 Jan 2021 at 17:53, Bharath Rupireddy wrote:
> On Mon, Dec 28, 2020 at 5:56 PM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>>
>> On Tue, Dec 22, 2020 at 7:01 PM Bharath Rupireddy
>> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>> > Currently, $subject is not allowed. We do plan the mat view query
>> > before every refresh. I propose to show the explain/explain analyze of
>> > the select part of the mat view in case of Refresh Mat View(RMV). It
>> > will be useful for the user to know what exactly is being planned and
>> > executed as part of RMV. Please note that we already have
>> > explain/explain analyze CTAS/Create Mat View(CMV), where we show the
>> > explain/explain analyze of the select part. This proposal will do the
>> > same thing.
>> >
>> > The behaviour can be like this:
>> > EXPLAIN REFRESH MATERIALIZED VIEW mv1; --> will not refresh the mat
>> > view, but shows the select part's plan of mat view.
>> > EXPLAIN ANALYZE REFRESH MATERIALIZED VIEW mv1; --> will refresh the
>> > mat view and shows the select part's plan of mat view.
>> >
>> > Thoughts? If okay, I will post a patch later.
>>
>> Attaching below patches:
>>
>> 0001 - Rearrange Refresh Mat View Code - Currently, the function
>> ExecRefreshMatView in matview.c is having many lines of code which is
>> not at all good from readability and maintainability perspectives.
>> This patch adds a few functions and moves the code from
>> ExecRefreshMatView to them making the code look better.
>>
>> 0002 - EXPLAIN/EXPLAIN ANALYZE REFRESH MATERIALIZED VIEW support and tests.
>>
>> If this proposal is useful, I have few open points - 1) In the patch I
>> have added a new mat view info parameter to ExplainOneQuery(), do we
>> also need to add it to ExplainOneQuery_hook_type? 2) Do we document
>> (under respective command pages or somewhere else) that we allow
>> explain/explain analyze for a command?
>>
>> Thoughts?
>
> Attaching v2 patch set reabsed on the latest master f7a1a805cb. And
> also added an entry for upcoming commitfest -
> https://commitfest.postgresql.org/32/2928/
>
> Please consider the v2 patches for further review.
>

Thanks for updating the patch!

+ /* Get the data generating query. */
+ dataQuery = get_matview_query(stmt, &matviewRel, &matviewOid);

- /*
- * Check for active uses of the relation in the current transaction, such
- * as open scans.
- *
- * NB: We count on this to protect us against problems with refreshing the
- * data using TABLE_INSERT_FROZEN.
- */
- CheckTableNotInUse(matviewRel, "REFRESH MATERIALIZED VIEW");
+ relowner = matviewRel->rd_rel->relowner;

After apply the patch, there is a duplicate

relowner = matviewRel->rd_rel->relowner;

+ else if(matviewInfo)
+ dest = CreateTransientRelDestReceiver(matviewInfo->OIDNewHeap);

If the `matviewInfo->OIDNewHeap` is invalid, IMO we don't need create
DestReceiver, isn't it? And we should add a space after `if`.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.Ltd.