Fix for #8082 by making engine to use user buffers directly #8145

aafemt · 2024-05-30T15:12:26Z

Plan "A" is simple:

Get rid of EOS variable because user buffers have no room for it.
Modify ParameterNode to use actual message metadata and message buffer instead of one generated during compilation and scratch area.

aafemt · 2024-06-28T16:03:49Z

At this point the testcase for #8082 produces exactly expected output.

aafemt · 2024-07-09T15:42:34Z

Modified testcase that also check coercion of input data.
ctest.zip

aafemt · 2024-07-29T16:10:38Z

Previous commit should completely resolve #8185 moving linkage to parent cursor from DsqlStatement into DsqlRequest so changes from #8189 were partially undone.

asfernandes · 2024-07-30T00:12:39Z

Previous commit should completely resolve #8185 moving linkage to parent cursor from DsqlStatement into DsqlRequest so changes from #8189 were partially undone.

#8185 is already fixed in different ways in master and v5. I see no relation of #8189 with this PR. I actually see no relation of this PR with actual problem of #8082.

In reality, I do not understand what is this PR about.

aafemt · 2024-07-30T09:23:51Z

What I see in master is just disabled caching for queries with cursor name set (which is 100% in Delphi applications). Reference to #8189 was mistype, I meant #8191. The change in this PR made DsqlStatement constant so it can be cached freely (up to reuse of single instance simultaneously).

aafemt · 2024-07-30T14:19:25Z

disabled caching for queries with cursor name set

Sorry, "reference to cursor" meant here. And Delphi applications are not affected more than others. My fault.

…ate message buffer

aafemt · 2024-08-09T16:20:38Z

Additionally failed QA tests:

tests/bugs/core_4374_test.py failed because BLR generated for SUSPEND is three bytes shorter. (Redundant blr_begin, blr_stall, blr_end were removed.)
tests/bugs/core_5973_test.py failed because deprecated SQLCODE was removed from error messages in refactored code.
tests/bugs/gh_6910_test.py failed because output message is no more generated for Execute Block without output.
tests/bugs/gh_7611_test.py failed because behavior of Batch matches behavior of BLR and the workaround doesn't work anymore.

aafemt · 2024-11-08T12:03:09Z

Could someone review this, please?

aafemt · 2024-12-05T12:43:46Z

Updated, ready to merge.

aafemt · 2024-12-05T13:02:28Z

I wonder why these Android runners are so unstable.

dyemanov · 2024-12-18T14:35:23Z

src/jrd/Statement.cpp

+				messages[i] = message;
+			}
+		}
+


How many messages do you expect inside the statement? The csb_rpt array may easily contain a hundred of elements, while only a couple of them may have messages, do I get it right? Isn't it an overkill to store the whole lot of useless nullptr's inside the messages array?

dyemanov · 2025-04-18T13:46:01Z

src/dsql/DsqlCompilerScratch.h

@@ -70,6 +73,7 @@ class DsqlCompilerScratch : public BlrDebugWriter
 	static const unsigned FLAG_DDL					= 0x2000;
 	static const unsigned FLAG_FETCH				= 0x4000;
 	static const unsigned FLAG_VIEW_WITH_CHECK		= 0x8000;
+	static const unsigned FLAG_EXEC_BLOCK			= 0x100000;


Was it intended to be 0x10000 ?

Yes, most likely.

dyemanov · 2025-04-19T10:56:20Z

src/dsql/DsqlCursor.cpp

+	  m_keyBuffer(nullptr),
+	  m_keyBufferLength(0),
+	  m_message(req->getDsqlStatement()->getReceiveMsg()->msg_number),
+	  m_messageLength(0),
 	  m_resultSet(NULL), m_flags(flags),
 	  m_space(req->getPool(), SCRATCH),
 	  m_state(BOS), m_eof(false), m_position(0), m_cachedCount(0)


Please move all trivial initializations (priorly existing and your new ones) to the class declaration in the header file.

dyemanov · 2025-04-19T10:57:25Z

src/dsql/DsqlCursor.cpp

+	{
+		delete[] m_keyBuffer;
+		m_keyBuffer = nullptr;
+	}


Unnecessary if -- it's safe to call delete on null pointers per C++ standard.

dyemanov · 2025-04-19T12:28:09Z

src/dsql/DsqlCursor.cpp

+		m_messageLength = msg->getFormat(req)->fmt_length;
+		// Save record key unconditionally because setCursorName() can be called after openCursor()
+		m_keyBufferLength = req->req_rpb.getCount() * sizeof(RecordKey);
+		m_keyBuffer = new RecordKey[req->req_rpb.getCount()];


Here and below (for buffer): why unpooled new?

DsqlCursor has no obvious pool and m_keyBuffer (and buffer below) is destructed explicitly, not via pool, so it doesn't rally matter what pool to use AFAIU. Default is not worse than any other.

dyemanov · 2025-04-19T16:39:55Z

src/dsql/DsqlRequests.cpp

 	{
-		TraceSQLStatementImpl stmt(dsqlRequest, NULL);
-		TraceManager::event_dsql_free(att, &stmt, DSQL_drop);
+		fb_assert(false);
 	}


Just silently swallow the exception and then finalize the cleanup? AFAIU, this differs from the original code which could throw. Why is this difference?

