Parallel INSERT (INTO ... SELECT ...)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
363 messages Options
1234567 ... 19
Reply | Threaded
Open this post in threaded view
|

Re: Parallel INSERT (INTO ... SELECT ...)

Greg Nancarrow
On Sat, Oct 10, 2020 at 3:32 PM Amit Kapila <[hidden email]> wrote:
>
> > OK, for the minimal patch, just allowing INSERT with parallel SELECT,
> > you're right, neither of those additional "commandType == CMD_SELECT"
> > checks are needed, so I'll remove them.
> >
>various
> Okay, that makes sense.
>

For the minimal patch (just allowing INSERT with parallel SELECT),
there are issues with parallel-mode and various parallel-mode-related
checks in the code.
Initially, I thought it was only a couple of XID-related checks (which
could perhaps just be tweaked to check for IsParallelWorker() instead,
as you suggested), but I now realise that there are a lot more cases.
This stems from the fact that just having a parallel SELECT (as part
of non-parallel INSERT) causes parallel-mode to be set for the WHOLE
plan. I'm not sure why parallel-mode is set globally like this, for
the whole plan. Couldn't it just be set for the scope of
Gather/GatherMerge? Otherwise, errors from these checks seem to be
misleading when outside the scope of Gather/GatherMerge, as
technically they are not occurring within the scope of parallel-leader
and parallel-worker(s). The global parallel-mode wouldn't have been an
issue before, because up to now INSERT has never had underlying
parallel operations.

For example, when running the tests under
"force_parallel_mode=regress", the test failures show that there are a
lot more cases affected:

"cannot assign TransactionIds during a parallel operation"
"cannot assign XIDs during a parallel operation"
"cannot start commands during a parallel operation"
"cannot modify commandid in active snapshot during a parallel operation"
"cannot execute nextval() during a parallel operation"
"cannot execute INSERT during a parallel operation"
"cannot execute ANALYZE during a parallel operation
"cannot update tuples during a parallel operation"

(and there are more not currently detected by the tests, found by
searching the code).

As an example, with the minimal patch applied, if you had a trigger on
INSERT that, say, attempted a table creation or UPDATE/DELETE, and you
ran an "INSERT INTO ... SELECT...", it would treat the trigger
operations as being attempted in parallel-mode, and so an error would
result.

Let me know your thoughts on how to deal with these issues.
Can you see a problem with only having parallel-mode set for scope of
Gather/GatherMerge, or do you have some other idea?

Regards,
Greg Nancarrow
Fujitsu Australia


Reply | Threaded
Open this post in threaded view
|

Re: Parallel INSERT (INTO ... SELECT ...)

Greg Nancarrow
In reply to this post by akapila
On Sun, Oct 11, 2020 at 1:05 PM Amit Kapila <[hidden email]> wrote:

>
> On Sat, Oct 10, 2020 at 5:25 PM Amit Kapila <[hidden email]> wrote:
> >
> > 8. You have made changes related to trigger execution for Gather node,
> > don't we need similar changes for GatherMerge node?
> >
> ..
> >
> > 10. Don't we need a change similar to cost_gather in
> > cost_gather_merge? It seems you have made only partial changes for
> > GatherMerge node.
> >
>
> Please ignore these two comments as we can't push Insert to workers
> when GatherMerge is involved as a leader backend does the final phase
> (merge the results by workers). So, we can only push the Select part
> of the statement.
>

Precisely, that's why I didn't make those changes for GatherMerge.

Regards,
Greg Nancarrow
Fujitsu Australia


Reply | Threaded
Open this post in threaded view
|

Re: Parallel INSERT (INTO ... SELECT ...)

Greg Nancarrow
In reply to this post by Thomas Munro-5
On Sun, Oct 11, 2020 at 1:39 PM Thomas Munro <[hidden email]> wrote:

>
> Yeah, I think this is trying to fix the problem too late.  Instead, we
> should fix the incorrect row estimates so we don't have to fudge it
> later like that.  For example, this should be estimating rows=0:
>
> postgres=# explain analyze insert into s select * from t t1 join t t2 using (i);
> ...
>  Insert on s  (cost=30839.08..70744.45 rows=1000226 width=4) (actual
> time=2940.560..2940.562 rows=0 loops=1)
>
> I think that should be done with something like this:
>
> --- a/src/backend/optimizer/util/pathnode.c
> +++ b/src/backend/optimizer/util/pathnode.c
> @@ -3583,16 +3583,11 @@ create_modifytable_path(PlannerInfo *root,
> RelOptInfo *rel,
>                 if (lc == list_head(subpaths))  /* first node? */
>                         pathnode->path.startup_cost = subpath->startup_cost;
>                 pathnode->path.total_cost += subpath->total_cost;
> -               pathnode->path.rows += subpath->rows;
> +               if (returningLists != NIL)
> +                       pathnode->path.rows += subpath->rows;
>                 total_size += subpath->pathtarget->width * subpath->rows;
>         }
>
> -       /*
> -        * Set width to the average width of the subpath outputs.  XXX this is
> -        * totally wrong: we should report zero if no RETURNING, else an average
> -        * of the RETURNING tlist widths.  But it's what happened historically,
> -        * and improving it is a task for another day.
> -        */
>         if (pathnode->path.rows > 0)
>                 total_size /= pathnode->path.rows;
>         pathnode->path.pathtarget->width = rint(total_size);

Agree, thanks (bug in existing Postgres code, right?)

Regards,
Greg Nancarrow
Fujitsu Australia


Reply | Threaded
Open this post in threaded view
|

Re: Parallel INSERT (INTO ... SELECT ...)

Thomas Munro-5
On Mon, Oct 12, 2020 at 3:42 PM Greg Nancarrow <[hidden email]> wrote:
> On Sun, Oct 11, 2020 at 1:39 PM Thomas Munro <[hidden email]> wrote:
> >                 pathnode->path.total_cost += subpath->total_cost;
> > -               pathnode->path.rows += subpath->rows;
> > +               if (returningLists != NIL)
> > +                       pathnode->path.rows += subpath->rows;
> >                 total_size += subpath->pathtarget->width * subpath->rows;
> >         }

