Skip to content

Commit 427da91

Browse files
committed
fix: 2PC connection pinning + scaffolding audit
Two big items from the review are now correct: - 2PC actually works against real backends (pinning fix) - Half-implemented scaffolding has been either finished or deleted 1115/1152 tests pass (0 failures, 37 skipped require Docker). +12 new tests for pinning and coercion, existing tests still pass. ----- 2PC connection pinning (the critical fix) ----- The DistributedTransactionManager used to route every phase 1 and phase 2 SQL through RemoteExecutor::execute_dml, which on ThreadSafeMultiRemoteExecutor checks out a fresh pooled connection for each call. That silently broke 2PC against real backends for both dialects: - MySQL XA: "Once XA START has been issued, the XA transaction is associated with the current session." XA END and XA PREPARE must be on the same session; a fresh pooled connection would respond with XAER_NOTA / "Unknown XID". - PostgreSQL: PREPARE TRANSACTION operates on the session's currently active transaction. BEGIN on one connection, PREPARE on another -- the second connection sees "no transaction in progress" and the PREPARE fails. The existing unit tests didn't catch this because MockDistributedExecutor didn't enforce session identity, but a real MySQL or PostgreSQL backend would have silently failed every distributed commit. Fix: new RemoteSession abstraction in remote_session.h. A session owns a specific physical connection for its lifetime; every execute/execute_dml call goes to that same connection. RemoteExecutor gets a new checkout_session(backend) virtual with a default nullptr implementation (legacy fallback). Implementations: - ThreadSafeMultiRemoteExecutor returns a PooledMySQLSession that holds a MYSQL* checked out from the ConnectionPool until destruction. Poisoned sessions close the connection rather than returning it to the pool. - MockDistributedExecutor returns a MockSession that records calls under a unique session id, so tests can verify phase 1 and phase 2 landed on the same session (which the new pinning tests now do explicitly). DistributedTransactionManager holds unordered_map<backend, unique_ptr<RemoteSession>> sessions_ and uses it for every phase 1/phase 2/rollback statement on an enlisted backend. Sessions are acquired in enlist_backend, held across phases, and released on commit()/rollback() by clearing the map (destructors return connections to the pool). If an executor returns nullptr from checkout_session, the manager falls back to the legacy unpinned path -- works for non-pooled executors and mock tests. Real pooled executors now take the pinned path automatically. Also added a new public method: DmlResult execute_participant_dml(backend, sql) which routes in-transaction DML to the pinned session. Session. execute_statement does NOT yet auto-route through this -- that's a follow-up that requires Session to know about the txn manager. New tests (DistributedTxnPinning suite): - AllMysqlPhasesOnSameSession: XA START, XA END, XA PREPARE, XA COMMIT all land on one session id. - TwoBackendsUseTwoDistinctSessions: different backends get different sessions. - PostgreSqlPrepareAndCommitOnSameSession: BEGIN, PREPARE TRANSACTION, COMMIT PREPARED share a session. - RollbackPinsSession: XA END + XA ROLLBACK stay on the same session as XA START. - UnpinnedFallbackStillWorks: executors reporting no session support still go through the legacy path. - ExecuteParticipantDmlUsesSameSession: in-transaction DML through execute_participant_dml lands on the same session as phase 1 and phase 2. ----- Scaffolding audit ----- Three pieces of half-implemented scaffolding have been either completed or deleted: 1. projection_pruning.h was a documented no-op that the optimizer pipeline still called. Deleted the file and removed the call. The Project operator already does column subsetting at eval time, so the pruning rule's only remaining value was inserting slimming Projects above Scans in long chains -- a real optimization but not a correctness issue and out of scope. Optimizer is now honest about running exactly three rules. 2. INTERVAL type (Value::TAG_INTERVAL, SqlType::INTERVAL, value_interval() constructor, union member, tag_kind_map entries, local_txn serialization, sqlengine formatter, mysql_server formatter). Declared but no producer existed anywhere in the codebase: the parser doesn't recognize INTERVAL literals, expression_eval never constructs one, neither remote executor maps a database type to it. Removed all of it. Re-add alongside a real producer (e.g. PostgreSQL INTERVAL OID parsing, or a DATE_ADD(date, INTERVAL N unit) parser path) if and when that work is done. 3. Coercion.h had a default-returns-null behavior that looked like missing support but was actually semantically correct for PostgreSQL's strict type rules. However it WAS missing a real feature: string -> temporal coercion in MySQL. Queries like `WHERE date_col = '2026-04-10'` would fall through to a NULL comparison because there was no path from STRING to DATE. Added to MySQL CoercionRules: - STRING -> DATE (parses "YYYY-MM-DD" leniently) - STRING -> TIME (parses "HH:MM:SS[.uuuuuu]") - STRING -> DATETIME / TIMESTAMP (parses full datetime) - DATE -> DATETIME/TIMESTAMP (promoted to midnight UTC) - DATETIME -> DATE (floor-divides to day boundary) PostgreSQL CoercionRules gets the within-temporal promotions (DATE -> TIMESTAMP) but still rejects string -> temporal with NULL, matching PG's strict rules that require explicit CAST. +13 new coercion tests covering MySQL lenient paths and PG strict behavior. ----- Still deferred ----- - Session.execute_statement automatic enlistment + routing through execute_participant_dml. Requires Session to hold a reference to the TransactionManager, which is currently decoupled. Not done in this commit to keep the refactor scoped. - The connection-pinning fallback path (when checkout_session returns nullptr) is only triggered by non-pooled executors like the mock. Real workloads always take the pinned path, so the legacy code is only kept for test compatibility.
1 parent 15d65dd commit 427da91

16 files changed

Lines changed: 909 additions & 234 deletions

