Re: lazy vxid locks, v2

Here is an updated version of the "lazy vxid locks" patch [1], which
applies over the latest "reduce the overhead of frequent table
locks"[2] patch.

[1] https://commitfest.postgresql.org/action/patch_view?id=585
[2] https://commitfest.postgresql.org/action/patch_view?id=572

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jul 5, 2011 at 1:13 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Here is an updated version of the "lazy vxid locks" patch [1], which
> applies over the latest "reduce the overhead of frequent table
> locks"[2] patch.
>
> [1] https://commitfest.postgresql.org/action/patch_view?id=585
> [2] https://commitfest.postgresql.org/action/patch_view?id=572

And then I forgot the attachment.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, 2011-07-05 at 13:15 -0400, Robert Haas wrote:
> On Tue, Jul 5, 2011 at 1:13 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > Here is an updated version of the "lazy vxid locks" patch [1], which
> > applies over the latest "reduce the overhead of frequent table
> > locks"[2] patch.
> >
> > [1] https://commitfest.postgresql.org/action/patch_view?id=585
> > [2] https://commitfest.postgresql.org/action/patch_view?id=572
>
> And then I forgot the attachment.

The patch looks good, and I like the concept.

My only real comment is one that you already made: the
BackendIdGetProc() mechanism is not "awesome". However, that seems like
material for a separate patch, if at all.

Big disclaimer: I did not do any performance review, despite the fact
that this is a performance patch.

I see that there are some active performance concerns around this patch,
specifically that it may cause an increase in spinlock contention:

http://archives.postgresql.org/message-id/BANLkTikp4EGbfw9xDx9bQ_vK8DQa11WbPg@mail.gmail.com

Fortunately, there's a subsequent discussion that shows a lot of
promise:

http://archives.postgresql.org/pgsql-hackers/2011-07/msg00293.php

I'll mark this "waiting on author" pending the results of that
discussion.

I like the approach you're taking with this series of patches, so
perhaps we shouldn't set the bar so high that you have to remove all of
the bottlenecks before making any progress. Then again, maybe there's
not a huge cost to leaving these patches on the shelf until we're sure
that they lead somewhere.

Regards,
Jeff Davis

Robert,

I should be able to do some performance testing on this, but not today.

Questions:

(1) can you re-link me to the pgbench and sysbench setup you used to
test this originally? I'd like to implement those.

(2) the max machine I can test these on is 16 cores. Is that adequate,
or do we need more cores for real testing?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Fri, Jul 15, 2011 at 4:41 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> (1) can you re-link me to the pgbench and sysbench setup you used to
> test this originally?  I'd like to implement those.

I didn't use sysbench. The pgbench command I used was something like:

pgbench -n -S -T $TIME -c $CLIENTS -j $CLIENTS

...with varying times and client counts. Three minutes or so seems to
be enough to get a reasonably good measurement. We have a report from
another tester that -M prepared exacerbates the effect of the main
fastlock patch (which I just committed) so that might be worth trying
here, too.

> (2) the max machine I can test these on is 16 cores.  Is that adequate,
> or do we need more cores for real testing?

I think you should be able to see a small benefit on a machine of that
size. My memory is a bit fuzzy because it's 1 AM and I haven't looked
at this in over a week, but I believe that this patch produces only a
small improvement in performance by itself, apparently because most of
the gain that we get from reducing lock manager contention gets eaten
up by additional spinlock contention. However, I think the change is
worthwhile anyway, because the benefit of any changes we might make to
reduce spinlock contention is going to be limited by the rate at which
we can shove traffic through the lock manager. In other words, aside
from whatever benefit this patch may have on its own, I believe it's
an enabler for future performance work.

In mulling over this patch, it's occurred to me that there are some
stylistic things about it that can stand to be improved. So I'm going
to do that for the next version. However, if you want to test on this
version, feel free, because I think those changes are going to have
little if any effect on performance.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company