Erm, except the condition should of course cover total_size too.

> Agree, thanks (bug in existing Postgres code, right?)

Yeah, I think we should go ahead and fix that up front.  Here's a
version with a commit message.

0001-Fix-row-estimate-for-ModifyTable-paths.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Parallel INSERT (INTO ... SELECT ...)

akapila
In reply to this post by Greg Nancarrow
On Mon, Oct 12, 2020 at 6:51 AM Greg Nancarrow <[hidden email]> wrote:

>
> On Sat, Oct 10, 2020 at 3:32 PM Amit Kapila <[hidden email]> wrote:
> >
> > > OK, for the minimal patch, just allowing INSERT with parallel SELECT,
> > > you're right, neither of those additional "commandType == CMD_SELECT"
> > > checks are needed, so I'll remove them.
> > >
> >various
> > Okay, that makes sense.
> >
>
> For the minimal patch (just allowing INSERT with parallel SELECT),
> there are issues with parallel-mode and various parallel-mode-related
> checks in the code.
> Initially, I thought it was only a couple of XID-related checks (which
> could perhaps just be tweaked to check for IsParallelWorker() instead,
> as you suggested), but I now realise that there are a lot more cases.
> This stems from the fact that just having a parallel SELECT (as part
> of non-parallel INSERT) causes parallel-mode to be set for the WHOLE
> plan. I'm not sure why parallel-mode is set globally like this, for
> the whole plan. Couldn't it just be set for the scope of
> Gather/GatherMerge? Otherwise, errors from these checks seem to be
> misleading when outside the scope of Gather/GatherMerge, as
> technically they are not occurring within the scope of parallel-leader
> and parallel-worker(s). The global parallel-mode wouldn't have been an
> issue before, because up to now INSERT has never had underlying
> parallel operations.
>

That is right but there is another operation which works like that.
For ex. a statement like "create table test_new As select * from
test_parallel where c1 < 1000;" will use parallel select but the write
operation will be performed in a leader. I agree that the code flow of
Insert is different so we will have a different set of challenges in
that case but to make it work there shouldn't be any fundamental
problem.

> For example, when running the tests under
> "force_parallel_mode=regress", the test failures show that there are a
> lot more cases affected:
>
> "cannot assign TransactionIds during a parallel operation"
> "cannot assign XIDs during a parallel operation"
> "cannot start commands during a parallel operation"
> "cannot modify commandid in active snapshot during a parallel operation"
> "cannot execute nextval() during a parallel operation"
> "cannot execute INSERT during a parallel operation"
> "cannot execute ANALYZE during a parallel operation
> "cannot update tuples during a parallel operation"
>
> (and there are more not currently detected by the tests, found by
> searching the code).
>

Did you get these after applying your patch? If so, can you share the
version which you are using, or if you have already posted the same
then point me to the same?

> As an example, with the minimal patch applied, if you had a trigger on
> INSERT that, say, attempted a table creation or UPDATE/DELETE, and you
> ran an "INSERT INTO ... SELECT...", it would treat the trigger
> operations as being attempted in parallel-mode, and so an error would
> result.
>

Oh, I guess this happens because you need to execute Insert in
parallel-mode even though Insert is happening in the leader, right?
And probably we are not facing this with "Create Table As .." because
there is no trigger execution involved there.

> Let me know your thoughts on how to deal with these issues.
> Can you see a problem with only having parallel-mode set for scope of
> Gather/GatherMerge, or do you have some other idea?
>

I have not thought about this yet but I don't understand your
proposal. How will you set it only for the scope of Gather (Merge)?
The execution of the Gather node will be interleaved with the Insert
node, basically, you fetch a tuple from Gather, and then you need to
Insert it. Can you be a bit more specific on what you have in mind for
this?

--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

Re: Parallel INSERT (INTO ... SELECT ...)

Greg Nancarrow
In reply to this post by Thomas Munro-5
On Mon, Oct 12, 2020 at 2:11 PM Thomas Munro <[hidden email]> wrote:

>
> On Mon, Oct 12, 2020 at 3:42 PM Greg Nancarrow <[hidden email]> wrote:
> > On Sun, Oct 11, 2020 at 1:39 PM Thomas Munro <[hidden email]> wrote:
> > >                 pathnode->path.total_cost += subpath->total_cost;
> > > -               pathnode->path.rows += subpath->rows;
> > > +               if (returningLists != NIL)
> > > +                       pathnode->path.rows += subpath->rows;
> > >                 total_size += subpath->pathtarget->width * subpath->rows;
> > >         }
>
> Erm, except the condition should of course cover total_size too.
>
> > Agree, thanks (bug in existing Postgres code, right?)
>
> Yeah, I think we should go ahead and fix that up front.  Here's a
> version with a commit message.

I've checked it and tested it, and it looks fine to me.
Also, it seems to align with the gripe in the old comment about width
("XXX this is totally wrong: we should report zero if no RETURNING
...").
I'm happy for you to commit it.

Regards,
Greg Nancarrow
Fujitsu Australia


Reply | Threaded
Open this post in threaded view
|

Re: Parallel INSERT (INTO ... SELECT ...)

akapila
In reply to this post by akapila
On Mon, Oct 12, 2020 at 9:01 AM Amit Kapila <[hidden email]> wrote:

>
> On Mon, Oct 12, 2020 at 6:51 AM Greg Nancarrow <[hidden email]> wrote:
> >
>
> > Let me know your thoughts on how to deal with these issues.
> > Can you see a problem with only having parallel-mode set for scope of
> > Gather/GatherMerge, or do you have some other idea?
> >
>
> I have not thought about this yet but I don't understand your
> proposal. How will you set it only for the scope of Gather (Merge)?
> The execution of the Gather node will be interleaved with the Insert
> node, basically, you fetch a tuple from Gather, and then you need to
> Insert it. Can you be a bit more specific on what you have in mind for
> this?
>

One more thing I would like to add here is that we can't exit
parallel-mode till the workers are running (performing the scan or
other operation it is assigned with) and shared memory is not
destroyed. Otherwise, the leader can perform un-safe things like
assigning new commandsids or probably workers can send some error for
which the leader should still be in parallel-mode. So, considering
this I think we need quite similar checks (similar to parallel
inserts) to make even the Select part parallel for Inserts. If we do
that then you won't face many of the problems you mentioned above like
executing triggers that contain parallel-unsafe stuff. I feel still it
will be beneficial to do this as it will cover cases like Insert with
GatherMerge underneath it which would otherwise not possible.

--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

Re: Parallel INSERT (INTO ... SELECT ...)

Greg Nancarrow
On Mon, Oct 12, 2020 at 5:36 PM Amit Kapila <[hidden email]> wrote:

>
> >
> > I have not thought about this yet but I don't understand your
> > proposal. How will you set it only for the scope of Gather (Merge)?
> > The execution of the Gather node will be interleaved with the Insert
> > node, basically, you fetch a tuple from Gather, and then you need to
> > Insert it. Can you be a bit more specific on what you have in mind for
> > this?
> >
>
> One more thing I would like to add here is that we can't exit
> parallel-mode till the workers are running (performing the scan or
> other operation it is assigned with) and shared memory is not
> destroyed. Otherwise, the leader can perform un-safe things like
> assigning new commandsids or probably workers can send some error for
> which the leader should still be in parallel-mode. So, considering
> this I think we need quite similar checks (similar to parallel
> inserts) to make even the Select part parallel for Inserts. If we do
> that then you won't face many of the problems you mentioned above like
> executing triggers that contain parallel-unsafe stuff. I feel still it
> will be beneficial to do this as it will cover cases like Insert with
> GatherMerge underneath it which would otherwise not possible.
>

Yes, I see what you mean, exiting parallel-mode can't be done safely
where I had hoped it could, so looks like, even for making just the
Select part of Insert parallel, I need to add checks (along the same
lines as for Parallel Insert) to avoid the parallel Select in certain
potentially-unsafe cases.

Regards,
Greg Nancarrow
Fujitsu Australia


Reply | Threaded
Open this post in threaded view
|

Re: Parallel INSERT (INTO ... SELECT ...)

akapila
On Mon, Oct 12, 2020 at 12:38 PM Greg Nancarrow <[hidden email]> wrote:

>
> On Mon, Oct 12, 2020 at 5:36 PM Amit Kapila <[hidden email]> wrote:
> >
> > >
> > > I have not thought about this yet but I don't understand your
> > > proposal. How will you set it only for the scope of Gather (Merge)?
> > > The execution of the Gather node will be interleaved with the Insert
> > > node, basically, you fetch a tuple from Gather, and then you need to
> > > Insert it. Can you be a bit more specific on what you have in mind for
> > > this?
> > >
> >
> > One more thing I would like to add here is that we can't exit
> > parallel-mode till the workers are running (performing the scan or
> > other operation it is assigned with) and shared memory is not
> > destroyed. Otherwise, the leader can perform un-safe things like
> > assigning new commandsids or probably workers can send some error for
> > which the leader should still be in parallel-mode. So, considering
> > this I think we need quite similar checks (similar to parallel
> > inserts) to make even the Select part parallel for Inserts. If we do
> > that then you won't face many of the problems you mentioned above like
> > executing triggers that contain parallel-unsafe stuff. I feel still it
> > will be beneficial to do this as it will cover cases like Insert with
> > GatherMerge underneath it which would otherwise not possible.
> >
>
> Yes, I see what you mean, exiting parallel-mode can't be done safely
> where I had hoped it could, so looks like, even for making just the
> Select part of Insert parallel, I need to add checks (along the same
> lines as for Parallel Insert) to avoid the parallel Select in certain
> potentially-unsafe cases.
>

Right, after we take care of that, we can think of assigning xid or
things like that before we enter parallel mode. Say we have a function
like PrepareParallelMode (or PrepareEnterParallelMode) or something
like that where we can check whether we need to perform
parallel-safe-write operation (as of now Insert) and then do the
required preparation like assign xid, etc. I think this might not be
idle because it is possible that we don't fetch even a single row (say
due to filter condition) which needs to be inserted and then we will
waste xid but such cases might not occur often enough to worry.

--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

Re: Parallel INSERT (INTO ... SELECT ...)

