MIME-Version: 1.0
References: 
 <CAOj6k6dkseVvZzmEAWvBd6twZsCU0DbN+qeM7CoDuMM3r9doiw@mail.gmail.com>
 <CAOj6k6eSsQ_PTUTxW0upn6wp7tzbvQwqkQco-=+majwX8p6JJg@mail.gmail.com>
 <4c70b0f6-5aba-464c-b145-464a620c1222@eisentraut.org>
 <aUOgmypuun0rc2xO@paquier.xyz>
 <CAOj6k6fxs0Bwjr34W4aURzFPta0DTGLN-1ic-U+-M_EqJ+Wd8A@mail.gmail.com>
 <aUSDs7SaEcOa65gD@paquier.xyz>
 <CABV9wwOdeFHHNnw+rUsVnFek-oqwsJnJX9KFG6AT5Yqz5RA_vg@mail.gmail.com>
 <aUyEWfBXYBLKGkKQ@paquier.xyz>
 <CAOj6k6dG0M-G0gs_Htray5y_pfvacXSjKYw0uJaaYJvOeqk7xw@mail.gmail.com>
 <CAOj6k6dEVi0NvLjMLDhyrJS_n_NZO5D_OU89AO1u53u6NCDDwQ@mail.gmail.com>
 <aVMSqzx7dGSDu4uG@paquier.xyz>
In-Reply-To: <aVMSqzx7dGSDu4uG@paquier.xyz>
From: Dharin Shah <dharinshah95@gmail.com>
Date: Wed, 31 Dec 2025 16:02:24 +0100
Message-ID: 
 <CAOj6k6eSVoz8Z-i6u2vGgofid6yXenb1a2AhbBkfi-N11jorHA@mail.gmail.com>
Subject: Re: Fwd: [PATCH] Add zstd compression for TOAST using extended header
 format
To: Michael Paquier <michael@paquier.xyz>
Cc: Robert Treat <rob@xzilla.net>, Peter Eisentraut <peter@eisentraut.org>,
	pgsql-hackers@lists.postgresql.org
Content-Type: multipart/alternative; boundary="0000000000003dbc7e064740c4a3"
Archived-At: 
 <https://www.postgresql.org/message-id/CAOj6k6eSVoz8Z-i6u2vGgofid6yXenb1a2AhbBkfi-N11jorHA%40mail.gmail.com>
Precedence: bulk

--0000000000003dbc7e064740c4a3
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Thanks Michael,

After looking more closely at your =E2=80=9C8=E2=80=91byte TOAST values / i=
nfinite loop=E2=80=9D
thread and patch series, I see this is very much the same direction you
outlined there: introduce a normalized in-memory representation for
external pointers (toast_external_data) and keep most call sites from
having to reason about vartag_external/va_extinfo details directly [1].

For this refactor patch I kept the decoder local to detoast.c to minimize
scope and avoid committing to a broader API boundary too early. But if the
consensus heads toward a shared interface closer to the format definitions
(as in your toast_external approach), I=E2=80=99m happy to respin/rework th=
is patch
to align with that direction, rather than working on parallel
abstractions. It should also be straightforward to mold this refactor in
the direction of the 8=E2=80=91byte value-id work without changing the over=
all
detoast structure.

On HAS_TCINFO flag: the intent is to make payload layout explicit. In the
current code, =E2=80=9Cexternal is compressed=E2=80=9D effectively implies =
=E2=80=9Cpayload begins
with tcinfo=E2=80=9D, which is wired into fetch/slice logic. For a vartag-b=
ased
follow-up (e.g., zstd), we may want compressed payloads without a tcinfo
prefix, so having an explicit flag keeps detoast paths uniform and avoids
method-specific shortcuts.

Let me know what you=E2=80=99d prefer for next steps: keep this patch as a
detoast-local refactor, or respin it to align more directly with a shared
decoded external-pointer interface in the direction of the 8=E2=80=91byte w=
ork.

[1]
https://www.postgresql.org/message-id/flat/CAN-LCVNsE4x0k11ZRWvU4ySTbe98fwA=
16qzV7p8dxogWnD5Jng%40mail.gmail.com#8253d63beab18b73706e3555ce19a0f4


Thanks,
Dharin

On Tue, Dec 30, 2025 at 12:46=E2=80=AFAM Michael Paquier <michael@paquier.x=
yz>
wrote:

> On Mon, Dec 29, 2025 at 02:45:27PM +0100, Dharin Shah wrote:
> > The goal is to centralize =E2=80=9Chow do we interpret an external datu=
m?=E2=80=9D so
> that
> > detoast code paths don=E2=80=99t have to reason directly about va_extin=
fo
> encoding
> > vs payload layout details. This is intended as groundwork for a follow-=
up
> > patch adding a new vartag-based method (e.g., zstd) without scattering
> > special cases in detoast paths.
>
> +static bool
> +decode_external_toast_pointer(const struct varlena *attr,
> +                                                  DecodedExternalToast
> *decoded)
> [...]
> +typedef enum ToastDecompressMethod
> +{
> +       TOAST_DECOMP_NONE =3D 0,
> +       TOAST_DECOMP_PGLZ =3D 1,
> +       TOAST_DECOMP_LZ4 =3D 2
> +} ToastDecompressMethod;
> +
> +typedef struct DecodedExternalToast
> +{
> +       Oid                     toastrelid;
> +       Oid                     valueid;
> +       uint32          rawsize;                /* Decompressed size; for
> future methods without tcinfo */
> +       uint32          extsize;                /* On-disk payload size *=
/
> +       ToastDecompressMethod method;
> +       uint8           flags;
> +} DecodedExternalToast;
>
> Yeah, honestly this is a layer I have been thinking about as well as
> one option, but contrary to you I have been focusing on putting that
> into varatt.h, with the exception of the value being an Oid8.  I think
> that you have an interesting point in focusing your implementation to
> be stored in the detoast part, though.  I'd need to spend a bit more
> time to see the result this would lead at with the larger 8-byte issue
> in mind, but this is something that would come at no real cost as it
> has no function pointer redirection compared to what I was first
> envisioning on the other thread.  That's especially true if it makes
> the CompressionId business easier to mold around when adding a new
> vartag.
>
> > Why HAS_TCINFO?
> > - Previously, =E2=80=9Cis compressed?=E2=80=9D was used as a proxy for =
whether the
> external
> > payload begins with tcinfo. This patch makes that explicit: HAS_TCINFO
> > captures payload layout, which is distinct from whether the value is
> > compressed. This separation is needed for future methods that may store
> > external compressed payloads without tcinfo.
>
> It is possible to model the on-memory data as we want.  This
> suggestion would be OK with some flags.
> --
> Michael
>

--0000000000003dbc7e064740c4a3
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Thanks Michael,<br><br>After looking more closely at your =
=E2=80=9C8=E2=80=91byte TOAST values / infinite loop=E2=80=9D thread and pa=
tch series, I see this is very much the same direction you outlined there: =
introduce a normalized in-memory representation for external pointers (toas=
t_external_data) and keep most call sites from having to reason about varta=
g_external/va_extinfo details directly [1].<br><br>For this refactor patch =
I kept the decoder local to detoast.c to minimize scope and avoid committin=
g to a broader API boundary too early. But if the consensus heads toward a =
shared interface closer to the format definitions (as in your toast_externa=
l approach), I=E2=80=99m happy to respin/rework this patch to align with th=
at direction, rather than working on parallel abstractions.=C2=A0It should =
also be straightforward to mold this refactor in the direction of the 8=E2=
=80=91byte value-id work without changing the overall detoast structure.<br=
><br>On HAS_TCINFO flag: the intent is to make payload layout explicit. In =
the current code, =E2=80=9Cexternal is compressed=E2=80=9D effectively impl=
ies =E2=80=9Cpayload begins with tcinfo=E2=80=9D, which is wired into fetch=
/slice logic. For a vartag-based follow-up (e.g., zstd), we may want compre=
ssed payloads without a tcinfo prefix, so having an explicit flag keeps det=
oast paths uniform and avoids method-specific shortcuts.<div><br></div><div=
>Let me know what you=E2=80=99d prefer for next steps: keep this patch as a=
 detoast-local refactor, or respin it to align more directly with a shared =
decoded external-pointer interface in the direction of the 8=E2=80=91byte w=
ork.</div><div><br></div><div>[1]=C2=A0<a href=3D"https://www.postgresql.or=
g/message-id/flat/CAN-LCVNsE4x0k11ZRWvU4ySTbe98fwA16qzV7p8dxogWnD5Jng%40mai=
l.gmail.com#8253d63beab18b73706e3555ce19a0f4">https://www.postgresql.org/me=
ssage-id/flat/CAN-LCVNsE4x0k11ZRWvU4ySTbe98fwA16qzV7p8dxogWnD5Jng%40mail.gm=
ail.com#8253d63beab18b73706e3555ce19a0f4</a>=C2=A0<br><br>Thanks,<br>Dharin=
</div></div><br><div class=3D"gmail_quote gmail_quote_container"><div dir=
=3D"ltr" class=3D"gmail_attr">On Tue, Dec 30, 2025 at 12:46=E2=80=AFAM Mich=
ael Paquier &lt;<a href=3D"mailto:michael@paquier.xyz">michael@paquier.xyz<=
/a>&gt; wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0=
px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">O=
n Mon, Dec 29, 2025 at 02:45:27PM +0100, Dharin Shah wrote:<br>
&gt; The goal is to centralize =E2=80=9Chow do we interpret an external dat=
um?=E2=80=9D so that<br>
&gt; detoast code paths don=E2=80=99t have to reason directly about va_exti=
nfo encoding<br>
&gt; vs payload layout details. This is intended as groundwork for a follow=
-up<br>
&gt; patch adding a new vartag-based method (e.g., zstd) without scattering=
<br>
&gt; special cases in detoast paths.<br>
<br>
+static bool<br>
+decode_external_toast_pointer(const struct varlena *attr,<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 DecodedExternalToast *decoded)<br>
[...]<br>
+typedef enum ToastDecompressMethod<br>
+{<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0TOAST_DECOMP_NONE =3D 0,<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0TOAST_DECOMP_PGLZ =3D 1,<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0TOAST_DECOMP_LZ4 =3D 2<br>
+} ToastDecompressMethod;<br>
+<br>
+typedef struct DecodedExternalToast<br>
+{<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0Oid=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0toastrelid;<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0Oid=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0valueid;<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0uint32=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 rawsiz=
e;=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Decompressed s=
ize; for future methods without tcinfo */<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0uint32=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 extsiz=
e;=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* On-disk payloa=
d size */<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0ToastDecompressMethod method;<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0uint8=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0f=
lags;<br>
+} DecodedExternalToast;<br>
<br>
Yeah, honestly this is a layer I have been thinking about as well as<br>
one option, but contrary to you I have been focusing on putting that<br>
into varatt.h, with the exception of the value being an Oid8.=C2=A0 I think=
<br>
that you have an interesting point in focusing your implementation to<br>
be stored in the detoast part, though.=C2=A0 I&#39;d need to spend a bit mo=
re<br>
time to see the result this would lead at with the larger 8-byte issue<br>
in mind, but this is something that would come at no real cost as it<br>
has no function pointer redirection compared to what I was first<br>
envisioning on the other thread.=C2=A0 That&#39;s especially true if it mak=
es<br>
the CompressionId business easier to mold around when adding a new<br>
vartag.<br>
<br>
&gt; Why HAS_TCINFO?<br>
&gt; - Previously, =E2=80=9Cis compressed?=E2=80=9D was used as a proxy for=
 whether the external<br>
&gt; payload begins with tcinfo. This patch makes that explicit: HAS_TCINFO=
<br>
&gt; captures payload layout, which is distinct from whether the value is<b=
r>
&gt; compressed. This separation is needed for future methods that may stor=
e<br>
&gt; external compressed payloads without tcinfo.<br>
<br>
It is possible to model the on-memory data as we want.=C2=A0 This<br>
suggestion would be OK with some flags.<br>
--<br>
Michael<br>
</blockquote></div>

--0000000000003dbc7e064740c4a3--