Because throwing in destructor is a very bad idea. Calling code can be unprepared for this. It is emergency case that should never happen so it is better to swallow the exception (in release build only) and let resources be finalized by pool.

dyemanov · 2025-04-19T16:50:15Z

src/dsql/DsqlRequests.cpp

-		UCHAR* msgBuffer = req_msg_buffers[message->msg_buffer_number];
+		fb_assert(inMsg != nullptr);
+
+		ULONG inMsgLength = dsqlStatement->getStatement()->getMessage(message->msg_number)->getFormat(request)->fmt_length;


const ULONG inMsgLength

dyemanov · 2025-04-19T16:51:20Z

src/dsql/DsqlRequests.cpp

-			// In either case, there's more than one record.
+			MessageNode* msg = dsqlStatement->getStatement()->getMessage(message->msg_number);
+			// outMetadata can be nullptr. If not - it is already converted to message above
+			ULONG outMsgLength = msg->getFormat(request)->fmt_length;


const ULONG outMsgLength

dyemanov · 2025-04-19T17:12:32Z

src/dsql/StmtNodes.cpp

+UCHAR* MessageNode::getBuffer(Request* request) const
+{
+	MessageBuffer* buffer = request->getImpure<MessageBuffer>(impureOffset);
+	if (buffer->buffer == nullptr)


Better naming, please. buffer->buffer reads terribly. I'd suggest something like MessageBuffer::data instead.

dyemanov · 2025-04-19T17:13:31Z

src/dsql/StmtNodes.cpp

+	MessageBuffer* buffer = request->getImpure<MessageBuffer>(impureOffset);
+	if (buffer->buffer == nullptr)
+	{
+		size_t length = buffer->format == nullptr ? format->fmt_length : buffer->format->fmt_length;


Suggested change

size_t length = buffer->format == nullptr ? format->fmt_length : buffer->format->fmt_length;

const ULONG length = buffer->format == nullptr ? format->fmt_length : buffer->format->fmt_length;

aafemt marked this pull request as ready for review July 9, 2024 15:31

aafemt marked this pull request as draft July 10, 2024 16:22

aafemt mentioned this pull request Jul 23, 2024

SIGSEGV in Firebird 5.0.0.1306 embedded during update on cursor #8185

Closed

Fix FirebirdSQL#8082 by removing mapInOut() routine and one intermedi…

53e6ae4

…ate message buffer

aafemt force-pushed the issue8082 branch from ec108f1 to 53e6ae4 Compare August 9, 2024 16:17

aafemt changed the title ~~Attempt to fix #8082 by making engine to use user buffers directly~~ Fix for #8082 by making engine to use user buffers directly Aug 9, 2024

aafemt marked this pull request as ready for review August 9, 2024 17:02

Correct error message on non-positioned cursor

f8cb4a6

dyemanov self-requested a review November 8, 2024 12:20

Merge branch 'master' into issue8082

4a3153c

aafemt mentioned this pull request Jan 30, 2025

New 'UNLIST' function #8418

Merged

Merge branch 'master' into issue8082

e6bb98f

dyemanov requested changes Apr 19, 2025

View reviewed changes

aafemt added 2 commits April 20, 2025 19:16

Requested changes

9b887ed

mistype

7a35dbf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for #8082 by making engine to use user buffers directly #8145

Fix for #8082 by making engine to use user buffers directly #8145

aafemt commented May 30, 2024

aafemt commented Jun 28, 2024

aafemt commented Jul 9, 2024

aafemt commented Jul 29, 2024

asfernandes commented Jul 30, 2024

aafemt commented Jul 30, 2024 •

edited

Loading

aafemt commented Jul 30, 2024

aafemt commented Aug 9, 2024 •

edited

Loading

aafemt commented Nov 8, 2024

aafemt commented Dec 5, 2024

aafemt commented Dec 5, 2024

dyemanov Dec 18, 2024

dyemanov Apr 18, 2025

aafemt Apr 20, 2025

dyemanov Apr 19, 2025

dyemanov Apr 19, 2025

dyemanov Apr 19, 2025

aafemt Apr 20, 2025

dyemanov Apr 19, 2025

aafemt Apr 20, 2025

dyemanov Apr 19, 2025

dyemanov Apr 19, 2025

dyemanov Apr 19, 2025

dyemanov Apr 19, 2025

	size_t length = buffer->format == nullptr ? format->fmt_length : buffer->format->fmt_length;
	const ULONG length = buffer->format == nullptr ? format->fmt_length : buffer->format->fmt_length;

Fix for #8082 by making engine to use user buffers directly #8145

Are you sure you want to change the base?

Fix for #8082 by making engine to use user buffers directly #8145

Conversation

aafemt commented May 30, 2024

aafemt commented Jun 28, 2024

aafemt commented Jul 9, 2024

aafemt commented Jul 29, 2024

asfernandes commented Jul 30, 2024

aafemt commented Jul 30, 2024 • edited Loading

aafemt commented Jul 30, 2024

aafemt commented Aug 9, 2024 • edited Loading

aafemt commented Nov 8, 2024

aafemt commented Dec 5, 2024

aafemt commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aafemt commented Jul 30, 2024 •

edited

Loading

aafemt commented Aug 9, 2024 •

edited

Loading