On Tue, 26 Mar 2019 at 11:54, Tom Lane <t...@sss.pgh.pa.us> wrote: > > Andres Freund <and...@anarazel.de> writes: > > I was kinda hoping to keep block numbers out of the "main" APIs, to > > avoid assuming everything is BLCKSZ based. I don't have a particular > > problem allowing an optional setscanlimits type callback that works with > > block numbers. The planner could check its presence and just not build > > tid range scans if not present. Alternatively a bespoke scan API for > > tid range scans, like the later patches in the tableam series for > > bitmap, sample, analyze scans, might be an option. > > Given Andres' API concerns, and the short amount of time remaining in > this CF, I'm not sure how much of this patch set we can expect to land > in v12. It seems like it might be a good idea to scale back our ambitions > and see whether there's a useful subset we can push in easily.
I agree. It'll take some time to digest Andres' advice and write a better patch. Should I set update CF app to a) set the target version to 13, and/or move it to next commitfest? > With that in mind, I went ahead and pushed 0001+0004, since improving > the planner's selectivity estimate for a "ctid vs constant" qual is > likely to be helpful whether the executor is smart about it or not. Cool. > FWIW, I don't really see the point of treating 0002 as a separate patch. > If it had some utility on its own, then it'd be sensible, but what > would that be? Also, it looks from 0002 like you are trying to overload > rs_startblock with a different meaning than it has for syncscans, and > I think that might be a bad idea. Yeah I don't think either patch is useful without the other. They were separate because, initially, only some of the TidRangeScan functionality depended on it, and I was particularly uncomfortable with what I was doing to heapam.c. The changes in heapam.c were required for backward scan support, as used by ORDER BY ctid DESC and MAX(ctid); and also for FETCH LAST and FETCH PRIOR. I have removed the backward scans functionality from the current set of patches, but support for backward cursor fetches remains. I guess to brutally simplify the patch further, we could give up backward cursor fetches entirely? This means such cursors that end up using a TidRangeScan will require SCROLL to go backwards (which is a small pain for user experience), but TBH I don't think backwards-going cursors on CTID will be hugely common. I'm still not familiar enough with heapam.c to have any better ideas on how to support backward scanning a limited range.
