On Mon, Apr 8, 2024 at 10:18 AM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > On Mon, 8 Apr 2024 at 03:25, Alexander Korotkov <aekorotkov@gmail.com> wrote: >> >> Hi, >> >> On Mon, Apr 8, 2024 at 12:40 AM Andres Freund <andres@anarazel.de> wrote: >> > On 2024-03-30 23:33:04 +0200, Alexander Korotkov wrote: >> > > I've pushed 0001, 0002 and 0006. >> > >> > I briefly looked at 27bc1772fc81 and I don't think the state post this commit >> > makes sense. Before this commit another block based AM could implement analyze >> > without much code duplication. Now a large portion of analyze.c has to be >> > copied, because they can't stop acquire_sample_rows() from calling >> > heapam_scan_analyze_next_block(). >> > >> > I'm quite certain this will break a few out-of-core AMs in a way that can't >> > easily be fixed. >> >> I was under the impression there are not so many out-of-core table >> AMs, which have non-dummy analysis implementations. And even if there >> are some, duplicating acquire_sample_rows() isn't a big deal. >> >> But given your feedback, I'd like to propose to keep both options >> open. Turn back the block-level API for analyze, but let table-AM >> implement its own analyze function. Then existing out-of-core AMs >> wouldn't need to do anything (or probably just set the new API method >> to NULL). > > I think that providing both new and old interface functions for block-based and non-block based custom am is an excellent compromise. > > The patch v1-0001-Turn-back.. is mainly an undo of part of the 27bc1772fc81 that had turned off _analyze_next_tuple..analyze_next_block for external callers. If some extensions are already adapted to the old interface functions, they are free to still use it.
Please, check this. Instead of keeping two APIs, it generalizes acquire_sample_rows(). The downside is change of AcquireSampleRowsFunc signature, which would need some changes in FDWs too.
To me, both approaches v1-0001-Turn-back... and v2-0001-Generalize... and patch v2 look good.