Skip to content

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

Merged
dyemanov merged 11 commits into
FirebirdSQL:masterfrom
aafemt:issue8082
May 7, 2025
Merged

Fix for #8082 by making engine to use user buffers directly#8145
dyemanov merged 11 commits into
FirebirdSQL:masterfrom
aafemt:issue8082

Conversation

@aafemt

@aafemt aafemt commented May 30, 2024

Copy link
Copy Markdown
Contributor

Plan "A" is simple:

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

@aafemt

aafemt commented Jun 28, 2024

Copy link
Copy Markdown
Contributor Author

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

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

aafemt commented Jul 9, 2024

Copy link
Copy Markdown
Contributor Author

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

@aafemt

aafemt commented Jul 29, 2024

Copy link
Copy Markdown
Contributor Author

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

@asfernandes

Copy link
Copy Markdown
Member

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

aafemt commented Jul 30, 2024

Copy link
Copy Markdown
Contributor Author

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

aafemt commented Jul 30, 2024

Copy link
Copy Markdown
Contributor Author

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.

@aafemt

aafemt commented Aug 9, 2024

Copy link
Copy Markdown
Contributor Author

Additionally failed QA tests:

  1. 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.)
  2. tests/bugs/core_5973_test.py failed because deprecated SQLCODE was removed from error messages in refactored code.
  3. tests/bugs/gh_6910_test.py failed because output message is no more generated for Execute Block without output.
  4. tests/bugs/gh_7611_test.py failed because behavior of Batch matches behavior of BLR and the workaround doesn't work anymore.

@aafemt 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
aafemt marked this pull request as ready for review August 9, 2024 17:02
@aafemt

aafemt commented Nov 8, 2024

Copy link
Copy Markdown
Contributor Author

Could someone review this, please?

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

aafemt commented Dec 5, 2024

Copy link
Copy Markdown
Contributor Author

Updated, ready to merge.

@aafemt

aafemt commented Dec 5, 2024

Copy link
Copy Markdown
Contributor Author

I wonder why these Android runners are so unstable.

@aafemt aafemt mentioned this pull request Jan 30, 2025
Comment thread src/jrd/Statement.cpp
Comment thread src/dsql/DsqlCompilerScratch.h Outdated
Comment thread src/dsql/DsqlCursor.cpp Outdated
Comment thread src/dsql/DsqlCursor.cpp Outdated
Comment thread src/dsql/DsqlCursor.cpp Outdated
Comment thread src/dsql/DsqlRequests.cpp
Comment thread src/dsql/DsqlRequests.cpp Outdated
Comment thread src/dsql/DsqlRequests.cpp Outdated
Comment thread src/dsql/StmtNodes.cpp Outdated
Comment thread src/dsql/StmtNodes.cpp Outdated
@dyemanov

Copy link
Copy Markdown
Member

Please cleanup req_user_descs that's no longer needed. I'm OK with the PR, just willing to do a few tests before merging.

@asfernandes

Copy link
Copy Markdown
Member

This branch crashes in tcs test FB_SQL_SUBPROC_1.

@asfernandes

Copy link
Copy Markdown
Member

This PR broke WHERE CURRENT OF maybe mixed with RETURNING.
The tests I have is private, so I cannot easily share it, but probably any usage is going to cause problem.
Error is: undefined message number

@mrotteveel

Copy link
Copy Markdown
Member

Given the problems this PR seems to have introduced, shouldn't it be backed out/reverted?

@aafemt

aafemt commented May 18, 2025

Copy link
Copy Markdown
Contributor Author

These problems are not big enough for that, IMHO. I'd prefer to fix them all one-by-one.

@asfernandes

Copy link
Copy Markdown
Member

This PR also breaks TCS test AUTO_COMMIT.3.ESQL.

@asfernandes

Copy link
Copy Markdown
Member

And it's also not fixed by #8571.

@asfernandes

Copy link
Copy Markdown
Member

This PR broke WHERE CURRENT OF maybe mixed with RETURNING. The tests I have is private, so I cannot easily share it, but probably any usage is going to cause problem. Error is: undefined message number

@aafemt are you waiting for this problem to celebrate anniversary?

@aafemt

aafemt commented Jan 1, 2026

Copy link
Copy Markdown
Contributor Author

You do have the testcase, not me.

@aafemt

aafemt commented Jan 1, 2026

Copy link
Copy Markdown
Contributor Author

With or without RETURNING I cannot reproduce the issue.

PS: api6.c example that I tried to use as the testcase is been broken for 13 years.

@mrotteveel mrotteveel added the rlsnotes60: no Intentionally not added to the Firebird 6.0 release notes. label Mar 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix-version: 6.0 Alpha 1 rlsnotes60: no Intentionally not added to the Firebird 6.0 release notes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Transliteration error if connection charset is narrower than storage and requested charsets

5 participants