From 325fe51656ba7e52b073d2383234e9771daa3b97 Mon Sep 17 00:00:00 2001 From: CharlesKWON Date: Sat, 18 Apr 2026 23:24:14 +0900 Subject: [PATCH] fix(fivesql2): DML transaction + constraint ordering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- _FiveSql2/src/TSqlExecutor.prg | 54 +++++++++++++++++++++++++++++++--- 1 file changed, 50 insertions(+), 4 deletions(-) diff --git a/_FiveSql2/src/TSqlExecutor.prg b/_FiveSql2/src/TSqlExecutor.prg index 1f0b7d8..2e75f46 100644 --- a/_FiveSql2/src/TSqlExecutor.prg +++ b/_FiveSql2/src/TSqlExecutor.prg @@ -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++