Re: visibility maps and heap_prune

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

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

On Sat, Dec 6, 2008 at 8:08 PM, Heikki Linnakangas <
heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:

>
> If you see a straightforward way, please submit a patch!
>
>
Will do that.

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

On Mon, Dec 8, 2008 at 11:33 AM, Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com>wrote:

>
>
> On Sat, Dec 6, 2008 at 8:08 PM, Heikki Linnakangas <
> heikki(dot)linnakangas(at)enterprisedb(dot)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

Is this something for 8.4 CVS?

---------------------------------------------------------------------------

Pavan Deolasee wrote:
> On Mon, Dec 8, 2008 at 11:33 AM, Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com>wrote:
>
> >
> >
> > On Sat, Dec 6, 2008 at 8:08 PM, Heikki Linnakangas <
> > heikki(dot)linnakangas(at)enterprisedb(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(dot)deolasee(at)gmail(dot)com>wrote:
>
> >
> >
> > On Sat, Dec 6, 2008 at 8:08 PM, Heikki Linnakangas <
> > heikki(dot)linnakangas(at)enterprisedb(dot)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(at)momjian(dot)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(dot)deolasee(at)gmail(dot)com>wrote:
>>
>>>
>>> On Sat, Dec 6, 2008 at 8:08 PM, Heikki Linnakangas <
>>> heikki(dot)linnakangas(at)enterprisedb(dot)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(dot)deolasee(at)gmail(dot)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:
>
> /*>

On Wed, Jul 15, 2009 at 11:44 PM, Alex Hunsaker<badalex(at)gmail(dot)com> wrote:
> On Mon, Dec 8, 2008 at 06:56, Pavan Deolasee<pavan(dot)deolasee(at)gmail(dot)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(at)gmail(dot)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(dot)deolasee(at)gmail(dot)com> wrote:
> On Tue, Jul 21, 2009 at 10:38 AM, Robert Haas<robertmhaas(at)gmail(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com
PG 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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com
PG 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(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> On Thu, Feb 25, 2010 at 9:49 PM, Bruce Momjian <bruce(at)momjian(dot)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

On Fri, Feb 26, 2010 at 8:19 AM, Bruce Momjian <bruce(at)momjian(dot)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.

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

Pavan Deolasee wrote:
> On Fri, Feb 26, 2010 at 8:19 AM, Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com
PG 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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com
PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do
+ If your life is a hard drive, Christ can be your backup. +