Re: Parallel execution and prepared statements

Tobias Bussmann has discovered an oddity with prepared statements.

Parallel scan is used with prepared statements, but only if they have
been created with protocol V3 "Parse".
If a prepared statement has been prepared with the SQL statement PREPARE,
it will never use a parallel scan.

I guess that is an oversight in commit 57a6a72b, right?
PrepareQuery in commands/prepare.c should call CompleteCachedPlan
with cursor options CURSOR_OPT_PARALLEL_OK, just like
exec_prepare_message in tcop/postgres.c does.

The attached patch fixes the problem for me.

Yours,
Laurenz Albe

Thanks Laurenz, for your help with debugging on this topic. I was preparing a message to the list myself and found an earlier discussion [1] on the topic. If I understand it correctly, the issue is with CREATE TABLE ... AS EXECUTE .... So it seems parallel execution of prepared statements was intentionally disabled in 7bea19d. However, what is missing is at least a mention of that current limitation in the 9.6 docs at "When Can Parallel Query Be Used?" [2]

Best regards,
Tobias

[1] https://www.postgresql.org/message-id/CA%2BTgmoaxyXVr6WPDvPQduQpFhD9VRWExXU7axhDpJ7jZBvqxfQ%40mail.gmail.com
[2] https://www.postgresql.org/docs/current/static/when-can-parallel-query-be-used.html

> Am 15.11.2016 um 16:41 schrieb Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
>
> Tobias Bussmann has discovered an oddity with prepared statements.
>
> Parallel scan is used with prepared statements, but only if they have
> been created with protocol V3 "Parse".
> If a prepared statement has been prepared with the SQL statement PREPARE,
> it will never use a parallel scan.
>
> I guess that is an oversight in commit 57a6a72b, right?
> PrepareQuery in commands/prepare.c should call CompleteCachedPlan
> with cursor options CURSOR_OPT_PARALLEL_OK, just like
> exec_prepare_message in tcop/postgres.c does.
>
> The attached patch fixes the problem for me.
>
> Yours,
> Laurenz Albe
> <0001-Consider-parallel-plans-for-statements-prepared-with.patch>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

As the patch in [1] targeting the execution of the plan in ExecutePlan depending on the destination was declined, I hacked around a bit to find another way to use parallel mode with SQL prepared statements while disabling the parallel execution in case of an non read-only execution. For this I used the already present test for an existing intoClause in ExecuteQuery to set the parallelModeNeeded flag of the prepared statement. This results in a non parallel execution of the parallel plan, as we see with a non-zero fetch count used with the extended query protocol. Despite this patch seem to work in my tests, I'm by no means confident this being a proper way of handling the situation in question.

Best
Tobias

[1] https://www.postgresql.org/message-id/CAA4eK1KxiYm8F9Pe9xvqzoZocK43w%3DTRPUNHZpe_iOjF%3Dr%2B_Vw%40mail.gmail.com

On Wed, Nov 16, 2016 at 2:27 AM, Tobias Bussmann <t(dot)bussmann(at)gmx(dot)net> wrote:
> As the patch in [1] targeting the execution of the plan in ExecutePlan depending on the destination was declined, I hacked around a bit to find another way to use parallel mode with SQL prepared statements while disabling the parallel execution in case of an non read-only execution. For this I used the already present test for an existing intoClause in ExecuteQuery to set the parallelModeNeeded flag of the prepared statement. This results in a non parallel execution of the parallel plan, as we see with a non-zero fetch count used with the extended query protocol. Despite this patch seem to work in my tests, I'm by no means confident this being a proper way of handling the situation in question.
>

Can you once test by just passing CURSOR_OPT_PARALLEL_OK in
PrepareQuery() and run the tests by having forc_parallel_mode=regress?
It seems to me that the code in the planner is changed for setting a
parallelModeNeeded flag, so it might work as it is.

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

