Re: Tid scan improvements

On 12 August 2018 at 14:29, Edmund Horner <ejrh00(at)gmail(dot)com> wrote:
> To scratch an itch, I have been working on teaching TidScan how to do
> range queries, i.e. those using >=, <, BETWEEN, etc. This means we
> can write, for instance,
>
> SELECT * FROM t WHERE ctid >= '(1000,0)' AND ctid < '(2000,0)';

I think this will be useful to UPDATE records at the end of a bloated
table to move them into space that's been freed up by vacuum to allow
the table to be trimmed back to size again.

> Since range scan execution is rather different from the existing
> TidScan execution, I ended up making a new plan type, TidRangeScan.
> There is still only one TidPath, but it has an additional member that
> describes which method to use.

I always thought that this would be implemented by overloading
TidScan. I thought that TidListEval() could be modified to remove
duplicates accounting for range scans. For example:

SELECT * FROM t WHERE ctid BETWEEN '(0,1)' AND (0,10') OR ctid
IN('(0,5)','(0,30)');

would first sort all the tids along with their operator and then make
a pass over the sorted array to remove any equality ctids that are
redundant because they're covered in a range.

> As part of the work I also taught TidScan that its results are ordered
> by ctid, i.e. to set a pathkey on a TidPath. The benefit of this is
> that queries such as
>
> SELECT MAX(ctid) FROM t;
> SELECT * FROM t WHERE ctid IN (...) ORDER BY ctid;

I think that can be done as I see you're passing allow_sync as false
in heap_beginscan_strat(), so the scan will start at the beginning of
the heap.

> Attached are a couple of patches:
> - 01_tid_scan_ordering.patch
> - 02_tid_range_scan.patch, to be applied on top of 01.
>
> Can I add this to the next CommitFest?

Please do.

> As well as actual correctness, some aspects that I am particularly
> unsure about include:
>
> - Is it messy to use TidPath for both types of scan?

I wonder if there is a good reason to have a separate node type at
all? I've not looked, but if you've managed to overload the TidPath
struct without it getting out of control, then perhaps the same can be
done with the node type.

> - What is the planning cost for plans that don't end up being a
> TidScan or TidRangeScan?

I suppose that wouldn't matter if there was just 1 path for a single node type.

> - Is there a less brittle way to create tables of a specific number
> of blocks/tuples in the regression tests?

Perhaps you could just populate a table with some number of records
then DELETE the ones above ctid (x,100) on each page, where 100 is
whatever you can be certain will fit on a page on any platform. I'm
not quite sure if our regress test would pass with a very small block
size anyway, but probably worth verifying that before you write the
first test that will break it.

I'll try to look in a bit more detail during the commitfest.

It's perhaps a minor detail at this stage, but generally, we don't
have code lines over 80 chars in length. There are some exceptions,
e.g not breaking error message strings so that they're easily
greppable. src/tools/pgindent has a tool that you can run to fix the
whitespace so it's in line with project standard.

Thanks for working on this. It will great to see improvements made in this area.

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