Re: extensible external toast tuple support

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,