On Wed, Nov 16, 2016 at 7:10 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Wed, Nov 16, 2016 at 2:27 AM, Tobias Bussmann <t(dot)bussmann(at)gmx(dot)net> wrote:
>> As the patch in [1] targeting the execution of the plan in ExecutePlan depending on the destination was declined, I hacked around a bit to find another way to use parallel mode with SQL prepared statements while disabling the parallel execution in case of an non read-only execution. For this I used the already present test for an existing intoClause in ExecuteQuery to set the parallelModeNeeded flag of the prepared statement. This results in a non parallel execution of the parallel plan, as we see with a non-zero fetch count used with the extended query protocol. Despite this patch seem to work in my tests, I'm by no means confident this being a proper way of handling the situation in question.
>>
>
> Can you once test by just passing CURSOR_OPT_PARALLEL_OK in
> PrepareQuery() and run the tests by having forc_parallel_mode=regress?

Typo.
/forc_parallel_mode/force_parallel_mode

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

On Tue, Nov 15, 2016 at 10:41 AM, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at> wrote:
> Tobias Bussmann has discovered an oddity with prepared statements.
>
> Parallel scan is used with prepared statements, but only if they have
> been created with protocol V3 "Parse".
> If a prepared statement has been prepared with the SQL statement PREPARE,
> it will never use a parallel scan.
>
> I guess that is an oversight in commit 57a6a72b, right?
> PrepareQuery in commands/prepare.c should call CompleteCachedPlan
> with cursor options CURSOR_OPT_PARALLEL_OK, just like
> exec_prepare_message in tcop/postgres.c does.
>
> The attached patch fixes the problem for me.

Actually, commit 57a6a72b made this change, and then 7bea19d0 backed
it out again because it turned out to break things.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> Can you once test by just passing CURSOR_OPT_PARALLEL_OK in
> PrepareQuery() and run the tests by having forc_parallel_mode=regress?
> It seems to me that the code in the planner is changed for setting a
> parallelModeNeeded flag, so it might work as it is.

Do you mean running a "make installcheck" with "force_parallel_mode=regress" in postgresql.conf? I did so with just CURSOR_OPT_PARALLEL_OK in PrepareQuery (like the original commit 57a6a72b) and still got 3 failed tests, all with CREATE TABLE .. AS EXECUTE .. . With my patch applied, all tests were successful.

On Tue, Nov 15, 2016 at 3:57 PM, Tobias Bussmann <t(dot)bussmann(at)gmx(dot)net> wrote:
> As the patch in [1] targeting the execution of the plan in ExecutePlan depending on the destination was declined, I hacked around a bit to find another way to use parallel mode with SQL prepared statements while disabling the parallel execution in case of an non read-only execution. For this I used the already present test for an existing intoClause in ExecuteQuery to set the parallelModeNeeded flag of the prepared statement. This results in a non parallel execution of the parallel plan, as we see with a non-zero fetch count used with the extended query protocol. Despite this patch seem to work in my tests, I'm by no means confident this being a proper way of handling the situation in question.

Yeah, we could do something like this, perhaps not in exactly this
way, but I'm not sure it's a good idea to just execute the parallel
plan without workers.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> Yeah, we could do something like this, perhaps not in exactly this
> way, but I'm not sure it's a good idea to just execute the parallel
> plan without workers.

sure, executing parallel plans w/o workers seems a bit of a hack. But:
- we already do it this way in some other situations
- the alternative in this special situation would be to _force_ replanning without the CURSOR_OPT_PARALLEL_OK. The decision for replanning is hidden deep within plancache.c and while we could influence it with CURSOR_OPT_CUSTOM_PLAN this wouldn't have an effect if the prepared statement doesn't have any parameters. Additionally, influencing the decision and generating a non-parallel plan would shift the avg cost calculation used to choose custom or generic plans.

Maybe someone can come up with a better idea for a solution. These three approaches are all I see so far.

Best regards,
Tobias Bussmann

On Thu, Nov 17, 2016 at 10:07 AM, Tobias Bussmann <t(dot)bussmann(at)gmx(dot)net> wrote:
>> Yeah, we could do something like this, perhaps not in exactly this
>> way, but I'm not sure it's a good idea to just execute the parallel
>> plan without workers.
>
> sure, executing parallel plans w/o workers seems a bit of a hack. But:
> - we already do it this way in some other situations

True, but we also try to avoid it whenever possible, because it's
likely to lead to poor performance.

> - the alternative in this special situation would be to _force_ replanning without the CURSOR_OPT_PARALLEL_OK. The decision for replanning is hidden deep within plancache.c and while we could influence it with CURSOR_OPT_CUSTOM_PLAN this wouldn't have an effect if the prepared statement doesn't have any parameters. Additionally, influencing the decision and generating a non-parallel plan would shift the avg cost calculation used to choose custom or generic plans.

I think it would be a good idea to come up with a way for a query to
produce both a parallel and a non-parallel plan and pick between them
at execution time. However, that's more work than I've been willing
to undertake.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> On Tue, Nov 15, 2016 at 10:41 AM, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at> wrote:
>> Tobias Bussmann has discovered an oddity with prepared statements.
>>
>> Parallel scan is used with prepared statements, but only if they have
>> been created with protocol V3 "Parse".
>> If a prepared statement has been prepared with the SQL statement PREPARE,
>> it will never use a parallel scan.
>>
>> I guess that is an oversight in commit 57a6a72b, right?
>> PrepareQuery in commands/prepare.c should call CompleteCachedPlan
>> with cursor options CURSOR_OPT_PARALLEL_OK, just like
>> exec_prepare_message in tcop/postgres.c does.
>>
>> The attached patch fixes the problem for me.
>
> Actually, commit 57a6a72b made this change, and then 7bea19d0 backed
> it out again because it turned out to break things.

I didn't notice the CREATE TABLE ... AS EXECUTE problem.

But then the change to use CURSOR_OPT_PARALLEL_OK in tcop/postgres.c should
be reverted as well, because it breaks things just as bad:

/* creates a parallel-enabled plan */
PQprepare(conn, "pstmt", "SELECT id FROM l WHERE val = $1", 1, NULL);
/* blows up with "cannot insert tuples during a parallel operation" */
PQexec(conn, "CREATE TABLE bad(id) AS EXECUTE pstmt('abcd')");

With Tobias' patch, this does not fail.

I think we should either apply something like that patch or disable parallel
execution for prepared statements altogether and document that.

Yours,
Laurenz Albe

Amit Kapila wrote:
> Can you once test by just passing CURSOR_OPT_PARALLEL_OK in
> PrepareQuery() and run the tests by having forc_parallel_mode=regress?
> It seems to me that the code in the planner is changed for setting a
> parallelModeNeeded flag, so it might work as it is.

No, that doesn't work.

But with Tobias' complete patch "make installcheck-world" succeeds.

Yours,
Laurenz Albe

On Fri, Nov 18, 2016 at 8:21 AM, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at> wrote:
> I didn't notice the CREATE TABLE ... AS EXECUTE problem.
>
> But then the change to use CURSOR_OPT_PARALLEL_OK in tcop/postgres.c should
> be reverted as well, because it breaks things just as bad:
>
> /* creates a parallel-enabled plan */
> PQprepare(conn, "pstmt", "SELECT id FROM l WHERE val = $1", 1, NULL);
> /* blows up with "cannot insert tuples during a parallel operation" */
> PQexec(conn, "CREATE TABLE bad(id) AS EXECUTE pstmt('abcd')");

Ouch. I missed that.

> With Tobias' patch, this does not fail.
>
> I think we should either apply something like that patch or disable parallel
> execution for prepared statements altogether and document that.

I agree. I think probably the first option is better, but I might
have to commit with one hand so I can hold my nose with the other.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
>> /* creates a parallel-enabled plan */
>> PQprepare(conn, "pstmt", "SELECT id FROM l WHERE val = $1", 1, NULL);
>> /* blows up with "cannot insert tuples during a parallel operation" */
>> PQexec(conn, "CREATE TABLE bad(id) AS EXECUTE pstmt('abcd')");
>
> Ouch. I missed that.
>
>> With Tobias' patch, this does not fail.
>>
>> I think we should either apply something like that patch or disable parallel
>> execution for prepared statements altogether and document that.
>
> I agree. I think probably the first option is better, but I might
> have to commit with one hand so I can hold my nose with the other.

Is there a better, more consistent approach for that?

Yours,
Laurenz Albe

> True, but we also try to avoid it whenever possible, because it's
> likely to lead to poor performance.

This non-readonly case should be way less often hit compared to other uses of prepared statements. But sure, it depends on the individual use case and a likely performance regession in these edge cases is nothing to decide for easily.

> I think it would be a good idea to come up with a way for a query to
> produce both a parallel and a non-parallel plan and pick between them
> at execution time. However, that's more work than I've been willing
> to undertake.

Wouldn't the precautionary generation of two plans always increase the planning overhead, which precisely is what one want to reduce by using prepared statements?

Best regards
Tobias

Am 18.11.2016 um 14:21 schrieb Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
> But then the change to use CURSOR_OPT_PARALLEL_OK in tcop/postgres.c should
> be reverted as well, because it breaks things just as bad:
>
> /* creates a parallel-enabled plan */
> PQprepare(conn, "pstmt", "SELECT id FROM l WHERE val = $1", 1, NULL);
> /* blows up with "cannot insert tuples during a parallel operation" */
> PQexec(conn, "CREATE TABLE bad(id) AS EXECUTE pstmt('abcd')");

Great example of mixing a v3 prepare with an simple query execute. I didn't think about that while even the docs state clearly: "Named prepared statements can also be created and accessed at the SQL command level, using PREPARE and EXECUTE." Sticking with either protocol version does not trigger the error.

> I think we should either apply something like that patch or disable parallel
> execution for prepared statements altogether and document that.

So we likely need to backpatch something more then a doc-fix for 9.6. Given the patch proposals around, this would either disable a feature (in extended query protocol) or add a new one (in simple query protocol/SQL). Or would it be more appropriate to split the patch and use CURSOR_OPT_PARALLEL_OK in prepare.c on master only? I'm asking in case there is a necessity to prepare a proposal for an documentation patch targeting 9.6 specifically.

Best regards
Tobias

There has been a previous discussion about this topic, including an attempted fix by Amit Kapila:
http://www.postgresql.org/message-id/flat/CAA4eK1L=tHmmHDK_KW_ja1_dusJxJF+SGQHi=APS4MdNPk7HFQ(at)mail(dot)gmail(dot)com/

On Fri, Nov 18, 2016 at 9:01 PM, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at> wrote:
> Amit Kapila wrote:
>> Can you once test by just passing CURSOR_OPT_PARALLEL_OK in
>> PrepareQuery() and run the tests by having forc_parallel_mode=regress?
>> It seems to me that the code in the planner is changed for setting a
>> parallelModeNeeded flag, so it might work as it is.
>
> No, that doesn't work.
>
> But with Tobias' complete patch "make installcheck-world" succeeds.
>

Okay. I have looked into patch proposed and found that it will
resolve the regression test problem (Create Table .. AS Execute). I
think it is slightly prone to breakage if tomorrow we implement usage
of EXECUTE with statements other Create Table (like Copy). Have you
registered this patch in CF to avoid loosing track?

We have two patches (one proposed in this thread and one proposed by
me earlier [1]) and both solves the regression test failure. Unless
there is some better approach, I think we can go with one of these.

[1] - https://www.postgresql.org/message-id/CAA4eK1L%3DtHmmHDK_KW_ja1_dusJxJF%2BSGQHi%3DAPS4MdNPk7HFQ%40mail.gmail.com
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila wrote:
>> But with Tobias' complete patch "make installcheck-world" succeeds.
>
> Okay. I have looked into patch proposed and found that it will
> resolve the regression test problem (Create Table .. AS Execute). I
> think it is slightly prone to breakage if tomorrow we implement usage
> of EXECUTE with statements other Create Table (like Copy). Have you
> registered this patch in CF to avoid loosing track?
>
> We have two patches (one proposed in this thread and one proposed by
> me earlier [1]) and both solves the regression test failure. Unless
> there is some better approach, I think we can go with one of these.

