Re: Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold <

From: Andres Freund <andres(at)anarazel(dot)de>
To: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Noah Misch <noah(at)leadboat(dot)com>, Kevin Grittner <kgrittn(at)gmail(dot)com>, Kevin Grittner <kgrittn(at)postgresql(dot)org>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold <
Date: 2016-04-13 15:08:39
Message-ID: 20160413150839.mevdlgekizxyjhc5@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-committers pgsql-hackers

On 2016-04-13 11:08:21 -0300, Alvaro Herrera wrote:
> The patch being proposed for commit is fiddly architecture-specific
> stuff which is likely to destabilize the tree for quite some time, and
> cause lots of additional work to Andres and anyone else likely to work
> on such low-level details, such as Robert, both of which already have
> plenty to do.

Personally I think this is an 9.6 open-item, and primarily Kevin has to
work on it.

Note that there really shouldn't be too much fiddly bits, we've already
had 64bit atomics, just no fallback. This is just copying the fallback
code from 32bit atomics to 64bit atomics.

But what I'm actually proposing isn't even using the 64bit atomics from
that patch, just to add

--- a/src/include/port/atomics/arch-ppc.h
+++ b/src/include/port/atomics/arch-ppc.h
@@ -24,3 +24,6 @@
#define pg_read_barrier_impl() __asm__ __volatile__ ("lwsync" : : : "memory")
#define pg_write_barrier_impl() __asm__ __volatile__ ("lwsync" : : : "memory")
#endif
+
+/* per architecture manual doubleword accesses have single copy atomicity */
+#define PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY

to the appropriate files (ia64, ppc, x86) and then add an #ifndef
PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY to GetXLogInsertRecPtr's acquisition
of the spinlock. I.e.
#ifndef PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY
SpinLockAcquire(&Insert->insertpos_lck);
#endif
current_bytepos = Insert->CurrBytePos;
#ifndef PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY
SpinLockRelease(&Insert->insertpos_lck);
#endif

not because I think it's perfectly pretty that way, but because it's
very easy to demonstrate that there's no regressions for anybody.

> The regression only seems to show up if you turn it on and have a
> crazily high rate of read-only transactions. I think this can wait
> for 9.7.

I don't think 120k read tps is all that high anymore these days. And you
can easily create scenarios that are *much* worse than pgbench. E.g. a
loop in a volatile plpgsql function will acquire

Greetings,

Andres Freund

In response to

Browse pgsql-committers by date

  From Date Subject
Next Message Andres Freund 2016-04-13 15:18:01 Re: Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold <
Previous Message Robert Haas 2016-04-13 15:06:09 Re: Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold <

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2016-04-13 15:18:01 Re: Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold <
Previous Message Robert Haas 2016-04-13 15:06:09 Re: Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold <