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

On 2016-02-08 15:58:49 +0900, Michael Paquier wrote:
> 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.

Unconvinced. For one, the equivalent behaviour for checkpoints has
existed since at least 9.0. Secondly, the problem only really appears if
you use archiving, configure a nondefault archive timeout, and that
timeout is significantly smaller than checkpoint timeout. And then the
cost is partially filled segment every now and then. That just doesn't
stand into relation into adding some nontrivial code into backbranches.

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

We don't set valid values in WALInsertLockAcquireExclusive for all locks
either. I don't see consistency to what this is going to buy us.

> /*
> + * XLogInsert
> + *
> + * A shorthand for XLogInsertExtended, to update the progress of WAL
> + * activity by default.
> + */
> +XLogRecPtr
> +XLogInsert(RmgrId rmid, uint8 info)
> +{
> + return XLogInsertExtended(rmid, info, true);
> +}
> +
> +/*
> + * XLogInsertExtended

I'm not really a fan. I'd rather change existing callers to add a
'flags' bitmask argument. Right now there can't really be XLogInserts()
in extension code, so that's pretty ok to change.

Greetings,

Andres Freund