MIME-Version: 1.0
References: 
 <CA+hUKGJ_7NKd46nx1wbyXWriuZSNzsTfm+rhEuvU6nxZi3-KVw@mail.gmail.com>
 <d8c3bbe9-690d-4d51-9d80-51f9856ee086@postgresql.org>
 <CA+hUKG+x2BcqWzBC77cN0ewhzMF0kYhC6c4G_T2gJLPbqYQ6Ow@mail.gmail.com>
 <CA+hUKGL-3mBtkA9RTbLFHuSS5cviuv0ko7nBhCg9KM7Q-GSEkw@mail.gmail.com>
 <CAAKRu_ZVxzwRRbxedgb_LtkFaGf78XAbTO9uExvadV2DzaE=Jg@mail.gmail.com>
 <CA+hUKG+zLmkD9zus=JOjjC+j5p9R1+CSXNZgd5=exZ01ZTaKoA@mail.gmail.com>
 <CA+hUKGJx6FNqzsxfSOGH0nJZJq1MBc+t7NBKtAmy6zj4HD86tA@mail.gmail.com>
 <CAN55FZ16TEhgYbK=qSEbkO8utz+u232NksCEmJMC1G4iZvnbvA@mail.gmail.com>
 <CA+hUKGL7-Dx8KiUo=G91Y5tfFpwDUFFQJ6=9D8Gr1n=DZxGh+w@mail.gmail.com>
In-Reply-To: 
 <CA+hUKGL7-Dx8KiUo=G91Y5tfFpwDUFFQJ6=9D8Gr1n=DZxGh+w@mail.gmail.com>
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Tue, 18 Nov 2025 16:17:28 -0500
Message-ID: 
 <CAAKRu_ZGhnWZXOyEyZ2r47g-F7U8asMRA6U8YZw3h=2rR=m_hQ@mail.gmail.com>
Subject: Re: Trying out read streams in pgvector (an extension)
To: Thomas Munro <thomas.munro@gmail.com>
Cc: Nazir Bilal Yavuz <byavuz81@gmail.com>,
 "Jonathan S. Katz" <jkatz@postgresql.org>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Type: multipart/mixed; boundary="0000000000007a046f0643e4fe43"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAKRu_ZGhnWZXOyEyZ2r47g-F7U8asMRA6U8YZw3h%3D2rR%3Dm_hQ%40mail.gmail.com>
Precedence: bulk

--0000000000007a046f0643e4fe43
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Nov 12, 2025 at 5:47=E2=80=AFAM Thomas Munro <thomas.munro@gmail.co=
m> wrote:
>
> I suppose an alternative design would be that _next_buffer() returns
> InvalidBuffer only once (=3D the block number callback returns
> InvalidBlock once) and then automatically resumes (=3D it restores the
> distance) and then you can call read_stream_next_buffer() again (=3D the
> block number callback will be called again to fill the stream up with
> new buffers before waiting for the first one to be ready to give to
> you if it isn't already).  That would have the advantage of not
> requiring a new function at all and make the patch even shorter, but I
> don't know, I guess I thought that would be a bit more fragile in some
> way, less explicit.  Hmm, would it actually be better if it worked
> like that?

We discussed off-list and decided that changing existing functionality
in an unexpected way is undesirable. So, it is better we stick with
adding read_stream_resume. However, in talking about
read_stream_resume() further, Thomas and I also thought of potential
issues with it:

If read_stream_resume() is called before the read stream user callback
has ever returned InvalidBlockNumber,
1) The value of resume_distance will be the original value of distance
from read_stream_begin_relation(). You don't want to reset the
distance to that value.
2) There may be inflight or completed buffers that have yet to be
yielded which will be returned the next time read_stream_next_buffer()
is invoked. If the user resets the state the callback is using to
return blocks and expects the next invocation of
read_stream_next_buffer() to return buffers with those blocks, they
will be disappointed.

