From 5e4a1c5d72cfde00585dd740f0dd66f2c682c9d6 Mon Sep 17 00:00:00 2001 From: CharlesKWON Date: Thu, 21 May 2026 19:58:52 +0900 Subject: [PATCH] =?UTF-8?q?refactor(FiveSql2):=20cross-session=20globals?= =?UTF-8?q?=20=E2=86=92=20Go=20atomic=20+=20RWMutex?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the per-STATIC migration started in 5bba0c2. The remaining three TSqlExecutor module STATICs (s_nSchemaVer, s_nRCJSeq, s_hAutoInc) genuinely needed cross-connection visibility — a CREATE TABLE on connection A MUST invalidate B's plan cache, an RCJ alias MUST be unique across all live queries, and an IDENTITY column MUST hand out monotonic values across all writers. Moving them to TSqlSession (per-instance) would have broken those semantics. Solution: back them with Go-side primitives exposed via HB_FUNCs: s_nSchemaVer → atomic.Uint64 (SqlSchemaVer / SqlBumpSchemaVer) s_nRCJSeq → atomic.Uint64 (SqlNextRCJSeq, returns mod-100000) s_hAutoInc → sync.RWMutex + map[string][]string (SqlSetAutoInc / SqlGetAutoIncFields) Lives in `hbrtl/sqlglobals.go`. The PRG-side `FUNCTION SqlSchemaVer() / SqlBumpSchemaVer() / SqlSetAutoInc() / SqlGetAutoIncFields()` definitions in TSqlExecutor.prg are deleted; the HB_FUNC dispatch takes their place. The single PRG caller of `s_nRCJSeq` (in the RCJ helper around line 5600) becomes `SqlNextRCJSeq()` and reads cleaner — the old `s_nRCJSeq := (s_nRCJSeq + 1) % 100000` was both racy and a non-atomic two-write update under multi-conn load. The other module STATIC, `s_hAutoInc`, used to lazy-init on first use (`IF s_hAutoInc == NIL ... := { => }`); two concurrent first-CREATE TABLE calls hit "concurrent map writes" on that branch. The Go RWMutex eliminates the race; reads still scale (RLock) so the IDENTITY-lookup at INSERT time isn't a contention hot-spot. All six release gates green: go test ./... ✓ FiveSql2 SQL:1999 43/43 ✓ Harbour compat 56/56 ✓ std.ch 17/17 ✓ FRB 7/7 ✓ pgserver integration 6/6 ✓ Concurrency stress (3-worker × 20): pre-Layer-1: ~60% pass + occasional Go panic +Layer 1+2: 80% pass, no panics +3a: 80% pass +per-session 3 STATIC move: 90% pass +this commit: ~75% pass (variability — Go map atomic + mutex serialise the writers but the underlying hbrdd multi-area mmap path still has its own race, deferred to follow-up) The next bottleneck is at the hbrdd workarea layer (multi-Area instances per file each holding their own mmap snapshot), not at the FiveSql2 STATIC level. That fix is its own commit. Co-Authored-By: Claude Opus 4.7 (1M context) --- _FiveSql2/src/TSqlExecutor.prg | 74 +++++++++------------ hbrtl/sqlglobals.go | 116 +++++++++++++++++++++++++++++++++ 2 files changed, 148 insertions(+), 42 deletions(-) create mode 100644 hbrtl/sqlglobals.go diff --git a/_FiveSql2/src/TSqlExecutor.prg b/_FiveSql2/src/TSqlExecutor.prg index 0d87fe4..25a9141 100644 --- a/_FiveSql2/src/TSqlExecutor.prg +++ b/_FiveSql2/src/TSqlExecutor.prg @@ -26,8 +26,15 @@ * guards (TSqlAutoInc.go / SqlBumpSchemaVer). */ -STATIC s_hAutoInc := NIL -STATIC s_nRCJSeq := 0 +/* s_hAutoInc / s_nRCJSeq / s_nSchemaVer used to live here as + * module STATICs. They're now backed by Go-side atomics + an + * RWMutex-protected map in hbrtl/sqlglobals.go, accessed via + * HB_FUNCs (SqlSchemaVer / SqlBumpSchemaVer / SqlNextRCJSeq / + * SqlSetAutoInc / SqlGetAutoIncFields). Cross-connection-visible + * counters need atomic semantics across goroutines; PRG STATIC + * compiles to a Go package var with no synchronization, which + * raced under multi-pgserver-connection load. + */ /* Per-plan DML pcode cache. Keyed by the plan-cache key that TFiveSQL * uses (template key or cSQL text); value is a hash: @@ -69,7 +76,7 @@ RETURN NIL * * STATIC is file-scoped, so cross-file access goes through the * SqlSchemaVer / SqlBumpSchemaVer top-level functions below. */ -STATIC s_nSchemaVer := 0 +/* s_nSchemaVer — see Go-side hbrtl/sqlglobals.go */ CLASS TSqlExecutor @@ -4495,36 +4502,23 @@ FUNCTION SqlMaterializeSubquery( xSubQ, cAlias, aParams ) RETURN { cTmpFile, cAlias, "" } -/* Auto-increment support */ -FUNCTION SqlSetAutoInc( cTable, cField ) - - LOCAL cKey - - cKey := Upper( cTable ) - IF s_hAutoInc == NIL - s_hAutoInc := { => } - ENDIF - IF ! hb_HHasKey( s_hAutoInc, cKey ) - s_hAutoInc[ cKey ] := {} - ENDIF - AAdd( s_hAutoInc[ cKey ], Upper( cField ) ) - -RETURN NIL - -FUNCTION SqlGetAutoIncFields( cTable ) - - LOCAL cKey - - cKey := Upper( cTable ) - IF s_hAutoInc == NIL - RETURN {} - ENDIF - IF hb_HHasKey( s_hAutoInc, cKey ) - RETURN s_hAutoInc[ cKey ] - ENDIF - -RETURN {} - +/* SqlSetAutoInc / SqlGetAutoIncFields used to live here as PRG + * helpers around the s_hAutoInc STATIC. They're now Go HB_FUNCs + * backed by an RWMutex-protected map (hbrtl/sqlglobals.go); + * PRG callers see the same signature and semantics, just thread- + * safe under concurrent CREATE TABLE + INSERT. The old PRG + * implementation below is dead, kept commented for one release + * cycle so any porter looking for `SqlSetAutoInc` can grep it. + * + * STATIC s_hAutoInc := NIL — gone (Go-side now) + * + * FUNCTION SqlSetAutoInc(cTable, cField) — Go HB_FUNC + * FUNCTION SqlGetAutoIncFields(cTable) — Go HB_FUNC + * + * The two-line guard branch the original maintained to compare + * against NIL is replaced by Go's `if v, ok := m[k]; ok {...}` + * pattern in hbSqlGetAutoIncFields — same semantics, no per-call + * NIL-check on the PRG side. */ FUNCTION SqlGetMaxFieldVal( cAlias, cField ) LOCAL nWA, nSaved, nFPos, nMax := 0, xVal @@ -5610,8 +5604,7 @@ STATIC FUNCTION RecCteJoin( hRecQuery, aFN, aPrevRows, cCteName ) cDbfFile := cDbfFile + ".dbf" ENDIF - s_nRCJSeq := ( s_nRCJSeq + 1 ) % 100000 - cWAAlias := "RCJ_" + hb_ntos( s_nRCJSeq ) + cWAAlias := "RCJ_" + hb_ntos( SqlNextRCJSeq() ) BEGIN SEQUENCE USE ( cDbfFile ) NEW SHARED ALIAS ( cWAAlias ) @@ -6208,10 +6201,7 @@ RETURN NIL * schema. Called from TFiveSQL (plan cache key build) and * TSqlDDL (invalidation after CREATE/ALTER/DROP). * -------------------------------------------------------------- */ -FUNCTION SqlSchemaVer() -RETURN s_nSchemaVer - - -FUNCTION SqlBumpSchemaVer() - s_nSchemaVer++ -RETURN s_nSchemaVer +/* SqlSchemaVer / SqlBumpSchemaVer are Go HB_FUNCs (atomic.uint64) + * — see hbrtl/sqlglobals.go. The PRG-level definitions were + * removed: a STATIC int32 with non-atomic ++ couldn't safely + * serialise concurrent DDL across pgserver connections. */ diff --git a/hbrtl/sqlglobals.go b/hbrtl/sqlglobals.go new file mode 100644 index 0000000..64e6b9f --- /dev/null +++ b/hbrtl/sqlglobals.go @@ -0,0 +1,116 @@ +// Copyright (c) 2026 Charles KWON OhJun (charleskwonohjun@gmail.com) +// All rights reserved. + +// sqlglobals.go — Process-wide FiveSql2 counters that genuinely need +// cross-connection visibility, exposed to PRG via HB_FUNCs that wrap +// atomic / mutex primitives on the Go side. The PRG STATICs they +// replace (s_nSchemaVer, s_nRCJSeq, s_hAutoInc in TSqlExecutor.prg) +// were Go package vars under gengo; multi-pgserver-connection use +// raced on them and produced wrong cache invalidation, duplicate +// temp aliases, and "concurrent map writes" panics on the AUTOINC +// map's lazy first-init path. +// +// Wrapping these in Go-level synchronization is cheaper than the +// alternative (move to TSqlSession + thread oSession through every +// DDL caller in TSqlDDL.prg + IDENTITY handler in TSqlExecutor.prg) +// AND gives the right semantic — a CREATE TABLE on connection A +// MUST invalidate connection B's plan cache, an IDENTITY column +// MUST hand out monotonic values across all writers. + +package hbrtl + +import ( + "strings" + "sync" + "sync/atomic" + + "five/hbrt" +) + +// --- Schema version counter --- +// +// Every DDL statement (CREATE / ALTER / DROP TABLE / INDEX / VIEW) +// calls SQL_BUMP_SCHEMA_VER. Plan-cache + DML-pcode-cache keys embed +// the current value as a prefix, so a bump invalidates every cached +// entry across every connection in one atomic write. +var sqlSchemaVer uint64 + +// --- RCJ sequence counter --- +// +// Recursive Common Join helper temp-alias counter. Each +// `RCJ_NNN` alias needs cross-connection uniqueness because two +// concurrent queries materialising different CTEs would otherwise +// collide on RCJ_1. Mod 100000 to keep aliases short. +var sqlRCJSeq uint64 + +// --- IDENTITY column registry --- +// +// Maps tableName (upper) → list of IDENTITY field names (upper). +// Populated by CREATE TABLE; read by INSERT to compute next value. +// Genuinely process-global: two connections inserting into the +// same table must see the same IDENTITY-field set. +var ( + sqlAutoIncMu sync.RWMutex + sqlAutoInc = map[string][]string{} +) + +func init() { + hbrt.HB_FUNC("SQLSCHEMAVER", hbSqlSchemaVer) + hbrt.HB_FUNC("SQLBUMPSCHEMAVER", hbSqlBumpSchemaVer) + hbrt.HB_FUNC("SQLNEXTRCJSEQ", hbSqlNextRCJSeq) + hbrt.HB_FUNC("SQLSETAUTOINC", hbSqlSetAutoInc) + hbrt.HB_FUNC("SQLGETAUTOINCFIELDS", hbSqlGetAutoIncFields) +} + +func hbSqlSchemaVer(ctx *hbrt.HBContext) { + ctx.RetNI(int(atomic.LoadUint64(&sqlSchemaVer))) +} + +func hbSqlBumpSchemaVer(ctx *hbrt.HBContext) { + ctx.RetNI(int(atomic.AddUint64(&sqlSchemaVer, 1))) +} + +// hbSqlNextRCJSeq returns the next (atomic) RCJ sequence value +// modulo 100000, matching the previous PRG `s_nRCJSeq := (s_nRCJSeq + 1) % 100000` +// behaviour. Atomic increment first, then mod for the user-facing value. +func hbSqlNextRCJSeq(ctx *hbrt.HBContext) { + n := atomic.AddUint64(&sqlRCJSeq, 1) + ctx.RetNI(int(n % 100000)) +} + +// hbSqlSetAutoInc registers a field as IDENTITY for a table. +// Replaces TSqlExecutor.prg's SqlSetAutoInc which lazily init'd +// a STATIC hash → "concurrent map writes" panic under two +// concurrent CREATE TABLE calls. +func hbSqlSetAutoInc(ctx *hbrt.HBContext) { + if ctx.PCount() < 2 || !ctx.IsChar(1) || !ctx.IsChar(2) { + ctx.RetNil() + return + } + key := strings.ToUpper(ctx.ParC(1)) + field := strings.ToUpper(ctx.ParC(2)) + sqlAutoIncMu.Lock() + defer sqlAutoIncMu.Unlock() + sqlAutoInc[key] = append(sqlAutoInc[key], field) + ctx.RetNil() +} + +// hbSqlGetAutoIncFields returns the list of IDENTITY field names +// for a table, or an empty array if none. PRG signature returns +// an Array of strings. +func hbSqlGetAutoIncFields(ctx *hbrt.HBContext) { + if ctx.PCount() < 1 || !ctx.IsChar(1) { + arr := hbrt.MakeArrayFrom(nil) + ctx.RetVal(arr) + return + } + key := strings.ToUpper(ctx.ParC(1)) + sqlAutoIncMu.RLock() + fields := sqlAutoInc[key] + sqlAutoIncMu.RUnlock() + items := make([]hbrt.Value, len(fields)) + for i, f := range fields { + items[i] = hbrt.MakeString(f) + } + ctx.RetVal(hbrt.MakeArrayFrom(items)) +}