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

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