Hi Michael I understand from some earlier messages that in one of the patches you refactored the code to make it easier to add new VARLENA on disk types.
Where should I start looking for this? ----- Hannu On Thu, Aug 6, 2026 at 6:39 AM Michael Paquier <[email protected]> wrote: > > On Thu, Jul 30, 2026 at 05:01:02PM +0900, Yugo Nagata wrote: > > On Thu, 30 Jul 2026 13:38:18 +0900 Yugo Nagata <[email protected]> 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
