REFRESH MATERIALIZED VIEW command in PL block hitting Assert

Started by Jeevan Chalkeover 12 years ago13 messages

jeevan.chalke@enterprisedb.com

over 12 years ago

Hi,

I have observed that following sequence is causing server crash.

CREATE MATERIALIZED VIEW temp_class_mv AS
SELECT * FROM pg_class
WITH NO DATA;

CREATE OR REPLACE FUNCTION test_refresh_mv()
RETURNS int
AS $$
BEGIN
REFRESH MATERIALIZED VIEW temp_class_mv;
return 1;
END; $$ LANGUAGE plpgsql;

SELECT test_refresh_mv();

I had a quick look over the crash and it is hitting following Assert in
spi.c:

else if (IsA(stmt, RefreshMatViewStmt))
{
Assert(strncmp(completionTag,
"REFRESH MATERIALIZED VIEW ", 23) == 0);
_SPI_current->processed = strtoul(completionTag + 23,
NULL, 10);
}

It seems like we are missing expected value for completionTag in
ExecRefreshMatView()

Thanks

--
Jeevan B Chalke
Senior Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. If you are
not the intended recipient or authorized to receive this for the intended
recipient, any use, dissemination, distribution, retention, archiving, or
copying of this communication is strictly prohibited. If you have received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.

Andres Freund

andres@2ndquadrant.com

over 12 years ago

