Refactored PSQL savepoint envelopes. Made the savepoint undo logic consistent across the codebase. Added assertions to prevent releasing the savepoint that must be undone.

src/dsql/BoolNodes.cpp

@@ -1801,6 +1801,8 @@ bool RseBoolNode::execute(thread_db* tdbb, jrd_req* request) const
 
 	subQuery->close(tdbb);
 
+	savePoint.release();
+
 	if (blrOp == blr_any || blrOp == blr_unique)
 		request->req_flags &= ~req_null;
 

src/dsql/ExprNodes.cpp

@@ -11327,11 +11327,11 @@ dsc* SubQueryNode::execute(thread_db* tdbb, jrd_req* request) const
 
 	ULONG flag = req_null;
 
+	StableCursorSavePoint savePoint(tdbb, request->req_transaction,
+		blrOp == blr_via && ownSavepoint);
+
 	try
 	{
-		StableCursorSavePoint savePoint(tdbb, request->req_transaction,
-			blrOp == blr_via && ownSavepoint);
-
 		subQuery->open(tdbb);
 
 		SLONG count = 0;
@@ -11438,6 +11438,9 @@ dsc* SubQueryNode::execute(thread_db* tdbb, jrd_req* request) const
 	// Close stream and return value.
 
 	subQuery->close(tdbb);
+
+	savePoint.release();
+
 	request->req_flags &= ~req_null;
 	request->req_flags |= flag;
 
@@ -12995,6 +12998,7 @@ dsc* UdfCallNode::execute(thread_db* tdbb, jrd_req* request) const
 		}
 
 		jrd_tra* transaction = request->req_transaction;
+
 		const SavNumber savNumber = transaction->tra_save_point ?
 			transaction->tra_save_point->getNumber() : 0;
 
@@ -13018,7 +13022,7 @@ dsc* UdfCallNode::execute(thread_db* tdbb, jrd_req* request) const
 
 			EXE_receive(tdbb, funcRequest, 1, outMsgLength, outMsg);
 
-			// Clean up all savepoints started during execution of the procedure.
+			// Clean up all savepoints started during execution of the function
 
 			if (!(transaction->tra_flags & TRA_system))
 			{

src/dsql/Nodes.h

@@ -1402,7 +1402,6 @@ public:
 		TYPE_RECEIVE,
 		TYPE_RETURN,
 		TYPE_SAVEPOINT,
-		TYPE_SAVEPOINT_ENCLOSE,
 		TYPE_SELECT,
 		TYPE_SESSION_MANAGEMENT_WRAPPER,
 		TYPE_SET_GENERATOR,
@@ -1565,28 +1564,6 @@ public:
 };
 
 
