Обсуждение: Pluggable toaster

On Thu, 30 Dec 2021 at 16:40, Teodor Sigaev <teodor@sigaev.ru> wrote:

> We are working on custom toaster for JSONB [1], because current TOAST is
> universal for any data type and because of that it has some disadvantages:
>     - "one toast fits all"  may be not the best solution for particular
>       type or/and use cases
>     - it doesn't know the internal structure of data type, so it  cannot
>       choose an optimal toast strategy
>     - it can't  share common parts between different rows and even
>       versions of rows

Agreed, Oleg has made some very clear analysis of the value of having
a higher degree of control over toasting from within the datatype.

In my understanding, we want to be able to
1. Access data from a toasted object one slice at a time, by using
knowledge of the structure
2. If toasted data is updated, then update a minimum number of
slices(s), without rewriting the existing slices
3. If toasted data is expanded, then allownew slices to be appended to
the object without rewriting the existing slices

> Modification of current toaster for all tasks and cases looks too
> complex, moreover, it  will not works for  custom data types. Postgres
> is an extensible database,  why not to extent its extensibility even
> further, to have pluggable TOAST! We  propose an idea to separate
> toaster from  heap using  toaster API similar to table AM API etc.
> Following patches are applicable over patch in [1]

ISTM that we would want the toast algorithm to be associated with the
datatype, not the column?
Can you explain your thinking?

We already have Expanded toast format, in-memory, which was designed
specifically to allow us to access sub-structure of the datatype
in-memory. So I was expecting to see an Expanded, on-disk, toast
format that roughly matched that concept, since Tom has already shown
us the way. (varatt_expanded). This would be usable by both JSON and
PostGIS.


Some other thoughts:

I imagine the data type might want to keep some kind of dictionary
inside the main toast pointer, so we could make allowance for some
optional datatype-specific private area in the toast pointer itself,
allowing a mix of inline and out-of-line data, and/or a table of
contents to the slices.

I'm thinking could also tackle these things at the same time:
* We want to expand TOAST to 64-bit pointers, so we can have more
pointers in a table
* We want to avoid putting the data length into the toast pointer, so
we can allow the toasted data to be expanded without rewriting
everything (to avoid O(N^2) cost)

--
Simon Riggs                http://www.EnterpriseDB.com/

On 1/14/22 19:41, Teodor Sigaev wrote:
>
>> In my understanding, we want to be able to
>> 1. Access data from a toasted object one slice at a time, by using
>> knowledge of the structure
>> 2. If toasted data is updated, then update a minimum number of
>> slices(s), without rewriting the existing slices
>> 3. If toasted data is expanded, then allownew slices to be appended to
>> the object without rewriting the existing slices
>
> There are more options:
> 1 share common parts between not only versions of row but between all
> rows in a column. Seems strange but examples:
>    - urls often have a common prefix and so storing in a prefix tree (as
>      SP-GiST does) allows significantly decrease storage size
>    - the same for json - it's often use case with common part of its
>      hierarchical structure
>    - one more usecase for json. If json use only a few schemes
>      (structure) it's possible to store in toast storage only values and
>      don't store keys and structure

This sounds interesting, but very much like column compression, which
was proposed some time ago. If we haven't made much progrees with that
patch (AFAICS), what's the likelihood we'll succeed here, when it's
combined with yet more complexity?

Maybe doing that kind of compression in TOAST is somehow simpler, but I
don't see it.

> 2 Current toast storage stores chunks in heap accesses method and to
> provide fast access by toast id it makes an index. Ideas:
>    - store chunks directly in btree tree, pgsql's btree already has an
>      INCLUDE columns, so, chunks and visibility data will be stored only
>      in leaf pages. Obviously it reduces number of disk's access for
>      "untoasting".
>    - use another access method for chunk storage
>

Maybe, but that probably requires more thought - e.g. btree requires the
values to be less than 1/3 page, so I wonder how would that play with
toasting of values.

>> ISTM that we would want the toast algorithm to be associated with the
>> datatype, not the column?
>> Can you explain your thinking?
> Hm. I'll try to explain my motivation.
> 1) Datatype could have more than one suitable toasters. For different
>     usecases: fast retrieving, compact storage, fast update etc. As I
>     told   above, for jsonb there are several optimal strategies for
>     toasting:   for values with a few different structures, for close to
>     hierarchical structures,  for values with different parts by access
>     mode (easy to imagine json with some keys used for search and some
>     keys only for   output to user)
> 2) Toaster could be designed to work with different data type. Suggested
>     appendable toaster is designed to work with bytea but could work with
>     text
>
> Looking on this point I have doubts where to store connection between
> toaster and datatype. If we add toasteroid to pg_type how to deal with
> several toaster for one datatype? (And we could want to has different
> toaster on one table!) If we add typoid to pg_toaster then how it will
> work with several datatypes? An idea to add a new many-to-many
> connection table seems workable but here there are another questions,
> such as will any toaster work with any table access method?
>
> To resolve this bundle of question we propose validate() method of
> toaster, which should be called during DDL operation, i.e. toaster is
> assigned to column or column's datatype is changed.
>

Seems you'd need a mapping table, to allow M:N mapping between types and
toasters, linking it to all "compatible" types. It's not clear to me how
would this work with custom data types, domains etc.

Also, what happens to existing values when you change the toaster? What
if the toasters don't use the same access method to store the chunks
(heap vs. btree)? And so on.

> More thought:
> Now postgres has two options for column: storage and compression and now
> we add toaster. For me it seems too redundantly. Seems, storage should
> be binary value: inplace (plain as now) and toastable. All other
> variation such as toast limit, compression enabling, compression kind
> should be an per-column option for toaster (that's why we suggest valid
> toaster oid for any column with varlena/toastable datatype). It looks
> like a good abstraction but we will have a problem with backward
> compatibility and I'm afraid I can't implement it very fast.
>

So you suggest we move all of this to toaster? I'd say -1 to that,
because it makes it much harder to e.g. add custom compression method, etc.


regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 1/18/22 15:56, Teodor Sigaev wrote:
> Hi!
>
>> Maybe doing that kind of compression in TOAST is somehow simpler, but
>> I don't see it.
> Seems, in ideal world, compression should be inside toaster.
>

I'm not convinced that's universally true. Yes, I'm sure certain TOAST
implementations would benefit from tighter control over compression, but
does that imply compression and toast are redundant? I doubt that,
because we compress non-toasted types too, for example. And layering has
a value too, as makes it easier to replace the pieces.

>>
>>> 2 Current toast storage stores chunks in heap accesses method and to
>>> provide fast access by toast id it makes an index. Ideas:
>>>    - store chunks directly in btree tree, pgsql's btree already has an
>>>      INCLUDE columns, so, chunks and visibility data will be stored only
>>>      in leaf pages. Obviously it reduces number of disk's access for
>>>      "untoasting".
>>>    - use another access method for chunk storage
>>>
>>
>> Maybe, but that probably requires more thought - e.g. btree requires
>> the values to be less than 1/3 page, so I wonder how would that play
>> with toasting of values.
> That's ok, because chunk size is 2000 bytes right now and its could be
> saved.
>>

Perhaps. My main point is that we should not be making too many radical
changes at once - it makes it much harder to actually get anything done.
So yeah, doing TOAST through IOT might be interesting, but I'd leave
that for a separate patch.

>
>> Seems you'd need a mapping table, to allow M:N mapping between types
>> and toasters, linking it to all "compatible" types. It's not clear to
>> me how would this work with custom data types, domains etc.
> If toaster will look into internal structure then it should know type's
> binary format. So, new custom types have a little chance to work with
> old custom toaster. Default toaster works with any types.

The question is what happens when you combine data type with a toaster
that is not designed for that type. I mean, imagine you have a JSONB
toaster and you set it for a bytea column. Naive implementation will
just crash, because it'll try to process bytea as if it was JSONB.

It seems better to prevent such incompatible combinations and restrict
each toaster to just compatible data types, and the mapping table
(linking toaster and data types) seems a way to do that.

However, it seems toasters are either generic (agnostic to data types,
treating everything as bytea) or specialized. I doubt any specialized
toaster can reasonably support multiple data types, so maybe each
toaster can have just one "compatible type" OID. If it's invalid, it'd
be "generic" and otherwise it's useful for that type and types derived
from it (e.g. domains).

So you'd have the toaster OID in two places:

pg_type.toaster_oid      - default toaster for the type
pg_attribute.toaster_oid - current toaster for this column

and then you'd have

pg_toaster.typid - type this toaster handles (or InvalidOid for generic)


>>
>> Also, what happens to existing values when you change the toaster?
>> What if the toasters don't use the same access method to store the
>> chunks (heap vs. btree)? And so on.
>
> vatatt_custom contains an oid of toaster and toaster is not allowed to
> delete (at least, in suggested patches). So, if column's toaster has
> been changed then old values will be detoasted  by toaster pointed in
> varatt_custom structure, not in column definition. This is very similar
> to storage attribute works: we we alter storage attribute only new
> values will be stored with pointed storage type.
>

IIRC we do this for compression methods, right?

>>
>>> More thought:
>>> Now postgres has two options for column: storage and compression and
>>> now we add toaster. For me it seems too redundantly. Seems, storage
>>> should be binary value: inplace (plain as now) and toastable. All
>>> other variation such as toast limit, compression enabling,
>>> compression kind should be an per-column option for toaster (that's
>>> why we suggest valid toaster oid for any column with
>>> varlena/toastable datatype). It looks like a good abstraction but we
>>> will have a problem with backward compatibility and I'm afraid I
>>> can't implement it very fast.
>>>
>>
>> So you suggest we move all of this to toaster? I'd say -1 to that,
>> because it makes it much harder to e.g. add custom compression method,
>> etc.
> Hmm, I suggested to leave only toaster at upper level. Compression kind
> could be chosen in toaster's options (not implemented yet) or even make
> an API interface to compression to make it configurable. Right now,
> module developer could not implement a module with new compression
> method and it is a disadvantage.

