Re: opportunistic tuple freezing

Attached is a patch to implement the idea discussed here:

http://archives.postgresql.org/pgsql-hackers/2009-08/msg01137.php

If VACUUM freezes one tuple on a page, it's likely that there are others
on the same page that are close to vacuum_freeze_min_age, but not quite.
Because the page is already dirty from freezing one tuple, it makes
sense to be more aggressive about freezing the rest, in the hope that
all the tuples will be frozen, and we will not have to dirty the page
again later.

This patch introduces a GUC vacuum_freeze_opportunistic_ratio. If one
tuple on a page is frozen by vacuum, it effectively multiplies
vacuum_freeze_min_age by vacuum_freeze_opportunistic_ratio and uses that
lower (more aggressive) value only for the current page.

The reason we don't just freeze all the tuples we can (effectively
setting the vacuum_freeze_opportunistic_ratio to zero) is to preserve
transaction ID information for diagnosing problems.

Regards,
Jeff Davis

On Mon, Aug 17, 2009 at 2:32 AM, Jeff Davis<pgsql(at)j-davis(dot)com> wrote:
>
> This patch introduces a GUC vacuum_freeze_opportunistic_ratio. If one
> tuple on a page is frozen by vacuum, it effectively multiplies
> vacuum_freeze_min_age by vacuum_freeze_opportunistic_ratio and uses that
> lower (more aggressive) value only for the current page.

I thought Josh's idea to apply this opportunistic threshold if the
page is already dirty for any reason was a good idea. Ie, if some
other dml or hint bit was set since the page was loaded even if vacuum
doesn't find any tuples are freezable.

So basically I think the logic should be:

normal-vacuum-processing
if (page-is-clean)
try-to-freeze(normal-threshold)
if (page-is-dirty)
try-to-freeze(opportunistic-threshold)

Sure it's duplicated work but I don't think it will add up to much.
The normal pass could remember the oldest xid found and we could skip
the second pass if the oldest xid is still too young.

--
greg
http://mit.edu/~gsstark/resume.pdf

Looking at the patch I didn't like that duplicated the page scan in a
second function without refactoring out the first scan. I think this
comes from the usual noncommiter-itus of trying to modify the existing
code as little as possible. At least that's the problem I've had which
led to things like this. Instead try to figure how you would have
written it if you were writing it from scratch.

If it's convenient to have a function to handle the scan then use it
for both scans. You could have a flag that indicates it should abort
after the first freeze. I think it would be simpler to have a return
value indicating the oldest transaction found and then just call it
again if that's old enough for the opportunistic threshold.

On Mon, 2009-08-17 at 04:01 +0100, Greg Stark wrote:
> If it's convenient to have a function to handle the scan then use it
> for both scans. You could have a flag that indicates it should abort
> after the first freeze. I think it would be simpler to have a return
> value indicating the oldest transaction found and then just call it
> again if that's old enough for the opportunistic threshold.

Thanks for the suggestions.

I think it will take some significant refactoring. It needs to work from
both scan_heap and lazy_scan_heap, so I would need to make a loop that
works for both of those cases. Additionally, the second scan needs to
avoid all of the side effects (like double-counting), which might mean
some ugly branching. For instance, the big switch statement is
completely unnecessary during the second scan.

I'll come up with a refactored version of the patch.

Regards,
Jeff Davis

On Mon, 2009-08-17 at 03:43 +0100, Greg Stark wrote:
> I thought Josh's idea to apply this opportunistic threshold if the
> page is already dirty for any reason was a good idea.

Is there a good way to tell if a buffer is dirty or not?

I don't see an exported function to do that. I could check by looking at
the flags directly, but I don't see that being done in any file that
doesn't have the word "buf" in it, so I assume it's breaking an
abstraction.

> Ie, if some
> other dml or hint bit was set since the page was loaded even if vacuum
> doesn't find any tuples are freezable.

For this patch, I think this optimization might be useful, but I don't
see how it would be important. The primary optimization in my patch is
that VACUUM is likely to freeze all of the xids on a page at once.

