Thread: [HACKERS] PATCH: enabling parallel execution for cursors explicitly(experimental)

Hi,

One of the existing limitations of parallel query is that cursors
generally do not benefit from it [1]. Commit 61c2e1a95f [2] improved the
situation for cursors from procedural languages, but unfortunately for
user-defined cursors parallelism is still disabled.

For many use cases that is perfectly fine, but for applications that
need to process large amounts of data this is rather annoying. When the
result sets are large, cursors are extremely efficient - in terms of
memory consumption, for example. So the applications have to choose
between "cursor" approach (and no parallelism), or parallelism and
uncomfortably large result sets.

I believe there are two main reasons why parallelism is disabled for
user-defined cursors (or queries that might get suspended):

(1) We can't predict what will happen while the query is suspended (and
the transaction is still in "parallel mode"), e.g. the user might run
arbitrary DML which is not allowed.

(2) If the cursor gets suspended, the parallel workers would be still
assigned to it and could not be used for anything else.

Clearly, we can't solve those issues in general, so the default will
probably remain "parallelism disabled".

I propose is to add a new cursor option (PARALLEL), which would allow
parallel plans for that particular user-defined cursor. Attached is an
experimental patch doing this (I'm sure there are some loose ends).

This does not make either any of the issues go away, of course. We still
enforce "no DML while parallel operation in progress" as before, so this
will not work:

BEGIN;
DECLARE x PARALLEL CURSOR FOR SELECT * FROM t2 WHERE ...;
FETCH 1000 FROM x;
INSERT INTO t2 VALUES (1);
FETCH 1000 FROM x;
COMMIT;

but this will

BEGIN;
DECLARE x PARALLEL CURSOR FOR SELECT * FROM t2 WHERE ...;
FETCH 1000 FROM x;
...
FETCH 1000 FROM x;
CLOSE x;
INSERT INTO t2 VALUES (1);
COMMIT;

Regarding (2), if the user suspends the cursor for a long time, bummer.
The parallel workers will remain assigned, doing nothing. I don't have
any idea how to get around that, but I don't see how we could do better.
I don't see either of these limitations as fatal.

Any opinions / obvious flaws that I missed?

regards

[1]
https://www.postgresql.org/docs/9.6/static/when-can-parallel-query-be-used.html

[2]
https://www.postgresql.org/message-id/CAOGQiiMfJ%2B4SQwgG%3D6CVHWoisiU0%2B7jtXSuiyXBM3y%3DA%3DeJzmg%40mail.gmail.com

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, Oct 14, 2017 at 11:44 PM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> Hi,
>
>
> I propose is to add a new cursor option (PARALLEL), which would allow
> parallel plans for that particular user-defined cursor. Attached is an
> experimental patch doing this (I'm sure there are some loose ends).
>

How will this work for backward scans?


>
> Regarding (2), if the user suspends the cursor for a long time, bummer.
> The parallel workers will remain assigned, doing nothing. I don't have
> any idea how to get around that, but I don't see how we could do better.
>

One idea could be that whenever someone uses Parallel option, we can
fetch and store all the data locally before returning control to the
user (something like WITH HOLD option).

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 10/16/2017 01:13 PM, Amit Kapila wrote:
> On Sat, Oct 14, 2017 at 11:44 PM, Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
>> Hi,
>>
>>
>> I propose is to add a new cursor option (PARALLEL), which would allow
>> parallel plans for that particular user-defined cursor. Attached is an
>> experimental patch doing this (I'm sure there are some loose ends).
>>
> 
> How will this work for backward scans?
> 

It may not work, just like for regular cursors without SCROLL. And if
you specify SCROLL, then I believe a Materialize node will be added
automatically if needed, but haven't tried.

> 
>>
>> Regarding (2), if the user suspends the cursor for a long time, bummer.
>> The parallel workers will remain assigned, doing nothing. I don't have
>> any idea how to get around that, but I don't see how we could do better.
>>
> 
> One idea could be that whenever someone uses Parallel option, we can 
> fetch and store all the data locally before returning control to the 
> user (something like WITH HOLD option).
> 

I don't like that very much, as it adds unnecessary overhead. As I said
before, I'm particularly interested in cursors returning large amounts
of data, so the overhead may be quite significant.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, Oct 14, 2017 at 2:14 PM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> I propose is to add a new cursor option (PARALLEL), which would allow
> parallel plans for that particular user-defined cursor. Attached is an
> experimental patch doing this (I'm sure there are some loose ends).

I think you would need to do a huge amount of additional work in order
to actually make this robust.  I believe that a fair amount of what
goes on in parallel mode right now is checked with elog() if we think
that it's unreachable without writing C code -- but this will make a
lot of those things reachable, which means they would need to be
checked some other way.  Also, I doubt this guarantees that we won't
try to call parallel-unsafe functions will parallel mode is active, so
things will just blow up in whatever way they do, maybe crashing the
server or rendering the database inconsistent or whatever.

Possibly I'm overestimating the extent of the danger, but I don't
think so.  You're try to take a mechanism that was only ever meant to
be active during the course of one query and applying it for long
periods of time during which a user can do anything, with basically no
upgrade of the infrastructure.  I think something like this could be
made to work if you put a large amount of energy into it, but I think
the patch as proposed is about the easiest 3-5% of what would actually
be required to cover all the holes.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 10/17/2017 03:16 PM, Robert Haas wrote:
> On Sat, Oct 14, 2017 at 2:14 PM, Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
>> I propose is to add a new cursor option (PARALLEL), which would allow
>> parallel plans for that particular user-defined cursor. Attached is an
>> experimental patch doing this (I'm sure there are some loose ends).
> 
> I think you would need to do a huge amount of additional work in
> order to actually make this robust. I believe that a fair amount of
> what goes on in parallel mode right now is checked with elog() if we
> think that it's unreachable without writing C code -- but this will
> make a lot of those things reachable, which means they would need to
> be checked some other way.

Sure, additional checks may be necessary. I've tried to come up with
examples causing crashes, and haven't succeeded. Of course, that's no
proof of correctness, so if you have an example that will crash and burn
I'd like to see see it.

In general, it may be acceptable to rely on the elog() checks - which is
pretty much what the FETCH+INSERT+SHARE example I shared in the first
message of this thread does. I agree it's not particularly convenient,
and perhaps we should replace it with checks while planning the queries.

> Also, I doubt this guarantees that we won't try to call
> parallel-unsafe functions will parallel mode is active, so things
> will just blow up in whatever way they do, maybe crashing the server
> or rendering the database inconsistent or whatever.
> 

Probably. What happens right now is that declaring the cursor switches
the whole transaction into parallel mode (EnterParallelMode), so if you
do FETCH + INSERT + FETCH that will fail with elog(ERROR).

But yeah, this should probably be checked at planning time, and the
whole query should fail if the transaction is in parallel mode and the
query contains unsafe/restricted functions.

> Possibly I'm overestimating the extent of the danger, but I don't
> think so.  You're try to take a mechanism that was only ever meant to
> be active during the course of one query and applying it for long
> periods of time during which a user can do anything, with basically no
> upgrade of the infrastructure.  I think something like this could be
> made to work if you put a large amount of energy into it, but I think
> the patch as proposed is about the easiest 3-5% of what would actually
> be required to cover all the holes.
> 

Well, soliciting feedback like this was one of the points of sharing the
experimental patch, so thank you for that. I'm not sure if the estimate
of 3-5% is accurate, but I'm sure the patch is incomplete - which is why
I marked it as experimental, after all.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Oct 17, 2017 at 12:06 PM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> In general, it may be acceptable to rely on the elog() checks - which is
> pretty much what the FETCH+INSERT+SHARE example I shared in the first
> message of this thread does. I agree it's not particularly convenient,
> and perhaps we should replace it with checks while planning the queries.

Those checks are very much not comprehensive, though.  For example,
consider commit e9baa5e9fa147e00a2466ab2c40eb99c8a700824, which
allowed heap_insert() in parallel mode.  Here's the comment:
    /*
+     * Parallel operations are required to be strictly read-only in a parallel
+     * worker.  Parallel inserts are not safe even in the leader in the
+     * general case, because group locking means that heavyweight locks for
+     * relation extension or GIN page locks will not conflict between members
+     * of a lock group, but we don't prohibit that case here because there are
+     * useful special cases that we can safely allow, such as CREATE TABLE AS.
+     */
+    if (IsParallelWorker())        ereport(ERROR,                (errcode(ERRCODE_INVALID_TRANSACTION_STATE),
+                 errmsg("cannot insert tuples in a parallel worker")));

Now, as things stand, there's no way for this code to be reached
except in the case where the inserts are targeting a new table, or at
least I hope there isn't, because that would be a bug.  But if we
applied your patch then it could be easily done: just start a parallel
cursor and then run an INSERT command.  It might take up a little work
to gin up a test case that shows this really crashing and burning, but
I'm very confident that it's possible.  I wouldn't have gone to the
trouble of installing checks to prevent inserts from running in
parallel mode if inserts were safe in parallel mode.

>> Also, I doubt this guarantees that we won't try to call
>> parallel-unsafe functions will parallel mode is active, so things
>> will just blow up in whatever way they do, maybe crashing the server
>> or rendering the database inconsistent or whatever.
>
> Probably. What happens right now is that declaring the cursor switches
> the whole transaction into parallel mode (EnterParallelMode), so if you
> do FETCH + INSERT + FETCH that will fail with elog(ERROR).

But, again, only in the limited cases that the elog() checks catch.  A
C function can be parallel-unsafe without doing anything that hits the
elog() checks; there is no way for the system to protect itself
against arbitrary C code.  The elog() checks are intended to catch the
common or likely ways of breaking the world, but arbitrarily C code
can work around those checks -- if nothing else, duplicate one of the
functions that has an elog() in it, remove the elog(), and then call
it.  You just did something parallel-safe, unchecked!  That's an
artificial example, of course, which is not likely to occur in
practice, but I'm pretty confident that there are non-artificial
examples also.

I think this is a more or less classic kind of programming problem -
you're proposing to take a set of checks that were intended to ensure
safety under a limited set of circumstances and generalize them to a
much broader context than the one for which they were designed.  They
will not be adequate to those circumstances, and a considerable amount
of analysis will be needed to figure out where the gaps are.  If
somebody wants to do that analysis, I'm willing to review it and try
to spot any holes, but I don't really want to spend the time to go do
the whole analysis myself.

The main points I want to make clearly understood is the current
design relies on (1) functions being labeled correctly and (2) other
dangerous code paths being unreachable because there's nothing that
runs between EnterParallelMode and ExitParallelMode which could invoke
them, except by calling a mislabeled function.  Your patch expands the
vulnerability surface from "executor code that can be reached without
calling a mislabeled function" to "any code that can be reached by
typing an SQL command".  Just rejecting any queries that are
parallel-unsafe probably closes a good chunk of the holes, but that
still leaves a lot of code that's never been run in parallel mode
before potentially now running in parallel mode - e.g. any DDL command
you happen to type, transaction control commands, code that only runs
when the server is idle like idle_in_transaction_timeout, cursor
operations.  A lot of that stuff is probably fine, but it's got to be
thought through.  Error handling might be a problem, too: what happens
if a parallel worker is killed while the query is suspended?  I
suspect that doesn't work very nicely at all.

One way to get some ideas about where the problem lies would be to
write a test patch that keeps parallel mode active at all times except
when running a query that contains something parallel-unsafe.  Then
run the regression tests and see if anything blows up.  That's not
going to find everything that needs to be fixed but it would be a good
experiment to try.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

On 10/20/2017 03:23 PM, Robert Haas wrote:
>
> ...
> 
> The main points I want to make clearly understood is the current
> design relies on (1) functions being labeled correctly and (2) other
> dangerous code paths being unreachable because there's nothing that
> runs between EnterParallelMode and ExitParallelMode which could invoke
> them, except by calling a mislabeled function.  Your patch expands the
> vulnerability surface from "executor code that can be reached without
> calling a mislabeled function" to "any code that can be reached by
> typing an SQL command".  Just rejecting any queries that are
> parallel-unsafe probably closes a good chunk of the holes, but that
> still leaves a lot of code that's never been run in parallel mode
> before potentially now running in parallel mode - e.g. any DDL command
> you happen to type, transaction control commands, code that only runs
> when the server is idle like idle_in_transaction_timeout, cursor
> operations.  A lot of that stuff is probably fine, but it's got to be
> thought through.  Error handling might be a problem, too: what happens
> if a parallel worker is killed while the query is suspended?  I
> suspect that doesn't work very nicely at all.
> 

OK, understood and thanks for explaining what may be the possible
issues. I do appreciate that.

I still think it'd be valuable to support this, though, so I'm going to
spend more time on investigating what needs to be handled.

But maybe there's a simpler option - what if we only allow fetches from
the PARALLEL cursor while the cursor is open? That is, this would work:
   BEGIN;   ...   DECLARE x PARALLEL CURSOR FOR SELECT * FROM t2 WHERE ...;   FETCH 1000 FROM x;   FETCH 1000 FROM x;
FETCH1000 FROM x;   CLOSE x;   ...   COMMIT;
 

but adding any other command between the OPEN/CLOSE commands would fail.
That should close all the holes with parallel-unsafe stuff, right?

Of course, this won't solve the issue with error handling / killing
suspended workers (which didn't occur to me before as a possible issue
at all, so that's for pointing that out). But that's a significantly
more limited issue to fix than all the parallel-unsafe bits.

Now, I agree this is somewhat more limited than I hoped for, but OTOH it
still solves the issue I initially aimed for (processing large query
results in efficient way).


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

> Now, I agree this is somewhat more limited than I hoped for, but OTOH it
> still solves the issue I initially aimed for (processing large query
> results in efficient way).

I don't quite understand this part.

We already send results to the client in a stream unless it's
something we have to materialize, in which case a cursor won't help
anyway.

If the client wants to fetch in chunks it can use a portal and limited
size fetches. That shouldn't (?) be parallel-unsafe, since nothing
else can happen in the middle anyway.

But in most cases the client can just fetch all, and let the socket
buffering take care of things, reading results only when it wants
them, and letting the server block when the windows are full.

That's not to say that SQL-level cursor support wouldn't be nice. I'm
just trying to better understand what it's solving.

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Nov 1, 2017 at 7:49 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
> If the client wants to fetch in chunks it can use a portal and limited
> size fetches. That shouldn't (?) be parallel-unsafe, since nothing
> else can happen in the middle anyway.

I believe sending a limited-size fetch forces serial execution
currently.  If it's true that nothing else can happen in the middle
then we could relax that, but I don't see why that should be true?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Nov 1, 2017 at 3:47 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> But maybe there's a simpler option - what if we only allow fetches from
> the PARALLEL cursor while the cursor is open? That is, this would work:
>
>     BEGIN;
>     ...
>     DECLARE x PARALLEL CURSOR FOR SELECT * FROM t2 WHERE ...;
>     FETCH 1000 FROM x;
>     FETCH 1000 FROM x;
>     FETCH 1000 FROM x;
>     CLOSE x;
>     ...
>     COMMIT;
>
> but adding any other command between the OPEN/CLOSE commands would fail.
> That should close all the holes with parallel-unsafe stuff, right?

I think that still leaves a fair number of scenarios to consider, and
the error handling by itself seems pretty thorny.  Plus it's kind of a
weird mode and, like Craig, I'm not really sure what it gets you.
Maybe if somebody has the use case where this would help, they should
just do:

CREATE TEMP TABLE x AS SELECT * FROM t2 WHERE ...;
DECLARE x CURSOR FOR SELECT * FROM x;

Since e9baa5e9fa147e00a2466ab2c40eb99c8a700824, that ought to work.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2 November 2017 at 10:01, Robert Haas <robertmhaas@gmail.com> wrote:

> I think that still leaves a fair number of scenarios to consider, and
> the error handling by itself seems pretty thorny.  Plus it's kind of a
> weird mode and, like Craig, I'm not really sure what it gets you.
> Maybe if somebody has the use case where this would help, they should
> just do:
>
> CREATE TEMP TABLE x AS SELECT * FROM t2 WHERE ...;
> DECLARE x CURSOR FOR SELECT * FROM x;

That forces materialization, and I'm guessing part of Tomas's goal
here is to prevent the need to materialize into a temp table /
tuplestore / etc.

It's not clear to me why an unbounded portal fetch, using the tcp
socket windows and buffers for flow control, isn't sufficient.

Tomas, can you explain the use case a bit more?


-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Nov 2, 2017 at 8:01 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
> That forces materialization, and I'm guessing part of Tomas's goal
> here is to prevent the need to materialize into a temp table /
> tuplestore / etc.

I get that, but if you're running a query like "SELECT * FROM
bigtable", you don't need parallel query in the first place, because a
single backend is quite capable of sending back the rows as fast as a
client can read them.  If you're running a query like "SELECT * FROM
bigtable WHERE <highly selective predicate>" then that's a good use
case for parallel query, but then materializing it isn't that bad
because the result set is a lot smaller than the original table.

I am not disputing the idea that there are *some* cases where parallel
query is useful and materialization is still undesirable, of course.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Nov 2, 2017 at 11:35 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Nov 2, 2017 at 8:01 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
>> That forces materialization, and I'm guessing part of Tomas's goal
>> here is to prevent the need to materialize into a temp table /
>> tuplestore / etc.
>
> I get that, but if you're running a query like "SELECT * FROM
> bigtable", you don't need parallel query in the first place, because a
> single backend is quite capable of sending back the rows as fast as a
> client can read them.  If you're running a query like "SELECT * FROM
> bigtable WHERE <highly selective predicate>" then that's a good use
> case for parallel query, but then materializing it isn't that bad
> because the result set is a lot smaller than the original table.
>
> I am not disputing the idea that there are *some* cases where parallel
> query is useful and materialization is still undesirable, of course.

Not seeing a code-level review of the proposed patch, I am moving it
to next CF to let the discussion move on. Nobody has registered as
reviewer yet.
-- 
Michael

On 17 October 2017 at 14:16, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sat, Oct 14, 2017 at 2:14 PM, Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
>> I propose is to add a new cursor option (PARALLEL), which would allow
>> parallel plans for that particular user-defined cursor. Attached is an
>> experimental patch doing this (I'm sure there are some loose ends).
>
> I think you would need to do a huge amount of additional work in order
> to actually make this robust.  I believe that a fair amount of what
> goes on in parallel mode right now is checked with elog() if we think
> that it's unreachable without writing C code -- but this will make a
> lot of those things reachable, which means they would need to be
> checked some other way.  Also, I doubt this guarantees that we won't
> try to call parallel-unsafe functions will parallel mode is active, so
> things will just blow up in whatever way they do, maybe crashing the
> server or rendering the database inconsistent or whatever.
>
> Possibly I'm overestimating the extent of the danger, but I don't
> think so.  You're try to take a mechanism that was only ever meant to
> be active during the course of one query and applying it for long
> periods of time during which a user can do anything, with basically no
> upgrade of the infrastructure.  I think something like this could be
> made to work if you put a large amount of energy into it, but I think
> the patch as proposed is about the easiest 3-5% of what would actually
> be required to cover all the holes.

Maybe so.


At present, one major use of Cursors is in postgres_fdw.

In that usage, the query executes and returns all the rows. No other
side execution is possible.

How do we make parallel query work for Cursors, if not by Tomas' proposal?

What more restrictive proposal would you prefer?

-- 
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2 November 2017 at 01:55, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Nov 1, 2017 at 7:49 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
>> If the client wants to fetch in chunks it can use a portal and limited
>> size fetches. That shouldn't (?) be parallel-unsafe, since nothing
>> else can happen in the middle anyway.
>
> I believe sending a limited-size fetch forces serial execution
> currently.  If it's true that nothing else can happen in the middle
> then we could relax that, but I don't see why that should be true?

Perhaps the question is not "why" but "when"?

If a parallel cursor is requested, we could simply prevent other
intermediate commands other than FETCH (next).

-- 
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Jan 17, 2018 at 8:53 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> At present, one major use of Cursors is in postgres_fdw.
>
> In that usage, the query executes and returns all the rows. No other
> side execution is possible.

True, although foreign tables using postgres_fdw can't be scanned in
parallel for other reasons anyway -- it's the user backend, not any
workers we might spin up, that has the connection to the remote
server.  Also, I'm talking about the external-facing use of cursors by
users, not the fact that postgres_fdw uses them internally to talk to
other machines.

> How do we make parallel query work for Cursors, if not by Tomas' proposal?
>
> What more restrictive proposal would you prefer?

In all honestly, if I knew how to fix this problem with a reasonable
amount of work, I would have done it already.  It's a very hard
problem.

One idea I've thought about is providing some kind of infrastructure
for workers to detach from a parallel query.  This could be useful in
a variety of situations, including cursors.  Let's say the leader is
either (a) suspending execution of the query, because it's a cursor,
or (b not able to absorb rows as fast as workers are generating them.
In the former situation, we'd like to get rid of all workers; in the
latter situation, some workers.  In the former situation, getting all
workers to shut down cleanly would let us exit parallel mode (and
perhaps re-enter it later when we resume execution of the query).  In
the latter situation, we could avoid wasting workers on queries where
the leader can't keep up so that those worker slots are available to
other queries that can benefit from them.

However, this is not simple.  In the case of a parallel sequential
scan, once a worker claims a page, it must scan all the tuples on that
page.  No other backend will ever get that page, and therefore if the
backend that did claim it doesn't scan the whole thing, the query may
return the wrong answer.  Every other parallel-aware executor node we
have has similar problems: there are points where we could safely stop
without changing the final answer to the query, and other points where
it is not safe to stop.  One idea is to provide a new callback for
parallel-aware executor nodes that tells them to stop returning tuples
at the next safe stop point.  When we want to get rid of workers, we
somehow signal them to invoke this method on every node in the plan
tree; at some point, they'll drop off but the query will still return
the right answer.  In the worst case, however, the worker might emit
an arbitrarily large number of tuples before reaching a safe stop
point and exiting (imagine a parallel-sequential scan cross-joined to
generate_series(1, 100000000000) or the equivalent), which seems
pretty crappy.

Or we could go the other way and try to keep the workers running.  I
don't really like that because it ties down those workers for
potentially a long period of time, but that might be acceptable for
some users.  The main implementation problem is that somehow we'd need
to propagate to them an updated version of any state that has changed
while the query was suspended, such as new combo CIDs that have been
created in the meantime.  dshash seems like a useful tool toward such
a goal, but there are details to work out, and there are similar
problems with everything else that is copied from leader to workers.
We could possibly prevent these problems from arising by placing
draconian restrictions on what a backend is allowed to do while a
parallel cursor is open, such as in your follow-on proposal to lock
out everything except FETCH.  I'm not really that excited about such a
thing because it's extremely limiting and still doesn't solve all the
problems: in particular, after BEGIN ... DECLARE CURSOR PARALLEL ...
FETCH ... FETCH ... syntax error, there is going to be trouble around
the state of group locking.  It will be very bad if the workers think
the transaction is still alive and the leader thinks it is in a new
transaction and they're all sharing locks.  You also have to worry
about what things can be accomplished by protocol messages, not just
what can be done via SQL.  But it's probably feasible with enough
work.

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

On Thu, Jan 18, 2018 at 12:59 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Jan 17, 2018 at 8:53 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>
> Or we could go the other way and try to keep the workers running.  I
> don't really like that because it ties down those workers for
> potentially a long period of time, but that might be acceptable for
> some users.  The main implementation problem is that somehow we'd need
> to propagate to them an updated version of any state that has changed
> while the query was suspended, such as new combo CIDs that have been
> created in the meantime.  dshash seems like a useful tool toward such
> a goal, but there are details to work out, and there are similar
> problems with everything else that is copied from leader to workers.
> We could possibly prevent these problems from arising by placing
> draconian restrictions on what a backend is allowed to do while a
> parallel cursor is open, such as in your follow-on proposal to lock
> out everything except FETCH.  I'm not really that excited about such a
> thing because it's extremely limiting and still doesn't solve all the
> problems: in particular, after BEGIN ... DECLARE CURSOR PARALLEL ...
> FETCH ... FETCH ... syntax error, there is going to be trouble around
> the state of group locking.  It will be very bad if the workers think
> the transaction is still alive and the leader thinks it is in a new
> transaction and they're all sharing locks.
>

On error, workers should be terminated.  What kind of problem do you
have in mind?

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

Hi Tomas,

On 1/22/18 7:05 AM, Amit Kapila wrote:
> On Thu, Jan 18, 2018 at 12:59 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Wed, Jan 17, 2018 at 8:53 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>
>> Or we could go the other way and try to keep the workers running.  I
>> don't really like that because it ties down those workers for
>> potentially a long period of time, but that might be acceptable for
>> some users.  The main implementation problem is that somehow we'd need
>> to propagate to them an updated version of any state that has changed
>> while the query was suspended, such as new combo CIDs that have been
>> created in the meantime.  dshash seems like a useful tool toward such
>> a goal, but there are details to work out, and there are similar
>> problems with everything else that is copied from leader to workers.
>> We could possibly prevent these problems from arising by placing
>> draconian restrictions on what a backend is allowed to do while a
>> parallel cursor is open, such as in your follow-on proposal to lock
>> out everything except FETCH.  I'm not really that excited about such a
>> thing because it's extremely limiting and still doesn't solve all the
>> problems: in particular, after BEGIN ... DECLARE CURSOR PARALLEL ...
>> FETCH ... FETCH ... syntax error, there is going to be trouble around
>> the state of group locking.  It will be very bad if the workers think
>> the transaction is still alive and the leader thinks it is in a new
>> transaction and they're all sharing locks.
>>
> 
> On error, workers should be terminated.  What kind of problem do you
> have in mind?

Since there appears to be an ongoing discussion about how this patch
should work I am marking it Returned with Feedback for this CF.

Regards,
-- 
-David
david@pgmasters.net

On Mon, Jan 22, 2018 at 7:05 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On error, workers should be terminated.  What kind of problem do you
> have in mind?

Hmm.  Yeah, I guess that makes sense.  If the only thing you can do is
fetch from the cursor -- and you have to make sure to lock down
protocol messages as well as SQL commands -- and if any error kills
the workers -- then I guess this might be workable.  However, I think
there are still problems.  Say the worker hits an error while the
leader is idle; how do we report the error?  It's a protocol version
for the leader to send an unsolicited ErrorResponse, which is why we
have to use FATAL rather than ERROR in various places for recovery
conflicts, query cancellation, etc.

Also, if you're OK with not being able to do anything except fetch
from the cursor until you reach the end, then couldn't you just issue
the query without the cursor and use PQsetSingleRowMode?

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

On Wed, Feb 7, 2018 at 9:47 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Jan 22, 2018 at 7:05 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> On error, workers should be terminated.  What kind of problem do you
>> have in mind?
>
> Hmm.  Yeah, I guess that makes sense.  If the only thing you can do is
> fetch from the cursor -- and you have to make sure to lock down
> protocol messages as well as SQL commands -- and if any error kills
> the workers -- then I guess this might be workable.  However, I think
> there are still problems.  Say the worker hits an error while the
> leader is idle; how do we report the error?
>

I guess workers need to wait till leader become active and processes
the error message.

>  It's a protocol version
> for the leader to send an unsolicited ErrorResponse, which is why we
> have to use FATAL rather than ERROR in various places for recovery
> conflicts, query cancellation, etc.
>



> Also, if you're OK with not being able to do anything except fetch
> from the cursor until you reach the end, then couldn't you just issue
> the query without the cursor and use PQsetSingleRowMode?
>

I think there is a lot of cursor usage via plpgsql in which it could be useful.


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

On Thu, Feb 8, 2018 at 9:04 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> I guess workers need to wait till leader become active and processes
> the error message.

So if you kill a worker, it doesn't die but sits there waiting for
someone to run a command in the leader?  That sounds terrible.

>> Also, if you're OK with not being able to do anything except fetch
>> from the cursor until you reach the end, then couldn't you just issue
>> the query without the cursor and use PQsetSingleRowMode?
>
> I think there is a lot of cursor usage via plpgsql in which it could be useful.

I don't see how.  If you can't do anything but fetch from the cursor
while the cursor is open, then you can't go start doing things in
plpgsql code either.  If you allow plpgsql code to run during that
time, then it can do arbitrary stuff which breaks the design
assumptions around parallel mode, like calling parallel-unsafe
functions or trying to do transaction control or running DML.

The whole point here is that the parallel-mode stuff is only designed
to cleanly error out in cases that we think we can hit during the
course of executing a query.  In other cases, it may elog(), Assert(),
crash, give wrong answers, etc.  As soon as you let parallel-mode
survive beyond the context of a query, you've opened a huge can of
worms which you will not be easily able to shut.  If we restrict what
you can do outside of a query to a very narrow set of operations, then
it might survive, but "execute arbitrary plpgsql code" is not a narrow
set of operations.

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

On Thu, Feb 8, 2018 at 7:49 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Feb 8, 2018 at 9:04 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>> Also, if you're OK with not being able to do anything except fetch
>>> from the cursor until you reach the end, then couldn't you just issue
>>> the query without the cursor and use PQsetSingleRowMode?
>>
>> I think there is a lot of cursor usage via plpgsql in which it could be useful.
>
> I don't see how.  If you can't do anything but fetch from the cursor
> while the cursor is open, then you can't go start doing things in
> plpgsql code either.
>

I am not saying to allow other things.  I am just replying to your
question that why can't we use PQsetSingleRowMode.  I mean to say that
one can fetch the data parallelly via the usage of cursors (by having
restrictions like don't allow other parallel unsafe statements) in
plpgsql.

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

On Fri, Feb 9, 2018 at 7:53 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> I am not saying to allow other things.  I am just replying to your
> question that why can't we use PQsetSingleRowMode.  I mean to say that
> one can fetch the data parallelly via the usage of cursors (by having
> restrictions like don't allow other parallel unsafe statements) in
> plpgsql.

I'm pretty sure that other parallel-unsafe statements is an
insufficient restriction.  I think we're just going around in circles
here.

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