If you have to implement custom toaster to implement custom compression
method, doesn't that make things more complex? You'd have to solve all
the issues for custom compression methods and also all issues for custom
toaster. Also, what if you want to just compress the column, not toast?


regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> I agree ... but I'm also worried about what happens when we have
> multiple table AMs. One can imagine a new table AM that is
> specifically optimized for TOAST which can be used with an existing
> heap table. One can imagine a new table AM for the main table that
> wants to use something different for TOAST. So, I don't think it's
> right to imagine that the choice of TOASTer depends solely on the
> column data type. I'm not really sure how this should work exactly ...
> but it needs careful thought.

Right. that's why we propose a validate method  (may be, it's a wrong
name, but I don't known better one) which accepts several arguments, one
of which is table AM oid. If that method returns false then toaster
isn't useful with current TAM, storage or/and compression kinds, etc.

--
Teodor Sigaev                      E-mail: teodor@sigaev.ru
                                       WWW: http://www.sigaev.ru/

> I agree ... but I'm also worried about what happens when we have
> multiple table AMs. One can imagine a new table AM that is
> specifically optimized for TOAST which can be used with an existing
> heap table. One can imagine a new table AM for the main table that
> wants to use something different for TOAST. So, I don't think it's
> right to imagine that the choice of TOASTer depends solely on the
> column data type. I'm not really sure how this should work exactly ...
> but it needs careful thought.

Right. that's why we propose a validate method  (may be, it's a wrong
name, but I don't known better one) which accepts several arguments, one
of which is table AM oid. If that method returns false then toaster
isn't useful with current TAM, storage or/and compression kinds, etc.

--
Teodor Sigaev                      E-mail: teodor@sigaev.ru
                                       WWW: http://www.sigaev.ru/

Hi,

On 2022-03-22 02:31:21 +0300, Nikita Malakhov wrote:
> Hi Hackers,
> Because of 3 months have passed since Pluggable Toaster presentation and a
> lot of
> commits were pushed into v15 master - we would like to re-introduce
> this patch
> rebased onto actual master. Last commit being used -
> commit 641f3dffcdf1c7378cfb94c98b6642793181d6db (origin/master)
> Author: Tom Lane <tgl@sss.pgh.pa.us>
> Date:   Fri Mar 11 13:47:26 2022 -0500

It currently fails to apply: http://cfbot.cputube.org/patch_37_3490.log

Given the size of the patch, and the degree of review it has gotten so far, it
seems not realistically a fit for 15, but is marked as such.

Think it should be moved to the next CF. Marked as waiting-on-author for now.

Greetings,

Andres Freund

It looks like it's still not actually building on some compilers. I
see a bunch of warnings as well as an error:

[03:53:24.660] dummy_toaster.c:97:2: error: void function
'dummyDelete' should not return a value [-Wreturn-type]

Also the "publication" regression test needs to be adjusted as it
includes \d+ output which has changed to include the toaster.

Hi,

On April 2, 2022 6:20:36 PM PDT, Greg Stark <stark@mit.edu> wrote:
>Hm. It compiles but it's failing regression tests:
>
>diff -U3 /tmp/cirrus-ci-build/contrib/dummy_toaster/expected/dummy_toaster.out
>/tmp/cirrus-ci-build/contrib/dummy_toaster/results/dummy_toaster.out
>--- /tmp/cirrus-ci-build/contrib/dummy_toaster/expected/dummy_toaster.out
>2022-04-02 16:02:47.874360253 +0000
>+++ /tmp/cirrus-ci-build/contrib/dummy_toaster/results/dummy_toaster.out
>2022-04-02 16:07:20.878047769 +0000
>@@ -20,186 +20,7 @@
>....
>+server closed the connection unexpectedly
>+ This probably means the server terminated abnormally
>+ before or while processing the request.
>+connection to server was lost
>I think this will require some real debugging, so I'm marking this
>Waiting on Author.

Yes, dumps core (just like in several previous runs):

https://cirrus-ci.com/task/4710272324599808?logs=cores#L44

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Hi,

On April 2, 2022 6:20:36 PM PDT, Greg Stark <stark@mit.edu> wrote:
>Hm. It compiles but it's failing regression tests:
>
>diff -U3 /tmp/cirrus-ci-build/contrib/dummy_toaster/expected/dummy_toaster.out
>/tmp/cirrus-ci-build/contrib/dummy_toaster/results/dummy_toaster.out
>--- /tmp/cirrus-ci-build/contrib/dummy_toaster/expected/dummy_toaster.out
>2022-04-02 16:02:47.874360253 +0000
>+++ /tmp/cirrus-ci-build/contrib/dummy_toaster/results/dummy_toaster.out
>2022-04-02 16:07:20.878047769 +0000
>@@ -20,186 +20,7 @@
>....
>+server closed the connection unexpectedly
>+ This probably means the server terminated abnormally
>+ before or while processing the request.
>+connection to server was lost
>I think this will require some real debugging, so I'm marking this
>Waiting on Author.

Yes, dumps core (just like in several previous runs):

https://cirrus-ci.com/task/4710272324599808?logs=cores#L44

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

On Mon, Apr 4, 2022 at 12:13 PM Nikita Malakhov <hukutoc@gmail.com> wrote:
> I'm really sorry. Messed up some code while merging rebased branches with previous (v1)
> patches issued in December and haven't noticed that seg fault because of corrupted code
> while running check-world.
> I've fixed messed code in Dummy Toaster package and checked twice - all's correct now,
> patches are applied correctly and tests are clean.
> Thank you for pointing out the error and for your patience!

Hi,

This patch set doesn't seem anywhere close to committable to me. For example:

- It apparently adds a new command called CREATE TOASTER but that
command doesn't seem to be documented anywhere.

- contrib/dummy_toaster is added in patch 1 with a no implementation
and code comments that say "Bloom index utilities" and then those
comments are fixed and an implementation is added in later patches.

- What is supposedly patch 1 is actually 32 patch files concatenated
together. It doesn't apply properly either with 'patch' or 'git am' so
I don't understand what we would even do with this.

- None of these patches have appropriate commit messages.

- Many of these patches add 'XXX' comments or errors or other
obviously unfinished bits of code. Some of this may be cleaned up by
later patches but it's hard to tell because right now one can't even
apply the patch set properly. Even if that were possible, it's the job
of the person submitting the patch to organize the patch into
independent, separately committable chunks that are self-contained and
have good comments and good commit messages for each one.

I don't think based on the status of this patch set that it's even
possible to provide useful feedback on the design at this point, never
mind getting anything committed.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Apr 4, 2022 at 4:05 PM Nikita Malakhov <hukutoc@gmail.com> wrote:
> - Is 'git apply' not a valid way to apply such patches?

I have found that it never works. This case is no exception:

[rhaas pgsql]$ git apply ~/Downloads/1_toaster_interface_v4.patch
/Users/rhaas/Downloads/1_toaster_interface_v4.patch:253: trailing whitespace.
toasterapi.o
/Users/rhaas/Downloads/1_toaster_interface_v4.patch:1276: trailing whitespace.
{
/Users/rhaas/Downloads/1_toaster_interface_v4.patch:1294: trailing whitespace.
 * CREATE TOASTER name HANDLER handler_name
/Users/rhaas/Downloads/1_toaster_interface_v4.patch:2261: trailing whitespace.
 * va_toasterdata could contain varatt_external structure for old Toast
/Users/rhaas/Downloads/1_toaster_interface_v4.patch:3047: trailing whitespace.
SELECT attnum, attname, atttypid, attstorage, tsrname
error: patch failed: src/backend/commands/tablecmds.c:42
error: src/backend/commands/tablecmds.c: patch does not apply
error: patch failed: src/backend/commands/tablecmds.c:943
error: src/backend/commands/tablecmds.c: patch does not apply
error: patch failed: src/backend/commands/tablecmds.c:973
error: src/backend/commands/tablecmds.c: patch does not apply
error: patch failed: src/backend/commands/tablecmds.c:44
error: src/backend/commands/tablecmds.c: patch does not apply

I would really encourage you to use 'git format-patch' to generate a
stack of patches. But there is no point in reposting 30+ patches that
haven't been properly refactored into separate chunks. You need to
maintain a branch, periodically rebased over master, with some
probably-small number of patches on it, each of which is a logically
independent patch with its own commit message, its own clear purpose,
etc. And then generate patches to post from there using 'git
format-patch'. Look into using 'git rebase -i --autosquash' and 'git
commit --fixup' to maintain the branch, if you're not already familiar
with those things.

Also, it is a really good idea when you post the patch set to include
in the email a clear description of the overall purpose of the patch
set and what each patch does toward that goal. e.g. "The overall goal
of this patch set is to support faster-than-light travel. Currently,
PostgreSQL does not know anything about the speed of light, so 0001
adds some code for speed-of-light detection. Building on this, 0002
adds general support for disabling physical laws of the universe.
Then, 0003 makes use of this support to disable specifically the speed
of light." Perhaps you want a little more text than that for each
patch, depending on the situation, but this gives you the idea, I
hope.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

I reworked previous patch set according to recommendations. Patches

are generated by format-patch and applied by git am. Patches are based on

master from 03.11. Also, now we've got clean branch with incremental commits 

which could be easily rebased onto a fresh master.

Currently, there are 8 patches:

1) 0001_create_table_storage_v3.patch - SET STORAGE option for CREATE

TABLE command by Teodor Sigaev which is required by all the following functionality;

2) 0002_toaster_interface_v6.patch - Toaster API (SQL syntax for toasters + API) 

with Dummy toaster as an example of how this API should be used, but with default 

toaster left 'as-is';

3) 0003_toaster_default_v5.patch - default (regular) toaster is implemented

via new API;

4) 0004_toaster_snapshot_v5.patch - refactoring of default toaster and support

