Hi, On 2016-05-04 14:25:14 -0400, Robert Haas wrote: > On Wed, May 4, 2016 at 12:42 PM, Andres Freund <and...@anarazel.de> wrote: > > On 2016-05-04 13:35:02 -0300, Alvaro Herrera wrote: > >> Honestly, I don't see any strong ground in which to base a revert threat > >> for this feature. > > > > It's datastructures are badly designed. But releasing it there's no > > pressure to fix that. If this were an intrinsic cost - ok. But it's > > not. > > I don't want to rule out the possibility that you are right, because > you're frequently right about lots of things. However, you haven't > provided much detail. AFAIK, the closest you've come to articulating > a case for reverting this patch is to say that the tax ought to be > paid by the write-side, rather than the read-side.
I think I did go into some more detail than that, but let me explain here: My concern isn't performing checks at snapshot build time and at buffer access time. That seems fairly reasonable. My concern is that the time->xid mapping used to determine the xid horizon is built at snapshot access time. The relevant function for that is MaintainOldSnapshotTimeMapping() called by GetSnapshotData(), if you want to look at it. Building the mapping when building the snapshot requires that an exclusive lwlock is taken. Given that there's very few access patterns in which more xids are assigned than snapshots taken, that just doesn't make much sense; even leaving the fact that adding an exclusive lock in a function that's already one of the biggest bottlenecks in postgres, isn't a good idea. If we instead built the map somewhere around GetNewTransactionId() we'd only pay the cost when advancing an xid. It'd also make it possible to avoid another GetCurrentIntegerTimestamp() per snapshot, including the acquiration of oldSnapshotControl->mutex_current. In addition the data structure would be easier to maintain, because xids grow monotonically (yes, yes wraparound, but that's not a problem) - atm we need somwehat more complex logic to determine into which bucket an xid/timestamp pair has to be mapped to. So I'm really not just concerned with the performance, I think that's just fallout from choosing the wrong data representation. If you look at at bottom of http://www.postgresql.org/message-id/20160413171955.i53me46fqqhdl...@alap3.anarazel.de which shows performance numbers for old_snapshot_threshold=0 vs. old_snapshot_threshold=10. While that wasn't intended at the time, old_snapshot_threshold=0 shows the cost of the feature without maintaining the mapping, and old_snapshot_threshold=10 shows it while building the mapping. Pretty dramatic difference - that's really the cost of map maintenance. FWIW, it's not just me doubting the data structure here, Ants did as well. > Surely nobody here is blind to the fact that holding back xmin often > creates a lot of bloat for no reason - many or all of the retained old > row versions may never be accessed. Definitely. I *like* the feature. > So I would like to hear more detail about why you think that the data > structures are badly designed, and how they could be designed > differently without changing what the patch does (which amounts to > wishing that Kevin had implemented a different feature than the one > you think he should have implemented). Well, there'd be some minor differences what determines "too old" based on the above, but I think it'd just be a bit easier to explain. > >> It doesn't scale as well as we would like in the case > >> where a high-level is fully stressed with a read-only load -- okay. But > >> that's unlikely to be a case where this feature is put to work. > > > > It'll be just the same in a read mostly workload, which is part of the > > case for this feature. > > So what? SSI doesn't scale either, and nobody's saying we have to rip > it out. It's still useful to people. pgbench is not the world. Sure, pgbench is not the world. But this isn't a particularly pgbench specific issue. > >> So I think accepting the promise that this feature would be improved > >> in a future release to better support that case is good enough. > > > > I've not heard any such promise. > > The question Alvaro is raising isn't whether such a promise has been > made but whether it would suffice. In fairness, such promises are not > enforceable. Kevin can promise to work on this and then be run over > by a rogue Mr. Softee truck tomorrow, and the work won't get done; or > he can go to work for some non-PostgreSQL-supporting company and > vanish. Sure, it's not a guarantee. But I think a "promise" (signed in blood of course) of a senior contributor makes quite the difference. > But personally, I generally agree with Alvaro's analysis. If this > patch is slow when turned on, and you don't like that, don't use it. > If you need to use it and want it to scale better, then you can write > a patch to make it do that. Nobody is more qualified than you. I think that's what ticks me off about this. By introducing a feature implemented with the wrong structure, the onus of work is placed on others. It's imo perfectly reasonable not to unneccesarily perform micro-optimization before benchmarks show a problem, and if it were just a question of doing that in 9.7, I'd be fine. But what we're talking about is rewriting the central part of the feature. > I think it's a show-stopper if the patch regresses performance more > than trivially when turned off, but we need fresh test results before > reaching a conclusion about that. I'm not concerned about that at this stage. Kevin addressed those problems as far as I'm aware. > I also think it's a show-stopper if you can hold out specific design > issues which we can generally agree are signs of serious flaws in > Kevin's thinking. I think that's my issue. > I do not think it's a show-stopper if you wish he'd worked harder on > the performance under heavy concurrency with very short transactions. > You could have raised that issue at any time, but instead waited until > after feature freeze. Sorry, I don't buy that argument. There were senior community people giving feedback on the patch, the potential for performance regressions wasn't called out, the patch was updated pretty late in the CF. I find it really scary that review didn't call out the potential for regressions here, at the very least the ones with the feature disabled really should have been caught. Putting exclusive lwlocks and spinlocks in critical paths isn't a subtle, easy to miss, issue. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
