Обсуждение: Comment on GatherPath.single_copy
Hello.
The comment on GatherPath.single_copy is the following.
===
/** GatherPath runs several copies of a plan in parallel and collects the* results. The parallel leader may also
executethe plan, unless the* single_copy flag is set.*/
typedef struct GatherPath
{ Path path; Path *subpath; /* path for each worker */ bool
single_copy; /* path must not be executed >1x */
} GatherPath;
===
The ">1x" looks to me as a kind of typo but looking the comment
above the struct it came to look as "more than once (or one
copy)". But it seems to me that it would be better to be in
ordinary words.
> bool single_copy; /* path must not be executed multiply */
If anyone feel that it is confusing with a verb form, the
following might be better.
> bool single_copy; /* path must not span on multiple processes */
Since anyway I cannot find a comfortable expression for this, I
attached a patch that does the last one.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/executor/nodeGather.c b/src/backend/executor/nodeGather.c
index e4cfc44..ed9c71f 100644
--- a/src/backend/executor/nodeGather.c
+++ b/src/backend/executor/nodeGather.c
@@ -312,6 +312,7 @@ gather_getnext(GatherState *gatherstate) if (gatherstate->need_to_scan_locally) {
+ elog(LOG, "EXECLOCAL"); outerTupleSlot = ExecProcNode(outerPlan); if
(!TupIsNull(outerTupleSlot))
@@ -385,15 +386,18 @@ gather_readnext(GatherState *gatherstate) /* Have we visited every (surviving)
TupleQueueReader?*/ nvisited++;
+ elog(LOG, "NEXT"); if (nvisited >= gatherstate->nreaders) { /* * If
(still)running plan locally, return NULL so caller can * generate another tuple from the local copy of the
plan. */
+ elog(LOG, "WAIT0"); if (gatherstate->need_to_scan_locally) return NULL;
+ elog(LOG, "WAIT"); /* Nothing to do except wait for developments. */
WaitLatch(MyLatch,WL_LATCH_SET, 0); ResetLatch(MyLatch);
diff --git a/src/include/nodes/relation.h b/src/include/nodes/relation.h
index fcfb0d4..b6b9779 100644
--- a/src/include/nodes/relation.h
+++ b/src/include/nodes/relation.h
@@ -1189,7 +1189,7 @@ typedef struct GatherPath{ Path path; Path *subpath; /* path for each
worker*/
- bool single_copy; /* path must not be executed >1x */
+ bool single_copy; /* path must not span on multiple processes */} GatherPath;/*
Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> - bool single_copy; /* path must not be executed >1x */
> + bool single_copy; /* path must not span on multiple processes */
I agree that the existing comment sucks, but this isn't a lot better
(and it will probably not look nice after pgindent gets done with it).
Possibly it's too complicated to jam a reasonable explanation into the
same-line comment, and what is needed is to expand the sentence about
it in the comment above the struct.
Robert, could you fix the documentation for that field so it's
intelligible?
regards, tom lane
On Tue, Aug 30, 2016 at 6:38 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes: >> - bool single_copy; /* path must not be executed >1x */ >> + bool single_copy; /* path must not span on multiple processes */ > > I agree that the existing comment sucks, but this isn't a lot better > (and it will probably not look nice after pgindent gets done with it). > Possibly it's too complicated to jam a reasonable explanation into the > same-line comment, and what is needed is to expand the sentence about > it in the comment above the struct. > > Robert, could you fix the documentation for that field so it's > intelligible? Uh, maybe. The trick, as you've already noted, is finding something better. Maybe this: - bool single_copy; /* path must not be executed >1x */ + bool single_copy; /* don't execute path in multiple processes */ -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Aug 30, 2016 at 6:38 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Robert, could you fix the documentation for that field so it's
>> intelligible?
> Uh, maybe. The trick, as you've already noted, is finding something
> better. Maybe this:
> - bool single_copy; /* path must not be executed >1x */
> + bool single_copy; /* don't execute path in multiple processes */
OK by me.
regards, tom lane
At Wed, 31 Aug 2016 07:26:22 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <5934.1472642782@sss.pgh.pa.us> > Robert Haas <robertmhaas@gmail.com> writes: > > On Tue, Aug 30, 2016 at 6:38 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> Robert, could you fix the documentation for that field so it's > >> intelligible? > > > Uh, maybe. The trick, as you've already noted, is finding something > > better. Maybe this: > > > - bool single_copy; /* path must not be executed >1x */ > > + bool single_copy; /* don't execute path in multiple processes */ > > OK by me. > > regards, tom lane Me too, thanks. -- Kyotaro Horiguchi NTT Open Source Software Center
On Thu, Sep 1, 2016 at 3:15 AM, Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote: > At Wed, 31 Aug 2016 07:26:22 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <5934.1472642782@sss.pgh.pa.us> >> Robert Haas <robertmhaas@gmail.com> writes: >> > On Tue, Aug 30, 2016 at 6:38 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> >> Robert, could you fix the documentation for that field so it's >> >> intelligible? >> >> > Uh, maybe. The trick, as you've already noted, is finding something >> > better. Maybe this: >> >> > - bool single_copy; /* path must not be executed >1x */ >> > + bool single_copy; /* don't execute path in multiple processes */ >> >> OK by me. >> >> regards, tom lane > > Me too, thanks. OK, changed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company