Re: extensible external toast tuple support

On Tue, Jun 18, 2013 at 1:58 AM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> On 2013-06-18 00:56:17 -0700, Hitoshi Harada wrote:
> > 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.
>
> Cool!
>
> > - heap_tuple_fetch_attr doesn't need to be updated to reflect ONDISK
> macro?
>
> It calls toast_fetch_datum() for any kind of external datum, so it
> should be fine since ONDISK is handled in there.
>
>
toast_fetch_datum doesn't expect the input is INDIRECT. At least I see the
code path in the same file around toast_insert_or_update() where we have a
chance to (possibly accidentally) try to fetch ONDISK toasted value from
non-ONDISK datum.

> - -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
>
> Well, relkind cannot easily be a enum because it needs to be stored in a
> char field. I like using enums because it gives you the chance of using
> switch()es at some point and getting warned about missed cases.
>
> Why do you dislike it?
>
>
> I put -1 just because it doesn't have to be now. If you argue relkind is
char field, tag is also uint8.

> > - don't we need cast for tag value comparison in VARTAG_SIZE macro, since
> > tag is unit8 and enum is signed int?
>
> I think it should be fine (and the compiler doesn't warn) due to the
> integer promotion rules.
>
>
>
> - Is there better way to represent ONDISK size, instead of magic number
> > 18? I'd suggest constructing it with sizeof(varatt_external).
>
> I explicitly did not want to do that, since the numbers really don't
> have anything to do with the struct size anymore. Otherwise the next
> person reading that part will be confused because there's a 8byte struct
> with the enum value 1. Or somebody will try adding another type of
> external tuple that also needs 18 bytes by copying the sizeof(). Which
> will fail in ugly, non-obvious ways.
>
>
Sounds reasonable. I was just confused when I looked at it first.

> > 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.
>
> Yea, I only use the API in the changeset extraction patch. That actually
> worries me to. But I am not sure where to introduce usage of it in core
> without making the patchset significantly larger. I was thinking of
> adding an option to heap_form_tuple/heap_fill_tuple to allow it to
> reference _4B Datums instead of copying them, but that would require
> quite some adjustments on the callsites.
>
>
Maybe you can create a user-defined function that creates such datum for
testing, just in order to demonstrate it works fine.

> > 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.
>
> Separate patchset by now, yep ;).
>
>
I just found. Will look into it.

Thanks,