Hint bits are likely to be set before the tuples are eligible for
freezing (unless the new GUC is set close to zero), and assorted dml is
likely to mean that there are still non-freezable xids on the page
(meaning that we still need to write it again, anyway).

Regards,
Jeff Davis

On Sun, 2009-08-16 at 18:32 -0700, Jeff Davis wrote:
> If VACUUM freezes one tuple on a page, it's likely that there are others
> on the same page that are close to vacuum_freeze_min_age, but not quite.
> Because the page is already dirty from freezing one tuple, it makes
> sense to be more aggressive about freezing the rest, in the hope that
> all the tuples will be frozen, and we will not have to dirty the page
> again later.

In the old days, where all new tuples were put at the end, this would
have made a lot of sense. But nowadays, with fillfacter, HOT, and so
on, it's quite likely that all the stuff around an outdated tuple are
newer versions of the same tuple or newer versions of other tuples close
by.

The patch might make sense anyway, but I think it might not be such an
overwhelming winner in practice.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> The patch might make sense anyway, but I think it might not be such an
> overwhelming winner in practice.

As always with patches that are meant to improve performance,
some experimental evidence would be a good thing.

regards, tom lane

On Mon, 2009-08-17 at 10:22 -0400, Tom Lane wrote:
> As always with patches that are meant to improve performance,
> some experimental evidence would be a good thing.

I haven't had time to performance test this patch yet, and it looks like
it will take a significant amount of effort to do so. I'm focusing on my
other work, so I don't know if this one is going to be in shape for the
September commitfest.

If someone is interested in doing some performance testing for this
patch, let me know. I still think it has potential.

Regards,
Jeff Davis

On Mon, Sep 14, 2009 at 2:07 AM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:

> On Mon, 2009-08-17 at 10:22 -0400, Tom Lane wrote:
> > As always with patches that are meant to improve performance,
> > some experimental evidence would be a good thing.
>
> I haven't had time to performance test this patch yet, and it looks like
> it will take a significant amount of effort to do so. I'm focusing on my
> other work, so I don't know if this one is going to be in shape for the
> September commitfest.
>
> If someone is interested in doing some performance testing for this
> patch, let me know. I still think it has potential.
>

Under what kind of circumstances/workload to you think this patch is most
likely to show its full potential? I can try to test it out, but would like
some guidance. I am guessing it is when the anti-wrap around vacuums come
due, but that is such a rare event, it could both be hard to test for and
also be of limited real-world applicability.

Cheers,

Jeff (Janes)

On Tue, 2009-09-15 at 20:56 -0700, Jeff Janes wrote:
> Under what kind of circumstances/workload to you think this patch is
> most likely to show its full potential? I can try to test it out, but
> would like some guidance. I am guessing it is when the anti-wrap
> around vacuums come due, but that is such a rare event, it could both
> be hard to test for and also be of limited real-world applicability.

I would expect the benefit to come when tuples start to reach
vacuum_freeze_min_age, and the vacuums start freezing them. Without the
patch, I expect that, on average, vacuum will freeze the same page
multiple times even if the tuples are all quite old during the first
round of freezing.

So this would really only be a problem in a long steady state with a
high volume of transactions. The patch will hopefully reduce the write
volume going on in the background.

I expect the biggest benefit comes when the tuples on a given page may
have been inserted/updated over a few million transactions. Under normal
circumstances, it won't be a huge win, but it's not a huge patch,
either ;)

Regards,
Jeff Davis

On Mon, Sep 14, 2009 at 2:07 AM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:

> On Mon, 2009-08-17 at 10:22 -0400, Tom Lane wrote:
> > As always with patches that are meant to improve performance,
> > some experimental evidence would be a good thing.
>
> I haven't had time to performance test this patch yet, and it looks like
> it will take a significant amount of effort to do so. I'm focusing on my
> other work, so I don't know if this one is going to be in shape for the
> September commitfest.
>
> If someone is interested in doing some performance testing for this
> patch, let me know. I still think it has potential.
>
>
Does the community think that experimental performance testing is a
must-have in order for this patch to be acceptable? If so, it sounds like
this should be marked as rejected or RwF, and no longer considered for this
commit fest, and I should move on to a different patch.

I'll do some work on benchmarking it, but since it takes hundreds of
millions of transactions to make a plausible scenario, this will not be done
any time soon.

Also, I see that the number of frozen tuples is not logged. I'd like to add
that to the info reported under Log_autovacuum_min_duration, both to help
evaluate this patch and because it seems like something that should be
reported.

Cheers,

Jeff Janes

Jeff Janes <jeff(dot)janes(at)gmail(dot)com> writes:
> Does the community think that experimental performance testing is a
> must-have in order for this patch to be acceptable?

Dunno about others, but I think so. It's complicating both the
implementation and the users-eye view of VACUUM, and I want more than
a hypothesis that we're going to get something useful out of that.

If we can't test it in a reasonable time frame for this commitfest,
then we should move it to the queue for the next one.

regards, tom lane

On Thu, Sep 17, 2009 at 12:24 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Jeff Janes <jeff(dot)janes(at)gmail(dot)com> writes:
>> Does the community think that experimental performance testing is a
>> must-have in order for this patch to be acceptable?
>
> Dunno about others, but I think so.  It's complicating both the
> implementation and the users-eye view of VACUUM, and I want more than
> a hypothesis that we're going to get something useful out of that.
>
> If we can't test it in a reasonable time frame for this commitfest,
> then we should move it to the queue for the next one.

Despite my recent screw-up in this department, it should really be the
patch author's responsibility to test the patch first. Then the
reviewing process can involve additional testing. So I would say this
should be moved to Returned With Feedback, and then it can be
resubmitted later with test results.

The problem with bumping things to the next CommitFest is that it then
becomes the CommitFest management team's problem to sort out which
patches were bumped but the necessary to-do items weren't completed,
versus being the patch author's problem to let us know when they have
completed the necessary to-do items.

So I am in favor of a policy that things should only be moved to the
next CommitFest when they have ALREADY satisfied the requirements for
being reviewed during that CommitFest.

...Robert

On Thu, 2009-09-17 at 12:36 -0400, Robert Haas wrote:
> Despite my recent screw-up in this department, it should really be the
> patch author's responsibility to test the patch first. Then the
> reviewing process can involve additional testing. So I would say this
> should be moved to Returned With Feedback, and then it can be
> resubmitted later with test results.

Fine with me. I already suspected that this patch wouldn't make it to
the September commitfest:

http://archives.postgresql.org/pgsql-hackers/2009-09/msg00798.php

Regards,
Jeff Davis

I assume no progress has been made on testing the performance of this
patch.

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

Jeff Davis wrote:
> Attached is a patch to implement the idea discussed here:
>
> http://archives.postgresql.org/pgsql-hackers/2009-08/msg01137.php
>
> If VACUUM freezes one tuple on a page, it's likely that there are others
> on the same page that are close to vacuum_freeze_min_age, but not quite.
> Because the page is already dirty from freezing one tuple, it makes
> sense to be more aggressive about freezing the rest, in the hope that
> all the tuples will be frozen, and we will not have to dirty the page
> again later.
>
> This patch introduces a GUC vacuum_freeze_opportunistic_ratio. If one
> tuple on a page is frozen by vacuum, it effectively multiplies
> vacuum_freeze_min_age by vacuum_freeze_opportunistic_ratio and uses that
> lower (more aggressive) value only for the current page.
>
> The reason we don't just freeze all the tuples we can (effectively
> setting the vacuum_freeze_opportunistic_ratio to zero) is to preserve
> transaction ID information for diagnosing problems.
>
> Regards,
> Jeff Davis

[ 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
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 Tue, 2010-02-23 at 17:49 -0500, Bruce Momjian wrote:
> I assume no progress has been made on testing the performance of this
> patch.
>

That's correct. As of right now, the potential benefits of the patch do
not seem to justify the performance testing effort.

Others are welcome to try, of course.

Regards,
Jeff Davis