Tid scan improvements

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
93 messages Options
12345
Reply | Threaded
Open this post in threaded view
|

Re: Tid scan improvements

David Rowley-3
On Sun, 7 Jul 2019 at 15:32, Edmund Horner <[hidden email]> wrote:

> I'm not really sure how to proceed.  I started with a fairly pragmatic
> solution to "WHERE ctid > ? AND ctid < ?" for tables, and then tableam
> came along.
>
> The options I see are:
>
> 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 <[hidden email]> 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.

> 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.

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


Reply | Threaded
Open this post in threaded view
|

Re: Tid scan improvements

Edmund Horner
On Thu, 11 Jul 2019 at 10:22, David Rowley <[hidden email]> 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 <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Tid scan improvements

Edmund Horner
In reply to this post by David Rowley-3
On Thu, 11 Jul 2019 at 10:22, David Rowley <[hidden email]> wrote:

> 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.
Hi.  Here's a new patch.

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).

The patch now only targets the simple use case of restricting the
range of a table to scan.  I think it would be nice to eventually
support the other major use cases of ORDER BY ctid ASC/DESC and
MIN/MAX(ctid), but that can be another feature...

Edmund

v8-0001-Add-a-new-plan-type-Tid-Range-Scan-to-support-range-.patch (83K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Tid scan improvements

David Rowley-3
On Mon, 15 Jul 2019 at 17:54, Edmund Horner <[hidden email]> 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

v8_to_v9_delta.patch (37K) Download Attachment
v9_tid_range_scans.patch (82K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Tid scan improvements

Edmund Horner
Thanks for the edits and fixing that pretty glaring copy-paste bug.

Regarding enable_tidscan, I couldn't decide whether we really need it,
and erred on the side of not adding yet another setting.

The current patch only creates a tid range path if there's at least
one ctid qual.  But during development of earlier patches I was a bit
concerned about the possibility of tid range scan being picked instead
of seq scan when the whole table is scanned, perhaps due to a tiny
discrepency in costing.  Both scans will scan the whole table, but seq
scan is preferred since it can be parallellised, synchronised with
other scans, and has a bit less overhead with tuple checking.  If a
future change creates tid range paths for more queries, for instance
for MIN/MAX(ctid) or ORDER BY ctid, then it might be more important to
have a separate setting for it.

On Wed, 17 Jul 2019 at 23:11, David Rowley <[hidden email]> wrote:

>
> On Mon, 15 Jul 2019 at 17:54, Edmund Horner <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Tid scan improvements

Andres Freund
In reply to this post by David Rowley-3
Hi,

On 2019-07-17 23:10:52 +1200, David Rowley wrote:
> 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:

Is it really a problem to add that flag?  We've obviously so far not
care about space in RelOptInfo, otherwise it'd have union members for
the per reloptinfo contents...


> 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?

I'm inclined to think that 1) isn't a good idea. I'd very much like to
avoid adding further dependencies on BlockNumber in non-optional APIs
(we ought to go the other way). Most of the current ones are at least
semi-reasonably implementable for most AMs (e.g. converting to PG
pagesize for relation_estimate_size isn't a problem), but it doesn't
seem to make sense to implement this for scan limits: Many AMs won't use
the BlockNumber/Offset split as heap does.

I think the AM part of this patch might be the wrong approach - it won't
do anything meaningful for an AM that doesn't directly map the ctid to a
specific location in a block (e.g. zedstore).  To me it seems the
callback ought to be to get a range of tids, and the tidrange scan
shouldn't do anything but determine the range of tids the AM should
return.

- Andres


Reply | Threaded
Open this post in threaded view
|

Re: Tid scan improvements

David Rowley-3
On Thu, 18 Jul 2019 at 14:30, Andres Freund <[hidden email]> wrote:
> I think the AM part of this patch might be the wrong approach - it won't
> do anything meaningful for an AM that doesn't directly map the ctid to a
> specific location in a block (e.g. zedstore).  To me it seems the
> callback ought to be to get a range of tids, and the tidrange scan
> shouldn't do anything but determine the range of tids the AM should
> return.

Sounds like that's going to require adding some new fields to
HeapScanDescData, then some callback similar to heap_setscanlimits to
set those fields.

Then, we'd either need to:

1. Make the table_scan_getnextslot() implementations check the tuple
falls within the range, or
2. add another callback that pays attention to the set TID range.

The problem with #1 is that would add overhead to normal seqscans,
which seems like a bad idea.

Did you imagined two additional callbacks, 1 to set the TID range,
then one to scan it?  Duplicating the logic in heapgettup_pagemode()
and heapgettup() looks pretty horrible, but I guess we could add a
wrapper around it that loops until it gets the first tuple and bails
once it scans beyond the final tuple.

Is that what you had in mind?

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


Reply | Threaded
Open this post in threaded view
|

Re: Tid scan improvements

Andres Freund
Hi,

On 2019-07-19 13:54:59 +1200, David Rowley wrote:

> On Thu, 18 Jul 2019 at 14:30, Andres Freund <[hidden email]> wrote:
> > I think the AM part of this patch might be the wrong approach - it won't
> > do anything meaningful for an AM that doesn't directly map the ctid to a
> > specific location in a block (e.g. zedstore).  To me it seems the
> > callback ought to be to get a range of tids, and the tidrange scan
> > shouldn't do anything but determine the range of tids the AM should
> > return.
>
> Sounds like that's going to require adding some new fields to
> HeapScanDescData, then some callback similar to heap_setscanlimits to
> set those fields.
>
> Then, we'd either need to:
>
> 1. Make the table_scan_getnextslot() implementations check the tuple
> falls within the range, or
> 2. add another callback that pays attention to the set TID range.

> The problem with #1 is that would add overhead to normal seqscans,
> which seems like a bad idea.
>
> Did you imagined two additional callbacks, 1 to set the TID range,
> then one to scan it?  Duplicating the logic in heapgettup_pagemode()
> and heapgettup() looks pretty horrible, but I guess we could add a
> wrapper around it that loops until it gets the first tuple and bails
> once it scans beyond the final tuple.
>
> Is that what you had in mind?

Yea, I was thinking of something like 2.  We already have a few extra
types of scan nodes (bitmap heap and sample scan), it'd not be bad to
add another. And as you say, they can just share most of the guts: For
heap I'd just implement pagemode, and perhaps split heapgettup_pagemode
into two parts (one to do the page processing, the other to determine
the relevant page).

You say that we'd need new fields in HeapScanDescData - not so sure
about that, it seems feasible to just provide the boundaries in the
call? But I think it'd also just be fine to have the additional fields.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: Tid scan improvements

David Rowley-3
On Sat, 20 Jul 2019 at 06:21, Andres Freund <[hidden email]> wrote:

>
> Hi,
>
> On 2019-07-19 13:54:59 +1200, David Rowley wrote:
> > Did you imagined two additional callbacks, 1 to set the TID range,
> > then one to scan it?  Duplicating the logic in heapgettup_pagemode()
> > and heapgettup() looks pretty horrible, but I guess we could add a
> > wrapper around it that loops until it gets the first tuple and bails
> > once it scans beyond the final tuple.
> >
> > Is that what you had in mind?
>
> Yea, I was thinking of something like 2.  We already have a few extra
> types of scan nodes (bitmap heap and sample scan), it'd not be bad to
> add another. And as you say, they can just share most of the guts: For
> heap I'd just implement pagemode, and perhaps split heapgettup_pagemode
> into two parts (one to do the page processing, the other to determine
> the relevant page).
>
> You say that we'd need new fields in HeapScanDescData - not so sure
> about that, it seems feasible to just provide the boundaries in the
> call? But I think it'd also just be fine to have the additional fields.

Thanks for explaining.

I've set the CF entry for the patch back to waiting on author.

I think if we get this part the way Andres would like it, then we're
pretty close.

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


Reply | Threaded
Open this post in threaded view
|

Re: Tid scan improvements

Edmund Horner
On Mon, 22 Jul 2019 at 19:25, David Rowley <[hidden email]>

> On Sat, 20 Jul 2019 at 06:21, Andres Freund <[hidden email]> wrote:
> > Yea, I was thinking of something like 2.  We already have a few extra
> > types of scan nodes (bitmap heap and sample scan), it'd not be bad to
> > add another. And as you say, they can just share most of the guts: For
> > heap I'd just implement pagemode, and perhaps split heapgettup_pagemode
> > into two parts (one to do the page processing, the other to determine
> > the relevant page).
> >
> > You say that we'd need new fields in HeapScanDescData - not so sure
> > about that, it seems feasible to just provide the boundaries in the
> > call? But I think it'd also just be fine to have the additional fields.
>
> Thanks for explaining.
>
> I've set the CF entry for the patch back to waiting on author.
>
> I think if we get this part the way Andres would like it, then we're
> pretty close.

I've moved the code in question into heapam, with:

  * a new scan type SO_TYPE_TIDRANGE (renumbering some of the other
    enums).

  * a wrapper table_beginscan_tidrange(Relation rel, Snapshot snapshot);
    I'm not sure whether we need scankeys and the other flags?

  * a new optional callback scan_settidrange(TableScanDesc scan,
    ItemPointer startTid, ItemPointer endTid) with wrapper
table_scan_settidrange.
    I thought about combining it with table_beginscan_tidrange but we're not
    really doing that with any of the other beginscan methods.

  * another optional callback scan_getnexttidrangeslot.  The presence of
    these two callbacks indicates that TidRangeScan is supported for
this relation.

Let me know if you can think of better names.

However, the heap_getnexttidrangeslot function is just the same
iterative code calling heap_getnextslot and checking the tuples
against the tid range (two fields added to the ScanDesc).

I'll have to spend a bit of time looking at heapgettup_pagemode to
figure how to split it as Andres suggests.

Thanks,
Edmund


Reply | Threaded
Open this post in threaded view
|

Re: Tid scan improvements

Edmund Horner
On Mon, 22 Jul 2019 at 19:44, Edmund Horner <[hidden email]> wrote:

> On Mon, 22 Jul 2019 at 19:25, David Rowley <[hidden email]>
> > On Sat, 20 Jul 2019 at 06:21, Andres Freund <[hidden email]> wrote:
> > > Yea, I was thinking of something like 2.  We already have a few extra
> > > types of scan nodes (bitmap heap and sample scan), it'd not be bad to
> > > add another. And as you say, they can just share most of the guts: For
> > > heap I'd just implement pagemode, and perhaps split heapgettup_pagemode
> > > into two parts (one to do the page processing, the other to determine
> > > the relevant page).
> > >
> > > You say that we'd need new fields in HeapScanDescData - not so sure
> > > about that, it seems feasible to just provide the boundaries in the
> > > call? But I think it'd also just be fine to have the additional fields.
> >
> > Thanks for explaining.
> >
> > I've set the CF entry for the patch back to waiting on author.
> >
> > I think if we get this part the way Andres would like it, then we're
> > pretty close.

> [...]

> I'll have to spend a bit of time looking at heapgettup_pagemode to
> figure how to split it as Andres suggests.

Hi everyone,

Sadly it is the end of the CF and I have not had much time to work on
this.  I'll probably get some time in the next month to look at the
heapam changes.

Should we move to CF?  We have been in the CF cycle for almost a year now...

Edmund


Reply | Threaded
Open this post in threaded view
|

Re: Tid scan improvements

Thomas Munro-5
On Thu, Aug 1, 2019 at 5:34 PM Edmund Horner <[hidden email]> wrote:
> Sadly it is the end of the CF and I have not had much time to work on
> this.  I'll probably get some time in the next month to look at the
> heapam changes.
>
> Should we move to CF?  We have been in the CF cycle for almost a year now...

Hi Edmund,

No worries, that's how it goes sometimes.  Please move it to the next
CF if you think you'll find some time before September.  Don't worry
if it might have to be moved again.  We want the feature, and good
things take time!

--
Thomas Munro
https://enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Tid scan improvements

Edmund Horner
On Thu, 1 Aug 2019 at 17:47, Thomas Munro <[hidden email]> wrote:
> On Thu, Aug 1, 2019 at 5:34 PM Edmund Horner <[hidden email]> wrote:
> > Should we move to CF?  We have been in the CF cycle for almost a year now...
>
> Hi Edmund,
>
> No worries, that's how it goes sometimes.  Please move it to the next
> CF if you think you'll find some time before September.  Don't worry
> if it might have to be moved again.  We want the feature, and good
> things take time!

Ok thanks.

I tried moving it to the new commitfest, but it seems I cannot from
its current state.

If it's ok, I'll just leave it to you in 7 hours' time!

Edmund


Reply | Threaded
Open this post in threaded view
|

Re: Tid scan improvements

Thomas Munro-5
On Thu, Aug 1, 2019 at 5:51 PM Edmund Horner <[hidden email]> wrote:

> On Thu, 1 Aug 2019 at 17:47, Thomas Munro <[hidden email]> wrote:
> > On Thu, Aug 1, 2019 at 5:34 PM Edmund Horner <[hidden email]> wrote:
> > > Should we move to CF?  We have been in the CF cycle for almost a year now...
> >
> > Hi Edmund,
> >
> > No worries, that's how it goes sometimes.  Please move it to the next
> > CF if you think you'll find some time before September.  Don't worry
> > if it might have to be moved again.  We want the feature, and good
> > things take time!
>
> Ok thanks.
>
> I tried moving it to the new commitfest, but it seems I cannot from
> its current state.

Done.  You have to move it to "Needs review" first, and then move to
next.  (Perhaps we should change that... I don't think that obstacle
achieves anything?)

--
Thomas Munro
https://enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Tid scan improvements

David Rowley-3
On Thu, 1 Aug 2019 at 17:57, Thomas Munro <[hidden email]> wrote:
>
> On Thu, Aug 1, 2019 at 5:51 PM Edmund Horner <[hidden email]> wrote:
> > I tried moving it to the new commitfest, but it seems I cannot from
> > its current state.
>
> Done.  You have to move it to "Needs review" first, and then move to
> next.  (Perhaps we should change that... I don't think that obstacle
> achieves anything?)

I think it's there as a measure to try to trim down the number of
patches that are constantly bounced to the nest 'fest that are still
waiting on author.  In my experience, it's a little annoying since if
you don't set it to "needs review" first, then it means closing the
patch and having to create a new CF entry when you're ready, with all
the history of the previous one lost.

It seems reasonable to me to keep the patch in the queue if the author
is still actively working on the patch and it seems pretty unfair if a
last-minute review came in just before the end of the CF and that
means their patch must be "returned with feedback" instead of pushed
to the next 'fest.

Perhaps there are other measures we could take to reduce the number of
patches getting kicked out to the next CF all the time. Maybe some
icons that appear if it's been waiting on author for more than 2
months, or if it went through an entire CF as waiting on author.

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


Reply | Threaded
Open this post in threaded view
|

Re: Tid scan improvements

Alvaro Herrera-9
In reply to this post by Edmund Horner
On 2019-Aug-01, Edmund Horner wrote:

> Hi everyone,
>
> Sadly it is the end of the CF and I have not had much time to work on
> this.  I'll probably get some time in the next month to look at the
> heapam changes.
>
> Should we move to CF?  We have been in the CF cycle for almost a year now...

Hello, do we have an update on this patch?  Last version that was posted
was v9 from David on July 17th; you said you had made some changes on
July 22nd but didn't attach any patch.  v9 doesn't apply anymore.

Thanks

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Tid scan improvements

Edmund Horner
On Wed, 4 Sep 2019 at 10:34, Alvaro Herrera <[hidden email]> wrote:

>
> On 2019-Aug-01, Edmund Horner wrote:
>
> > Hi everyone,
> >
> > Sadly it is the end of the CF and I have not had much time to work on
> > this.  I'll probably get some time in the next month to look at the
> > heapam changes.
> >
> > Should we move to CF?  We have been in the CF cycle for almost a year now...
>
> Hello, do we have an update on this patch?  Last version that was posted
> was v9 from David on July 17th; you said you had made some changes on
> July 22nd but didn't attach any patch.  v9 doesn't apply anymore.

Hi pgsql-hackers,

I have had a lot of difficulty making the changes to heapam.c and I
think it's becoming obvious I won't get them done by myself.

The last *working* patch pushed the work into heapam.c, but it was
just a naive loop over the whole table.  I haven't found how to
rewrite heapgettup_pagemode in the way Andres suggests.

So, I think we need to either get some help from someone familiar with
heapam.c, or maybe shelve the patch.  It has been work in progress for
over a year now.

Edmund


Reply | Threaded
Open this post in threaded view
|

Re: Tid scan improvements

Michael Paquier-2
On Thu, Sep 05, 2019 at 01:06:56PM +1200, Edmund Horner wrote:
> So, I think we need to either get some help from someone familiar with
> heapam.c, or maybe shelve the patch.  It has been work in progress for
> over a year now.

Okay, still nothing has happened after two months.  Once this is
solved a new patch submission could be done.  For now I have marked
the entry as returned with feedback.  
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Tid scan improvements

David Fetter
On Sun, Dec 01, 2019 at 11:34:16AM +0900, Michael Paquier wrote:
> On Thu, Sep 05, 2019 at 01:06:56PM +1200, Edmund Horner wrote:
> > So, I think we need to either get some help from someone familiar with
> > heapam.c, or maybe shelve the patch.  It has been work in progress for
> > over a year now.
>
> Okay, still nothing has happened after two months.  Once this is
> solved a new patch submission could be done.  For now I have marked
> the entry as returned with feedback.  

I dusted off the last version of the patch without implementing the
suggestions in this thread between here and there.

I think this is a capability worth having, as I was surprised when it
turned out that it didn't exist when I was looking to make an
improvement to pg_dump. My idea, which I'll get back to when this is
in, was to use special knowledge of heap AM tables to make it possible
to parallelize dumps of large tables by working separately on each
underlying file, of which there could be quite a few for a large one.

Will try to understand the suggestions upthread better and implement
same.

Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

v10-0001-first-cut.patch (62K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Tid scan improvements

Edmund Horner
On Fri, 1 Jan 2021 at 14:30, David Fetter <[hidden email]> wrote:
On Sun, Dec 01, 2019 at 11:34:16AM +0900, Michael Paquier wrote:
> Okay, still nothing has happened after two months.  Once this is
> solved a new patch submission could be done.  For now I have marked
> the entry as returned with feedback. 

I dusted off the last version of the patch without implementing the
suggestions in this thread between here and there.

I think this is a capability worth having, as I was surprised when it
turned out that it didn't exist when I was looking to make an
improvement to pg_dump. My idea, which I'll get back to when this is
in, was to use special knowledge of heap AM tables to make it possible
to parallelize dumps of large tables by working separately on each
underlying file, of which there could be quite a few for a large one.

Will try to understand the suggestions upthread better and implement
same.

Hi David,

Thanks for updating the patch.  I'd be very happy if this got picked up again, and I'd certainly follow along and do some review.

+                               splan->tidrangequals =
+                                       fix_scan_list(root, splan->tidrangequals,
+                                                                 rtoffset, 1); /* v9_tid XXX Not sure this is right */

I'm pretty sure the parameter num_exec = 1 is fine; it seems to only affect correlated subselects, which shouldn't really make their way into the tidrangequals expressions.  It's more or less the same situation as tidquals for TidPath, anyway.  We could put a little comment:  /* correlated subselects shouldn't get into tidquals/tidrangequals */ or something to that effect.

Edmund

12345