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

On Sun, Feb 7, 2016 at 2:49 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> 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.

The original bug report of this thread is based on 9.4, which is the
first version where the standby snapshot in the bgwriter has been
introduced. Knowing that most of the systems in the world are actually
let idle. If those are running Postgres, I'd like to think that it is
a waste. Now it is true that this is not a critical issue.

>> + /*
>> + * 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.

Hm, OK. I think that the best way to deal with that is to introduce
XLogInsertExtended which wraps the existing XLogInsert(). By default
the flag is true. This will reduce the footprint of this patch on the
code base and will ease future backpatches on those code paths at
least down to 9.5 where WAL refactoring has been introduced.

>> + if (holdingAllLocks)
>> + {
>> + int i;
>> +
>> + for (i = 0; i < NUM_XLOGINSERT_LOCKS; i++)
>> + WALInsertLocks[i].l.progressAt = StartPos;
>
> Why update all?

For consistency. I agree that updating one, say the first one is
enough. I have added a comment explaining that as well.

>> /*
>> + * 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.

Check.

>> /*
>> + * 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 */

Check.

> Looks much better now.

Thanks. Let's wrap that! An updated patch is attached.
--
Michael