Edmund Horner <ejrh00@gmail.com> writes:
> My patch uses the same path type and executor for all extractable tidquals.
> This worked pretty well, but I am finding it difficult to reimplement it in
> the new tidpath.c code.
I didn't like that approach to begin with, and would suggest that you go
over to using a separate path type and executor node. I don't think the
amount of commonality for the two cases is all that large, and doing it
as you had it required some ugly ad-hoc conventions about the semantics
of the tidquals list. Where I think this should go is that the tidquals
list still has OR semantics in the existing path type, but you use AND
semantics in the new path type, so that "ctid > ? AND ctid < ?" is just
represented as an implicit-AND list of two simple RestrictInfos.
Thanks for the advice. This approach resembles my first draft, which had a separate executor type. However, it did have a combined path type, with an enum TidPathMethod to determine how tidquals was interpreted. At this point, I think a different path type is clearer, though generation of both types can live in tidpath.c (just as indxpath.c generates different index path types).
Hi, here's a new set of patches. This one adds a new path type called TidRangePath and a new execution node called TidRangeScan. I haven't included any of the patches for adding pathkeys to TidPaths or TidRangePaths.
1. v6-0001-Add-selectivity-estimate-for-CTID-system-variables.patch
2. v6-0002-Support-backward-scans-over-restricted-ranges-in-hea.patch
3. v6-0003-Support-range-quals-in-Tid-Scan.patch
4. v6-0004-TID-selectivity-reduce-the-density-of-the-last-page-.patch
Patches 1, 2, and 4 are basically unchanged from my previous post. Patch 4 is an optional tweak to the CTID selectivity estimates.
Patch 3 is a substantial rewrite from what I had before. I've checked David's most recent review and tried to make sure the new code meets his suggestions where applicable, although there is one spot where I left the code as "if (tidrangequals) ..." instead of the preferred "if (tidrangequals != NIL) ...", just for consistency with the surrounding code.
Questions --
1. Tid Range Paths are costed as random_page_cost for the first page, and sequential page cost for the remaining pages. It made sense when there could be multiple non-overlapping ranges. Now that there's only one range, it might not, but it has the benefit of making Tid Range Scans a little bit more expensive than Sequential Scans, so that they are less likely to be picked when a Seq Scan will do just as well. Is there a better cost formula to use?
2. Is it worth trying to get rid of some of the code duplication between the TidPath and TidRangePath handling, such as in costsize.c or createplan.c?
3. TidRangeRecheck (copied from TidRecheck) has an existing comment asking whether it should actually be performing a check on the returned tuple. It seems to me that as long as TidRangeNext doesn't return a tuple outside the requested range, then the check shouldn't be necessary (and we'd simplify the comment to "nothing to check"). If a range key can change at runtime, it should never have been included in the TidRangePath. Is my understanding correct?
4. I'm a little uncomfortable with the way heapam.c changes the scan limits ("--scan->rs_numblocks") as it progresses through the pages. I have the executor node reset the scan limits after scanning all the tuples, which seems to work for the tests I have, but I'm using the heap_setscanlimits feature in a slightly different way from the only existing use, which is for the one-off scans when building a BRIN index. I have added some tests for cursor fetches which seems to exercise the code, but I'd still appreciate close review of how I'm using heapam.
Edmund