On Thu, 30 Jul 2026 13:38:18 +0900 Yugo Nagata <[email protected]> wrote:
> On Fri, 8 May 2026 15:07:13 +0900 > Michael Paquier <[email protected]> wrote: > > > Hi all, > > > > This is a follow-up of the previous thread about $subject, with a > > reworked patch set for discussion in v20, as I care about the subject: > > https://www.postgresql.org/message-id/[email protected] > > > > The main feedback of the previous thread is that the previous > > implementation with its callbacks for each vartag was not liked much, > > and their were concerns with pointer redirections and performance. > > This patch set uses what I am calling the "brutal" approach, relying > > on a vartag_external of a varlena or the atttype of the TOAST relation > > to decide which external toast pointer we should use. This uses no > > function pointers, and patches the code to deal with Oid or Oid8 TOAST > > values where it matters. So, this time, performance cannot really be > > an issue. > > > > The patch set is structured so as all the ground work happens first > > (most of it comes from the previous patch set, reorganized a bit), and > > the introduction of the varatt pieces are last, based on the following > > rules: > > - A table can use a 8-byte TOAST value with a new reloption, named > > toast_value_type that can be set to "oid" (default) or "oid8", > > creating a TOAST table with a value of the assigned type. This > > includes support for dumps as well as binary upgrades, so as the > > atttype of the chunk_id of the TOAST table is preserved. A table with > > a TOAST type assigned cannot be changed to a different type through a > > VACUUM FULL or a rewrite, as a matter of implementation simplicity. > > - Renames and cleanup of various areas related to varatt_external, > > renaming things to use OID. > > - The 8-byte TOAST values rely on a Oid8, whose value is retrieved > > from the control file extended by 4 bytes. The code supports > > wraparound of values so as we don't assign anything between 0 and > > FirstNormalObjectId for the lower bytes, same way as before. > > > > The last patch introduces a new vartag_external and the new > > varatt_external_oid8, with an Oid8 as value. Well, not exactly, the > > patch uses two uint32 fields so as the structure is packed without > > padding, as of: > > typedef struct varatt_external_oid8 > > { > > int32 va_rawsize; > > uint32 va_extinfo; > > uint32 va_valueid_lo; > > uint32 va_valueid_hi; > > Oid va_toastrelid; > > } varatt_external_oid8; > > > > Note that if applying all the patches except the last one, the code > > would use an varatt_external_oid with an oid8 TOAST table. This > > works, the split is to make reviews easier. An oid8 TOAST table > > always uses a varatt_external_oid8. > > > > I have done a lot of back-and-forth in the patch to try to find a good > > balance between the manipulation of the varlenas in the detoast and > > compression paths, as well as reorderbuffer.c and amcheck. And I have > > finished with the attached, which is kind of nice. The last patch has > > a low footprint: > > 9 files changed, 537 insertions(+), 208 deletions(-) > > I applied the patches to the master branch and run a simple test. > > After running: > > $ pg_resetwal -o 4300000000 -D data > > CREATE TABLE tbl_oid8(t text) with (toast_value_type = 'oid8'); > ALTER TABLE tbl_oid8 ALTER t SET STORAGE EXTERNAL; > CREATE TABLE tbl_oid(t text) with (toast_value_type = 'oid'); > ALTER TABLE tbl_oid ALTER t SET STORAGE EXTERNAL; > > INSERT INTO tbl_oid8 VALUES (repeat(md5('x'),100)); > INSERT INTO tbl_oid VALUES (repeat(md5('x'),100)); > > SELECT tableoid, t = repeat(md5('x'),100) FROM tbl_oid8; > tableoid | ?column? > ----------+---------- > 5032704 | t > (1 row) > > SELECT chunk_id, chunk_seq FROM pg_toast.pg_toast_5032704; > chunk_id | chunk_seq > ------------+----------- > 4300000010 | 0 > 4300000010 | 1 > (2 rows) > > SELECT tableoid, t = repeat(md5('x'),100) FROM tbl_oid; > tableoid | ?column? > ----------+---------- > 5032709 | t > (1 row) > > SELECT chunk_id, chunk_seq FROM pg_toast.pg_toast_5032709; > chunk_id | chunk_seq > ----------+----------- > 5032715 | 0 > 5032715 | 1 > (2 rows) > > This appears to work as expected. 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: Prepare: drop table if exists tbl; create table tbl (id int primary key, j int, t text); alter table tbl alter column t set storage external; insert into tbl select i, i, repeat(md5(i::text),100) from generate_series(1,10000) i; query.sql: \set id random(1, 10000) update tbl set (j,t) = (j + 1, repeat(md5((j+1)::text),100)) where id = :id; 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 I could not observe any performance degradation with the curernt patches, at least in my environment. I haven't tested whether the previous version of the patches showed the performance degradation that has been a concern, though. > > I noticed one issue, though. > When toast_value_type is specified as something other than oid or oid8, the > error > message is: > > 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 also have a few small comments on the patches. > > - 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. > > (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. > > (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. > > - 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? > > (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) > > ? > > (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? I have one additional, very trivial comment: (7) /* - * Open the toast relation and its indexes + * Determine the pointer type from the datum's vartag and extract the + * toast relation OID and value ID accordingly. The vartag tells us + * everything we need ― no TOAST table schema lookup required. 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. Regards, Yugo Nagata -- Yugo Nagata <[email protected]>
