Обсуждение: 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

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

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

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

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

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

<div dir="ltr"><br /><div class="gmail_extra"><br /><br /><div class="gmail_quote">On Tue, Apr 23, 2013 at 11:05 PM,
AndresFreund <span dir="ltr"><<a href="mailto:andres@2ndquadrant.com"
target="_blank">andres@2ndquadrant.com</a>></span>wrote:<br /><blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px#ccc solid;padding-left:1ex"><div class="im">On 2013-04-23 19:33:24 +0530, Jeevan Chalke wrote:<br
/>> On Tue, Apr 23, 2013 at 7:01 PM, Merlin Moncure <<a
href="mailto:mmoncure@gmail.com">mmoncure@gmail.com</a>>wrote:<br /> ><br /> > > On Tue, Apr 23, 2013 at
7:18AM, Jeevan Chalke<br /> > > <<a
href="mailto:jeevan.chalke@enterprisedb.com">jeevan.chalke@enterprisedb.com</a>>wrote:<br /> > > > Hi
Tom,<br/> > > ><br /> > > > Since we are close to release, we should not have crashes like this.<br
/>> ><br /> > > huh? we are not even in beta yet....<br /> > ><br /> ><br /> > I mean, beta
release.<br/> ><br /> > BTW, as per Bruce's mail "we are planning to package 9.3<br /></div>> beta1 on *April
29*,with a release on May 2", we are close enough.<br /><br /> The 2nd May date is for the release of packaged beta,
notoverall<br /> release unless I missed something major like months of testing ;)<br /></blockquote></div>Last year
9.2.0was released at the beginning of September, so there would be at least 4 months to test and improve 9.3 during
betaassuming next release is made on the same timeline.<br /> -- <br />Michael<br /></div></div> 

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