Re: Row pattern recognition

Hi jian,

Thanks for the refactor. I went through it change by change -- the
per-file notes are at the bottom.

A note up front, so the notes don't read as dismissive: at this stage I'd
rather prioritize validated code over better code. The branch has a lot of
coverage and Valgrind behind it, and every change -- even a clean,
equivalent one -- moves a line out from under that. So where I say "drop",
it means "not now", not "not good"; most of these would be welcome as a
follow-up. One piece, though, does need a fix before any of it lands.

That piece is the new guard in computeAbsorbability(): it carries a
behavior change that no test covers.

> Move isAbsorbable initialization from finalizeRPRPattern() into
> makeRPRPattern(), and guard the computeAbsorbabilityRecursive() call in
> computeAbsorbability() so it is only entered when the first element is
> an ALT or has an unbounded quantifier.

First, what "absorbable" means here: the first unbounded repetition of a
fixed-length body reachable from the start of the pattern. This rule is
worth writing down -- in README.rpr and in the comment on
computeAbsorbability() (or wherever fits best) -- so it is stated once,
explicitly (patch attached: nocfbot-absorb-definition-doc).

By that definition the leading A+ in (A+ B){2,3} is absorbable, but the
patch drops it.

The offending check is in computeAbsorbability():

if (RPRElemIsAlt(&pattern->elements[0]) ||
pattern->elements[0].max == RPR_QUANTITY_INF)
computeAbsorbabilityRecursive(pattern, 0, &hasAbsorbable);

It looks only at elements[0], so a bounded outer group's BEGIN (finite max)
fails the check and the recursion that would descend to A+ never runs.

I'm attaching a small test for this (nocfbot-absorb-bounded-outer-test): it
passes on the branch and fails with v50-0001 applied, pinning both the a+"
marker and the absorbed-context count.

For the rest, change by change -- "ok" = take as-is, "drop" = leave out:

src/backend/optimizer/plan/rpr.c

The branch is already validated under coverage and Valgrind, so I'd rather
not re-open verified logic for equivalent refactors -- glad to take those
as a follow-up. In file order:

ok palloc_array / palloc0_array in makeRPRPattern()
result->varNames = palloc_array(char *, numVars);
result->elements = palloc0_array(RPRPatternElement, numElements);

ok move the isAbsorbable = false init into makeRPRPattern()

drop rename beginElem -> elem in fillRPRPatternGroup()
only beginElem was renamed; with endElem kept, the begin/end pair is
no longer symmetric (elem / endElem)

drop rename isFixedLengthChildren -> isFixedQuantifier -- keep the old
name
"fixed-length" is the term everywhere else (README.rpr, the nearby
comments, even this function's own body), and it drops the
"children"
that flags a whole-subtree check

drop the jump-1 traversal rewrite in that function -- equivalent
refactor,
defer to a follow-up

drop rename to start_with_unbounded_quantifier -- keep isUnboundedStart
Lone snake_case among camelCase neighbors -- including
isFixedQuantifier,
renamed in this same patch -- and it reads as a predicate though it
sets flags.

drop start_with_*'s new signature + if/else restructure -- equivalent
refactor; keep the original isUnboundedStart body

drop rename branchFirst -> branchElement -- cosmetic churn, no behavior
change

drop the new computeAbsorbability() guard -- the regression above
if (RPRElemIsAlt(&pattern->elements[0]) ||
pattern->elements[0].max == RPR_QUANTITY_INF)

src/include/nodes/plannodes.h

ok drop the redundant /* T_RPRPattern */ on the type field
NodeTag type;

drop reworded "see isUnboundedStart" block comment -- keep the original
(it still matches now that isUnboundedStart stays)

ok drop the inline isAbsorbable comment -- the block comment above
covers it
bool isAbsorbable;

src/backend/executor/README.rpr

ok spell out the skip mode -- a clear doc fix on its own
(1) SKIP PAST LAST ROW (not SKIP TO NEXT ROW)

drop isUnboundedStart -> start_with_unbounded_quantifier -- follows the
rename

Tatsuo, what's your read on all this -- in particular, hold the equivalent
refactors until after commit, or take them now?

Best regards,
Henson