Re: pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server - Mailing list pgsql-committers

From Simon Riggs
Subject Re: pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server
Date
Msg-id CANP8+jKuy8W6xkMHF_V0YzWWMQdZ-LcUQiQ71kv9BgBjaq5T4Q@mail.gmail.com
Whole thread Raw
In response to Re: pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server  (Andres Freund <andres@anarazel.de>)
Responses Re: pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server  (Andres Freund <andres@anarazel.de>)
List pgsql-committers
On 4 April 2016 at 08:22, Andres Freund <andres@anarazel.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/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

pgsql-committers by date:

Previous
From: Andres Freund
Date:
Subject: Re: pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server
Next
From: Andres Freund
Date:
Subject: Re: pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server