Date: Sat, 6 Feb 2016 18:49:30 +0100
From: Andres Freund <andres@anarazel.de>
To: Michael Paquier <michael.paquier@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>,
 Robert Haas <robertmhaas@gmail.com>,
 Jeff Janes <jeff.janes@gmail.com>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: Re: BUG #13685: Archiving while idle every
 archive_timeout with wal_level hot_standby
Message-ID: <20160206174930.GA21471@awork2.anarazel.de>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAB7nPqT_AUQS=41n6yy_=w2Txt=LBvq+hZr+2jD4=oevHe7e6Q@mail.gmail.com>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

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