-// Add savepoint pair of nodes to statement having error handlers.
-class SavepointEncloseNode : public TypedNode<DsqlOnlyStmtNode, StmtNode::TYPE_SAVEPOINT_ENCLOSE>
-{
-public:
-	explicit SavepointEncloseNode(MemoryPool& pool, StmtNode* aStmt)
-		: TypedNode<DsqlOnlyStmtNode, StmtNode::TYPE_SAVEPOINT_ENCLOSE>(pool),
-		  stmt(aStmt)
-	{
-	}
-
-public:
-	static StmtNode* make(MemoryPool& pool, DsqlCompilerScratch* dsqlScratch, StmtNode* node);
-
-public:
-	virtual Firebird::string internalPrint(NodePrinter& printer) const;
-	virtual void genBlr(DsqlCompilerScratch* dsqlScratch);
-
-private:
-	NestConst<StmtNode> stmt;
-};
-
-
 struct ScaledNumber
 {
 	FB_UINT64 number;

src/dsql/StmtNodes.cpp

@@ -293,39 +293,6 @@ string StmtNode::internalPrint(NodePrinter& printer) const
 //--------------------
 
 
-StmtNode* SavepointEncloseNode::make(MemoryPool& pool, DsqlCompilerScratch* dsqlScratch, StmtNode* node)
-{
-	if (dsqlScratch->errorHandlers)
-	{
-		node = FB_NEW_POOL(pool) SavepointEncloseNode(pool, node);
-		node->dsqlPass(dsqlScratch);
-	}
-
-	return node;
-}
-
-string SavepointEncloseNode::internalPrint(NodePrinter& printer) const
-{
-	DsqlOnlyStmtNode::internalPrint(printer);
-
-	NODE_PRINT(printer, stmt);
-
-	return "SavepointEncloseNode";
-}
-
-void SavepointEncloseNode::genBlr(DsqlCompilerScratch* dsqlScratch)
-{
-	dsqlScratch->appendUChar(blr_begin);
-	dsqlScratch->appendUChar(blr_start_savepoint);
-	stmt->genBlr(dsqlScratch);
-	dsqlScratch->appendUChar(blr_end_savepoint);
-	dsqlScratch->appendUChar(blr_end);
-}
-
-
-//--------------------
-
-
 static RegisterNode<AssignmentNode> regAssignmentNode({blr_assignment});
 
 DmlNode* AssignmentNode::parse(thread_db* tdbb, MemoryPool& pool, CompilerScratch* csb, const UCHAR /*blrOp*/)
@@ -690,17 +657,17 @@ const StmtNode* BlockNode::execute(thread_db* tdbb, jrd_req* request, ExeState*
 					savNumber = *request->getImpure<SavNumber>(impureOffset);
 
 					// Since there occurred an error (req_unwind), undo all savepoints
-					// up to, _but not including_, the savepoint of this block.
+					// up to, *but not including*, the savepoint of this block.
-					// That's why transaction->rollbackToSavepoint() cannot be used here
+					// That's why transaction->rollbackToSavepoint() cannot be used here.
 					// The savepoint of this block will be dealt with below.
-					// Do this only if error handlers exist. If not - leave rollbacking to caller node
+					// Do this only if error handlers exist. Otherwise, leave undo up to callers.
 
 					while (transaction->tra_save_point &&
-						savNumber < transaction->tra_save_point->getNumber() &&
+						transaction->tra_save_point->getNumber() > savNumber &&
 						transaction->tra_save_point->getNext() &&
-						savNumber < transaction->tra_save_point->getNext()->getNumber())
+						transaction->tra_save_point->getNext()->getNumber() > savNumber)
 					{
-						transaction->rollforwardSavepoint(tdbb);
+						transaction->rollforwardSavepoint(tdbb, false);
 					}
 
 					// There can be no savepoints above the given one
@@ -767,8 +734,8 @@ const StmtNode* BlockNode::execute(thread_db* tdbb, jrd_req* request, ExeState*
 						}
 					}
 				}