If we try to address this by requiring that stream->distance is 0 when
read_stream_resume() is called, that won't work because while it is
set to 0 when the callback returns InvalidBlockNumber, there may still
be unreturned buffers in the stream.

If the user wants to use read_stream_reset() to exhaust the stream
before calling read_stream_resume(), read_stream_reset() sets
stream->distance to 1 at the end, so read_stream_resume() couldn't
detect if reset() was correctly called first or if the distance is > 0
because the stream is still in progress.

To make sure 1) distance isn't reset to a resume_distance from
read_stream_begin_relation() and 2) unexpected buffers aren't returned
from the read stream, we could error out in read_stream_resume() if
pinned_buffers > 0. And in read_stream_reset(), we would save distance
in resume_distance before clearing distance. That would allow calling
read_stream_resume() either if you called read_stream_reset() or if
you exhausted the stream yourself. See rough attached patch for a
sketch of this.

It would be nicer if we could error out if read_stream_next_buffer()
didn't return InvalidBuffer, but we can't do that if we want to allow
calling read_stream_reset() followed by read_stream_resume() because
distance won't be 0.

I tried this with a modified pgvector with an hnsw read stream user
and it seemed to work correctly.

- Melanie

--0000000000007a046f0643e4fe43
Content-Type: text/x-patch; charset="US-ASCII"; name="0001-resume.patch"
Content-Disposition: attachment; filename="0001-resume.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_mi52m3ce0>
X-Attachment-Id: f_mi52m3ce0

