Hi Robert, Thanks for sparing part of your precious time to look at the patch. I acknowledge it is a very complex one. Since you're going to take another look, providing some preliminary comments related to some of the implementation concerns.
> I don't understand how this would be safe against interrupts or > errors. If a running query is interrupted, what would cause > ProgressiveExplainCleanup() to be called? If the answer is "nothing," > why isn't that unsafe? The strategy I used here is to use a MemoryContextCallback (ProgressiveExplainReleaseFunc), configured in the memory context where the query is being executed, being responsible for calling ProgressiveExplainCleanup() if the query doesn't end gracefully. > It looks very strange to me that ProgressiveExplainPrint() seems to > have a theoretical possibility of generating the output and then > throwing it away if we end up with entry == NULL. I guess maybe that > case is not supposed to happen because ProgressiveExplainInit() is > supposed to create the entry, but then why isn't this an elog(ERROR) > or something instead of a no-op? Agreed. Will fix this. > It seems like when we replace a longer entry with a shorter one, we > forget that it was originally longer. Hence, if the length of a > progressive EXPLAIN is alternately 2922 characters and 2923 > characters, we'll reallocate on every other progressive EXPLAIN > instead of at most once. Are you talking about re-printing the plan in the same query execution? The logic for the code, using your example, would be to allocate 2922 + PROGRESSIVE_EXPLAIN_FREE_SIZE (4096, currently) initially. If next plans alternate between 2922 and 2923 no additional allocation will be done. A reallocation will be needed only if the plan length ends up exceeding 2922+4096. At the end of query execution (or cancellation) that DSA will be freed and a next query execution will have to allocate again using the same logic. Regarding the execProcNode wrapper strategy. It used it precisely because of the discussion in that other patch. I actually tried not using it here, and call ProgressiveExplainPrint() in the timeout callback. This resulted in sporadic crashes, confirming the suspicion that it is not a good idea. Regarding all other comments related to variable/function names and having the feature in a separate file, agree with all the comments. Will send a new version with the fixes. Rafael.
