Re: Detoasting optionally to make Explain-Analyze less misleading

Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> writes:
> Attached is v9, which is rebased on master to handle 24eebc65's
> changed signature of pq_sendcountedtext.
> It now also includes autocompletion, and a second patch which adds
> documentation to give users insights into this new addition to
> EXPLAIN.

I took a quick look through this. Some comments in no particular
order:

Documentation is not optional, so I don't really see the point of
splitting this into two patches.

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'd lose the stuff about measuring memory consumption. Nobody asked
for that and the total is completely misleading, because in reality
we'll reclaim the memory used after each row. It would allow cutting
the text-mode output down to one line, too, instead of having your
own format that's not like anything else.

I thought the upthread agreement was to display the amount of
data sent rounded to kilobytes, so why is the code displaying
an exact byte count?

I don't especially care for magic numbers like these:

+ /* see printtup.h why we add 18 bytes here. These are the infos
+ * needed for each attribute plus the attribute's name */
+ receiver->metrics.bytesSent += (int64) namelen + 1 + 18;

If the protocol is ever changed in a way that invalidates this,
there's about 0 chance that somebody would remember to touch
this code.

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.

Don't bother with duplicating valgrind-related logic in
serializeAnalyzeReceive --- that's irrelevant to actual users.

This seems like cowboy coding:

+ self->destRecevier.mydest = DestNone;

You should define a new value of the CommandDest enum and
integrate this receiver type into the support functions
in dest.c.

BTW, "destRecevier" is misspelled...

regards, tom lane