Re: pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

On 4 April 2016 at 08:22, Andres Freund <andres(at)anarazel(dot)de> wrote:

> On 2016-04-04 08:08:32 +0100, Simon Riggs wrote:
> > For that matter, it's also important for hot standby; to deal with
> > > FATALed transactions which didn't write an abort record. It's kinda
> > > important to call StandbyReleaseOldLocks for those. And if a standby
> is
> > > in STANDBY_SNAPSHOT_READY it's also important to see this kind of
> > > record.
> > >
> >
> > Those objections apply to all solutions to the problem so far
> > proposed,
>
> No, because in the alternative proposal we determine that the system
> indeed has been idle since the last time a WAL record was logged.

Why would that change anything? If something aborts without writing an
abort record, the system can easily be idle afterwards. So an idle system
means nothing. Your objection applies to *all* cases, not just for this
patch and if we revert, we block all cases.

> And
> only a few select records are exempt from being considered idle; every
> other type of record causes the system to be considered non-idle.

> likely any solution. My understanding was that those issues are considered
> > the lesser of the two evils. I'm happy to revert this patch, as long as
> we
> > agree that it also blocks all further "bug fixes" so far proposed based
> > upon those arguments.
>
> Yes, please do.
>
> > > Additionally, we're now logging more WAL if archive timeout isn't
> > > configured, which doesn't strike me as a good idea.
>
> (to clarify, I'm comparing a configured XLogArchiveTimeout to none, not
> the before/after patch state)
>
> > That's not true either...
>
> + if (running->xcnt == 0 &&
> + nlocks == 0 &&
> + XLogArchiveTimeout > 0 &&
> + !last_snapshot_overflowed)
> + {
> + LWLockRelease(XidGenLock);
> + return InvalidXLogRecPtr;
> + }
>
> How can XLogArchiveTimeout not imply a behavioural difference here?

The bug appears when we have archive_timeout set; the bug fix applies in
that case also.

> > Thanks for your comments, but you seem to be misreading the patch with
> > respect to the if clause and new brackets.
>
> I indeed missed the outer if; which addresses the logical issue. So the
> problem there isn't fixed at all.
>
> You ignored a good number of messages and just committed a patch with a
> different approach without any further discussion. So it shouldn't be
> particularly surprising that people aren't willing to look super careful
> at what you did.
>

I haven't ignored any messages, regrettably I read them all. I've committed
a much simpler patch, which is what committers do. That patch does exactly
the same thing as the patch you prefer, just does it differently; I'm sorry
if that annoys you. If you have time for debate, please read the patch and
lets have an even-handed and rational debate. If you don't have time to
review the patch, lets skip the debate.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services