Rework positional DML because it cannot use additional parameters in standard messages anymore

Author: aafemt

src/dsql/DsqlBatch.cpp

@@ -203,12 +203,6 @@ DsqlBatch* DsqlBatch::open(thread_db* tdbb, DsqlDmlRequest* req, IMessageMetadat
 
 	const auto statement = req->getDsqlStatement();
 
-	if (statement->getFlags() & DsqlStatement::FLAG_ORPHAN)
-	{
-		ERRD_post(Arg::Gds(isc_sqlerr) << Arg::Num(-901) <<
-		          Arg::Gds(isc_bad_req_handle));
-	}
-
 	switch (statement->getType())
 	{
 		case DsqlStatement::TYPE_INSERT:
@@ -665,6 +659,8 @@ Firebird::IBatchCompletionState* DsqlBatch::execute(thread_db* tdbb)
 	// map message to internal engine format
 	// Do it one time only to avoid parsing its metadata for every message
 	m_dsqlRequest->metadataToFormat(m_meta, sendMessage);
+	// Using of positional DML in batch is strange but not forbidden
+	m_dsqlRequest->mapCursorKey(tdbb);
 	bool startRequest = true;
 
 	bool isExecBlock = dStmt->getType() == DsqlStatement::TYPE_EXEC_BLOCK;

src/dsql/DsqlCompilerScratch.h

@@ -51,7 +51,10 @@ typedef Firebird::Pair<
 	Firebird::NonPooled<NestConst<ValueListNode>, NestConst<ValueListNode>>> ReturningClause;
 
 
-// DSQL Compiler scratch block - may be discarded after compilation in the future.
+// DSQL Compiler scratch block.
+// Contains any kind of objects used during DsqlStatement compilation
+// Is deleted with its pool as soon as DsqlStatement is fully formed in prepareStatement()
+// or with the statement itself (if the statement reqested it returning true from shouldPreserveScratch())
 class DsqlCompilerScratch : public BlrDebugWriter
 {
 public:
@@ -106,7 +109,8 @@ class DsqlCompilerScratch : public BlrDebugWriter
 
 protected:
 	// DsqlCompilerScratch should never be destroyed using delete.
-	// It dies together with it's pool in release_request().
+	// It dies together with it's pool.
+	void operator delete(void*) = delete;
 	~DsqlCompilerScratch()
 	{
 	}
@@ -318,6 +322,7 @@ class DsqlCompilerScratch : public BlrDebugWriter
 	DsqlCompilerScratch* mainScratch = nullptr;
 	Firebird::NonPooledMap<USHORT, USHORT> outerMessagesMap;	// <outer, inner>
 	Firebird::NonPooledMap<USHORT, USHORT> outerVarsMap;		// <outer, inner>
+	dsql_msg* recordKeyMessage = nullptr;	// Side message for positioned DML
 
 private:
 	Firebird::HalfStaticArray<SelectExprNode*, 4> ctes; // common table expressions

src/dsql/DsqlRequests.cpp

@@ -56,6 +56,59 @@ DsqlRequest::~DsqlRequest()
 {
 }
 
+void DsqlRequest::releaseRequest(thread_db* tdbb)
+{
+	if (req_timer)
+	{
+		req_timer->stop();
+		req_timer = nullptr;
+	}
+
+	// Prevent new children from appear
+	if (req_cursor_name.hasData())
+		req_dbb->dbb_cursors.remove(req_cursor_name);
+
+	// If request is parent, orphan the children and release a portion of their requests
+
+	for (auto childStatement : cursors)
+	{
+		// Without parent request prepared DsqlRequest will throw error on execute which is exactly what one would expect
+		childStatement->onReferencedCursorClose();
+
+		// hvlad: lines below is commented out as
+		// - child is already unlinked from its parent request
+		// - we should not free child's sql text until its owner request is alive
+		// It seems to me we should destroy owner request here, not a child
+		// statement - as it always was before
+
+		//Jrd::ContextPoolHolder context(tdbb, &childStatement->getPool());
+		//releaseStatement(childStatement);
+	}
+
+	// If the request had an open cursor, close it
+
+	if (req_cursor)
+		DsqlCursor::close(tdbb, req_cursor);
+
+	if (req_batch)
+	{
+		delete req_batch;
+		req_batch = nullptr;
+	}
+
+	Jrd::Attachment* att = req_dbb->dbb_attachment;
+	const bool need_trace_free = req_traced && TraceManager::need_dsql_free(att);
+	if (need_trace_free)
+	{
+		TraceSQLStatementImpl stmt(this, nullptr, nullptr);
+		TraceManager::event_dsql_free(att, &stmt, DSQL_drop);
+	}
+
+	// If a request has been compiled, release it now
+	if (getRequest())
+		EXE_release(tdbb, getRequest());
+}
+
 void DsqlRequest::setCursor(thread_db* /*tdbb*/, const TEXT* /*name*/)
 {
 	status_exception::raise(
@@ -166,71 +219,102 @@ void DsqlRequest::destroy(thread_db* tdbb, DsqlRequest* dsqlRequest)
 {
 	SET_TDBB(tdbb);
 
-	if (dsqlRequest->req_timer)
+	// Increase the statement refCount so its pool is not destroyed before the request is gone.
+	auto dsqlStatement = dsqlRequest->getDsqlStatement();
+
+	// Let request to clean itself
+	try
 	{
-		dsqlRequest->req_timer->stop();
-		dsqlRequest->req_timer = nullptr;
+		dsqlRequest->releaseRequest(tdbb);
 	}
-
-	// If request is parent, orphan the children and release a portion of their requests
-
-	for (auto childStatement : dsqlRequest->cursors)
+	catch(...)
 	{
-		childStatement->addFlags(DsqlStatement::FLAG_ORPHAN);
-		childStatement->setParentRequest(nullptr);
+		fb_assert(false);
+	}
 
-		// hvlad: lines below is commented out as
-		// - child is already unlinked from its parent request
-		// - we should not free child's sql text until its owner request is alive
-		// It seems to me we should destroy owner request here, not a child
-		// statement - as it always was before
+	// Release the entire request
+	delete dsqlRequest;
+	dsqlStatement = nullptr;
+}
 
-		//Jrd::ContextPoolHolder context(tdbb, &childStatement->getPool());
-		//releaseStatement(childStatement);
-	}
+// DsqlDmlRequest
 
-	// If the request had an open cursor, close it
+DsqlDmlRequest::DsqlDmlRequest(thread_db* tdbb, MemoryPool& pool, dsql_dbb* dbb, DsqlDmlStatement* aStatement)
+	: DsqlRequest(pool, dbb, aStatement)
+{
+	request = aStatement->getStatement()->findRequest(tdbb);
+	tdbb->getAttachment()->att_requests.add(request);
 
-	if (dsqlRequest->req_cursor)
-		DsqlCursor::close(tdbb, dsqlRequest->req_cursor);
+	// If this is positional DML - subscribe to parent cursor as well
 
-	if (dsqlRequest->req_batch)
+	if (aStatement->parentCursorName.hasData())
 	{
-		delete dsqlRequest->req_batch;
-		dsqlRequest->req_batch = nullptr;
-	}
+		const auto* const symbol = dbb->dbb_cursors.get(aStatement->parentCursorName.c_str());
 
-	Jrd::Attachment* att = dsqlRequest->req_dbb->dbb_attachment;
-	const bool need_trace_free = dsqlRequest->req_traced && TraceManager::need_dsql_free(att);
-	if (need_trace_free)
-	{
-		TraceSQLStatementImpl stmt(dsqlRequest, nullptr, nullptr);
-		TraceManager::event_dsql_free(att, &stmt, DSQL_drop);
-	}
+		if (!symbol)
+		{
+			// cursor is not found
+			ERRD_post(Arg::Gds(isc_sqlerr) << Arg::Num(-504) <<
+					  Arg::Gds(isc_dsql_cursor_err) <<
+					  Arg::Gds(isc_dsql_cursor_not_found) << aStatement->parentCursorName);
+		}
 
-	if (dsqlRequest->req_cursor_name.hasData())
-		dsqlRequest->req_dbb->dbb_cursors.remove(dsqlRequest->req_cursor_name);
+		parentRequest = *symbol;
+		fb_assert(parentRequest != nullptr);
 
-	// If a request has been compiled, release it now
-	if (dsqlRequest->getRequest())
-		EXE_release(tdbb, dsqlRequest->getRequest());
+		// Verify that the cursor is appropriate and updatable
 
-	// Increase the statement refCount so its pool is not destroyed before the request is gone.
-	auto dsqlStatement = dsqlRequest->getDsqlStatement();
+		if (parentRequest->getDsqlStatement()->getType() != DsqlStatement::TYPE_SELECT_UPD)
+		{
+			// cursor is not updatable
+			ERRD_post(Arg::Gds(isc_sqlerr) << Arg::Num(-510) <<
+					  Arg::Gds(isc_dsql_cursor_update_err) << aStatement->parentCursorName);
+		}
 
-	// Release the entire request
-	delete dsqlRequest;
+		// Check that it contains this relation name
+		Request* request = parentRequest->getRequest();
+		fb_assert(request->req_rpb.getCount() > 0 && request->req_rpb[0].rpb_relation != nullptr);
 
-	dsqlStatement = nullptr;
-}
+		const MetaName& relName = request->req_rpb[0].rpb_relation->rel_name;
+		bool found = false;
+		for (FB_SIZE_T i = 0; i < request->req_rpb.getCount(); ++i)
+		{
+			jrd_rel* relation = request->req_rpb[i].rpb_relation;
 
-// DsqlDmlRequest
+			if (relation && relation->rel_name == relName)
+			{
+				if (found)
+				{
+					// Relation is used twice in cursor
+					ERRD_post(Arg::Gds(isc_dsql_cursor_err)
+								<< Arg::Gds(isc_dsql_cursor_rel_ambiguous) << relName << aStatement->parentCursorName);
+				}
+				parentContext = i;
+				found = true;
+			}
+		}
 
-DsqlDmlRequest::DsqlDmlRequest(thread_db* tdbb, MemoryPool& pool, dsql_dbb* dbb, DsqlStatement* aStatement)
-	: DsqlRequest(pool, dbb, aStatement)
+		if (!found)
+		{
+			// Relation is not in cursor
+			ERRD_post(Arg::Gds(isc_dsql_cursor_err)
+						<< Arg::Gds(isc_dsql_cursor_rel_not_found) << relName << aStatement->parentCursorName);
+		}
+		parentRequest->cursors.add(this);
+	}
+}
+
+void DsqlDmlRequest::releaseRequest(thread_db* tdbb)
 {
-	request = aStatement->getStatement()->findRequest(tdbb);
-	tdbb->getAttachment()->att_requests.add(request);
+	// If request is a child - unsubscribe
+	if (parentRequest)
+	{
+		parentRequest->cursors.findAndRemove(this);
+		parentRequest = nullptr;
+	}
+
+	// Let ancestor do cleanup as well
+	DsqlRequest::releaseRequest(tdbb);
 }
 
 Statement* DsqlDmlRequest::getStatement() const
@@ -417,12 +501,6 @@ DsqlCursor* DsqlDmlRequest::openCursor(thread_db* tdbb, jrd_tra** traHandle,
 
 	Jrd::ContextPoolHolder context(tdbb, &getPool());
 
-	if (dsqlStatement->getFlags() & DsqlStatement::FLAG_ORPHAN)
-	{
-		ERRD_post(Arg::Gds(isc_sqlerr) << Arg::Num(-901) <<
-		          Arg::Gds(isc_bad_req_handle));
-	}
-
 	// Validate transaction handle
 
 	if (!*traHandle)
@@ -590,6 +668,8 @@ void DsqlDmlRequest::execute(thread_db* tdbb, jrd_tra** traHandle,
 		metadataToFormat(inMetadata, message);
 	}
 
+	mapCursorKey(tdbb);
+
 	// we need to set new format before tracing of execution start to let trace
 	// manager know statement parameters values
 	TraceDSQLExecute trace(req_dbb->dbb_attachment, this, inMsg);
@@ -792,6 +872,57 @@ void DsqlDmlRequest::metadataToFormat(Firebird::IMessageMetadata* meta, const ds
 	msg->setFormat(request, newFormat);
 }
 
+void DsqlDmlRequest::mapCursorKey(thread_db* tdbb)
+{
+	const auto dsqlStatement = getDsqlStatement();
+	if (!dsqlStatement->parentCursorName.hasData())
+		return;
+
+	if (!parentRequest) // It has been already closed
+	{
+		ERRD_post(Arg::Gds(isc_sqlerr) << Arg::Num(-504) <<
+				  Arg::Gds(isc_dsql_cursor_err) <<
+				  Arg::Gds(isc_dsql_cursor_not_found) << dsqlStatement->parentCursorName);
+	}
+
+	Request* master = parentRequest->getRequest();
+	fb_assert(request);
+	fb_assert(master);
+
+	record_param& rpb = master->req_rpb[parentContext];
+	if (!rpb.rpb_number.isValid() || rpb.rpb_number.isBof() || !rpb.rpb_relation)
+	{
+		ERRD_post(Arg::Gds(isc_sqlerr) << Arg::Num(-504) <<
+				  Arg::Gds(isc_dsql_cursor_err) <<
+				  Arg::Gds(isc_cursor_not_positioned) << dsqlStatement->parentCursorName);
+	}
+
+	// Assign record key
+	MessageNode* message = request->getStatement()->getMessage(2);
+	fb_assert(message);
+
+	dsc desc = message->getFormat(request)->fmt_desc[0];
+	UCHAR* msgBuffer = message->getBuffer(request);
+	desc.dsc_address = msgBuffer + (IPTR) desc.dsc_address;
+
+	RecordNumber::Packed dbKey = {};
+	dbKey.bid_encode(rpb.rpb_number.getValue() + 1);
+	dbKey.bid_relation_id = rpb.rpb_relation->rel_id;
+
+	dsc parentDesc;
+	parentDesc.makeDbkey(&dbKey);
+
+	MOVD_move(tdbb, &parentDesc, &desc);
+
+	// Assign version number
+
+	desc = message->getFormat(request)->fmt_desc[1];
+	desc.dsc_address = msgBuffer + (IPTR) desc.dsc_address;
+
+	parentDesc.makeDbkey(&rpb.rpb_transaction_nr);
+
+	MOVD_move(tdbb, &parentDesc, &desc);
+}
 
 // DsqlDdlRequest