Skip to content

Commit 11d5d59

Browse files
authored
Fix for #8082 by making engine to use user buffers directly (#8145)
* Fix #8082 by removing mapInOut() routine and one intermediate message buffer * Correct error message on non-positioned cursor * Requested changes * mistype * More requested changes * Fix MSVC2017 build * Use FB_NEW as requested by Alex * Cleanup of req_user_descs * Fix crash on cursor not based on any record source
1 parent 007cd03 commit 11d5d59

35 files changed

+909
-939
lines changed

src/dsql/DsqlBatch.cpp

Lines changed: 16 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -208,12 +208,6 @@ DsqlBatch* DsqlBatch::open(thread_db* tdbb, DsqlDmlRequest* req, IMessageMetadat
208208

209209
const auto statement = req->getDsqlStatement();
210210

211-
if (statement->getFlags() & DsqlStatement::FLAG_ORPHAN)
212-
{
213-
ERRD_post(Arg::Gds(isc_sqlerr) << Arg::Num(-901) <<
214-
Arg::Gds(isc_bad_req_handle));
215-
}
216-
217211
switch (statement->getType())
218212
{
219213
case DsqlStatement::TYPE_INSERT:
@@ -229,7 +223,7 @@ DsqlBatch* DsqlBatch::open(thread_db* tdbb, DsqlDmlRequest* req, IMessageMetadat
229223
}
230224

231225
const dsql_msg* message = statement->getSendMsg();
232-
if (! (inMetadata && message && req->parseMetadata(inMetadata, message->msg_parameters)))
226+
if (! (inMetadata && message && message->msg_parameter > 0))
233227
{
234228
ERRD_post(Arg::Gds(isc_sqlerr) << Arg::Num(-901) <<
235229
Arg::Gds(isc_batch_param));
@@ -659,18 +653,23 @@ Firebird::IBatchCompletionState* DsqlBatch::execute(thread_db* tdbb)
659653
// execute request
660654
m_dsqlRequest->req_transaction = transaction;
661655
Request* req = m_dsqlRequest->getRequest();
656+
DsqlStatement* dStmt = m_dsqlRequest->getDsqlStatement();
662657
fb_assert(req);
663658

664659
// prepare completion interface
665660
AutoPtr<BatchCompletionState, SimpleDispose> completionState
666661
(FB_NEW BatchCompletionState(m_flags & (1 << IBatch::TAG_RECORD_COUNTS), m_detailed));
667662
AutoSetRestore<bool> batchFlag(&req->req_batch_mode, true);
668-
const dsql_msg* message = m_dsqlRequest->getDsqlStatement()->getSendMsg();
663+
const dsql_msg* sendMessage = dStmt->getSendMsg();
664+
// map message to internal engine format
665+
// Do it one time only to avoid parsing its metadata for every message
666+
m_dsqlRequest->metadataToFormat(m_meta, sendMessage);
667+
// Using of positional DML in batch is strange but not forbidden
668+
m_dsqlRequest->mapCursorKey(tdbb);
669669
bool startRequest = true;
670670

671-
bool isExecBlock = m_dsqlRequest->getDsqlStatement()->getType() == DsqlStatement::TYPE_EXEC_BLOCK;
672-
const auto receiveMessage = isExecBlock ? m_dsqlRequest->getDsqlStatement()->getReceiveMsg() : nullptr;
673-
auto receiveMsgBuffer = isExecBlock ? m_dsqlRequest->req_msg_buffers[receiveMessage->msg_buffer_number] : nullptr;
671+
bool isExecBlock = dStmt->getType() == DsqlStatement::TYPE_EXEC_BLOCK;
672+
const dsql_msg* receiveMessage = isExecBlock ? dStmt->getReceiveMsg() : nullptr;
674673

675674
// process messages
676675
ULONG remains;
@@ -726,25 +725,18 @@ Firebird::IBatchCompletionState* DsqlBatch::execute(thread_db* tdbb)
726725
*id = newId;
727726
}
728727

729-
// map message to internal engine format
730-
// pass m_meta one time only to avoid parsing its metadata for every message
731-
m_dsqlRequest->mapInOut(tdbb, false, message, start ? m_meta : nullptr, nullptr, data);
732-
data += m_messageSize;
733-
remains -= m_messageSize;
734-
735-
UCHAR* msgBuffer = m_dsqlRequest->req_msg_buffers[message->msg_buffer_number];
736728
try
737729
{
738730
// runsend data to request and collect stats
739731
ULONG before = req->req_records_inserted + req->req_records_updated +
740732
req->req_records_deleted;
741-
EXE_send(tdbb, req, message->msg_number, message->msg_length, msgBuffer);
733+
EXE_send(tdbb, req, sendMessage->msg_number, m_messageSize, data);
742734
ULONG after = req->req_records_inserted + req->req_records_updated +
743735
req->req_records_deleted;
744736
completionState->regUpdate(after - before);
745737

746-
if (isExecBlock)
747-
EXE_receive(tdbb, req, receiveMessage->msg_number, receiveMessage->msg_length, receiveMsgBuffer);
738+
if (receiveMessage)
739+
EXE_receive(tdbb, req, receiveMessage->msg_number, receiveMessage->msg_length, nullptr); // We don't care about returned record
748740
}
749741
catch (const Exception& ex)
750742
{
@@ -764,6 +756,9 @@ Firebird::IBatchCompletionState* DsqlBatch::execute(thread_db* tdbb)
764756

765757
startRequest = true;
766758
}
759+
760+
data += m_messageSize;
761+
remains -= m_messageSize;
767762
}
768763

769764
UCHAR* alignedData = FB_ALIGN(data, m_alignment);

src/dsql/DsqlCompilerScratch.cpp

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -421,10 +421,7 @@ dsql_var* DsqlCompilerScratch::resolveVariable(const MetaName& varName)
421421
// Generate BLR for a return.
422422
void DsqlCompilerScratch::genReturn(bool eosFlag)
423423
{
424-
const bool hasEos = !(flags & (FLAG_TRIGGER | FLAG_FUNCTION));
425-
426-
if (hasEos && !eosFlag)
427-
appendUChar(blr_begin);
424+
const bool hasEos = !(flags & (FLAG_TRIGGER | FLAG_FUNCTION | FLAG_EXEC_BLOCK));
428425

429426
appendUChar(blr_send);
430427
appendUChar(1);
@@ -455,12 +452,6 @@ void DsqlCompilerScratch::genReturn(bool eosFlag)
455452
}
456453

457454
appendUChar(blr_end);
458-
459-
if (hasEos && !eosFlag)
460-
{
461-
appendUChar(blr_stall);
462-
appendUChar(blr_end);
463-
}
464455
}
465456

