Re: Tid scan improvements

On Thu, 11 Jul 2019 at 10:22, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> wrote:
> So it seems that the plan is to insist that TIDs are tuple identifiers
> for all table AMs, for now. If that changes in the future, then so be
> it, but I don't think that's cause for delaying any work on TID Range
> Scans. Also from scanning around tableam.h, I see that there's no
> shortage of usages of BlockNumber, so it seems reasonable to assume
> table AMs must use blocks... It's hard to imagine moving away from
> that given that we have shared buffers.
>
> We do appear to have some table AM methods that are optional, although
> I'm not sure where the documentation is about that. For example, in
> get_relation_info() I see:
>
> info->amhasgetbitmap = amroutine->amgetbitmap != NULL &&
> relation->rd_tableam->scan_bitmap_next_block != NULL;
>
> so it appears that at least scan_bitmap_next_block is optional.
>
> I think what I'd do would be to add a table_setscanlimits API method
> for table AM and perhaps have the planner only add TID range scan
> paths if the relation has a non-NULL function pointer for that API
> function. It would be good to stick a comment at least in tableam.h
> that mentions that the callback is optional. That might help a bit
> when it comes to writing documentation on each API function and what
> they do.

Hi. Here's a new patch.

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).

The patch now only targets the simple use case of restricting the
range of a table to scan. I think it would be nice to eventually
support the other major use cases of ORDER BY ctid ASC/DESC and
MIN/MAX(ctid), but that can be another feature...

Edmund