EXPLAIN/EXPLAIN ANALYZE REFRESH MATERIALIZED VIEW

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.

Hi Japin,

On 1/8/21 9:02 PM, japin wrote:

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:

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.

Do you know if you will have time to review this patch during the
current commitfest?

Regards,
--
-David
david@pgmasters.net

On Wed, 03 Mar 2021 at 20:56, David Steele <david@pgmasters.net> wrote:

Do you know if you will have time to review this patch during the
current commitfest?

Sorry for the late reply! I think I have time to review this patch
and I will do it later.

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

On Thu, Mar 4, 2021 at 11:41 AM Japin Li <japinli@hotmail.com> wrote:

On Wed, 03 Mar 2021 at 20:56, David Steele <david@pgmasters.net> wrote:

Do you know if you will have time to review this patch during the
current commitfest?

Sorry for the late reply! I think I have time to review this patch
and I will do it later.

Thanks! I will look forward for more review comments.

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

On Thu, 04 Mar 2021 at 14:53, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

Thanks! I will look forward for more review comments.

v4-0001-Rearrange-Refresh-Mat-View-Code.patch
---------------------------------------------

+static Oid
+get_new_heap_oid(RefreshMatViewStmt *stmt, Relation matviewRel, Oid matviewOid,
+				 char *relpersistence)
+{
+	Oid			OIDNewHeap;
+	bool		concurrent;
+	Oid			tableSpace;
+
+	concurrent = stmt->concurrent;
+
+	/* Concurrent refresh builds new data in temp tablespace, and does diff. */
+	if (concurrent)
+	{
+		tableSpace = GetDefaultTablespace(RELPERSISTENCE_TEMP, false);
+		*relpersistence = RELPERSISTENCE_TEMP;
+	}

Since the concurrent only use in one place, I think we can remove the local variable
concurrent in get_new_heap_oid().

The others looks good to me.

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

On Fri, Mar 5, 2021 at 9:32 AM Japin Li <japinli@hotmail.com> wrote:

On Thu, 04 Mar 2021 at 14:53, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

Thanks! I will look forward for more review comments.

v4-0001-Rearrange-Refresh-Mat-View-Code.patch
---------------------------------------------

+static Oid
+get_new_heap_oid(RefreshMatViewStmt *stmt, Relation matviewRel, Oid matviewOid,
+                                char *relpersistence)
+{
+       Oid                     OIDNewHeap;
+       bool            concurrent;
+       Oid                     tableSpace;
+
+       concurrent = stmt->concurrent;
+
+       /* Concurrent refresh builds new data in temp tablespace, and does diff. */
+       if (concurrent)
+       {
+               tableSpace = GetDefaultTablespace(RELPERSISTENCE_TEMP, false);
+               *relpersistence = RELPERSISTENCE_TEMP;
+       }

Since the concurrent only use in one place, I think we can remove the local variable
concurrent in get_new_heap_oid().

Done.

The others looks good to me.

Thanks.

Attaching v5 patch set for further review.

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

On Fri, 05 Mar 2021 at 19:48, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

Attaching v5 patch set for further review.

The v5 patch looks good to me, if there is no objection, I'll change the
cf status to "Ready for Committer" in few days.

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

On Sun, Mar 7, 2021 at 11:49 AM Japin Li <japinli@hotmail.com> wrote:

On Fri, 05 Mar 2021 at 19:48, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

Attaching v5 patch set for further review.

The v5 patch looks good to me, if there is no objection, I'll change the
cf status to "Ready for Committer" in few days.

Thanks for the review.

As I mentioned upthread, I have 2 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? IMO, we should not (for now), because this
would create a backward compatibility issue.
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 Sun, 07 Mar 2021 at 14:25, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

On Sun, Mar 7, 2021 at 11:49 AM Japin Li <japinli@hotmail.com> wrote:

On Fri, 05 Mar 2021 at 19:48, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

Attaching v5 patch set for further review.

The v5 patch looks good to me, if there is no objection, I'll change the
cf status to "Ready for Committer" in few days.

Thanks for the review.

As I mentioned upthread, I have 2 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? IMO, we should not (for now), because this
would create a backward compatibility issue.

Sorry, I do not know how PostgreSQL handle the backward compatibility issue.
Is there a guideline?

2) Do we document (under respective command pages or somewhere else)
that we allow explain/explain analyze for a command?

IMO, we can add a new page to list the commands that can be explain/explain analyze,
since it's clear for users.

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

On Sun, Mar 7, 2021 at 12:13 PM Japin Li <japinli@hotmail.com> wrote:

On Sun, 07 Mar 2021 at 14:25, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

On Sun, Mar 7, 2021 at 11:49 AM Japin Li <japinli@hotmail.com> wrote:

On Fri, 05 Mar 2021 at 19:48, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

Attaching v5 patch set for further review.

The v5 patch looks good to me, if there is no objection, I'll change the
cf status to "Ready for Committer" in few days.

Thanks for the review.

As I mentioned upthread, I have 2 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? IMO, we should not (for now), because this
would create a backward compatibility issue.

Sorry, I do not know how PostgreSQL handle the backward compatibility issue.
Is there a guideline?

I'm not aware of any guidelines as such, but we usually avoid any
changes to existing API, adding/making changes to system catalogs and
so on.

2) Do we document (under respective command pages or somewhere else)
that we allow explain/explain analyze for a command?

