Matthias van de Meent <boekewurm+postg...@gmail.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