Redesign procedure 2PC skip: static analysis via PLpgSQL plugin#8566
Redesign procedure 2PC skip: static analysis via PLpgSQL plugin#8566codeforall wants to merge 2 commits into
Conversation
Replace the runtime counter approach from PR #8524 with pre-execution static analysis of procedure bodies using the PLpgSQL plugin func_beg hook. This avoids partial commits followed by ERROR that the original design could produce for multi-statement procedures. Key changes: - New procedure_body_analysis.c: PLpgSQL plugin that walks the statement tree at func_beg time, counting SQL-producing statements (EXECSQL, PERFORM, DYNEXECUTE, CALL) and detecting disqualifiers (COMMIT, ROLLBACK, and all loop types). - Loops (FOR, WHILE, LOOP, FOREACH) are treated as disqualifiers since they can execute SQL multiple times, risking partial commits. - Multi-statement procedures fall back gracefully to normal coordinated transactions (2PC) instead of raising an ERROR. - Remove ProcedureNonCoordinatedExecutionCount global counter and its three reset sites in utility_hook.c. - Update tests Replace the runtime counter approach from PR #8524 with pre-execution static analysis of procedure bodies using the PLpgSQ. Adds a temporary benchmarking script, for testing/dev purposes only (will not be in final commit)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8566 +/- ##
==========================================
- Coverage 88.79% 88.71% -0.08%
==========================================
Files 287 288 +1
Lines 64053 64150 +97
Branches 8054 8063 +9
==========================================
+ Hits 56874 56913 +39
- Misses 4851 4906 +55
- Partials 2328 2331 +3 🚀 New features to boost your workflow:
|
| } | ||
| } | ||
|
|
||
| count += WalkStatementList(if_stmt->else_body, disqualified); |
There was a problem hiding this comment.
Would alternate branches of an IF statement return a count of 2 ? This, for example:
IF do_insert
THEN INSERT INTO t VALUES (x);
ELSE UPDATE t SET val = x WHERE key = y; -- count = 2 after seeing this statement ?
END IF;
If so would something like return count + max( Walk (when), Walk(else) ) ensure only one branch is accounted for ? Nbd if not, take this as a nice-to-have suggestion.
| * executes and set the ProcedureBodyIsSingleStatement flag. | ||
| * | ||
| * The analysis recursively walks the PLpgSQL statement tree counting | ||
| * SQL-producing statements (EXECSQL, PERFORM, DYNEXECUTE) and detecting |
There was a problem hiding this comment.
let's add CALL to this list, as thats the use-case that motivated this feature
| return count; | ||
| } | ||
| } | ||
| count += WalkStatementList(case_stmt->else_stmts, disqualified); |
There was a problem hiding this comment.
Any reason not to also short-circuit after walking the else branch ?
| { | ||
| *disqualified = true; | ||
| return count; | ||
| } |
There was a problem hiding this comment.
Should it allow loop bodies with 0 SQL statements ? As it stands, this procedure body would be disqualified right?
FOR i IN 1..n LOOP
total := total + i; -- no SQL statements in this loop
END LOOP;
INSERT INTO t VALUES (total); -- Total SQL statement count = 1
Nbd if additional logic make this awkward, its outside of the core use-case for the feature.
| "Multi-statement procedures gracefully fall back to the " | ||
| "normal coordinated transaction path."), | ||
| &EnableProcedureTransactionSkip, | ||
| false, |
There was a problem hiding this comment.
This looks like a positive change overall, let's enable the GUC by default to see impact on existing CI tests, and even consider removing the GUC and having this be the default behavior for procedure calls.
2 participants
Replace the runtime counter approach from PR #8524 with pre-execution static analysis of procedure bodies using the PLpgSQL plugin func_beg hook. This avoids partial commits followed by ERROR that the original design could produce for multi-statement procedures.
Key changes:
Adds a temporary benchmarking script, for testing/dev purposes only (will not be in final commit)