include/sql_engine/coercion.h

Lines changed: 80 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
#include "sql_engine/types.h"
55
#include "sql_engine/value.h"
6+
#include "sql_engine/datetime_parse.h"
67
#include "sql_parser/common.h" // Dialect, StringRef
78
#include "sql_parser/arena.h" // Arena
89
#include <cstdlib>
@@ -179,6 +180,61 @@ inline Value CoercionRules<Dialect::MySQL>::coerce_value(const Value& val, Value
179180
if (val.tag == Value::TAG_DOUBLE) return value_bool(val.double_val != 0.0);
180181
return value_null();
181182
}
183+
case Value::TAG_DATE: {
184+
// String -> DATE: parse "YYYY-MM-DD" (MySQL is lenient).
185+
if (val.tag == Value::TAG_STRING && val.str_val.ptr && val.str_val.len > 0) {
186+
char buf[32];
187+
uint32_t n = val.str_val.len < 31 ? val.str_val.len : 31;
188+
for (uint32_t i = 0; i < n; ++i) buf[i] = val.str_val.ptr[i];
189+
buf[n] = '\0';
190+
return value_date(datetime_parse::parse_date(buf));
191+
}
192+
if (val.tag == Value::TAG_DATETIME) {
193+
// Truncate datetime to date by floor-dividing by microseconds/day.
194+
int64_t us_per_day = 86400LL * 1000000LL;
195+
int64_t days = val.datetime_val / us_per_day;
196+
if (val.datetime_val < 0 && val.datetime_val % us_per_day != 0) --days;
197+
return value_date(static_cast<int32_t>(days));
198+
}
199+
return value_null();
200+
}
201+
case Value::TAG_TIME: {
202+
if (val.tag == Value::TAG_STRING && val.str_val.ptr && val.str_val.len > 0) {
203+
char buf[32];
204+
uint32_t n = val.str_val.len < 31 ? val.str_val.len : 31;
205+
for (uint32_t i = 0; i < n; ++i) buf[i] = val.str_val.ptr[i];
206+
buf[n] = '\0';
207+
return value_time(datetime_parse::parse_time(buf));
208+
}
209+
return value_null();
210+
}
211+
case Value::TAG_DATETIME:
212+
case Value::TAG_TIMESTAMP: {
213+
if (val.tag == Value::TAG_STRING && val.str_val.ptr && val.str_val.len > 0) {
214+
char buf[64];
215+
uint32_t n = val.str_val.len < 63 ? val.str_val.len : 63;
216+
for (uint32_t i = 0; i < n; ++i) buf[i] = val.str_val.ptr[i];
217+
buf[n] = '\0';
218+
int64_t us = datetime_parse::parse_datetime(buf);
219+
return (target == Value::TAG_DATETIME)
220+
? value_datetime(us)
221+
: value_timestamp(us);
222+
}
223+
// DATE promoted to DATETIME at midnight UTC.
224+
if (val.tag == Value::TAG_DATE) {
225+
int64_t us = static_cast<int64_t>(val.date_val) * 86400LL * 1000000LL;
226+
return (target == Value::TAG_DATETIME)
227+
? value_datetime(us)
228+
: value_timestamp(us);
229+
}
230+
// DATETIME and TIMESTAMP share representation; allow interchange.
231+
if (val.tag == Value::TAG_DATETIME || val.tag == Value::TAG_TIMESTAMP) {
232+
return (target == Value::TAG_DATETIME)
233+
? value_datetime(val.datetime_val)
234+
: value_timestamp(val.timestamp_val);
235+
}
236+
return value_null();
237+
}
182238
default:
183239
return value_null();
184240
}
@@ -233,7 +289,12 @@ inline Value CoercionRules<Dialect::PostgreSQL>::coerce_value(const Value& val,
233289
if (val.tag == target) return val;
234290
if (val.is_null()) return value_null();
235291

236-
// PostgreSQL: only within-category promotions
292+
// PostgreSQL's implicit coercion rules are deliberately strict: no
293+
// implicit string<->numeric, no implicit int<->bool, no implicit
294+
// string<->temporal. Users must write explicit CAST(col AS type) for
295+
// those. We return value_null() for the cases PG rejects, which
296+
// surfaces as a NULL in the calling comparison -- the same end
297+
// result PG would produce (via an error, in its case).
237298
switch (target) {
238299
case Value::TAG_INT64: {
239300
if (val.tag == Value::TAG_BOOL) return value_int(val.bool_val ? 1 : 0);
@@ -252,6 +313,24 @@ inline Value CoercionRules<Dialect::PostgreSQL>::coerce_value(const Value& val,
252313
// PostgreSQL does not implicitly convert int to bool
253314
return value_null();
254315
}
316+
case Value::TAG_DATETIME:
317+
case Value::TAG_TIMESTAMP: {
318+
// Within-temporal promotion: DATE -> DATETIME/TIMESTAMP at
319+
// midnight UTC. PG considers this implicit.
320+
if (val.tag == Value::TAG_DATE) {
321+
int64_t us = static_cast<int64_t>(val.date_val) * 86400LL * 1000000LL;
322+
return (target == Value::TAG_DATETIME)
323+
? value_datetime(us)
324+
: value_timestamp(us);
325+
}
326+
if (val.tag == Value::TAG_DATETIME || val.tag == Value::TAG_TIMESTAMP) {
327+
return (target == Value::TAG_DATETIME)
328+
? value_datetime(val.datetime_val)
329+
: value_timestamp(val.timestamp_val);
330+
}
331+
// String -> timestamp: NOT implicit; requires CAST.
332+
return value_null();
333+
}
255334
default:
256335
return value_null();
257336
}

0 commit comments

Comments
 (0)