Thread: EXPLAIN/EXPLAIN ANALYZE REFRESH MATERIALIZED VIEW

Hi,

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.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Dec 22, 2020 at 7:01 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.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?



With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Dec 28, 2020 at 5:56 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Tue, Dec 22, 2020 at 7:01 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.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.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Thu, 07 Jan 2021 at 17:53, Bharath Rupireddy wrote:
> On Mon, Dec 28, 2020 at 5:56 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
>>
>> On Tue, Dec 22, 2020 at 7:01 PM Bharath Rupireddy
>> <bharath.rupireddyforpostgres@gmail.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.

On Fri, Jan 8, 2021 at 1:50 PM japin <japinli@hotmail.com> wrote:
> 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;

Corrected that.

> +       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`.

Yes, we can skip creating the dest receiver when OIDNewHeap is
invalid, this can happen for plain explain refresh mat view case.

    if (explainInfo && !explainInfo->es->analyze)
        OIDNewHeap = InvalidOid;
    else
        OIDNewHeap = get_new_heap_oid(stmt, matviewRel, matviewOid,
                                      &relpersistence);

Since we don't call ExecutorRun for plain explain, we can skip the
dest receiver creation. I modified the code as below in explain.c.

    if (into)
        dest = CreateIntoRelDestReceiver(into);
    else if (matviewInfo && OidIsValid(matviewInfo->OIDNewHeap))
        dest = CreateTransientRelDestReceiver(matviewInfo->OIDNewHeap);
    else
        dest = None_Receiver;

Thanks for taking a look at the patches.

Attaching v3 patches, please consider these for further review.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Fri, 08 Jan 2021 at 17:24, Bharath Rupireddy wrote:
> On Fri, Jan 8, 2021 at 1:50 PM japin <japinli@hotmail.com> wrote:
>> 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;
>
> Corrected that.
>
>> +       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`.
>
> Yes, we can skip creating the dest receiver when OIDNewHeap is
> invalid, this can happen for plain explain refresh mat view case.
>
>     if (explainInfo && !explainInfo->es->analyze)
>         OIDNewHeap = InvalidOid;
>     else
>         OIDNewHeap = get_new_heap_oid(stmt, matviewRel, matviewOid,
>                                       &relpersistence);
>
> Since we don't call ExecutorRun for plain explain, we can skip the
> dest receiver creation. I modified the code as below in explain.c.
>
>     if (into)
>         dest = CreateIntoRelDestReceiver(into);
>     else if (matviewInfo && OidIsValid(matviewInfo->OIDNewHeap))
>         dest = CreateTransientRelDestReceiver(matviewInfo->OIDNewHeap);
>     else
>         dest = None_Receiver;
>
> Thanks for taking a look at the patches.
>

Thanks!

> Attaching v3 patches, please consider these for further review.
>

I find that both the declaration and definition of match_matview_with_new_data()
have a tab between type and variable.  We can use pgindent to fix it.
What do you think?


static void
match_matview_with_new_data(RefreshMatViewStmt *stmt, Relation  matviewRel,
                                                              ^
                            Oid matviewOid, Oid OIDNewHeap, Oid relowner,
                            int save_sec_context, char relpersistence,
                            uint64  processed)
                                  ^

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

On Fri, Jan 8, 2021 at 9:50 PM japin <japinli@hotmail.com> wrote:
> > Attaching v3 patches, please consider these for further review.
> >
>
> I find that both the declaration and definition of match_matview_with_new_data()
> have a tab between type and variable.  We can use pgindent to fix it.
> What do you think?
>
>
> static void
> match_matview_with_new_data(RefreshMatViewStmt *stmt, Relation  matviewRel,
>                                                               ^
>                             Oid matviewOid, Oid OIDNewHeap, Oid relowner,
>                             int save_sec_context, char relpersistence,
>                             uint64  processed)
>                                   ^

I ran pgindent on 0001 patch to fix the above. 0002 patch has no
changes. If I'm correct, pgindent will be run periodically on master.

Attaching v4 patch set for further review.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Sat, 09 Jan 2021 at 09:38, Bharath Rupireddy wrote:
> On Fri, Jan 8, 2021 at 9:50 PM japin <japinli@hotmail.com> wrote:
>> > Attaching v3 patches, please consider these for further review.
>> >
>>
>> I find that both the declaration and definition of match_matview_with_new_data()
>> have a tab between type and variable.  We can use pgindent to fix it.
>> What do you think?
>>
>>
>> static void
>> match_matview_with_new_data(RefreshMatViewStmt *stmt, Relation  matviewRel,
>>                                                               ^
>>                             Oid matviewOid, Oid OIDNewHeap, Oid relowner,
>>                             int save_sec_context, char relpersistence,
>>                             uint64  processed)
>>                                   ^
>
> I ran pgindent on 0001 patch to fix the above. 0002 patch has no
> changes. If I'm correct, pgindent will be run periodically on master.
>

Thanks for your point out. I don't know before.

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

On Tue, 16 Mar 2021 at 20:13, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> On Tue, Mar 16, 2021 at 1:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> [ Sorry for not looking at this thread sooner ]
>>
>> Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
>> > 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).
>>
>> TBH, I think we should reject this.  The problem with it is that it
>> binds us to the assumption that REFRESH MATERIALIZED VIEW has an
>> explainable plan.  There are various people poking at ideas like
>> incremental matview updates, which might rely on some implementation
>> that doesn't exactly equate to a SQL query.  Incremental updates are
>> hard enough already; they'll be even harder if they also have to
>> maintain compatibility with a pre-existing EXPLAIN behavior.
>>
>> I don't really see that this feature buys us anything you can't
>> get by explaining the view's query, so I think we're better advised
>> to keep our options open about how REFRESH might be implemented
>> in future.
>
> That makes sense to me. Thanks for the comments. I'm fine to withdraw the patch.
>
> I would like to see if the 0001 patch(attaching here) will be useful
> at all. It just splits up the existing ExecRefreshMatView into a few
> functions to make the code readable.

+1.

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

On Sat, Jul 10, 2021 at 5:19 PM John Naylor
<john.naylor@enterprisedb.com> wrote:
> Side note for future reference: While the feature named in the CF entry has been rejected, the remaining 0001 patch
currentlyproposed no longer matches the title, or category. It is possible within the CF app, and helpful, to rename
theentry when the scope changes. 
>
> The proposed patch in the CF for incremental view maintenance [1] does some refactoring of its own in implementing
thefeature. I don't think it makes sense to commit a refactoring that conflicts with that, while not necessarily making
lifeeasier for that feature. Incremental view maintenance is highly desirable, so I don't want to put up unnecessary
roadblocks.

Thanks. I'm okay to close the CF
entry(https://commitfest.postgresql.org/33/2928/) and stop this
thread.

Regards,
Bharath Rupireddy.