of versioned toasted rows;

5) 0005_bytea_appendable_toaster_v5.patch - bytea toaster by Nikita Glukhov

Custom toaster for bytea data with support of appending (instead of rewriting)

stored data;

6) 0006_toasterapi_docs_v1.patch - brief documentation on Toaster API in Pg docs;

7) 0007_fix_alignment_of_custom_toast_pointers.patch - fixes custom toast pointer's

alignment required by bytea toaster by Nikita Glukhov;

8) 0008_fix_toast_tuple_externalize.patch - fixes toast_tuple_externalize function 

not to call toast if old data is the same as new one.

I would be grateful for feedback on the reworked patch set.

On Mon, Apr 4, 2022 at 11:18 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Apr 4, 2022 at 4:05 PM Nikita Malakhov <hukutoc@gmail.com> wrote:
> - Is 'git apply' not a valid way to apply such patches?

I have found that it never works. This case is no exception:

[rhaas pgsql]$ git apply ~/Downloads/1_toaster_interface_v4.patch
/Users/rhaas/Downloads/1_toaster_interface_v4.patch:253: trailing whitespace.
toasterapi.o
/Users/rhaas/Downloads/1_toaster_interface_v4.patch:1276: trailing whitespace.
{
/Users/rhaas/Downloads/1_toaster_interface_v4.patch:1294: trailing whitespace.
 * CREATE TOASTER name HANDLER handler_name
/Users/rhaas/Downloads/1_toaster_interface_v4.patch:2261: trailing whitespace.
 * va_toasterdata could contain varatt_external structure for old Toast
/Users/rhaas/Downloads/1_toaster_interface_v4.patch:3047: trailing whitespace.
SELECT attnum, attname, atttypid, attstorage, tsrname
error: patch failed: src/backend/commands/tablecmds.c:42
error: src/backend/commands/tablecmds.c: patch does not apply
error: patch failed: src/backend/commands/tablecmds.c:943
error: src/backend/commands/tablecmds.c: patch does not apply
error: patch failed: src/backend/commands/tablecmds.c:973
error: src/backend/commands/tablecmds.c: patch does not apply
error: patch failed: src/backend/commands/tablecmds.c:44
error: src/backend/commands/tablecmds.c: patch does not apply

I would really encourage you to use 'git format-patch' to generate a
stack of patches. But there is no point in reposting 30+ patches that
haven't been properly refactored into separate chunks. You need to
maintain a branch, periodically rebased over master, with some
probably-small number of patches on it, each of which is a logically
independent patch with its own commit message, its own clear purpose,
etc. And then generate patches to post from there using 'git
format-patch'. Look into using 'git rebase -i --autosquash' and 'git
commit --fixup' to maintain the branch, if you're not already familiar
with those things.

Also, it is a really good idea when you post the patch set to include
in the email a clear description of the overall purpose of the patch
set and what each patch does toward that goal. e.g. "The overall goal
of this patch set is to support faster-than-light travel. Currently,
PostgreSQL does not know anything about the speed of light, so 0001
adds some code for speed-of-light detection. Building on this, 0002
adds general support for disabling physical laws of the universe.
Then, 0003 makes use of this support to disable specifically the speed
of light." Perhaps you want a little more text than that for each
patch, depending on the situation, but this gives you the idea, I
hope.

--
Robert Haas
EDB: http://www.enterprisedb.com

--

Regards,

Nikita Malakhov

Postgres Professional 

https://postgrespro.ru/

Hi hackers,

> For a pluggable toaster - in previous patch set part 7 patch file contains invalid string.
> Fixup (v2 file should used instead of previous) patch:
> 7) 0007_fix_alignment_of_custom_toast_pointers.patch - fixes custom toast pointer's
> alignment required by bytea toaster by Nikita Glukhov;

I finished digesting the thread and the referred presentations per
Matthias (cc:'ed) suggestion in [1] discussion. Although the patchset
got a fair amount of push-back above, I prefer to stay open minded and
invest some of my time into this effort as a tester/reviewer during
the July CF. Even if the patchset will not make it entirely to the
core, some of its parts can be useful.

Unfortunately, I didn't manage to find something that can be applied
and tested. cfbot is currently not happy with the patchset.
0001_create_table_storage_v3.patch doesn't apply to the current
origin/master manually either:

```
error: patch failed: src/backend/parser/gram.y:2318
error: src/backend/parser/gram.y: patch does not apply
```

Any chance we can see a rebased patchset for the July CF?

[1]: https://commitfest.postgresql.org/38/3626/

--
Best regards,
Aleksander Alekseev

Hi Nikita,

> We're currently working on rebase along other TOAST improvements, hope to do it in time for July CF.
> Thank you for your patience.

Just to clarify, does it include the dependent "CREATE TABLE ( ..
STORAGE .. )" patch [1]? I was considering changing the patch
according to the feedback it got, but if you are already working on
this I'm not going to interfere.

[1]: https://postgr.es/m/de83407a-ae3d-a8e1-a788-920eb334f25b%40sigaev.ru
--
Best regards,
Aleksander Alekseev

Hi,

Alexander, thank you for your feedback and willingness to help. You can send a suggested fixup in this thread, I'll check the issue you've mentioned.

Best regards,

Nikita Malakhov

On Thu, Jun 23, 2022 at 4:38 PM Aleksander Alekseev <aleksander@timescale.com> wrote:

Hi Nikita,

> We're currently working on rebase along other TOAST improvements, hope to do it in time for July CF.
> Thank you for your patience.

Just to clarify, does it include the dependent "CREATE TABLE ( ..
STORAGE .. )" patch [1]? I was considering changing the patch
according to the feedback it got, but if you are already working on
this I'm not going to interfere.

[1]: https://postgr.es/m/de83407a-ae3d-a8e1-a788-920eb334f25b%40sigaev.ru
--
Best regards,
Aleksander Alekseev

Hi Nikita,

> Here is the patch set rebased onto current master (15 rel beta 2 with commit from 29.06).

Thanks for the rebased patchset.

This is a huge effort though. I suggest splitting it into several CF
entries as we previously did with other patches (64-bit XIDs to name
one, which BTW is arguably much simpler, but still we had to split
it). This will simplify the review, limit the scope of the discussion
and simplify passing the CI. Cfbot is already not happy with the
patchset.

0001 - is already in a separate thread [1], that's good. I suggest
marking it in the patch description for clarity.
0002, 0003 - I suggest focusing on these two in this thread and keep
the rest of the changes for later discussion. Please submit 0004,
0005... next time, when we finish with 0001-0003.

The order of proposed changes IMO is wrong.

0002 should refactor the default TOASTer in a manner similar to a
pluggable one. Nothing should change from the user's perspective. If
you do benchmarks, I suggest not to reference the previous talks. I
familiarized myself with all the related talks linked before (took me
some time...) and found them useless for the discussion since they
don't provide exact steps to reproduce. Please provide exact scripts
and benchmarks results for 0002 in this thread.

0003 should add an interface that allows replacing the default TOASTer
with an alternative one. There is no need for contrib/dummy_toaster
similarly as there is no contrib/ for a dummy TableAM. The provided
example doesn't do much anyway since all the heavy lifting should be
done in the callbacks implementation. For test purposes please use
src/test/regress/.

[1]: https://www.postgresql.org/message-id/de83407a-ae3d-a8e1-a788-920eb334f25b%40sigaev.ru

--
Best regards,
Aleksander Alekseev

On Thu, 30 Jun 2022 at 22:26, Nikita Malakhov <hukutoc@gmail.com> wrote:
>
> Hi hackers!
> Here is the patch set rebased onto current master (15 rel beta 2 with commit from 29.06).

Thanks!

> Just to remind:
> With this patch set you will be able to develop and plug in your own TOAST mechanics for table columns. Knowing internals and/or workflow and workload
> of data being TOASTed makes Custom Toasters much more efficient in performance and storage.

The new toast API doesn't seem to be very well documented, nor are the
new features. Could you include a README or extend the comments on how
this is expected to work, and/or how you expect people to use (the
result of) `get_vtable`?

> Patch set consists of 9 incremental patches:
> [...]
> 0002_toaster_interface_v7.patch - TOAST API interface and SQL syntax allowing creation of custom Toaster (CREATE TOASTER ...)
> and setting Toaster to a table column (CREATE TABLE t (data bytea STORAGE EXTERNAL TOASTER bytea_toaster);)

This patch 0002 seems to include changes to log files (!) that don't
exist in current HEAD, but at the same time are not created by patch
0001. Could you please check and sanitize your patches to ensure that
the changes are actually accurate?

Like Robert Haas mentioned earlier[0], please create a branch in a git
repository that has a commit containing the changes for each patch,
and then use git format-patch to generate a single patchset, one that
shares a single version number. Keeping track of what patches are
needed to test this CF entry is already quite difficult due to the
amount of patches and their packaging (I'm having troubles managing
these seperate .patch.gz), and the different version tags definitely
don't help in finding the correct set of patches to apply once
downloaded.

Kind regards,

Matthias van de Meent

[0] https://www.postgresql.org/message-id/CA%2BTgmoZBgNipyKuQAJzNw2w7C9z%2B2SMC0SAHqCnc_dG1nSLNcw%40mail.gmail.com

Hi!

We have branch with incremental commits worm where patches were generated with format-patch -

https://github.com/postgrespro/postgres/tree/toasterapi_clean

I'll clean up commits from garbage files asap, sorry, haven't noticed them while moving changes.

Best regards,

Nikita Malakhov

On Fri, Jul 1, 2022 at 3:27 PM Matthias van de Meent <boekewurm+postgres@gmail.com> wrote:

On Thu, 30 Jun 2022 at 22:26, Nikita Malakhov <hukutoc@gmail.com> wrote:
>
> Hi hackers!
> Here is the patch set rebased onto current master (15 rel beta 2 with commit from 29.06).

