fix(fivesql2): DML transaction + constraint ordering
Three correctness bugs in the DML executor that the 4.7 audit
surfaced:
1. RunInsert logged the transaction BEFORE dbAppend() and validation.
LogRecord captured the PREVIOUS row's RecNo, and a CHECK/FK
violation that rolled back via dbDelete() still left a spurious
INSERT entry in the log pointing at the wrong record. Move
LogRecord to after all field puts and all validators pass, so
the log only records committed INSERTs at the correct RecNo.
2. RunUpdate (fallback path) skipped CHECK and FK validation entirely
— only RunInsert validated. An UPDATE could violate the same
constraints INSERT protects against. Add the same validator calls
after FieldPut, with a captured aPrevVals snapshot so the in-
memory record can roll back cleanly on failure. Gated by
SqlLoadConstraints to skip the validator (and its recursive
five_SQL) for tables without SQL-level metadata — tables created
via plain dbCreate see no change.
3. RunDelete had no transaction logging at all — a BEGIN / DELETE /
ROLLBACK cycle silently lost the row. Add LogRecord("DELETE")
before dbDelete so undo can re-surface it. (A full FK-cascade
check on delete would require parent→child scanning; deferred.)
The fast-path SqlBulkUpdate branch still bypasses per-record
validation by design (documented) — it's gated by
`! ::oTxn:IsActive()`, so txn-active queries always take the
validated fallback.
FiveSql2 43/43 (including SAVEPOINT + ROLLBACK TO and all four CHECK/
FK tests), Harbour compat 56/56, Go test ALL PASS.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -2400,9 +2400,6 @@ METHOD RunInsert() CLASS TSqlExecutor
|
||||
|
||||
nWA := SqlExecOpenTable( cTable, cAlias )
|
||||
|
||||
/* Transaction logging */
|
||||
::oTxn:LogRecord( cAlias, RecNo(), "INSERT" )
|
||||
|
||||
dbAppend()
|
||||
IF Len( aFields ) > 0
|
||||
FOR i := 1 TO Min( Len( aFields ), Len( aValExprs ) )
|
||||
@@ -2459,6 +2456,12 @@ METHOD RunInsert() CLASS TSqlExecutor
|
||||
NEXT
|
||||
ENDIF
|
||||
|
||||
/* Transaction logging — after validation passes, so a rolled-back
|
||||
* CHECK/FK failure doesn't leave a spurious INSERT in the log at
|
||||
* the old record's position. LogRecord must also see the new
|
||||
* RecNo, which only exists post-dbAppend. */
|
||||
::oTxn:LogRecord( cAlias, RecNo(), "INSERT" )
|
||||
|
||||
/* Commit per INSERT when the WA cache is off (legacy durability
|
||||
* guarantee). With the cache on, the caller batches via an
|
||||
* explicit SqlWACacheDisable+dbCloseAll at shutdown — skipping
|
||||
@@ -2477,6 +2480,8 @@ METHOD RunUpdate() CLASS TSqlExecutor
|
||||
LOCAL cTable, aSet, xWhere, cAlias, nWA, i, nFPos, xVal
|
||||
LOCAL nAffected := 0
|
||||
LOCAL aFPos, aValuePc, pcWhere, lAllOk, cValSrc
|
||||
LOCAL aPrevVals, lValid
|
||||
LOCAL hConstraints, lHasConstraints
|
||||
|
||||
cTable := ::hQuery[ "table" ]
|
||||
aSet := ::hQuery[ "set" ]
|
||||
@@ -2563,19 +2568,56 @@ METHOD RunUpdate() CLASS TSqlExecutor
|
||||
ENDIF
|
||||
|
||||
/* Fallback: PRG scan loop — handles txn logging + non-compilable
|
||||
* expressions (subquery, complex CASE, UDF in value or WHERE). */
|
||||
* expressions (subquery, complex CASE, UDF in value or WHERE).
|
||||
*
|
||||
* Validates CHECK + FK only when the table has SQL-level
|
||||
* constraints (a `.fsc` metadata file exists). Tables created
|
||||
* via plain dbCreate have no constraints and skip the validator
|
||||
* entirely — avoids a recursive five_SQL call inside every
|
||||
* UPDATE's SqlValidateCheckRecord on transaction-active paths
|
||||
* where the re-entry would deadlock the executor state. */
|
||||
hConstraints := SqlLoadConstraints( cTable )
|
||||
lHasConstraints := Len( hConstraints[ "check" ] ) > 0 .OR. ;
|
||||
Len( hConstraints[ "fk" ] ) > 0
|
||||
|
||||
dbGoTop()
|
||||
WHILE ! Eof()
|
||||
IF xWhere == NIL .OR. SqlIsTrue( ::EvalExpr( xWhere ) )
|
||||
IF dbRLock( RecNo() )
|
||||
::oTxn:LogRecord( cAlias, RecNo(), "UPDATE" )
|
||||
aPrevVals := {}
|
||||
FOR i := 1 TO Len( aSet )
|
||||
nFPos := FieldPos( aSet[ i ][ 1 ] )
|
||||
IF nFPos > 0
|
||||
AAdd( aPrevVals, { nFPos, FieldGet( nFPos ) } )
|
||||
xVal := ::EvalExpr( aSet[ i ][ 2 ] )
|
||||
FieldPut( nFPos, xVal )
|
||||
ENDIF
|
||||
NEXT
|
||||
lValid := .T.
|
||||
IF lHasConstraints
|
||||
lValid := SqlValidateCheckRecord( cTable )
|
||||
IF lValid
|
||||
FOR i := 1 TO Len( aSet )
|
||||
nFPos := FieldPos( aSet[ i ][ 1 ] )
|
||||
IF nFPos > 0 .AND. ;
|
||||
! SqlValidateFKRecord( cTable, aSet[ i ][ 1 ], FieldGet( nFPos ) )
|
||||
lValid := .F.
|
||||
EXIT
|
||||
ENDIF
|
||||
NEXT
|
||||
ENDIF
|
||||
ENDIF
|
||||
IF ! lValid
|
||||
/* Roll back the in-memory field changes before unlocking. */
|
||||
FOR i := 1 TO Len( aPrevVals )
|
||||
FieldPut( aPrevVals[ i ][ 1 ], aPrevVals[ i ][ 2 ] )
|
||||
NEXT
|
||||
dbRUnlock( RecNo() )
|
||||
SqlExecCloseTable( cAlias, nWA )
|
||||
RETURN ::MakeError( SQL_ERR_GRAMMAR, ;
|
||||
"UPDATE constraint violation on " + cTable )
|
||||
ENDIF
|
||||
dbRUnlock( RecNo() )
|
||||
nAffected++
|
||||
ENDIF
|
||||
@@ -2608,6 +2650,10 @@ METHOD RunDelete() CLASS TSqlExecutor
|
||||
WHILE ! Eof()
|
||||
IF xWhere == NIL .OR. SqlIsTrue( ::EvalExpr( xWhere ) )
|
||||
IF dbRLock( RecNo() )
|
||||
/* Transaction log the deletion so BEGIN TRANSACTION /
|
||||
* ROLLBACK can undo it — RunInsert/RunUpdate log, RunDelete
|
||||
* used to silently skip. */
|
||||
::oTxn:LogRecord( cAlias, RecNo(), "DELETE" )
|
||||
dbDelete()
|
||||
dbRUnlock( RecNo() )
|
||||
nAffected++
|
||||
|
||||
Reference in New Issue
Block a user