Fix SQLRDD: two fixes omitted from #2044's merge, plus Date/DateTime detection and null-DataTable crashes - #2050
Merged
RobertvanderHulst merged 10 commits intoAug 13, 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.
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
Four fixes. The first two were already described in #2044's summary but the
underlying commits landed on the branch after that PR's merge window closed,
so they never actually made it into
dev- resubmitting them here alongwith two new fixes found since, all from continued stress-testing of SQLRDD
against a real SQL Server-backed app.
row's recno to the pending-writes list
GoCold()iterates, so a delete/recall withno other field change on the row was silently lost -
GoCold()saw nothing toflush and never sent the UPDATE/DELETE to the server. On tables with no
DeletedColumn this was compounded by
Deleted/_UpdateRowfalling back to aWorkarea.Deletedbase stub that is hardcodedFALSE, and byGoCold()'s dirtycheck only looking at
DataRowState, which these methods never touch when there'sno column to write to - so even a queued delete could be skipped entirely. Also
removed
Recall()'s call into twoWorkareamethods that are unimplemented stubs,which made recalling a row with no DeletedColumn throw every time.
fetches the (nonexistent) next page; when that came back empty, the internal
"no more data" flag was set but the public EOF flag was not, so it only became
visible on a second Skip() call. In between, RecNo/CurrentRow reflect the blank
phantom row - callers that check EOF immediately after Skip() (e.g. "if eof() then
goto(oldRecno)") miss it on the first call, then capture the phantom row's blank
recno on the second, landing the position anywhere. Separately,
_hasEOFwas onlyever set when a fetched page came back shorter than
PageSize, which never happenswhen the total record count is an exact multiple of
PageSize- that last (exactlyfull) page never got flagged during sequential forward paging.
GetColumnInfo()toldDBF
"D"(Date) apart from"T"(DateTime) purely byNumericPrecision, but SQLServer's
datetype isn't numeric so ADO.NET reportsNumericPrecisionas thedriver's "not applicable" sentinel (255 via
System.Data.SqlClient) - the samevalue
datetime2reports, so a genuine date-only column could never be recognizedas
"D"and always came back as"T"instead. Reading it back through the RDD thenreturned an unconverted raw
DateTimeinstead of aDbDate, so Date fieldsappeared empty in the app.
NumericScaleis the reliable signal instead: a realtime-bearing column (datetime/datetime2/smalldatetime, any fractional-seconds
precision) always reports a genuine small scale (0-7), while a
datecolumn keepsthe 255 sentinel there too. Added as an addition to the existing
NumericPrecisioncheck rather than replacing it, so any other DBMS provider relying on the old check
is unaffected.
_OpenTable()canfail (e.g. the underlying SELECT throws, or
GetDataTable()swallows an ADO.NETexception into
Connection:LastException) and leaveDataTablenull -_OpenTable()itself now detects this and raises a proper RDD error instead ofreturning
TRUEwith no data loaded, but several call sites downstream neverchecked for a null
DataTableand crashed with a bareNullReferenceExceptioninstead of failing gracefully:
Open()'s Query-mode branch (no later retry path,since
_ForceOpen()is a permanent no-op outside Table mode),Append()/PutValue()(discarded_ForceOpen()return value, masked by a stale phantom rowsurviving a prior
_CloseCursor()),Seek(),GoTo()/GoToId(),_ClearTable(),_GotoRow(), and_UpdateRow(). Each now treats a nullDataTablethe same waythe method already treats an empty one (no rows / nothing to persist) instead of
crashing.
Test plan
without a DeletedColumn, paging past the last record, Date-typed field
round-tripping (write then read back), and the crash scenarios for each guarded
call site (failed open on a fresh/empty table, seek against a table with a
failed SELECT, append/putvalue after a connection hiccup).
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com