Re: Tid scan improvements

On Sat, 19 Jan 2019 at 17:04, Edmund Horner <ejrh00(at)gmail(dot)com> wrote:

> On Sat, 19 Jan 2019 at 05:35, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
>> Edmund Horner <ejrh00(at)gmail(dot)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