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 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? Regards, Yugo Nagata -- Yugo Nagata <[email protected]>