RnJvbSBjZjQxZjU2YmEwZDhjN2YzYmY5MjI0MjAyMGY3NDJmMjAxY2YwOGQ2IE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBNZWxhbmllIFBsYWdlbWFuIDxtZWxhbmllcGxhZ2VtYW5AZ21h
aWwuY29tPgpEYXRlOiBUdWUsIDE4IE5vdiAyMDI1IDE0OjI0OjEyIC0wNTAwClN1YmplY3Q6IFtQ
QVRDSF0gcmVzdW1lCgotLS0KIHNyYy9iYWNrZW5kL3N0b3JhZ2UvYWlvL3JlYWRfc3RyZWFtLmMg
fCAxOSArKysrKysrKysrKysrKysrKysrCiBzcmMvaW5jbHVkZS9zdG9yYWdlL3JlYWRfc3RyZWFt
LmggICAgIHwgIDEgKwogMiBmaWxlcyBjaGFuZ2VkLCAyMCBpbnNlcnRpb25zKCspCgpkaWZmIC0t
Z2l0IGEvc3JjL2JhY2tlbmQvc3RvcmFnZS9haW8vcmVhZF9zdHJlYW0uYyBiL3NyYy9iYWNrZW5k
L3N0b3JhZ2UvYWlvL3JlYWRfc3RyZWFtLmMKaW5kZXggMDMxZmRlOWY0Y2IuLjc1YmY5MmRjNjgz
IDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9zdG9yYWdlL2Fpby9yZWFkX3N0cmVhbS5jCisrKyBi
L3NyYy9iYWNrZW5kL3N0b3JhZ2UvYWlvL3JlYWRfc3RyZWFtLmMKQEAgLTEwMCw2ICsxMDAsNyBA
QCBzdHJ1Y3QgUmVhZFN0cmVhbQogCWludDE2CQlwaW5uZWRfYnVmZmVyczsKIAlpbnQxNgkJZGlz
dGFuY2U7CiAJaW50MTYJCWluaXRpYWxpemVkX2J1ZmZlcnM7CisJaW50MTYJCXJlc3VtZV9kaXN0
YW5jZTsKIAlpbnQJCQlyZWFkX2J1ZmZlcnNfZmxhZ3M7CiAJYm9vbAkJc3luY19tb2RlOwkJLyog
dXNpbmcgaW9fbWV0aG9kPXN5bmMgKi8KIAlib29sCQliYXRjaF9tb2RlOwkJLyogUkVBRF9TVFJF
QU1fVVNFX0JBVENISU5HICovCkBAIC00NjQsNiArNDY1LDcgQEAgcmVhZF9zdHJlYW1fbG9va19h
aGVhZChSZWFkU3RyZWFtICpzdHJlYW0pCiAJCWlmIChibG9ja251bSA9PSBJbnZhbGlkQmxvY2tO
dW1iZXIpCiAJCXsKIAkJCS8qIEVuZCBvZiBzdHJlYW0uICovCisJCQlzdHJlYW0tPnJlc3VtZV9k
aXN0YW5jZSA9IHN0cmVhbS0+ZGlzdGFuY2U7CiAJCQlzdHJlYW0tPmRpc3RhbmNlID0gMDsKIAkJ
CWJyZWFrOwogCQl9CkBAIC03MTEsNiArNzEzLDcgQEAgcmVhZF9zdHJlYW1fYmVnaW5faW1wbChp
bnQgZmxhZ3MsCiAJCXN0cmVhbS0+ZGlzdGFuY2UgPSBNaW4obWF4X3Bpbm5lZF9idWZmZXJzLCBz
dHJlYW0tPmlvX2NvbWJpbmVfbGltaXQpOwogCWVsc2UKIAkJc3RyZWFtLT5kaXN0YW5jZSA9IDE7
CisJc3RyZWFtLT5yZXN1bWVfZGlzdGFuY2UgPSBzdHJlYW0tPmRpc3RhbmNlOwogCiAJLyoKIAkg
KiBTaW5jZSB3ZSBhbHdheXMgYWNjZXNzIHRoZSBzYW1lIHJlbGF0aW9uLCB3ZSBjYW4gaW5pdGlh
bGl6ZSBwYXJ0cyBvZgpAQCAtODYyLDYgKzg2NSw3IEBAIHJlYWRfc3RyZWFtX25leHRfYnVmZmVy
KFJlYWRTdHJlYW0gKnN0cmVhbSwgdm9pZCAqKnBlcl9idWZmZXJfZGF0YSkKIAkJZWxzZQogCQl7
CiAJCQkvKiBObyBtb3JlIGJsb2NrcywgZW5kIG9mIHN0cmVhbS4gKi8KKwkJCXN0cmVhbS0+cmVz
dW1lX2Rpc3RhbmNlID0gc3RyZWFtLT5kaXN0YW5jZTsKIAkJCXN0cmVhbS0+ZGlzdGFuY2UgPSAw
OwogCQkJc3RyZWFtLT5vbGRlc3RfYnVmZmVyX2luZGV4ID0gc3RyZWFtLT5uZXh0X2J1ZmZlcl9p
bmRleDsKIAkJCXN0cmVhbS0+cGlubmVkX2J1ZmZlcnMgPSAwOwpAQCAtMTAzNCw2ICsxMDM4LDE5
IEBAIHJlYWRfc3RyZWFtX25leHRfYmxvY2soUmVhZFN0cmVhbSAqc3RyZWFtLCBCdWZmZXJBY2Nl
c3NTdHJhdGVneSAqc3RyYXRlZ3kpCiAJcmV0dXJuIHJlYWRfc3RyZWFtX2dldF9ibG9jayhzdHJl
YW0sIE5VTEwpOwogfQogCisvKgorICogUmVzdW1lIGxvb2tpbmcgYWhlYWQgYWZ0ZXIgdGhlIGJs
b2NrIG51bWJlciBjYWxsYmFjayByZXBvcnRlZCBlbmQtb2Ytc3RyZWFtLgorICogVGhpcyBpcyB1
c2VmdWwgZm9yIHN0cmVhbXMgb2Ygc2VsZi1yZWZlcmVudGlhbCBibG9ja3MsIGFmdGVyIGEgYnVm
ZmVyIG5lZWRlZAorICogdG8gYmUgY29uc3VtZWQgYW5kIGV4YW1pbmVkIHRvIGZpbmQgbW9yZSBi
bG9jayBudW1iZXJzLgorICovCit2b2lkCityZWFkX3N0cmVhbV9yZXN1bWUoUmVhZFN0cmVhbSAq
c3RyZWFtKQoreworCWlmIChzdHJlYW0tPnBpbm5lZF9idWZmZXJzID4gMCkKKwkJZWxvZyhFUlJP
UiwgInJlYWQgc3RyZWFtIG11c3QgYmUgZXhoYXVzdGVkIGJlZm9yZSByZXN1bWluZyIpOworCXN0
cmVhbS0+ZGlzdGFuY2UgPSBzdHJlYW0tPnJlc3VtZV9kaXN0YW5jZTsKK30KKwogLyoKICAqIFJl
c2V0IGEgcmVhZCBzdHJlYW0gYnkgcmVsZWFzaW5nIGFueSBxdWV1ZWQgdXAgYnVmZmVycywgYWxs
b3dpbmcgdGhlIHN0cmVhbQogICogdG8gYmUgdXNlZCBhZ2FpbiBmb3IgZGlmZmVyZW50IGJsb2Nr
cy4gIFRoaXMgY2FuIGJlIHVzZWQgdG8gY2xlYXIgYW4KQEAgLTEwNDcsNiArMTA2NCw4IEBAIHJl
YWRfc3RyZWFtX3Jlc2V0KFJlYWRTdHJlYW0gKnN0cmVhbSkKIAlCdWZmZXIJCWJ1ZmZlcjsKIAog
CS8qIFN0b3AgbG9va2luZyBhaGVhZC4gKi8KKwlpZiAoc3RyZWFtLT5kaXN0YW5jZSA+IDApCisJ
CXN0cmVhbS0+cmVzdW1lX2Rpc3RhbmNlID0gc3RyZWFtLT5kaXN0YW5jZTsKIAlzdHJlYW0tPmRp
c3RhbmNlID0gMDsKIAogCS8qIEZvcmdldCBidWZmZXJlZCBibG9jayBudW1iZXIgYW5kIGZhc3Qg
cGF0aCBzdGF0ZS4gKi8KZGlmZiAtLWdpdCBhL3NyYy9pbmNsdWRlL3N0b3JhZ2UvcmVhZF9zdHJl
YW0uaCBiL3NyYy9pbmNsdWRlL3N0b3JhZ2UvcmVhZF9zdHJlYW0uaAppbmRleCA5YjBkNjUxNjFk
MC4uZTI5YWM1MGZjOWUgMTAwNjQ0Ci0tLSBhL3NyYy9pbmNsdWRlL3N0b3JhZ2UvcmVhZF9zdHJl
YW0uaAorKysgYi9zcmMvaW5jbHVkZS9zdG9yYWdlL3JlYWRfc3RyZWFtLmgKQEAgLTk5LDYgKzk5
LDcgQEAgZXh0ZXJuIFJlYWRTdHJlYW0gKnJlYWRfc3RyZWFtX2JlZ2luX3NtZ3JfcmVsYXRpb24o
aW50IGZsYWdzLAogCQkJCQkJCQkJCQkJICAgUmVhZFN0cmVhbUJsb2NrTnVtYmVyQ0IgY2FsbGJh
Y2ssCiAJCQkJCQkJCQkJCQkgICB2b2lkICpjYWxsYmFja19wcml2YXRlX2RhdGEsCiAJCQkJCQkJ
CQkJCQkgICBzaXplX3QgcGVyX2J1ZmZlcl9kYXRhX3NpemUpOworZXh0ZXJuIHZvaWQgcmVhZF9z
dHJlYW1fcmVzdW1lKFJlYWRTdHJlYW0gKnN0cmVhbSk7CiBleHRlcm4gdm9pZCByZWFkX3N0cmVh
bV9yZXNldChSZWFkU3RyZWFtICpzdHJlYW0pOwogZXh0ZXJuIHZvaWQgcmVhZF9zdHJlYW1fZW5k
KFJlYWRTdHJlYW0gKnN0cmVhbSk7CiAKLS0gCjIuNDcuMwoK
--0000000000007a046f0643e4fe43--