Re: Re: BUG #13685: Archiving while idle every archive_timeout with wal_level hot_standby

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