Hi, On 2018-06-08 09:23:02 +0100, Simon Riggs wrote: > I have also found another bug which affects what we do next. > > For context, AEL locks are normally removed by COMMIT or ABORT. > StandbyReleaseOldLocks() is just a sweeper to catch anything that > didn't send an abort before it died, so it hardly ever activates. The > coding of StandbyReleaseOldLocks() is backwards... if it ain't in the > running list, then we remove it. > > But that wasn't working correctly either, since as of 49bff5300d527 we > assigned AELs to subxids. Subxids weren't in the running list and so > AELs held by them would have been removed at the wrong time, an extant > bug in PG10. It looks to me like they would have been removed either > early or late, up to the next runningxact info record. They would be > removed, so no leakage, but the late timing wouldn't be noticeable for > tests or most usage, since it would look just like lock contention. > Early release might give same issue of block access to non-existent > block/file.
Yikes, that's kinda bad. It can also just cause plain crashes, no? The on-disk / catalog state isn't necessarily consistent during DDL, which is why we hold AE locks. At the very least it can cause corruption of in-use relcache entries and such. In practice the fact this probably hits only a limited number of people because it requires DDL to happen in subtransactions, which probably isn't *that* common. > So the attached patch fixes both the bug in the recent commit and the > one I just found by observation of 49bff5300d527, since they are > related. Can we please keep them separate? > StandbyReleaseOldLocks() can sweep in the same way as > ExpireOldKnownAssignedTransactionIds(). > > > I also don't understand why this change would be backpatched in the > > first place. It's a relatively minor efficiency thing, no? > > As for everything, that is open to discussion. Yes, it seems minor to > me.... until it affects you, then its not. How is it any worse than any other normal short-lived write transaction? The truncation is done shortly before commit. > It seems to have affected Greg. As far as I can tell Greg was just theorizing? Greetings, Andres Freund