In reply to: Jeevan Chalke (#1)

Re: REFRESH MATERIALIZED VIEW command in PL block hitting Assert

On 2013-04-22 18:35:04 +0530, Jeevan Chalke wrote:

Hi,

I have observed that following sequence is causing server crash.

CREATE MATERIALIZED VIEW temp_class_mv AS
SELECT * FROM pg_class
WITH NO DATA;

CREATE OR REPLACE FUNCTION test_refresh_mv()
RETURNS int
AS $$
BEGIN
REFRESH MATERIALIZED VIEW temp_class_mv;
return 1;
END; $$ LANGUAGE plpgsql;

SELECT test_refresh_mv();

I had a quick look over the crash and it is hitting following Assert in
spi.c:

else if (IsA(stmt, RefreshMatViewStmt))
{
Assert(strncmp(completionTag,
"REFRESH MATERIALIZED VIEW ", 23) == 0);
_SPI_current->processed = strtoul(completionTag + 23,
NULL, 10);
}

It seems like we are missing expected value for completionTag in
ExecRefreshMatView()

Possibly independent from this issue, but where did that 23 come from?
ISTM we're strtoul()ing "EW somenumber" here.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Jeevan Chalke

jeevan.chalke@enterprisedb.com

over 12 years ago

In reply to: Andres Freund (#2)

1 attachment(s)

Re: REFRESH MATERIALIZED VIEW command in PL block hitting Assert

On Mon, Apr 22, 2013 at 6:41 PM, Andres Freund <andres@2ndquadrant.com>wrote:

On 2013-04-22 18:35:04 +0530, Jeevan Chalke wrote:

Hi,

I have observed that following sequence is causing server crash.

CREATE MATERIALIZED VIEW temp_class_mv AS
SELECT * FROM pg_class
WITH NO DATA;

CREATE OR REPLACE FUNCTION test_refresh_mv()
RETURNS int
AS $$
BEGIN
REFRESH MATERIALIZED VIEW temp_class_mv;
return 1;
END; $$ LANGUAGE plpgsql;

SELECT test_refresh_mv();

I had a quick look over the crash and it is hitting following Assert in
spi.c:

else if (IsA(stmt, RefreshMatViewStmt))
{
Assert(strncmp(completionTag,
"REFRESH MATERIALIZED VIEW ", 23) ==

0);

_SPI_current->processed = strtoul(completionTag + 23,
NULL, 10);
}

It seems like we are missing expected value for completionTag in
ExecRefreshMatView()

Possibly independent from this issue, but where did that 23 come from?

23 is also bogus here.
It should be 26 i.e. length of "REFRESH MATERIALIZED VIEW "

BTW, attached is the patch which works well for me, but need details review.

Thanks

ISTM we're strtoul()ing "EW somenumber" here.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Jeevan B Chalke
Senior Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

Attachments:

mv_crash.patchapplication/octet-stream; name=mv_crash.patchDownload

diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index ac7719e..333d3cc 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -48,8 +48,8 @@ static void transientrel_startup(DestReceiver *self, int operation, TupleDesc ty
 static void transientrel_receive(TupleTableSlot *slot, DestReceiver *self);
 static void transientrel_shutdown(DestReceiver *self);
 static void transientrel_destroy(DestReceiver *self);
-static void refresh_matview_datafill(DestReceiver *dest, Query *query,
-									 const char *queryString);
+static uint32 refresh_matview_datafill(DestReceiver *dest, Query *query,
+									   const char *queryString);
 
 /*
  * SetMatViewToPopulated
@@ -118,6 +118,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
 	Oid			tableSpace;
 	Oid			OIDNewHeap;
 	DestReceiver *dest;
+	uint32		nprocessed = 0;
 
 	/*
 	 * Get a lock until end of transaction.
@@ -193,7 +194,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
 	dest = CreateTransientRelDestReceiver(OIDNewHeap);
 
 	if (!stmt->skipData)
-		refresh_matview_datafill(dest, dataQuery, queryString);
+		nprocessed = refresh_matview_datafill(dest, dataQuery, queryString);
 
 	/*
 	 * Swap the physical files of the target and transient tables, then
@@ -202,13 +203,21 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
 	finish_heap_swap(matviewOid, OIDNewHeap, false, false, true, true,
 					 RecentXmin, ReadNextMultiXactId());
 
+	/* Set back completionTag */
+	if (completionTag)
+	{
+		snprintf(completionTag, COMPLETION_TAG_BUFSIZE,
+				 "REFRESH MATERIALIZED VIEW %u", nprocessed);
+elog(INFO, "nprocessed: %u", nprocessed);
+	}
+
 	RelationCacheInvalidateEntry(matviewOid);
 }
 
 /*
  * refresh_matview_datafill
  */
-static void
+static uint32
 refresh_matview_datafill(DestReceiver *dest, Query *query,
 						 const char *queryString)
 {
@@ -218,6 +227,7 @@ refresh_matview_datafill(DestReceiver *dest, Query *query,
 	List	   *rtable;
 	RangeTblEntry	*initial_rte;
 	RangeTblEntry	*second_rte;
+	uint32		nprocessed;
 
 	rewritten = QueryRewrite((Query *) copyObject(query));
 
@@ -271,6 +281,7 @@ refresh_matview_datafill(DestReceiver *dest, Query *query,
 
 	/* run the plan */
 	ExecutorRun(queryDesc, ForwardScanDirection, 0L);
+	nprocessed = queryDesc->estate->es_processed;
 
 	/* and clean up */
 	ExecutorFinish(queryDesc);
@@ -279,6 +290,8 @@ refresh_matview_datafill(DestReceiver *dest, Query *query,
 	FreeQueryDesc(queryDesc);
 
 	PopActiveSnapshot();
+
+	return nprocessed;
 }
 
 DestReceiver *
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index cc7764d..63862cb 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2125,8 +2125,8 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
 				else if (IsA(stmt, RefreshMatViewStmt))
 				{
 					Assert(strncmp(completionTag,
-								   "REFRESH MATERIALIZED VIEW ", 23) == 0);
-					_SPI_current->processed = strtoul(completionTag + 23,
+								   "REFRESH MATERIALIZED VIEW ", 26) == 0);
+					_SPI_current->processed = strtoul(completionTag + 26,
 													  NULL, 10);
 				}
 				else if (IsA(stmt, CopyStmt))

Jeevan Chalke

jeevan.chalke@enterprisedb.com

over 12 years ago

In reply to: Jeevan Chalke (#3)

Re: REFRESH MATERIALIZED VIEW command in PL block hitting Assert

Hi Tom,

Since we are close to release, we should not have crashes like this.

Please have a look. My patch may not be correct as I haven't looked closely.

Thanks

On Mon, Apr 22, 2013 at 7:28 PM, Jeevan Chalke <
jeevan.chalke@enterprisedb.com> wrote:

On Mon, Apr 22, 2013 at 6:41 PM, Andres Freund <andres@2ndquadrant.com>wrote:

On 2013-04-22 18:35:04 +0530, Jeevan Chalke wrote:

Hi,

I have observed that following sequence is causing server crash.

CREATE MATERIALIZED VIEW temp_class_mv AS
SELECT * FROM pg_class
WITH NO DATA;

CREATE OR REPLACE FUNCTION test_refresh_mv()
RETURNS int
AS $$
BEGIN
REFRESH MATERIALIZED VIEW temp_class_mv;
return 1;
END; $$ LANGUAGE plpgsql;

SELECT test_refresh_mv();

I had a quick look over the crash and it is hitting following Assert in
spi.c:

else if (IsA(stmt, RefreshMatViewStmt))
{
Assert(strncmp(completionTag,
"REFRESH MATERIALIZED VIEW ", 23) ==

0);

_SPI_current->processed = strtoul(completionTag +

23,

NULL, 10);
}

It seems like we are missing expected value for completionTag in
ExecRefreshMatView()

Possibly independent from this issue, but where did that 23 come from?

23 is also bogus here.
It should be 26 i.e. length of "REFRESH MATERIALIZED VIEW "

BTW, attached is the patch which works well for me, but need details
review.

Thanks

ISTM we're strtoul()ing "EW somenumber" here.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Jeevan B Chalke
Senior Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. If you are
not the intended recipient or authorized to receive this for the intended
recipient, any use, dissemination, distribution, retention, archiving, or
copying of this communication is strictly prohibited. If you have received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.

--
Jeevan B Chalke
Senior Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

Andres Freund

andres@2ndquadrant.com

over 12 years ago

In reply to: Jeevan Chalke (#4)

Re: REFRESH MATERIALIZED VIEW command in PL block hitting Assert

Hi,

On 2013-04-23 17:48:49 +0530, Jeevan Chalke wrote:

Hi Tom,

Since we are close to release, we should not have crashes like this.

Please have a look. My patch may not be correct as I haven't looked closely.

Isn't that Kevin's departement?

Andres

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Kevin Grittner

kgrittn@ymail.com

over 12 years ago

In reply to: Andres Freund (#5)

Re: REFRESH MATERIALIZED VIEW command in PL block hitting Assert

Andres Freund <andres@2ndquadrant.com> wrote:

On 2013-04-23 17:48:49 +0530, Jeevan Chalke wrote:

Hi Tom,

Since we are close to release, we should not have crashes like
this.

Please have a look. My patch may not be correct as I haven't
looked closely.

Isn't that Kevin's departement?

I'll gladly pick this up. Tom had moved some code around and
dropped a couple comments that made it sound like he was still
working on it, so I was trying to stay out of his way; but if I've
misinterpreted that, I can jump in here.

--
Kevin Grittner
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Merlin Moncure

mmoncure@gmail.com

over 12 years ago

In reply to: Jeevan Chalke (#4)

Re: REFRESH MATERIALIZED VIEW command in PL block hitting Assert

On Tue, Apr 23, 2013 at 7:18 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:

Hi Tom,

Since we are close to release, we should not have crashes like this.

huh? we are not even in beta yet....

merlin

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Jeevan Chalke

jeevan.chalke@enterprisedb.com

over 12 years ago

In reply to: Merlin Moncure (#7)

Re: REFRESH MATERIALIZED VIEW command in PL block hitting Assert

On Tue, Apr 23, 2013 at 7:01 PM, Merlin Moncure <mmoncure@gmail.com> wrote:

On Tue, Apr 23, 2013 at 7:18 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:

Hi Tom,

Since we are close to release, we should not have crashes like this.

huh? we are not even in beta yet....

I mean, beta release.

BTW, as per Bruce's mail "we are planning to package 9.3
beta1 on *April 29*, with a release on May 2", we are close enough.

Thanks

merlin

--
Jeevan B Chalke
Senior Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

Andres Freund

andres@2ndquadrant.com

over 12 years ago

In reply to: Jeevan Chalke (#8)

Re: REFRESH MATERIALIZED VIEW command in PL block hitting Assert

On 2013-04-23 19:33:24 +0530, Jeevan Chalke wrote:

On Tue, Apr 23, 2013 at 7:01 PM, Merlin Moncure <mmoncure@gmail.com> wrote:

On Tue, Apr 23, 2013 at 7:18 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:

Hi Tom,

Since we are close to release, we should not have crashes like this.

huh? we are not even in beta yet....

I mean, beta release.

BTW, as per Bruce's mail "we are planning to package 9.3
beta1 on *April 29*, with a release on May 2", we are close enough.

The 2nd May date is for the release of packaged beta, not overall
release unless I missed something major like months of testing ;)

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#10

Kevin Grittner

kgrittn@ymail.com

over 12 years ago

In reply to: Jeevan Chalke (#3)

1 attachment(s)

Re: REFRESH MATERIALIZED VIEW command in PL block hitting Assert

Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote:

On Mon, Apr 22, 2013 at 6:41 PM, Andres Freund <andres@2ndquadrant.com> wrote:

On 2013-04-22 18:35:04 +0530, Jeevan Chalke wrote:

I have observed that following sequence is causing server crash.

CREATE MATERIALIZED VIEW temp_class_mv AS
   SELECT * FROM pg_class
   WITH NO DATA;

CREATE OR REPLACE FUNCTION test_refresh_mv()
RETURNS int
AS $$
BEGIN
REFRESH MATERIALIZED VIEW temp_class_mv;
return 1;
END; $$ LANGUAGE plpgsql;

SELECT test_refresh_mv();

I had a quick look over the crash and it is hitting following Assert in
spi.c:

                 else if (IsA(stmt, RefreshMatViewStmt))
                 {
                     Assert(strncmp(completionTag,
                                    "REFRESH MATERIALIZED VIEW ", 23) == 0);
                     _SPI_current->processed = strtoul(completionTag + 23,
                                                       NULL, 10);
                 }

It seems like we are missing expected value for completionTag in
ExecRefreshMatView()

Possibly independent from this issue, but where did that 23 come from?

When the consensus developed to change the syntax from LOAD
MATERIALIZED VIEW I failed to noticed the length here when making
the changes for that.

BTW, attached is the patch which works well for me, but need details review.

I suggest that we just rip out this section of code. Trying to
provide a number here is probably all wrong, anyway. As the
features evolve, there may not be a readily accessible rowcount for
this command in all cases.

Any objections to the attached to fix this issue?

--
Kevin Grittner
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachments:

mv_crash-v2.patchtext/x-patch; name=mv_crash-v2.patchDownload

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index cc7764d..de8d59a 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2122,13 +2122,6 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
 					if (((CreateTableAsStmt *) stmt)->is_select_into)
 						res = SPI_OK_SELINTO;
 				}
-				else if (IsA(stmt, RefreshMatViewStmt))
-				{
-					Assert(strncmp(completionTag,
-								   "REFRESH MATERIALIZED VIEW ", 23) == 0);
-					_SPI_current->processed = strtoul(completionTag + 23,
-													  NULL, 10);
-				}
 				else if (IsA(stmt, CopyStmt))
 				{
 					Assert(strncmp(completionTag, "COPY ", 5) == 0);

#11

Michael Paquier

michael.paquier@gmail.com

over 12 years ago

In reply to: Andres Freund (#9)

Re: REFRESH MATERIALIZED VIEW command in PL block hitting Assert

On Tue, Apr 23, 2013 at 11:05 PM, Andres Freund <andres@2ndquadrant.com>wrote:

On 2013-04-23 19:33:24 +0530, Jeevan Chalke wrote:

On Tue, Apr 23, 2013 at 7:01 PM, Merlin Moncure <mmoncure@gmail.com>

wrote:

On Tue, Apr 23, 2013 at 7:18 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:

Hi Tom,

Since we are close to release, we should not have crashes like this.

huh? we are not even in beta yet....

I mean, beta release.

BTW, as per Bruce's mail "we are planning to package 9.3
beta1 on *April 29*, with a release on May 2", we are close enough.

The 2nd May date is for the release of packaged beta, not overall
release unless I missed something major like months of testing ;)

Last year 9.2.0 was released at the beginning of September, so there would
be at least 4 months to test and improve 9.3 during beta assuming next
release is made on the same timeline.
--
Michael

#12

Jeevan Chalke

jeevan.chalke@enterprisedb.com

over 12 years ago

In reply to: Kevin Grittner (#10)

Re: REFRESH MATERIALIZED VIEW command in PL block hitting Assert

On Wed, Apr 24, 2013 at 3:04 AM, Kevin Grittner <kgrittn@ymail.com> wrote:

Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote:

On Mon, Apr 22, 2013 at 6:41 PM, Andres Freund <andres@2ndquadrant.com>

wrote:

On 2013-04-22 18:35:04 +0530, Jeevan Chalke wrote:

I have observed that following sequence is causing server crash.

CREATE MATERIALIZED VIEW temp_class_mv AS
SELECT * FROM pg_class
WITH NO DATA;

CREATE OR REPLACE FUNCTION test_refresh_mv()
RETURNS int
AS $$
BEGIN
REFRESH MATERIALIZED VIEW temp_class_mv;
return 1;
END; $$ LANGUAGE plpgsql;

SELECT test_refresh_mv();

I had a quick look over the crash and it is hitting following Assert in
spi.c:

else if (IsA(stmt, RefreshMatViewStmt))
{
Assert(strncmp(completionTag,
"REFRESH MATERIALIZED VIEW ", 23)

== 0);

_SPI_current->processed = strtoul(completionTag +

23,

NULL, 10);
}

It seems like we are missing expected value for completionTag in
ExecRefreshMatView()

Possibly independent from this issue, but where did that 23 come from?

When the consensus developed to change the syntax from LOAD
MATERIALIZED VIEW I failed to noticed the length here when making
the changes for that.

BTW, attached is the patch which works well for me, but need details

review.

I suggest that we just rip out this section of code. Trying to
provide a number here is probably all wrong, anyway. As the
features evolve, there may not be a readily accessible rowcount for
this command in all cases.

OK.

Any objections to the attached to fix this issue?

Nope. Fine with me.

Thanks

--
Kevin Grittner
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Jeevan B Chalke
Senior Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

#13

Kevin Grittner

kgrittn@ymail.com

over 12 years ago

In reply to: Jeevan Chalke (#12)

Re: REFRESH MATERIALIZED VIEW command in PL block hitting Assert

Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote:

On Wed, Apr 24, 2013 at 3:04 AM, Kevin Grittner <kgrittn@ymail.com> wrote:

Any objections to the attached to fix this issue?

Nope. Fine with me.

Pushed. Thanks for the report!

--
Kevin Grittner
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers