Thread: Should we cacheline align PGXACT?

On Fri, Aug 19, 2016 at 11:42 AM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
> Hackers,
>
> originally this idea was proposed by Andres Freund while experimenting with
> lockfree Pin/UnpinBuffer [1].
> The patch is attached as well as results of pgbench -S on 72-cores machine.
> As before it shows huge benefit in this case.
> For sure, we should validate that it doesn't cause performance regression in
> other cases.  At least we should test read-write and smaller machines.
> Any other ideas?
>

may be test on Power m/c as well.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> originally this idea was proposed by Andres Freund while experimenting with
> lockfree Pin/UnpinBuffer [1].
> The patch is attached as well as results of pgbench -S on 72-cores
> machine.  As before it shows huge benefit in this case.

That's one mighty ugly patch.  Can't you do it without needing to
introduce the additional layer of struct nesting?
        regards, tom lane

Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> On Fri, Aug 19, 2016 at 4:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> That's one mighty ugly patch.  Can't you do it without needing to
>> introduce the additional layer of struct nesting?

> That's worrying me too.
> We could use anonymous struct, but it seems to be prohibited in C89 which
> we stick to.
> Another idea, which comes to my mind, is to manually calculate size of
> padding and insert it directly to PGXACT struct.  But that seems rather
> ugly too.  However, it would be ugly definition not ugly usage...
> Do you have better ideas?

No, that was the best one that had occurred to me, too.  You could
probably introduce a StaticAssert that sizeof(PGXACT) is a power of 2
as a means of checking that the manual padding calculation hadn't
gotten broken.
        regards, tom lane

On Fri, Aug 19, 2016 at 8:24 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
> On Fri, Aug 19, 2016 at 3:19 PM, Amit Kapila <amit.kapila16@gmail.com>
> wrote:
>>
>> On Fri, Aug 19, 2016 at 11:42 AM, Alexander Korotkov
>> <a.korotkov@postgrespro.ru> wrote:
>> > Hackers,
>> >
>> > originally this idea was proposed by Andres Freund while experimenting
>> > with
>> > lockfree Pin/UnpinBuffer [1].
>> > The patch is attached as well as results of pgbench -S on 72-cores
>> > machine.
>> > As before it shows huge benefit in this case.
>> > For sure, we should validate that it doesn't cause performance
>> > regression in
>> > other cases.  At least we should test read-write and smaller machines.
>> > Any other ideas?
>> >
>>
>> may be test on Power m/c as well.
>
>
> Good idea.  I don't have any such machine at hand now.  Do you have one?
>

Yes, I can help in testing this patch during CF.  Feel free to ping,
if I missed or forgot to do same.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Aug 19, 2016, at 2:12 AM, Alexander Korotkov <a.korotkov@postgrespro.ru> wrote:
> Hackers,
>
> originally this idea was proposed by Andres Freund while experimenting with lockfree Pin/UnpinBuffer [1].
> The patch is attached as well as results of pgbench -S on 72-cores machine.  As before it shows huge benefit in this
case.
> For sure, we should validate that it doesn't cause performance regression in other cases.  At least we should test
read-writeand smaller machines. 
> Any other ideas?

Wow, nice results.  My intuition on why PGXACT helped in the first place was that it minimized the number of cache
linesthat had to be touched to take a snapshot. Padding obviously would somewhat increase that again, so I can't quite
understandwhy it seems to be helping... any idea? 

...Robert

Robert Haas <robertmhaas@gmail.com> writes:
> Wow, nice results.  My intuition on why PGXACT helped in the first place was that it minimized the number of cache
linesthat had to be touched to take a snapshot. Padding obviously would somewhat increase that again, so I can't quite
understandwhy it seems to be helping... any idea?
 

That's an interesting point.  I wonder whether this whole thing will be
useless or even counterproductive after (if) Heikki's CSN-snapshot patch
gets in.  I would certainly not mind reverting the PGXACT/PGPROC
separation if it proves no longer helpful after that.
        regards, tom lane

On 2016-08-20 14:33:13 -0400, Robert Haas wrote:
> On Aug 19, 2016, at 2:12 AM, Alexander Korotkov <a.korotkov@postgrespro.ru> wrote:
> > Hackers,
> > 
> > originally this idea was proposed by Andres Freund while experimenting with lockfree Pin/UnpinBuffer [1].
> > The patch is attached as well as results of pgbench -S on 72-cores machine.  As before it shows huge benefit in
thiscase.
 
> > For sure, we should validate that it doesn't cause performance regression in other cases.  At least we should test
read-writeand smaller machines.
 
> > Any other ideas?
> 
> Wow, nice results.  My intuition on why PGXACT helped in the first
> place was that it minimized the number of cache lines that had to be
> touched to take a snapshot. Padding obviously would somewhat increase
> that again, so I can't quite understand why it seems to be
> helping... any idea?

I don't think it's that surprising: PGXACT->xid is written to each
transaction, and ->xmin is often written to multiple times per
transaction. That means that if a PGXACT's cacheline is shared between
backends one write will often first have another CPU flush it's store
buffer / L1 / L2 cache. If there's several hops between two cores, that
can mean quite a bit of added latency.  I previously played around with
*removing* the optimization of resetting ->xmin when not required
anymore - and on a bigger machine it noticeably increased throughput on
higher client counts.

To me it's pretty clear that rounding up PGXACT's size to a 16 bytes
(instead of the current 12, with 4 byte alignment) is going to be a win,
the current approach just leeds to pointless sharing.  Besides, storing
the database oid in there will allow GetOldestXmin() to only use PGXACT,
and could, with a bit more work, allow to ignore other databases in
GetSnapshotData().

I'm less sure that going up to a full cacheline is always a win.

Andres

On 2016-08-19 09:46:12 -0400, Tom Lane wrote:
> Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> > originally this idea was proposed by Andres Freund while experimenting with
> > lockfree Pin/UnpinBuffer [1].
> > The patch is attached as well as results of pgbench -S on 72-cores
> > machine.  As before it shows huge benefit in this case.
> 
> That's one mighty ugly patch.

My version of it was only intended to nail down some variability on the
pgpro machine, it wasn't intended for submission.


