On Thu, Mar 4, 2021 at 1:07 AM Dilip Kumar <dilipbal...@gmail.com> wrote:
>
> On Wed, Mar 3, 2021 at 5:50 PM Greg Nancarrow <gregn4...@gmail.com> wrote:
> >
> > Asserts are normally only enabled in a debug-build, so for a
> > release-build that Assert has no effect.
> > The Assert is being used as a sanity-check that the function is only
> > currently getting called for INSERT, because that's all it currently
> > supports.
>
> I agree that assert is only for debug build, but once we add and
> assert that means we are sure that it should only be called for insert
> and if it is called for anything else then it is a programming error
> from the caller's side. So after the assert, adding if check for the
> same condition doesn't look like a good idea. That means we think
> that the code can hit assert in the debug mode so we need an extra
> protection in the release mode.
>
The if-check isn't there for "extra protection".
It's to help with future changes; inside that if-block is code only
applicable to INSERT (and to UPDATE - sorry, before I said DELETE), as
the code-comment indicates, whereas the rest of the function is
generic to all command types. I don't see any problem with having this
if-block here, to help in this way, when other command types are
added.
>
> >
> > > 2.
> > > In patch 0004, We are still charging the parallel_tuple_cost for each
> > > tuple, are we planning to do something about this? I mean after this
> > > patch tuple will not be transferred through the tuple queue, so we
> > > should not add that cost.
> > >
> >
> > I believe that for Parallel INSERT, cost_modifytable() will set
> > path->path.rows to 0 (unless there is a RETURNING list), so, for
> > example, in cost_gather(), it will not add to the run_cost as
> > "run_cost += parallel_tuple_cost * path->path.rows;"
> >
>
> But the cost_modifytable is setting the number of rows to 0 in
> ModifyTablePath whereas the cost_gather will multiply the rows from
> the GatherPath. I can not see the rows from GatherPath is ever set to
> 0.
>
OK, I see the problem now.
It works the way I described, but currently there's a problem with
where it's getting the rows for the GatherPath, so there's a
disconnect.
When generating the GatherPaths, it's currently always taking the
rel's (possibly estimated) row-count, rather than using the rows from
the cheapest_partial_path (the subpath: ModifyTablePath). See
generate_gather_paths().
So when generate_useful_gather_paths() is called from the planner, for
the added partial paths for Parallel INSERT, it should be passing
"true" for the "override_rows" parameter, not "false".
So I think that in the 0004 patch, the if-block:
+ if (parallel_modify_partial_path_added)
+ {
+ final_rel->rows = current_rel->rows; /* ??? why
hasn't this been
+
* set above somewhere ???? */
+ generate_useful_gather_paths(root, final_rel, false);
+ }
+
can be reduced to:
+ if (parallel_modify_partial_path_added)
+ generate_useful_gather_paths(root, final_rel, true);
+
Regards,
Greg Nancarrow
Fujitsu Australia
