Thread: Misleading comment about single_copy, and some bikeshedding

Misleading comment about single_copy, and some bikeshedding

From
Thomas Munro
Date:
Hi,

/*
 * GatherMergePath runs several copies of a plan in parallel and collects
 * the results, preserving their common sort order.  For gather merge, the
 * parallel leader always executes the plan too, so we don't need single_copy.
 */
typedef struct GatherMergePath

The second sentence is not true as of commit e5253fdc, and the
attached patch removes it.

Even before that commit, the comment was a bit circular: the reason
GatherMergePath doesn't need a single_copy field is because
force_parallel_mode specifically means "try to stick a Gather node on
top in a test mode with one worker and no leader participation", and
this isn't a Gather node.

Hmm.  I wonder if we should rename force_parallel_mode to
force_gather_node in v13.  The current name has always seemed slightly
misleading to me; it sounds like some kind of turbo boost button but
really it's a developer-only test mode.  Also, does it belong under
DEVELOPER_OPTIONS instead of QUERY_TUNING_OTHER?  I'm also wondering
if the variable single_copy would be better named
no_leader_participation or single_participant.  I find "copy" a
slightly strange way to refer to the number of copies *allowed to
run*, but maybe that's just me.

-- 
Thomas Munro
https://enterprisedb.com

Attachment

Re: Misleading comment about single_copy, and some bikeshedding

From
Tom Lane
Date:
Thomas Munro <thomas.munro@gmail.com> writes:
> Hmm.  I wonder if we should rename force_parallel_mode to
> force_gather_node in v13.  The current name has always seemed slightly
> misleading to me; it sounds like some kind of turbo boost button but
> really it's a developer-only test mode.  Also, does it belong under
> DEVELOPER_OPTIONS instead of QUERY_TUNING_OTHER?  I'm also wondering
> if the variable single_copy would be better named
> no_leader_participation or single_participant.  I find "copy" a
> slightly strange way to refer to the number of copies *allowed to
> run*, but maybe that's just me.

FWIW, I agree 100% that these names are opaque.  I don't know if your
suggestions are the best we can do, but they each seem like improvements.
And yes, force_parallel_mode should be under DEVELOPER_OPTIONS; it's a
performance-losing test option.

            regards, tom lane