Re: Row pattern recognition

Hi Jian, Tatsuo,

Jian, two questions of yours from earlier in the thread never got an
answer from me. Late, but here are both.

> Except that i want to rename RPSkipTo to RPRSkipTo and rpSkipTo to
> rprSkipTo, What do you think?

Rather than a yes or no on this one name, I would like to treat it as
a policy call over the whole list.

The split we have is historical. RP -- RPCommonSyntax, RPSkipTo --
predates my joining this patch. RPR came later, from the pattern
compiler and the NFA I wrote. Neither author was aware of the other's
prefix; each of us just used his own. Now the two meet in one place,
since RPCommonSyntax holds an RPRPatternNode *rpPattern.

The full list as it stands:

RP* (parse / plan / exec nodes)
RPCommonSyntax, RPSkipTo
rpCommonSyntax, rpSkipTo, rpPattern, rpDefs (parsenodes.h)
rpSkipTo, rpPattern (plannodes.h, execnodes.h)

RPR* (pattern compiler, NFA, navigation)
RPRPatternNode, RPRPatternNodeType, RPRPattern,
RPRPatternElement, RPRElemFlags, RPRElemIdx, RPRVarId,
RPRQuantity, RPRDepth, RPRNFAState, RPRNFAContext,
RPRNavExpr, RPRNavKind
RPR_ELEM_*, RPR_VARID_*, RPR_NAV_*, RPR_QUANTITY_INF,
RPR_VARID_MAX

typedefs.list carries 15 of these -- 2 RP, 13 RPR.

So what we would be choosing between is three policies:

(a) RPR* throughout -- one prefix for the feature; the RP names
(7 identifiers, plus 2 typedefs.list entries) move over.

(b) RP* throughout -- the other direction. It moves far more (13
types, plus the whole RPR_ELEM_* / RPR_VARID_* / RPR_NAV_*
macro families) and cuts against everything already settled on
RPR: the file names (rpr.c, rpr.h, execRPR.c, README.rpr), the
tests (rpr.sql, rpr_explain.sql), and the acronym of the
feature itself (row pattern recognition).

(c) RP for the row pattern syntax as the standard names it (row
pattern common syntax, AFTER MATCH SKIP TO), RPR for the
machinery that recognizes it -- a deliberate boundary, written
down so it reads as intent rather than as an accident.

I have no strong preference between (a) and (c). (b) I list for
completeness -- with everything around it already RPR, it looks like
the most expensive of the three. Either way, what I am against is
settling this one name at a time.

On timing, too: I would rather we decided it after the correctness
defects are closed. A rename touches the parse, plan and executor
nodes at once, so it collides with every patch in flight and with the
defect work under way -- and it buys a reader nothing today. Once the
defects are settled it is a single mechanical patch, and a much easier
review.

Tatsuo, since RP is yours and RPR is mine, this is really a call for
you to make. Either way suits me.

Jian, while I am here: v50-0003 (EMPTY_LOOP -> RPR_ELEM_EMPTY_LOOP) is
taken.

> I am wondering if we can have one query to enable (COSTS ON) for
> testing.
>
> For many rpr tests, the table size is small, EXPLAIN ANALYZE won't
> take long, and probably all rows will be sampled.
> Of course, once the patch is in committable shape, we need to
> switch it to OFF.

It can be done. I would still rather we did not. Costs can come out
differently from one OS to another -- I have had the same query cost
differently on Linux and on macOS. That means keeping several
expected outputs and maintaining them, and it makes the tests harder
to get through cfbot. In all honesty I cannot even set up a build on
Windows or BSD, so for those I rely on cfbot alone.

The need behind it is fair, though: we should be able to see that the
model charges what we think it charges. That belongs outside the
committed tests -- an ad hoc EXPLAIN (COSTS ON) run whenever we want
to check it, rather than expected output we then have to keep. We see
the same thing either way.

Best regards,
Henson