Thanks!

> Just to remind:
> With this patch set you will be able to develop and plug in your own TOAST mechanics for table columns. Knowing internals and/or workflow and workload
> of data being TOASTed makes Custom Toasters much more efficient in performance and storage.

The new toast API doesn't seem to be very well documented, nor are the
new features. Could you include a README or extend the comments on how
this is expected to work, and/or how you expect people to use (the
result of) `get_vtable`?

> Patch set consists of 9 incremental patches:
> [...]
> 0002_toaster_interface_v7.patch - TOAST API interface and SQL syntax allowing creation of custom Toaster (CREATE TOASTER ...)
> and setting Toaster to a table column (CREATE TABLE t (data bytea STORAGE EXTERNAL TOASTER bytea_toaster);)

This patch 0002 seems to include changes to log files (!) that don't
exist in current HEAD, but at the same time are not created by patch
0001. Could you please check and sanitize your patches to ensure that
the changes are actually accurate?

Like Robert Haas mentioned earlier[0], please create a branch in a git
repository that has a commit containing the changes for each patch,
and then use git format-patch to generate a single patchset, one that
shares a single version number. Keeping track of what patches are
needed to test this CF entry is already quite difficult due to the
amount of patches and their packaging (I'm having troubles managing
these seperate .patch.gz), and the different version tags definitely
don't help in finding the correct set of patches to apply once
downloaded.

Kind regards,

Matthias van de Meent

[0] https://www.postgresql.org/message-id/CA%2BTgmoZBgNipyKuQAJzNw2w7C9z%2B2SMC0SAHqCnc_dG1nSLNcw%40mail.gmail.com

Hi hackers!

According to previous requests the patch branch was cleaned up from garbage, logs, etc. All conflicts' resolutions were merged

into patch commits where they appear, branch was rebased to present one commit for one patch. The branch was actualized,

and a fresh patch set was generated.

https://github.com/postgrespro/postgres/tree/toasterapi_clean

What we propose in short:

We suggest a way to make TOAST pluggable as Storage (in a way like Pluggable Access Methods) - detached TOAST 

mechanics from Heap AM, and made it an independent pluggable and extensible part with our freshly developed TOAST API.

With this patch set you will be able to develop and plug in your own TOAST mechanics for table columns. Knowing internals 

and/or workflow and workload of data being TOASTed makes Custom Toasters much more efficient in performance and storage.

We keep backwards compatibility and default TOAST mechanics works as it worked previously, working silently with any 

Toastable datatype 

(and TOASTed values and tables from previous versions, no changes in this) and set as default Toaster is not stated otherwise, 

but through our TOAST API.

We've already presented out work at HighLoad, PgCon and PgConf conferences, you can find materials here 

http://www.sai.msu.su/~megera/postgres/talks/

Testing scripts used in talks are a bit scarce and have a lot of manual handling, so it is another bit of work to bunch them into 

patch set, please be patient, I'll try to make it ASAP.

We have ready to plug in extension Toasters 

- bytea appendable toaster for bytea datatype (impressive speedup with bytea append operation) is included in this patch set;

- JSONB toaster for JSONB (very cool performance improvements when dealing with TOASTed JSONB) will be provided later;

- Prototype Toasters (in development) for PostGIS (much faster then default with geometric data), large binary objects 

(like pg_largeobject, but much, much larger, and without existing large object limitations), and currently we're checking default 

Toaster implementation without using Indexes (direct access by TIDs, up to 3 times faster than default on smaller values, 

less storage due to absence of index tree).

Patch set consists of 8 incremental patches:

0001_create_table_storage_v5.patch - SQL syntax fix for CREATE TABLE clause, processing SET STORAGE... correctly;

This patch is already discussed in a separate thread;

0002_toaster_interface_v8.patch - TOAST API interface and SQL syntax allowing creation of custom Toaster (CREATE TOASTER ...) 

and setting Toaster to a table column (CREATE TABLE t (data bytea STORAGE EXTERNAL TOASTER bytea_toaster);)

0003_toaster_default_v7.patch - Default TOAST implemented via TOAST API;

0004_toaster_snapshot_v7.patch - refactoring of Default TOAST and support for versioned Toast rows;

0005_bytea_appendable_toaster_v7.patch - contrib module bytea_appendable_toaster - special Toaster for bytea datatype with customized append operation;

0006_toasterapi_docs_v3.patch - documentation package for Pluggable TOAST;

0007_fix_alignment_of_custom_toast_pointers_v3.patch - fixes custom toast pointer's

alignment required by bytea toaster by Nikita Glukhov;

0008_fix_toast_tuple_externalize_v3.patch - fixes toast_tuple_externalize function 

not to call toast if old data is the same as new one.

The example of usage the TOAST API:

CREATE EXTENSION bytea_toaster;CREATE TABLE test_bytea_append (id int, a bytea STORAGE EXTERNAL);
ALTER TABLE test_bytea_append ALTER a SET TOASTER bytea_toaster;
INSERT INTO test_bytea_append SELECT i, repeat('a', 10000)::bytea FROM generate_series(1, 10) i;
UPDATE test_bytea_append SET a = a || repeat('b', 3000)::bytea;

This patch set opens the following issues:

1) With TOAST independent of AM it is used by it makes sense to move compression from AM into Toaster and make Compression one of Toaster's options.

Actually, Toasters allow to use any compression methods independently of AM;

2) Implement default Toaster without using Indexes (currently in development)?

3) Allows different, SQL-accessed large objects of almost infinite size IN DATABASE, unlike current large_object functionality and does not limit their quantity;

4) Several already developed Toasters show impressive results for datatypes they were designed for.

We're awaiting feedback.

Regards,

Nikita Malakhov

Postgres Professional 

https://postgrespro.ru/

On Mon, Jul 11, 2022 at 3:03 PM Nikita Malakhov <hukutoc@gmail.com> wrote:

Hi!

We have branch with incremental commits worm where patches were generated with format-patch -

https://github.com/postgrespro/postgres/tree/toasterapi_clean

I'll clean up commits from garbage files asap, sorry, haven't noticed them while moving changes.

Best regards,

Nikita Malakhov

On Fri, Jul 1, 2022 at 3:27 PM Matthias van de Meent <boekewurm+postgres@gmail.com> wrote:

On Thu, 30 Jun 2022 at 22:26, Nikita Malakhov <hukutoc@gmail.com> wrote:
>
> Hi hackers!
> Here is the patch set rebased onto current master (15 rel beta 2 with commit from 29.06).

Thanks!

> Just to remind:
> With this patch set you will be able to develop and plug in your own TOAST mechanics for table columns. Knowing internals and/or workflow and workload
> of data being TOASTed makes Custom Toasters much more efficient in performance and storage.

The new toast API doesn't seem to be very well documented, nor are the
new features. Could you include a README or extend the comments on how
this is expected to work, and/or how you expect people to use (the
result of) `get_vtable`?

> Patch set consists of 9 incremental patches:
> [...]
> 0002_toaster_interface_v7.patch - TOAST API interface and SQL syntax allowing creation of custom Toaster (CREATE TOASTER ...)
> and setting Toaster to a table column (CREATE TABLE t (data bytea STORAGE EXTERNAL TOASTER bytea_toaster);)

This patch 0002 seems to include changes to log files (!) that don't
exist in current HEAD, but at the same time are not created by patch
0001. Could you please check and sanitize your patches to ensure that
the changes are actually accurate?

Like Robert Haas mentioned earlier[0], please create a branch in a git
repository that has a commit containing the changes for each patch,
and then use git format-patch to generate a single patchset, one that
shares a single version number. Keeping track of what patches are
needed to test this CF entry is already quite difficult due to the
amount of patches and their packaging (I'm having troubles managing
these seperate .patch.gz), and the different version tags definitely
don't help in finding the correct set of patches to apply once
downloaded.

Kind regards,

Matthias van de Meent

[0] https://www.postgresql.org/message-id/CA%2BTgmoZBgNipyKuQAJzNw2w7C9z%2B2SMC0SAHqCnc_dG1nSLNcw%40mail.gmail.com

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!

Hi Nikita,

Thanks for an update!

> 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).

This numbering is confusing. Please use a command like:

```
git format-patch origin/master -v 42
```

This will produce a patchset with a consistent naming like:

```
v42-0001-foo-bar.patch
v42-0002-baz-qux.patch
... etc ...
```

Also cfbot [1] will know in which order to apply them.

> GIT branch with this patch resides here:
> https://github.com/postgrespro/postgres/tree/toasterapi_clean

Unfortunately the three patches in question from this branch don't
pass `make check`. Please update
src/test/regress/expected/publication.out and make sure the patchset
passes the rest of the tests at least on one platform before
submitting.

Personally I have a little set of scripts for this [2]. The following
commands should pass:

```
# quick check
./quick-build.sh && ./single-install.sh && make installcheck

# full check
./full-build.sh && ./single-install.sh && make installcheck-world
```

Finally, please update the commit messages. Each commit message should
include a brief description (one line) , a detailed description (the
body), and also the list of the authors, the reviewers and a link to
the discussion. Please use [3] as a template.

[1]: http://cfbot.cputube.org/
[2]: https://github.com/afiskon/pgscripts/
[3]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=784cedda0604ee4ac731fd0b00cd8b27e78c02d3

--
Best regards,
Aleksander Alekseev

Hi,

On 2022-08-02 09:15:12 +0300, Nikita Malakhov wrote:
> Attach includes:
> v11-0002-toaster-interface.patch - contains TOAST API with default Toaster
> left as-is (reference implementation) and Dummy toaster as an example
> (will be removed later as a part of refactoring?).
>
> v11-0003-toaster-default.patch - 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.
>
> v11-0004-toaster-snapshot.patch - supports row versioning for TOASTed values
> and some refactoring.

I'm a bit confused by the patch numbering - why isn't there a patch 0001 and
0005?

