Re: Detoasting optionally to make Explain-Analyze less misleading

On Wed, 3 Apr 2024 at 23:50, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> writes:
>> On Tue, 2 Apr 2024 at 17:47, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> IIUC, it's not possible to use the SERIALIZE option when explaining
>>> CREATE TABLE AS, because you can't install the instrumentation tuple
>>> receiver when the IntoRel one is needed. I think that's fine because
>>> no serialization would happen in that case anyway, but should we
>>> throw an error if that combination is requested? Blindly reporting
>>> that zero bytes were serialized seems like it'd confuse people.
>
>> I think it's easily explained as no rows were transfered to the
>> client. If there is actual confusion, we can document it, but
>> confusing disk with network is not a case I'd protect against. See
>> also: EXPLAIN (ANALYZE, SERIALIZE) INSERT without the RETURNING
>> clause.
>
> Fair enough. There were a couple of spots in the code where I thought
> this was important to comment about, though.

Yeah, I'll agree with that.

>>> However, isn't the bottom half of serializeAnalyzeStartup doing
>>> exactly what the comment above it says we don't do, that is accounting
>>> for the RowDescription message? Frankly I agree with the comment that
>>> it's not worth troubling over, so I'd just drop that code rather than
>>> finding a solution for the magic-number problem.
>
>> I'm not sure I agree with not including the size of RowDescription
>> itself though, as wide results can have a very large RowDescription
>> overhead; up to several times the returned data in cases where few
>> rows are returned.
>
> Meh --- if we're rounding off to kilobytes, you're unlikely to see it.
> In any case, if we start counting overhead messages, where shall we
> stop? Should we count the eventual CommandComplete or ReadyForQuery,
> for instance? I'm content to say that this measures data only; that
> seems to jibe with other aspects of EXPLAIN's behavior.

Fine with me.

> > Removed. I've instead added buffer usage, as I realised that wasn't
> > covered yet, and quite important to detect excessive detoasting (it's
> > not covered at the top-level scan).
>
> Duh, good catch.
>
> I've pushed this after a good deal of cosmetic polishing -- for
> example, I spent some effort on making serializeAnalyzeReceive
> look as much like printtup as possible, in hopes of making it
> easier to keep the two functions in sync in future.

Thanks for the review, and for pushing!

-Matthias