Further _bt_first simplifications for parallel index scans

On Thu, 2 Jan 2025 at 17:38, Peter Geoghegan <pg@bowt.ie> wrote:

Attached patch goes a bit further with simplifying _bt_first's
handling of seizing the parallel scan. This continues recent work from
commits 4e6e375b and b5ee4e52.

Aside from requiring less code, the new structure relieves _bt_first
from having separate calls to _bt_start_array_keys for the serial and
parallel cases. It also places emphasis on the idea that it's expected
that calls to _bt_first will go through _bt_readfirstpage (not
_bt_readnextpage). While it is of course possible for a parallel
scan's _bt_first to call _bt_readnextpage instead, that is the
exception, not the rule.

Apart from comments on comment contents and placement, no specific issues:

+     *
+     * Initialize arrays during first (unscheduled) primitive index scan.
+     */

I think this could be more clear about why these conditions indicate
the first unscheduled primitive index scan.

/*
* Count an indexscan for stats, now that we know that we'll call
* _bt_search/_bt_endpoint below
+     *
+     * _bt_readfirstpage will release the seized parallel scan, if any.
*/

I don't understand the placement of that comment, as it's quite far
away from any parallel scan related code and it's very unrelated to
the index scan statistics.
If this needs to be added, I think I'd put it next to the call to
_bt_parallel_seize().

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

On Tue, Jan 7, 2025 at 6:56 AM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:

Apart from comments on comment contents and placement, no specific issues:

Pushed this just now. Thanks for the review!

+     *
+     * Initialize arrays during first (unscheduled) primitive index scan.
+     */

I think this could be more clear about why these conditions indicate
the first unscheduled primitive index scan.

Improved this in the committed patch.

I don't understand the placement of that comment, as it's quite far
away from any parallel scan related code and it's very unrelated to
the index scan statistics.
If this needs to be added, I think I'd put it next to the call to
_bt_parallel_seize().

Done that way in the committed patch.

--
Peter Geoghegan