On Thu, 11 Jul 2019 at 10:22, David Rowley <david.row...@2ndquadrant.com> wrote: > > 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 <and...@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. This seems like a good path forward. > > 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. Yeah, it's not perfectly efficient in that regard. But it's at least a step in the right direction. Thanks for the guidance David. I think I'll be able to make some progress now before hitting the next obstacle! Edmund
