Re: Pluggable Storage - Andres's take

Hi,

On 2019-04-08 15:34:46 +0300, Heikki Linnakangas wrote:
> index_update_stats() calls RelationGetNumberOfBlocks(<table>). If the AM
> doesn't use normal data files, that won't work. I bumped into that with my
> toy implementation, which wouldn't need to create any data files, if it
> wasn't for this.

There are a few more of these:

1) index_update_stats(), computing pg_class.relpages

Feels like the number of both heap and index blocks should be
computed by the index build and stored in IndexInfo. That'd also get
a bit closer towards allowing indexams not going through smgr (useful
e.g. for memory only ones).

2) commands/analyze.c, computing pg_class.relpages

This should imo be moved to the tableam callback. It's currently done
a bit weirdly imo, with fdws computing relpages the callback, but
then also returning the acquirefunc. Seems like it should entirely be
computed as part of calling acquirefunc.

3) nodeTidscan, skipping over too large tids
I think this should just be moved into the AMs, there's no need to
have this in nodeTidscan.c

4) freespace.c, used for the new small-rels-have-no-fsm paths.
That's being revised currently anyway. But I'm not particularly
concerned even if it stays as is - freespace use is optional
anyway. And I can't quite see an AM that doesn't want to use
postgres' storage mechanism wanting to use freespace.c

Therefore I'm inclined not to thouch this independent of fixing the
others.

I think none of these are critical issues for tableam, but we should fix
them.

I'm not sure about doing so for v12 though. 1) and 3) are fairly
trivial, but 2) would involve changing the FDW interface, by changing
the AnalyzeForeignTable, AcquireSampleRowsFunc signatures. But OTOH,
we're not even in beta1.

Comments?

Greetings,

Andres Freund