Re: Pluggable toaster

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!

On Fri, Jul 22, 2022 at 4:17 PM Matthias van de Meent <
boekewurm+postgres(at)gmail(dot)com> wrote:

> On Wed, 20 Jul 2022 at 11:16, Nikita Malakhov <hukutoc(at)gmail(dot)com> wrote:
> >
> > Hi hackers!
>
> Hi,
>
> Please don't top-post here. See
> https://wiki.postgresql.org/wiki/Mailing_Lists#Email_etiquette_mechanics.
>
> > We really need your feedback on the last patchset update!
>
> This is feedback on the latest version that was shared on the mailing
> list here [0]. Your mail from today didn't seem to have an attachment,
> and I haven't checked the git repository for changes.
>
> 0001: Create Table Storage:
> LGTM
>
> ---
>
> 0002: Toaster API interface
>
> > tablecmds.c
>
> > SetIndexStorageProperties(Relation rel, Relation attrelation,
> > AttrNumber attnum,
> > bool setstorage, char newstorage,
> > bool setcompression, char newcompression,
> > + bool settoaster, Oid toasterOid,
> > LOCKMODE lockmode)
>
> Indexes cannot index toasted values, so why would the toaster oid be
> interesting for index storage properties?
>
> > static List *MergeAttributes(List *schema, List *supers, char
> relpersistence,
> > - bool is_partition, List **supconstr);
> > + bool is_partition, List **supconstr,
> > + Oid accessMethodId);
>
> > toasterapi.h:
>
> > +SearchTsrCache(Oid toasterOid)
> > ...
> > + for_each_from(lc, ToasterCache, 0)
> > + {
> > + entry = (ToasterCacheEntry*)lfirst(lc);
> > +
> > + if (entry->toasterOid == toasterOid)
> > + {
> > + /* remove entry from list, it will be added in a head of
> list below */
> > + foreach_delete_current(ToasterCache, lc);
> > + goto out;
> > + }
> > + }
>
> 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?
>
> > postgres.h
>
> > +/* varatt_custom uses 16bit aligment */
> 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?
>
> ---
>
> 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?
>
> 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.
>
> With the new API, I'm getting confused about the location of the
> various toast_* functions. They are spread around in various files
> that have no clear distinction on why it is (still) located there:
> some functions are moved to access/toast/*, while others are moved
> around in catalog/toasting.c, access/common/toast_internals.c and
> access/table/toast_helper.c.
>
> > detoast.c / tableam.h
> According to a quick search, all core usage of
> table_relation_fetch_toast_slice is removed. Shouldn't we remove that
> tableAM API (+ heapam implementation) instead of updating and
> maintaining it? Same question for table_relation_toast_am - I'm not
> sure that it remains the correct way of dealing with toast.
>
> > toast_helper.c
>
> toast_delete_external_datum:
> Please clean up code that was commented out from the patches, it
> detracts from the readability of a patch.
>
> > toast_internals.c
>
> This seems like a bit of a mess, considering the lack of
> Can't we split this up into a heaptoast (or whatever we're going to
> call the default toaster) and actual toast internals? It seems to me
> that
>
> > generic_toaster.c
>
> Could you align name styles in this new file? It has both camelCase
> and snake_case for function names.
>
> > toasting.c
>
> I'm not entirely sure that we should retain catalog/toasting.c if we
> are going to depend on the custom toaster API. Shouldn't the creation
> of toast tables be delegated to the toaster?
>
> > + * toast_get_valid_index
> > + *
> > + * Get OID of valid index associated to given toast relation. A toast
> > + * relation can have only one valid index at the same time.
>
> 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.
>
> ---
>
> 0004: refactoring and optimization of default toaster
> 0005: bytea appendable toaster
>
> I dind't review these yet.
>
> ---
>
> 0006: docs
>
> Seems like a good start, but I'm not sure that we need the struct
> definition in the docs. I think the BRIN extensibility docs [1] are a
> better example on what I think the documentation for this API should
> look like.
>
> ---
>
> 0007: fix alignment of custom toast pointers
> This is not a valid fix for the alignment requirement for custom toast
> pointers. You now leave one byte empty if you are not already aligned,
> which for on-disk toast pointers means that we're dealing with a
> 4-byte aligned value, which is not the case because this is a
> 2-byte-aligned value.
>
> Regardless, this should be merged into 0002, not remain a seperate patch.
>
> ---
>
> 0008: fix tuple externalization
> Should be merged into the relevant patch as well, not as a separate patch.
>
> ---
>
> > Currently I'm working on an update to the default Toaster (some internal
> optimizations, not affecting functionality)
> > and readme files explaining Pluggable TOAST.
>
> That would be greatly appreciated - 0006 does not cover why we need
> vtable, nor how it's expected to be used in type-aware code.
>
>
>
> Kind regards,
>
> Matthias van de Meent
>
> [0]
> https://www.postgresql.org/message-id/CAN-LCVNkU%2Bkdieu4i_BDnLgGszNY1RCnL6Dsrdz44fY7FOG3vg%40mail.gmail.com
> [1] https://www.postgresql.org/docs/15/brin-extensibility.html
>

--
Regards,
Nikita Malakhov
Postgres Professional
https://postgrespro.ru/