> Can't you do it without needing to introduce the additional layer of
> struct nesting?

If we required support for anonymous unions, such things would be a lot
easier to do.  That aside, the only alternative seems tob e hard-coding
padding space - which probably isn't all that un-fragile either.

On 2016-08-22 11:25:55 +0800, Craig Ringer wrote:
> On 22 August 2016 at 10:40, Andres Freund <andres@anarazel.de> wrote:
> 
> > On 2016-08-19 09:46:12 -0400, Tom Lane wrote:
> > > Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> > > > originally this idea was proposed by Andres Freund while experimenting
> > with
> > > > lockfree Pin/UnpinBuffer [1].
> > > > The patch is attached as well as results of pgbench -S on 72-cores
> > > > machine.  As before it shows huge benefit in this case.
> > >
> > > That's one mighty ugly patch.
> >
> > My version of it was only intended to nail down some variability on the
> > pgpro machine, it wasn't intended for submission.
> >
> >
> > > Can't you do it without needing to introduce the additional layer of
> > > struct nesting?
> >
> > If we required support for anonymous unions, such things would be a lot
> > easier to do.  That aside, the only alternative seems tob e hard-coding
> > padding space - which probably isn't all that un-fragile either.
> > <http://www.postgresql.org/mailpref/pgsql-hackers>
> >
> 
> Somewhat naïve question from someone with much less clue about low level
> cache behaviour trying to follow along: given that we determine such
> padding at compile time, how do we ensure that the cacheline size we're
> targeting is right at runtime?

There's basically only very few common cacheline sizes. Pretty much only
64 byte and 128 bytes are common these days. By usually padding to the
larger of those two, we waste a bit of memory, but not actually cache
space on platforms with smaller lines, because the padding is never
accessed.

> Or is it safe to assume that using 16 bytes so we don't cross cache
> line boundaries is always helpful, whether we have 4 PGXACT entries
> (64 byte line) or some other number per cacheline?

That's generally a good thing to do, yes.  It's probably not going to
give the full benefits here though.

Regards

Andres

Hi,

As discussed at the Developer meeting ~ a week ago, I've ran a number of 
benchmarks on the commit, on a small/medium-size x86 machines. I 
currently don't have access to a machine as big as used by Alexander 
(with 72 physical cores), but it seems useful to verify the patch does 
not have negative impact on smaller machines.

In particular I've ran these tests:

* r/o pgbench
* r/w pgbench
* 90% reads, 10% writes
* pgbench with skewed distribution
* pgbench with skewed distribution and skipping

And each of that with a number of clients, depending on the number of 
cores available. I've used the usual two boxes I use for all benchmarks, 
i.e. a small i5-2500k machine (8GB RAM, 4 cores), and a medium e5-2620v4 
box (32GB RAM, 16/32 cores).

Comparing averages of tps, measured on 5 runs (each 5 minutes long), the 
difference between master and patched master is usually within 2%, which 
is pretty much within noise.

I'm attaching spreadsheets with summary of the results, so that we have 
it in the archives. As usual, the scripts and much more detailed results 
are available here:

* e5-2620: https://bitbucket.org/tvondra/test-xact-alignment
* i5-2500k: https://bitbucket.org/tvondra/test-xact-alignment-i5

I do plan to run these results on the Power8 box I have access to, but 
that will have to wait for a bit, because it's currently doing something 
else.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 02/11/2017 02:44 AM, Ashutosh Sharma wrote:
> Hi,
>
> I am currently testing this patch on a large machine and will share the
> test results in few days of time.
>

FWIW it might be interesting to have comparable results from the same 
benchmarks I did. The scripts available in the git repositories should 
not be that hard to tweak. Let me know if you're interested and need 
help with that.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 02/11/2017 01:21 PM, Alexander Korotkov wrote:
> Hi, Tomas!
>
> On Sat, Feb 11, 2017 at 2:28 AM, Tomas Vondra
> <tomas.vondra@2ndquadrant.com <mailto:tomas.vondra@2ndquadrant.com>> wrote:
>
>     As discussed at the Developer meeting ~ a week ago, I've ran a
>     number of benchmarks on the commit, on a small/medium-size x86
>     machines. I currently don't have access to a machine as big as used
>     by Alexander (with 72 physical cores), but it seems useful to verify
>     the patch does not have negative impact on smaller machines.
>
>     In particular I've ran these tests:
>
>     * r/o pgbench
>     * r/w pgbench
>     * 90% reads, 10% writes
>     * pgbench with skewed distribution
>     * pgbench with skewed distribution and skipping
>
>
> Thank you very much for your efforts!
> I took a look at these tests.  One thing catch my eyes.  You warmup
> database using pgbench run.  Did you consider using pg_prewarm instead?
>
> SELECT sum(x.x) FROM (SELECT pg_prewarm(oid) AS x FROM pg_class WHERE
> relkind IN ('i', 'r') ORDER BY oid) x;
>
> In my experience pg_prewarm both takes less time and leaves less
> variation afterwards.
>

I've considered it, but the problem I see in using pg_prewarm for 
benchmarking purposes is that it only loads the data into memory, but it 
does not modify the tuples (so all tuples have the same xmin/xmax, no 
dead tuples, ...), it does not set usage counters on the buffers and 
also does not generate any clog records.

I don't think there's a lot of variability in the results I measured. If 
you look at (max-min) for each combination of parameters, the delta is 
generally within 2% of average, with a very few exceptions, usually 
caused by the first run (so perhaps the warmup should be a bit longer).

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> FWIW it might be interesting to have comparable results from the same
> benchmarks I did. The scripts available in the git repositories should not
> be that hard to tweak. Let me know if you're interested and need help with
> that.
>

Sure, I will have a look into those scripts once I am done with the
simple pgbench testing. Thanks and sorry for the delayed response.

On 8/22/16 12:24 AM, Andres Freund wrote:
>> Somewhat naïve question from someone with much less clue about low level
>> cache behaviour trying to follow along: given that we determine such
>> padding at compile time, how do we ensure that the cacheline size we're
>> targeting is right at runtime?
> There's basically only very few common cacheline sizes. Pretty much only
> 64 byte and 128 bytes are common these days. By usually padding to the
> larger of those two, we waste a bit of memory, but not actually cache
> space on platforms with smaller lines, because the padding is never
> accessed.

