Thread: 7.2 crash...

7.2 crash...

From
"Rod Taylor"
Date:
7.2 crashes with the below function:

CREATE OR REPLACE FUNCTION runMaintenance()
RETURNS BOOL AS '
  VACUUM;
  SELECT TRUE;
' LANGUAGE sql;

I was going to toss a bunch of system maintenance stuff in a database
function to make administration for those who administer the boxes
(not me -- I just tell how).

Anyway, any crash is a bad crash I suppose.
--
Rod Taylor

Your eyes are weary from staring at the CRT. You feel sleepy. Notice
how restful it is to watch the cursor blink. Close your eyes. The
opinions stated above are yours. You cannot imagine why you ever felt
otherwise.

Re: 7.2 crash...

From
Tom Lane
Date:
"Rod Taylor" <rbt@zort.ca> writes:
> 7.2 crashes with the below function:

> CREATE OR REPLACE FUNCTION runMaintenance()
> RETURNS BOOL AS '
>   VACUUM;
>   SELECT TRUE;
> ' LANGUAGE sql;

Ugh.  The problem is that VACUUM's implicit CommitTransaction calls
wipe out all the transient memory allocated by the function evaluation.
I don't see any reasonable way to support VACUUM inside a function
call; I think we have to prohibit it.

Unfortunately I don't see any clean way to test for this situation
either.  VACUUM's IsTransactionBlock() test obviously doesn't get the
job done.  Any ideas how to catch this?
        regards, tom lane


Re: 7.2 crash...

From
Tom Lane
Date:
"Rod Taylor" <rbt@zort.ca> writes:
> 7.2 crashes with the below function:
> CREATE OR REPLACE FUNCTION runMaintenance()
> RETURNS BOOL AS '
>   VACUUM;
>   SELECT TRUE;
> ' LANGUAGE sql;

AFAICS there is no way that we can support VACUUM inside a function;
the forced transaction commits that VACUUM performs will recycle any
memory allocated by the function executor, leading to death and
destruction upon return from VACUUM.

Accordingly, what we really need is a way of preventing VACUUM from
executing in the above scenario.  The IsTransactionBlock() test it
already has isn't sufficient.

I have thought of something that probably would be sufficient:

    if (!MemoryContextContains(QueryContext, vacstmt))
        elog(ERROR, "VACUUM cannot be executed from a function");

This is truly, horribly ugly ... but it'd get the job done, because only
interactive queries will generate parsetrees in QueryContext.

Can someone think of a better way?

            regards, tom lane

Re: 7.2 crash...

From
Bruce Momjian
Date:
Tom Lane wrote:
> "Rod Taylor" <rbt@zort.ca> writes:
> > 7.2 crashes with the below function:
> > CREATE OR REPLACE FUNCTION runMaintenance()
> > RETURNS BOOL AS '
> >   VACUUM;
> >   SELECT TRUE;
> > ' LANGUAGE sql;
>
> AFAICS there is no way that we can support VACUUM inside a function;
> the forced transaction commits that VACUUM performs will recycle any
> memory allocated by the function executor, leading to death and
> destruction upon return from VACUUM.
>
> Accordingly, what we really need is a way of preventing VACUUM from
> executing in the above scenario.  The IsTransactionBlock() test it
> already has isn't sufficient.
>
> I have thought of something that probably would be sufficient:
>
>     if (!MemoryContextContains(QueryContext, vacstmt))
>         elog(ERROR, "VACUUM cannot be executed from a function");
>
> This is truly, horribly ugly ... but it'd get the job done, because only
> interactive queries will generate parsetrees in QueryContext.
>
> Can someone think of a better way?

Well, this could would be in vacuum.c, right?  Seems like a nice
central location for it.  I don't see it as terribly ugly only because
the issue is that we can't run vacuum inside a memory context that can't
be free'ed by vacuum.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Re: 7.2 crash...

From
Bruce Momjian
Date:
Here is a patch with a fix outlined by Tom:

    test=> CREATE OR REPLACE FUNCTION runMaintenance()
    test-> RETURNS BOOL AS '
    test'>   VACUUM;
    test'>   SELECT TRUE;
    test'> ' LANGUAGE sql;
    CREATE
    test=>
    test=> select runMaintenance();
    ERROR:  VACUUM cannot be executed from a function

Looks good.  Will commit after typical delay.

---------------------------------------------------------------------------

Tom Lane wrote:
> "Rod Taylor" <rbt@zort.ca> writes:
> > 7.2 crashes with the below function:
> > CREATE OR REPLACE FUNCTION runMaintenance()
> > RETURNS BOOL AS '
> >   VACUUM;
> >   SELECT TRUE;
> > ' LANGUAGE sql;
>
> AFAICS there is no way that we can support VACUUM inside a function;
> the forced transaction commits that VACUUM performs will recycle any
> memory allocated by the function executor, leading to death and
> destruction upon return from VACUUM.
>
> Accordingly, what we really need is a way of preventing VACUUM from
> executing in the above scenario.  The IsTransactionBlock() test it
> already has isn't sufficient.
>
> I have thought of something that probably would be sufficient:
>
>     if (!MemoryContextContains(QueryContext, vacstmt))
>         elog(ERROR, "VACUUM cannot be executed from a function");
>
> This is truly, horribly ugly ... but it'd get the job done, because only
> interactive queries will generate parsetrees in QueryContext.
>
> Can someone think of a better way?
>
>             regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/users-lounge/docs/faq.html
>

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026
Index: src/backend/commands/vacuum.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/vacuum.c,v
retrieving revision 1.223
diff -c -r1.223 vacuum.c
*** src/backend/commands/vacuum.c    12 Apr 2002 20:38:25 -0000    1.223
--- src/backend/commands/vacuum.c    14 Apr 2002 16:41:37 -0000
***************
*** 181,186 ****
--- 181,189 ----
      if (IsTransactionBlock())
          elog(ERROR, "%s cannot run inside a BEGIN/END block", stmttype);

+     if (!MemoryContextContains(QueryContext, vacstmt))
+         elog(ERROR, "VACUUM cannot be executed from a function");
+
      /*
       * Send info about dead objects to the statistics collector
       */

Re: 7.2 crash...

From
Tom Lane
Date:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> *** src/backend/commands/vacuum.c    12 Apr 2002 20:38:25 -0000    1.223
> --- src/backend/commands/vacuum.c    14 Apr 2002 16:41:37 -0000
> ***************
> *** 181,186 ****
> --- 181,189 ----
>       if (IsTransactionBlock())
>           elog(ERROR, "%s cannot run inside a BEGIN/END block", stmttype);

> +     if (!MemoryContextContains(QueryContext, vacstmt))
> +         elog(ERROR, "VACUUM cannot be executed from a function");
> +
>       /*
>        * Send info about dead objects to the statistics collector
>        */

> --ELM1018803173-10746-0_--

Compare to immediately preceding error check.  Isn't there something
missing here?

            regards, tom lane

Re: 7.2 crash...

From
Bruce Momjian
Date:
Oops, I see now.  How is this?

Remember, I am not incredibly capable, just persistent.  :-)

---------------------------------------------------------------------------

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > *** src/backend/commands/vacuum.c    12 Apr 2002 20:38:25 -0000    1.223
> > --- src/backend/commands/vacuum.c    14 Apr 2002 16:41:37 -0000
> > ***************
> > *** 181,186 ****
> > --- 181,189 ----
> >       if (IsTransactionBlock())
> >           elog(ERROR, "%s cannot run inside a BEGIN/END block", stmttype);
>
> > +     if (!MemoryContextContains(QueryContext, vacstmt))
> > +         elog(ERROR, "VACUUM cannot be executed from a function");
> > +
> >       /*
> >        * Send info about dead objects to the statistics collector
> >        */
>
> > --ELM1018803173-10746-0_--
>
> Compare to immediately preceding error check.  Isn't there something
> missing here?
>
>             regards, tom lane
>

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026
Index: src/backend/commands/vacuum.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/vacuum.c,v
retrieving revision 1.223
diff -c -r1.223 vacuum.c
*** src/backend/commands/vacuum.c    12 Apr 2002 20:38:25 -0000    1.223
--- src/backend/commands/vacuum.c    14 Apr 2002 16:41:37 -0000
***************
*** 181,186 ****
--- 181,189 ----
      if (IsTransactionBlock())
          elog(ERROR, "%s cannot run inside a BEGIN/END block", stmttype);

+     if (!MemoryContextContains(QueryContext, vacstmt))
+         elog(ERROR, "%s cannot be executed from a function", stmttype);
+
      /*
       * Send info about dead objects to the statistics collector
       */

Re: 7.2 crash...

From
Tom Lane
Date:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Oops, I see now.  How is this?

Better.  A comment explaining what the thing is doing would help too.

            regards, tom lane

Re: 7.2 crash...

From
Bruce Momjian
Date:
I have applied a patch based on Tom's suggestion that will disable
VACUUM in a function in 7.3.

---------------------------------------------------------------------------

Rod Taylor wrote:
> 7.2 crashes with the below function:
>
> CREATE OR REPLACE FUNCTION runMaintenance()
> RETURNS BOOL AS '
>   VACUUM;
>   SELECT TRUE;
> ' LANGUAGE sql;
>
> I was going to toss a bunch of system maintenance stuff in a database
> function to make administration for those who administer the boxes
> (not me -- I just tell how).
>
> Anyway, any crash is a bad crash I suppose.
> --
> Rod Taylor
>
> Your eyes are weary from staring at the CRT. You feel sleepy. Notice
> how restful it is to watch the cursor blink. Close your eyes. The
> opinions stated above are yours. You cannot imagine why you ever felt
> otherwise.
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/users-lounge/docs/faq.html
>

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026