| From: | Henson Choi <assam258(at)gmail(dot)com> |
|---|---|
| To: | jian he <jian(dot)universality(at)gmail(dot)com>, Tatsuo Ishii <ishii(at)postgresql(dot)org> |
| Cc: | zsolt(dot)parragi(at)percona(dot)com, sjjang112233(at)gmail(dot)com, vik(at)postgresfriends(dot)org, er(at)xs4all(dot)nl, jacob(dot)champion(at)enterprisedb(dot)com, david(dot)g(dot)johnston(at)gmail(dot)com, peter(at)eisentraut(dot)org, li(dot)evan(dot)chao(at)gmail(dot)com, pgsql-hackers(at)postgresql(dot)org |
| Subject: | Re: Row pattern recognition |
| Date: | 2026-06-11 10:29:48 |
| Message-ID: | CAAAe_zDYxq0d3exCDwvKncD0kaL2uehDir6HXo4r5DXMitKrSg@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi hackers,
This refreshes the v48 series [1]: Jian He's review of the v47-000x
cleanup patches [2] (my point-by-point reply [3]) is now applied as
nocfbot-0069..0077. nocfbot-0001..0068 are unchanged from the last post
[1] (rebase only), except nocfbot-0039, which is voided here (see below).
Resolved since the last post:
Jian He, round 5 -- the v47-0001..0004 patches plus the inline comments
[2]. Applied across nocfbot-0069..0077 (issue map below).
For Tatsuo -- two calls I'd like from you before this settles:
1. nocfbot-0073 moves the DEFINE volatility rejection out of parse
analysis into the planner, per the convention Jian and Tom noted. Two
things to weigh: it is a small behavior change -- a volatile DEFINE
hidden in a view is now rejected when the view is read, not at CREATE
VIEW -- and the planner has no ParseState, so the error cursor is
reconstructed from debug_query_string, a first at the optimizer stage.
If either gives you pause, I'll rework it or split nocfbot-0073 out for
separate review ahead of the cosmetic patches.
2. LOWPRICE/UP/DOWN casing. I've left the variables upper-case (the
standard and Oracle show them so), but EXPLAIN and deparse lower-case
them today, so the examples and the actual output disagree. If you'd
prefer the section lower-cased, I'll do it as a doc patch.
Deferred:
v47-0003 -- in its final verification stage I'm keeping logic changes to
those backed by a clear review or test finding. The two new
group-merge tests are folded in (nocfbot-0076); the in-place
list_delete_nth_cell reshaping and the pg_mul_s32_overflow guard are
held out and, absent a concrete finding, will go in as a separate
patch rather than this fold.
On cfbot:
The SIGILL cfbot reports against this entry is not from RPR. After Andres
Freund noted cfbot failing inside the JIT on this patch [4], I found that
any JIT-compiled query crashes the same way on the CI's libLLVM 19.1 +
ASAN
build; it reproduces on plain master with a trivial aggregate, landing in
libLLVM's decodeDiscriminator. I reported it as its own issue with a
standalone reproducer [5]; its CF entry is only there to surface the
problem on cfbot, and I'll withdraw it once the community has taken note.
Not a blocker for this series.
Attached: the v47 feature series (v47-0001..0009) rebased onto current
master, plus the full incremental series (nocfbot-0001..0077) on top.
Base:
9d141466ff0 2026-06-10 Undo thinko in commit e78d1d6d4.
Already posted (nocfbot-0001..0068): unchanged since v48 [1], rebase only,
except nocfbot-0039 (voided; no patch). Titles for reference:
nocfbot-0001 Add DEFINE non-volatile baseline to rpr_integration B9
nocfbot-0002 Unify RPR DEFINE walkers and reject volatile callees
nocfbot-0003 Cover RPR empty-match path with EXPLAIN tests; fix stale
XXX comments
nocfbot-0004 Reclassify DEFINE qualifier check and reword diagnostic to
"expression"
nocfbot-0005 Sync stale comments on DEFINE/PATTERN handling
nocfbot-0006 Add trailing commas to RPR enum definitions
nocfbot-0007 Remove optional outer parentheses from ereport() calls in
RPR files
nocfbot-0008 Add high-water mark tracking to NFA visited bitmap reset
nocfbot-0009 Document DEFINE subquery rejection as intentional
over-rejection
nocfbot-0010 Remove duplicate #include in nodeWindowAgg.c
nocfbot-0011 Normalize SQL/RPR standard references
nocfbot-0012 Add rpr_integration B7 cases for RPR in recursive query
nocfbot-0013 Reject row pattern recognition in recursive queries
nocfbot-0014 Enhance README.rpr per Tatsuo Ishii's review
nocfbot-0015 Round out README.rpr WindowAggState field coverage
nocfbot-0016 Add raw_expression_tree_walker coverage for RPR raw nodes
nocfbot-0017 Enhance README.rpr per Jian He's review
nocfbot-0018 Clarify execRPR.c comments and tighten an Assert
nocfbot-0019 Change nfa_add_state_unique signature from bool to void
nocfbot-0020 Add reluctant bounded mid-band test to rpr_nfa
nocfbot-0021 Define RPR absorption terminology in README.rpr
nocfbot-0022 Document the get_reduced_frame_status cascade invariant
nocfbot-0023 Explain the completed-head-context branch in
update_reduced_frame
nocfbot-0024 Tighten the RPR frame-boundary check from >= to ==
nocfbot-0025 Reject single-row window frame in row pattern recognition
nocfbot-0026 Remove the redundant zero check on the RPR frame ending
offset
nocfbot-0027 Restore findTargetlistEntrySQL99 to static (per Tatsuo)
nocfbot-0028 Clarify RPR comments
nocfbot-0029 Rename RPR NFA constructors to make/clone
nocfbot-0030 Demote RPR nfaVisitedNWords to a local
nocfbot-0031 Rename the AST-level prefix/suffix rewrite from
"absorption" to "merging"
nocfbot-0032 Drop non-standard per-group banner labels from RPR
forward declarations
nocfbot-0033 Clarify that ExecRPRCleanupDeadContexts always frees the
failed context
nocfbot-0034 Correct stale RPR comments and document a defensive
window check
nocfbot-0035 Fix unsafe (A{n,})* quantifier flattening
nocfbot-0036 Avoid INF-valued quantifier bound in consecutive-merge
nocfbot-0037 Fix count slot leak in row pattern recognition absorption
nocfbot-0038 Demote dead runtime checks in the RPR executor to
assertions
nocfbot-0039 (voided -- no patch; the v48 DEFINE memory-leak fix reset
the wrong context and is backed out here, being re-fixed on
a separate thread)
nocfbot-0040 Honor reluctant quantifier for non-leading optional RPR
variables
nocfbot-0041 Reject column-less compound navigation
nocfbot-0042 Reserve the high varId nibble for RPR control elements
nocfbot-0043 Generalize quantifier multiplication
nocfbot-0044 Tidy up row pattern recognition pattern compilation
nocfbot-0045 Drop redundant parentheses from row pattern recognition
ereports
nocfbot-0046 Tidy up forward declarations and helper placement
nocfbot-0047 Update the varId documentation
nocfbot-0048 Reformat the design-decisions chapter in README.rpr
nocfbot-0049 Point to the absorption-analysis docs from the RPR flag
definitions
nocfbot-0050 Fix signed-integer overflow in frame-end clamp
nocfbot-0051 Rework row pattern EXPLAIN deparser to fix grouped
alternation branches
nocfbot-0052 Handle row pattern navigation nodes in exprTypmod and
isSimpleNode
nocfbot-0053 Restore the error cursor for too many row pattern variables
nocfbot-0054 Test deparse of an inline row pattern window
nocfbot-0055 Fix a mislabeled INITIAL test
nocfbot-0056 Reject invalid column references in row pattern DEFINE
clauses
nocfbot-0057 Fix shortest match for reluctant nullable quantifiers
nocfbot-0058 Compare varno when preserving DEFINE-referenced columns
nocfbot-0059 Allow a row pattern quantifier with no space before the
alternation operator
nocfbot-0060 Fix outdated function and file references in RPR docs
nocfbot-0061 Assert that row pattern nesting depth never aliases the
RPR_DEPTH_NONE sentinel
nocfbot-0062 Invalidate the row pattern nav slot cache when a window
partition changes
nocfbot-0063 Tidy up formatting in row pattern recognition code
nocfbot-0064 Modernize idioms in row pattern recognition code
nocfbot-0065 Use width-explicit integer limit macros
nocfbot-0066 Tidy up row pattern recognition regression test comments
nocfbot-0067 Add row pattern recognition negative and coverage tests
nocfbot-0068 Use foreach_node and friends in RPR code
New since v48 [1] -- nocfbot-0069..0077, in series order. These apply
Jian He's round-5 review [2] (issue map below). Behavior-changing patches
are tagged [behavior change].
nocfbot-0069 Move RPR EXPLAIN marker description and add a worked
example
The double-quote (") and single-quote (') absorption-marker
description moves out of the advanced.sgml tutorial into perform.sgml
"Using EXPLAIN" -- where PG documents EXPLAIN output markers -- with a
worked example showing Pattern: (up' down')+".
nocfbot-0070 Clarify the non-trivial quantifier comments in row pattern
recognition
All inline "non-trivial quantifier" comments now read "non-trivial
quantifier (not {1,1}).", matching the form already used in the
function header.
nocfbot-0071 Rename allocateRPRPattern to makeRPRPattern in row pattern
recognition
Matches the makeNode/makeRPR* convention; buildRPRPattern keeps its
name (make = low-level allocation, build = higher-level compilation).
nocfbot-0072 Remove the unused reluctant_location field from
RPRPatternNode
The field was only written and propagated, never read back; the
reluctance stays as a plain bool.
nocfbot-0073 Reject volatile DEFINE expressions in the planner, not at
parse time [behavior change]
Moves the DEFINE volatility rejection out of parse analysis into the
planner. The DEFINE walker keeps only the RPR-specific nav checks.
See the note to Tatsuo above (view-read timing; debug_query_string
error cursor).
nocfbot-0074 Drop query_jumble_ignore from the RPR trailing_alt field
splitRPRTrailingAlt finalizes trailing_alt to false before
JumbleQuery runs, so the attribute was a no-op.
nocfbot-0075 Quote the offending token in RPR quantifier syntax errors
v47-0001: the offending token is now quoted -- invalid token "%s"
after range quantifier, and likewise after "*"/"+" quantifier. (Known
gap: the "invalid quantifier combination" else-branch, e.g. A? ??,
still doesn't quote its token -- follow-up.)
nocfbot-0076 Clean up RPR regression test comments and add group-merge
tests
v47-0002: internal static function names dropped from the regress
comments. Plus the two new group-merge tests from v47-0003 (cannot-
merge prefix; 5-way prefix+suffix), rebuilt against the current code
so the expected output matches.
nocfbot-0077 Improve comment accuracy in RPR parse analysis and tests
v47-0004: rpSkipTo/initial comments reworded as flag assignments,
keeping the "PATTERN AST" wording (it is an AST, not a flag) and the
deparse note.
Issue-to-patch map (Jian He, round 5 [2] -> decision -> landing):
EXPLAIN marker -> explain.sgml accept (perform.sgml) -> nocfbot-0069
LOWPRICE/UP/DOWN casing defer (Tatsuo) -> (no change)
non-trivial quantifier (not {1,1}) accept -> nocfbot-0070
allocateRPRPattern -> make rename accept -> nocfbot-0071
reluctant_location field accept (remove) -> nocfbot-0072
nav_volatile_func_checker move to planner -> nocfbot-0073
query_jumble_ignore on trailing_alt accept (remove) -> nocfbot-0074
v47-0001 quote offending token accept -> nocfbot-0075
v47-0002 drop fn names in comments accept -> nocfbot-0076
v47-0003 new merge tests accept -> nocfbot-0076
v47-0003 in-place + overflow guard hold (verification) -> (no change)
v47-0004 misc comment accuracy accept (keep AST) -> nocfbot-0077
Still to come, each tracked on the thread where I raised it and to land in
a later fold once its approach is agreed:
- The DEFINE-evaluation memory leak (nocfbot-0039's interim fix turned it
into a use-after-free, so it is voided here); the proper fix is a
dedicated DEFINE ExprContext that resolves both.
- The PREV/NEXT/FIRST/LAST namespace collision (the same change also
rejects the nested-nav-in-offset case).
Quality work to run alongside, once those land: Valgrind (leak and
use-after-free) and standing gcov coverage to catch untested planner paths,
on Linux. Independently of the cfbot JIT issue, I also intend to review the
RPR spool's tuplestore handling -- mark position and trimming in particular.
Longer term, and out of scope for this CF entry:
- Smaller documentation and error-message follow-ups (glossary, the
bounded-quantifier message, README wording).
- Short-circuit / tri-state DEFINE evaluation, as a separate series.
- SEEK clause support (SQL:2016).
- Empty pattern PATTERN () -- correctly rejected under R020 today, and
meaningful only once R010 lands.
- Relaxing the DEFINE-subquery over-rejection where the standard permits
it (the subquery does no RPR of its own and makes no outer reference).
- Prefix-pattern absorption (an optimization; designed and intentionally
split out of v47 as its own series).
- R010 (MATCH_RECOGNIZE in the FROM clause) and the shared RPRContext it
would back.
Please let me know if any of the slicing or grouping looks off.
Thanks again to Jian for the careful reading.
Best regards,
Henson
[1] The v48 fold (2026-06-09):
https://postgr.es/m/CAAAe_zCd1pA6vCaMD7e3hB3Ou+=mZziB1=CO_tBybuAiE5K=vQ@mail.gmail.com
[2] Jian's review of the v47-000x patches (2026-06-09):
https://postgr.es/m/CACJufxFnwdQSApt2vWwYCd0gtf+JjFDxT2hbxHi=+dhFJc+-1g@mail.gmail.com
[3] My point-by-point reply (2026-06-09):
https://postgr.es/m/CAAAe_zATnkqsbLYDj8MJV1TriX9Wi0wShDg3nK3qYpiupKwhFA@mail.gmail.com
[4] Andres Freund's note that cfbot was failing inside the JIT on this
patch:
https://postgr.es/m/p7r5bekdbl2zcazid7agvfo2nfnq5bim2a5jkckqygld32n325@fctfp6ou6qnb
[5] LLVM JIT: any JIT-compiled query crashes (SIGILL) on a libLLVM 19 + ASAN
build (2026-06-10; CF entry 6870):
https://postgr.es/m/CAAAe_zD6jGANGZFKnHLKHF8izqmqqJbVe=NOuERFwN_Spj5VOA@mail.gmail.com
| From | Date | Subject | |
|---|---|---|---|
| Next Message | solai v | 2026-06-11 10:31:36 | Re: extend JSON_TABLE top level path expression |
| Previous Message | Ewan Young | 2026-06-11 09:59:55 | Re: Make SPI_prepare argtypes argument const |