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