Thanks! > > 2/ > > I don't see we have tests for other timeout based GUCs, but it would nice > > to ensure that this woks correctly. Maybe as a custom_stats test where we > > SET stats_flush_interval inside the transaction and make sure the stats > > flush > > only after the new timeout has passed. Maybe? > > Not sure I follow, that's what 0002 is doing.
I was referring to "SET stats_flush_interval", which you are doing in 0005, so disregard this comment. I wrote this comment when I read 0003 only. > > Will this be tue at all time? Let's imagine a Kind that flushes all the > > fields > > ANYTIME, would we not want to delete the pending entry? > > > > + /* if successfull non-partial flush, remove entry */ > > + if (did_flush && !is_partial_flush) > > pgstat_delete_pending_entry(entry_ref); > > > > Right, and that's why the MIXED flush mode was useful, i.e to be able to > distinguish > here. So, in the attached, instead of re-introducing the MIXED flush mode, I > added > a "is_partial" bool paramater to the flush_pending_cb(). > Yeah, MIXED flush mode to deal with this is still not the right idea, IMO. I do agree with what you did here by passing "is_partial" to the callbacks. Now it will be up to the callbacks to determine if they performed a partial or a complete flush ( even in the case of any anytime flush, if they so happened to flush stats for all the fields ). I do not have any further comments on this patchset. -- Sami Imseih Amazon Web Services (AWS)
