Re: Pluggable toaster - Mailing list pgsql-hackers

Hi Nikita,

> I'm going to submit a more detailed code review soon.

As promised, here is my feedback.

```
+      Toaster could be assigned to toastable column with expression
+      <literal>STORAGE external TOASTER
<replaceable>toaster_name</replaceable></literal>
```

1.1. Could you please clarify what is the motivation behind such a
verbose syntax?

```
+Please note that custom Toasters could be assigned only to External
+storage type.
```

1.2. That's odd. TOAST should work for EXTENDED and MAIN storage
strategies as well. On top of that, why should custom TOASTers have
any knowledge of the default four-stage algorithm and the storage
strategies? If the storage strategy is actually ignored, it shouldn't
be used in the syntax.

```
+Toasters could use any storage, advanced compression, encryption, and
```

2. Although it's possible to implement some encryption in a TOASTer I
don't think the documentation should advertise this.

```
+typedef struct TsrRoutine
+{
+    NodeTag        type;
+
+    /* interface functions */
+    toast_init init;
+    toast_function toast;
+    update_toast_function update_toast;
+    copy_toast_function copy_toast;
+    detoast_function detoast;
+    del_toast_function deltoast;
+    get_vtable_function get_vtable;
+    toastervalidate_function toastervalidate;
+} TsrRoutine;
```

3.1. I believe we should rename this to something like `struct
ToastImpl`. The `Tsr` abbreviation only creates confusion, and this is
not a routine.
3.2. The names of the fields should be made consistent - e.g. if you
have update_toast and copy_toast then del_toast should be renamed to
delete_toast.
3.2. Additionally, in some parts of the path del_toast is used, while
in others - deltoast.

4. The user documentation should have clear answers on the following questions:
4.1. What will happen if the user tries to delete a TOASTer while
still having data that was TOASTed using this TOASTer? Or this is not
supported and the TOASTers should exist in the database indefinitely
after creation?
4.2. Is it possible to delete and/or alter the default TOASTer?
4.3. Please make sure the previously discussed clarification regarding
"N TOASTers vs M TableAMs" and the validation function is explicitly
present.

```
Toaster initialization. Initial TOAST table creation and other startup
preparations.
```

5.1. The documentation should clarify how many times init() is called
- is it done once for the TOASTer, once per relation, etc.
5.2. Why there is no free() method?

```
Updates TOASTed value. Returns new TOAST pointer.
```

6. It's not clear for what reason update_toast() will be executed to
begin with. This should be clarified in the documentation. How does it
differ from copy_toast()?

```
Validates Toaster for given data type, storage and compression options.
```

7. It should be explicitly stated when validate() is called and what
happens depending on the return value.

```
+Virtual Functions Table API Extension
```

8. IMO this section does a very poor job in explaining what this is
for and when I may want to use this; what particular problem are we
addressing here?

9. There are typos in the comments and the documentation,
s/Vaitual/Virtual/, s/vurtual/virtual/ etc. Also there are missing
articles. Please run your patches through a spellchecker.

I suggest we address this piece of feedback before proceeding further.

-- 
Best regards,
Aleksander Alekseev