| From: | David Rowley <dgrowleyml(at)gmail(dot)com> |
|---|---|
| To: | Junwang Zhao <zhjwpku(at)gmail(dot)com> |
| Cc: | Andres Freund <andres(at)anarazel(dot)de>, John Naylor <johncnaylorls(at)gmail(dot)com>, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: More speedups for tuple deformation |
| Date: | 2026-03-13 12:18:46 |
| Message-ID: | CAApHDvqp_DGNkz3Rr29hPuSYdai5c_32mWSv2inrW2ufsE+qZQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Thanks for having a look.
On Sun, 8 Mar 2026 at 05:36, Junwang Zhao <zhjwpku(at)gmail(dot)com> 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.
> 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-ycAcWnK3L7ZASTuM8mW46kvRqMaUHD46HSuJmx7A@mail.gmail.com
| Attachment | Content-Type | Size |
|---|---|---|
| v13-0001-Introduce-deform_bench-test-module.patch | text/plain | 7.3 KB |
| v13-0002-Allow-sibling-call-optimization-in-slot_getsomea.patch | text/plain | 7.3 KB |
| v13-0003-Add-empty-TupleDescFinalize-function.patch | text/plain | 29.0 KB |
| v13-0004-Optimize-tuple-deformation.patch | text/plain | 81.1 KB |
| v13-0005-Reduce-size-of-CompactAttribute-struct-to-8-byte.patch | text/plain | 5.6 KB |
| v13-0006-WIP-Introduce-selective-tuple-deforming.patch | text/plain | 47.0 KB |
| master_v_v13-0001-0006.gif | image/gif | 119.8 KB |
| master_v_v13-0001-0005.gif | image/gif | 157.7 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Greg Sabino Mullane | 2026-03-13 12:24:04 | Re: Exit walsender before confirming remote flush in logical replication |
| Previous Message | Etsuro Fujita | 2026-03-13 12:16:49 | Re: Import Statistics in postgres_fdw before resorting to sampling. |