Hi,
On 2024-04-08 02:25:17 +0300, Alexander Korotkov wrote:
> 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.
I know of at least 4 that have some production usage.
> And even if there are some, duplicating acquire_sample_rows() isn't a big
> deal.
I don't agree. The code has evolved a bunch over time, duplicating it into
various AMs is a bad idea.
> 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 this patch simply hasn't been reviewed even close to careful enough
and should be reverted. It's IMO to late for a redesign. Sorry for not
looking earlier, I was mostly out sick for the last few months.
I think a dedicated tableam callback for sample acquisition probably makes
sense, but if we want that, we need to provide an easy way for AMs that are
sufficiently block-like to reuse the code, not have two different ways to
implement analyze.
ISTM that ->relation_analyze is quite misleading as a name. For one, it it
just sets some callbacks, no? But more importantly, it sounds like it'd
actually allow to wrap the whole analyze process, rather than just the
acquisition of samples.
Greetings,
Andres Freund