-				// The error is dealt with by the application, cleanup
+
-				// this block's savepoint.
+				// The error is dealt with by the application, cleanup our savepoint
 
 				if (handled && !(transaction->tra_flags & TRA_system))
 				{
@@ -783,9 +750,8 @@ const StmtNode* BlockNode::execute(thread_db* tdbb, jrd_req* request, ExeState*
 					// Because of this following assert is commented out
 					//fb_assert(transaction->tra_save_point && transaction->tra_save_point->getNumber() == savNumber);
 
-					for (const Savepoint* save_point = transaction->tra_save_point;
+					while (transaction->tra_save_point &&
-							save_point && savNumber <= save_point->getNumber();
+						transaction->tra_save_point->getNumber() >= savNumber)
-							save_point = transaction->tra_save_point)
 					{
 						transaction->rollforwardSavepoint(tdbb);
 					}
@@ -804,9 +770,9 @@ const StmtNode* BlockNode::execute(thread_db* tdbb, jrd_req* request, ExeState*
 				savNumber = *request->getImpure<SavNumber>(impureOffset);
 
 				// rollforward all savepoints
-				for (const Savepoint* save_point = transaction->tra_save_point;
+				while (transaction->tra_save_point &&
-					 save_point && save_point->getNext() && savNumber <= save_point->getNumber();
+					transaction->tra_save_point->getNext() &&
-					 save_point = transaction->tra_save_point)
+					transaction->tra_save_point->getNumber() >= savNumber)
 				{
 					transaction->rollforwardSavepoint(tdbb);
 				}
@@ -2579,7 +2545,6 @@ const StmtNode* EraseNode::execute(thread_db* tdbb, jrd_req* request, ExeState*
 
 	if (request->req_operation == jrd_req::req_unwind)
 		retNode = parentStmt;
-
 	else if (request->req_operation == jrd_req::req_return && subStatement)
 	{
 		if (!exeState->topNode)
@@ -3262,6 +3227,7 @@ void ExecProcedureNode::executeProcedure(thread_db* tdbb, jrd_req* request) cons
 	}
 
 	jrd_tra* transaction = request->req_transaction;
+
 	const SavNumber savNumber = transaction->tra_save_point ?
 		transaction->tra_save_point->getNumber() : 0;
 
@@ -3287,7 +3253,7 @@ void ExecProcedureNode::executeProcedure(thread_db* tdbb, jrd_req* request) cons
 
 		EXE_receive(tdbb, procRequest, 1, outMsgLength, outMsg);
 
-		// Clean up all savepoints started during execution of the procedure.
+		// Clean up all savepoints started during execution of the procedure
 
 		if (!(transaction->tra_flags & TRA_system))
 		{
@@ -4071,7 +4037,7 @@ const StmtNode* InAutonomousTransactionNode::execute(thread_db* tdbb, jrd_req* r
 			transaction->tra_save_point->isSystem() &&
 			transaction->tra_save_point->isChanging())
 		{
-			transaction->rollforwardSavepoint(tdbb);
+			transaction->rollforwardSavepoint(tdbb, false);
 		}
 
 		{ // scope
@@ -4096,7 +4062,7 @@ const StmtNode* InAutonomousTransactionNode::execute(thread_db* tdbb, jrd_req* r
 					transaction->tra_save_point->isSystem() &&
 					transaction->tra_save_point->isChanging())
 				{
-					transaction->rollforwardSavepoint(tdbb);
+					transaction->rollforwardSavepoint(tdbb, false);
 				}
 
 				AutoSetRestore2<jrd_req*, thread_db> autoNullifyRequest(
@@ -5089,11 +5055,13 @@ const StmtNode* ForNode::execute(thread_db* tdbb, jrd_req* request, ExeState* /*
 			}
 			cursor->open(tdbb);
 			request->req_records_affected.clear();
+
 			// fall into
 
 		case jrd_req::req_return:
 			if (stall)
 				return stall;
+
 			// fall into
 
 		case jrd_req::req_sync:
@@ -5130,36 +5098,47 @@ const StmtNode* ForNode::execute(thread_db* tdbb, jrd_req* request, ExeState* /*
 				restartRequest(request, transaction);
 
 			request->req_operation = jrd_req::req_return;
-			// fall into
 
-		case jrd_req::req_unwind:
+			if (impure->savepoint)
-		{
-			const LabelNode* label = nodeAs<LabelNode>(parentStmt.getObject());
-
-			if (label && request->req_label == label->labelNumber &&
-				(request->req_flags & req_continue_loop))
 			{
-				request->req_flags &= ~req_continue_loop;
+				while (transaction->tra_save_point &&
-				request->req_operation = jrd_req::req_sync;
+					transaction->tra_save_point->getNumber() >= impure->savepoint)
-				return this;
+				{
+					transaction->rollforwardSavepoint(tdbb);
+				}
 			}
 
 			// fall into
-		}
 
 		default:
 		{
-			const SavNumber savNumber = impure->savepoint;
+			if (request->req_operation == jrd_req::req_unwind)
-
-			if (savNumber)
 			{
-				while (transaction->tra_save_point &&
+				if (request->req_flags & (req_leave | req_continue_loop))
-					transaction->tra_save_point->getNumber() >= savNumber)
 				{
-					if (transaction->tra_save_point->isChanging()) // we must rollback this savepoint
+					const auto label = nodeAs<LabelNode>(parentStmt.getObject());
-						transaction->rollbackSavepoint(tdbb);
+
-					else
+					// If CONTINUE matches our label, restart fetching records
-						transaction->rollforwardSavepoint(tdbb);
+
+					if (label && request->req_label == label->labelNumber &&
+						(request->req_flags & req_continue_loop))
+					{
+						request->req_flags &= ~req_continue_loop;
+						request->req_operation = jrd_req::req_sync;
+						return this;
+					}
+
+					// Otherwise (BREAK/LEAVE/EXIT or mismatched CONTINUE), we should unwind further.
+					// Thus cleanup our savepoint.
+
+					if (impure->savepoint)
+					{
+						while (transaction->tra_save_point &&
+							transaction->tra_save_point->getNumber() >= impure->savepoint)
+						{
+							transaction->rollforwardSavepoint(tdbb);
+						}
+					}
 				}
 			}
 
@@ -8333,72 +8312,94 @@ void ReturnNode::genBlr(DsqlCompilerScratch* dsqlScratch)
 //--------------------
 
 
-static RegisterNode<SavePointNode> regSavePointNode({blr_start_savepoint, blr_end_savepoint});
+static RegisterNode<SavepointEncloseNode> regSavePointNode({blr_start_savepoint});
 
-DmlNode* SavePointNode::parse(thread_db* /*tdbb*/, MemoryPool& pool, CompilerScratch* /*csb*/, const UCHAR blrOp)
+DmlNode* SavepointEncloseNode::parse(thread_db* tdbb, MemoryPool& pool, CompilerScratch* csb, const UCHAR /*blrOp*/)
 {
-	SavePointNode* node = FB_NEW_POOL(pool) SavePointNode(pool, blrOp);
+	const auto statement = PAR_parse_stmt(tdbb, csb);
+
+	const auto node = FB_NEW_POOL(pool) SavepointEncloseNode(pool, statement);
+
+	// skip blr_end_savepoint
+	const auto blrOp = csb->csb_blr_reader.getByte();
+	fb_assert(blrOp == blr_end_savepoint);
+
 	return node;
 }
 
-SavePointNode* SavePointNode::dsqlPass(DsqlCompilerScratch* /*dsqlScratch*/)
+StmtNode* SavepointEncloseNode::make(MemoryPool& pool, DsqlCompilerScratch* dsqlScratch, StmtNode* node)
 {
-	return this;
+	// Add savepoint wrapper around the statement having error handlers
+
+	return dsqlScratch->errorHandlers ?
+		FB_NEW_POOL(pool) SavepointEncloseNode(pool, node) : node;
 }
 
-string SavePointNode::internalPrint(NodePrinter& printer) const
+SavepointEncloseNode* SavepointEncloseNode::dsqlPass(DsqlCompilerScratch* dsqlScratch)
+{
+	const auto node = FB_NEW_POOL(dsqlScratch->getPool()) SavepointEncloseNode(dsqlScratch->getPool(), statement);
+	node->statement = statement->dsqlPass(dsqlScratch);
+	return node;
+}
+
+string SavepointEncloseNode::internalPrint(NodePrinter& printer) const
 {
 	StmtNode::internalPrint(printer);
 
-	NODE_PRINT(printer, blrOp);
+	NODE_PRINT(printer, statement);
 
-	return "SavePointNode";
+	return "SavepointEncloseNode";
 }
 
-void SavePointNode::genBlr(DsqlCompilerScratch* dsqlScratch)
+void SavepointEncloseNode::genBlr(DsqlCompilerScratch* dsqlScratch)
 {
-	dsqlScratch->appendUChar(blrOp);
+	dsqlScratch->appendUChar(blr_begin);
+	dsqlScratch->appendUChar(blr_start_savepoint);
+	statement->genBlr(dsqlScratch);
+	dsqlScratch->appendUChar(blr_end_savepoint);
+	dsqlScratch->appendUChar(blr_end);
 }
 
-const StmtNode* SavePointNode::execute(thread_db* tdbb, jrd_req* request, ExeState* exeState) const
+SavepointEncloseNode* SavepointEncloseNode::pass1(thread_db* tdbb, CompilerScratch* csb)
 {
-	jrd_tra* transaction = request->req_transaction;
+	doPass1(tdbb, csb, statement.getAddress());
+	return this;
+}
 
-	switch (blrOp)
+SavepointEncloseNode* SavepointEncloseNode::pass2(thread_db* tdbb, CompilerScratch* csb)
+{
+	doPass2(tdbb, csb, statement.getAddress(), this);
+	impureOffset = csb->allocImpure<SavNumber>();
+	return this;
+}
+
+const StmtNode* SavepointEncloseNode::execute(thread_db* tdbb, jrd_req* request, ExeState* /*exeState*/) const
+{
+	const auto transaction = request->req_transaction;
+
+	if (request->req_operation == jrd_req::req_evaluate)
 	{
-		case blr_start_savepoint:
+		if (!(transaction->tra_flags & TRA_system))
-			if (request->req_operation == jrd_req::req_evaluate)
+		{
+			const auto savepoint = transaction->startSavepoint();
+			*request->getImpure<SavNumber>(impureOffset) = savepoint->getNumber();
+		}
+
+		return statement;
+	}
+
+	if (request->req_operation == jrd_req::req_return)
+	{
+		if (!(transaction->tra_flags & TRA_system))
+		{
+			const auto savNumber = *request->getImpure<SavNumber>(impureOffset);
+
+			while (transaction->tra_save_point &&
+				transaction->tra_save_point->getNumber() >= savNumber)
 			{
-				// Start a save point.
+				transaction->rollforwardSavepoint(tdbb);
-				if (!(transaction->tra_flags & TRA_system))
-					transaction->startSavepoint();
-
-				request->req_operation = jrd_req::req_return;
 			}
-			break;
+		}
-
-		case blr_end_savepoint:
-			if (request->req_operation == jrd_req::req_evaluate ||
-				request->req_operation == jrd_req::req_unwind)
-			{
-				// If any requested modify/delete/insert ops have completed, forget them.
-				if (!(transaction->tra_flags & TRA_system))
-				{
-					// If an error is still pending when the savepoint is supposed to end, then the
-					// application didn't handle the error and the savepoint should be undone.
-					if (exeState->errorPending)
-						transaction->rollbackSavepoint(tdbb);
-					else
-						transaction->rollforwardSavepoint(tdbb);
-				}
-
-				if (request->req_operation == jrd_req::req_evaluate)
-					request->req_operation = jrd_req::req_return;
-			}
-			break;
-
-		default:
-			fb_assert(false);
 	}
 
 	return parentStmt;

src/dsql/StmtNodes.h

@@ -1488,37 +1488,31 @@ public:
 };
 
 
-class SavePointNode : public TypedNode<StmtNode, StmtNode::TYPE_SAVEPOINT>
+class SavepointEncloseNode : public TypedNode<StmtNode, StmtNode::TYPE_SAVEPOINT>
 {
 public:
-	explicit SavePointNode(MemoryPool& pool, UCHAR aBlrOp)
+	explicit SavepointEncloseNode(MemoryPool& pool, StmtNode* stmt)
 		: TypedNode<StmtNode, StmtNode::TYPE_SAVEPOINT>(pool),
-		  blrOp(aBlrOp)
+		  statement(stmt)
 	{
-		fb_assert(blrOp == blr_start_savepoint || blrOp == blr_end_savepoint);
 	}
 
 public:
 	static DmlNode* parse(thread_db* tdbb, MemoryPool& pool, CompilerScratch* csb, const UCHAR blrOp);
 
+	static StmtNode* make(MemoryPool& pool, DsqlCompilerScratch* dsqlScratch, StmtNode* node);
+
 	virtual Firebird::string internalPrint(NodePrinter& printer) const;
-	virtual SavePointNode* dsqlPass(DsqlCompilerScratch* dsqlScratch);
+	virtual SavepointEncloseNode* dsqlPass(DsqlCompilerScratch* dsqlScratch);
 	virtual void genBlr(DsqlCompilerScratch* dsqlScratch);
 
-	virtual SavePointNode* pass1(thread_db* /*tdbb*/, CompilerScratch* /*csb*/)
+	virtual SavepointEncloseNode* pass1(thread_db* tdbb, CompilerScratch* csb);
-	{
+	virtual SavepointEncloseNode* pass2(thread_db* tdbb, CompilerScratch* csb);
-		return this;
-	}
-
-	virtual SavePointNode* pass2(thread_db* /*tdbb*/, CompilerScratch* /*csb*/)
-	{
-		return this;
-	}
 
 	virtual const StmtNode* execute(thread_db* tdbb, jrd_req* request, ExeState* exeState) const;
 
 public:
-	UCHAR blrOp;
+	NestConst<StmtNode> statement;
 };
 
 

src/dsql/dsql.cpp

@@ -855,12 +855,14 @@ void DsqlDmlRequest::execute(thread_db* tdbb, jrd_tra** traHandle,
 	if (req_transaction && (req_transaction->tra_flags & TRA_read_consistency) &&
 		statement->getType() != DsqlCompiledStatement::TYPE_SAVEPOINT)
 	{
-		AutoSavePoint savePoint(tdbb, req_transaction);
 		req_request->req_flags &= ~req_update_conflict;
 		int numTries = 0;
 		const int MAX_RESTARTS = 10;
+
 		while (true)
 		{
+			AutoSavePoint savePoint(tdbb, req_transaction);
+
 			// Don't set req_restart_ready flas at last attempt to restart request.
 			// It allows to raise update conflict error (if any) as usual and
 			// handle error by PSQL handler.
@@ -893,6 +895,7 @@ void DsqlDmlRequest::execute(thread_db* tdbb, jrd_tra** traHandle,
 					ERRD_post_warning(Arg::Warning(isc_random) << Arg::Str(s));
 				}
 #endif
+				savePoint.release();	// everything is ok
 				break;
 			}
 
@@ -902,8 +905,9 @@ void DsqlDmlRequest::execute(thread_db* tdbb, jrd_tra** traHandle,
 			req_transaction->tra_flags &= ~TRA_ex_restart;
 			fb_utils::init_status(tdbb->tdbb_status_vector);
 
-			req_transaction->rollbackSavepoint(tdbb, true);
+			// Undo current savepoint but preserve already taken locks.
-			req_transaction->startSavepoint(tdbb);
+			// Savepoint will be restarted at the next loop iteration.
+			savePoint.rollback(true);
 
 			numTries++;
 			if (numTries >= MAX_RESTARTS)
@@ -913,7 +917,6 @@ void DsqlDmlRequest::execute(thread_db* tdbb, jrd_tra** traHandle,
 						 "\tQuery:\n%s\n", numTries, req_request->getStatement()->sqlText->c_str() );
 			}
 		}
-		savePoint.release();	// everything is ok
 	} else {
 		doExecute(tdbb, traHandle, inMetadata, inMsg, outMetadata, outMsg, singleton);
 	}

src/jrd/Savepoint.cpp

@@ -621,22 +621,44 @@ Savepoint* Savepoint::release(Savepoint* prior)
 // AutoSavePoint implementation
 
 AutoSavePoint::AutoSavePoint(thread_db* tdbb, jrd_tra* trans)
-	: m_tdbb(tdbb), m_transaction(trans), m_released(false)
+	: m_tdbb(tdbb), m_transaction(trans), m_number(0)
 {
-	trans->startSavepoint();
+	const auto savepoint = trans->startSavepoint();
+	m_number = savepoint->getNumber();
 }
 
 AutoSavePoint::~AutoSavePoint()
 {
-	if (!(m_tdbb->getDatabase()->dbb_flags & DBB_bugcheck))
+	if (m_number && !(m_tdbb->getDatabase()->dbb_flags & DBB_bugcheck))
 	{
-		if (m_released)
+		fb_assert(m_transaction->tra_save_point);
-			m_transaction->rollforwardSavepoint(m_tdbb);
+		fb_assert(m_transaction->tra_save_point->getNumber() == m_number);
-		else
+		m_transaction->rollbackSavepoint(m_tdbb);
-			m_transaction->rollbackSavepoint(m_tdbb);
 	}
 }
 
+void AutoSavePoint::release()
+{
+	if (!m_number)
+		return;
+
+	fb_assert(m_transaction->tra_save_point);
+	fb_assert(m_transaction->tra_save_point->getNumber() == m_number);
+	m_transaction->rollforwardSavepoint(m_tdbb);
+	m_number = 0;
+}
+
+void AutoSavePoint::rollback(bool preserveLocks)
+{
+	if (!m_number)
+		return;
+
+	fb_assert(m_transaction->tra_save_point);
+	fb_assert(m_transaction->tra_save_point->getNumber() == m_number);
+	m_transaction->rollbackSavepoint(m_tdbb, preserveLocks);
+	m_number = 0;
+}
+
 
 // StableCursorSavePoint implementation
 
@@ -652,7 +674,7 @@ StableCursorSavePoint::StableCursorSavePoint(thread_db* tdbb, jrd_tra* trans, bo
 	if (!trans->tra_save_point)
 		return;
 
-	const Savepoint* const savepoint = trans->startSavepoint();
+	const auto savepoint = trans->startSavepoint();
 	m_number = savepoint->getNumber();
 }
 
@@ -662,8 +684,11 @@ void StableCursorSavePoint::release()
 	if (!m_number)
 		return;
 
-	while (m_transaction->tra_save_point && m_transaction->tra_save_point->getNumber() >= m_number)
+	while (m_transaction->tra_save_point &&
+		m_transaction->tra_save_point->getNumber() >= m_number)
+	{
 		m_transaction->rollforwardSavepoint(m_tdbb);
+	}
 
 	m_number = 0;
 }

src/jrd/Savepoint.h

@@ -324,8 +324,8 @@ namespace Jrd
 		VerbAction* m_freeActions;		// free verb actions
 	};
 
-
+	// Start a savepoint and rollback it in destructor,
-	// Starts a savepoint and rollback it in destructor if release() is not called
+	// unless it was released / rolled back explicitly
 
 	class AutoSavePoint
 	{
@@ -333,26 +333,22 @@ namespace Jrd
 		AutoSavePoint(thread_db* tdbb, jrd_tra* trans);
 		~AutoSavePoint();
 
-		void release()
+		void release();
-		{
+		void rollback(bool preserveLocks = false);
-			m_released = true;
-		}
 
 	private:
 		thread_db* const m_tdbb;
 		jrd_tra* const m_transaction;
-		bool m_released;
+		SavNumber m_number;
 	};
 
+	// Conditional savepoint used to ensure cursor stability in sub-queries
+
 	class StableCursorSavePoint
 	{
 	public:
 		StableCursorSavePoint(thread_db* tdbb, jrd_tra* trans, bool start);
-
+		~StableCursorSavePoint() {} // undo is left up to the callers
-		~StableCursorSavePoint()
-		{
-			release();
-		}
 
 		void release();
 

src/jrd/tra.cpp

@@ -3896,14 +3896,14 @@ void jrd_tra::rollbackToSavepoint(thread_db* tdbb, SavNumber number)
  *
  **************************************/
 {
-	// Merge all but one folowing savepoints into one
+	// Merge all savepoints (except the given one) into a single one
 	while (tra_save_point && tra_save_point->getNumber() > number &&
 		tra_save_point->getNext() && tra_save_point->getNext()->getNumber() >= number)
 	{
-		rollforwardSavepoint(tdbb);
+		rollforwardSavepoint(tdbb, false);
 	}
 
-	// Check that savepoint with given number really exists
+	// Check that savepoint with the given number really exists
 	fb_assert(tra_save_point && tra_save_point->getNumber() == number);
 
 	if (tra_save_point && tra_save_point->getNumber() >= number) // second line of defence
@@ -3915,7 +3915,7 @@ void jrd_tra::rollbackToSavepoint(thread_db* tdbb, SavNumber number)
 }
 
 
-void jrd_tra::rollforwardSavepoint(thread_db* tdbb)
+void jrd_tra::rollforwardSavepoint(thread_db* tdbb, bool assertChanging)
 /**************************************
  *
  *	 r o l l f o r w a r d S a v e p o i n t
@@ -3929,6 +3929,8 @@ void jrd_tra::rollforwardSavepoint(thread_db* tdbb)
 {
 	if (tra_save_point && !(tra_flags & TRA_system))
 	{
+		fb_assert(!assertChanging || !tra_save_point->isChanging());
+
 		REPL_save_cleanup(tdbb, this, tra_save_point, false);
 
 		Jrd::ContextPoolHolder context(tdbb, tra_pool);

src/jrd/tra.h

@@ -389,7 +389,7 @@ public:
 	Savepoint* startSavepoint(bool root = false);
 	void rollbackSavepoint(thread_db* tdbb, bool preserveLocks = false);
 	void rollbackToSavepoint(thread_db* tdbb, SavNumber number);
-	void rollforwardSavepoint(thread_db* tdbb);
+	void rollforwardSavepoint(thread_db* tdbb, bool assertChanging = true);
 	DbCreatorsList* getDbCreatorsList();
 	void checkBlob(thread_db* tdbb, const bid* blob_id, jrd_fld* fld, bool punt);