Re: assert in nested SQL procedure call in current HEAD
On 12.07.18 18:43, Andres Freund wrote: > On 2018-06-29 13:52:23 +0200, Peter Eisentraut wrote: >> From 95fc7156afe521b715fab08d44606774df875e92 Mon Sep 17 00:00:00 2001 >> From: Peter Eisentraut >> Date: Fri, 29 Jun 2018 13:28:39 +0200 >> Subject: [PATCH] Fix assert in nested SQL procedure call > > Andrew, Peter, are you happy with this? It'd be good to close this open > item. Yes, it's done. I'm going to update the wiki. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: assert in nested SQL procedure call in current HEAD
On 12.06.18 18:47, Andrew Gierth wrote: > While testing this, I ran into another semi-related issue: > shmem_exit_inprogress isn't ever being cleared in the postmaster, which > means that if you ever have a crash-restart, any attempt to do a > rollback in a procedure will then crash or get some other form of > corruption again every time until you manually restart the cluster. I have committed a fix for this. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: assert in nested SQL procedure call in current HEAD
Hi, On 2018-06-29 13:52:23 +0200, Peter Eisentraut wrote: > From 95fc7156afe521b715fab08d44606774df875e92 Mon Sep 17 00:00:00 2001 > From: Peter Eisentraut > Date: Fri, 29 Jun 2018 13:28:39 +0200 > Subject: [PATCH] Fix assert in nested SQL procedure call Andrew, Peter, are you happy with this? It'd be good to close this open item. - Andres
Re: assert in nested SQL procedure call in current HEAD
On 12.06.18 18:47, Andrew Gierth wrote: > While testing this, I ran into another semi-related issue: > shmem_exit_inprogress isn't ever being cleared in the postmaster, which > means that if you ever have a crash-restart, any attempt to do a > rollback in a procedure will then crash or get some other form of > corruption again every time until you manually restart the cluster. I think we could unset shmem_exit_inprogress at the end of shmem_exit(). I'm trying to remember why we didn't just use proc_exit_inprogress for this. I'll need to test this more. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: assert in nested SQL procedure call in current HEAD
On 6/27/18 18:51, Peter Eisentraut wrote:
> On 6/27/18 17:44, Andres Freund wrote:
>> This hasn't progressed in a while. Peter, since you committed the
>> relevant change, could you update us please?
>
> I've been on vacation for a bit. I will work on this next. I hope to
> have a solution in a few days.
Proposed patch attached.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 95fc7156afe521b715fab08d44606774df875e92 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Fri, 29 Jun 2018 13:28:39 +0200
Subject: [PATCH] Fix assert in nested SQL procedure call
When executing CALL in PL/pgSQL, we need to set a snapshot before
invoking the to-be-called procedure. Otherwise, the to-be-called
procedure might end up running without a snapshot. For LANGUAGE SQL
procedures, this would result in an assertion failure. (For most other
languages, this is usually not a problem, because those use SPI and SPI
sets snapshots in most cases.) Setting the snapshot restores the
behavior of how CALL worked when it was handled as a generic SQL
statement in PL/pgSQL (exec_stmt_execsql()).
This change revealed another problem: In SPI_commit(), we popped the
active snapshot before committing the transaction, to avoid "snapshot %p
still active" errors. However, there is no particular reason why only
at most one snapshot should be on the stack. So change this to pop all
active snapshots instead of only one.
---
src/backend/executor/spi.c| 7 +++-
.../src/expected/plpgsql_transaction.out | 19 +++
src/pl/plpgsql/src/pl_exec.c | 32 ---
.../plpgsql/src/sql/plpgsql_transaction.sql | 20
4 files changed, 72 insertions(+), 6 deletions(-)
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 22dd55c378..5756365c8f 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -228,8 +228,13 @@ SPI_commit(void)
_SPI_current->internal_xact = true;
- if (ActiveSnapshotSet())
+ /*
+* Before committing, pop all active snapshots to avoid error about
+* "snapshot %p still active".
+*/
+ while (ActiveSnapshotSet())
PopActiveSnapshot();
+
CommitTransactionCommand();
MemoryContextSwitchTo(oldcontext);
diff --git a/src/pl/plpgsql/src/expected/plpgsql_transaction.out
b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
index 7f008ac57e..274b2c6f17 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_transaction.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
@@ -432,6 +432,25 @@ END;
$$;
ERROR: EXECUTE of transaction commands is not implemented
CONTEXT: PL/pgSQL function inline_code_block line 3 at EXECUTE
+-- snapshot handling test
+TRUNCATE test2;
+CREATE PROCEDURE transaction_test9()
+LANGUAGE SQL
+AS $$
+INSERT INTO test2 VALUES (42);
+$$;
+DO LANGUAGE plpgsql $$
+BEGIN
+ ROLLBACK;
+ CALL transaction_test9();
+END
+$$;
+SELECT * FROM test2;
+ x
+
+ 42
+(1 row)
+
DROP TABLE test1;
DROP TABLE test2;
DROP TABLE test3;
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 66ecf5eb55..e39f7357bd 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2075,6 +2075,7 @@ exec_stmt_call(PLpgSQL_execstate *estate,
PLpgSQL_stmt_call *stmt)
ParamListInfo paramLI;
LocalTransactionId before_lxid;
LocalTransactionId after_lxid;
+ boolpushed_active_snap = false;
int rc;
if (expr->plan == NULL)
@@ -2090,6 +2091,7 @@ exec_stmt_call(PLpgSQL_execstate *estate,
PLpgSQL_stmt_call *stmt)
/*
* The procedure call could end transactions, which would upset
the
* snapshot management in SPI_execute*, so don't let it do it.
+* Instead, we set the snapshots ourselves below.
*/
expr->plan->no_snapshots = true;
}
@@ -2098,6 +2100,16 @@ exec_stmt_call(PLpgSQL_execstate *estate,
PLpgSQL_stmt_call *stmt)
before_lxid = MyProc->lxid;
+ /*
+* Set snapshot only for non-read-only procedures, similar to SPI
+* behavior.
+*/
+ if (!estate->readonly_func)
+ {
+ PushActiveSnapshot(GetTransactionSnapshot());
+ pushed_active_snap = true;
+ }
+
PG_TRY();
{
rc = SPI_execute_plan_with_paramlist(expr->plan, paramLI,
@@ -2126,12 +2138,22 @@ exec_stmt_call(PLpgSQL_execstate *estate,
PLpgSQL_stmt_call *stmt)
elog(ERROR, "SPI_execute_plan_with_paramlist failed executing
query \"%s\": %s",
expr->query, SPI_result_code_string(rc));
- /*
-* If we are in a new transaction after the call, we need to reset some
-* internal state.
-*/
- if (befo
Re: assert in nested SQL procedure call in current HEAD
On 6/27/18 17:44, Andres Freund wrote: > This hasn't progressed in a while. Peter, since you committed the > relevant change, could you update us please? I've been on vacation for a bit. I will work on this next. I hope to have a solution in a few days. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: assert in nested SQL procedure call in current HEAD
Hi Peter, On 2018-06-10 21:06:59 +0200, Dmitry Dolgov wrote: > > On 8 June 2018 at 06:20, Andrew Gierth wrote: > > > Joe> My colleague Yogesh Sharma discovered an assert in nested SQL > > Joe> procedure calls after ROLLBACK is used. Minimal test case and > > Joe> backtrace below. I have not yet tried to figure out exactly what > > Joe> is going on beyond seeing that it occurs in pg_plan_query() where > > Joe> the comment says "Planner must have a snapshot in case it calls > > Joe> user-defined functions"... > > > > Andrew> > > https://www.postgresql.org/message-id/29608.1518533...@sss.pgh.pa.us > > > > I added it to the open items list since nobody else seems to have taken > > notice; from Tom's linked message it seems this should be Peter E's bag? > > I've taken a look at this - indeed, the situation looks similar to what > described in the linked message, namely after a transaction rollback and > creation of a new one no active snapshot was pushed. But in this particular > case the timeframe without an active snapshot is actually limited and includes > only some initialization and planning activity (after that a new one is > pushed). The commentary says that "Planner must have a snapshot in case it > calls user-defined functions." - I tried to simulate this in order to see > what > would happen, but got no errors. Is there a chance that it's an outdated > Assert? This hasn't progressed in a while. Peter, since you committed the relevant change, could you update us please? Greetings, Andres Freund
Re: assert in nested SQL procedure call in current HEAD
> "Peter" == Peter Eisentraut writes: >> Did you miss the fact that the issue only occurs when the top-level >> procedure does a rollback? The problem is not with nested calls, but >> rather with the fact that commit or rollback is leaving >> ActiveSnapshot unset, which is (as Tom pointed out at the linked >> post) not the expected condition inside PL code. Peter> We need that so that we can run things like SET TRANSACTION Peter> ISOLATION. While testing this, I ran into another semi-related issue: shmem_exit_inprogress isn't ever being cleared in the postmaster, which means that if you ever have a crash-restart, any attempt to do a rollback in a procedure will then crash or get some other form of corruption again every time until you manually restart the cluster. -- Andrew (irc:RhodiumToad)
Re: assert in nested SQL procedure call in current HEAD
On 6/12/18 10:03, Andrew Gierth wrote: >> "Peter" == Peter Eisentraut writes: > > Peter> The problem with these nested procedure calls is that if the > Peter> nested call > > Did you miss the fact that the issue only occurs when the top-level > procedure does a rollback? The problem is not with nested calls, but > rather with the fact that commit or rollback is leaving ActiveSnapshot > unset, which is (as Tom pointed out at the linked post) not the expected > condition inside PL code. We need that so that we can run things like SET TRANSACTION ISOLATION. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: assert in nested SQL procedure call in current HEAD
> "Peter" == Peter Eisentraut writes: Peter> The problem with these nested procedure calls is that if the Peter> nested call Did you miss the fact that the issue only occurs when the top-level procedure does a rollback? The problem is not with nested calls, but rather with the fact that commit or rollback is leaving ActiveSnapshot unset, which is (as Tom pointed out at the linked post) not the expected condition inside PL code. -- Andrew (irc:RhodiumToad)
Re: assert in nested SQL procedure call in current HEAD
On 6/10/18 15:06, Dmitry Dolgov wrote: >> I added it to the open items list since nobody else seems to have taken >> notice; from Tom's linked message it seems this should be Peter E's bag? > I've taken a look at this - indeed, the situation looks similar to what > described in the linked message, namely after a transaction rollback and > creation of a new one no active snapshot was pushed. But in this particular > case the timeframe without an active snapshot is actually limited and includes > only some initialization and planning activity (after that a new one is > pushed). The commentary says that "Planner must have a snapshot in case it > calls user-defined functions." - I tried to simulate this in order to see > what > would happen, but got no errors. Is there a chance that it's an outdated > Assert? The problem with these nested procedure calls is that if the nested call does a commit, you would get the dreaded "snapshot %p still active" warning. So PL/pgSQL in combination with SPI is going out of its way to make sure that there is no active snapshot when we call out to ProcessUtility() for the nested CALL. However, if whatever the nested CALL executes requires an active snapshot to be set, then there is this problem. Apparently, the LANGUAGE SQL procedure requires a snapshot to be set previously. This is not a problem, for example, if you replace the example procedure with LANGUAGE plpgsql, which does its own snapshot management. So perhaps a fix here is to teach the LANGUAGE SQL handler to set a snapshot if there isn't one already. That would make the different language handlers behave consistently. (The alternative is to get rid of the warning in snapmgr.c.) -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: assert in nested SQL procedure call in current HEAD
> On 8 June 2018 at 06:20, Andrew Gierth wrote: > Joe> My colleague Yogesh Sharma discovered an assert in nested SQL > Joe> procedure calls after ROLLBACK is used. Minimal test case and > Joe> backtrace below. I have not yet tried to figure out exactly what > Joe> is going on beyond seeing that it occurs in pg_plan_query() where > Joe> the comment says "Planner must have a snapshot in case it calls > Joe> user-defined functions"... > > Andrew> https://www.postgresql.org/message-id/29608.1518533...@sss.pgh.pa.us > > I added it to the open items list since nobody else seems to have taken > notice; from Tom's linked message it seems this should be Peter E's bag? I've taken a look at this - indeed, the situation looks similar to what described in the linked message, namely after a transaction rollback and creation of a new one no active snapshot was pushed. But in this particular case the timeframe without an active snapshot is actually limited and includes only some initialization and planning activity (after that a new one is pushed). The commentary says that "Planner must have a snapshot in case it calls user-defined functions." - I tried to simulate this in order to see what would happen, but got no errors. Is there a chance that it's an outdated Assert?
Re: assert in nested SQL procedure call in current HEAD
> "Andrew" == Andrew Gierth writes: > "Joe" == Joe Conway writes: Joe> My colleague Yogesh Sharma discovered an assert in nested SQL Joe> procedure calls after ROLLBACK is used. Minimal test case and Joe> backtrace below. I have not yet tried to figure out exactly what Joe> is going on beyond seeing that it occurs in pg_plan_query() where Joe> the comment says "Planner must have a snapshot in case it calls Joe> user-defined functions"... Andrew> https://www.postgresql.org/message-id/29608.1518533...@sss.pgh.pa.us I added it to the open items list since nobody else seems to have taken notice; from Tom's linked message it seems this should be Peter E's bag? -- Andrew (irc:RhodiumToad)
Re: assert in nested SQL procedure call in current HEAD
> "Joe" == Joe Conway writes: Joe> My colleague Yogesh Sharma discovered an assert in nested SQL Joe> procedure calls after ROLLBACK is used. Minimal test case and Joe> backtrace below. I have not yet tried to figure out exactly what Joe> is going on beyond seeing that it occurs in pg_plan_query() where Joe> the comment says "Planner must have a snapshot in case it calls Joe> user-defined functions"... https://www.postgresql.org/message-id/29608.1518533...@sss.pgh.pa.us -- Andrew (irc:RhodiumToad)
