Summary
A single-table SELECT ... ORDER BY (also DISTINCT / LIMIT) returns the live source table carrying a recno_sequence, rather than a materialised static cursor. An application that indexes or browses that cursor therefore operates on the production table.
The most damaging consequence: an app that runs INDEX ON ... ; DBSETORDER(n) over the result — a very common xBase browse idiom — rewrites the source table's production .cdx. In our deployment (a Harbour/FiveWin ERP whose record-picker runs SELECT * FROM [articulo.dat] WHERE UPPER(cnombreart) LIKE 'X%' ORDER BY cnombreart and then indexes the result for the browse), every such lookup silently rebuilt the real table's official index bag.
Details
AdsExecuteSQLDirect installs a recno_sequence on the opened source Table and returns that same handle as the cursor. So for the caller:
- the cursor is the production
.dbf, and any index operation on it lands on the production .cdx;
DBSETORDER on the cursor is effectively a no-op for the browse, because the traversal is driven by the sequence, not by an order;
AdsGetRecordNum reports source recnos and AdsGetKeyCount reports the source key count, so a bookmark/OrdKeyNo/GetRelKeyPos round-trip over the result does not agree with the walk.
Real ACE returns a static cursor (its own temp table) for these queries — which is also what this codebase already assumes elsewhere. AdsExecuteSQLDirect's own INDEX ON guard tells callers:
INDEX ON requires a table name, not a SELECT result; use SELECT ... ORDER BY/DISTINCT/LIMIT to materialize first
so the intent is already "ORDER BY materialises" — it just isn't what happens. The join / union / aggregate / CASE paths in the same function do materialise into a temp DBF; the single-table ORDER BY path is the outlier.
Proposed change
Materialise the single-table ORDER BY / DISTINCT / LIMIT result into a standalone temp DBF (its own recnos 1..N in result order, its own index space) and close the live source cursor — the same shape the other paths already produce.
Two details that are easy to miss:
- Column permissions must be evaluated before materialising. Today the column-level ACL is applied as a cursor projection on the live handle; a materialised cursor cannot inherit that, so the temp table must be built with only the permitted columns (and an explicit projection naming a forbidden column still denied).
- The temp must be deleted when the cursor closes. Otherwise every
SELECT ... ORDER BY leaves a temp .dbf — plus whatever index the application built on it — in the customer's data directory.
Effect on the existing tests
Three cases in abi_sql_orderby_test assert the source recnos after ORDER BY (e.g. seq[0] == 2) as a proxy for "the rows came out sorted". Under a static cursor those recnos become positional, so the assertions have to move to the sorted data, which is what the tests actually mean to verify. No other test in the suite depends on the current numbering.
Status on our side
Implemented and verified on our fork on top of v1.8.31: full suite 1187/1199, i.e. exactly the 12 pre-existing SQL-parser (7200) failures that pristine v1.8.31 has, and no others. A full run leaves no temp files behind.
Happy to open a focused PR rebased onto main if you want it — opening this as an issue first because it changes observable SQL-layer behaviour and touches those three tests, so it seemed like your call rather than something to slip into a fix PR.
Related PRs from the same deployment: #134, #135.
Summary
A single-table
SELECT ... ORDER BY(alsoDISTINCT/LIMIT) returns the live source table carrying arecno_sequence, rather than a materialised static cursor. An application that indexes or browses that cursor therefore operates on the production table.The most damaging consequence: an app that runs
INDEX ON ... ; DBSETORDER(n)over the result — a very common xBase browse idiom — rewrites the source table's production.cdx. In our deployment (a Harbour/FiveWin ERP whose record-picker runsSELECT * FROM [articulo.dat] WHERE UPPER(cnombreart) LIKE 'X%' ORDER BY cnombreartand then indexes the result for the browse), every such lookup silently rebuilt the real table's official index bag.Details
AdsExecuteSQLDirectinstalls arecno_sequenceon the opened sourceTableand returns that same handle as the cursor. So for the caller:.dbf, and any index operation on it lands on the production.cdx;DBSETORDERon the cursor is effectively a no-op for the browse, because the traversal is driven by the sequence, not by an order;AdsGetRecordNumreports source recnos andAdsGetKeyCountreports the source key count, so a bookmark/OrdKeyNo/GetRelKeyPosround-trip over the result does not agree with the walk.Real ACE returns a static cursor (its own temp table) for these queries — which is also what this codebase already assumes elsewhere.
AdsExecuteSQLDirect's ownINDEX ONguard tells callers:so the intent is already "ORDER BY materialises" — it just isn't what happens. The join / union / aggregate / CASE paths in the same function do materialise into a temp DBF; the single-table ORDER BY path is the outlier.
Proposed change
Materialise the single-table ORDER BY / DISTINCT / LIMIT result into a standalone temp DBF (its own recnos
1..Nin result order, its own index space) and close the live source cursor — the same shape the other paths already produce.Two details that are easy to miss:
SELECT ... ORDER BYleaves a temp.dbf— plus whatever index the application built on it — in the customer's data directory.Effect on the existing tests
Three cases in
abi_sql_orderby_testassert the source recnos afterORDER BY(e.g.seq[0] == 2) as a proxy for "the rows came out sorted". Under a static cursor those recnos become positional, so the assertions have to move to the sorted data, which is what the tests actually mean to verify. No other test in the suite depends on the current numbering.Status on our side
Implemented and verified on our fork on top of
v1.8.31: full suite 1187/1199, i.e. exactly the 12 pre-existing SQL-parser (7200) failures that pristinev1.8.31has, and no others. A full run leaves no temp files behind.Happy to open a focused PR rebased onto
mainif you want it — opening this as an issue first because it changes observable SQL-layer behaviour and touches those three tests, so it seemed like your call rather than something to slip into a fix PR.Related PRs from the same deployment: #134, #135.