Re: Support for 8-byte TOAST values, round two

Hi Michael

I understand from some earlier messages that in one of the patches you
refactored the code to make it easier to add new VARLENA on disk
types.

Where should I start looking for this?

-----
Hannu

On Thu, Aug 6, 2026 at 6:39 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Thu, Jul 30, 2026 at 05:01:02PM +0900, Yugo Nagata wrote:
> > On Thu, 30 Jul 2026 13:38:18 +0900 Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
> > I also run a simple performance test. I measured the TPS on
> > the master branch and with the current patches applied under the
> > following conditions:
> >
> > Running pbench three times:
> >
> > $ pgbench test -f query.sql -c 8 -j 4 -T 100
> >
> > Results:
> >
> > master: 2686.229104, 2697.655096, 2687.894343
> > patched: 2685.292256, 2692.080214, 2690.005753
>
> Thanks for the test.
>
> > I haven't tested whether the previous version of the patches showed the
> > performance degradation that has been a concern, though.
>
> I am not sure that it is worth doing; I am not planning to get back to
> these versions, but it's true that it could be useful to see if your
> environment shows an impact of some impact. That's low priority, IMO.
>
> >> postgres=# CREATE TABLE tbl(t text) with (toast_value_type = 'x');
> >> ERROR: invalid value for enum option "toast_value_type": x
> >> DETAIL: Valid values are "oid".
> >>
> >> The detail message should be updated to include oid8.
>
> I had this one already fixed on my local branch.
>
> >> - 0009
> >>
> >> (1)
> >> The commit message says:
> >>
> >> TOAST pointers still rely on varatt_external and a single vartag, with
> >> all the values inserted in the bigint TOAST tables fed from the existing
> >> OID value generator.
> >>
> >> This made me a bit confused because chunk_ids insereted into TOAST tables using oid8
> >> are generated by GetNewObjectId8().
> >>
> >> + else if (toast_typid == OID8OID)
> >> + toast_pointer.va_valueid = GetNewObjectId8();
> >>
> >> This value is later cast to Oid here, so what is actually stored is a 4-byte chunk_id,
> >> but saying that the values are "fed from the existing OID value generator" seems
> >> a bit inaccurate to me.
>
> Okay, edited that a bit. I tend to rework all my commit messages
> before pushing, still you are right that this sounded a bit confusing.
>
> >> (2)
> >> /*
> >> - * Choose an OID to use as the value ID for this toast value.
> >> + * Choose a new value to use as the value ID for this toast value, be it
> >> + * for OID or int8-based TOAST relations.
> >>
> >> This patch relies on oid8, introduced by b139bd3b6ef, rather than int8, so, I
> >> wonder if this comment sould also be updated.
>
> Fixed.
>
> >> (3)
> >> belonging to the owning table. Every
> >> -<acronym>TOAST</acronym> table has the columns <structfield>chunk_id</structfield> (an OID
> >> -identifying the particular <acronym>TOAST</acronym>ed value),
> >> +<acronym>TOAST</acronym> table has the columns
> >> +<structfield>chunk_id</structfield> (an OID or an 8-byte integer identifying
> >> +the particular <acronym>TOAST</acronym>ed value),
> >> <structfield>chunk_seq</structfield> (a sequence number for the chunk within its value),
> >>
> >> Similarly, I think the documentation should mention oid8 rather than 8-byte
> >> integer. That would also be consistent with the CREATE TABLE documentation for
> >> the toast_value_type parameter.
>
> Hole in the docs, indeed.
>
> >> - 0011
> >>
> >> (4)
> >> + /*
> >> + * Check if this value already exists in the new toast
> >> + * table (corner case during table rewrite with multiple
> >> + * versions of the same row).
> >> + */
> >> + if (toastrel_valueid_exists(toastrel, va_valueid))
> >> + {
> >> + /* Match, so short-circuit the data storage loop below */
> >> + data_todo = 0;
> >> + }
> >>
> >> The same code appears later, together with a more detailed comment. How about
> >> moving the detailed comment here instead, or simply referring to
> >> the later comment?
>
> FWIW, I was looking again at this block of code, and reminded myself
> why this piece is this way: I want GetNewObjectId8(), InvalidOid8 and
> varatt_external_oid8 to have their own references in each block
> depending on the type of chunk_id. Perhaps a bit repetitive,
> efficient at least.
>
> >> (5)
> >> +/* Is varlena datum a pointer to on-disk toasted data with 8-byte value ID? */
> >> +static inline bool
> >> +VARATT_IS_EXTERNAL_ONDISK_OID8(const void *PTR)
> >> +{
> >> + return VARATT_IS_EXTERNAL(PTR) && VARTAG_EXTERNAL(PTR) == VARTAG_ONDISK_OID8;
> >> }
> >>
> >> This macro is defined but not used. Is it intended to be used in places that
> >> check VARTAG_ONDISK_OID8 directly, for example:
> >>
> >> + if (VARTAG_EXTERNAL(attr) == VARTAG_ONDISK_OID8)
> >>
> >> ?
>
> Consistency with the other one, for extensions, as all the code paths
> checking for VARTAG_ONDISK_OID8 already make sure that we are dealing
> with an external pointer, so I did not see a point in checking
> VARATT_IS_EXTERNAL_ONDISK_OID8, that would make the extra
> VARATT_IS_EXTERNAL() check pointless.
>
> >> (6)
> >>
> >> any. Allowing for the varlena header bytes,
> >> -the total size of an on-disk <acronym>TOAST</acronym> pointer datum is therefore 18
> >> -bytes regardless of the actual size of the represented value.
> >> +the total size of an on-disk <acronym>TOAST</acronym> pointer datum is 18
> >> +bytes when using an OID as <structfield>chunk_id</structfield>, or 22 bytes
> >> +when using an 8-byte integer, regardless of the actual size of the represented
> >> +value.
> >> </para>
> >>
> >> Should this also refer to Oid8 rather than 8-byte integner, for consistency?
>
> Fixed.
>
> > (7)
> >
> > Non-ASCII hyphen is used in this comment. This may not be prohibitted,
> > but I don't think it's common in the PostgreSQL code base.
>
> Seems like my keyboard has slipped a bit here. Will fix after
> replying to your other comments.
> --
> Michael