Re: Duplicate Workers entries in some EXPLAIN plans

Andres Freund <andres(at)anarazel(dot)de> writes:
> I wonder if we could introduce a debug GUC that makes parallel worker
> acquisition just retry in a loop, for a time determined by the GUC. That
> obviously would be a bad idea to do in a production setup, but it could
> be good enough for regression tests? There are some deadlock dangers,
> but I'm not sure they really matter for the tests.

Hmmm .... might work. Seems like a better idea than "run it by itself"
as we have to do now.

> I'd personally move this into a separate function, given the patches
> moves the code around already. ExplainNode() is already hard enough to
> navigate...

Well, it was already inline in ExplainNode, so this just moved the
code a few lines. I'm not that excited about moving little bits of
that function out-of-line.

>> +/*
>> + * Begin or resume output into the set-aside group for worker N.
>> + */
>> +static void

> Would it make sense to make these functions non-static? It seems
> plausible that code for a custom node or such would want to add
> its own information?

Maybe, but seems to me that there'd be a whole lot of other infrastructure
needed to track additional per-worker state. I'd just as soon not
expose this stuff until (a) there's a concrete not hypothetical use case
and (b) it's been around long enough to feel comfortable that there's
nothing wrong with the design.

>> + /*
>> + * In TEXT format, prefix the first output line for this worker with
>> + * "Worker N:". Then, any additional lines should be indented one more
>> + * stop than the "Worker N" line is.
>> + */

> I don't quite get the Worker %d bit. Why are we not outputting that in
> the !worker_inited block?

We might strip it off again in ExplainCloseWorker, and then potentially
add it back again in a later ExplainOpenWorker. That could only happen
if an earlier ExplainOpen/CloseWorker fragment decides not to emit any
text and then a later one wants to do so. Which I'm pretty sure is
unreachable right now, but I don't think this code should assume that.

>> + appendStringInfoString(es->str, wstate->worker_str[i].data);

> Probably never matters, but given we do have the string length already,
> we could use appendBinaryStringInfo().

Ah, I was thinking about making that change but then forgot.

>> + ExplainCloseGroup("Worker", NULL, true, es);

> Not related to this patch: I never got why we maintain a grouping stack
> for some things, but don't do it for the group properties
> themselves.

Right now it'd just be extra overhead. If we ever have a case where it's
not convenient for an ExplainCloseGroup caller to provide the same data
as for ExplainOpenGroup, then I'd be on board with storing that info.

> Hm. It might be worthwhile to rename ExplainOpenSetAsideGroup and use it
> from ExplainOpenGroup()? Seems we could just call it after
> ExplainOpenGroup()'s switch, and remove all the indent/grouping_stack
> related code from ExplainOpenGroup().

Hmm. It seemed easier to me to keep them separate, but ...

I did consider a design in which, instead of ExplainOpenSetAsideGroup,
there was some function that would initialize the "state_save" area and
then you'd call the "restore" function to make that state active. It
seemed like that would be too dissimilar from ExplainOpenGroup --- but
conceivably, we could reimplement ExplainOpenGroup as calling the
initialize function and then the restore function (along with doing some
output). Not really sure that'd be an improvement though: it'd involve
less duplicate code, but the functions would individually be harder to
wrap your brain around.

regards, tom lane