On Wed, Apr 1, 2020 at 11:09 AM Andres Freund <and...@anarazel.de> wrote: > That doesn't exist in all the back branches. Think it'd be easier to add > code to explicitly prune it during MaintainOldSnapshotTimeMapping().
That's reasonable. > There's really no reason at all to have bins of one minute. As it's a > PGC_POSTMASTER GUC, it should just have didided time into bins of > (old_snapshot_threshold * USEC_PER_SEC) / 100 or such. For a threshold > of a week there's no need to keep 10k bins, and the minimum threshold of > 1 minute obviously is problematic. I am very doubtful that this approach would have been adequate. It would mean that, with old_snapshot_threshold set to a week, the threshold for declaring a snapshot "old" would jump forward 16.8 hours at a time. It's hard for me to make a coherent argument right now as to exactly what problems that would create, but it's not very granular, and a lot of bloat-related things really benefit from more granularity. I also don't really see what the problem with keeping a bucket per minute in memory is, even for a week. It's only 60 * 24 * 7 = ~10k buckets, isn't it? That's not really insane for an in-memory data structure. I agree that the code that does that maintenance being buggy is a problem to whatever extent that is the case, but writing the code to have fewer buckets wouldn't necessarily have made it any less buggy. > They probably are fixable. But there's a lot more, I think: > > Looking at TransactionIdLimitedForOldSnapshots() I think the ts == > update_ts threshold actually needs to be ts >= update_ts, right now we > don't handle being newer than the newest bin correctly afaict (mitigated > by autovacuum=on with naptime=1s doing a snapshot more often). It's hard > to say, because there's no comments. That test and the following one for "if (ts == update_ts)" both make me nervous too. If only two of <, >, and = are expected, there should be an Assert() to that effect, at least. If all three values are expected then we need an explanation of why we're only checking for equality. > The whole lock nesting is very hazardous. Most (all?) > TestForOldSnapshot() calls happen with locks on on buffers held, and can > acquire lwlocks itself. In some older branches we do entire *catalog > searches* with the buffer lwlock held (for RelationHasUnloggedIndex()). The catalog searches are clearly super-bad, but I'm not sure that the other ones have a deadlock risk or anything. They might, but I think we'd need some evidence of that. > GetSnapshotData() using snapshot->lsn = GetXLogInsertRecPtr(); as the > basis to detect conflicts seems dangerous to me. Isn't that ignoring > inserts that are already in progress? How so? > It currently silently causes wrong query results. There's no > infrastructure to detect / protect against that (*). Sure, and what if you break more stuff ripping it out? Ripping this volume of code out in a supposedly-stable branch is totally insane almost no matter how broken the feature is. I also think, and we've had this disagreement before, that you're far too willing to say "well, that's wrong so we need to hit it with a nuke." I complained when you added those error checks to vacuum in back-branches, and since that release went out people are regularly tripping those checks and taking prolonged outages for a problem that wasn't making them unhappy before. I know that in theory those people are better off because their database was always corrupted and now they know. But for some of them, those prolonged outages are worse than the problem they had before. I believe it was irresponsible to decide on behalf of our entire user base that they were better off with such a behavior change in a supposedly-stable branch, and I believe the same thing here. I have no objection to the idea that *if* the feature is hopelessly broken, it should be removed. But I don't have confidence at this point that you've established that, and I think ripping out thousands of lines of codes in the back-branches is terrible. Even hard-disabling the feature in the back-branches without actually removing the code is an awfully strong reaction, but it could be justified if we find out that things are actually super-bad and not really fixable. Actually removing the code is unnecessary, protects nobody, and has risk. > Do we actually have any evidence of this feature ever beeing used? I > didn't find much evidence for that in the archives (except Thomas > finding a problem). Given that it currently will switch between not > preventing bloat and causing wrong query results, without that being > noticed... I believe that at least one EnterpriseDB customer used it, and possibly more than one. I am not sure how extensively, though. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
