Re: Tid scan improvements

On Sun, 7 Jul 2019 at 15:32, Edmund Horner <ejrh00@gmail.com> wrote:
> I'm not really sure how to proceed.  I started with a fairly pragmatic
> solution to "WHERE ctid > ? AND ctid < ?" for tables, and then tableam
> came along.
>
> The options I see are:
>
> A.  Continue to target only heapam tables, making the bare minimum
> changes necessary for the new tableam api.
> B.  Try to do something more general that works on all tableam
> implementations for which it may be useful.

Going by the conversation with Andres above:

On Tue, 26 Mar 2019 at 10:52, Andres Freund <andres@anarazel.de> wrote:
>
> On 2019-03-18 22:35:05 +1300, David Rowley wrote:
> > The commit message in 8586bf7ed mentions:
> >
> > > Subsequent commits will incrementally abstract table access
> > > functionality to be routed through table access methods. That change
> > > is too large to be reviewed & committed at once, so it'll be done
> > > incrementally.
> >
> > and looking at [1] I see patch 0004 introduces some changes in
> > nodeTidscan.c to call a new tableam API function named
> > heapam_fetch_row_version. I see this function does have a ItemPointer
> > argument, so I guess we must be keeping those as unique row
> > identifiers in the API.
>
> Right, we are. At least for now - there's some discussions around
> allowing different format for TIDs, to allow things like index organized
> tables, but that's for later.

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.

> There may not be much different between them, but B. means a bit more
> research into zheap, zstore and other possible tableams.
>
> Next question, how will the executor access the table?
>
> 1. Continue to use the seqscan tableam methods, by setting limits.
> 2. Use the bitmap scan methods, for instance by faking a BitmapIteratorResuit.
> 3. Add new tableam methods specially for scanning a range of TIDs.
>
> Any thoughts?

I think #1 is fine for now. #3 might be slightly more efficient since
you'd not need to read each tuple in the given page before the given
offset and throw it away, but I don't really think it's worth adding a
new table AM method for.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services