466457
void DsqlCompilerScratch::genParameters(Array<NestConst<ParameterClause> >& parameters,

src/dsql/DsqlCompilerScratch.h

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,10 @@ typedef Firebird::Pair<
5151
Firebird::NonPooled<NestConst<ValueListNode>, NestConst<ValueListNode>>> ReturningClause;
5252

5353

54-
// DSQL Compiler scratch block - may be discarded after compilation in the future.
54+
// DSQL Compiler scratch block.
55+
// Contains any kind of objects used during DsqlStatement compilation
56+
// Is deleted with its pool as soon as DsqlStatement is fully formed in prepareStatement()
57+
// or with the statement itself (if the statement reqested it returning true from shouldPreserveScratch())
5558
class DsqlCompilerScratch : public BlrDebugWriter
5659
{
5760
public:
@@ -70,6 +73,7 @@ class DsqlCompilerScratch : public BlrDebugWriter
7073
static const unsigned FLAG_DDL = 0x2000;
7174
static const unsigned FLAG_FETCH = 0x4000;
7275
static const unsigned FLAG_VIEW_WITH_CHECK = 0x8000;
76+
static const unsigned FLAG_EXEC_BLOCK = 0x010000;
7377

7478
static const unsigned MAX_NESTING = 512;
7579

@@ -105,7 +109,7 @@ class DsqlCompilerScratch : public BlrDebugWriter
105109

106110
protected:
107111
// DsqlCompilerScratch should never be destroyed using delete.
108-
// It dies together with it's pool in release_request().
112+
// It dies together with it's pool.
109113
~DsqlCompilerScratch()
110114
{
111115
}
@@ -317,6 +321,7 @@ class DsqlCompilerScratch : public BlrDebugWriter
317321
DsqlCompilerScratch* mainScratch = nullptr;
318322
Firebird::NonPooledMap<USHORT, USHORT> outerMessagesMap; // <outer, inner>
319323
Firebird::NonPooledMap<USHORT, USHORT> outerVarsMap; // <outer, inner>
324+
dsql_msg* recordKeyMessage = nullptr; // Side message for positioned DML
320325

321326
private:
322327
Firebird::HalfStaticArray<SelectExprNode*, 4> ctes; // common table expressions

src/dsql/DsqlCursor.cpp

Lines changed: 94 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828

2929
#include "../dsql/dsql_proto.h"
3030
#include "../dsql/DsqlCursor.h"
31+
#include "../dsql/StmtNodes.h"
3132

3233
using namespace Firebird;
3334
using namespace Jrd;
@@ -36,10 +37,10 @@ static const char* const SCRATCH = "fb_cursor_";
3637
static const ULONG PREFETCH_SIZE = 65536; // 64 KB
3738

3839
DsqlCursor::DsqlCursor(DsqlDmlRequest* req, ULONG flags)
39-
: m_dsqlRequest(req), m_message(req->getDsqlStatement()->getReceiveMsg()),
40-
m_resultSet(NULL), m_flags(flags),
41-
m_space(req->getPool(), SCRATCH),
42-
m_state(BOS), m_eof(false), m_position(0), m_cachedCount(0)
40+
: m_dsqlRequest(req),
41+
m_message(req->getDsqlStatement()->getReceiveMsg()->msg_number),
42+
m_flags(flags),
43+
m_space(req->getPool(), SCRATCH)
4344
{
4445
TRA_link_cursor(m_dsqlRequest->req_transaction, this);
4546
}
@@ -48,6 +49,8 @@ DsqlCursor::~DsqlCursor()
4849
{
4950
if (m_resultSet)
5051
m_resultSet->resetHandle();
52+
53+
delete[] m_keyBuffer;
5154
}
5255

5356
jrd_tra* DsqlCursor::getTransaction() const
@@ -66,6 +69,31 @@ void DsqlCursor::setInterfacePtr(JResultSet* interfacePtr) noexcept
6669
m_resultSet = interfacePtr;
6770
}
6871

72+
bool DsqlCursor::getCurrentRecordKey(USHORT context, RecordKey& key) const
73+
{
74+
if (m_keyBuffer == nullptr)
75+
{
76+
// A possible situation for a cursor not based on any record source such as
77+
// a = 1;
78+
// suspend;
79+
return false;
80+
}
81+
82+
if (context * sizeof(RecordKey) >= m_keyBufferLength)
83+
{
84+
fb_assert(false);
85+
return false;
86+
}
87+
88+
if (m_state != POSITIONED)
89+
{
90+
return false;
91+
}
92+
93+
key = m_keyBuffer[context];
94+
return key.recordNumber.bid_relation_id != 0;
95+
}
96+
6997
void DsqlCursor::close(thread_db* tdbb, DsqlCursor* cursor)
7098
{
7199
if (!cursor)
@@ -88,7 +116,7 @@ void DsqlCursor::close(thread_db* tdbb, DsqlCursor* cursor)
88116

89117
if (dsqlRequest->req_traced && TraceManager::need_dsql_free(attachment))
90118
{
91-
TraceSQLStatementImpl stmt(dsqlRequest, NULL);
119+
TraceSQLStatementImpl stmt(dsqlRequest, nullptr, nullptr);
92120
TraceManager::event_dsql_free(attachment, &stmt, DSQL_close);
93121
}
94122

@@ -115,6 +143,17 @@ int DsqlCursor::fetchNext(thread_db* tdbb, UCHAR* buffer)
115143
return 1;
116144
}
117145

146+
if (m_keyBufferLength == 0)
147+
{
148+
Request* req = m_dsqlRequest->getRequest();
149+
m_keyBufferLength = req->req_rpb.getCount() * sizeof(RecordKey);
150+
if (m_keyBufferLength > 0)
151+
m_keyBuffer = FB_NEW_POOL(m_dsqlRequest->getPool()) RecordKey[req->req_rpb.getCount()];
152+
}
153+
154+
if (m_keyBufferLength > 0)
155+
m_dsqlRequest->gatherRecordKey(m_keyBuffer);
156+
118157
m_state = POSITIONED;
119158
return 0;
120159
}
@@ -163,7 +202,7 @@ int DsqlCursor::fetchAbsolute(thread_db* tdbb, UCHAR* buffer, SLONG position)
163202
{
164203
if (!m_eof)
165204
{
166-
cacheInput(tdbb);
205+
cacheInput(tdbb, buffer);
167206
fb_assert(m_eof);
168207
}
169208

@@ -248,7 +287,7 @@ void DsqlCursor::getInfo(thread_db* tdbb,
248287
case IResultSet::INF_RECORD_COUNT:
249288
if (isScrollable && !m_eof)
250289
{
251-
cacheInput(tdbb);
290+
cacheInput(tdbb, nullptr);
252291
fb_assert(m_eof);
253292
}
254293
response.insertInt(tag, isScrollable ? m_cachedCount : -1);
@@ -291,48 +330,83 @@ int DsqlCursor::fetchFromCache(thread_db* tdbb, UCHAR* buffer, FB_UINT64 positio
291330
{
292331
if (position >= m_cachedCount)
293332
{
294-
if (m_eof || !cacheInput(tdbb, position))
333+
if (m_eof || !cacheInput(tdbb, buffer, position))
295334
{
296335
m_state = EOS;
297336
return 1;
298337
}
299338
}
300339

301340
fb_assert(position < m_cachedCount);
341+
fb_assert(m_messageLength > 0); // At this point m_messageLength must be set by cacheInput
302342

303-
UCHAR* const msgBuffer = m_dsqlRequest->req_msg_buffers[m_message->msg_buffer_number];
343+
FB_UINT64 offset = position * (m_messageLength + m_keyBufferLength);
344+
FB_UINT64 readBytes = m_space.read(offset, buffer, m_messageLength);
304345

305-
const FB_UINT64 offset = position * m_message->msg_length;
306-
const FB_UINT64 readBytes = m_space.read(offset, msgBuffer, m_message->msg_length);
307-
fb_assert(readBytes == m_message->msg_length);
346+
if (m_keyBufferLength > 0)
347+
{
348+
offset += m_messageLength;
349+
readBytes += m_space.read(offset, m_keyBuffer, m_keyBufferLength);
350+
}
308351

309-
m_dsqlRequest->mapInOut(tdbb, true, m_message, NULL, buffer);
352+
fb_assert(readBytes == m_messageLength + m_keyBufferLength);
310353

311354
m_position = position;
312355
m_state = POSITIONED;
313356
return 0;
314357
}
315358

316-
bool DsqlCursor::cacheInput(thread_db* tdbb, FB_UINT64 position)
359+
bool DsqlCursor::cacheInput(thread_db* tdbb, UCHAR* buffer, FB_UINT64 position)
317360
{
318361
fb_assert(!m_eof);
319362

320-
const ULONG prefetchCount = MAX(PREFETCH_SIZE / m_message->msg_length, 1);
321-
const UCHAR* const msgBuffer = m_dsqlRequest->req_msg_buffers[m_message->msg_buffer_number];
363+
// It could not be done before: user buffer length may be unknown until call setDelayedOutputMetadata()
364+
if (m_messageLength == 0)
365+
{
366+
Request* req = m_dsqlRequest->getRequest();
367+
const MessageNode* msg = req->getStatement()->getMessage(m_message);
368+
m_messageLength = msg->getFormat(req)->fmt_length;
369+
m_keyBufferLength = req->req_rpb.getCount() * sizeof(RecordKey);
370+
if (m_keyBufferLength > 0)
371+
{
372+
// Save record key unconditionally because setCursorName() can be called after openCursor()
373+
m_keyBuffer = FB_NEW_POOL(m_dsqlRequest->getPool()) RecordKey[req->req_rpb.getCount()];
374+
}
375+
}
376+
377+
std::unique_ptr<UCHAR[]> ownBuffer;
378+
if (buffer == nullptr)
379+
{
380+
// We are called from getInfo() and there is no user-provided buffer for data.
381+
// Create a temporary one.
382+
// This code cannot be moved into getInfo() itself because it is most likely called before fetch()
383+
// so m_messageLength is still unknown there.
384+
ownBuffer.reset(buffer = FB_NEW UCHAR[m_messageLength]);
385+
}
386+
387+
const ULONG prefetchCount = MAX(PREFETCH_SIZE / (m_messageLength + m_keyBufferLength), 1);
322388

323389
while (position >= m_cachedCount)
324390
{
325391
for (ULONG count = 0; count < prefetchCount; count++)
326392
{
327-
if (!m_dsqlRequest->fetch(tdbb, NULL))
393+
if (!m_dsqlRequest->fetch(tdbb, buffer))
328394
{
329395
m_eof = true;
330396
break;
331397
}
332398

333-
const FB_UINT64 offset = m_cachedCount * m_message->msg_length;
334-
const FB_UINT64 writtenBytes = m_space.write(offset, msgBuffer, m_message->msg_length);
335-
fb_assert(writtenBytes == m_message->msg_length);
399+
FB_UINT64 offset = m_cachedCount * (m_messageLength + m_keyBufferLength);
400+
FB_UINT64 writtenBytes = m_space.write(offset, buffer, m_messageLength);
401+
402+
if (m_keyBufferLength > 0)
403+
{
404+
offset += m_messageLength;
405+
m_dsqlRequest->gatherRecordKey(m_keyBuffer);
406+
writtenBytes += m_space.write(offset, m_keyBuffer, m_keyBufferLength);
407+
}
408+
409+
fb_assert(writtenBytes == m_messageLength + m_keyBufferLength);
336410
m_cachedCount++;
337411
}
338412

0 commit comments

Comments
 (0)