Thanks for reviewing! A few quick thoughts from me since I write a
bunch of the design for this patch.
On Wed, Dec 21, 2016 at 10:16 AM, Anastasia Lubennikova
<lubennikovaav@gmail.com> wrote:
> 1. Can't we simply use "if (scan->parallel_scan != NULL)" instead of xs_temp_snap flag?
>
> + if (scan->xs_temp_snap)
> + UnregisterSnapshot(scan->xs_snapshot);
>
> I must say that I'm quite new with all this parallel stuff. If you give me a link,
> where to read about snapshots for parallel workers, my review will be more helpful.
> Anyway, it would be great to have more comments about it in the code.
I suspect it would be better to keep those two things formally
separate, even though they may always be the same right now.
> 2. Don't you mind to rename 'amestimateparallelscan' to let's say 'amparallelscan_spacerequired'
> or something like this? As far as I understand there is nothing to estimate, we know this size
> for sure. I guess that you've chosen this name because of 'heap_parallelscan_estimate'.
> But now it looks similar to 'amestimate' which refers to indexscan cost for optimizer.
> That leads to the next question.
"estimate" is being used this way quite widely now, in places like
ExecParallelEstimate. So if we're going to change the terminology we
should do it broadly.
> 3. Are there any changes in cost estimation? I didn't find related changes in the patch.
> Parallel scan is expected to be faster and optimizer definitely should know that.
Generally the way that's reflected in the optimized is by having the
parallel scan have a lower row count. See cost_seqscan() for an
example.
In general, you'll probably find a lot of parallels between this patch
and ee7ca559fcf404f9a3bd99da85c8f4ea9fbc2e92, which is probably a good
thing.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company