From: Tom Lane <tgl@sss.pgh.pa.us>
To: Nathan Bossart <nathandbossart@gmail.com>
cc: Greg Stark <stark@mit.edu>, pgsql-hackers@postgresql.org
Subject: Re: identifying the backend that owns a temporary schema
In-reply-to: <20220929024038.GA4945@nathanxps13>
References: <20220815205811.GA250990@nathanxps13>
 <CAM-w4HMZ03AbQjBbfN_mTxct68HX3NPRQVfLaRxLGJTejJ8xbg@mail.gmail.com>
 <20220829230757.GA542616@nathanxps13> <2297420.1664041298@sss.pgh.pa.us>
 <20220926160822.GA1298186@nathanxps13> <3499642.1664221809@sss.pgh.pa.us>
 <20220926201113.GA1340606@nathanxps13> <4157446.1664405780@sss.pgh.pa.us>
 <20220929024038.GA4945@nathanxps13>
Comments: In-reply-to Nathan Bossart <nathandbossart@gmail.com>
	message dated "Wed, 28 Sep 2022 19:40:38 -0700"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <68997.1664462826.1@sss.pgh.pa.us>
Date: Thu, 29 Sep 2022 10:47:06 -0400
Message-ID: <68998.1664462826@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/68998.1664462826%40sss.pgh.pa.us>
Precedence: bulk

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Wed, Sep 28, 2022 at 06:56:20PM -0400, Tom Lane wrote:
>> 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.

Right.  I'd been thinking the current code wouldn't generate duplicate IDs
--- but it might produce duplicate query output anyway, in case a given
backend's entry is later in the array than it was before.  So really
there's not much guarantees here in any case.

>> -	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.

Ah, I see: if the user passes in a "backend ID" that is close to INT_MIN,
then the comparator's subtraction could overflow.  We could fix that by
writing out the comparator code longhand ("if (a < b) return -1;" etc),
but I don't really think it's necessary.  bsearch is guaranteed to
correctly report that such a key is not present, even if it takes a
strange search path through the array due to inconsistent comparator
results.  So the test quoted above just serves to fail a bit more quickly,
but we certainly shouldn't be optimizing for the case of a bad ID.

> Overall, LGTM.

OK.  Will push shortly.

			regards, tom lane