Re: Dead code with short varlenas in toast_save_datum()

Hi!

Michael, as far as I understand while externalizing tuple we always check
tuple size with toast_tuple_find_biggest_attribute(), where
biggest attribute size is always >= MAXALIGN(TOAST_POINTER_SIZE)
so currently I cannot see how it is possible to get into VARATT_IS_SHORT
branch.

I've commented this code and done some tests and did not see any unexpected
behavior. It certainly looks like some legacy code left "just because".

On Mon, Aug 4, 2025 at 6:16 AM Michael Paquier <michael@paquier.xyz> wrote:

> Hi all,
>
> Nikhil (in CC.), has noticed while looking at the 8B-value TOAST patch
> that this code block used for short varlenas in toast_save_datum() is
> dead based on the current coverage:
>     if (VARATT_IS_SHORT(dval))
>     {
>         data_p = VARDATA_SHORT(dval);
>         data_todo = VARSIZE_SHORT(dval) - VARHDRSZ_SHORT;
>         toast_pointer.va_rawsize = data_todo + VARHDRSZ;    /* as if not
> short */
>         toast_pointer.va_extinfo = data_todo;
>     }
>
> Coverage link:
>
> https://coverage.postgresql.org/src/backend/access/common/toast_internals.c.gcov.html
>
> toast_save_datum() is taken only through toast_tuple_externalize(),
> and I was assuming first that an EXTERNAL attribute with a minimal
> toast_tuple_target of 128B would have been enough to trigger this,
> still even a minimal bound is not enough to trigger
> heap_toast_insert_or_update(), which heap_prepare_insert() would call
> if:
> - the hardcoded check on TOAST_TUPLE_THRESHOLD is reached, which of
> course would not happen.
> - tuple is marked with HEAP_HASEXTERNAL, something that happens only
> if fill_val() finds an external varlena (aka VARATT_IS_EXTERNAL),
> and that does not happen for SHORT varlenas.
>
> The insertion of short varlenas in TOAST is old enough to vote and it
> assumed as supported when reading external on-disk TOAST datums, still
> it's amazing to see that there no in-core coverage for it.
>
> Anyway, There is an action item here.  I don't see a SQL pattern that
> would trigger this code path on HEAD (perhaps I lack the imagination
> to do so), so the only alternative I can think of is the introduction
> of a C function in regress.c to force our way through, calling
> directly toast_save_datum() or something equivalent, so as
> toast_save_datum() is tested with more varlena patterns.  That would
> be something similar to what we do for indirect TOAST tuples.
>
> Another possibility is to assume that this code is dead.  But I doubt
> that it is possible to claim that as the TOAST code is assumed as
> being usable by out-of-core code, so depending on how fancy things are
> being done a TOAST relation could use a short varatt for an insert.
>
> Thoughts?
> --
> Michael
>


-- 
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/