Fix SQLRDD: crashes and data-integrity bugs found while stress-testing against SQL Server - #2035
Merged
RobertvanderHulst merged 1 commit intoAug 7, 2026
Conversation
…g against a real SQL Server app Found and fixed while migrating an application (HomeBase) from DBF to SQLRDD/SQL Server and exercising Open/Close, Skip, SetOrder, Seek and bulk data migration against real tables: - Close()/UnLock()/Lock()/destructor: guard against being invoked a second time on an already-closed instance (happens when the work area manager reuses a work area number and re-closes whatever RDD was previously in it), which crashed with a NullReferenceException on the now-null _connection. - _GotoRecord(): removed a redundant/incorrect "already in loaded page" pre-check. GoTo() already verifies the record isn't in the current buffer before calling _GotoRecord(), so the brute-walk path must always run; the removed check could otherwise skip loading the correct page entirely. - SqlDbOrder.CalculateKeyLength(): now evaluates the key expression with TrimValues temporarily disabled, and falls back to summing declared column widths (via new SQLRDD.TableColumns) when no record is available to evaluate against. - OrderListFocus(): Order 0 (or an empty order name) is now handled explicitly as "natural order" instead of going through FindOrder(), matching VO's SetOrder(0) always succeeding. - Seek(): no longer forces the page size to 1 row when a filter is active, since evaluating the filter may require skipping past several candidate keys. - BuildRowNumberStatement(): fixed a NullReferenceException when CurrentOrder is null (natural order) by using the already-resolved local variable with a null check instead of dereferencing _oRdd:CurrentOrder directly. - SqlDbProviderSqlServer: override CaseSync() to leave identifiers unchanged. The default (lowercasing) implementation caused "Invalid object name" errors against databases/tables with a case-sensitive collation, since SQL Server is case-preserving, not case-insensitive by definition. - SqlDbConnection.DoesTableExist(): removed a process-wide static cache that was never invalidated on DROP TABLE. Once a table name was seen to exist, DoesTableExist() kept reporting it as existing forever - including immediately after dropping it - which caused callers using the common "drop if exists, then create if not exists" pattern to silently skip re-creating the table. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Member
|
I have not (yet) tested this, but I assume it works in your environment. |
Merged
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
While migrating a real application (HomeBase) from DBF to SQLRDD/SQL Server and exercising Open/Close, Skip, SetOrder, Seek and bulk data migration against real tables, I ran into several crashes and data-integrity bugs. This PR fixes all of them.
Close()/UnLock()/Lock()/ destructor (SQLRDD-Main.prg,SQLRDD-Private.prg): guard against being invoked a second time on an already-closed instance. This happens when the work area manager reuses a work area number and re-closes whatever RDD was previously in it — it crashed with aNullReferenceExceptionon the now-null_connection._GotoRecord()(SQLRDD-Private.prg): removed a redundant/incorrect "already in loaded page" pre-check.GoTo()already verifies the record isn't in the current buffer before calling_GotoRecord(), so the brute-walk path must always run; the removed check could otherwise skip loading the correct page entirely.SqlDbOrder.CalculateKeyLength()(SQLDbOrder.prg): now evaluates the key expression withTrimValuestemporarily disabled (to avoid understating the length whenGetValue()would otherwise strip trailing spaces), and falls back to summing declared column widths (via newSQLRDD.TableColumnsproperty) when no record is available to evaluate against.OrderListFocus()(SQLRDD-Orders.prg): Order 0 (or an empty order name) is now handled explicitly as "natural order" instead of going throughFindOrder(), matching VO'sSetOrder(0)always succeeding.Seek()(SQLRDD-Orders.prg): no longer forces the page size to 1 row when a filter is active, since evaluating the filter may require skipping past several candidate keys that don't match it.BuildRowNumberStatement()(SqlDbTableCommandBuilder.prg): fixed aNullReferenceExceptionwhenCurrentOrderis null (natural order) by using the already-resolved local variable with a null check instead of dereferencing_oRdd:CurrentOrderdirectly.SqlDbProviderSqlServer.CaseSync()(SqlServer.prg): now leaves identifiers unchanged instead of inheriting the default (lowercasing) implementation. SQL Server is case-preserving, not case-insensitive by definition — against a database/table with case-sensitive collation, the forced lowercasing causedSELECT * FROM [tablename]to fail with "Invalid object name" for any table created with upper/mixed case.SqlDbConnection.DoesTableExist()(Connection.prg): removed a process-wide static cache that was never invalidated onDROP TABLE. Once a table name was seen to exist,DoesTableExist()kept reporting it as existing forever — including immediately after dropping it — which silently broke the common "drop if exists, then create if not exists" pattern (the re-create step gets skipped).Test plan
All fixes were verified interactively against a real SQL Server database from an X# application, across multiple runs:
BuildRowNumberStatement)DoesTableExist→DROP TABLE IF EXISTS→DoesTableExist→CREATE TABLEcycle, run repeatedly across process restarts against a database where the table already existed from a previous run🤖 Generated with Claude Code