On Tue, Jun 18, 2013 at 1:58 AM, Andres Freund <and...@2ndquadrant.com>wrote:
> On 2013-06-18 00:56:17 -0700, Hitoshi Harada wrote: > > On Fri, Jun 14, 2013 at 4:06 PM, Andres Freund <and...@2ndquadrant.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,
