Re: Pluggable toaster

Hi hackers!

Here is the patch set rebased to master from 22.07. I've got some trouble
rebasing it due to
conflicts, so it took some time.
I've made some corrections according to Matthias and Aleksander comments,
though not all
of them, because some require refactoring of very old code and would have
to take much more
effort. I keep these recommended corrections in mind and already working on
them but it will
require extensive testing and some more work, so the will be presented
later, in next iteration
or next patch - these are optimization of heap AM according
table_relation_fetch_toast_slice,
double-index problem and I'm continue to straighten out code related to
TOAST functionality.
It's quite a task because as I mentioned before, this core was scattered
over Heap AM and
reference implementation of TOAST is very tightly intertwined with Heap AM
itself. Default
toaster uses Heap AM storage so it is unlikely that it will be possible to
fully detach it from
Heap.

However, I've made some more refactoring, removed empty sources, corrected
code according
to naming conventions, and extended README.toastapi document.

0002_toaster_interface_v10 contains TOAST API with Dummy toaster as an
example (but I would,
as recommended, remove Dummy toaster and provide it as an extension), and
default Toaster
was left as-is (reference implementation).

0003_toaster_default_v9 implements reference TOAST as Default Toaster via
TOAST API,
so Heap AM calls Toast only via API, and does not have direct calls to
Toast functionality.

0004_toaster_snapshot_v8 continues refactoring and has some important
changes (added
into README.toastapi new part related TOAST API extensibility - the virtual
functions table).

Also, I'll provide documentation package corrected according to Matthias'
remarks later,
in the next patch set.

Please check attached patch set.
Also, GIT branch with this patch resides here:
https://github.com/postgrespro/postgres/tree/toasterapi_clean

Thank all reviewers for feedback!

On Sat, Jul 23, 2022 at 10:15 AM Nikita Malakhov <hukutoc(at)gmail(dot)com> wrote:

> 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!
>
> --
Regards,
Nikita Malakhov
Postgres Professional
https://postgrespro.ru/