Thanks for the edits and fixing that pretty glaring copy-paste bug. Regarding enable_tidscan, I couldn't decide whether we really need it, and erred on the side of not adding yet another setting.
The current patch only creates a tid range path if there's at least one ctid qual. But during development of earlier patches I was a bit concerned about the possibility of tid range scan being picked instead of seq scan when the whole table is scanned, perhaps due to a tiny discrepency in costing. Both scans will scan the whole table, but seq scan is preferred since it can be parallellised, synchronised with other scans, and has a bit less overhead with tuple checking. If a future change creates tid range paths for more queries, for instance for MIN/MAX(ctid) or ORDER BY ctid, then it might be more important to have a separate setting for it. On Wed, 17 Jul 2019 at 23:11, David Rowley <david.row...@2ndquadrant.com> wrote: > > On Mon, 15 Jul 2019 at 17:54, Edmund Horner <ejr...@gmail.com> wrote: > > Summary of changes compared to last time: > > - I've added the additional "scan_setlimits" table AM method. To > > check whether it's implemented in the planner, I have added an > > additional "has_scan_setlimits" flag to RelOptInfo. It seems to work. > > - I've also changed nodeTidrangescan to not require anything from heapam > > now. > > - To simply the patch and avoid changing heapam, I've removed the > > backward scan support (which was needed for FETCH LAST/PRIOR) and made > > ExecSupportsBackwardScan return false for this plan type. > > - I've removed the vestigial passing of "direction" through > > nodeTidrangescan. If my understanding is correct, the direction > > passed to TidRangeNext will always be forward. But I did leave FETCH > > LAST/PRIOR in the regression tests (after adding SCROLL to the > > cursor). > > I spent some time today hacking at this. I fixed a bug in how > has_scan_setlimits set, rewrite a few comments and simplified some of > the code. > > 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: > > 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? > > The other thing I was thinking about was if enable_tidscan should be > in charge of TID Range scans too. I see you have it that way, but > should we be adding enable_tidrangescan? The docs claim that > enable_tidscan: "Enables or disables the query planner's use of TID > scan plan types.". Note: "types" is plural. Maybe we could call that > fate and keep it the way the patch has it already. Does anyone have > another idea about that? > > I've attached a delta of the changes I made and also a complete v9 patch. > > -- > David Rowley http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training & Services