I think 0002 needs to be split further - the relevant part isn't that it
introduces the "dummy toaster" module, it's a large change doing lots of
things, the addition of the contrib module is irrelevant comparatively.

As is the patches unfortunately are close to unreviewable. Lots of code gets
moved around in one patch, then again in the next patch, then again in the
next.


Unfortunately, scanning through these patches, it seems this is a lot of
complexity, with a (for me) comensurate benefit. There's a lot of more general
improvements to toasting and the json type that we can do, that are a lot less
complex than this.


> From 6b35d6091248e120d2361cf0a806dbfb161421cf Mon Sep 17 00:00:00 2001
> From: Nikita Malakhov <n.malakhov@postgrespro.ru>
> Date: Tue, 12 Apr 2022 18:37:21 +0300
> Subject: [PATCH] Pluggable TOAST API interface with dummy_toaster contrib
>  module
>
> Pluggable TOAST API is introduced with implemented contrib example
> module.
> Pluggable TOAST API consists of 4 parts:
> 1) SQL syntax supports manipulations with toasters - CREATE TABLE ...
> (column type STORAGE storage_type TOASTER toaster), ALTER TABLE ALTER
> COLUMN column SET TOASTER toaster and Toaster definition.
> TOAST API requires earlier patch with CREATE TABLE SET STORAGE clause;
> New column atttoaster is added to pg_attribute.
> Toaster drop is not allowed for not to lose already toasted data;
> 2) New VARATT_CUSTOM data structure with fixed header and variable
> tail to store custom toasted data, with according macros set;

That's adding overhead to every toast interaction, independent of any new
infrastructure being used.



> 4) Dummy toaster implemented via new TOAST API to be used as sample.
> In this patch regular (default) TOAST function is left as-is and not
> yet implemented via new API.
> TOAST API syntax and code explanation provided in additional docs patch.

I'd make this a separate commit.



> @@ -445,6 +447,8 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
>                       return false;
>               if (attr1->attstorage != attr2->attstorage)
>                       return false;
> +             if (attr1->atttoaster != attr2->atttoaster)
> +                     return false;

So we're increasing pg_attribute - often already the largest catalog table in
a database.

Am I just missing something, or is atttoaster not actually used in this patch?
So most of the contrib module added is unreachable code?


> +/*
> + * Toasters is very often called so syscache lookup and TsrRoutine allocation are
> + * expensive and we need to cache them.

Ugh.

> + * We believe what there are only a few toasters and there is high chance that
> + * only one or only two of them are heavy used, so most used toasters should be
> + * found as easy as possible. So, let us use a simple list, in future it could
> + * be changed to other structure. For now it will be stored in TopCacheContext
> + * and never destroed in backend life cycle - toasters are never deleted.
> + */

That seems not great.


> +typedef struct ToasterCacheEntry
> +{
> +     Oid                     toasterOid;
> +     TsrRoutine *routine;
> +} ToasterCacheEntry;
> +
> +static List  *ToasterCache = NIL;
> +
> +/*
> + * SearchTsrCache - get cached toaster routine, emits an error if toaster
> + * doesn't exist
> + */
> +TsrRoutine*
> +SearchTsrCache(Oid   toasterOid)
> +{
> +     ListCell                   *lc;
> +     ToasterCacheEntry  *entry;
> +     MemoryContext           ctx;
> +
> +     if (list_length(ToasterCache) > 0)
> +     {
> +             /* fast path */
> +             entry = (ToasterCacheEntry*)linitial(ToasterCache);
> +             if (entry->toasterOid == toasterOid)
> +                     return entry->routine;
> +     }
> +
> +     /* didn't find in first position */
> +     ctx = MemoryContextSwitchTo(CacheMemoryContext);
> +
> +     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);

That needs to move later list elements!


> +                     goto out;
> +             }
> +     }
> +
> +     /* did not find entry, make a new one */
> +     entry = palloc(sizeof(*entry));
> +
> +     entry->toasterOid = toasterOid;
> +     entry->routine = GetTsrRoutineByOid(toasterOid, false);
> +
> +out:
> +     ToasterCache = lcons(entry, ToasterCache);

That move the whole list around! On a cache hit. Tthis would likely already be
slower than syscache.


> diff --git a/contrib/dummy_toaster/dummy_toaster.c b/contrib/dummy_toaster/dummy_toaster.c
> index 0d261f6042..02f49052b7 100644
> --- a/contrib/dummy_toaster/dummy_toaster.c
> +++ b/contrib/dummy_toaster/dummy_toaster.c

So this is just changing around the code added in the prior commit. Why was it
then included before?


> +++ b/src/include/access/generic_toaster.h

> +HeapTuple
> +heap_toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup,
> +                                                     int options);

The generic toast API has heap_* in its name?




> From 4112cd70b05dda39020d576050a98ca3cdcf2860 Mon Sep 17 00:00:00 2001
> From: Nikita Malakhov <n.malakhov@postgrespro.ru>
> Date: Tue, 12 Apr 2022 22:57:21 +0300
> Subject: [PATCH] Versioned rows in TOASTed values for Default Toaster support
>
> Original TOAST mechanics does not support rows versioning
> for TOASTed values.
> Toaster snapshot - refactored generic toaster, implements
> rows versions check in toasted values to share common parts
> of toasted values between different versions of rows.

This misses explaining *WHY* this is changed.



> diff --git a/src/backend/access/common/detoast.c b/src/backend/access/common/detoast.c
> deleted file mode 100644
> index aff8042166..0000000000
> --- a/src/backend/access/common/detoast.c
> +++ /dev/null

These patches really move things around in a largely random way.


> -static bool toastrel_valueid_exists(Relation toastrel, Oid valueid);
> -static bool toastid_valueid_exists(Oid toastrelid, Oid valueid);
> -
> +static void
> +toast_extract_chunk_fields(Relation toastrel, TupleDesc toasttupDesc,
> +                                                Oid valueid, HeapTuple ttup, int32 *seqno,
> +                                                char **chunkdata, int *chunksize);
> +
> +static void
> +toast_write_slice(Relation toastrel, Relation *toastidxs,
> +                               int num_indexes, int validIndex,
> +                               Oid valueid, int32 value_size, int32 slice_offset,
> +                               int32 slice_length, char *slice_data,
> +                               int options,
> +                               void *chunk_header, int chunk_header_size,
> +                               ToastChunkVisibilityCheck visibility_check,
> +                               void *visibility_cxt);

What do all these changes have to do with "Versioned rows in TOASTed
values for Default Toaster support"?


