On 2020-Jul-07, Kyotaro Horiguchi wrote: > At Mon, 6 Jul 2020 20:54:36 -0400, Alvaro Herrera <alvhe...@2ndquadrant.com> > wrote in
> > I propose the attached. This is pretty much what was proposed by > > Kyotaro, but I made a couple of changes. Most notably, I moved the > > calculation to the view code itself rather than creating a function in > > xlog.c, mostly because it seemed to me that the new function was > > creating an abstraction leakage without adding any value; also, if we > > add per-slot size limits later, it would get worse. > > I'm not sure that detailed WAL segment calculation fits slotfuncs.c > but I don't object to the change. However if we do that: > > + /* determine how many segments slots can be kept by > slots ... */ > + keepSegs = max_slot_wal_keep_size_mb / > (wal_segment_size / (1024 * 1024)); > > Couldn't we move ConvertToXSegs from xlog.c to xlog_ingernals.h and > use it intead of the bare expression? I was of two minds about that, and the only reason I didn't do it is that we'll need to give it a better name if we do it ... I'm open to suggestions. > > The other change was to report negative values when the slot becomes > > unreserved, rather than zero. It shows how much beyond safety your > > slots are getting, so it seems useful. Clamping at zero seems to serve > > no purpose. > > The reason for the clamping is the signedness of the values, or > integral promotion. However, I believe the calculation cannot go > beyond the range of signed long so the signedness conversion in the > patch looks fine. Yeah, I think the negative values are useful to see. I think if you ever get close to 2^62, you're in much more serious trouble anyway :-) But I don't deny that the math there could be subject of overflow issues. If you want to verify, please be my guest ... > > One thing that got my attention while going over this is that the error > > message we throw when making a slot invalid is not very helpful; it > > doesn't say what the current insertion LSN was at that point. Maybe we > > should add that? (As a separate patch, of couse.) > > It sounds helpful to me. (I remember that I sometime want to see > checkpoint LSNs in server log..) Hmm, ... let's do that for pg14! Thanks for looking, -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