Tobias did that here: https://commitfest.postgresql.org/12/890/

He added your patch as well.

Yours,
Laurenz Albe

On Tue, Nov 29, 2016 at 4:40 PM, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at> wrote:
> Amit Kapila wrote:
>>> But with Tobias' complete patch "make installcheck-world" succeeds.
>>
>> Okay. I have looked into patch proposed and found that it will
>> resolve the regression test problem (Create Table .. AS Execute). I
>> think it is slightly prone to breakage if tomorrow we implement usage
>> of EXECUTE with statements other Create Table (like Copy). Have you
>> registered this patch in CF to avoid loosing track?
>>
>> We have two patches (one proposed in this thread and one proposed by
>> me earlier [1]) and both solves the regression test failure. Unless
>> there is some better approach, I think we can go with one of these.
>
> Tobias did that here: https://commitfest.postgresql.org/12/890/
>
> He added your patch as well.
>

Okay, Thanks.

Robert, do you have any better ideas for this problem?

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

On Tue, Nov 29, 2016 at 6:24 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Tue, Nov 29, 2016 at 4:40 PM, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at> wrote:
>> Amit Kapila wrote:
>>>> But with Tobias' complete patch "make installcheck-world" succeeds.
>>>
>>> Okay. I have looked into patch proposed and found that it will
>>> resolve the regression test problem (Create Table .. AS Execute). I
>>> think it is slightly prone to breakage if tomorrow we implement usage
>>> of EXECUTE with statements other Create Table (like Copy). Have you
>>> registered this patch in CF to avoid loosing track?
>>>
>>> We have two patches (one proposed in this thread and one proposed by
>>> me earlier [1]) and both solves the regression test failure. Unless
>>> there is some better approach, I think we can go with one of these.
>>
>> Tobias did that here: https://commitfest.postgresql.org/12/890/
>>
>> He added your patch as well.
>>
>
> Okay, Thanks.
>
> Robert, do you have any better ideas for this problem?

Not really. I think your prepared_stmt_parallel_query_v2.patch is
probably the best approach proposed so far, but I wonder why we need
to include DestCopyOut and DestTupleStore. DestIntoRel and
DestTransientRel both write to an actual relation, which is a problem
for parallel mode, but I think the others don't.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Nov 30, 2016 at 11:57 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Nov 29, 2016 at 6:24 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>
>> Robert, do you have any better ideas for this problem?
>
> Not really. I think your prepared_stmt_parallel_query_v2.patch is
> probably the best approach proposed so far, but I wonder why we need
> to include DestCopyOut and DestTupleStore. DestIntoRel and
> DestTransientRel both write to an actual relation, which is a problem
> for parallel mode, but I think the others don't.
>

I have tried to restrict all the non-readonly operation modes or modes
where parallelism might not make sense like DestTupleStore. If we
want to just prohibit the cases where it can fail now, then I think
prohibiting only DestIntoRel should be sufficient because that is a
case where the user is allowed to do DDL for an already prepared read
only statement like Create Table AS .. EXECUTE.

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

On Thu, Dec 1, 2016 at 7:57 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Wed, Nov 30, 2016 at 11:57 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Tue, Nov 29, 2016 at 6:24 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>>
>>> Robert, do you have any better ideas for this problem?
>>
>> Not really. I think your prepared_stmt_parallel_query_v2.patch is
>> probably the best approach proposed so far, but I wonder why we need
>> to include DestCopyOut and DestTupleStore. DestIntoRel and
>> DestTransientRel both write to an actual relation, which is a problem
>> for parallel mode, but I think the others don't.
>>
>
> I have tried to restrict all the non-readonly operation modes or modes
> where parallelism might not make sense like DestTupleStore. If we
> want to just prohibit the cases where it can fail now, then I think
> prohibiting only DestIntoRel should be sufficient because that is a
> case where the user is allowed to do DDL for an already prepared read
> only statement like Create Table AS .. EXECUTE.

