Re: extensible external toast tuple support

On Fri, Jun 14, 2013 at 4:06 PM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

>
> Here's the updated version. It shouldn't contain any obvious WIP pieces
> anymore, although I think it needs some more documentation. I am just
> not sure where to add it yet, postgres.h seems like a bad place :/
>
>
I have a few comments and questions after reviewing this patch.

- heap_tuple_fetch_attr doesn't need to be updated to reflect ONDISK macro?
- I'm not sure if plural for datum is good to use. Datum values?
- -1 from me to use enum for tag types, as I don't think it needs to be.
This looks more like other "kind" macros such as relkind, but I know
there's pros/cons
- don't we need cast for tag value comparison in VARTAG_SIZE macro, since
tag is unit8 and enum is signed int?
- Is there better way to represent ONDISK size, instead of magic number
18? I'd suggest constructing it with sizeof(varatt_external).

Other than that, the patch applies fine and make check runs, though I don't
think the new code path is exercised well, as no one is creating INDIRECT
datum yet.

Also, I wonder how we could add more compression in datum, as well as how
we are going to add more compression options in database. I'd love to see
pluggability here, as surely the core cannot support dozens of different
compression algorithms, but because the data on disk is critical and we
cannot do anything like user defined functions. The algorithms should be
optional builtin so that once the system is initialized the the plugin
should not go away. Anyway pluggable compression is off-topic here.

Thanks,
--
Hitoshi Harada