Though, with an N-way associative cache 2x more padding than necessary 
cuts the amount you can fit into the cache by half. That could be 
meaningful in some cases.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)

Am Samstag, den 11.02.2017, 00:28 +0100 schrieb Tomas Vondra:
> Comparing averages of tps, measured on 5 runs (each 5 minutes long),
> the 
> difference between master and patched master is usually within 2%,
> which 
> is pretty much within noise.
> 
> I'm attaching spreadsheets with summary of the results, so that we
> have 
> it in the archives. As usual, the scripts and much more detailed
> results 
> are available here:

I've done some benchmarking of this patch against the E850/ppc64el
Ubuntu LPAR we currently have access to and got the attached results.
pg_prewarm as recommended by Alexander was used, the tests run 300s
secs, scale 1000, each with a testrun before. The SELECT-only pgbench
was run twice each, the write tests only once.

Looks like the influence of this patch isn't that big, at least on this
machine.

We're going to reassign the resources to an AIX LPAR soon, which
doesn't give me enough time to test with Tomas' test scripts again.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alexander Korotkov wrote:

> Yes, influence seems to be low.  But nevertheless it's important to insure
> that there is no regression here.
> Despite pg_prewarm'ing and running tests 300s, there is still significant
> variation.
> For instance, with clients count = 80:
>  * pgxact-result-2.txt – 474704
>  * pgxact-results.txt – 574844
> Could some background processes influence the tests?  Or could it be
> another virtual machine?
> Also, I wonder why I can't see this variation on the graphs.
> Another issue with graphs is that we can't see details of read and write
> TPS variation on the same scale, because write TPS values are too low.  I
> think you should draw write benchmark on the separate graph.

So, I'm reading that on PPC64 there is no effect, and on the "lesser"
machine Tomas tested on, there is no effect either; this patch only
seems to benefit Alexander's 72 core x86_64 machine.

It seems to me that Andres comments here were largely ignored:
https://www.postgresql.org/message-id/20160822021747.u5bqx2xwwjzac5u5@alap3.anarazel.de
He was suggesting to increase the struct size to 16 bytes rather than
going all the way up to 128.  Did anybody test this?

Re the coding of the padding computation, seems it'd be better to use
our standard "offsetof(last-struct-member) + sizeof(last-struct-member)"
rather than adding each of the members' sizes individually.

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

Am Montag, den 13.02.2017, 16:55 +0300 schrieb Alexander Korotkov:
> 
> Thank you for testing.
> 
> Yes, influence seems to be low.  But nevertheless it's important to
> insure
> that there is no regression here.
> Despite pg_prewarm'ing and running tests 300s, there is still
> significant
> variation.
> For instance, with clients count = 80:
>  * pgxact-result-2.txt – 474704
>  * pgxact-results.txt – 574844

> Could some background processes influence the tests?  Or could it be
> another virtual machine?
> Also, I wonder why I can't see this variation on the graphs.
> Another issue with graphs is that we can't see details of read and
> write

Whoops, good catch. I've mistakenly copied the wrong y-axis for these
results in the gnuplot script, shame on me. New plots attached.

You're right, the 2nd run with the pgxact alignment patch is notable.
I've realized that there was a pgbouncer instance running from a
previous test, but not sure if that could explain the difference.

> TPS variation on the same scale, because write TPS values are too
> low.  I
> think you should draw write benchmark on the separate graph.
> 

The Linux LPAR is the only one used atm. We got some more time for
Linux now and i'm going to prepare Tomas' script to run. Not sure i can
get to it today, though.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi All,

I too have performed benchmarking of this patch on a large machine
(with 128 CPU(s), 520GB RAM, intel x86-64 architecture) and would like
to share my observations for the same (Please note that, as I had to
reverify readings on few client counts, it did take some time for me
to share these test-results.)

Case1: Data fits in shared buffer, Read Only workload:
-------------------------------------------------------------------------------
For data fits in shared buffer, I have taken readings at 300 SF. The
result sheet 'results-readonly-300-1000-SF' containing the median of 3
runs is attached with this mail. In this case, I could see very good
performance improvement with the patch basically at high client counts
(156 - 256).

Case2: Data doesn't fit in shared buffer, Read Only workload:
--------------------------------------------------------------------------------------
For data doesn't fit in shared buffer, I have taken readings at 1000
SF. The result sheet 'results-readonly-300-1000-SF' is attached with
this mail. In this case, the performance improvement is not as in
Case1, Infact it just varies in the range of 2-7%. but the good thing
is that there is no regression.

Case3: Data fits in shared buffer, Read-write workload:
-----------------------------------------------------------------------------
In this case, I could see that the tps on head and patch are very
close to each other with a small variation of (+-)3-4% which i assume
is a run-to-run variation. PFA result sheet
'results-readwrite-300-1000-SF' containing the test-results.

Case4: Data doesn't fit in shared buffer, Read-write workload:
----------------------------------------------------------------------------------------
In this case as well, tps on head and patch are very close to each
other with small variation of 1-2% which again is a run-to-run
variation. PFA result sheet 'results-readwrite-300-1000-SF' containing
the test-results.

Please note that details on the non-default guc params and pgbench is
all provided in the result sheets attached with this mail. Also, I
have not used pg_prewarm in my test script. Thank you.

