Re: pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

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

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

Greetings,

Andres Freund