Hi, Nikhil
On Fri, 25 Sep 2026 at 02:21, Nikhil Kumar Veldanda <[email protected]> wrote: > On Fri, Sep 25, 2026 at 1:04 AM Michael Paquier <[email protected]> wrote: >> >> On Thu, Sep 24, 2026 at 04:00:29PM -0700, Nikhil Kumar Veldanda wrote: >> > 0001 removes toast_compress_header and the TOAST_COMPRESS_* macros in >> > favor of the varatt.h accessors and >> > toast_compress_set_size_and_method(). >> >> In this one, I got mixed feelings about the addition of the new inline >> routine in toast_internals.h while we have already a set of two getter >> routines in varatt.h that interact with va_tcinfo, so I have moved an >> equivalent to varatt.h. An extra benefit of this move is that we >> don't need to include toast_compression.h in toast_internals.h >> anymore. Applied the result. Cool cleanup, thanks for that. > > Thanks. That is a better home for it, and it sets the pattern for the > long form: the format patch now adds > VARDATA_COMPRESSED_SET_TCINFO_LONG() next to > VARDATA_COMPRESSED_SET_TCINFO() in varatt.h, and > toast_compress_datum() picks one or the other based on > toast_compression_id_needs_cmid_byte(). The series does not touch > toast_internals.h at all anymore. One small change to the routine you > committed: its assertion on the method becomes "cmethod < > VARLENA_COMPRESS_METHOD_LONG", since from that patch on the value 3 is > a flag for the long form, not a method. > >> > 0002 adds toast_pointer_build(). >> >> +static varlena * >> +toast_pointer_build(vartag_external tag, const void *fixed, Size fixedsize) >> +{ >> + varlena *result; >> + >> + Assert(tag == VARTAG_ONDISK_OID || tag == VARTAG_ONDISK_OID8); >> + Assert(VARTAG_SIZE(tag) == fixedsize); >> + >> + result = (varlena *) palloc(VARHDRSZ_EXTERNAL + VARTAG_SIZE(tag)); >> + SET_VARTAG_EXTERNAL(result, tag); >> + memcpy(VARDATA_EXTERNAL(result), fixed, fixedsize); >> + >> + return result; >> +} >> >> In this one, is there a need for fixedsize at all? We can guess the >> size of the target based on the vartag and VARTAG_SIZE(), without >> needing an assertion. If this brings any kind of extra protection, I >> may have missed it.. > > Only a cross-check that the struct the caller filled is the one the > tag announces; without it, a mismatch would read four bytes past the > caller's local. With both callers sitting right next to the struct > they fill, that is not worth a parameter. Removed, and the size now > comes from VARTAG_SIZE(). In the format patch the position of the > method byte is derived from it as well, so the helper's inputs are > down to the tag, the struct and the method. > >> Rename *fixed to *ptr? Should this use a VARATT_IS_EXTERNAL_ONDISK() >> in the first assertion instead of declaring the two vartags? >> > > Both done. VARATT_IS_EXTERNAL_ONDISK() wants a datum rather than a > tag, so the assertion moves after SET_VARTAG_EXTERNAL() and checks the > result. A nice side effect is that the format patch does not need to > touch that assertion when it adds the long tags. > >> > 0003 adds compress_method to toast_external_data and makes the three >> > callers read it from there. >> >> /* Compressed attributes should have a valid compression >> method */ >> - cmid = >> VARATT_EXTINFO_GET_COMPRESS_METHOD(toast_ext_data.extinfo); >> + cmid = toast_ext_data.compress_method; >> >> It looks like we could remove VARATT_EXTINFO_GET_COMPRESS_METHOD(). >> That's kind of tempting.. > > Done in that patch. Once the callers read toast_external_data, the > only user left was toast_external_info_get() itself, which now shifts > the bits directly, and keeping the function around would only invite > code to bypass the struct. The format patch was removing it anyway, > so this just moves it earlier; the comment on > VARATT_EXTINFO_GET_EXTSIZE() points at compress_method instead. > > v4 attached, rebased on e27f3b2cad7 and renumbered now that the first > patch is in: > > 0001 adds toast_pointer_build(). > 0002 adds compress_method to toast_external_data and removes > VARATT_EXTINFO_GET_COMPRESS_METHOD(). > 0003 is the format change. > 0004 is zstd. > In the v4-0001 commit message, it says no behavior change. However, it seems an Assert introduced after setting vartag. Since the access/detoast.h include the access/toast_compression.h, should we remove the access/toast_compression.h in files that already include access/toash.h? $ grep -rl '#include[[:space:]]*[<"]access/detoast\.h[>"]' . \ | xargs grep -l '#include[[:space:]]*[<"]access/toast_compression\.h[>"]' ./src/backend/access/brin/brin_tuple.c ./src/backend/access/common/toast_compression.c ./src/backend/access/common/detoast.c ./src/backend/access/common/toast_internals.c ./src/backend/utils/adt/varlena.c ./contrib/amcheck/verify_heapam.c -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