Thomas Munro-5
In reply to this post by Greg Nancarrow
On Mon, Oct 12, 2020 at 6:35 PM Greg Nancarrow <[hidden email]> wrote:
> On Mon, Oct 12, 2020 at 2:11 PM Thomas Munro <[hidden email]> wrote:
> > Yeah, I think we should go ahead and fix that up front.  Here's a
> > version with a commit message.
>
> I've checked it and tested it, and it looks fine to me.
> Also, it seems to align with the gripe in the old comment about width
> ("XXX this is totally wrong: we should report zero if no RETURNING
> ...").
> I'm happy for you to commit it.

Pushed, though I left most of that comment there because the width
estimate still needs work when you do use RETURNING.  At least we now
have rows=0 for queries without RETURNING, which was a bigger problem
for your patch.


Reply | Threaded
Open this post in threaded view
|

Re: Parallel INSERT (INTO ... SELECT ...)

Greg Nancarrow
In reply to this post by akapila
On Fri, Oct 9, 2020 at 8:09 PM Amit Kapila <[hidden email]> wrote:
>
> It might be a good idea to first just get this patch committed, if
> possible. So, I have reviewed the latest version of this patch:
>
> 0001-InsertParallelSelect

I've attached an updated InsertParallelSelect patch (v2) - allowing
underlying parallel SELECT for "INSERT INTO ... SELECT ...".
I think I've addressed most of the issues identified in the previous
version of the patch.
I'm still seeing a couple of errors in the tests when
"force_parallel_mode=regress" is in effect, and those need to be
addressed (extra checks required to avoid parallel SELECT in certain
cases).
Also, I'm seeing a partition-related error when running
installcheck-world - I'm investigating that.

Regards,
Greg Nancarrow
Fujitsu Australia

v2-0001-Enable-parallel-SELECT-for-INSERT-INTO-.-SELECT.patch (17K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Parallel INSERT (INTO ... SELECT ...)

akapila
On Thu, Oct 15, 2020 at 9:56 AM Greg Nancarrow <[hidden email]> wrote:

>
> On Fri, Oct 9, 2020 at 8:09 PM Amit Kapila <[hidden email]> wrote:
> >
> > It might be a good idea to first just get this patch committed, if
> > possible. So, I have reviewed the latest version of this patch:
> >
> > 0001-InsertParallelSelect
>
> I've attached an updated InsertParallelSelect patch (v2) - allowing
> underlying parallel SELECT for "INSERT INTO ... SELECT ...".
> I think I've addressed most of the issues identified in the previous
> version of the patch.
> I'm still seeing a couple of errors in the tests when
> "force_parallel_mode=regress" is in effect, and those need to be
> addressed (extra checks required to avoid parallel SELECT in certain
> cases).
>

I am getting below error in force_parallel_mode:
@@ -1087,9 +1087,14 @@
 ERROR:  value for domain inotnull violates check constraint "inotnull_check"
 create table dom_table (x inotnull);
 insert into dom_table values ('1');
+ERROR:  cannot start commands during a parallel operation
+CONTEXT:  SQL function "sql_is_distinct_from"

It happened with below test:

create function sql_is_distinct_from(anyelement, anyelement)
returns boolean language sql
as 'select $1 is distinct from $2 limit 1';

create domain inotnull int
  check (sql_is_distinct_from(value, null));

create table dom_table (x inotnull);
insert into dom_table values ('1');

So it is clear that this is happening because we have allowed insert
that is parallel-unsafe. The attribute is of type domain which has a
parallel-unsafe constraint. As per your current code, we need to
detect it in IsRelParallelModeSafeForModify. The idea would be to
check the type of each attribute and if it is domain type then we need
to check if it has a constraint (See function ExecGrant_Type on how to
detect a domain type and then refer to functions
AlterTypeNamespaceInternal and AlterConstraintNamespaces to know how
to determine constraint for domain type). Once you can find a
constraint then you already have code in your patch to find if it
contains parallel-unsafe expression.


> Also, I'm seeing a partition-related error when running
> installcheck-world - I'm investigating that.
>

Okay.

Few more comments:
==================
1.
+ /*
+ * Can't support execution of row-level or transition-table triggers
+ * during parallel-mode, since such triggers may query the table
+ * into which the data is being inserted, and the content returned
+ * would vary unpredictably according to the order of retrieval by
+ * the workers and the rows already inserted.
+ */
+ if (trigdesc != NULL &&
+ (trigdesc->trig_insert_instead_row ||
+   trigdesc->trig_insert_before_row ||
+   trigdesc->trig_insert_after_row ||
+   trigdesc->trig_insert_new_table))
+ {
+ return false;
+ }

I don't think it is a good idea to prohibit all before/after/instead
row triggers because for the case you are referring to should mark
trigger functions as parallel-unsafe. We might want to have to Assert
somewhere to detect if there is illegal usage but I don't see the need
to directly prohibit them.

2.
@@ -56,8 +57,8 @@ GetNewTransactionId(bool isSubXact)
  * Workers synchronize transaction state at the beginning of each parallel
  * operation, so we can't account for new XIDs after that point.
  */
- if (IsInParallelMode())
- elog(ERROR, "cannot assign TransactionIds during a parallel operation");
+ if (IsParallelWorker())
+ elog(ERROR, "cannot assign TransactionIds in a parallel worker");

  /*
  * During bootstrap initialization, we return the special bootstrap
diff --git a/src/backend/access/transam/xact.c
b/src/backend/access/transam/xact.c
index af6afce..ef423fb 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -580,8 +580,8 @@ AssignTransactionId(TransactionState s)
  * Workers synchronize transaction state at the beginning of each parallel
  * operation, so we can't account for new XIDs at this point.
  */
- if (IsInParallelMode() || IsParallelWorker())
- elog(ERROR, "cannot assign XIDs during a parallel operation");
+ if (IsParallelWorker())
+ elog(ERROR, "cannot assign XIDs in a parallel worker");


I think we don't need these changes at least for the purpose of this
patch if you follow the suggestion related to having a new function
like PrepareParallelMode in the email above [1]. One problem I see
with removing these checks is how do we ensure that leader won't
assign a new transactionid once we start executing a parallel node. It
can do via sub-transactions maybe that is already protected at some
previous point but I would like to see if we can retain these checks.

[1] - https://www.postgresql.org/message-id/CAA4eK1JogfXUa%3D3wMPO%2BK%3DUiOLgHgCO7-fj1wCHsSxdaXsfVbw%40mail.gmail.com

--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

Re: Parallel INSERT (INTO ... SELECT ...)

akapila
On Thu, Oct 15, 2020 at 6:13 PM Amit Kapila <[hidden email]> wrote:
>
> On Thu, Oct 15, 2020 at 9:56 AM Greg Nancarrow <[hidden email]> wrote:
> >
> > Also, I'm seeing a partition-related error when running
> > installcheck-world - I'm investigating that.
> >
>
> Okay.
>

The attached patch fixes this partition case for me. Basically, we
need to check the parallel-safety of PartitionKey. I have only checked
for partsupfunc but I think we need to check the parallel-safety of
partexprs as well. Also, I noticed that you have allowed for
parallelism only when all expressions/functions involved with Insert
are parallel-safe, can't we allow parallel-restricted case because
anyway Inserts have to be performed by the leader for this patch.

--
With Regards,
Amit Kapila.

fix_paratition_failure_1.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Parallel INSERT (INTO ... SELECT ...)

Greg Nancarrow
On Fri, Oct 16, 2020 at 3:43 PM Amit Kapila <[hidden email]> wrote:

>
> On Thu, Oct 15, 2020 at 6:13 PM Amit Kapila <[hidden email]> wrote:
> >
> > On Thu, Oct 15, 2020 at 9:56 AM Greg Nancarrow <[hidden email]> wrote:
> > >
> > > Also, I'm seeing a partition-related error when running
> > > installcheck-world - I'm investigating that.
> > >
> >
> > Okay.
> >
>
> The attached patch fixes this partition case for me. Basically, we
> need to check the parallel-safety of PartitionKey. I have only checked
> for partsupfunc but I think we need to check the parallel-safety of
> partexprs as well.

Thanks, I had already added the parallel-safety check for partexprs
when I saw this, so your patch hopefully completes all the
partition-related checks required.

> Also, I noticed that you have allowed for
> parallelism only when all expressions/functions involved with Insert
> are parallel-safe, can't we allow parallel-restricted case because
> anyway Inserts have to be performed by the leader for this patch.
>

Yes, I think you're right.
"A parallel restricted operation is one which cannot be performed in a
parallel worker, but which can be performed in the leader while
parallel query is in use."
I'll make the change and test that everything works OK.

Regards,
Greg Nancarrow
Fujitsu Australia


Reply | Threaded
Open this post in threaded view
|

Re: Parallel INSERT (INTO ... SELECT ...)

akapila
On Fri, Oct 16, 2020 at 2:16 PM Greg Nancarrow <[hidden email]> wrote:

>
> On Fri, Oct 16, 2020 at 3:43 PM Amit Kapila <[hidden email]> wrote:
> >
>
> > Also, I noticed that you have allowed for
> > parallelism only when all expressions/functions involved with Insert
> > are parallel-safe, can't we allow parallel-restricted case because
> > anyway Inserts have to be performed by the leader for this patch.
> >
>
> Yes, I think you're right.
> "A parallel restricted operation is one which cannot be performed in a
> parallel worker, but which can be performed in the leader while
> parallel query is in use."
> I'll make the change and test that everything works OK.
>

Cool, let me try to explain my thoughts a bit more. The idea is first
(in standard_planner) we check if there is any 'parallel_unsafe'
function/expression (via max_parallel_hazard) in the query tree. If we
don't find anything 'parallel_unsafe' then we mark parallelModeOk. At
this stage, the patch is checking whether there is any
'parallel_unsafe' or 'parallel_restricted' expression/function in the
target relation and if there is none then we mark parallelModeOK as
true. So, if there is anything 'parallel_restricted' then we will mark
parallelModeOK as false which doesn't seem right to me.

Then later in the planner during set_rel_consider_parallel, we
determine if a particular relation can be scanned from within a
worker, then we consider that relation for parallelism. Here, we
determine if certain things are parallel-restricted then we don't
consider this for parallelism. Then we create partial paths for the
relations that are considered for parallelism. I think we don't need
to change anything for the current patch in these later stages because
we anyway are not considering Insert to be pushed into workers.
However, in the second patch where we are thinking to push Inserts in
workers, we might need to do something to filter parallel-restricted
cases during this stage of the planner.

--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

Re: Parallel INSERT (INTO ... SELECT ...)

Greg Nancarrow
On Fri, Oct 16, 2020 at 9:26 PM Amit Kapila <[hidden email]> wrote:

>
>
> Cool, let me try to explain my thoughts a bit more. The idea is first
> (in standard_planner) we check if there is any 'parallel_unsafe'
> function/expression (via max_parallel_hazard) in the query tree. If we
> don't find anything 'parallel_unsafe' then we mark parallelModeOk. At
> this stage, the patch is checking whether there is any
> 'parallel_unsafe' or 'parallel_restricted' expression/function in the
> target relation and if there is none then we mark parallelModeOK as
> true. So, if there is anything 'parallel_restricted' then we will mark
> parallelModeOK as false which doesn't seem right to me.
>
> Then later in the planner during set_rel_consider_parallel, we
> determine if a particular relation can be scanned from within a
> worker, then we consider that relation for parallelism. Here, we
> determine if certain things are parallel-restricted then we don't
> consider this for parallelism. Then we create partial paths for the
> relations that are considered for parallelism. I think we don't need
> to change anything for the current patch in these later stages because
> we anyway are not considering Insert to be pushed into workers.
> However, in the second patch where we are thinking to push Inserts in
> workers, we might need to do something to filter parallel-restricted
> cases during this stage of the planner.
>
Posting an update to the smaller patch (Parallel SELECT for INSERT
INTO...SELECT...).

Most of this patch feeds into the larger Parallel INSERT patch, for
which I'll also be posting an update soon.

Patch updates include:
- Removed explicit trigger-type checks (instead rely on declared
trigger parallel safety)
- Restored parallel-related XID checks that previous patch altered;
now assign XID prior to entering parallel-mode
- Now considers parallel-SELECT for parallel RESTRICTED cases (not
just parallel SAFE cases)
- Added parallel-safety checks for partition key expressions and
support functions
- Workaround added for test failure in "partition-concurrent-attach"
test; since ALTER TABLE operations may exclusively lock a relation
until end-of-transaction, now assume and return UNSAFE if can't
acquire a share-lock on the relation, rather than block until
potentially end of the other concurrent transaction in which the
exclusive lock is held.
Examples of when a relation is exclusively locked
(AccessExclusiveLock) until end-of-transaction include:
    ALTER TABLE DROP COLUMN
    ALTER TABLE ... RENAME
    ALTER TABLE ... ATTACH PARTITION  (locks default partition)

Regards,
Greg Nancarrow
Fujitsu Australia

v3-0001-Enable-parallel-SELECT-for-INSERT-INTO-.-SELECT.patch (26K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Parallel INSERT (INTO ... SELECT ...)

Greg Nancarrow
In reply to this post by akapila
On Fri, Oct 16, 2020 at 9:26 PM Amit Kapila <[hidden email]> wrote:

>
>
> Cool, let me try to explain my thoughts a bit more. The idea is first
> (in standard_planner) we check if there is any 'parallel_unsafe'
> function/expression (via max_parallel_hazard) in the query tree. If we
> don't find anything 'parallel_unsafe' then we mark parallelModeOk. At
> this stage, the patch is checking whether there is any
> 'parallel_unsafe' or 'parallel_restricted' expression/function in the
> target relation and if there is none then we mark parallelModeOK as
> true. So, if there is anything 'parallel_restricted' then we will mark
> parallelModeOK as false which doesn't seem right to me.
>
> Then later in the planner during set_rel_consider_parallel, we
> determine if a particular relation can be scanned from within a
> worker, then we consider that relation for parallelism. Here, we
> determine if certain things are parallel-restricted then we don't
> consider this for parallelism. Then we create partial paths for the
> relations that are considered for parallelism. I think we don't need
> to change anything for the current patch in these later stages because
> we anyway are not considering Insert to be pushed into workers.
> However, in the second patch where we are thinking to push Inserts in
> workers, we might need to do something to filter parallel-restricted
> cases during this stage of the planner.
>
Posting an updated Parallel INSERT patch which (mostly) addresses
previously-identified issues and suggestions.

More work needs to be done in order to support parallel UPDATE and
DELETE (even after application of Thomas Munro's combo-cid
parallel-support patch), but it is getting closer.

Regards,
Greg Nancarrow
Fujitsu Australia

v5-0001-Enable-parallel-INSERT-and-or-SELECT-for-INSERT-INTO.patch (69K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Parallel INSERT (INTO ... SELECT ...)

akapila
In reply to this post by Greg Nancarrow
On Thu, Oct 22, 2020 at 9:47 AM Greg Nancarrow <[hidden email]> wrote:

>
> On Fri, Oct 16, 2020 at 9:26 PM Amit Kapila <[hidden email]> wrote:
> >
>
> Posting an update to the smaller patch (Parallel SELECT for INSERT
> INTO...SELECT...).
>
> Most of this patch feeds into the larger Parallel INSERT patch, for
> which I'll also be posting an update soon.
>
> Patch updates include:
> - Removed explicit trigger-type checks (instead rely on declared
> trigger parallel safety)
> - Restored parallel-related XID checks that previous patch altered;
> now assign XID prior to entering parallel-mode
> - Now considers parallel-SELECT for parallel RESTRICTED cases (not
> just parallel SAFE cases)
> - Added parallel-safety checks for partition key expressions and
> support functions
> - Workaround added for test failure in "partition-concurrent-attach"
> test;
>

IIUC, below is code for this workaround:

+MaxRelParallelHazardForModify(Oid relid,
+ CmdType commandType,
+ max_parallel_hazard_context *context)
+{
+ Relation        rel;
+ TupleDesc tupdesc;
+ int attnum;
+
+ LOCKMODE lockmode = AccessShareLock;
+
+ /*
+ * It's possible that this relation is locked for exclusive access
+ * in another concurrent transaction (e.g. as a result of a
+ * ALTER TABLE ... operation) until that transaction completes.
+ * If a share-lock can't be acquired on it now, we have to assume this
+ * could be the worst-case, so to avoid blocking here until that
+ * transaction completes, conditionally try to acquire the lock and
+ * assume and return UNSAFE on failure.
+ */
+ if (ConditionalLockRelationOid(relid, lockmode))
+ {
+ rel = table_open(relid, NoLock);
+ }
+ else
+ {
+ context->max_hazard = PROPARALLEL_UNSAFE;
+ return context->max_hazard;
+ }

Do we need this workaround if we lock just the parent table instead of
locking all the tables? Basically, can we safely identify the
parallel-safety of partitioned relation if we just have a lock on
parent relation? One more thing I have noticed is that for scan
relations (Select query), we do such checks much later based on
RelOptInfo (see set_rel_consider_parallel) which seems to have most of
the information required to perform parallel-safety checks but I guess
for ModifyTable (aka the Insert table) the equivalent doesn't seem
feasible but have you thought of doing at the later stage in planner?

Few other comments on latest patch:
===============================
1.
MaxRelParallelHazardForModify()
{
..
+ if (commandType == CMD_INSERT || commandType == CMD_UPDATE)
+ {
+ /*
..

Why to check CMD_UPDATE here?

2.
+void PrepareParallelModeForModify(CmdType commandType, bool
isParallelModifyLeader)
+{
+ Assert(!IsInParallelMode());
+
+ if (isParallelModifyLeader)
+ (void)GetCurrentCommandId(true);
+
+ (void)GetCurrentFullTransactionId();

Here, we should use GetCurrentTransactionId() similar to heap_insert
or other heap operations. I am not sure why you have used
GetCurrentFullTransactionId?

3. Can we have a test to show why we need to check all the partitions
for parallel-safety? I think it would be possible when there is a
trigger on only one of the partitions and that trigger has
corresponding parallel_unsafe function. But it is good to verify that
once.

4. Have you checked the overhead of this on the planner for different
kinds of statements like inserts into tables having 100 or 500
partitions? Similarly, it is good to check the overhead of domain
related checks added in the patch.

5. Can we have a separate patch for parallel-selects for Insert? It
will make review easier.

--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

Re: Parallel INSERT (INTO ... SELECT ...)

Greg Nancarrow
On Tue, Oct 27, 2020 at 8:56 PM Amit Kapila <[hidden email]> wrote:

>
> IIUC, below is code for this workaround:
>
> +MaxRelParallelHazardForModify(Oid relid,
> + CmdType commandType,
> + max_parallel_hazard_context *context)
> +{
> + Relation        rel;
> + TupleDesc tupdesc;
> + int attnum;
> +
> + LOCKMODE lockmode = AccessShareLock;
> +
> + /*
> + * It's possible that this relation is locked for exclusive access
> + * in another concurrent transaction (e.g. as a result of a
> + * ALTER TABLE ... operation) until that transaction completes.
> + * If a share-lock can't be acquired on it now, we have to assume this
> + * could be the worst-case, so to avoid blocking here until that
> + * transaction completes, conditionally try to acquire the lock and
> + * assume and return UNSAFE on failure.
> + */
> + if (ConditionalLockRelationOid(relid, lockmode))
> + {
> + rel = table_open(relid, NoLock);
> + }
> + else
> + {
> + context->max_hazard = PROPARALLEL_UNSAFE;
> + return context->max_hazard;
> + }
>
> Do we need this workaround if we lock just the parent table instead of
> locking all the tables? Basically, can we safely identify the
> parallel-safety of partitioned relation if we just have a lock on
> parent relation?
I believe the workaround is still needed in this case, because the
workaround was added because of a test in which the parent table was
exclusively locked in another concurrent transaction (as a result of
ALTER TABLE ... ATTACH PARTITION ...) so we could not even get a
ShareLock on the parent table without hanging (and then ending up
failing the test because of it).
So at the moment the workaround is needed, even if just trying to lock
the parent table.
I'll do some more testing to determine the secondary issue of whether
locks on the partition tables are needed, but at the moment I believe
they are.

>One more thing I have noticed is that for scan
> relations (Select query), we do such checks much later based on
> RelOptInfo (see set_rel_consider_parallel) which seems to have most of
> the information required to perform parallel-safety checks but I guess
> for ModifyTable (aka the Insert table) the equivalent doesn't seem
> feasible but have you thought of doing at the later stage in planner?
>

Yes, and in fact I tried putting the checks in a later stage of the
planner, and it's almost successful, except it then makes setting
"parallelModeNeeded" very tricky indeed, because that is expected to
be set based on whether the SQL is safe to run in parallel mode
(paralleModeOK == true) and whether force_parallel_mode is not off.
With parallel safety checks delayed to a later stage in the planner,
it's then not known whether there are certain types of parallel-unsafe
INSERTs (such as INSERT INTO ... VALUES ... ON CONFLICT DO UPDATE
...), because processing for those doesn't reach those later stages of
the planner where parallelism is being considered. So then to avoid
errors from when parallel-mode is forced on and such unsafe INSERTs
are run, the only real choice is to only allow parallelModeNeeded to
be true for SELECT only (not INSERT), and this is kind of cheating and
also not picking up cases where parallel-safe INSERT is run but
invokes parallel-mode-unsafe features.
My conclusion, at least for the moment, is to leave the check where it is.


> Few other comments on latest patch:
> ===============================
> 1.
> MaxRelParallelHazardForModify()
> {
> ..
> + if (commandType == CMD_INSERT || commandType == CMD_UPDATE)
> + {
> + /*
> ..
>
> Why to check CMD_UPDATE here?
>
That was a bit of forward-thinking, for when/if UPDATE/DELETE is
supported in parallel-mode.
Column default expressions and check-constraints are only applicable
to INSERT and UPDATE.
Note however that currently this function can only ever be called with
commandType == CMD_INSERT.

> 2.
> +void PrepareParallelModeForModify(CmdType commandType, bool
> isParallelModifyLeader)
> +{
> + Assert(!IsInParallelMode());
> +
> + if (isParallelModifyLeader)
> + (void)GetCurrentCommandId(true);
> +
> + (void)GetCurrentFullTransactionId();
>
> Here, we should use GetCurrentTransactionId() similar to heap_insert
> or other heap operations. I am not sure why you have used
> GetCurrentFullTransactionId?
>
GetCurrentTransactionId() and GetCurrentFullTransactionId() actually
have the same functionality, just a different return value (which is
not being used here).
But anyway I've changed it to use GetCurrentTransactionId().


> 3. Can we have a test to show why we need to check all the partitions
> for parallel-safety? I think it would be possible when there is a
> trigger on only one of the partitions and that trigger has
> corresponding parallel_unsafe function. But it is good to verify that
> once.
>

I can't imagine how you could check parallel-safety properly without
checking all of the partitions.
We don't know which partition that data will get inserted into until
runtime (e.g. range/list partitioning).
Each partition can have its own column default expressions,
check-constraints, triggers etc. (which may or may not be
parallel-safe) and a partition may itself be a partitioned table.


> 4. Have you checked the overhead of this on the planner for different
> kinds of statements like inserts into tables having 100 or 500
> partitions? Similarly, it is good to check the overhead of domain
> related checks added in the patch.
>

Checking that now and will post results soon.

> 5. Can we have a separate patch for parallel-selects for Insert? It
> will make review easier.
>

See attached patches.


Regards,
Greg Nancarrow
Fujitsu Australia

v6-0001-Enable-parallel-SELECT-for-INSERT-INTO-.-SELECT.patch (26K) Download Attachment
v6-0002-Enable-parallel-INSERT-and-or-SELECT-for-INSERT-INTO.patch (46K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Parallel INSERT (INTO ... SELECT ...)

akapila
On Fri, Oct 30, 2020 at 6:09 AM Greg Nancarrow <[hidden email]> wrote:

>
> On Tue, Oct 27, 2020 at 8:56 PM Amit Kapila <[hidden email]> wrote:
> >
> > IIUC, below is code for this workaround:
> >
> > +MaxRelParallelHazardForModify(Oid relid,
> > + CmdType commandType,
> > + max_parallel_hazard_context *context)
> > +{
> > + Relation        rel;
> > + TupleDesc tupdesc;
> > + int attnum;
> > +
> > + LOCKMODE lockmode = AccessShareLock;
> > +
> > + /*
> > + * It's possible that this relation is locked for exclusive access
> > + * in another concurrent transaction (e.g. as a result of a
> > + * ALTER TABLE ... operation) until that transaction completes.
> > + * If a share-lock can't be acquired on it now, we have to assume this
> > + * could be the worst-case, so to avoid blocking here until that
> > + * transaction completes, conditionally try to acquire the lock and
> > + * assume and return UNSAFE on failure.
> > + */
> > + if (ConditionalLockRelationOid(relid, lockmode))
> > + {
> > + rel = table_open(relid, NoLock);
> > + }
> > + else
> > + {
> > + context->max_hazard = PROPARALLEL_UNSAFE;
> > + return context->max_hazard;
> > + }
> >
> > Do we need this workaround if we lock just the parent table instead of
> > locking all the tables? Basically, can we safely identify the
> > parallel-safety of partitioned relation if we just have a lock on
> > parent relation?
>
> I believe the workaround is still needed in this case, because the
> workaround was added because of a test in which the parent table was
> exclusively locked in another concurrent transaction (as a result of
> ALTER TABLE ... ATTACH PARTITION ...) so we could not even get a
> ShareLock on the parent table without hanging (and then ending up
> failing the test because of it).
>

Don't you think the test case design is flawed in that case? Because
even simple "select * from tpart;" will hang in planner while taking
share lock (the code flow is:
add_other_rels_to_query->expand_inherited_rtentry->expand_partitioned_rtentry)
once you take exclusive lock for a parallel session on the table.
Currently we never need to acquire any lock for Inserts in the planner
but not sure we can design a test case based on that assumption as we
can see it fails in this basic case.


> So at the moment the workaround is needed, even if just trying to lock
> the parent table.
>

I am not convinced, rather I think that the test case is not well
designed unless there is any other way (without taking share lock on
the relation) to determine parallel-safety of Inserts which neither of
us have thought of. I understand that you don't want to change that
test case as part of this patch so you are using this workaround.

> I'll do some more testing to determine the secondary issue of whether
> locks on the partition tables are needed, but at the moment I believe
> they are.
>

Fair enough but lets determine that by some testing and analysis. I
feel we should even add a comment if we require to lock all partition
tables. I see that we are already doing it for SELECT in the above
mentioned code path so maybe it is okay to do so for Inserts as well.

> >One more thing I have noticed is that for scan
> > relations (Select query), we do such checks much later based on
> > RelOptInfo (see set_rel_consider_parallel) which seems to have most of
> > the information required to perform parallel-safety checks but I guess
> > for ModifyTable (aka the Insert table) the equivalent doesn't seem
> > feasible but have you thought of doing at the later stage in planner?
> >
>
> Yes, and in fact I tried putting the checks in a later stage of the
> planner, and it's almost successful, except it then makes setting
> "parallelModeNeeded" very tricky indeed, because that is expected to
> be set based on whether the SQL is safe to run in parallel mode
> (paralleModeOK == true) and whether force_parallel_mode is not off.
> With parallel safety checks delayed to a later stage in the planner,
> it's then not known whether there are certain types of parallel-unsafe
> INSERTs (such as INSERT INTO ... VALUES ... ON CONFLICT DO UPDATE
> ...), because processing for those doesn't reach those later stages of
> the planner where parallelism is being considered.
>

I guess if that is the only case then you can have that check in the
earlier stage of planner (we should be able to do that as the
information is present in Query) and other checks in the later stage.
However, I guess that is not the only case, we need to determine
parallel-safety of index expressions, trigger functions if any, any
other CHECK expressions on each of attribute, etc.

> So then to avoid
> errors from when parallel-mode is forced on and such unsafe INSERTs
> are run, the only real choice is to only allow parallelModeNeeded to
> be true for SELECT only (not INSERT), and this is kind of cheating and
> also not picking up cases where parallel-safe INSERT is run but
> invokes parallel-mode-unsafe features.
> My conclusion, at least for the moment, is to leave the check where it is.
>

Okay, then can we integrate the functionality of
MaxParallelHazardForModify in max_parallel_hazard? Calling it
separately looks bit awkward.

>
> > Few other comments on latest patch:
> > ===============================
> > 1.
> > MaxRelParallelHazardForModify()
> > {
> > ..
> > + if (commandType == CMD_INSERT || commandType == CMD_UPDATE)
> > + {
> > + /*
> > ..
> >
> > Why to check CMD_UPDATE here?
> >
>
> That was a bit of forward-thinking, for when/if UPDATE/DELETE is
> supported in parallel-mode.
> Column default expressions and check-constraints are only applicable
> to INSERT and UPDATE.
> Note however that currently this function can only ever be called with
> commandType == CMD_INSERT.
>

I feel then for other command types there should be an Assert rather
than try to handle something which is not yet implemented nor it is
clear what all is required for that. It confuses the reader, at least
it confused me. Probably we can write a comment but I don't think we
should have any check for Update at this stage of work.

> > 2.
> > +void PrepareParallelModeForModify(CmdType commandType, bool
> > isParallelModifyLeader)
> > +{
> > + Assert(!IsInParallelMode());
> > +
> > + if (isParallelModifyLeader)
> > + (void)GetCurrentCommandId(true);
> > +
> > + (void)GetCurrentFullTransactionId();
> >
> > Here, we should use GetCurrentTransactionId() similar to heap_insert
> > or other heap operations. I am not sure why you have used
> > GetCurrentFullTransactionId?
> >
>
> GetCurrentTransactionId() and GetCurrentFullTransactionId() actually
> have the same functionality, just a different return value (which is
> not being used here).
>

Sure but lets use what is required.

> But anyway I've changed it to use GetCurrentTransactionId().
>

But comments in ExecutePlan and PrepareParallelModeForModify still
refer to FullTransactionId.

>
>
> > 4. Have you checked the overhead of this on the planner for different
> > kinds of statements like inserts into tables having 100 or 500
> > partitions? Similarly, it is good to check the overhead of domain
> > related checks added in the patch.
> >
>
> Checking that now and will post results soon.
>

Thanks.

--
With Regards,
Amit Kapila.


1234567 ... 19