On Wed, 3 Apr 2024 at 23:50, Tom Lane <t...@sss.pgh.pa.us> wrote: > > Matthias van de Meent <boekewurm+postg...@gmail.com> writes: >> On Tue, 2 Apr 2024 at 17:47, Tom Lane <t...@sss.pgh.pa.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