Re: Row pattern recognition

Hi Jian,

> Hi.
>
> Please check the attached refactoring for nodeWindowAgg.c and execRPR.c.
> It's based on https://github.com/assam258-5892/postgres/commits/RPR.

Thank you for the patch. Unfortunately the patch does not apply to v50
+ current master branch (I tried against v50 + the master branch at
the point also fails). So just a quick comment.

> What is being changes:
> 1. ExecRPRFreeContext: we should also reset other field values.
> 2. nfa_update_absorption_flags() now takes the WindowAggState and walks
> the context list itself. This is more intuitive, I think.
> I did the similar thing for nfa_absorb_contexts.
> 3. Remove some duplicated Asserts; preceding callers have already done
> the equivalent Asserts.
> 4. Some local variable, used only once, should be just removed.
> 5. Add some elog(ERROR) to avoid circular winstate->nfaContext,
> RPRNFAContext->states.
> 6. Only ExecRPRProcessRow uses the variable frameOffset, refactor to
> make it as a local variable.
> 7. ExecRPRStartContext->nfa_context_make. nfa_context_make() used to return a
> bare struct, leaving the initial state, matchStartRow, and active-list linkage
> for the caller to fill in, that is not intuitive, it looks like a half-built
> context. Have nfa_context_make() return a fully-formed one: allocate the
> initial state, set the start row, and append it to the list tail.
> 8. ExecRPRGetHeadContext(pos) external function can be removed.
> 9. Refactoring update_reduced_frame. Now update_reduced_frame code
> flow is more intuitive: look up or
> create the context for pos, drive the NFA forward with
> advance_reduced_frame_nfa(), then record the match result.
> 10. Restructure get_reduced_frame_status as a single decision tree: first
> "no record" (start < 0), then "the record's own row" (pos == start),
> where length alone gives the verdict (-1 unmatched, 0 empty match,
> >= 1 frame head), then the range test for everything else.
>
> Summary: The code flow in update_reduced_frame, advance_reduced_frame_nfa, and
> get_reduced_frame_status is now much more intuitive. Using elog(ERROR) prevents
> us from getting stuck in an infinite loop if something goes wrong.

For me, it's not clear why you add elog(ERROR), rather than Assert. If
the infinite loop should not happen when our program is correct, it
would be better to fix the problem and use Assert.

Also I noticed in nfa_update_absorption_flags() two local variables
hasAbsorbable and allAbsorbable are moved from very begging of the
function body. Why?

+nfa_update_absorption_flags(WindowAggState *winstate)
{
- RPRNFAState *state;
- bool hasAbsorbable = false;
- bool allAbsorbable = true;
-
- /*
- * Optimization: Once hasAbsorbableState becomes false, it stays false. No
- * need to recalculate - both flags remain false permanently.
- */
- if (!ctx->hasAbsorbableState)
- {
- ctx->allStatesAbsorbable = false;
+ if (!winstate->rpPattern->isAbsorbable)
return;
- }

- /* No states means no absorbable states */
- if (ctx->states == NULL)
+ for (RPRNFAContext *ctx = winstate->nfaContext; ctx != NULL; ctx = ctx->next)
{
- ctx->hasAbsorbableState = false;
- ctx->allStatesAbsorbable = false;
- return;
- }
+ bool hasAbsorbable = false;
+ bool allAbsorbable = true;

Regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp