On Fri, Mar 13, 2026 at 8:19 PM David Rowley <[email protected]> wrote: > > Thanks for having a look. > > > On Sun, 8 Mar 2026 at 05:36, Junwang Zhao <[email protected]> wrote: > > I have some comments on v12-0004. > > > > 1. > > > > + off += cattr->attlen; > > + firstNonCachedOffsetAttr = i + 1; > > + } > > + > > + tupdesc->firstNonCachedOffsetAttr = firstNonCachedOffsetAttr; > > + tupdesc->firstNonGuaranteedAttr = firstNonGuaranteedAttr; > > +} > > > > The firstNonCachedOffsetAttr seems to be the first variable width > > attribute, but it seems that the offset of this attribute can be cached, > > for example, in a table defined as (int, text), the offset of > > firstNonCachedOffsetAttr should be 4, is that correct? > > Yes. > > > If TupleDescFinalize records the offset firstNonCachedOffsetAttr, > > it might save one iterator of the deforming loop. For example, > > add something like the following after the above mentioned code. > > > > if (firstNonCachedOffsetAttr < tupdesc->natts) > > { > > cattr = TupleDescCompactAttr(tupdesc, firstNonCachedOffsetAttr); > > cattr->attcacheoff = off; > > } > > The problem is that short varlenas have 1 byte alignment and normal > varlenas have 4 byte alignment. It might be possible to do something > if the previous column is 4-byte aligned and has a length of 4 or 8, > since that means the offset must also be 1-byte aligned. The main > reason I don't want to do this is that the only positive is that > *maybe* 1 extra column can be deformed with a fixed offset. The > drawback is that the following code *has* to use > att_addlength_pointer(), *regardless* instead of "off += attlen;". > This means more deforming code and more complexity in > TupleDescFinalize(). I'd rather not do this.
That explains, thanks. > > > 2. > > > > in slot_deform_heap_tuple, there are multiple statements setting > > firstNonCacheOffsetAttr, > > > > + firstNonCacheOffsetAttr = tupleDesc->firstNonCachedOffsetAttr; > > > > + /* We can only use any cached offsets until the first NULL attr */ > > + firstNonCacheOffsetAttr = Min(firstNonCacheOffsetAttr, > > + firstNullAttr); > > > > + /* We can only fetch as many attributes as the tuple has. */ > > + firstNonCacheOffsetAttr = Min(firstNonCacheOffsetAttr, natts); > > > > Based on the logic, it seems the second one could be moved > > to the third position, and the third one could then be safely > > removed? > > Yeah. Well spotted. I've done that in the attached. > > I've also modified the 0006 patch to add a new deform_bench_select() > function which allows the benchmark to call the new selective deform > function. See the attached graphs comparing master to v13-0001-0005 > and master to v13-0001-0006. It's good to see that there's still quite > a large speedup even from the tests that don't have an attcacheoff for > the column being deformed. Tests 1 and 5 do have a attcacheoff for the > column deformed, so they're a good bit faster again. To get the > 0001-0006 results, I used the deform_test_run.sh script from [1] and > modified it to call deform_bench_select() instead of deform_bench(). > > I also noticed that when building with older gcc versions, I was > getting warnings about attlen and 'off' not being initialised. I ended > up switching back to the do/while loops to fix that rather than adding > needless initialisation, which would add overhead. 1 loop is > guaranteed, and the older compiler is not clever enough to work that > out. > > David > > [1] > https://postgr.es/m/caaphdvo1i-ycacwnk3l7zastum8mw46kvrqmauhd46hsujm...@mail.gmail.com -- Regards Junwang Zhao
