Re: Detoasting optionally to make Explain-Analyze less misleading

Hi Tom, Hi Matthias,

you are right of course. I have looked at the code from printtup.c and
made a new version of the patch.

Thanks for the MemoryContextReset hint too (@Matthias)

This time is called  EXPLAIN(ANALYZE,SERIALIZE) (hey, it also sounds
nicer phonetically)

If the option SERIALIZE is active, the output functions are called and
they perform the detoasting, which I have even checked.

So things are better this way, however I hardcoded the output option
"Text" (format=0). In printtup.c there is an incoming array which
applies Text (format=0) or Binary (format=1) for each column
individually. I am not sure whether this is even needed. I left in the
if-statement from printtup.c which calls the binary output method of a
given type. The result of the output is ignored and apparently free'd
because of the memory-context-reset at the end.

Please also note, that I added a call to DestReceiver's rDestroy hook,
which was missing from explain.c before altogether.

Feedback is appreciated.

/Stepan


On 12.09.23 17:26, Tom Lane wrote:
> Matthias van de Meent <boekewurm+postgres@gmail.com> writes:
>> Hmm, maybe we should measure the overhead of serializing the tuples instead.
>> The difference between your patch and "serializing the tuples, but not
>> sending them" is that serializing also does the detoasting, but also
>> includes any time spent in the serialization functions of the type. So
>> an option "SERIALIZE" which measures all the time the server spent on
>> the query (except the final step of sending the bytes to the client)
>> would likely be more useful than "just" detoasting.
> +1, that was my immediate reaction to the proposal as well.  Some
> output functions are far from cheap.  Doing only the detoast part
> seems like it's still misleading.
>
> Do we need to go as far as offering both text-output and binary-output
> options?
>
>             regards, tom lane