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