Horton

Re: Adding skip scan (including MDAM style range skip scan) to nbtree

Alena Rybakina <a.rybakina@postgrespro.ru>

View thread
From: Alena Rybakina <a.rybakina@postgrespro.ru>
To: Peter Geoghegan <pg@bowt.ie>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>, Masahiro Ikeda <ikedamsh@oss.nttdata.com>, Tomas Vondra <tomas@vondra.me>, Masahiro.Ikeda@nttdata.com, pgsql-hackers@lists.postgresql.org, Masao.Fujii@nttdata.com
Date: 2025-03-18T10:54:48Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. nbtree: Always set skipScan flag on rescan.

  2. meson: Build numeric.c with -ftree-vectorize.

  3. Fix "variable not found in subplan target lists" in semijoin de-duplication.

  4. Revert "nbtree: Remove useless row compare arg."

  5. nbtree: Remove useless row compare arg.

  6. Prevent premature nbtree array advancement.

  7. nbtree: tighten up array recheck rules.

  8. Avoid treating nonrequired nbtree keys as required.

  9. Adjust overstrong nbtree skip array assertion.

  10. Make NULL tuple values always advance skip arrays.

  11. Avoid extra index searches through preprocessing.

  12. Improve nbtree skip scan primitive scan scheduling.

  13. Further optimize nbtree search scan key comparisons.

  14. Add nbtree skip scan optimization.

  15. Improve nbtree array primitive scan scheduling.

  16. nbtree: Make BTMaxItemSize into object-like macro.

  17. Show index search count in EXPLAIN ANALYZE, take 2.

  18. Make parallel nbtree index scans use an LWLock.

  19. Show index search count in EXPLAIN ANALYZE.

  20. Avoid nbtree parallel scan currPos confusion.

  21. nbtree: Remove useless 'strat' local variable.

  22. Normalize nbtree truncated high key array behavior.

  23. Refactor handling of nbtree array redundancies.

  24. Fix nbtree pgstats accounting with parallel scans.

  25. Avoid parallel nbtree index scan hangs with SAOPs.

  26. Show Parallel Bitmap Heap Scan worker stats in EXPLAIN ANALYZE

  27. Enhance nbtree ScalarArrayOp execution.

  28. Skip checking of scan keys required for directional scan in B-tree

  29. Instead of using a numberOfRequiredKeys count to distinguish required

Attachments

On 12.03.2025 23:50, Peter Geoghegan wrote:
> On Wed, Mar 12, 2025 at 4:28 PM Alena Rybakina
> <a.rybakina@postgrespro.ru>  wrote:
>> Thank you for the explanation!
>>
>> Now I see why these changes were made.
>>
>> After your additional explanations, everything really became clear and I
>> fully agree with the current code regarding this part.
> Cool.
>
>> However I did not see an explanation to the commit regarding this place,
>> as well as a comment next to the assert and the parallel_aware check and
>> why BitmapIndexScanState was added in the ExecParallelReInitializeDSM.
> I added BitmapIndexScanState to the switch statement in
> ExecParallelReInitializeDSM because it is in the category of
> planstates that never need their shared memory reinitialized -- that's
> just how we represent such a plan state there.
>
> I think that this is supposed to serve as a kind of documentation,
> since it doesn't really affect how things behave. That is, it wouldn't
> actually affect anything if I had forgotten to add
> BitmapIndexScanState to the ExecParallelReInitializeDSM switch
> statement "case" that represents that it is in this "plan state
> category": the switch ends with catch-all "default: break;".
Agree.
>> In my opinion, there is not enough additional explanation about this in
>> the form of comments, although I think that it has already been
>> explained here enough for someone who will look at this code.
> What can be done to improve the situation? For example, would adding a
> comment next to the new assertions recently added to
> ExecIndexScanReInitializeDSM and ExecIndexOnlyScanReInitializeDSM be
> an improvement? And if so, what would the comment say?
>
After reviewing the logic again, I realized that I was confused 
precisely in the reinitialization of memory for IndexScanState and 
IndexOnlyScanState.

As far as I can see, either assert is not needed here, the functions 
ExecIndexScanReInitializeDSM and ExecIndexScanReInitializeDSM can be 
called only if parallel_aware is positive, or it makes sense that 
reinitialization is needed only if parallel_aware is positive, then the 
condition noted above is not needed. According to your letter (0), the 
check should be removed there too, but I got confused in the comment. We 
do not need to reinitialize memory because DSM is instrumentation state 
only, but it turns out that we are reinitializing the memory, so we 
don't do it at all?

I attached a diff file to the letter with the comment.

[0] 
https://www.postgresql.org/message-id/CAH2-WzkMpFsE_hM9-5tecF22jVJSGtKMFMsYqMa-uo73MOxsWw%40mail.gmail.com



-- 
Regards,
Alena Rybakina
Postgres Professional