Hi, Following up on the earlier discussion:
https://www.postgresql.org/message-id/xzflwwjtwxin3dxziyblrnygy3gfygo5dsuw6ltcoha73ecmnf%40nh6nonzta7kw Andres pointed out that the TupIsNull(slot) check in ExecScanExtended() is redundant with the result of table_scan_getnextslot(), but that the compiler cannot infer this relationship. Amit proposed adding a pg_assume(), and David confirmed through objdump that GCC could then remove the redundant check. I noticed that this change is still not present on current master, so I prepared the attached patch. The patch adds the following one-way invariant: found = sscan->rs_rd->rd_tableam->scan_getnextslot(sscan, direction, slot); pg_assume(!found || !TupIsNull(slot)); return found; I used the implication form mentioned by Andres instead of: found == !TupIsNull(slot) The executor only requires that a successful call leave the slot non-empty. It does not need to require the slot to be empty whenever the table AM returns false. The only in-tree scan_getnextslot implementation is heap_getnextslot(). It clears the slot before returning false and calls ExecStoreBufferHeapTuple() before returning true. The latter clears TTS_FLAG_EMPTY. I also updated the TableAmRoutine callback comment to document the successful-call contract. With GCC 12.2.1 on AArch64 at -O2, objdump shows that the TTS_FLAG_EMPTY load and branch following the table AM callback are removed. I observed the same optimization with GCC 16. Apple Clang 21 did not eliminate the check in this code shape. I also ran an in-memory microbenchmark using a single-client count(*) SeqScan over a 20 million-row, 692 MB heap table, with parallel query and JIT disabled. Across four alternating baseline/patched pairs of 100 scans each, the paired median improvement was about 0.53%, but the result remained within measurement noise. This is consistent with Amit's earlier observation, so I do not claim a measurable runtime improvement; the demonstrated benefit is the simpler generated code. Both regular and assertion-enabled GCC builds passed: - core regression tests: 243/243 - test_extensible: 1/1 Does this look worthwhile as a small Table AM/executor code-generation cleanup? Regards, Xiaoyu
From 3aa4cb9ea6e2e84d1f075f0fdd6d0db8765feb9b Mon Sep 17 00:00:00 2001 From: xiaoyu liu <[email protected]> Date: Mon, 7 Sep 2026 16:49:19 +0800 Subject: [PATCH] tableam: Assume successful scans return nonempty slots Record the one-way scan_getnextslot contract with pg_assume so the compiler can remove a redundant slot emptiness check from executor scan paths. --- src/include/access/tableam.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index ff03a2b816f..b4cac9b177c 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -378,7 +378,8 @@ typedef struct TableAmRoutine bool allow_sync, bool allow_pagemode); /* - * Return next tuple from `scan`, store in slot. + * Return true if the next tuple from `scan` was stored in slot. A + * successful call must leave slot non-empty. */ bool (*scan_getnextslot) (TableScanDesc scan, ScanDirection direction, @@ -1095,13 +1096,18 @@ table_rescan_set_params(TableScanDesc scan, ScanKeyData *key, static inline bool table_scan_getnextslot(TableScanDesc sscan, ScanDirection direction, TupleTableSlot *slot) { + bool found; + slot->tts_tableOid = RelationGetRelid(sscan->rs_rd); /* We don't expect actual scans using NoMovementScanDirection */ Assert(direction == ForwardScanDirection || direction == BackwardScanDirection); - return sscan->rs_rd->rd_tableam->scan_getnextslot(sscan, direction, slot); + found = sscan->rs_rd->rd_tableam->scan_getnextslot(sscan, direction, slot); + pg_assume(!found || !TupIsNull(slot)); + + return found; } /* ---------------------------------------------------------------------------- -- 2.44.0
