On Mon, Apr 15, 2024 at 12:37 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > In my understanding, the downside of 041b96802ef is bringing read_stream* things from being heap-only-related up to the level of acquire_sample_rows() that is not supposed to be tied to heap. And changing *_analyze_next_block() function signature to use ReadStream explicitly in the signature.
I don't think that really clarifies anything. The ReadStream is basically just acting as a wrapper for a stream of block numbers, and the API took a BlockNumber before. So why does it make any difference?
If I understand correctly, Alexander thinks that, before 041b96802ef, the block number didn't necessarily have to be the physical block number on disk, but could instead be any 32-bit quantity that the table AM wanted to pack into the block number. But I don't think that's true, because acquire_sample_rows() was already passing those block numbers to PrefetchBuffer(), which already requires physical block numbers.
Hi, Robert!
Why it makes a difference looks a little bit unclear to me, I can't comment on this. I noticed that before 041b96802ef we had a block number and block sampler state that tied acquire_sample_rows() to the actual block structure. After we have the whole struct ReadStream which doesn't comprise just a wrapper for the same variables, but the state that ties acquire_sample_rows() to the streaming read algorithm (and heap). Yes, we don't have other access methods other than heap implemented for analyze routine, so the patch works anyway, but from the view on acquire_sample_rows() as a general method that is intended to have different implementations in the future it doesn't look good.
It's my impression on 041b96802ef, please forgive me if I haven't understood something.