From: Andres Freund <andres@anarazel.de>
To: Michael Paquier <michael.paquier@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Jeff Janes <jeff.janes@gmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: Re: BUG #13685: Archiving while idle every archive_timeout with wal_level hot_standby
Date: Sat, 6 Feb 2016 18:49:30 +0100
Message-ID: <20160206174930.GA21471@awork2.anarazel.de> (raw)
In-Reply-To: <CAB7nPqT_AUQS=41n6yy_=w2Txt=LBvq+hZr+2jD4=oevHe7e6Q@mail.gmail.com>
References: <CAB7nPqQ4OGNvOEUgrOwQyxsVotdXa0AeVtKHM0xm+oNy2dtkPA@mail.gmail.com>
<20160129081330.e5mj65du2cknxsja@alap3.anarazel.de>
<CAB7nPqT5XdZYo0rub8hyBC9CiWxB6=TSG7ffm_QBR+Q4L8ZFGg@mail.gmail.com>
<CAB7nPqTe_FzHg7gctybgff-1S_1OmcE5AAVsOVeisO=V3Z4FZA@mail.gmail.com>
<CAA4eK1LbbWPiwzP3NMEVijFb_PymQB6GyTDTgSyaDOpHZqm69g@mail.gmail.com>
<20160203093803.ui5h2avzz534cxrw@alap3.anarazel.de>
<CAA4eK1+s2wt07So8B-FiHqjjJkYxvgx8LtoQX-kL+dOw3_hj5A@mail.gmail.com>
<20160204131003.h7obwkeaellkcewb@alap3.anarazel.de>
<CAB7nPqQYYesPsmGfeWzuZ9yPQgvJ9KkhzXmoV9O+FYLAq1iNKA@mail.gmail.com>
<CAB7nPqT_AUQS=41n6yy_=w2Txt=LBvq+hZr+2jD4=oevHe7e6Q@mail.gmail.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
On 2016-02-06 22:03:15 +0900, Michael Paquier wrote:
> The patch attached will apply on master, on 9.5 there is one minor
> conflict. For older versions we will need another reworked patch.
FWIW, I don't think we should backpatch this. It'd look noticeably
different in the back branches, and this isn't really a critical
issue. I think it makes sense to see this as an optimization.
> + /*
> + * Update the progress LSN positions. At least one WAL insertion lock
> + * is already taken appropriately before doing that, and it is just more
> + * simple to do that here where WAL record data and type is at hand.
> + * The progress is set at the start position of the record tracked that
> + * is being added, making easier checkpoint progress tracking as the
> + * control file already saves the start LSN position of the last
> + * checkpoint run.
> + */
> + if (!isStandbySnapshot)
> + {
I don't like isStandbySnapshot much, it seems better to do this more
generally, via a flag passed down by the inserter.
> + if (holdingAllLocks)
> + {
> + int i;
> +
> + for (i = 0; i < NUM_XLOGINSERT_LOCKS; i++)
> + WALInsertLocks[i].l.progressAt = StartPos;
Why update all?
> /*
> + * GetProgressRecPtr -- Returns the newest WAL activity position, aimed
> + * at the last significant WAL activity, or in other words any activity
> + * not referring to standby logging as of now. Finding the last activity
> + * position is done by scanning each WAL insertion lock by taking directly
> + * the light-weight lock associated to it.
> + */
> +XLogRecPtr
> +GetProgressRecPtr(void)
> +{
> + XLogRecPtr res = InvalidXLogRecPtr;
> + int i;
> +
> + /*
> + * Look at the latest LSN position referring to the activity done by
> + * WAL insertion.
> + */
> + for (i = 0; i < NUM_XLOGINSERT_LOCKS; i++)
> + {
> + XLogRecPtr progress_lsn;
> +
> + LWLockAcquire(&WALInsertLocks[i].l.lock, LW_EXCLUSIVE);
> + progress_lsn = WALInsertLocks[i].l.progressAt;
> + LWLockRelease(&WALInsertLocks[i].l.lock);
Add a comment explaining that we a) need a lock because of the potential
for "torn reads" on some platforms. b) need an exclusive one, because
the insert lock logic currently only expects exclusive locks.
> /*
> + * Fetch the progress position before taking any WAL insert lock. This
> + * is normally an operation that does not take long, but leaving this
> + * lookup out of the section taken an exclusive lock saves a couple
> + * of instructions.
> + */
> + progress_lsn = GetProgressRecPtr();
too long for my taste. How about:
/* get progress, before acuiring insert locks to shorten locked section */
Looks much better now.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: andres@anarazel.de, michael.paquier@gmail.com, amit.kapila16@gmail.com, robertmhaas@gmail.com, jeff.janes@gmail.com, pgsql-hackers@postgresql.org
Subject: Re: Re: BUG #13685: Archiving while idle every archive_timeout with wal_level hot_standby
In-Reply-To: <20160206174930.GA21471@awork2.anarazel.de>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox