Dead code with short varlenas in toast_save_datum()
Michael Paquier <michael@paquier.xyz>
From: Michael Paquier <michael@paquier.xyz>
To: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Cc: Nikhil Kumar Veldanda <veldanda.nikhilkumar17@gmail.com>
Date: 2025-08-04T03:15:59Z
Lists: pgsql-hackers
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Add isolation test for TOAST value reuse during CLUSTER
- f57e214d1cbb 19 (unreleased) landed
-
Add SQL test for TOAST value allocations on rewrite
- 69f75d671475 19 (unreleased) landed
-
Add regression test for short varlenas saved in TOAST relations
- 225ebfe30a1a 19 (unreleased) landed
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