Re: Parallel Index Scans

On Fri, Jan 13, 2017 at 11:06 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jan 13, 2017 at 9:28 AM, Anastasia Lubennikova
> <a(dot)lubennikova(at)postgrespro(dot)ru> wrote:
>> I didn't find any case of noticeable performance degradation,
>> so set status to "Ready for committer".
>
> The very first hunk of doc changes looks like it makes the whitespace
> totally wrong - surely it can't be right to have 0-space indents in C
> code.
>

Fixed.

> + The <literal>index_size</> parameter indicate the size of generic parallel
>
> indicate -> indicates
> size of generic -> size of the generic
>

Fixed.

> + index-type-specific parallel information which will be stored immediatedly
>
> Typo.
>

Fixed.

> + Initialize the parallel index scan state. It will be used to initialize
> + index-type-specific parallel information which will be stored immediatedly
> + after generic parallel information required for parallel index scans. The
> + required state information will be set in <literal>target</>.
> + </para>
> +
> + <para>
> + The <function>aminitparallelscan</> and
> <function>amestimateparallelscan</>
> + functions need only be provided if the access method supports
> <quote>parallel</>
> + index scans. If it doesn't, the <structfield>aminitparallelscan</> and
> + <structfield>amestimateparallelscan</> fields in its
> <structname>IndexAmRoutine</>
> + struct must be set to NULL.
> + </para>
>
> Inconsistent indentation.

Fixed.

> <quote> seems like a strange choice of tag.
>

I have seen that <quote> is used in indexam.sgml at multiple places to
refer to "bitmap" and "plain" index scans. So thought of using same
for "parallel" index scans.

> + /* amestimateparallelscan is optional; assume no-op if not
> provided by AM */
>
> The fact that amestimateparallelscan is optional even when parallel
> index scans are supported is undocumented.
>

Okay, I have added that information in docs.

> Similarly for the other
> functions, which also seem to be optional but not documented as such.
> The code and documentation need to match.
>

All the functions introduced by this patch are documented in
indexam.sgml as optional. Not sure, which other place you are
expecting an update.

> + void *amtarget = (char *) ((void *) target) + offset;
>
> Passing an unaligned pointer to the AM sounds like a recipe for
> crashes on obscure platforms that can't tolerate alignment violations,
> and possibly bad performance on others. I'd arrange MAXALIGN the size
> of the generic structure in index_parallelscan_estimate and
> index_parallelscan_initialize.

Right, changed as per suggestion.

> Also, why pass the size of the generic
> structure to the AM-specific estimate routine, anyway? It can't
> legally return a smaller value, and we can add_size() just as well as
> the AM-specific code. Wouldn't it make more sense for the AM-specific
> code to return the amount of space that is needed for AM-specific
> stuff, and let the generic code deal with the generic stuff?
>

makes sense, so changed accordingly.

> + * status - True indicates that the block number returned is valid and scan
> + * is continued or block number is invalid and scan has just begun
> + * or block number is P_NONE and scan is finished. False indicates
> + * that we have reached the end of scan for current
> scankeys and for
> + * that we return block number as P_NONE.
>
> It is hard to parse a sentence with that many "and" and "or" clauses,
> especially since English, unlike C, does not have strict operator
> precedence rules. Perhaps you could reword to make it more clear.
>

Okay, I have changed the comment.

> Also, does that survive pgindent with that indentation?
>

Yes.

> + BTParallelScanDesc btscan = (BTParallelScanDesc) OffsetToPointer(
> + (void *) scan->parallel_scan,
> + scan->parallel_scan->ps_offset);
>
> You could avoid these uncomfortable line breaks by declaring the
> variable on one line and the initializing it on a separate line.
>

Okay, changed.

> + SpinLockAcquire(&btscan->ps_mutex);
> + pageStatus = btscan->ps_pageStatus;
> + if (so->arrayKeyCount < btscan->ps_arrayKeyCount)
> + *status = false;
> + else if (pageStatus == BTPARALLEL_DONE)
> + *status = false;
> + else if (pageStatus != BTPARALLEL_ADVANCING)
> + {
> + btscan->ps_pageStatus = BTPARALLEL_ADVANCING;
> + nextPage = btscan->ps_nextPage;
> + exit_loop = true;
> + }
> + SpinLockRelease(&btscan->ps_mutex);
>
> IMHO, this needs comments.
>

Sure, added a comment.

> + * It updates the value of next page that allows parallel scan to move forward
> + * or backward depending on scan direction. It wakes up one of the sleeping
> + * workers.
>
> This construction is commonly used in India but sounds awkward to
> other English-speakers, or at least to me. You can either drop the
> word "it" and just start with the verb "Updates the value of ..." or
> you can replace the first instance of "It" with "This function".
> Although actually, I think this whole comment needs rewriting. Maybe
> something like "Save information about scan position and wake up next
> worker to continue scan."
>

Changed as per suggestion.

> + * This must be called when there are no pages left to scan. Notify end of
> + * parallel scan to all the other workers associated with this scan.
>
> Suggest: When there are no pages left to scan, this function should be
> called to notify other workers. Otherwise, they might wait forever
> for the scan to advance to the next page.
>
> + if (status == false)
>
> if (!status) is usually preferred for bools. (Multiple instances.)
>
> +#define BTPARALLEL_NOT_INITIALIZED 0x01
> +#define BTPARALLEL_ADVANCING 0x02
> +#define BTPARALLEL_DONE 0x03
> +#define BTPARALLEL_IDLE 0x04
>
> Let's change this to an enum. We can keep the names of the members
> as-is, just use typedef enum { ... } instead of #defines.
>

Changed as per suggestion.

> +#define OffsetToPointer(base, offset)\
> +((void *)((char *)base + offset))
>
> Blech. Aside from the bad formatting, this is an awfully generic
> thing to stick into relscan.h.

Agreed and moved to c.h where some similar defines are present.

> I'm not sure we should have it at all,
> but certainly not in this file.
>

Yeah, but I think there is no harm in keeping it and maybe start using
in code at other places as well.

> +/*
> + * BTParallelScanDescData contains btree specific shared information required
> + * for parallel scan.
> + */
> +typedef struct BTParallelScanDescData
> +{
> + BlockNumber ps_nextPage; /* latest or next page to be scanned */
> + uint8 ps_pageStatus; /* indicates whether next page is available
> + * for scan. see nbtree.h for possible states
> + * of parallel scan. */
> + int ps_arrayKeyCount; /* count indicating number of array
> + * scan keys processed by parallel
> + * scan */
> + slock_t ps_mutex; /* protects above variables */
> + ConditionVariable cv; /* used to synchronize parallel scan */
> +} BTParallelScanDescData;
>
> Why are the states declared a separate header file from the variable
> that uses them? Let's put them all in the same place.
>

Agreed and changed accordingly.

> Why do all of these fields except for the last one have a ps_ prefix,
> but the last one doesn't?
>

No specific reason, so Changed as per suggestion.

> I assume "ps" stands for "parallel scan" but maybe "btps" would be
> better since this is btree-specific.
>

Changed as per suggestion.

> ps_nextPage sometimes contains something other than the next page, so
> maybe we should choose a different name, like ps_currentPage or
> ps_scanPage.
>

Changed as per suggestion.

I have also rebased the optimizer/executor support patch
(parallel_index_opt_exec_support_v4.patch) and added a test case in
it.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com