Re: Pluggable toaster - Mailing list pgsql-hackers

Hi hackers!

Matthias, thank you very much for your feedback!

Sorry, I forgot to attach files.

Attaching here, but they are for the commit tagged "15beta2", I am currently 

rebasing this branch onto the actual master and will provide rebased version,

with some corrections according to your feedback, in a day or two.

>Indexes cannot index toasted values, so why would the toaster oid be
>interesting for index storage properties?

Here Teodor might correct me. Toast tables are indexed, and Heap TOAST 

mechanics accesses Toasted tuples by index, isn't it the case?

>Moving toasters seems quite expensive when compared to just index
>checks. When you have many toasters, but only a few hot ones, this
>currently will move the "cold" toasters around a lot. Why not use a
>stack instead (or alternatively, a 'zipper' (or similar) data
>structure), where the hottest toaster is on top, so that we avoid
>larger memmove calls?

This is a reasonable remark, we'll consider it for the next iteration. Our reason

is that we think there won't be a lot of custom Toasters, most likely less then

a dozen, for the most complex/heavy datatypes so we haven't considered 

these expenses.

>To the best of my knowledge varlena-pointers are unaligned; and this
>is affirmed by the comment immediately under varattrib_1b_e. Assuming
>alignment to 16 bits should/would be incorrect in some of the cases.
>This is handled for normal varatt_external values by memcpy-ing the
>value into local (aligned) fields before using them, but that doesn't
>seem to be the case for varatt_custom?

Alignment correction seemed reasonable for us because structures are

anyway aligned in memory, so when we use 1 and 2-byte fields along 

with 4-byte it may create a lot of padding. Am I wrong? Also, correct 

alignment somewhat simplifies access to structure fields.

>0003: (re-implement default toaster using toaster interface)
>I see significant changes to the dummy toaster (created in 0002),
>could those be moved to patch 0002 in the next iteration?

Will do.

>detoast.c and detoast.h are effectively empty after this patch (only
>imports and commented-out code remain), please fully remove them
>instead - that saves on patch diff size.

Will do.

About the location of toast_ functions: these functions are part of Heap 

TOAST mechanics, and they were scattered among other Heap internals

sources. I've tried to gather them and put them in more straight order, but 

this work is not fully finished yet and will take some time. Working on it.

I'll check if table_relation_fetch_toast_slice could be removed, thanks for

the remark.

toast_helper - done, will be provided in rebased version.

toast_internals - this one is an internal part of TOAST implemented in 

Heap AM, but I'll try to straighten it out as much as I could.

naming conventions in some sources - done, will be provided in rebased

patch set.

>Shouldn't the creation of toast tables be delegated to the toaster?

Yes, you're right, and actually, it is. I'll check that and correct in rebased

version.

>Although this is code being moved around, the comment is demonstrably
>false: A cancelled REINDEX CONCURRENTLY with a subsequent REINDEX can
>leave a toast relation with 2 valid indexes.

This code is quite old, we've not changed it but thanks for the remark, 

I'll check it more carefully.

Small fixes are already merged into larger patches in attached files. Also,

I appreciate your feedback on documentation - if you would have an opportunity

please check README provided in 0003. I've took your comments on documentation

into account and will include corrections according to them into rebased patch.

As Aleksander recommended, I've shortened the patch set and left only the most

important part - API and re-implemented default Toast. All bells and whistles are not

of so much importance and could be sent later after the API itself will be straightened

out and commited.

Thank you very much!