Re: Row pattern recognition

Hi Henson,

> Hi Tatsuo,
>
> Thank you for the feedback.
>
> Note that applying 0001-0006 produced compiler warning.
>>
>> execExprInterp.c: In function ‘ExecEvalRPRNavSet’:
>> execExprInterp.c:6005:23: warning: ‘target_pos’ may be used uninitialized
>> [-Wmaybe-uninitialized]
>> 6005 | target_slot = ExecRPRNavGetSlot(winstate, target_pos);
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> execExprInterp.c:5959:25: note: ‘target_pos’ was declared here
>> 5959 | int64 target_pos;
>> | ^~~~~~~~~~
>>
>
> `ExecRPRNavGetSlot()`, along with the enclosing `ExecEvalRPRNavSet()`
> and its `int64 target_pos` declaration, is introduced by nocfbot-0017
> (Implement 1-slot PREV/NEXT navigation for RPR), not at the 0001-0006
> level. Could you double-check the actual applied range on your side?
> The function simply does not exist at 0001-0006.
>
> Since the function is not present at 0001-0006, I suspect the
> applied patch set on your side is in a different state than
> expected. Could you verify the range and contents of the patches
> actually applied?

> But regression test was ok.
>> However, after applying 0007, regression test failed.
>>
>> not ok 196 + rpr_integration 514 ms
>> # (test process exited with exit code 2)
>>
>
> nocfbot-0006 was written specifically to prevent the crashes that
> 0007's tests expose, so with 0001-0007 applied in order the issue
> should already be resolved. On my authoritative base (cfbot/4460) all
> rpr_integration tests pass through 0008. I had been routinely
> building only on macOS ARM64 recently, so I just re-verified on
> Rocky 9 and Ubuntu 24 AMD64 as well; no build warning and no
> rpr_integration failure on either.

You are right. I checked and found the 0006 patch I applied was wrong
one.

nocfbot-0006-Implement-1-slot-PREV-NEXT-navigation-for-RPR.txt:

This should have been:
nocfbot-0006-Fix-DEFINE-expression-handling-RPR-window-planning.txt

Sorry for confusion.

> For reference, here is my working tree layout. Titles only for my
> post-base commits (hashes on my side won't match yours); base
> hashes are public and directly comparable on your side:
>
> [post-base, newest first]
> 31. Remove unused include and fix header ordering in RPR files
> (origin/RPR)
> 30. Add inline comments for complex RPR algorithms and design notes
> 29. Fix nav_slot pass-by-ref dangling pointer in RPR navigation
> 28. Fix RPR documentation: synopsis, grammar, and terminology
> 27. Fix comment typos, grammar, and inaccuracies in RPR code
> 26. Fix RPR error message style: hint format, terminology, capitalization
> 25. Guard against int64 overflow in RPR bounded frame end computation
> 24. Implement FIRST/LAST and compound navigation for RPR
> 23. Add Nav Mark Lookback to EXPLAIN and fix compute_nav_max_offset()
> 22. Add 2-arg PREV/NEXT test for row pattern navigation with host
> variable
> 21. Enable JIT compilation for PREV/NEXT navigation tests in RPR
> 20. Update RPR code comments to reflect 1-slot navigation model
> 19. Add tuplestore trim optimization for RPR PREV navigation
> 18. Add JIT compilation support for RPR PREV/NEXT navigation
> 17. Implement 1-slot PREV/NEXT navigation for RPR
> 16. Normalize RPR element flag macros to return bool
> 15. Narrow variable scope in ExecInitWindowAgg DEFINE clause loop
> 14. Add CHECK_FOR_INTERRUPTS to RPR context cleanup and finalize loops
> 13. Remove unused force_colno parameter from RPR deparse functions
> 12. Fix execRPR.o ordering in executor Makefile to match meson.build
> 11. Fix quote_identifier() for RPR pattern variable name deparse
> 10. Rename rpr_explain test views from sequential numbers to descriptive
> names
> 9. Add fixed-length group absorption for RPR
> 8. Replace reduced frame map with single match result
> 7. Add RPR planner integration tests
> 6. Fix DEFINE expression handling in RPR window planning
> 5. Fix mark handling for last_value() under RPR
> 4. Fix in-place modification of defineClause TargetEntry in setrefs.c
> 3. Add CHECK_FOR_INTERRUPTS() to nfa_try_absorb_context() loop
> 2. Add CHECK_FOR_INTERRUPTS() to nfa_add_state_unique() for state
> explosion patterns
> 1. Remove unused regex/regex.h include from nodeWindowAgg.c
>
> [base, public hashes]
> 732acf9b7c6 (cfbot/cf/4460) [CF 4460] v46 - Implement row pattern
> recognition feature
> 6eb8841afaf Row pattern recognition patch (typedefs.list).
> 5213dbb3c4f Row pattern recognition patch (tests: expected).
> bdc3a17982b Row pattern recognition patch (tests: SQL, data).
> 596bcc434b5 Row pattern recognition patch (docs).
> e02872fd59c Row pattern recognition patch (executor and commands).
> 742c6eb673e Row pattern recognition patch (planner).
> 8514ca64f2d Row pattern recognition patch (rewriter).
> d26741d4f1d Row pattern recognition patch (parse/analysis).
> f85d2ee8f4a Row pattern recognition patch for raw parser.
> e484b0eea61 (origin/RPR-base, RPR-base) Fix two issues in fast-path FK
> check introduced by commit 2da86c1ef9
>
> Could you send me the equivalent `git log` for your working tree
> covering the full v46 application and the commit immediately
> preceding it?

Here it is:

t-ishii$ git log --oneline -10|cat
6dd0765463a Row pattern recognition patch (typedefs.list).
009b38f4d2b Row pattern recognition patch (tests: expected).
07864e40e36 Row pattern recognition patch (tests: SQL, data).
55b467849eb Row pattern recognition patch (docs).
28a1ff7e744 Row pattern recognition patch (executor and commands).
728b1fdf106 Row pattern recognition patch (planner).
c388e3e7eb0 Row pattern recognition patch (rewriter).
232725a3e95 Row pattern recognition patch (parse/analysis).
c71940f9e21 Row pattern recognition patch for raw parser.
322bab79744 Move declarations related to locktags from lock.h to new locktag.h

> In particular, could you also check whether the
> recent master commit titled "Fix integer overflow in
> nodeWindowAgg.c" (backpatched to 14) is included in your base? It
> touches the same ROWS/GROUPS frame boundary code path that RPR
> interacts with, so its presence or absence will help narrow things
> down. Once I can match the same base locally, I will reproduce and
> include the fix in the next revision.

No, the commit is not in my working tree.

So I guess I should have rebased v46 tree so that the commit is in the
work tree before applying your patches. Am I correct?

> A quick status update:
>
> - nocfbot-0006: self-review complete, not yet sent (comment tweak per
> review)
> - nocfbot-0007: under self-review (comment details per review)
> - nocfbot-0008: in progress
>
> I would like to resolve the crash first before sending the next
> revision, so that the full set goes out together once our bases are
> aligned and A3 passes on your side.

Let me check the crash first. Since apparently the crash was caused by
my mis operation.

> One process question: so far I have been folding review feedback
> into the relevant existing patches (i.e. amending nocfbot-0008 to
> reflect your comments on it).

I prefer this way.

> Would you prefer that I instead
> append the review fixes as new patches at the end of the series
> (e.g. nocfbot-0032, 0033, ...), so the history of reviewed changes
> stays visible? Happy to follow whichever workflow you find easier
> to review.

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