On Wed, Sep 28, 2022 at 06:56:20PM -0400, Tom Lane wrote:
> I reviewed this and made some changes, some cosmetic some less so.
Thanks for the detailed review.
> A point that still bothers me a bit about pg_stat_get_backend_idset is
> that it could miss or duplicate some backend IDs if the user calls
> pg_stat_clear_snapshot() partway through the SRF's run, and we reload
> a different set of backend entries than we had before. I added a comment
> about that, with an argument why it's not worth working harder, but
> is the argument convincing? If not, what should we do?
Isn't this an existing problem? Granted, it'd manifest differently with
this patch, but ISTM we could already end up with too many or too few
backend IDs if there's a refresh partway through. I don't know if there's
an easy way to avoid mismatches altogether besides perhaps ERROR-ing if
there's a concurrent refresh.
> - if (beid < 1 || beid > localNumBackends)
> - return NULL;
The reason I'd kept this in was because I was worried about overflow in the
comparator function. Upon further inspection, I don't think there's
actually any way that will produce incorrect results. And I'm not sure we
should worry about such cases too much, anyway.
Overall, LGTM.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com