On Mon, Jan 26, 2026 at 5:27 PM Amit Kapila <[email protected]> wrote: > > On Sat, Jan 24, 2026 at 9:01 PM Andres Freund <[email protected]> wrote: > > > > On 2026-01-24 15:23:44 +0530, Amit Kapila wrote: > > > On Sat, Jan 24, 2026 at 1:46 AM Andres Freund <[email protected]> wrote: > > > > > > > > - The checkXidAlive checks that have been added to > > > > table_scan_getnextslot() > > > > show up noticeably and in every loop iteration, despite afaict never > > > > being reachable > > > > > > > > It's not obvious to me that this should > > > > a) be in table_scan_getnextslot(), rather than in beginscan - how > > > > could it > > > > change in the middle of a scan? That would require a wrapper around > > > > rd_tableam->scan_begin(), but that seems like it might be good > > > > anyway. > > > > b) not just be an assertion? > > > > > > > > > > IIRC, the main reason for having this precautionary check in the API > > > is to ensure that during logical decoding we never access the table AM > > > or > > > heap APIs directly when scanning catalog tables. This restriction > > > exists because we only check for concurrent aborts inside the > > > systable_* APIs. > > > > I know why the check exists - but why does it have to be in > > table_scan_getnextslot(), which is executed very frequently, rather than > > table_beginscan*(), which is executed much less frequently. > > > > I thought about this point and couldn't think of any reason why this > check can't be in table_beginscan*(). I think your idea of having a > wrapper around scan_begin() to handle this check is a good one.
Here is the patch. I've used table_scan_begin_wrapper() to wrap the scan_begin() callback for now. If you have a better naming preference to avoid the 'wrapper' suffix, please let me know. -- Regards, Dilip Kumar Google
v1-0001-Refactor-Move-CheckXidAlive-check-to-table_begins.patch
Description: Binary data