On Mon, Feb 13, 2017 at 9:43 PM, Bernd Helmle <mailings@oopsware.de> wrote:
> Am Montag, den 13.02.2017, 16:55 +0300 schrieb Alexander Korotkov:
>>
>> Thank you for testing.
>>
>> Yes, influence seems to be low.  But nevertheless it's important to
>> insure
>> that there is no regression here.
>> Despite pg_prewarm'ing and running tests 300s, there is still
>> significant
>> variation.
>> For instance, with clients count = 80:
>>  * pgxact-result-2.txt – 474704
>>  * pgxact-results.txt – 574844
>
>> Could some background processes influence the tests?  Or could it be
>> another virtual machine?
>> Also, I wonder why I can't see this variation on the graphs.
>> Another issue with graphs is that we can't see details of read and
>> write
>
> Whoops, good catch. I've mistakenly copied the wrong y-axis for these
> results in the gnuplot script, shame on me. New plots attached.
>
> You're right, the 2nd run with the pgxact alignment patch is notable.
> I've realized that there was a pgbouncer instance running from a
> previous test, but not sure if that could explain the difference.
>
>> TPS variation on the same scale, because write TPS values are too
>> low.  I
>> think you should draw write benchmark on the separate graph.
>>
>
> The Linux LPAR is the only one used atm. We got some more time for
> Linux now and i'm going to prepare Tomas' script to run. Not sure i can
> get to it today, though.
>
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alexander Korotkov wrote:
> On Mon, Feb 13, 2017 at 7:07 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
> wrote:

> > Re the coding of the padding computation, seems it'd be better to use
> > our standard "offsetof(last-struct-member) + sizeof(last-struct-member)"
> > rather than adding each of the members' sizes individually.
> 
> It was done so in order to evade extra level of nesting for PGXACT.  See
> discussion with Tom Lane in [1] and upthread.

Yes, I understand.  I just mean that it could be done something like
this:

#define PGXACTPadSize (PG_CACHE_LINE_SIZE - (offsetof(PGXACT, nxid) + sizeof(uint8)))

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

Alexander Korotkov wrote:
> On Tue, Feb 14, 2017 at 3:57 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
> wrote:

> > Yes, I understand.  I just mean that it could be done something like
> > this:
> >
> > #define PGXACTPadSize (PG_CACHE_LINE_SIZE - (offsetof(PGXACT, nxid) +
> > sizeof(uint8)))
> 
> Yes, but I can't use such macro in the definition of PGXACT itself.

/me slaps forehead

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

On Mon, Feb 13, 2017 at 11:07 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> It seems to me that Andres comments here were largely ignored:
> https://www.postgresql.org/message-id/20160822021747.u5bqx2xwwjzac5u5@alap3.anarazel.de
> He was suggesting to increase the struct size to 16 bytes rather than
> going all the way up to 128.  Did anybody test this?

So, I think that going up to 128 bytes can't really make sense.  If
that's the best-performing solution here, then maybe what we ought to
be doing is reverting the PGXACT/PGPROC separation, sticking these
critical members at the beginning, and padding the whole PGXACT out to
a multiple of the cache line size.  Something that only touches the
PGXACT fields will touch the exact same number of cache lines with
that design, but cases that touch more than just the PGXACT fields
should perform better.  The point of moving PGXACT into a separate
structure was to reduce the number of cache lines required to build a
snapshot.  If that's going to go back up to 1 per PGPROC anyway, we
might as well at least use the rest of that cache line to store
something else that might be useful instead of pad bytes.  I think,
anyway.

If padding to 16 bytes is sufficient to get the performance
improvement (or as much as we care about), that seems more reasonable.
But it's still possible we may be gaining on one hand (because now
you'll never touch multiple cache lines for your own PGXACT) and
losing on the other (because now you're scanning 4/3 as many cache
lines to construct a snapshot).  Which of those is better might be
workload-dependent or hardware-dependent.

> Re the coding of the padding computation, seems it'd be better to use
> our standard "offsetof(last-struct-member) + sizeof(last-struct-member)"
> rather than adding each of the members' sizes individually.

I think our now-standard way of doing this is to have a "Padded"
version that is a union between the unpadded struct and char[].  c.f.
BufferDescPadded, LWLockPadded.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Feb 13, 2017 at 11:07 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>> It seems to me that Andres comments here were largely ignored:
>> https://www.postgresql.org/message-id/20160822021747.u5bqx2xwwjzac5u5@alap3.anarazel.de
>> He was suggesting to increase the struct size to 16 bytes rather than
>> going all the way up to 128.  Did anybody test this?

> So, I think that going up to 128 bytes can't really make sense.  If
> that's the best-performing solution here, then maybe what we ought to
> be doing is reverting the PGXACT/PGPROC separation, sticking these
> critical members at the beginning, and padding the whole PGXACT out to
> a multiple of the cache line size.

Yes.  That separation was never more than a horribly ugly kluge.
I would love to see it go away.  But keeping it *and* padding
PGXACT to something >= the size of PGPROC borders on insanity.
        regards, tom lane

On Wed, Feb 15, 2017 at 12:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Mon, Feb 13, 2017 at 11:07 AM, Alvaro Herrera
>> <alvherre@2ndquadrant.com> wrote:
>>> It seems to me that Andres comments here were largely ignored:
>>> https://www.postgresql.org/message-id/20160822021747.u5bqx2xwwjzac5u5@alap3.anarazel.de
>>> He was suggesting to increase the struct size to 16 bytes rather than
>>> going all the way up to 128.  Did anybody test this?
>
>> So, I think that going up to 128 bytes can't really make sense.  If
>> that's the best-performing solution here, then maybe what we ought to
>> be doing is reverting the PGXACT/PGPROC separation, sticking these
>> critical members at the beginning, and padding the whole PGXACT out to
>> a multiple of the cache line size.
>
> Yes.  That separation was never more than a horribly ugly kluge.
> I would love to see it go away.  But keeping it *and* padding
> PGXACT to something >= the size of PGPROC borders on insanity.

I don't think it would be bigger than a PGPROC.  PGPROCs are really
big, 816 bytes on my MacBook Pro.  But if you did what I suggested,
you could take a snapshot by touching 1 cache line per backend.  They
wouldn't be consecutive; it would be an upward pattern, with skips.
If you pad PGXACT out to one cache line, you could likewise take a
snapshot by touching 1 cache line per backend, and they'd be
consecutive.  Maybe that difference matters to the memory prefetching
controller, I dunno, but it seems funny that we did the PGXACT work to
reduce the number of cache lines that had to be touched in order to
take a snapshot to improve performance, and now we're talking about
increasing it again, also to improve performance.

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

