Re: Pluggable Storage - Andres's take

Hi,

On 2019-03-10 05:49:26 +0100, Dmitry Dolgov wrote:
> > On Sat, Mar 9, 2019 at 4:13 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> >
> > While 0001 is pretty bulky, the interesting bits concentrate on a
> > comparatively small area. I'd appreciate if somebody could give the
> > comments added in tableam.h a read (both on callbacks, and their
> > wrappers, as they have different audiences).
>
> Potentially stupid question, but I'm curious about this one (couldn't find any
> discussion about it):

Not stupid...

> +/*
> + * Generic descriptor for table scans. This is the base-class for
> table scans,
> + * which needs to be embedded in the scans of individual AMs.
> + */
> +typedef struct TableScanDescData
> // ...
> bool rs_pageatatime; /* verify visibility page-at-a-time? */
> bool rs_allow_strat; /* allow or disallow use of access strategy */
> bool rs_allow_sync; /* allow or disallow use of syncscan */
>
> + * allow_{strat, sync, pagemode} specify whether a scan strategy,
> + * synchronized scans, or page mode may be used (although not every AM
> + * will support those).
> // ...
> + TableScanDesc (*scan_begin) (Relation rel,
>
> The last commentary makes me think that those flags (allow_strat / allow_sync /
> pageatime) are more like AM specific, shouldn't they live in HeapScanDescData?

They're common enough across AMs, but more importantly calling code
currently specifies them in several places. As they're thus essentially
generic, rather than AM specific, I think it makes sense to have them in
the general scan struct.

Greetings,

Andres Freund