IMO, we can add a new page to list the commands that can be explain/explain analyze,
since it's clear for users.

We are listing all the supported commands in explain.sgml, so added
the CREATE MATERIALIZED VIEW(it's missing even though it's supported
prior to this patch) and REFRESH MATERIALIZED VIEW there.

Attaching v6 patch set. Please have a look.

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

On Sun, 07 Mar 2021 at 17:33, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

On Sun, Mar 7, 2021 at 12:13 PM Japin Li <japinli@hotmail.com> wrote:

On Sun, 07 Mar 2021 at 14:25, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

On Sun, Mar 7, 2021 at 11:49 AM Japin Li <japinli@hotmail.com> wrote:

On Fri, 05 Mar 2021 at 19:48, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

Attaching v5 patch set for further review.

The v5 patch looks good to me, if there is no objection, I'll change the
cf status to "Ready for Committer" in few days.

Thanks for the review.

As I mentioned upthread, I have 2 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? IMO, we should not (for now), because this
would create a backward compatibility issue.

Sorry, I do not know how PostgreSQL handle the backward compatibility issue.
Is there a guideline?

I'm not aware of any guidelines as such, but we usually avoid any
changes to existing API, adding/making changes to system catalogs and
so on.

Thanks for explaining, I'd be inclined keep it backward compatibility.

2) Do we document (under respective command pages or somewhere else)
that we allow explain/explain analyze for a command?

IMO, we can add a new page to list the commands that can be explain/explain analyze,
since it's clear for users.

We are listing all the supported commands in explain.sgml, so added
the CREATE MATERIALIZED VIEW(it's missing even though it's supported
prior to this patch) and REFRESH MATERIALIZED VIEW there.

Attaching v6 patch set. Please have a look.

LGTM.

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

Hi,

+        * EXPLAIN ANALYZE CREATE TABLE AS or REFRESH MATERIALIZED VIEW
+        * WITH NO DATA is weird.

Maybe it is clearer to spell out WITH NO DATA for both statements, instead
of sharing it.

-   if (!stmt->skipData)
+   if (!stmt->skipData && !explainInfo)
...
+   else if (explainInfo)

It would be cleaner to put the 'if (explainInfo)' as the first check. That
way, the check for skipData can be simplified.

Cheers

On Sun, Mar 7, 2021 at 1:34 AM Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:

On Sun, Mar 7, 2021 at 10:13 PM Zhihong Yu <zyu@yugabyte.com> wrote:

Hi,

+        * EXPLAIN ANALYZE CREATE TABLE AS or REFRESH MATERIALIZED VIEW
+        * WITH NO DATA is weird.

Maybe it is clearer to spell out WITH NO DATA for both statements, instead of sharing it.

Done that way.

-   if (!stmt->skipData)
+   if (!stmt->skipData && !explainInfo)
...
+   else if (explainInfo)

It would be cleaner to put the 'if (explainInfo)' as the first check. That way, the check for skipData can be simplified.

Changed.

Thanks for review comments. Attaching v7 patch set with changes only
in 0002 patch. Please have a look.

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