OK, then my vote is to do it that way for now.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Dec 1, 2016 at 9:40 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Dec 1, 2016 at 7:57 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>
>> I have tried to restrict all the non-readonly operation modes or modes
>> where parallelism might not make sense like DestTupleStore. If we
>> want to just prohibit the cases where it can fail now, then I think
>> prohibiting only DestIntoRel should be sufficient because that is a
>> case where the user is allowed to do DDL for an already prepared read
>> only statement like Create Table AS .. EXECUTE.
>
> OK, then my vote is to do it that way for now.
>

Done that way in attached patch.

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

> On Thu, Dec 1, 2016 at 9:40 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>
>> OK, then my vote is to do it that way for now.

Thanks for your opinion. That's fine with me.

> Am 02.12.2016 um 07:22 schrieb Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>:
> Done that way in attached patch.

Did a quick review: The patch applies cleanly against current head. make installcheck with force_parallel_mode = regress passes all tests. My manual tests show that parallel query is working for prepared statements in SQL with PREPARE and EXECUTE. CREATE TABLE AS EXECUTE is working, EXPLAIN on that shows a parallel plan, EXPLAIN ANALZE indicates 0 launched workers for that. Looks fine so far!

You should however include a sentence in the documentation on that parallel plan w/o workers corner-case behaviour. Feel free to take that from my patch or phase a better wording.

And again my question regarding back patching to 9.6:
- 9.6 is currently broken as Laurenz showed in [1]
- 9.6 does not have documented that SQL PREPARE prepared statements cannot not use parallel query

The former could be fixed by back patching the full patch which would void the latter. Or it could be fixed by disabling generation of parallel plans in extended query protocol prepare. Alternatively only the change in execMain.c could be back patched. In these cases we would need to have the a separate wording for the 9.6 docs.

Best regards,
Tobias

[1] A737B7A37273E048B164557ADEF4A58B539990D0(at)ntex2010i(dot)host(dot)magwien(dot)gv(dot)at

On Fri, Dec 2, 2016 at 3:31 PM, Tobias Bussmann <t(dot)bussmann(at)gmx(dot)net> wrote:
>
>> On Thu, Dec 1, 2016 at 9:40 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>
>>> OK, then my vote is to do it that way for now.
>
> Thanks for your opinion. That's fine with me.
>
>> Am 02.12.2016 um 07:22 schrieb Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>:
>> Done that way in attached patch.
>
> Did a quick review:
>

Thanks for the review.

> You should however include a sentence in the documentation on that parallel plan w/o workers corner-case behaviour. Feel free to take that from my patch or phase a better wording.
>

I have taken it from your patch.

> And again my question regarding back patching to 9.6:
> - 9.6 is currently broken as Laurenz showed in [1]
> - 9.6 does not have documented that SQL PREPARE prepared statements cannot not use parallel query
>
> The former could be fixed by back patching the full patch which would void the latter.
>

I think if we don't see any impact then we should backpatch this
patch. However, if we decide to go some other way, then I can provide
a separate patch for back branches. BTW, what is your opinion?

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

> I think if we don't see any impact then we should backpatch this
> patch. However, if we decide to go some other way, then I can provide
> a separate patch for back branches. BTW, what is your opinion?

I could not find anything on backporting guidelines in the wiki but my opinion would be to backpatch the patch in total. With a different behaviour between the simple and extended query protocol it would be hard to debug query performance issue in user applications that uses PQprepare. If the user tries to replicate a query with a PREPARE in psql and tries to EXPLAIN EXECUTE it, the results would be different then what happens within the application. That behaviour could be confusing, like differences between EXPLAIN SELECT and EXPLAIN EXECUTE can be to less experienced users.

Best regards
Tobias

Tobias Bussmann wrote:
>> I think if we don't see any impact then we should backpatch this
>> patch. However, if we decide to go some other way, then I can provide
>> a separate patch for back branches. BTW, what is your opinion?
>
> I could not find anything on backporting guidelines in the wiki but my opinion would be to backpatch
> the patch in total. With a different behaviour between the simple and extended query protocol it would
> be hard to debug query performance issue in user applications that uses PQprepare. If the user tries
> to replicate a query with a PREPARE in psql and tries to EXPLAIN EXECUTE it, the results would be
> different then what happens within the application. That behaviour could be confusing, like
> differences between EXPLAIN SELECT and EXPLAIN EXECUTE can be to less experienced users.