Robert Haas <robertmhaas@gmail.com> writes:
> ... Maybe that difference matters to the memory prefetching
> controller, I dunno, but it seems funny that we did the PGXACT work to
> reduce the number of cache lines that had to be touched in order to
> take a snapshot to improve performance, and now we're talking about
> increasing it again, also to improve performance.

Yes.  I was skeptical that the original change was adequately proven
to be a good idea, and I'm even more skeptical this time.  I think
every single number that's been reported about this is completely
machine-specific, and likely workload-specific too, and should not
be taken as a reason to do anything.

My druthers at this point would be to revert the separation on code
cleanliness grounds and call it a day, more or less independently of any
claims about performance.  I'd be willing to talk about padding PGPROC
to some reasonable stride, but I remain dubious that any changes of
that sort would have a half-life worth complicating the code for.
        regards, tom lane

Alexander Korotkov wrote:

> Difference between master, pgxact-align-2 and pgxact-align-3 doesn't exceed
> per run variation.

FWIW this would be more visible if you added error bars to each data
point.  Should be simple enough in gnuplot ...

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

On Wed, Feb 15, 2017 at 12:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> ... Maybe that difference matters to the memory prefetching
>> controller, I dunno, but it seems funny that we did the PGXACT work to
>> reduce the number of cache lines that had to be touched in order to
>> take a snapshot to improve performance, and now we're talking about
>> increasing it again, also to improve performance.
>
> Yes.  I was skeptical that the original change was adequately proven
> to be a good idea, and I'm even more skeptical this time.  I think
> every single number that's been reported about this is completely
> machine-specific, and likely workload-specific too, and should not
> be taken as a reason to do anything.

The original change definitely worked on read-only pgbench workloads
on both x86 and Itanium, and the gains were pretty significant at
higher client counts.  I don't know whether we tested POWER.
Read-only pgbench throughput is not the world, of course, but it's a
reasonable proxy for high-concurrency, read-mostly workloads involving
short transactions, so it's not nothing, either.

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

Hi,

On 2017-02-15 11:43:17 -0500, Robert Haas wrote:
> On Mon, Feb 13, 2017 at 11:07 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
> > It seems to me that Andres comments here were largely ignored:
> > https://www.postgresql.org/message-id/20160822021747.u5bqx2xwwjzac5u5@alap3.anarazel.de
> > He was suggesting to increase the struct size to 16 bytes rather than
> > going all the way up to 128.  Did anybody test this?
> 
> So, I think that going up to 128 bytes can't really make sense.  If
> that's the best-performing solution here, then maybe what we ought to
> be doing is reverting the PGXACT/PGPROC separation, sticking these
> critical members at the beginning, and padding the whole PGXACT out to
> a multiple of the cache line size.  Something that only touches the
> PGXACT fields will touch the exact same number of cache lines with
> that design, but cases that touch more than just the PGXACT fields
> should perform better.

I don't think that's true for several reasons.  Separating out PGXACT
didn't just mean reducing the stride size of the access / preventing
sharing. It also meant that frequently changing fields in PGPROC aren't
on the same cache-line as fields in PGXACT.  That makes quite a
difference, because with the split a lot of the cachelines "backing"
PGPROC can stay in 'shared' mode in several CPU caches, while
modifications to PGPROC largely can stay in 'exclusive' mode locally on
the CPU the backend is currently running on.  I think I previously
mentioned, even just removing the MyPgXact->xmin assignment in
SnapshotResetXmin() is measurable performance wise and cache-hit ratio
wise.

While Nasby is right that going to multiple of the actual line size can
cause problems due to the N-way associativity of caches, it's not like
unused padding memory >= cacheline-size actually causes cache to be
wasted; it just gets used for other things.


FWIW, I however think we should just set the cacheline size based on the
architecture, rather than defaulting to 128 - it's not like that
actually changes particularly frequently.  I doubt a special-case for
each of x86, arm, power, itanium, and the rest will cause us a lot of
maintenance issues.


> The point of moving PGXACT into a separate
> structure was to reduce the number of cache lines required to build a
> snapshot.  If that's going to go back up to 1 per PGPROC anyway, we
> might as well at least use the rest of that cache line to store
> something else that might be useful instead of pad bytes.  I think,
> anyway.

I don't think that's true, due to the aforementioned dirtyness issue,
which causes inter-node cache coherency traffic (busier bus & latency).

> I think our now-standard way of doing this is to have a "Padded"
> version that is a union between the unpadded struct and char[].  c.f.
> BufferDescPadded, LWLockPadded.

That's what my original POC patch did IIRC, but somebody complained it
changed a lot of accesses.

- Andres

Hi,

On 2017-02-15 12:24:44 -0500, Robert Haas wrote:
> If you pad PGXACT out to one cache line, you could likewise take a
> snapshot by touching 1 cache line per backend, and they'd be
> consecutive.  Maybe that difference matters to the memory prefetching
> controller, I dunno,

Unfortunately it's currently *not* consecutively accessed afaik.  We're
not iterating PGXACT sequentially, we're doing it in pgprocno order.
The reason is that otherwise we have to scan unused PGXACT entries.

I've previously played around with using actual sequential access, and
it was a good bit faster as long as nearly all connections are used -
but slower when only a few are in use, which is quite common...


> but it seems funny that we did the PGXACT work to
> reduce the number of cache lines that had to be touched in order to
> take a snapshot to improve performance, and now we're talking about
> increasing it again, also to improve performance.

I don't think it's that weird that aligning things properly is important
for performance, nor that reducing the amount of memory accessed is
beneficial.  Engineering is tradeoffs, and we might just have erred a
bit too far in this case.

Greetings,

Andres Freund

On Wed, Feb 15, 2017 at 2:15 PM, Andres Freund <andres@anarazel.de> wrote:
> I don't think that's true for several reasons.  Separating out PGXACT
> didn't just mean reducing the stride size of the access / preventing
> sharing. It also meant that frequently changing fields in PGPROC aren't
> on the same cache-line as fields in PGXACT.  That makes quite a
> difference, because with the split a lot of the cachelines "backing"
> PGPROC can stay in 'shared' mode in several CPU caches, while
> modifications to PGPROC largely can stay in 'exclusive' mode locally on
> the CPU the backend is currently running on.  I think I previously
> mentioned, even just removing the MyPgXact->xmin assignment in
> SnapshotResetXmin() is measurable performance wise and cache-hit ratio
> wise.

