Re: Duplicate Workers entries in some EXPLAIN plans - Mailing list pgsql-hackers

Andres Freund <andres@anarazel.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