From: | David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> |
---|---|
To: | Edmund Horner <ejrh00(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de> |
Cc: | Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, David Steele <david(at)pgmasters(dot)net>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Tid scan improvements |
Date: | 2019-07-17 11:10:52 |
Message-ID: | CAKJS1f-+JJpm6B_NThUWzFv4007zAjObBXX1CBHE_bH9nOAvSw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, 15 Jul 2019 at 17:54, Edmund Horner <ejrh00(at)gmail(dot)com> wrote:
> Summary of changes compared to last time:
> - I've added the additional "scan_setlimits" table AM method. To
> check whether it's implemented in the planner, I have added an
> additional "has_scan_setlimits" flag to RelOptInfo. It seems to work.
> - I've also changed nodeTidrangescan to not require anything from heapam now.
> - To simply the patch and avoid changing heapam, I've removed the
> backward scan support (which was needed for FETCH LAST/PRIOR) and made
> ExecSupportsBackwardScan return false for this plan type.
> - I've removed the vestigial passing of "direction" through
> nodeTidrangescan. If my understanding is correct, the direction
> passed to TidRangeNext will always be forward. But I did leave FETCH
> LAST/PRIOR in the regression tests (after adding SCROLL to the
> cursor).
I spent some time today hacking at this. I fixed a bug in how
has_scan_setlimits set, rewrite a few comments and simplified some of
the code.
When I mentioned up-thread about the optional scan_setlimits table AM
callback, I'd forgotten that you'd not have access to check that
directly during planning. As you mention above, you've added
RelOptInfo has_scan_setlimits so the planner knows if it can use TID
Range scans or not. It would be nice to not have to add this flag, but
that would require either:
1. Making scan_setlimits a non-optional callback function in table AM, or;
2. Allowing the planner to have access to the opened Relation.
#2 is not for this patch, but there has been some talk about it. It
was done for the executor last year in d73f4c74dd3.
I wonder if Andres has any thoughts on #1?
The other thing I was thinking about was if enable_tidscan should be
in charge of TID Range scans too. I see you have it that way, but
should we be adding enable_tidrangescan? The docs claim that
enable_tidscan: "Enables or disables the query planner's use of TID
scan plan types.". Note: "types" is plural. Maybe we could call that
fate and keep it the way the patch has it already. Does anyone have
another idea about that?
I've attached a delta of the changes I made and also a complete v9 patch.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachment | Content-Type | Size |
---|---|---|
v8_to_v9_delta.patch | application/octet-stream | 27.4 KB |
v9_tid_range_scans.patch | application/octet-stream | 60.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | tushar | 2019-07-17 11:24:55 | Re: Minimal logical decoding on standbys |
Previous Message | r.zharkov | 2019-07-17 10:40:36 | Re: Intermittent pg_ctl failures on Windows |