> +static void *
> +toast_fetch_old_chunk(Relation toastrel, SysScanDesc toastscan, Oid valueid,
> +                                       int32 expected_chunk_seq, int32 last_old_chunk_seq,
> +                                       ToastChunkVisibilityCheck visibility_check,
> +                                       void *visibility_cxt,
> +                                       int32 *p_old_chunk_size, ItemPointer old_tid)
> +{
> +     for (;;)
> +     {
> +             HeapTuple       old_toasttup;
> +             char       *old_chunk_data;
> +             int32           old_chunk_seq;
> +             int32           old_chunk_data_size;
> +
> +             old_toasttup = systable_getnext_ordered(toastscan, ForwardScanDirection);
> +
> +             if (old_toasttup)
> +             {
> +                     /* Skip aborted chunks */
> +                     if (!HeapTupleHeaderXminCommitted(old_toasttup->t_data))
> +                     {
> +                             TransactionId xmin = HeapTupleHeaderGetXmin(old_toasttup->t_data);
> +
> +                             Assert(!HeapTupleHeaderXminInvalid(old_toasttup->t_data));
> +
> +                             if (TransactionIdDidAbort(xmin))
> +                                     continue;
> +                     }

Why is there visibility logic in quite random places? Also, it's not "legal"
to call TransactionIdDidAbort() without having checked
TransactionIdIsInProgress() first. And what does this this have to do with
snapshots - it's pretty clearly not implementing snapshot logic.


Greetings,

Andres Freund

Hi Nikita,

> I've decided to leave only the first 2 patches for review and send less significant
> changes after the main patch will be straightened out.
> So, here is
> v13-0001-toaster-interface.patch - main TOAST API patch, with reference TOAST
> mechanics left as-is.
> v13-0002-toaster-default.patch - reference TOAST re-implemented via TOAST API.
> [...]

Great! Thank you.

Unfortunately the patchset still seems to have difficulties passing
the CI checks (see http://cfbot.cputube.org/ ). Any chance we may see
a version rebased to the current `master` branch for the September CF?

--
Best regards,
Aleksander Alekseev

On Wed, Aug 24, 2022 at 2:59 AM Nikita Malakhov <hukutoc@gmail.com> wrote:
> I've rebased actual branch onto the latest master and re-created patches. Checked with git am,
> all applied correctly. Please check the attached patches.
> Rebased branch resides here:
> https://github.com/postgrespro/postgres/tree/toasterapi_clean

I tried installing and using the dummy_toaster that's provided with
the gitlink. Upgrade of that cluster fails with the following message:

    pg_restore: creating TOASTER "dummy_toaster"
    pg_restore: while PROCESSING TOC:
    pg_restore: from TOC entry 2044; 9861 16390 TOASTER dummy_toaster (no owner)
    pg_restore: error: could not execute query: ERROR:  unrecognized
or unsupported class OID: 9861
    Command was: CREATE TOASTER "dummy_toaster" HANDLER
"public"."dummy_toaster_handler";

I was looking through the thread for a more in-depth description of
the "vtable" concept, but I didn't see one. It looks like it's just an
arbitrary extension point, and any new additions would require surgery
on whatever function needs the particular magic provided by the
toaster. E.g. your bytea-append toaster extension in the gitlink,
which still has to modify byteacat() in varlena.c to implement a very
specific optimization, and then declares its support for that
hardcoded optimization in the extension.

I'm skeptical that this would remain coherent as it grows. The patch
claims the vtable API is "powerful", which... I suppose it is, if you
get to make arbitrary modifications to the core whenever you implement
it. Did you already have thoughts about which operations would belong
under that umbrella? What would the procedure be for adding
functionality to that API? What happens if a toaster wants to
implement two magic performance optimizations instead of one?

--Jacob

On Mon, Sep 12, 2022 at 11:45 PM Nikita Malakhov <hukutoc@gmail.com> wrote:
> It would be more clear for complex data types like JSONB, where developers could
> need some additional functionality to work with internal representation of data type,
> and its full potential is revealed in our JSONB toaster extension. The JSONB toaster
> is still in development but we plan to make it available soon.

Okay. It'll be good to have that, because as it is now it's hard to
see the whole picture.

> On installing dummy_toaster contrib: I've just checked it by making a patch from commit
> and applying onto my clone of master and 2 patches provided in previous email without
> any errors and sll checks passed - applying with git am, configure with debug, cassert,
> depend and enable-tap-tests flags and run checks.
> Please advice what would cause such a behavior?

I don't think the default pg_upgrade tests will upgrade contrib
objects (there are instructions in src/bin/pg_upgrade/TESTING that
cover manual dumps, if you prefer that method). My manual steps were
roughly

    =# CREATE EXTENSION dummy_toaster;
    =# CREATE TABLE test (t TEXT
            STORAGE external
            TOASTER dummy_toaster_handler);
    =# \q
    $ initdb -D newdb
    $ pg_ctl -D olddb stop
    $ pg_upgrade -b <install path>/bin -B <install path>/bin -d
./olddb -D ./newdb

(where <install path>/bin is on the PATH, so we're using the right binaries).

Thanks,
--Jacob

Hi hackers!

Cfbot is not happy with previous patchset, so I'm attaching new one, rebased onto current master

(15b4). Also providing patch with documentation package, and the second one contains large 

README.toastapi file providing additional in-depth docs for developers.

Comments would be greatly appreciated.

Also, after checking patch sources I have a strong opinion that it needs some refactoring - 

move all files related to TOAST implementation into new folder /backend/access/toast where

Generic (default) Toaster resides.

Patchset consists of:

v15-0001-toaster-interface.patch - Pluggable TOAST API interface along with reference TOAST mechanics;

v15-0002-toaster-default.patch - Default TOAST re-implemented using Toaster API;
v15-0003-toaster-docs.patch - Pluggable TOAST API documentation package

On Tue, Sep 13, 2022 at 7:50 PM Jacob Champion <jchampion@timescale.com> wrote:

On Mon, Sep 12, 2022 at 11:45 PM Nikita Malakhov <hukutoc@gmail.com> wrote:
> It would be more clear for complex data types like JSONB, where developers could
> need some additional functionality to work with internal representation of data type,
> and its full potential is revealed in our JSONB toaster extension. The JSONB toaster
> is still in development but we plan to make it available soon.

Okay. It'll be good to have that, because as it is now it's hard to
see the whole picture.

> On installing dummy_toaster contrib: I've just checked it by making a patch from commit
> and applying onto my clone of master and 2 patches provided in previous email without
> any errors and sll checks passed - applying with git am, configure with debug, cassert,
> depend and enable-tap-tests flags and run checks.
> Please advice what would cause such a behavior?

I don't think the default pg_upgrade tests will upgrade contrib
objects (there are instructions in src/bin/pg_upgrade/TESTING that
cover manual dumps, if you prefer that method). My manual steps were
roughly

    =# CREATE EXTENSION dummy_toaster;
    =# CREATE TABLE test (t TEXT
            STORAGE external
            TOASTER dummy_toaster_handler);
    =# \q
    $ initdb -D newdb
    $ pg_ctl -D olddb stop
    $ pg_upgrade -b <install path>/bin -B <install path>/bin -d
./olddb -D ./newdb

(where <install path>/bin is on the PATH, so we're using the right binaries).

Thanks,
--Jacob

--

Regards,

Nikita Malakhov

Postgres Professional 

https://postgrespro.ru/

Hi hackers!

Cfbot is still not happy with the patchset, so I'm attaching a rebased one, rebased onto the current

master (from today). The third patch contains documentation package, and the second one contains large 

README.toastapi file providing additional in-depth docs for developers.

Comments would be greatly appreciated.

Again, after checking patch sources I have a strong opinion that it needs some refactoring - 

move all files related to TOAST implementation into new folder /backend/access/toast where

Generic (default) Toaster resides.

Patchset consists of:

v16-0001-toaster-interface.patch - Pluggable TOAST API interface along with reference TOAST mechanics;

v16-0002-toaster-default.patch - Default TOAST re-implemented using Toaster API;
v16-0003-toaster-docs.patch - Pluggable TOAST API documentation package

Actual GitHub branch resides at

https://github.com/postgrespro/postgres/tree/toasterapi_clean

On Fri, Sep 23, 2022 at 10:54 PM Nikita Malakhov <hukutoc@gmail.com> wrote:

Hi hackers!

Cfbot is not happy with previous patchset, so I'm attaching new one, rebased onto current master

(15b4). Also providing patch with documentation package, and the second one contains large 

README.toastapi file providing additional in-depth docs for developers.

Comments would be greatly appreciated.

Also, after checking patch sources I have a strong opinion that it needs some refactoring - 

move all files related to TOAST implementation into new folder /backend/access/toast where

Generic (default) Toaster resides.

Patchset consists of:

v15-0001-toaster-interface.patch - Pluggable TOAST API interface along with reference TOAST mechanics;

v15-0002-toaster-default.patch - Default TOAST re-implemented using Toaster API;
v15-0003-toaster-docs.patch - Pluggable TOAST API documentation package

On Tue, Sep 13, 2022 at 7:50 PM Jacob Champion <jchampion@timescale.com> wrote:

On Mon, Sep 12, 2022 at 11:45 PM Nikita Malakhov <hukutoc@gmail.com> wrote:
> It would be more clear for complex data types like JSONB, where developers could
> need some additional functionality to work with internal representation of data type,
> and its full potential is revealed in our JSONB toaster extension. The JSONB toaster
> is still in development but we plan to make it available soon.

Okay. It'll be good to have that, because as it is now it's hard to
see the whole picture.

> On installing dummy_toaster contrib: I've just checked it by making a patch from commit
> and applying onto my clone of master and 2 patches provided in previous email without
> any errors and sll checks passed - applying with git am, configure with debug, cassert,
> depend and enable-tap-tests flags and run checks.
> Please advice what would cause such a behavior?

I don't think the default pg_upgrade tests will upgrade contrib
objects (there are instructions in src/bin/pg_upgrade/TESTING that
cover manual dumps, if you prefer that method). My manual steps were
roughly

    =# CREATE EXTENSION dummy_toaster;
    =# CREATE TABLE test (t TEXT
            STORAGE external
            TOASTER dummy_toaster_handler);
    =# \q
    $ initdb -D newdb
    $ pg_ctl -D olddb stop
    $ pg_upgrade -b <install path>/bin -B <install path>/bin -d
./olddb -D ./newdb

(where <install path>/bin is on the PATH, so we're using the right binaries).

Thanks,
--Jacob

--

Regards,

Nikita Malakhov

Postgres Professional 

https://postgrespro.ru/

--

Regards,

Nikita Malakhov

Postgres Professional 

https://postgrespro.ru/

Hi hackers!

Last patchset has an invalid patch file - v16-0003-toaster-docs.patch. Here's corrected patchset,

sorry for the noise.

On Sat, Sep 24, 2022 at 3:50 PM Nikita Malakhov <hukutoc@gmail.com> wrote:

Hi hackers!

Cfbot is still not happy with the patchset, so I'm attaching a rebased one, rebased onto the current

master (from today). The third patch contains documentation package, and the second one contains large 

README.toastapi file providing additional in-depth docs for developers.

Comments would be greatly appreciated.

Again, after checking patch sources I have a strong opinion that it needs some refactoring - 

move all files related to TOAST implementation into new folder /backend/access/toast where

Generic (default) Toaster resides.

Patchset consists of:

v16-0001-toaster-interface.patch - Pluggable TOAST API interface along with reference TOAST mechanics;

v16-0002-toaster-default.patch - Default TOAST re-implemented using Toaster API;
v16-0003-toaster-docs.patch - Pluggable TOAST API documentation package

Actual GitHub branch resides at

https://github.com/postgrespro/postgres/tree/toasterapi_clean

On Fri, Sep 23, 2022 at 10:54 PM Nikita Malakhov <hukutoc@gmail.com> wrote:

Hi hackers!

Cfbot is not happy with previous patchset, so I'm attaching new one, rebased onto current master

(15b4). Also providing patch with documentation package, and the second one contains large 

README.toastapi file providing additional in-depth docs for developers.

Comments would be greatly appreciated.

Also, after checking patch sources I have a strong opinion that it needs some refactoring - 

move all files related to TOAST implementation into new folder /backend/access/toast where

Generic (default) Toaster resides.

Patchset consists of:

v15-0001-toaster-interface.patch - Pluggable TOAST API interface along with reference TOAST mechanics;

v15-0002-toaster-default.patch - Default TOAST re-implemented using Toaster API;
v15-0003-toaster-docs.patch - Pluggable TOAST API documentation package

On Tue, Sep 13, 2022 at 7:50 PM Jacob Champion <jchampion@timescale.com> wrote:

On Mon, Sep 12, 2022 at 11:45 PM Nikita Malakhov <hukutoc@gmail.com> wrote:
> It would be more clear for complex data types like JSONB, where developers could
> need some additional functionality to work with internal representation of data type,
> and its full potential is revealed in our JSONB toaster extension. The JSONB toaster
> is still in development but we plan to make it available soon.

Okay. It'll be good to have that, because as it is now it's hard to
see the whole picture.

> On installing dummy_toaster contrib: I've just checked it by making a patch from commit
> and applying onto my clone of master and 2 patches provided in previous email without
> any errors and sll checks passed - applying with git am, configure with debug, cassert,
> depend and enable-tap-tests flags and run checks.
> Please advice what would cause such a behavior?

I don't think the default pg_upgrade tests will upgrade contrib
objects (there are instructions in src/bin/pg_upgrade/TESTING that
cover manual dumps, if you prefer that method). My manual steps were
roughly

    =# CREATE EXTENSION dummy_toaster;
    =# CREATE TABLE test (t TEXT
            STORAGE external
            TOASTER dummy_toaster_handler);
    =# \q
    $ initdb -D newdb
    $ pg_ctl -D olddb stop
    $ pg_upgrade -b <install path>/bin -B <install path>/bin -d
./olddb -D ./newdb

(where <install path>/bin is on the PATH, so we're using the right binaries).

Thanks,
--Jacob

--

Regards,

Nikita Malakhov

Postgres Professional 

https://postgrespro.ru/

--

Regards,

Nikita Malakhov

Postgres Professional 

https://postgrespro.ru/

--

Regards,

Nikita Malakhov

Postgres Professional 

https://postgrespro.ru/

Hi,

Meson build for the patchset failed, meson build files attached and README/Doc package

reworked with more detailed explanation of virtual function table along with other corrections.

On Sun, Sep 25, 2022 at 1:41 AM Nikita Malakhov <hukutoc@gmail.com> wrote:

Hi hackers!

Last patchset has an invalid patch file - v16-0003-toaster-docs.patch. Here's corrected patchset,

sorry for the noise.

On Sat, Sep 24, 2022 at 3:50 PM Nikita Malakhov <hukutoc@gmail.com> wrote:

Hi hackers!

Cfbot is still not happy with the patchset, so I'm attaching a rebased one, rebased onto the current

master (from today). The third patch contains documentation package, and the second one contains large 

README.toastapi file providing additional in-depth docs for developers.

Comments would be greatly appreciated.

Again, after checking patch sources I have a strong opinion that it needs some refactoring - 

move all files related to TOAST implementation into new folder /backend/access/toast where

Generic (default) Toaster resides.

Patchset consists of:

v16-0001-toaster-interface.patch - Pluggable TOAST API interface along with reference TOAST mechanics;

v16-0002-toaster-default.patch - Default TOAST re-implemented using Toaster API;
v16-0003-toaster-docs.patch - Pluggable TOAST API documentation package

Actual GitHub branch resides at

https://github.com/postgrespro/postgres/tree/toasterapi_clean

On Fri, Sep 23, 2022 at 10:54 PM Nikita Malakhov <hukutoc@gmail.com> wrote:

Hi hackers!

Cfbot is not happy with previous patchset, so I'm attaching new one, rebased onto current master

(15b4). Also providing patch with documentation package, and the second one contains large 

README.toastapi file providing additional in-depth docs for developers.

Comments would be greatly appreciated.

Also, after checking patch sources I have a strong opinion that it needs some refactoring - 

move all files related to TOAST implementation into new folder /backend/access/toast where

Generic (default) Toaster resides.

Patchset consists of:

v15-0001-toaster-interface.patch - Pluggable TOAST API interface along with reference TOAST mechanics;

v15-0002-toaster-default.patch - Default TOAST re-implemented using Toaster API;
v15-0003-toaster-docs.patch - Pluggable TOAST API documentation package

On Tue, Sep 13, 2022 at 7:50 PM Jacob Champion <jchampion@timescale.com> wrote:

On Mon, Sep 12, 2022 at 11:45 PM Nikita Malakhov <hukutoc@gmail.com> wrote:
> It would be more clear for complex data types like JSONB, where developers could
> need some additional functionality to work with internal representation of data type,
> and its full potential is revealed in our JSONB toaster extension. The JSONB toaster
> is still in development but we plan to make it available soon.

Okay. It'll be good to have that, because as it is now it's hard to
see the whole picture.

> On installing dummy_toaster contrib: I've just checked it by making a patch from commit
> and applying onto my clone of master and 2 patches provided in previous email without
> any errors and sll checks passed - applying with git am, configure with debug, cassert,
> depend and enable-tap-tests flags and run checks.
> Please advice what would cause such a behavior?

I don't think the default pg_upgrade tests will upgrade contrib
objects (there are instructions in src/bin/pg_upgrade/TESTING that
cover manual dumps, if you prefer that method). My manual steps were
roughly

    =# CREATE EXTENSION dummy_toaster;
    =# CREATE TABLE test (t TEXT
            STORAGE external
            TOASTER dummy_toaster_handler);
    =# \q
    $ initdb -D newdb
    $ pg_ctl -D olddb stop
    $ pg_upgrade -b <install path>/bin -B <install path>/bin -d
./olddb -D ./newdb

(where <install path>/bin is on the PATH, so we're using the right binaries).

Thanks,
--Jacob

--

Regards,

Nikita Malakhov

Postgres Professional 

https://postgrespro.ru/

--

Regards,

Nikita Malakhov

Postgres Professional 

https://postgrespro.ru/

--

Regards,

Nikita Malakhov

Postgres Professional 

https://postgrespro.ru/

--

Regards,

Nikita Malakhov

Postgres Professional 

https://postgrespro.ru/

Hi hackers!

Cfbot failed in meson build with previous patchsets, so I've rebased them onto the latest master and added necessary meson build info.

Patchset consists of:

v19-0001-toaster-interface.patch - Pluggable TOAST API interface along with reference TOAST mechanics - new API is introduced but

reference TOAST is still unchanged;

v19-0002-toaster-default.patch - Default TOAST re-implemented using Toaster API - reference TOAST is re-implemented via new API;
v19-0003-toaster-docs.patch - Pluggable TOAST API documentation package

Actual GitHub branch resides at

https://github.com/postgrespro/postgres/tree/toasterapi_clean

On Tue, Sep 27, 2022 at 12:26 AM Nikita Malakhov <hukutoc@gmail.com> wrote:

Hi,

Meson build for the patchset failed, meson build files attached and README/Doc package

reworked with more detailed explanation of virtual function table along with other corrections.

On Sun, Sep 25, 2022 at 1:41 AM Nikita Malakhov <hukutoc@gmail.com> wrote:

Hi hackers!

Last patchset has an invalid patch file - v16-0003-toaster-docs.patch. Here's corrected patchset,

sorry for the noise.

On Sat, Sep 24, 2022 at 3:50 PM Nikita Malakhov <hukutoc@gmail.com> wrote:

Hi hackers!

Cfbot is still not happy with the patchset, so I'm attaching a rebased one, rebased onto the current

master (from today). The third patch contains documentation package, and the second one contains large 

README.toastapi file providing additional in-depth docs for developers.

Comments would be greatly appreciated.

Again, after checking patch sources I have a strong opinion that it needs some refactoring - 

move all files related to TOAST implementation into new folder /backend/access/toast where

Generic (default) Toaster resides.

Patchset consists of:

v16-0001-toaster-interface.patch - Pluggable TOAST API interface along with reference TOAST mechanics;

v16-0002-toaster-default.patch - Default TOAST re-implemented using Toaster API;
v16-0003-toaster-docs.patch - Pluggable TOAST API documentation package

Actual GitHub branch resides at

https://github.com/postgrespro/postgres/tree/toasterapi_clean

On Fri, Sep 23, 2022 at 10:54 PM Nikita Malakhov <hukutoc@gmail.com> wrote:

Hi hackers!

Cfbot is not happy with previous patchset, so I'm attaching new one, rebased onto current master

(15b4). Also providing patch with documentation package, and the second one contains large 

README.toastapi file providing additional in-depth docs for developers.

Comments would be greatly appreciated.

Also, after checking patch sources I have a strong opinion that it needs some refactoring - 

move all files related to TOAST implementation into new folder /backend/access/toast where

Generic (default) Toaster resides.

Patchset consists of:

v15-0001-toaster-interface.patch - Pluggable TOAST API interface along with reference TOAST mechanics;

v15-0002-toaster-default.patch - Default TOAST re-implemented using Toaster API;
v15-0003-toaster-docs.patch - Pluggable TOAST API documentation package

On Tue, Sep 13, 2022 at 7:50 PM Jacob Champion <jchampion@timescale.com> wrote:

On Mon, Sep 12, 2022 at 11:45 PM Nikita Malakhov <hukutoc@gmail.com> wrote:
> It would be more clear for complex data types like JSONB, where developers could
> need some additional functionality to work with internal representation of data type,
> and its full potential is revealed in our JSONB toaster extension. The JSONB toaster
> is still in development but we plan to make it available soon.

Okay. It'll be good to have that, because as it is now it's hard to
see the whole picture.

> On installing dummy_toaster contrib: I've just checked it by making a patch from commit
> and applying onto my clone of master and 2 patches provided in previous email without
> any errors and sll checks passed - applying with git am, configure with debug, cassert,
> depend and enable-tap-tests flags and run checks.
> Please advice what would cause such a behavior?

I don't think the default pg_upgrade tests will upgrade contrib
objects (there are instructions in src/bin/pg_upgrade/TESTING that
cover manual dumps, if you prefer that method). My manual steps were
roughly

    =# CREATE EXTENSION dummy_toaster;
    =# CREATE TABLE test (t TEXT
            STORAGE external
            TOASTER dummy_toaster_handler);
    =# \q
    $ initdb -D newdb
    $ pg_ctl -D olddb stop
    $ pg_upgrade -b <install path>/bin -B <install path>/bin -d
./olddb -D ./newdb

(where <install path>/bin is on the PATH, so we're using the right binaries).

Thanks,
--Jacob

--

Regards,

Nikita Malakhov

Postgres Professional 

https://postgrespro.ru/

--

Regards,

Nikita Malakhov

Postgres Professional 

https://postgrespro.ru/

--

Regards,

Nikita Malakhov

Postgres Professional 

https://postgrespro.ru/

--

Regards,

Nikita Malakhov

Postgres Professional 

https://postgrespro.ru/

--

Regards,

Nikita Malakhov

Postgres Professional 

https://postgrespro.ru/

Hi Nikita,

> Here's rebased patch:
> v23-0001-toaster-interface.patch - Pluggable TOAST API interface with reference (original) TOAST mechanics - new API is introduced but
> reference TOAST is still left unchanged;
> v23-0002-toaster-default.patch - Default TOAST mechanics is re-implemented using TOAST API and is plugged in as Default Toaster;
> v23-0003-toaster-docs.patch - Pluggable TOAST API documentation package

Thanks for keeping the patch up to date.

As I recall one of the open questions was: how this feature is
supposed to work with table access methods? Could you please summarize
what the current consensus is in this respect?

--
Best regards,
Aleksander Alekseev

Hi Nikita,

> Aleksander, we have had this in mind while developing this feature, and have checked it. Just a slight modification is needed
> to make it work with Pluggable Storage (Access Methods) API.

Could you please clarify this a little from the architectural point of view?

Let's say company A implements some specific TableAM (in-memory / the
one that uses undo logging / etc). Company B implements an alternative
TOAST mechanism.

How the TOAST extension is going to work without knowing any specifics
of the TableAM the user chooses for the given relation, and vice
versa? How one of the extensions is going to upgrade / downgrade
between the versions without knowing any implementation details of
another?

--
Best regards,
Aleksander Alekseev

Hi Nikita,

> Pluggable TOAST API was designed with storage flexibility in mind, and Custom TOAST mechanics is
> free to use any storage methods

Don't you think that this is an arguable design decision? Basically
all we know about the underlying TableAM is that it stores tuples
_somehow_ and that tuples have TIDs [1]. That's it. We don't know if
it even has any sort of pages, whether they are fixed in size or not,
whether it uses shared buffers, etc. It may not even require TOAST.
(Not to mention the fact that when you have N TOAST implementations
and M TableAM implementations now you have to run N x M compatibility
tests. And this doesn't account for different versions of Ns and Ms,
different platforms and different versions of PostgreSQL.)

I believe the proposed approach is architecturally broken from the beginning.

It looks like the idea should be actually turned inside out. I.e. what
would be nice to have is some sort of _framework_ that helps TableAM
authors to implement TOAST (alternatively, the rest of the TableAM
except for TOAST) if the TableAM is similar to the default one. In
other words the idea is not to implement alternative TOASTers that
will work with all possible TableAMs but rather to simplify the task
of implementing an alternative TableAM which is similar to the default
one except for TOAST. These TableAMs should reuse as much common code
as possible except for the parts where they differ.

Does it make sense?

Sorry, I realize this will probably imply a complete rewrite of the
patch. This is the reason why one should start proposing changes from
gathering the requirements, writing an RFC and run it through several
rounds of discussion.

[1]: https://www.postgresql.org/docs/current/tableam.html

--
Best regards,
Aleksander Alekseev

Hi Nikita,

I don't argue with most of what you say. I am just pointing out the
reason why the chosen approach "N TOASTers x M TableAMs" will not
work:

> Don't you think that this is an arguable design decision? Basically
> all we know about the underlying TableAM is that it stores tuples
> _somehow_ and that tuples have TIDs [1]. That's it. We don't know if
> it even has any sort of pages, whether they are fixed in size or not,
> whether it uses shared buffers, etc. It may not even require TOAST.
> [...]

Also I completely agree with:

> Implementing another Table AM just to implement another TOAST strategy seems too
> much, the TAM API is very heavy and complex, and you would have to add it as a contrib.

This is what I meant above when talking about the framework for
simplifying this task:

> It looks like the idea should be actually turned inside out. I.e. what
> would be nice to have is some sort of _framework_ that helps TableAM
> authors to implement TOAST (alternatively, the rest of the TableAM
> except for TOAST) if the TableAM is similar to the default one.

From the user perspective it's much easier to think about one entity -
TableAM, and choosing from heapam_with_default_toast and
heapam_with_different_toast.

From the extension implementer point of view creating TableAMs is a
difficult task. This is what the framework should address. Ideally the
interface should be as simple as:

CreateParametrizedDefaultHeapAM(SomeTOASTSubstitutionObject, ...other
arguments, in the future...)

Where the extension author should be worried only about an alternative
TOAST implementation.

I think at some point such a framework may address at least one more
issue we have - an inability to change the page size on the table
level. As it was shown by Tomas Vondra [1] the default 8 KB page size
can be suboptimal depending on the load. So it would be nice if the
user could change it without rebuilding PostgreSQL. Naturally this is
out of scope of this particular patchset. I just wanted to point out
opportunities we have here.

[1]: https://www.postgresql.org/message-id/flat/b4861449-6c54-ccf8-e67c-c039228cdc6d%40enterprisedb.com

--
Best regards,
Aleksander Alekseev

Hi Nikita,

> >I don't argue with most of what you say. I am just pointing out the
> >reason why the chosen approach "N TOASTers x M TableAMs" will not
> >work:
>
> We assume that TAM used in custom Toaster works as it is should work,
> and leave TAM internals to this TAM developer - say, we do not want to
> change internals of Heap AM.
>
> We don't want to create some kind of silver bullet.

This is exactly the point. In order to not to create a silver bullet,
TOASTers should be limited to a single TableAM. The one we know uses
pages of a known fixed size, the one that actually requires TOAST
because pages are relatively small, etc.

> That's what the TOAST API is - just an interface that all custom
> TOAST implementations use to have a common entry point from any TAM,
> [...]

I believe this is the source of misunderstanding. Note that not _any_
TableAM needs TOAST to begin with. As an example, if one chooses to
implement a column-organized TableAM that stores all text/bytea
attributes in a separate dictionary file this person doesn't need
TOAST and doesn't want to be constrained by the need of choosing one.

For this reason the "N TOASTers x M TableAMs" approach is
architecturally broken.

> keep in mind that different kinds of data require very
> different approach to external storage - say, JSON TOAST works with
> maps of keys and values, [...]

To clarify: is an ability to specify TOASTers for given columns and/or
types also part of the plan?

> Have I answered your question? Please don't hesitate to point to any unclear
> parts, I'd be glad to explain that.

No. To be honest, it looks like you are merely discarding most/any
feedback the community provided so far.

I really think that pluggable TOASTers would be a great feature.
However if the goal is to get it into the core I doubt that we are
going to make much progress with the current approach.

--
Best regards,
Aleksander Alekseev

Hi Nikita,

> Using Table AM Routine and routing AM methods calls via it is a topic for further discussion,
> if Pluggable TOAST will be committed. [...] And even then it would be an open issue.

From personal experience with the project I have serious doubts this
is going to happen. Before such invasive changes are going to be
accepted there should be a clear understanding of how exactly TOASTers
are supposed to be used. This should be part of the documentation in
the patchset. Additionally there should be an open-soruce or
source-available extension that actually demonstrates the benefits of
TOASTers with reproducible benchmarks (we didn't even get to that part
yet).

> TOAST implementation is not necessary for Table AM.

What other use cases for TOAST do you have in mind?

>> > Have I answered your question? Please don't hesitate to point to any unclear
>> > parts, I'd be glad to explain that.
>>
>> No. To be honest, it looks like you are merely discarding most/any
>> feedback the community provided so far.
>>
>> I really think that pluggable TOASTers would be a great feature.
>> However if the goal is to get it into the core I doubt that we are
>> going to make much progress with the current approach.

To clarify, the concern about "N TOASTers vs M TableAM" was expressed
by Robert Haas back in Jan 2022:

> I agree ... but I'm also worried about what happens when we have
> multiple table AMs. One can imagine a new table AM that is
> specifically optimized for TOAST which can be used with an existing
> heap table. One can imagine a new table AM for the main table that
> wants to use something different for TOAST. So, I don't think it's
> right to imagine that the choice of TOASTer depends solely on the
> column data type. I'm not really sure how this should work exactly ...
> but it needs careful thought.

This is the most important open question so far to my knowledge. It
was never addressed, it doesn't seem like there is a plan of doing so,
the suggested alternative approach was ignored, nor are there any
strong arguments that would defend this design choice and/or criticize
the alternative one (other than general words "don't worry we know
what we are doing").

This what I mean by the community feedback being discarded.

--
Best regards,
Aleksander Alekseev

Hi Nikita,

> > > TOAST implementation is not necessary for Table AM.
>
> >What other use cases for TOAST do you have in mind?
>
> The main use case is the same as for the TOAST mechanism - storing and retrieving
> oversized data. But we expanded this case with some details -
> - update TOASTed data (yes, current TOAST implementation cannot update stored
> data - is marks whole TOASTED object as dead and stores new one);
> - retrieve part of the stored data chunks without fully de-TOASTing stored data (even
> with existing TOAST this will be painful if you have to get just a small part of the several
>  hundreds Mb sized object);
> - be able to store objects of size larger than 1 Gb;
> - store more than 4 Tb of TOASTed data for one table;
> - optimize storage for fast search and retrieval of parts of TOASTed object - this is
> must-have for effectively using JSON, PostgreSQL already is in catching-up position
> in JSON performance field.

I see. Actually most of this is what TableAM does. We just happened to
give this part of TableAM a separate name. The only exception is the
last case, when you create custom TOASTers for particular types and
potentially specify them for the given column.

All in all, this makes sense.

> Right. that's why we propose a validate method  (may be, it's a wrong
> name, but I don't known better one) which accepts several arguments, one
> of which is table AM oid. If that method returns false then toaster
> isn't useful with current TAM, storage or/and compression kinds, etc.

OK, I missed this message. So there was some misunderstanding after
all, sorry for this.

That's exactly what I wanted to know. It's much better than allowing
any TOASTer to run on top of any TableAM.

--
Best regards,
Aleksander Alekseev