On Wed, 21 Jan 2026 at 07:38, Andres Freund <andres@anarazel.de> wrote:
> I wonder if it's possible to split the patch - it's big enough to be
> nontrivial to review... Perhaps the finalization could be introduced
> separately from the patch actually making use of it?
Seems reasonable. I've done that in the attached 0001, which contains
a dummy macro for TupleDescFinalize() and all the required calls to
it.
> I wonder if we should somehow change the API of tupledesc creation, to make
> old code that doesn't have TupleDescFinalize() fail to compile, instead of
> just warn...
I don't have any ideas on how to do that. I could maybe imagine some
preprocessor magic if we always expected a CreateTupleDesc() and
TupleDescFinalize() in the same function, but the TupleDescFinalize()
may be required after any modification to the TupleDesc that could
invalidate the processing that's done within that function.
> Think it'd be worth adding an assertion to BlessTupleDesc that
> TupleDescFinalize has been called, I think that'll lead to easier to
> understand backtraces in a lot of cases. Particularly if you consider cases
> where BlessTupleDesc() will create a tupdesc in shared memory, that could then
> trigger an assertion failure in a parallel worker or such.
Modified.
> Maybe add an assert for cattr->attbyval? Just to avoid a bad debugging
> experience if somebody tries to extend this logic to
> e.g. non-null-fixed-width-byref columns?
I ended up removing the OPTIMIZE_BYVAL code in the attached. Over all
the machines I tested on, with the benchmark results I previously
shared, it seemed to cause a slowdown rather than a speedup. Perhaps
it can be refined and tried again later, but I've removed it for now
to reduce complexity.
> I also wonder if we could have assert-only crosschecking of the "real" offsets
> against the cached ones?
I've modified the code to do that. v5 patches attached.
Thanks for reviewing.
David