Oh, hmm.  I didn't think about that angle.

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

On Thu, Feb 16, 2017 at 5:07 AM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
> On Wed, Feb 15, 2017 at 8:49 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
> wrote:
>> Alexander Korotkov wrote:
>>
>> > Difference between master, pgxact-align-2 and pgxact-align-3 doesn't
>> > exceed
>> > per run variation.
>>
>> FWIW this would be more visible if you added error bars to each data
>> point.  Should be simple enough in gnuplot ...
>
> Good point.
> Please find graph of mean and errors in attachment.

So ... no difference?

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

Hi,

>> On Thu, Feb 16, 2017 at 5:07 AM, Alexander Korotkov
>> <a.korotkov@postgrespro.ru> wrote:
>> > On Wed, Feb 15, 2017 at 8:49 PM, Alvaro Herrera
>> > <alvherre@2ndquadrant.com>
>> > wrote:
>> >> Alexander Korotkov wrote:
>> >>
>> >> > Difference between master, pgxact-align-2 and pgxact-align-3 doesn't
>> >> > exceed
>> >> > per run variation.
>> >>
>> >> FWIW this would be more visible if you added error bars to each data
>> >> point.  Should be simple enough in gnuplot ...
>> >
>> > Good point.
>> > Please find graph of mean and errors in attachment.
>>
>> So ... no difference?
>
>
> Yeah, nothing surprising.  It's just another graph based on the same data.
> I wonder how pgxact-align-3 would work on machine of Ashutosh Sharma,
> because I observed regression there in write-heavy benchmark of
> pgxact-align-2.

I am yet to benchmark pgxact-align-3 patch on a read-write workload. I
could not do it as our benchmarking machine was already reserved for
some other test work. But, I am planning to do it on this weekend.
Will try to post my results by Monday evening. Thank you and sorry for
the delayed response.

On 15 February 2017 at 19:15, Andres Freund <andres@anarazel.de> wrote:

> I think I previously
> mentioned, even just removing the MyPgXact->xmin assignment in
> SnapshotResetXmin() is measurable performance wise and cache-hit ratio
> wise.

Currently, we issue SnapshotResetXmin() pointlessly at end of xact, so
patch attached to remove that call, plus some comments to explain
that. This reduces the cause.

Also, another patch to reduce the calls to SnapshotResetXmin() using a
simple heuristic to reduce the effects.

-- 
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Feb 20, 2017 at 6:02 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 15 February 2017 at 19:15, Andres Freund <andres@anarazel.de> wrote:
>
>> I think I previously
>> mentioned, even just removing the MyPgXact->xmin assignment in
>> SnapshotResetXmin() is measurable performance wise and cache-hit ratio
>> wise.
>
> Currently, we issue SnapshotResetXmin() pointlessly at end of xact, so
> patch attached to remove that call, plus some comments to explain
> that. This reduces the cause.
>
> Also, another patch to reduce the calls to SnapshotResetXmin() using a
> simple heuristic to reduce the effects.

I think skip_SnapshotResetXmin_if_idle_timeout.v1.patch isn't a good
idea, because it could have the surprising result that setting
idle_in_transaction_timeout to a non-zero value makes bloat worse.  I
don't think users will like that.

Regarding reduce_pgxact_access_AtEOXact.v1.patch, it took me a few
minutes to figure out that the comment was referring to
ProcArrayEndTransaction(), so it might be good to be more explicit
about that if we go forward with this.  Have you checked whether this
patch makes any noticeable performance difference?  It's sure
surprising that we go to all of this trouble to clean things up in
AtEOXact_Snapshot() when we've already nuked MyPgXact->xmin from
orbit.  (Instead of changing AtEOXact_Snapshot, should we think about
removing the xid clear logic from ProcArrayEndTransaction and only
doing it here, or would that be wrong-headed?)

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

On 20 February 2017 at 16:53, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Feb 20, 2017 at 6:02 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> On 15 February 2017 at 19:15, Andres Freund <andres@anarazel.de> wrote:
>>
>>> I think I previously
>>> mentioned, even just removing the MyPgXact->xmin assignment in
>>> SnapshotResetXmin() is measurable performance wise and cache-hit ratio
>>> wise.
>>
>> Currently, we issue SnapshotResetXmin() pointlessly at end of xact, so
>> patch attached to remove that call, plus some comments to explain
>> that. This reduces the cause.
>>
>> Also, another patch to reduce the calls to SnapshotResetXmin() using a
>> simple heuristic to reduce the effects.
>
> I think skip_SnapshotResetXmin_if_idle_timeout.v1.patch isn't a good
> idea, because it could have the surprising result that setting
> idle_in_transaction_timeout to a non-zero value makes bloat worse.  I
> don't think users will like that.
>
> Regarding reduce_pgxact_access_AtEOXact.v1.patch, it took me a few
> minutes to figure out that the comment was referring to
> ProcArrayEndTransaction(), so it might be good to be more explicit
> about that if we go forward with this.

Sure, attached.

> Have you checked whether this
> patch makes any noticeable performance difference?

No, but then we're reducing the number of calls to PgXact directly;
there is no heuristic involved, its just a pure saving.

> It's sure
> surprising that we go to all of this trouble to clean things up in
> AtEOXact_Snapshot() when we've already nuked MyPgXact->xmin from
> orbit.  (Instead of changing AtEOXact_Snapshot, should we think about
> removing the xid clear logic from ProcArrayEndTransaction and only
> doing it here, or would that be wrong-headed?)

If anything, I'd move the call to PgXact->xmin = InvalidTransactionId
into a function inside procarray.c, so we only touch snapshots in
snapmgr.c and all procarray stuff is isolated. (Not done here, yet).

-- 
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Feb 20, 2017 at 10:49 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> Regarding reduce_pgxact_access_AtEOXact.v1.patch, it took me a few
>> minutes to figure out that the comment was referring to
>> ProcArrayEndTransaction(), so it might be good to be more explicit
>> about that if we go forward with this.
>
> Sure, attached.

