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
