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