+1

Yours,
Laurenz Albe

On Mon, Dec 5, 2016 at 5:18 AM, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at> wrote:
> Tobias Bussmann wrote:
>>> I think if we don't see any impact then we should backpatch this
>>> patch. However, if we decide to go some other way, then I can provide
>>> a separate patch for back branches. BTW, what is your opinion?
>>
>> I could not find anything on backporting guidelines in the wiki but my opinion would be to backpatch
>> the patch in total. With a different behaviour between the simple and extended query protocol it would
>> be hard to debug query performance issue in user applications that uses PQprepare. If the user tries
>> to replicate a query with a PREPARE in psql and tries to EXPLAIN EXECUTE it, the results would be
>> different then what happens within the application. That behaviour could be confusing, like
>> differences between EXPLAIN SELECT and EXPLAIN EXECUTE can be to less experienced users.
>
> +1

Done.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Done.

The comment seems quite confused now:

* If a tuple count was supplied or data is being written to relation, we
* must force the plan to run without parallelism, because we might exit
* early.

Exit early is exactly what we *won't* do if writing to an INTO rel, so
I think this will confuse future readers. I think it should be more like

* If a tuple count was supplied, we must force the plan to run without
* parallelism, because we might exit early. Also disable parallelism
* when writing into a relation, because [ uh, why exactly? ]

Considering that the writing would happen at top level of the executor,
and hence in the parent process, it's not actually clear to me why the
second restriction is there at all: can't we write tuples to a rel even
though they came from a parallel worker? In any case, the current wording
of the comment is a complete fail at explaining this.

regards, tom lane

On Tue, Dec 6, 2016 at 1:07 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Done.
>
> The comment seems quite confused now:
>
> * If a tuple count was supplied or data is being written to relation, we
> * must force the plan to run without parallelism, because we might exit
> * early.
>
> Exit early is exactly what we *won't* do if writing to an INTO rel, so
> I think this will confuse future readers. I think it should be more like
>
> * If a tuple count was supplied, we must force the plan to run without
> * parallelism, because we might exit early. Also disable parallelism
> * when writing into a relation, because [ uh, why exactly? ]
>
> Considering that the writing would happen at top level of the executor,
> and hence in the parent process, it's not actually clear to me why the
> second restriction is there at all: can't we write tuples to a rel even
> though they came from a parallel worker? In any case, the current wording
> of the comment is a complete fail at explaining this.

Oops. You're right. [ uh, why exactly? ] -> no database changes
whatsoever are allowed while in parallel mode. (This restriction
might be lifted someday, but right now we're stuck with it.)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Dec 7, 2016 at 12:30 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Dec 6, 2016 at 1:07 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> Done.
>>
>> The comment seems quite confused now:
>>
>> * If a tuple count was supplied or data is being written to relation, we
>> * must force the plan to run without parallelism, because we might exit
>> * early.
>>
>> Exit early is exactly what we *won't* do if writing to an INTO rel, so
>> I think this will confuse future readers. I think it should be more like
>>
>> * If a tuple count was supplied, we must force the plan to run without
>> * parallelism, because we might exit early. Also disable parallelism
>> * when writing into a relation, because [ uh, why exactly? ]
>>
>> Considering that the writing would happen at top level of the executor,
>> and hence in the parent process, it's not actually clear to me why the
>> second restriction is there at all: can't we write tuples to a rel even
>> though they came from a parallel worker? In any case, the current wording
>> of the comment is a complete fail at explaining this.
>
> Oops. You're right. [ uh, why exactly? ] -> no database changes
> whatsoever are allowed while in parallel mode. (This restriction
> might be lifted someday, but right now we're stuck with it.)
>

Attached patch changes the comment based on above suggestions.

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

On Wed, Dec 7, 2016 at 9:23 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> Attached patch changes the comment based on above suggestions.

Thanks. Committed and back-patched to 9.6.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company