Re: [proposal] de-TOAST'ing using a iterator

On Mon, Sep 23, 2019 at 9:55 PM Binguo Bao <djydewang(at)gmail(dot)com> wrote:
>
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> 于2019年9月17日周二 上午5:51写道：
>> In broad terms this patch looks pretty good to me. I only have a small
>> quibble with this API definition in fmgr.h -- namely that it forces us
>> to export the definition of all the structs (that could otherwise be
>> private to toast_internals.h) in order to satisfy callers of this macro.
>> I am wondering if it would be possible to change detoast_iterate and
>> PG_DETOAST_ITERATE in a way that those details remain hidden -- I am
>> thinking something like "if this returns NULL, then iteration has
>> finished"; and relieve the macro from doing the "->buf->buf" and
>> "->buf->limit" checks. I think changing that would require a change in
>> how the rest of the code is structured around this (the textpos internal
>> function), but seems like it would be better overall.
>>
>> (AFAICS that would enable us to expose much less about the
>> iterator-related structs to detoast.h -- you should be able to move the
>> struct defs to toast_internals.h)
>>
>> Then again, it might be just wishful thinking, but it seems worth
>> considering at least.
>
> I've tied to hide the details of the struct in patch v11 with checking "need" pointer
> inside detoast_iterate function.

I took a brief look at v11 to see if there's anything I can do to help
it move forward. I'm not yet sure how it would look code-wise to
implement Alvaro and Tomas's comments upthread, but I'm pretty sure
this part means the iterator-related structs are just as exposed as
before, but in a roundabout way that completely defeats the purpose of
hiding internals:

--- a/src/include/access/detoast.h
+++ b/src/include/access/detoast.h
@@ -11,6 +11,7 @@
*/
#ifndef DETOAST_H
#define DETOAST_H
+#include "toast_internals.h"

That said, the idea behind the PG_DETOAST_ITERATE macro was my
suggestion so that text_position_next_internal() didn't have to call
the iterator function every time the needle advances, which caused a
noticeable performance penalty. The toast code has moved around quite
a bit since then, and I'm not sure of the best way forward.

Also, c60e520f6e0 changed the standard pglz decompression algorithm.
It might be worth it to see if those changes are applicable to the
iterator case. At least one of the improved comments could be brought
over.

--
John Naylor https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services