Re: Qual push down to table AM

Hi,

On 2025-12-10 10:41:19 -0500, Robert Haas wrote:
> On Tue, Dec 9, 2025 at 6:08 PM Andres Freund <andres@anarazel.de> wrote:
> > I don't really see this being viable without first tackling two nontrivial
> > projects:
> >
> > 1) Make slot deforming for expressions & projections selective, i.e. don't
> >    deform all the leading columns, but only ones that will eventually be
> >    needed
> > 2) Perform ScanKey evaluation in slot form, to be able to cache the deforming
> >    and to make deforming of multiple columns sufficiently efficient.
> 
> IOW, I agree that we probably need to do #2. I am not entirely sure
> about #1. I'm a little afraid that trying to skip over columns without
> deforming them will add a bunch of code complexity that doesn't really
> pay off. You have to do the bookkeeping to know what to skip, and then
> how much are you really gaining by skipping it? If you can skip over a
> bunch of fixed-width columns, that's cool, but it's probably fairly
> normal to have lots of varlena columns, and then I don't really see
> that you're gaining much here. You still have to iterate through the
> tuple, and not storing the pointer to the start of each column as you
> find it doesn't seem like it will save much.

FWIW, in experiments I observed that not storing all the columns that never
are used saves pretty decent amount of cycles, just due to not having to store
the never-accessed datums in the slot (and in case of non-varlena columns, not
having to fetch the relevant data). It's probably true that the gain for
varlenas is smaller, due to the cost of determining the length, but the
difference is that if the cost of storing the columns is relevant, fields
after a NULL or a varlena still can beenfit from the optimization.


> What's your reasoning behind thinking that #1 will be necessary?

Tried to answer that downthread, in response to Amit.


> > > So, somewhat to my surprise, I think that v4-0001 might be basically
> > > fine. I wonder if anyone else sees a problem that I'm missing?
> >
> > I doubt this would be safe as-is: ISTM that if you release the page lock
> > between tuples, things like the number of items on the page can change. But we
> > store stuff like that in registers / on the stack, which could change while
> > the lock is not held.
> >
> > We could refetch the number items on the page for every loop iteration, but
> > that'd probably not be free. OTOH, it's probably nothing compared to the cost
> > of relocking the page...
> 
> We still hold a pin, though, which I think means very little can
> change. More items can be added to the page, so we might want to
> refresh the number of items on the page at least when we think we're
> done, but I believe that any sort of more invasive page rearrangement
> would be precluded by the pin.
> 
> I kind of wonder if it would be good to make a change along the lines
> of v4-0001 even if this patch set doesn't move forward overall, or
> will need a lot of slot-ification to do so. It seems weird to me that
> we're OK with calling out to arbitrary code with a buffer lock held,
> and even weirder that whether or not we do that depends on whether
> SO_ALLOW_PAGEMODE was set.

I don't think it's stated clearly anywhere - the only reason this is remotely
within a stone's throw of ok is that the only code using ScanKeys for table
scans is catcache, which in turn means that the set of effectively allowed
operators is tiny (oideq, int2eq, int4eq, int8eq, nameeq, chareq, booleq and
perhaps 2-3 more).

And for those we only support ScanKey mode because it's required as a fallback
for index based catcache searches.

I'm not against fixing qual-eval-under-buffer-lock, it shouldn't ever be used
in particularly performance sensitive cases.

Greetings,

Andres Freund