Re: Pluggable toaster

On Wed, 20 Jul 2022 at 11:16, Nikita Malakhov <hukutoc@gmail.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