Looks better, thanks.

>> Have you checked whether this
>> patch makes any noticeable performance difference?
>
> No, but then we're reducing the number of calls to PgXact directly;
> there is no heuristic involved, its just a pure saving.

Well, it's adding a branch where there wasn't one.  Maybe that costs
essentially nothing and the saved write to shared memory saves
something noticeable, but for all I know it's the reverse.  If I had
to guess, it would be that neither the costs nor the savings from this
are in the slightest way noticeable on a macrobenchmark, and therefore
there's not much point in changing it, but that could be 100% wrong.

>> It's sure
>> surprising that we go to all of this trouble to clean things up in
>> AtEOXact_Snapshot() when we've already nuked MyPgXact->xmin from
>> orbit.  (Instead of changing AtEOXact_Snapshot, should we think about
>> removing the xid clear logic from ProcArrayEndTransaction and only
>> doing it here, or would that be wrong-headed?)
>
> If anything, I'd move the call to PgXact->xmin = InvalidTransactionId
> into a function inside procarray.c, so we only touch snapshots in
> snapmgr.c and all procarray stuff is isolated. (Not done here, yet).

I'm not convinced that really buys us anything except more
function-call overhead.

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

On 20 February 2017 at 17:32, Robert Haas <robertmhaas@gmail.com> wrote:

>>> Have you checked whether this
>>> patch makes any noticeable performance difference?
>>
>> No, but then we're reducing the number of calls to PgXact directly;
>> there is no heuristic involved, its just a pure saving.
>
> Well, it's adding a branch where there wasn't one.

A branch that is avoided in almost all cases, so easy to predict.

> Maybe that costs
> essentially nothing and the saved write to shared memory saves
> something noticeable, but for all I know it's the reverse.  If I had
> to guess, it would be that neither the costs nor the savings from this
> are in the slightest way noticeable on a macrobenchmark, and therefore
> there's not much point in changing it, but that could be 100% wrong.

Given Andres' earlier measurements, it seems worth testing to me.



Hopefully someone can recheck. Thanks in advance.

-- 
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Feb 21, 2017 at 4:21 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
>
> Hi, Ashutosh!
> On Mon, Feb 20, 2017 at 1:20 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>>
>> Following are the pgbench results for read-write workload, I got with pgxact-align-3 patch. The results are for 300
scalefactor with 8GB of shared buffer i.e. when data fits into the shared buffer. For 1000 scale factor with 8GB shared
bufferthe test is still running, once it is completed I will share the results for that as well. 
>>
>> pgbench settings:
>> pgbench -i -s 300 postgres
>> pgbench -M prepared -c $thread -j $thread -T $time_for_reading  postgres
>>
>> where, time_for_reading = 30mins
>>
>> non default GUC param:
>> shared_buffers=8GB
>> max_connections=300
>>
>> pg_xlog is located in SSD.
>
>
> Thank you for testing.
> It seems that there is still regression.  While padding was reduced from 116 bytes to 4 bytes, it makes me think that
probablythere is something wrong in testing methodology. 
> Are you doing re-initdb and pgbench -i before each run?  I would ask you to do the both.
> Also, if regression would still exist, let's change the order of versions.  Thus, if you run master before patched
version,I would ask you to run patched version before master. 


Yes, there is still some regression however it has come down by a
small margin. I am not doing initdb for each run instead I am doing,
dropdb-->createdb-->pgbench -i. Is dropping old database and creating
a new one for every run not okay, Do I have to do initdb every time.
Okay, I can change the order of reading and let you know the results.

Hi,

On 2017-02-21 16:57:36 +0530, Ashutosh Sharma wrote:
> Yes, there is still some regression however it has come down by a
> small margin. I am not doing initdb for each run instead I am doing,
> dropdb-->createdb-->pgbench -i. Is dropping old database and creating
> a new one for every run not okay, Do I have to do initdb every time.
> Okay, I can change the order of reading and let you know the results.

That does make a difference. Primarily because WAL writes in a new
cluster are a more expensive than in an old one, because of segment
recycling.

- Andres

On Tue, Feb 21, 2017 at 5:52 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
> On Tue, Feb 21, 2017 at 2:37 PM, Andres Freund <andres@anarazel.de> wrote:
>>
>> On 2017-02-21 16:57:36 +0530, Ashutosh Sharma wrote:
>> > Yes, there is still some regression however it has come down by a
>> > small margin. I am not doing initdb for each run instead I am doing,
>> > dropdb-->createdb-->pgbench -i. Is dropping old database and creating
>> > a new one for every run not okay, Do I have to do initdb every time.
>> > Okay, I can change the order of reading and let you know the results.
>>
>> That does make a difference. Primarily because WAL writes in a new
>> cluster are a more expensive than in an old one, because of segment
>> recycling.
>
>
> +1

okay, understood. Will take care of it when I start with next round of
testing. Thanks.

I wonder if this "perf c2c" tool with Linux 4.10 might be useful in
studying this problem.
https://joemario.github.io/blog/2016/09/01/c2c-blog/

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

On Fri, Feb 24, 2017 at 10:41 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 24 February 2017 at 04:41, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>>
>> Okay. As suggested by Alexander, I have changed the order of reading and
>> doing initdb for each pgbench run. With these changes, I got following
>> results at 300 scale factor with 8GB of shared buffer.
>
>
> Would you be able to test my patch also please?
>

Sure, I can do that and share the results by early next week. Thanks.

On Wed, Mar 1, 2017 at 5:29 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>
> On 1 March 2017 at 04:50, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>>
>> On Tue, Feb 28, 2017 at 11:44 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>>
>>> On 28 February 2017 at 11:34, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>>>
>>>>
>>>> So, Here are the pgbench results I got with 'reduce_pgxact_access_AtEOXact.v2.patch' on a read-write workload.
>>>
>>>
>>> Thanks for performing a test.
>>>
>>> I see a low yet noticeable performance gain across the board on that workload.
>>>
>>> That is quite surprising to see a gain on that workload. The main workload we have been discussing was the full
read-onlytest (-S). For that case the effect should be much more noticeable based upon Andres' earlier comments.
 
