Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
May 7, 2025

Conversation

aafemt
Copy link
Contributor

@aafemt aafemt commented May 30, 2024

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
Copy link
Contributor Author

aafemt commented Jun 28, 2024

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
Copy link
Contributor Author

aafemt commented Jul 9, 2024

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

@aafemt
Copy link
Contributor Author

aafemt commented Jul 29, 2024

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
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
Copy link
Contributor Author

aafemt commented Jul 30, 2024

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
Copy link
Contributor Author

aafemt commented Jul 30, 2024

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
Copy link
Contributor Author

aafemt commented Aug 9, 2024

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
Copy link
Contributor Author

aafemt commented Nov 8, 2024

Could someone review this, please?

@dyemanov dyemanov self-requested a review November 8, 2024 12:20
@aafemt
Copy link
Contributor Author

aafemt commented Dec 5, 2024

Updated, ready to merge.

@aafemt
Copy link
Contributor Author

aafemt commented Dec 5, 2024

I wonder why these Android runners are so unstable.

@aafemt aafemt mentioned this pull request Jan 30, 2025
@dyemanov
Copy link
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
Member

This branch crashes in tcs test FB_SQL_SUBPROC_1.

@dyemanov dyemanov merged commit 11d5d59 into FirebirdSQL:master May 7, 2025
24 checks passed
@asfernandes
Copy link
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
Member

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

@aafemt
Copy link
Contributor Author

aafemt commented May 18, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transliteration error if connection charset is narrower that storage and requested charsets
5 participants