Hi,
On 2026-01-27 16:40:55 +0530, Dilip Kumar wrote:
> > > 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.
I'd probably just go for _internal(), _impl() or such.
> diff --git a/src/backend/access/table/tableam.c
> b/src/backend/access/table/tableam.c
> index 87491796523..15a92c052d3 100644
> --- a/src/backend/access/table/tableam.c
> +++ b/src/backend/access/table/tableam.c
> [...]
> +/*
> + * table_scan_begin_wrapper
> + *
> + * A wrapper around the Table Access Method scan_begin callback. This handles
> + * centralized error checking—specifically ensuring we aren't performing
> + * table scan while CheckXidAlive is valid. This state is reserved for
> + * specific logical decoding operations where direct relation scanning is
> + * prohibited.
> + */
> +TableScanDesc
> +table_scan_begin_wrapper(Relation rel, Snapshot snapshot, int nkeys,
> + ScanKeyData *key,
> ParallelTableScanDesc pscan,
> + uint32 flags)
> +{
> + /*
> + * We don't expect direct calls to this function with valid
> CheckXidAlive
> + * for catalog or regular tables. See detailed comments in xact.c where
> + * these variables are declared.
> + */
> + if (unlikely(TransactionIdIsValid(CheckXidAlive) && !bsysscan))
> + elog(ERROR, "unexpected table_scan_begin_wrapper call during
> logical decoding");
> +
> + return rel->rd_tableam->scan_begin(rel, snapshot, nkeys, key, pscan,
> flags);
> +}
Given that some of the callers are in inline functions in tableam.h, I would
implement the wrapper there. I doubt it'll make a meaningful difference, but
it doesn't seem worth "risking" that.
Greetings,
Andres Freund
