On Thu, Sep 28, 2023 at 8:46 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
> Once I started writing the function comment, however, I felt a bit
> awkward. In order to make the function available to both pruneheap.c
> and vacuumlazy.c, I had to put it in a header file. Writing a
> function, available to anyone including heapam.h, which takes an int
> and returns an HTSV_Result feels a bit odd. Do we want it to be common
> practice to use an int value outside the valid enum range to store
> "status not yet computed" for HTSV_Results?
I noticed the awkwardness of that return convention when I reviewed
the first version of this patch, but I decided it wasn't worth
spending time discussing. To avoid it, we'd either need to add a new
HTSV_Result that is only used here, or add a new type
HTSV_Result_With_An_Extra_Value and translate between the two, or pass
back a boolean + an enum instead of an array of int8. And all of those
seem to me to suck -- the first two are messy and the third would make
the return value much wider. So, no, I don't really like this, but
also, what would actually be any better? Also, IMV at least, it's more
of an issue of it being sort of ugly than of anything becoming common
practice, because how many callers of heap_page_prune() are there ever
going to be? AFAIK, we've only ever had two since forever, and even if
we grow one or two more at some point, that's still not that many.
I went ahead and committed 0001. If Andres still wants to push for
more renaming there, that can be a follow-up patch. And we can see if
he or anyone else has any comments on this new version of 0002. To me
we're down into the level of details that probably don't matter very
much one way or the other, but others may disagree.
--
Robert Haas
EDB: http://www.enterprisedb.com