Fix SQLRDD: NULL/Nullable-flag ADS-compat gaps, tag lookup, index creation, and cursor-reset overhead - #2058
Merged
RobertvanderHulst merged 13 commits intoAug 19, 2026
Conversation
Neither the schema/metadata methods (DoesTableExist, DoesDatabaseExist, GetTables, GetMetaDataCollections, GetMetaDataCollection) nor SqlDbCommand's Execute*/GetSchemaTable methods ever checked whether the underlying DbConnection was still open before using it. ForceOpen() was only ever called once, from the SqlDbConnection constructor. If the physical connection dropped for any reason (idle timeout, transient network error, server-side kill) after that, every later call failed and the connection stayed dead for the rest of the process. - Call ForceOpen() at the top of each of those methods so a dropped connection is transparently reopened before use. - LastException is now a real property backed by the existing field instead of two disconnected stores (a private field some methods wrote to directly, and a separate auto-property Command.prg wrote to), and its setter traces the exception via System.Diagnostics.Trace so the underlying cause of a connection failure is visible without requiring caller changes.
Root cause of intermittently losing the SQL connection while checking
tables at startup: the explicit Close() path (SQLRDD-Main.prg) correctly
calls connection:UnregisterRdd(self), which only closes the physical
connection when it's both the last registered work area AND KeepOpen is
off.
The destructor (finalizer) took a different, more aggressive path:
connection:Dispose() -> Close(), which unconditionally closes the
physical connection and deregisters it from SqlDbConnection.Connections
entirely, ignoring KeepOpen. Any work area that got left for the GC to
finalize instead of being explicitly closed - e.g. a DBWindow/Datenbank
instance opened just to inspect a table's index/schema and never closed
- would, at finalization time, force-close and deregister the shared
"DEFAULT" connection out from under every other still-open table on it.
Once deregistered, SqlDbConnection.FindByName("DEFAULT") returns null,
so every later Open() on that connection name fails immediately via
_PrepareOpen() with no exception and no LastException set - it just
silently produces a work area with fCount=0, surfacing as "table cannot
be opened" for whichever table happened to be opened next.
Fix: destructor now mirrors Close() and calls UnregisterRdd(self)
instead of Dispose(), so a leaked/finalized work area only affects the
shared connection the same way an explicit Close() would.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…antics Pending writes were never committed: transaction-end logic that only calls Commit() when IsLocked(0) is true never actually committed SQLRDD tables, because IsLocked()/RLockList rely on DBI_GETLOCKARRAY/DBI_LOCKCOUNT, which SQLRDD never implemented (locking is tracked entirely in xs_locks, not the base RDD's own lock bookkeeping). Info() now answers both from xs_locks, so Commit() fires when it should instead of changes sitting unflushed until an unrelated order change/close forced a GoCold. Lock-table cleanup had two bugs: the periodic timer was kept only in a local variable, so it could be silently garbage-collected and simply stop firing; and its "stale" threshold equaled its own refresh interval, leaving no margin before a still-active lock could be judged abandoned. The timer is now kept in a field, disposed on Close(), swept once immediately on connect (so a crashed process's locks don't linger for a full interval), and the refresh interval/stale threshold are separate, overridable connection settings (SqlRDDEventReason.LockRefreshInterval/LockStaleThreshold, in seconds, default 120/600) instead of hardcoded equal constants. GoTo() by physical recno was fully order-dependent: it built an order-filtered ROW_NUMBER() query and failed whenever the target record didn't satisfy the current order's FOR-condition, even though DBF's GoTo() is a physical operation that must succeed regardless of order. It now falls back to a direct, order-independent fetch by recno in that case, matching DBF: the record is found (RecNo set, BOF/EOF false) but Found is false and OrderKeyNo (DBOI_POSITION) is 0. Skip() from that position previously used the ad-hoc single-row buffer's stale page/row numbers, which pointed nowhere meaningful; it now matches DBF by treating that position like BOF - a negative skip lands on the first record of the order, a positive Skip(n) lands on record n. Also fixes _UpdateRow crashing (NullReferenceException) instead of failing gracefully when the record it needs to flush is no longer in the buffer, and GoTo() discarding its actual result and always returning TRUE.
…ncatenated-key conditions _hasEOF could get stuck true from an earlier GoBottom()/paging call and then leak into an unrelated position (fresh Seek(), a direct GoTo(), or a jump outside the current order), permanently blocking all further forward paging from that point. Reset it in _OpenTable(), _GotoRecord() and _GotoRecordOutsideOrder() so each reposition determines EOF for itself instead of inheriting stale state. _GetRecCount() ignored the current order's scope, so any recount triggered while a scope was active (e.g. GoCold() flushing a "hot" row) silently overwrote RecCount with the whole table's count instead of the scoped one, corrupting the page/EOF math for the rest of the browse. It now uses OrderKeyCount when an order is active. GoBottom() on a large table paged via the normal ascending, OFFSET-based query, forcing SQL Server to walk/skip almost the entire ordered result to reach the end - cost grows with table size. Added _FetchLastPage()/BuildLastPageStatement(), which sort descending and fetch at OFFSET 0 instead (always cheap), reversing the rows back into ascending order client-side. Falls back to the original approach for natural/descending order or on failure. SqlDbOrder's seek/scope conditions were always built against the fully concatenated key expression (e.g. [COL1]+[COL2] LIKE 'X%'), which SQL Server cannot use a normal index to seek into - it has to evaluate the concatenation per row. Added BuildColumnAwareCondition(), which expresses a value that covers one or more leading columns as a plain AND-chain of per-column conditions (equality for fully-covered columns, a range/prefix condition on the last partial one), allowing a real composite-index seek. Falls back to the original concatenation-based condition when the key has functions in it or column metadata can't be resolved. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
OrderListFocus() (SetOrder) called _CloseCursor() - which nulls out the buffer table - before the GoTo() further down triggered its internal GoCold() flush. CurrentRow reads that same table, so at the moment GoCold() ran it saw the empty phantom row instead of the real modified one, treated the row as unchanged, and skipped the actual write while still reporting success. Any write followed by a SetOrder() before the next natural flush (the common "save a record, then restore the caller's original order/position" pattern) was silently lost. Fixed by flushing via GoCold() before tearing down the cursor, so the write happens while the real row is still visible. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Seek() temporarily forced PageSize to 1 before fetching, to keep an unfiltered
existence-check cheap, then restored the normal PageSize right after. That left the
resulting buffer ("page 1") holding only one row while every later paging calculation
still assumed a full-size first page. A caller that finds a match and then walks
forward with Skip() while the key still matches - the standard "seek to the first
record of a key, then Skip() through the rest of the group" idiom used throughout the
app - triggers _FetchPage() for "page 2", whose offset ((CurrentPage-1) * PageSize) is
computed against the just-restored normal PageSize instead of the single row actually
consumed. That jumps straight to absolute offset PageSize, silently skipping every
other row that shares the seek's key. Fixed by always fetching a normal, full-size
page in Seek(), removing the mismatch.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Delete() and Recall() only touched the DeletedColumn DataColumn (when one exists) and never added the row's recno to _updatedRecNos, the list GoCold() iterates to decide what to write back. A pure delete/recall with no other field change on the row was therefore silently lost: GoCold() saw nothing to flush, so no UPDATE/DELETE statement was ever sent to the server. For tables without a DeletedColumn this was compounded by two more gaps: - Deleted/_UpdateRow fell back to `super:Deleted`, but Workarea.Deleted is a hardcoded `GET FALSE` stub with no state of its own, so a plain delete could never be detected even if it had been queued. - GoCold()'s lWasHot guard only looked at DataRowState, which Delete()/Recall() never change when there's no DeletedColumn to write to, so the write-back loop was skipped entirely regardless of _updatedRecNos. - Recall() unconditionally called super:GoTo()/super:Recall(), both `THROW NotImplementedException` stubs on Workarea, so recalling a row with no DeletedColumn always crashed. Fixes: - Delete()/Recall() now always register the row in _updatedRecNos and call GoHot(), and track deleted-without-column rows in a new _deletedRowIds set. - New _IsRowDeleted(row) checks the DeletedColumn when present, else _deletedRowIds; replaces the broken super:Deleted use in _UpdateRow and backs the Deleted property directly instead of delegating to the base stub. - lWasHot also fires when _updatedRecNos is non-empty. - Recall() no longer calls into the Workarea stubs.
Two related gaps let PgDn-past-the-end land on a bogus record instead of staying on the last row: - SkipRaw()'s "fetch the next page" branch never called _SetEOF(TRUE) itself, even when that fetch turned out empty. It relied on a *subsequent* Skip() noticing the already-set internal _hasEOF flag, so the first Skip() past the end left RowNumber pointing past RowCount with the public EOF flag still FALSE. Callers that check EOF right after Skip() (e.g. nextrec()'s "if eof() then goto(oldRecno)") don't catch it until one call too late - and by then oldRecno was captured from the phantom row, not a real record, so the eventual GoTo() lands wherever that blank value happens to point. SkipRaw() now sets EOF immediately when the fetched page is empty. - _FetchPage() only ever flagged _hasEOF when the fetched page came back shorter than PageSize. When the total record count is an exact multiple of PageSize, the last page is exactly full, so that check never fires during sequential forward paging (unlike GoBottom(), which jumps straight to the last page via _FetchLastPage() and flags it unconditionally). Now also compares the page's absolute record range against the known total.
GetColumnInfo() told DBF "D" (Date) apart from "T" (DateTime) purely by NumericPrecision, but SQL Server's `date` type isn't numeric so ADO.NET reports NumericPrecision as the driver's "not applicable" sentinel (255 via System.Data.SqlClient) - the same value `datetime2` reports, so a genuine date-only column could never be recognized as "D" and always came back as "T" instead. Reading it back through the RDD then returned an unconverted raw DateTime instead of a DbDate, so Date fields appeared empty in the app. NumericScale is the reliable signal instead: a real time-bearing column (datetime/datetime2/smalldatetime, any fractional-seconds precision) always reports a genuine small scale (0-7), while a `date` column keeps the 255 sentinel there too. Added as an addition to the existing NumericPrecision check in GetStructureForQuery() rather than replacing it, so any other DBMS provider relying on the old check is unaffected.
_OpenTable() can fail (e.g. the underlying SELECT throws, or GetDataTable() swallows an ADO.NET exception into Connection:LastException) and leave DataTable null - _OpenTable() itself now detects this and raises a proper RDD error instead of returning TRUE with no data loaded, but several call sites downstream never checked for a null DataTable and crashed with a bare NullReferenceException instead of failing gracefully: - Open()'s Query-mode branch: a failed GetDataTable() left DataTable null for the object's entire lifetime, since _ForceOpen() is a permanent no-op outside Table mode and never gets a chance to retry. - Append()/PutValue(): the return value of _ForceOpen() was discarded, so a stale phantom row surviving a prior _CloseCursor() let GoCold() report success anyway. - Seek(): indexed DataTable:Rows:Count right after _OpenTable() with no check at all. - GoTo()/GoToId(): indexed into DataTable/CurrentRow with no check. - _ClearTable(), _GotoRow(), _UpdateRow(): same unguarded pattern. Each now treats a null DataTable the same way the method already treats an empty one (no rows / nothing to persist) instead of crashing.
…reation DBNull cells and the DBFFieldFlags.Nullable marker leaked SQL-specific behavior into generic FIELDGET-style code written against classic DBF/ADS semantics, which has no per-cell NULL concept. FindOrder() only accepted a LONG tag number, silently failing for the DWORD VO code commonly passes. CreateIndex() also failed outright when a DBF-style key expression referenced the same column twice, leaving the index missing entirely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
_GetRecCount() re-ran COUNT(*) on every call even when nothing had changed since the last one - a Lister retrying GoTo(0)/GoBottom() many times in a row against unchanged data (observed: ~29 repeats) paid for a fresh query every time on large tables. GoCold() also treated the never-edited phantom row as "hot" whenever positioned at EOF, forcing the same unnecessary recount. SetOrder() and order-scope changes unconditionally tore down the server-side cursor even when the requested order/scope was already the active one, which classic DBF/ADS code paths do defensively and for free, but SqlRDD does not. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
RobertvanderHulst
approved these changes
Aug 19, 2026
RobertvanderHulst
approved these changes
Aug 19, 2026
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
2 participants
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
DBFFieldFlags.Nullableon open and unconditionally substitutes the phantom row's typed blank forDBNull.Value, so generic FIELDGET-style code written against classic DBF/ADS semantics (no per-cell NULL concept) doesn't silently break on SQL-backed nullable columns.FindOrder()now accepts any numeric type for a tag number, not justLONG- VO code commonly passes aDWORD(e.g. fromIndexCount()), which ADS/DBF accepts without complaint.CreateIndex()de-duplicates the column list before generatingCREATE INDEX, since a DBF-style key expression may legitimately reference the same physical column twice (e.g.IIF(EMPTY(DATUM), BUCHDATUM, DATUM)) - SQL Server previously rejected the duplicate outright, leaving the index silently missing._GetRecCount()now caches its result instead of re-runningCOUNT(*)on every call;GoCold()no longer treats the phantom row as "hot";SetOrder()and order-scope changes skip tearing down the server-side cursor when the requested order/scope is already active. Classic DBF/ADS code paths do these operations defensively and for free - SqlRDD was paying for a full cursor rebuild every time.Test plan
XSharp.SQLRDD.xsproj, Debug/net46)🤖 Generated with Claude Code