Re: Row pattern recognition
Henson Choi <assam258@gmail.com>
From: Henson Choi <assam258@gmail.com>
To: jian.universality@gmail.com
Cc: ishii@postgresql.org, pgsql-hackers@postgresql.org, zsolt.parragi@percona.com, sjjang112233@gmail.com, vik@postgresfriends.org, er@xs4all.nl, jacob.champion@enterprisedb.com, david.g.johnston@gmail.com, peter@eisentraut.org, li.evan.chao@gmail.com
Date: 2026-07-02T00:23:28Z
Lists: pgsql-hackers
Hi Jian, Thanks for the review, and for basing it on the RPR branch. Responses inline. > ``(only possible during early development/testing).`` comment is not necessary? Agreed, and it is actually misleading: varMatched == NULL is a live signal, not a dev-only artifact. nfa_match() is called with NULL to force every VAR to not-match at a frame boundary (execRPR.c, "Frame boundary reached: force mismatch"). I will rewrite the comment to describe that meaning instead of the "early development/testing" note. > nfa_match desperately needs an extra i(nt64 currentPos) argument. [...] > adding currentPos makes it much easier to understand the current > execution state when debugging with GDB. Makes sense -- nfa_match() has only the state list in scope, no row index, which is exactly the thing that is hard to place in GDB. I will thread currentPos through. > The comments above in nfa_match are unnecessary because in ``if > (RPRElemIsAbsorbableBranch(elem)`` we already have many comments, and > the meaning seems the same. Agreed -- the loop-top overview's second sentence restates the inline END-chain block. I will drop it and keep "Evaluate VAR elements against current row" as a one-line lead. > Similar to REG_DEBUG, we really need an RPR_DEBUG option. [...] verify > that ctx->states is not a circular linked list [...] without affecting > release builds. Fair point, and there is some history: RPR_DEBUG did exist earlier as debug logging, and I am the one who proposed removing it. My reason was narrow, though. I lean on the debugger, and with AI assistance the cost of inserting, watching, and then removing ad-hoc logs has dropped sharply -- so I treated those logs as ephemeral and optimized for a clean committed tree rather than for someone reading execRPR.c cold. For a new contributor that trade-off is the wrong one, and you have convinced me: a retained set of observation points, plus the Assert-style sanity checks you describe (non-circular state list, count and context-ordering invariants), would genuinely lower the barrier to this code. So I agree with the direction. Rather than settle on a shape now, I would leave it open: which observation points and checks are actually worth retaining behind #ifdef RPR_DEBUG can be picked out later as a separate follow-up, and it is really Tatsuo's call since he suggested the first version. > <para> The SQL standard defines more subclauses: MEASURES and SUBSET > [...] more variations in AFTER MATCH clause. </para> > This part should stay in the Compatibility section. Agreed. It is a standard-deviation note, and the SELECT Compatibility section currently has no row-pattern text, so I will move this paragraph there. Unless Tatsuo has a different view by the weekend, I will proceed with the concrete changes above, leaving the RPR_DEBUG shape open as noted. Best regards, Henson
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Adjust cross-version upgrade tests for seg_out() fix
- 3e3d7875e956 19 (unreleased) cited
-
Rationalize error comments in partition split/merge tests
- ecb2508aaf9b 19 (unreleased) cited
-
Add fast path for foreign key constraint checks
- 2da86c1ef9b5 19 (unreleased) cited
-
Fix assorted pretty-trivial memory leaks in the backend.
- e78d1d6d47dc 19 (unreleased) cited
-
Add temporal FOREIGN KEY contraints
- 89f908a6d0ac 18.0 cited
-
Add trailing commas to enum definitions
- 611806cd726f 17.0 cited
-
Remove obsolete executor cleanup code
- d060e921ea5a 17.0 cited