Re: Make tuple deformation faster

Hi,

On 2024-12-05 01:42:36 +1300, David Rowley wrote:
> On Tue, 3 Dec 2024 at 16:54, David Rowley <dgrowleyml@gmail.com> wrote:
> > After making some last-minute cosmetic adjustments, I've just pushed
> > the 0001 patch.
> 
> So that commit broke all of the debug_parallel_query = regress
> buildfarm animals.

Seems we need a query in the regression tests that trigger the issue even
without debug_parallel_query = regress.


> In the attached v7-0001 patch, I've now got rid of the
> TupleDescData->attrs field.  Figuring out the base address of the
> FormData_pg_attribute array is now done by the TupleDescAttr() inline
> function (this used to be a macro).

Possibly stupid idea: Could we instead store the attributes *before* the main
TupleDescData, with increasing "distance" for increased attnos?  That way we
wouldn't need to calculate any variable offsets. Of course the price would be
to have some slightly more complicated invocation of pfree(), but that's
comparatively rare.


> Because that function is called often in a tight loop, I wanted to ensure
> compilers wouldn't calculate the base address of the array on each iteration
> of the loop.  Looking at [1], it seems gcc and clang are smart about this
> and calculate the base address before the loop and add
> sizeof(FormData_pg_attribute) to the register that's being used for the
> element pointer.
> 
> Changing this then caused some other issues... The GIST code was doing
> the following to get a TupleDesc without the INCLUDE columns of the
> index:
> 
> giststate->nonLeafTupdesc = CreateTupleDescCopyConstr(index->rd_att);
> giststate->nonLeafTupdesc->natts =
> IndexRelationGetNumberOfKeyAttributes(index);

That is somewhat ugly...


> Since I'm calculating the base address of the FormData_pg_attribute
> array in TupleDesc by looking at natts, when this code changes natts
> on the fly, that means calls to TupleDescAttr end up looking in the
> wrong place for the required FormData_pg_attribute element.

It's possible out-of-core code is doing that too, could we detect this in
assert enabled builds?


> To fix this I invented CreateTupleDescTruncatedCopy() and used it in all the
> places that were fiddling with the natts field.

Makes sense.


> v7-0002 is not for commit. That's me cheekily exploiting the CFBot's CI
> testing machines to run in debug_parallel_query = regress mode.

I think it might actually make sense to enable debug_parallel_query = regress
in one of the CI tasks... Perhaps FreeBSD? That's reasonably fast right now
(and a *LOT* cheaper to run than macos).

Greetings,

Andres Freund