Hi Peter,
Thanks for v36 and for pushing the slot interface. The table/index AM
header split addresses my include concern.
1. On inlining, you wrote in the v35 mail:
> This approach
> seems to be more elegant and at least as efficient as the approach
> taken in v34.
I prefer your approach to the version I tried. Inlining
tableam_index_fill_ios_slot() while keeping
tableam_index_fill_ios_names() as a separate cold function also removes
the duplicate slot-filling code in heapam_index_getnext_slot(). My
experiment kept the name conversion loop inside the inline function, so
my earlier "inlining buys nothing" conclusion shouldn't be generalized
to your version.
I don't see a repeatable slowdown from the committed slot interface
on the two index-only scans from my last mail.
I compared the same base with and without v35-0001 as committed (-O2, no
assertions), swapping binaries on one data directory and restarting each
run with huge_pages=off and shared_buffers=256MB. Same pgbench -s 5
data, queries and client binary; one client, prepared protocol, Heap
Fetches 0; six alternating rounds, 15 seconds per query. The base is
newer than in my v34 tests, so absolute TPS figures aren't directly
comparable:
median tps without v35-0001 with v35-0001
point 49801 50404
10k-row range 1391 1392
min-max: point 48578-50833 / 49424-50657;
range 1347-1418 / 1352-1403.
2. On predicate locking, you wrote in the same mail:
> We have to call PredicateLockPage immediately after the corresponding
> VM_ALL_VISIBLE call -- it's not okay to delay it until the scan
> actually consumes an item.
I'd keep PredicateLockPage() immediately after the VM check. The extra
locks can cause a serialization failure even when the cursor never
reaches the modified row, but the alternatives I tried were worse.
I used the table and SERIALIZABLE session settings from
index-only-scan.spec, but closed the cursor after MOVE FORWARD 10 and
never fetched row 11:
s1: DECLARE c CURSOR FOR SELECT id FROM ios ORDER BY id;
s1: MOVE FORWARD 10 FROM c;
s1: CLOSE c;
s2: SELECT id FROM ios WHERE id = 1;
s2: DELETE FROM ios WHERE id = 11;
s2: COMMIT;
s1: DELETE FROM ios WHERE id = 1;
s1: COMMIT;
With only the committed slot interface, both transactions commit.
With v36, s1's DELETE raises a serialization failure. Just after the
MOVE FORWARD 10 FROM c, pg_locks shows 10 heap-page SIREAD locks with
only the slot interface versus 15 with v36. Disabling prefetch with
debug_disable_indexscan_prefetch=on leaves that v36 result unchanged.
The three existing index-only-scan.spec permutations also pass on v36.
I tried two alternatives on top of v36 to avoid taking SIREAD locks
ahead of the cursor.
First, I cached VM results without taking predicate locks in
heapam_index_batch_pos_visibility(). For SSI, I rechecked those
all-visible items in heapam_index_getnext_slot()'s index-only branch,
before the !all_visible check: take the predicate lock if still
all-visible, otherwise fetch the heap.
That let both transactions commit in the closed-cursor test. I then
ran a separate SERIALIZABLE test on a fresh copy of the table,
keeping the cursor open. I used GDB to pause s2's DELETE in
heap_delete(), just after CheckForSerializableConflictIn() returned
and before the VM bit was cleared:
s1: DECLARE c CURSOR FOR SELECT id FROM ios_gap ORDER BY id;
s1: MOVE FORWARD 10 FROM c;
s2: SELECT id FROM ios_gap WHERE id = 1;
s2: DELETE FROM ios_gap WHERE id = 11; -- paused inside heap_delete()
s1: FETCH 1 FROM c; -- returns row 11
s1: DELETE FROM ios_gap WHERE id = 1;
s1: COMMIT;
-- Resume s2 to finish its DELETE.
s2: COMMIT;
The VM recheck still saw all-visible, and my modified version let both
transactions commit. Unmodified v36 aborted s2 in this same schedule,
so I dropped that alternative.
The second forced curvmheapallvis to false in
heapam_index_batch_pos_visibility() when SSI conflict checking was
needed. This made heapam_index_getnext_slot() fetch the heap for
consumed items while keeping batching and prefetching. It avoided the
closed-cursor abort and caught the concurrent write, but gave up the
index-only benefit.
On the 10k-row range query, with explicit SERIALIZABLE transactions,
median TPS fell from 1403 with v36 to 568, and Heap Fetches went from
0 to 10000. This comparison used the same v36 base: -O2 without
assertions, one data directory with binary swaps, huge_pages=off,
shared_buffers=256MB, pgbench -s 5, one client, prepared protocol,
three alternating 10-second rounds per build.
To handle writes between caching VM results and consuming the batch,
your approach is the best of those I tried. READ COMMITTED and
REPEATABLE READ don't acquire these SIREAD locks, so production
workloads using either level won't see these extra serialization
failures.
3. My cursor and join checks gave the same results on v36 and on the
build with only the committed slot interface.
On 180,000 rows, I compared forward and backward cursor fetches,
including reaching the end and reversing direction, for ordinary and
index-only scans before and after updates and deletes. All 656 FETCH
results matched. Merge joins with repeated outer keys and an inner
index-only scan without Materialize also matched, as did repeated
nested-loop rescans.
4. Two small wording points in 0006:
The commit message still describes enable_indexscan_prefetch, default
on; the code now has debug_disable_indexscan_prefetch, default off.
In heapam_index_heap_fetch(), the comment above the
INDEX_PREFETCH_BLKSWITCH_THRESHOLD check says "distinct blocks".
I also checked a 14-row table occupying just two heap pages, A and B,
with the index scan visiting A -> B -> A -> B and so on. LIMIT 3 did
not create a read stream; LIMIT 4 did. The comment should say
"block visit count", not "distinct block count".
Regards,
Rui