>>>
>>> Would it be possible to re-run the test using only the -S workload? Thanks very much.
>>
>>
>> Okay, I already had the results for read-oly workload but just forgot to share it along with the results for
read-writetest. Here are the results for read-only
 
>> test,
>
>
> Thanks for conducting those comparisons.
>
> So we have variable and sometimes significant gains, with no regressions.
>
> By the shape of the results this helps in different places to the alignment patch. Do we have evidence to commit
both?

Well, We have seen some regression in read-write test with pgxact
alignment patch - [1]. I may need to run the test with both the
patches to see the combined effect on performance.

[1] - https://www.postgresql.org/message-id/CAE9k0Pk%2BrCuNY%2B7O5XwVXHPuki9t8%3DM7jr4kevxw-hdkpFhS2A%40mail.gmail.com

Hi Alexander

On 3/10/17 8:08 AM, Alexander Korotkov wrote:

> Results look good for me.  Idea of committing both of patches looks
> attractive.
> We have pretty much acceleration for read-only case and small
> acceleration for read-write case.
> I'll run benchmark on 72-cores machine as well.

Have you had a chance to run those tests yet?

Thanks,
-- 
-David
david@pgmasters.net

On Fri, Mar 24, 2017 at 10:12 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 10 March 2017 at 13:08, Alexander Korotkov <a.korotkov@postgrespro.ru>
> wrote:
>> Results look good for me.  Idea of committing both of patches looks
>> attractive.
>
> I'll commit mine since I understand what it does. I'll look at the other one
> also, but won't commit yet.

Might've been nice to include reviewers, testers, and a link to the
mailing list discussion in the commit message.

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

On 2017-03-25 19:35:35 +0300, Alexander Korotkov wrote:
> On Wed, Mar 22, 2017 at 12:23 AM, David Steele <david@pgmasters.net> wrote:
> 
> > Hi Alexander
> >
> > On 3/10/17 8:08 AM, Alexander Korotkov wrote:
> >
> > Results look good for me.  Idea of committing both of patches looks
> >> attractive.
> >> We have pretty much acceleration for read-only case and small
> >> acceleration for read-write case.
> >> I'll run benchmark on 72-cores machine as well.
> >>
> >
> > Have you had a chance to run those tests yet?
> >
> 
> I discovered an interesting issue.
> I found that ccce90b3 (which was reverted) gives almost same effect as
> PGXACT alignment on read-only test on 72-cores machine.

That's possibly because it changes alignment?


> That shouldn't be related to the functionality of ccce90b3 itself, because
> read-only test don't do anything with clog.  And that appears to be true.
> Padding of PGPROC gives same positive effect as ccce90b3.  Padding patch
> (pgproc-pad.patch) is attached.  It's curious that padding changes size of
> PGPROC from 816 bytes to 848 bytes.  So, size of PGPROC remains 16-byte
> aligned.  So, probably effect is related to distance between PGPROC
> members...
> 
> See comparison of 16-bytes alignment of PGXACT + reduce PGXACT access vs.
> padding of PGPROC.

My earlier testing had showed that padding everything is the best
approach :/


I'm inclined to push this to the next CF, it seems we need a lot more
benchmarking here.

Greetings,

Andres Freund

On 4/4/17 8:55 AM, Alexander Korotkov wrote:
> On Mon, Apr 3, 2017 at 9:58 PM, Andres Freund <andres@anarazel.de
> 
>     I'm inclined to push this to the next CF, it seems we need a lot more
>     benchmarking here.
> 
> 
> No objections.

This submission has been moved to CF 2017-07.

Thanks,
-- 
-David
david@pgmasters.net

On 4 April 2017 at 08:58, David Steele <david@pgmasters.net> wrote:
> On 4/4/17 8:55 AM, Alexander Korotkov wrote:
>> On Mon, Apr 3, 2017 at 9:58 PM, Andres Freund <andres@anarazel.de
>>
>>     I'm inclined to push this to the next CF, it seems we need a lot more
>>     benchmarking here.
>>
>>
>> No objections.
>
> This submission has been moved to CF 2017-07.

Please note that I'm expecting to re-commit my earlier patch once
fixed up, since it had all-positive test results.

-- 
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> On 04 Apr 2017, at 14:58, David Steele <david@pgmasters.net> wrote:
>
> On 4/4/17 8:55 AM, Alexander Korotkov wrote:
>> On Mon, Apr 3, 2017 at 9:58 PM, Andres Freund <andres@anarazel.de
>>
>>    I'm inclined to push this to the next CF, it seems we need a lot more
>>    benchmarking here.
>>
>> No objections.
>
> This submission has been moved to CF 2017-07.

This CF has now started (well, 201709 but that’s what was meant in above), can
we reach closure on this patch in this CF?

cheers ./daniel

> On 16 Sep 2017, at 01:51, Alexander Korotkov <a.korotkov@postgrespro.ru> wrote:
>
> On Tue, Sep 5, 2017 at 2:47 PM, Daniel Gustafsson <daniel@yesql.se <mailto:daniel@yesql.se>> wrote:
> > On 04 Apr 2017, at 14:58, David Steele <david@pgmasters.net <mailto:david@pgmasters.net>> wrote:
> >
> > On 4/4/17 8:55 AM, Alexander Korotkov wrote:
> >> On Mon, Apr 3, 2017 at 9:58 PM, Andres Freund <andres@anarazel.de <mailto:andres@anarazel.de>
> >>
> >>    I'm inclined to push this to the next CF, it seems we need a lot more
> >>    benchmarking here.
> >>
> >> No objections.
> >
> > This submission has been moved to CF 2017-07.
>
> This CF has now started (well, 201709 but that’s what was meant in above), can
> we reach closure on this patch in this CF?
>
> During previous commitfest I come to doubts if this patch is really needed when same effect could be achieved by
another(perhaps random) change of alignment.  The thing I can do now is to retry my benchmark on current master and
checkwhat effect this patch has now. 

Considering this I’ll mark this as Waiting on Author, in case you come to
conclusion that another patch is required then we’ll bump to a return status.

cheers ./daniel

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers