Thread: backend crash with FATAL: BeginInternalSubTransaction: unexpected state END

backend crash with FATAL: BeginInternalSubTransaction: unexpected state END

From
Frank van Vugt
Date:
Hi,

Migrating a number of sql-functions to plpgsql-functions with added
functionality resulted in a backend crash.

# select version();
                                version
------------------------------------------------------------------------
 PostgreSQL 8.2.4 on i686-pc-linux-gnu, compiled by GCC gcc (GCC) 3.4.3
(1 row)


The problem is easily reproduced by a copy&paste of the following code in a
terminal:

********************************************************
create table f1(id int);

CREATE OR REPLACE FUNCTION f1_crash()
RETURNS int
LANGUAGE 'plpgsql'
IMMUTABLE
STRICT
SECURITY INVOKER
AS '    DECLARE
        result INT := 0;
    BEGIN
        BEGIN
            SELECT INTO STRICT result 1;
            EXCEPTION
                WHEN NO_DATA_FOUND THEN
                    RAISE EXCEPTION ''Unknown record...!!'';
                WHEN TOO_MANY_ROWS THEN
                    RAISE EXCEPTION ''More than one record found...'';
        END;
        RETURN result;
    END;';

CREATE OR REPLACE FUNCTION tr_f1_def()
RETURNS trigger
LANGUAGE 'plpgsql'
VOLATILE
STRICT
SECURITY INVOKER
AS '    DECLARE
    BEGIN
        IF f1_crash() THEN
            RAISE NOTICE ''We got to here...'';
        END IF;
    RETURN NULL;
    END;';

CREATE CONSTRAINT TRIGGER f1_def AFTER INSERT ON f1 DEFERRABLE INITIALLY
DEFERRED FOR EACH ROW EXECUTE PROCEDURE tr_f1_def();
********************************************************

After which these statements will run ok:

    insert into f1 values (1);
    insert into f1 select * from generate_series(1, 1000);

However this will fail:

    begin;
    insert into f1 select * from generate_series(1, 1000);
    commit;

Resulting in:

FATAL:  BeginInternalSubTransaction: unexpected state END
CONTEXT:  PL/pgSQL function "f1_crash" line 4 at block variables
initialization
PL/pgSQL function "tr_f1_def" line 3 at if
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.
The connection to the server was lost. Attempting reset: Succeeded.


The problem seems to be caused by the nested BEGIN/END block in f1_crash(),
but we need that there in order to separate the RETURN from the EXCEPTION
block....


Cleanup:
********************************************************
drop table f1 cascade;
drop function tr_f1_def();
drop function f1_crash();
********************************************************


Looking forward to your remarks !



--
Best,





Frank.
Frank van Vugt <ftm.van.vugt@foxi.nl> writes:
> FATAL:  BeginInternalSubTransaction: unexpected state END

Hmm, do you get the impression that user-written constraint triggers
aren't very well tested ;-) ?

It looks to me like BeginInternalSubTransaction simply needs to allow
TBLOCK_END (and TBLOCK_PREPARE too) as acceptable initial states,
because these could be seen by a function executed during COMMIT or
PREPARE TRANSACTION-time processing of deferred triggers.  I think that
the other states it rejects are OK, because we don't try to execute any
user-written code while transiting through those TBLOCK states.

            regards, tom lane
> Hmm, do you get the impression that user-written constraint triggers
> aren't very well tested ;-) ?

Well, we users are here to serve ;)

> It looks to me like BeginInternalSubTransaction simply needs to allow
> TBLOCK_END (and TBLOCK_PREPARE too) as acceptable initial states

Ok, so for patch-sake, when I change BeginInternalSubTransaction() in xact.c
by moving these two cases to the upper set
(TBLOCK_STARTED/INPROGRESS/SUBINPROGRESS), then I should be ok?

At the moment, this looks like a showstopper, so I'd prefer patching and move
on ;)





--
Best,




Frank.
Frank van Vugt <ftm.van.vugt@foxi.nl> writes:
> Ok, so for patch-sake, when I change BeginInternalSubTransaction() in xact.c
> by moving these two cases to the upper set
> (TBLOCK_STARTED/INPROGRESS/SUBINPROGRESS), then I should be ok?

Try it and see ...

            regards, tom lane
> > Ok, so for patch-sake, when I change BeginInternalSubTransaction() in
> > xact.c by moving these two cases to the upper set
> > (TBLOCK_STARTED/INPROGRESS/SUBINPROGRESS), then I should be ok?

> Try it and see ...

Been there, done that, seems to work (both the example as well as my usecase).

Will keep an eye on the committers list to see what eventually makes it 'til
there. What did the trick for me was:


# diff -u src/backend/access/transam/xact.c_orig
src/backend/access/transam/xact.c
--- src/backend/access/transam/xact.c_orig      2007-05-30 17:38:10.000000000
+0200
+++ src/backend/access/transam/xact.c   2007-05-30 18:34:40.000000000 +0200
@@ -3319,6 +3319,8 @@
                case TBLOCK_STARTED:
                case TBLOCK_INPROGRESS:
                case TBLOCK_SUBINPROGRESS:
+               case TBLOCK_END:
+               case TBLOCK_PREPARE:
                        /* Normal subtransaction start */
                        PushTransaction();
                        s = CurrentTransactionState;            /* changed by
push */
@@ -3335,7 +3337,6 @@
                case TBLOCK_DEFAULT:
                case TBLOCK_BEGIN:
                case TBLOCK_SUBBEGIN:
-               case TBLOCK_END:
                case TBLOCK_SUBEND:
                case TBLOCK_ABORT:
                case TBLOCK_SUBABORT:
@@ -3345,7 +3346,6 @@
                case TBLOCK_SUBABORT_PENDING:
                case TBLOCK_SUBRESTART:
                case TBLOCK_SUBABORT_RESTART:
-               case TBLOCK_PREPARE:
                        elog(FATAL, "BeginInternalSubTransaction: unexpected
state %s",
                                 BlockStateAsString(s->blockState));
                        break;



So Tom, thanks a lot for your swift reply and solution !


--
Best,




Frank.
Frank van Vugt <ftm.van.vugt@foxi.nl> writes:
>>> Ok, so for patch-sake, when I change BeginInternalSubTransaction() in
>>> xact.c by moving these two cases to the upper set
>>> (TBLOCK_STARTED/INPROGRESS/SUBINPROGRESS), then I should be ok?

>> Try it and see ...

> Been there, done that, seems to work (both the example as well as my usecase).

Great, thanks for testing.  Will push the fix into CVS.

            regards, tom lane