Thread: visibility maps and heap_prune
<br />ISTM that the PD_ALL_VISIBLE flag and the visibility map bit can be set at the end of pruning operation if we knowthat there are only tuples visible to all transactions left in the page. The way pruning is done, I think it would bestraight forward to get this information.<br /><br />Thanks,<br />Pavan<br clear="all" /><br />-- <br />Pavan Deolasee<br/>EnterpriseDB <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br />
Pavan Deolasee wrote: > ISTM that the PD_ALL_VISIBLE flag and the visibility map bit can be set at > the end of pruning operation if we know that there are only tuples visible > to all transactions left in the page. Right. > The way pruning is done, I think it > would be straight forward to get this information. Is it? I thought about that a bit while writing the patch, but didn't see any obvious way to do it. Except by adding a loop through all tuples on the page, but that's extra overhead. I think we're looping through all tuples in the pruning, but it's not quite obvious. If you see a straightforward way, please submit a patch! -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
<br /><br /><div class="gmail_quote">On Sat, Dec 6, 2008 at 8:08 PM, Heikki Linnakangas <span dir="ltr"><<a href="mailto:heikki.linnakangas@enterprisedb.com">heikki.linnakangas@enterprisedb.com</a>></span>wrote:<br /><blockquoteclass="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left:1ex;"><br /> If you see a straightforward way, please submit a patch!<br /><br /></blockquote></div><br />Willdo that.<br /><br />Thanks,<br />Pavan<br /><br clear="all" /><br />-- <br />Pavan Deolasee<br />EnterpriseDB <ahref="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br />
On Mon, Dec 8, 2008 at 11:33 AM, Pavan Deolasee <pavan.deolasee@gmail.com> wrote:
Will do that.On Sat, Dec 6, 2008 at 8:08 PM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote:
If you see a straightforward way, please submit a patch!
Here is a patch which implements this. The PD_ALL_VISIBLE flag is set if all tuples in the page are visible to all transactions and there are no DEAD line pointers in the page. The second check is required so that VACUUM takes up the page. We could slightly distinguish the two cases (one where the page requires vacuuming only because of DEAD line pointers and the other where the page-tuples do not require any visibility checks), but I thought its not worth the complexity.
Thanks,
Pavan
--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com
Attachment
Is this something for 8.4 CVS? --------------------------------------------------------------------------- Pavan Deolasee wrote: > On Mon, Dec 8, 2008 at 11:33 AM, Pavan Deolasee <pavan.deolasee@gmail.com>wrote: > > > > > > > On Sat, Dec 6, 2008 at 8:08 PM, Heikki Linnakangas < > > heikki.linnakangas@enterprisedb.com> wrote: > > > >> > >> If you see a straightforward way, please submit a patch! > >> > >> > > Will do that. > > > > > > Here is a patch which implements this. The PD_ALL_VISIBLE flag is set if all > tuples in the page are visible to all transactions and there are no DEAD > line pointers in the page. The second check is required so that VACUUM takes > up the page. We could slightly distinguish the two cases (one where the page > requires vacuuming only because of DEAD line pointers and the other where > the page-tuples do not require any visibility checks), but I thought its not > worth the complexity. > > Thanks, > Pavan > > -- > Pavan Deolasee > EnterpriseDB http://www.enterprisedb.com [ Attachment, skipping... ] > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Thu, Jan 15, 2009 at 7:10 AM, Bruce Momjian <bruce@momjian.us> wrote: > > Is this something for 8.4 CVS? > I worked out the patch as per Heikki's suggestion. So I think he needs to review and decide it's fate. Thanks, Pavan -- Pavan Deolasee EnterpriseDB http://www.enterprisedb.com
Pavan Deolasee wrote: > On Thu, Jan 15, 2009 at 7:10 AM, Bruce Momjian <bruce@momjian.us> wrote: >> Is this something for 8.4 CVS? > > I worked out the patch as per Heikki's suggestion. So I think he needs > to review and decide it's fate. Yeah, I dropped the ball on that one. It's been knocking in the back of my head since, but I've never gotten around. I'm feeling reluctant to review it since it's not really a high priority thing, and I'm not sure whether we want it or not. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
> Yeah, I dropped the ball on that one. It's been knocking in the back of my > head since, but I've never gotten around. I'm feeling reluctant to review it > since it's not really a high priority thing, and I'm not sure whether we > want it or not. In that case perhaps we should add it to http://wiki.postgresql.org/wiki/CommitFest_2009-First and let it go for 8.4. ...Robert
Pavan Deolasee wrote: > On Mon, Dec 8, 2008 at 11:33 AM, Pavan Deolasee <pavan.deolasee@gmail.com>wrote: > > > > > > > On Sat, Dec 6, 2008 at 8:08 PM, Heikki Linnakangas < > > heikki.linnakangas@enterprisedb.com> wrote: > > > >> > >> If you see a straightforward way, please submit a patch! > >> > >> > > Will do that. > > > > > > Here is a patch which implements this. The PD_ALL_VISIBLE flag is set if all > tuples in the page are visible to all transactions and there are no DEAD > line pointers in the page. The second check is required so that VACUUM takes > up the page. We could slightly distinguish the two cases (one where the page > requires vacuuming only because of DEAD line pointers and the other where > the page-tuples do not require any visibility checks), but I thought its not > worth the complexity. Is this patch for 8.4? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > Pavan Deolasee wrote: >> On Mon, Dec 8, 2008 at 11:33 AM, Pavan Deolasee <pavan.deolasee@gmail.com>wrote: >> >>> >>> On Sat, Dec 6, 2008 at 8:08 PM, Heikki Linnakangas < >>> heikki.linnakangas@enterprisedb.com> wrote: >>> >>>> If you see a straightforward way, please submit a patch! >>>> >>>> >>> Will do that. >>> >>> >> Here is a patch which implements this. The PD_ALL_VISIBLE flag is set if all >> tuples in the page are visible to all transactions and there are no DEAD >> line pointers in the page. The second check is required so that VACUUM takes >> up the page. We could slightly distinguish the two cases (one where the page >> requires vacuuming only because of DEAD line pointers and the other where >> the page-tuples do not require any visibility checks), but I thought its not >> worth the complexity. > > Is this patch for 8.4? We already went through this: http://archives.postgresql.org/message-id/496F6A8E.8020908@enterprisedb.com I guess I'll follow Robert's advice and add this to the first 8.5 commit fest page. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Mon, Dec 8, 2008 at 06:56, Pavan Deolasee<pavan.deolasee@gmail.com> wrote:
> Here is a patch which implements this. The PD_ALL_VISIBLE flag is set if all
> tuples in the page are visible to all transactions and there are no DEAD
> line pointers in the page. The second check is required so that VACUUM takes
> up the page. We could slightly distinguish the two cases (one where the page
> requires vacuuming only because of DEAD line pointers and the other where
> the page-tuples do not require any visibility checks), but I thought its not
> worth the complexity.
Hi!
I was round robin assigned to review this. So take my comments with
the grain of salt (or novice HOT salt) they deserve.
I did some quick performance testing that basically boiled down to:
insert
(hot) update
select
to see if I could detect any noticeable performance difference (see
attachments for more detail for exact queries ran, all run with
autovac off).
The only major difference was with this patch vacuum time (after the
first select after some hot updates) was significantly reduced for my
test case (366ms vs 16494ms).
There was no noticeable (within noise) select or update slow down.
I was able to trigger WARNING: PD_ALL_VISIBLE flag once while running
pgbench but have not be able to re-create it... (should I keep
trying?)
See comments on patch below...
>Index: src/backend/access/heap/pruneheap.c
<snip>
>*************** heap_page_prune_opt(Relation relation, B
>*** 118,125 ****
> (void) heap_page_prune(relation, buffer, OldestXmin, false, true);
> }
>
>! /* And release buffer lock */
>! LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
> }
> }
>
>--- 124,150 ----
> (void) heap_page_prune(relation, buffer, OldestXmin, false, true);
> }
>
>! /*
>! * Since the visibility map page may require an I/O,release the buffer
>! * lock before updating the visibility map.
>! */
Would it be worth having heap_page_prune() return or pass in a ptr so
we can saw we need to update the visibility map because we set/changed
PageIsAllVisible?
>! if (PageIsAllVisible(page))
>! {
>! Buffer vmbuffer = InvalidBuffer;
>! /* Release buffer lock */
>! LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
>!
>! visibilitymap_pin(relation, BufferGetBlockNumber(buffer), &vmbuffer);
>! LockBuffer(buffer, BUFFER_LOCK_SHARE);
>! if (PageIsAllVisible(page))
>! visibilitymap_set(relation, BufferGetBlockNumber(buffer),
>! PageGetLSN(page), &vmbuffer);
>! LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
>! if (BufferIsValid(vmbuffer))
>! ReleaseBuffer(vmbuffer);
>! }
>! else
>! LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
> }
> }
>
<snip>
>*************** heap_page_prune(Relation relation, Buffe
>*** 245,250 ****
>--- 277,291 ----
> */
> PageClearFull(page);
>
>+ /* Update the all-visible flag on the page */
>+ if (!PageIsAllVisible(page) && prstate.all_visible)
>+ PageSetAllVisible(page);
>+ else if (PageIsAllVisible(page) && !prstate.all_visible)
>+ {
>+ elog(WARNING, "PD_ALL_VISIBLE flag was incorrectly set");
>+ PageClearAllVisible(page);
Hrm do we need to update the visibility map ? AFAICT this wont update
it it because we only check for PageIsAllVisible() in
heap_page_prune_opt().
>+ }
>+
> MarkBufferDirty(buffer);
>
> /*
>*************** heap_page_prune(Relation relation, Buffe
>*** 282,287 ****
>--- 323,341 ----
> PageClearFull(page);
> SetBufferCommitInfoNeedsSave(buffer);
> }
>+
>+ /* Update the all-visible flag on the page */
>+ if (!PageIsAllVisible(page) && prstate.all_visible)
>+ {
>+ PageSetAllVisible(page);
>+ SetBufferCommitInfoNeedsSave(buffer);
>+ }
>+ else if (PageIsAllVisible(page) && !prstate.all_visible)
>+ {
>+ elog(WARNING, "PD_ALL_VISIBLE flag was incorrectly set");
>+ PageClearAllVisible(page);
>+ SetBufferCommitInfoNeedsSave(buffer);
Same question as above.
>+ }
> }
>
> END_CRIT_SECTION();
<snip>
>*************** heap_prune_chain(Relation relation, Buff
>*** 495,503 ****
>--- 557,596 ----
> */
> heap_prune_record_prunable(prstate,
> HeapTupleHeaderGetXmax(htup));
>+ prstate->all_visible = false;
> break;
>
> case HEAPTUPLE_LIVE:
>+ /*
>+ * Is the tuple definitely visible to all transactions?
>+ *
>+ * NB: Like with per-tuple hint bits, we can't set the
>+ * PD_ALL_VISIBLE flag if the inserter committed
>+ * asynchronously. See SetHintBits for more info. Check
>+ * that the HEAP_XMIN_COMMITTED hint bit is set because of
>+ * that.
>+ */
>+ if (prstate->all_visible)
>+ {
>+ TransactionId xmin;
>+
>+ if (!(htup->t_infomask & HEAP_XMIN_COMMITTED))
>+ {
>+ prstate->all_visible = false;
>+ break;
>+ }
>+ /*
>+ * The inserter definitely committed. But is it
>+ * old enough that everyone sees it as committed?
>+ */
>+ xmin = HeapTupleHeaderGetXmin(htup);
>+ if (!TransactionIdPrecedes(xmin, OldestXmin))
>+ {
>+ prstate->all_visible = false;
>+ break;
>+ }
>+ }
>+ break;
(nitpick) missing newline
> case HEAPTUPLE_INSERT_IN_PROGRESS:
>
> /*>
Attachment
On Wed, Jul 15, 2009 at 11:44 PM, Alex Hunsaker<badalex@gmail.com> wrote: > On Mon, Dec 8, 2008 at 06:56, Pavan Deolasee<pavan.deolasee@gmail.com> wrote: >> Here is a patch which implements this. The PD_ALL_VISIBLE flag is set if all >> tuples in the page are visible to all transactions and there are no DEAD >> line pointers in the page. The second check is required so that VACUUM takes >> up the page. We could slightly distinguish the two cases (one where the page >> requires vacuuming only because of DEAD line pointers and the other where >> the page-tuples do not require any visibility checks), but I thought its not >> worth the complexity. > > Hi! > > I was round robin assigned to review this. So take my comments with > the grain of salt (or novice HOT salt) they deserve. Pavan, are you planning to respond to Alex's comments and/or update this patch? ...Robert
On Tue, Jul 21, 2009 at 10:38 AM, Robert Haas<robertmhaas@gmail.com> wrote: > > Pavan, are you planning to respond to Alex's comments and/or update this patch? > Yes, I will. Hopefully by end of this week. Thanks, Pavan -- Pavan Deolasee EnterpriseDB http://www.enterprisedb.com
On Tue, Jul 21, 2009 at 2:37 AM, Pavan Deolasee<pavan.deolasee@gmail.com> wrote: > On Tue, Jul 21, 2009 at 10:38 AM, Robert Haas<robertmhaas@gmail.com> wrote: >> Pavan, are you planning to respond to Alex's comments and/or update this patch? > > Yes, I will. Hopefully by end of this week. Since it has now been 10 days since this patch was reviewed, I think that it is more than fair to move this from "Waiting on Author" to "Returned with Feedback". As I've said on other threads, we want to give everyone a fair chance to respond to review comments, but we also don't want to tie up reviewers indefinitely on patches that aren't being updated in a timely fashion, and we don't want to be left with a crush of patches that need to be re-reviewed at the very end of the CommitFest when suddenly everyone updates them. So I'm going to go make this change. I hope, though, that this will be resubmitted, after appropriate updating, for a future CommitFest. I haven't read the code so I can't speak at all to whether it works (in which I'm including crash-safe, deadlock-proof, and correct with respect to locking), but if so it sounds like a nice improvement. Thanks, ...Robert
Whatever happened to this? It was in the first 9.0 commitfest but was returned with feedback but never updated: https://commitfest.postgresql.org/action/patch_view?id=75 --------------------------------------------------------------------------- Pavan Deolasee wrote: > ISTM that the PD_ALL_VISIBLE flag and the visibility map bit can be set at > the end of pruning operation if we know that there are only tuples visible > to all transactions left in the page. The way pruning is done, I think it > would be straight forward to get this information. > > Thanks, > Pavan > > -- > Pavan Deolasee > EnterpriseDB http://www.enterprisedb.com -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.comPG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do + If your life is a hard drive,Christ can be your backup. +
On Thu, Feb 25, 2010 at 9:49 PM, Bruce Momjian <bruce@momjian.us> wrote: > Whatever happened to this? It was in the first 9.0 commitfest but was > returned with feedback but never updated: > > https://commitfest.postgresql.org/action/patch_view?id=75 Well, the patch author chose not to pursue it. It's clearly far too late now, at least for 9.0. I'm pleased to see that you're not finding many patches that just completely slipped through the cracks - seems like most things were withdrawn on purpose, had problems, and/or were not pursued by the author. I think the CommitFest process has done a pretty good job of making sure everything gets looked at. The only small chink I see is that there may be some patches (especially small ones or from first-time contributors) which escaped getting added to a CommitFest in the first place; and we don't really have a way of policing that. Usually someone replies to the patch author and suggests adding it to the next CF, but I can't swear that that happens in every case. ...Robert
Robert Haas wrote: > On Thu, Feb 25, 2010 at 9:49 PM, Bruce Momjian <bruce@momjian.us> wrote: > > Whatever happened to this? ?It was in the first 9.0 commitfest but was > > returned with feedback but never updated: > > > > ? ? ? ?https://commitfest.postgresql.org/action/patch_view?id=75 > > Well, the patch author chose not to pursue it. It's clearly far too > late now, at least for 9.0. > > I'm pleased to see that you're not finding many patches that just > completely slipped through the cracks - seems like most things were > withdrawn on purpose, had problems, and/or were not pursued by the > author. I think the CommitFest process has done a pretty good job of > making sure everything gets looked at. The only small chink I see is > that there may be some patches (especially small ones or from > first-time contributors) which escaped getting added to a CommitFest > in the first place; and we don't really have a way of policing that. > Usually someone replies to the patch author and suggests adding it to > the next CF, but I can't swear that that happens in every case. Yea, the complex issues are often lost, and I stopped tracking commitfest items so I don't actually know if anything that got into the commit fest was eventually just dropped by the author. We can say we don't need to persue those but they might be valuable/important. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.comPG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do + If your life is a hard drive,Christ can be your backup. +
On Thu, Feb 25, 2010 at 10:32 PM, Bruce Momjian <bruce@momjian.us> wrote: > Robert Haas wrote: >> On Thu, Feb 25, 2010 at 9:49 PM, Bruce Momjian <bruce@momjian.us> wrote: >> > Whatever happened to this? ?It was in the first 9.0 commitfest but was >> > returned with feedback but never updated: >> > >> > ? ? ? ?https://commitfest.postgresql.org/action/patch_view?id=75 >> >> Well, the patch author chose not to pursue it. It's clearly far too >> late now, at least for 9.0. >> >> I'm pleased to see that you're not finding many patches that just >> completely slipped through the cracks - seems like most things were >> withdrawn on purpose, had problems, and/or were not pursued by the >> author. I think the CommitFest process has done a pretty good job of >> making sure everything gets looked at. The only small chink I see is >> that there may be some patches (especially small ones or from >> first-time contributors) which escaped getting added to a CommitFest >> in the first place; and we don't really have a way of policing that. >> Usually someone replies to the patch author and suggests adding it to >> the next CF, but I can't swear that that happens in every case. > > Yea, the complex issues are often lost, and I stopped tracking > commitfest items so I don't actually know if anything that got into the > commit fest was eventually just dropped by the author. We can say we > don't need to persue those but they might be valuable/important. Yes, they could be valuable/important - anything that falls into that category is probably going to turn into a TODO list item at this point. ...Robert
<br /><br /><div class="gmail_quote">On Fri, Feb 26, 2010 at 8:19 AM, Bruce Momjian <span dir="ltr"><<a href="mailto:bruce@momjian.us">bruce@momjian.us</a>></span>wrote:<br /><blockquote class="gmail_quote" style="border-left:1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br /> Whatever happenedto this? It was in the first 9.0 commitfest but was<br /> returned with feedback but never updated:<br /><br /></blockquote></div><br/>Though Alex did some useful tests and review, and in fact confirmed that the VACUUM time droppedfrom 16494 msec to 366 msec, I somehow kept waiting for Heikki's decision on the general direction of the patch andlost interest in between. If we are still interested in this, I can work out a patch and submit for next release if notthis.<br clear="all" /><br />Thanks,<br />Pavan<br /><br />-- <br />Pavan Deolasee<br />EnterpriseDB <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br/>
Pavan Deolasee wrote: > On Fri, Feb 26, 2010 at 8:19 AM, Bruce Momjian <bruce@momjian.us> wrote: > > > > > Whatever happened to this? It was in the first 9.0 commitfest but was > > returned with feedback but never updated: > > > > > Though Alex did some useful tests and review, and in fact confirmed that the > VACUUM time dropped from 16494 msec to 366 msec, I somehow kept waiting for > Heikki's decision on the general direction of the patch and lost interest in > between. If we are still interested in this, I can work out a patch and > submit for next release if not this. OK, TODO added: Have single-page pruning update the visibility map* https://commitfest.postgresql.org/action/patch_view?id=75 Hopefully Heikki can comment on this. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.comPG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do + If your life is a hard drive,Christ can be your backup. +
Bruce Momjian wrote: > Pavan Deolasee wrote: >> On Fri, Feb 26, 2010 at 8:19 AM, Bruce Momjian <bruce@momjian.us> wrote: >> >>> Whatever happened to this? It was in the first 9.0 commitfest but was >>> returned with feedback but never updated: >>> >>> >> Though Alex did some useful tests and review, and in fact confirmed that the >> VACUUM time dropped from 16494 msec to 366 msec, I somehow kept waiting for >> Heikki's decision on the general direction of the patch and lost interest in >> between. If we are still interested in this, I can work out a patch and >> submit for next release if not this. > > OK, TODO added: > > Have single-page pruning update the visibility map > * https://commitfest.postgresql.org/action/patch_view?id=75 > > Hopefully Heikki can comment on this. I think I was worried about the possible performance impact of having to clear the bit in visibility map again. If you're frequently updating a tuple so that HOT and page pruning is helping you, setting the bit in visibility map seems counter-productive; it's going to be cleared soon again by another UPDATE. That's just a hunch, though. Maybe the overhead is negligible. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas wrote: > Bruce Momjian wrote: > > Pavan Deolasee wrote: > >> On Fri, Feb 26, 2010 at 8:19 AM, Bruce Momjian <bruce@momjian.us> wrote: > >> > >>> Whatever happened to this? It was in the first 9.0 commitfest but was > >>> returned with feedback but never updated: > >>> > >>> > >> Though Alex did some useful tests and review, and in fact confirmed that the > >> VACUUM time dropped from 16494 msec to 366 msec, I somehow kept waiting for > >> Heikki's decision on the general direction of the patch and lost interest in > >> between. If we are still interested in this, I can work out a patch and > >> submit for next release if not this. > > > > OK, TODO added: > > > > Have single-page pruning update the visibility map > > * https://commitfest.postgresql.org/action/patch_view?id=75 > > > > Hopefully Heikki can comment on this. > > I think I was worried about the possible performance impact of having to > clear the bit in visibility map again. If you're frequently updating a > tuple so that HOT and page pruning is helping you, setting the bit in > visibility map seems counter-productive; it's going to be cleared soon > again by another UPDATE. That's just a hunch, though. Maybe the overhead > is negligible. Should I remove the TODO item? I updated the text to: Consider having single-page pruning update the visibility map and added a URL to Heikki's new comment. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.comPG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do + If your life is a hard drive,Christ can be your backup. +