Thanks! At Mon, 6 Jul 2020 20:54:36 -0400, Alvaro Herrera <alvhe...@2ndquadrant.com> wrote in > On 2020-Jul-06, Alvaro Herrera wrote: > > > Hmm, I like safe_wal_size.
I agree to the name, too. > > I've been looking at this intermittently since late last week and I > > intend to get it done in the next couple of days. > > 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? > 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. > I also made it report null immediately when slots are in state lost. > But beware of slots that appear lost but fall in the unreserved category > because they advanced before checkpointer signalled them. (This case > requires a debugger to hit ...) Oh! Okay, that change seems right to me. > 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..) > Any more thoughts? If not, I'll get this pushed tomorrow finally. regards. -- Kyotaro Horiguchi NTT Open Source Software Center