Re: Tid scan improvements

Hi,

On 2019-07-17 23:10:52 +1200, David Rowley wrote:
> When I mentioned up-thread about the optional scan_setlimits table AM
> callback, I'd forgotten that you'd not have access to check that
> directly during planning. As you mention above, you've added
> RelOptInfo has_scan_setlimits so the planner knows if it can use TID
> Range scans or not. It would be nice to not have to add this flag, but
> that would require either:

Is it really a problem to add that flag?  We've obviously so far not
care about space in RelOptInfo, otherwise it'd have union members for
the per reloptinfo contents...


> 1. Making scan_setlimits a non-optional callback function in table AM, or;
> 2. Allowing the planner to have access to the opened Relation.

> #2 is not for this patch, but there has been some talk about it. It
> was done for the executor last year in d73f4c74dd3.
> 
> I wonder if Andres has any thoughts on #1?

I'm inclined to think that 1) isn't a good idea. I'd very much like to
avoid adding further dependencies on BlockNumber in non-optional APIs
(we ought to go the other way). Most of the current ones are at least
semi-reasonably implementable for most AMs (e.g. converting to PG
pagesize for relation_estimate_size isn't a problem), but it doesn't
seem to make sense to implement this for scan limits: Many AMs won't use
the BlockNumber/Offset split as heap does.

I think the AM part of this patch might be the wrong approach - it won't
do anything meaningful for an AM that doesn't directly map the ctid to a
specific location in a block (e.g. zedstore).  To me it seems the
callback ought to be to get a range of tids, and the tidrange scan
shouldn't do anything but determine the range of tids the AM should
return.

- Andres