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-12T20:28:34Z
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

On 12.03.2025 01:59, Peter Geoghegan wrote:
> On Tue, Mar 11, 2025 at 6:24 PM Alena Rybakina
> <a.rybakina@postgrespro.ru> wrote:
>> Hi, reviewing the code I noticed that you removed the
>> parallel_aware check for DSM initialization for BitmapIndexScan,
>> IndexScan, IndexOnlyScan,
>> but you didn't do the same in the ExecParallelReInitializeDSM function
>> and I can't figure out why to be honest. I think it might be wrong or
>> I'm missing something.
> I didn't exactly remove the check -- not completely. You could say
> that I *moved* the check, from the caller (i.e. from functions in
> execParallel.c such as ExecParallelInitializeDSM) to the callee (i.e.
> into individual executor node functions such as
> ExecIndexScanInitializeDSM). I did it that way because it's more
> flexible.
>
> We need this flexibility because we need to allocate DSM for
> instrumentation state when EXPLAIN ANALYZE runs a parallel query with
> an index scan node -- even when the scan node runs inside a parallel
> worker, but is non-parallel-aware (parallel oblivious). Obviously, we
> might also need to allocate space for a shared index scan descriptor
> (including index AM opaque state), in the same way as before
> (or we might need to do both).
I see and agree with this changes now.
>> As I see, it might be necessary if the parallel executor needs to
>> reinitialize the shared memory state before launching a fresh batches of
>> workers (it is based on
>> the comment of the ExecParallelReinitialize function), and when it
>> happens all child nodes reset their state (see the comment next to the
>> call to the ExecParallelReInitializeDSM
>> function).
> I did not move/remove the parallel_aware check in
> ExecParallelReInitializeDSM because it doesn't have the same
> requirements -- we *don't* need that flexibility there, because it
> isn't necessary (or correct) to reinitialize anything when the only
> thing that's in DSM is instrumentation state. (Though I did add an
> assertion about parallel_aware-ness to functions like
> ExecIndexScanReInitializeDSM, which I thought might make this a little
> clearer to people reading files like nodeIndexScan.c, and wondering
> why ExecIndexScanReInitializeDSM doesn't specifically test
> parallel_aware.)
>
> Obviously, these node types don't have their state reset (quoting
> ExecParallelReInitializeDSM switch statement here):
>
>          case T_BitmapIndexScanState:
>          case T_HashState:
>          case T_SortState:
>          case T_IncrementalSortState:
>          case T_MemoizeState:
>              /* these nodes have DSM state, but no reinitialization is
> required */
>              break;
>
> I added T_BitmapIndexScanState to the top of this list -- the rest are
> from before today's commit. I did this since (like the other nodes
> shown) BitmapIndexScan's use of DSM is limited to instrumentation
> state -- which we never want to reset (there is no such thing as a
> parallel bitmap index scan, though bitmap index scans can run in
> parallel workers, and still need the instrumentation to work with
> EXPLAIN ANALYZE).
>
> We still need to call functions like ExecIndexOnlyScanReInitializeDSM
> from here/ExecParallelReInitializeDSM, of course, but that won't reset
> the new instrumentation state (because my patch didn't touch it at
> all, except for adding that assertion I already mentioned in passing).
>
> We actually specifically rely on *not* resetting the shared memory
> state to get correct behavior in cases like this one:
>
> https://www.postgresql.org/message-id/CAAKRu_YjBPfGp85ehY1t9NN%3DR9pB9k%3D6rztaeVkAm-OeTqUK4g%40mail.gmail.com
>
> See comments about this in places like ExecEndBitmapIndexScan (added
> by today's commit), or in ExecEndBitmapHeapScan (added by the similar
> bitmap heap instrumentation patch discussed on that other thread,
> which became commit 5a1e6df3).
>
>
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.
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.
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.

-- 
Regards,
Alena Rybakina
Postgres Professional