Thread

Re: Row pattern recognition

Henson Choi <assam258@gmail.com> — 2026-05-30T13:45:39Z

[mailing-list reply draft -- pgsql-hackers, RPR thread.
 In-reply-to Jian's round-3 review, 2026-05-30 06:45
 (Message-ID CACJufxEsaU8GQ4yeXTWhAO8VjbrZTh5CpvUqz=
4a3T0Cwz44pA@mail.gmail.com).
 Covers R3-1..R3-4 (comments) and the three attached refactors R3-5a/b/c.
 All accepted; fold into the v48 refactor series.]

Subject: Re: Row pattern recognition

Hi Jian,

Thanks for the round-3 pass -- all seven are good catches. Going through
them inline below, with a summary of decisions at the end.

> Comments like "Tests line 251" should not appear in SQL, since the
> line number would change.
> So, we need to rephrase this comment. Please don't delete it, it may
> clarify the patch for future readers.

Agreed on both counts -- the intent (this case exercises the
child->max == RPR_QUANTITY_INF branch in mergeConsecutiveVars, where a
finite prev meets an infinite child) is worth keeping; only the "line 251"
anchor is fragile. I'll rephrase it to name the branch and the condition
it covers instead of the line, so it survives any later shuffle.

There's a parallel one a few tests down -- "Tests line 325 ... in
mergeConsecutiveGroups" -- with the same problem, so I'll fix both
together on the same principle.

> The "at the END" confuses me. It may also refers to RPR_VARID_END /
> RPRElemIsEnd.
> How about something like:
> "Add the state to the end of the ctx->states linked list, but only if
> a duplicate state is not already present."

Your wording is clearer than mine -- I'll take it almost verbatim. "at the
END" there meant the list tail, and capitalizing it was exactly the wrong
choice next to the END element kind.

> I think a bunch of these uppercase ENDs should just be lowercase "end",
> [...]
> Could we lowercase the list-end ones and keep uppercase only when actually
> referring to the END element kind?

Yes -- that's the right rule, and I'll go through them on it: lowercase
"end" for the linked-list tail, uppercase END only when it means the
element kind (RPRElemIsEnd / RPR_VARID_END). On the two you singled out:

  - nfa_match, "Non-VAR elements (ALT, END, FIN) ..." -- element kind,
    listed with the others, stays uppercase. Agreed.

  - "advance through END chain inline ..." -- this one is actually the
    element kind too: it walks the chain of group-END *elements* up to the
    absorption judgment point, not the state list. So it stays uppercase,
    but since it tripped you up I'll reword it to "chain of END elements"
    to make that unambiguous rather than leaning on the reader to infer it.

> The comment under nfa_advance_var is confusing (for me).
> /* After a successful match, count >= 1, so at least one must be true */
> nfa_advance_var doesn't actually know anything about match status (i
> think), it can be reached with count == 0.
> [...] and it does fire, so the comment's premise isn't quite right.

You're right, and thanks for confirming it with the elog. nfa_advance_var
is part of the advance phase, which generates every reachable next state
regardless of what matched -- so it's routinely entered with count == 0,
for a VAR just reached through an ALT branch or a group that hasn't been
evaluated yet. The comment carries match-phase reasoning into a phase that
has no notion of matching. The Assert above still holds, but on the
structure of a VAR's bounds, not on any prior match, so I'll reword the
comment to say that instead.

> please check the attached minor refactoring.
> 1.  Refactor struct WindowAggState: Remove the nfaStateSize and
>     nfaVisitedNWords fields because they are constant, and we can easily
>     compute it, [...] not necessary to stay in WindowAggState.
> 2. Rename nfa_context_alloc() to nfa_context_make(), nfa_state_alloc()
>    to nfa_state_make(), nfa_state_create() to nfa_state_clone().
>    Rationale: We have makeNode [...]. In tablecmd.c, we have
>    CloneForeignKeyConstraints, which is based on existing information,
>    creating a new node, here we are doing the same in nfa_state_create.
> 3. Refactor functions: nfa_advance_alt, nfa_advance_begin,
>    nfa_advance_end, and nfa_advance_var. Because the
>    (RPRPatternElement *elem) parameter is unnecessary.

Looked at all three. 0002 and 0003 I'll take; 0001 I'd split.

  1. The two fields differ. nfaVisitedNWords -- yes: in the current tree
     it's read only once, at init, to size the visited bitmap, since the
     per-row reset clears just the high-water [min,max] range. Demoting it
     to a local is clean. (Heads up: the patch is on v47, before that
     high-water-mark change, so its nfa_advance hunk no longer applies and
     drops out on rebase.)

     nfaStateSize I'd keep. The patch recomputes it inside the allocator,
     i.e. on every state allocation -- the hottest path in the engine; a
     pathological (V1|...|Vk)* can allocate hundreds of millions of states.
     It's a loop-invariant fixed at init, so I'd rather keep it precomputed
     than rebuild offsetof + maxDepth*4 each alloc -- the same reasoning I
     gave for keeping the frame offsets out of the per-row loop.

  2. make/clone: agreed, and there's a closer precedent than I'd have
     reached for -- our own regex NFA engine already clones states
     (clonesuccessorstates / cloneouts in regcomp.c), so clone is the right
     word for duplicating an NFA state, not just the CloneForeignKey sense.
     The pair reads well too: make for the blank allocation, clone for
     building a state from an existing one's counts. (create was itself an
     earlier rename of mine from clone "for clarity," so this just lands us
     back on clone, which I now agree is better.) When I apply it I'll also
     tidy two comments the rename left behind -- the nfa_state_make header
     now reads "a new RPRNFAState state," and nfa_state_clone's body comment
     still says "Create."

  3. elem parameter: here I'd keep it, on the same grounds as nfaStateSize.
     The four functions have a single caller, nfa_advance_state, which
     already computes elem = &elements[state->elemIdx] for its own
     mark-visited check and switch, and hands it down. Dropping the
     parameter makes each function rebuild that same address on the
     per-dispatch advance path -- discarding a value the caller already
     holds. And with one trusted caller, the mismatch the change guards
     against can't really arise. The cost either way is tiny; I'd just
     rather keep the value flowing from where it's already computed than
     recompute it, which is the same call I made on nfaStateSize.

Summary of decisions, in the order above:

  "Tests line N" test comments     Rephrase -- name the branch, not the
line (both)
  "at the END" wording             Accept -- your wording, near-verbatim
  uppercase END cleanup            Accept -- lowercase the list-end ones,
keep the kind
  nfa_advance_var count comment    Reword -- wrong premise; state the
structural reason
  0001 nfaVisitedNWords            Accept -- demote to a local
  0001 nfaStateSize                Keep -- hottest path; leave it
precomputed
  0002 make / clone rename         Accept -- plus tidy two stray comments
  0003 drop the elem parameter     Keep -- single caller already holds it

So the next revision carries the four comment fixes, 0002, and the
nfaVisitedNWords half of 0001; nfaStateSize and the elem parameter I'd keep
as they are. Happy to be talked round on either if you see a cost I'm
missing.

Thanks again,
Henson