Re: [HACKERS] Proposed change to make cancellations safe

[Tom Lane]

Robert Haas  writes:
> On Mon, Apr 25, 2016 at 12:16 PM, Shay Rojansky  wrote:
>> Sure. I'd consider sending in a patch, but as this is a protocol-changing
>> feature it seems like working on this before the team "officially" starts
>> working on a new protocol might be a waste of time. Once there's critical
>> mass for a new protocol and agreement that PostgreSQL is going for it I'd be
>> happy to work on it.

> I don't immediately see a reason why this couldn't be done as an
> isolated change.  Suppose that we change the server to allow a cancel
> request to be either 16 bytes or 20 bytes, rather than always 16 bytes
> as they are currently.  Clients will need to be careful not to send
> the new type of cancel request to a server that is too old to
> understand it, but since they've got an active connection,
> server_version will have been previously reported.

> More generally, as long as new protocol bits are client-initiated, I
> don't think we really need to bump the protocol version.  If we want
> to change the kinds of responses the server sends or structurally
> change the format of protocol messages or deprecate messages that
> exist now, then we do.

Meh --- I'm fairly suspicious of shoehorning things in and pretending
it's not a protocol change.  That usually leads to crufty and ultimately
unmaintainable designs, because you're forced to do strange things when
you do it that way.  (Cf the COPY RAW thread for a recent example.)

Having said that, CANCEL is sufficiently outside the normal protocol
that maybe you are right: we could invent what amounts to a new cancel
protocol and trust clients to look at server_version to know what to send.

One problem is that we really ought to widen the random cancel key while
we're at it; 32 bits doesn't seem like enough to prevent brute-force
searches anymore.  However, since the cancel key is transmitted from the
server within the normal protocol, I don't see any way to do that without
a compatibility break.

Is there anything else people have complained about w.r.t. CANCEL?

regards, tom lane


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

Re: [HACKERS] Proposed change to make cancellations safe

[Robert Haas]

On Mon, Apr 25, 2016 at 12:16 PM, Shay Rojansky  wrote:
> Sure. I'd consider sending in a patch, but as this is a protocol-changing
> feature it seems like working on this before the team "officially" starts
> working on a new protocol might be a waste of time. Once there's critical
> mass for a new protocol and agreement that PostgreSQL is going for it I'd be
> happy to work on it.

I don't immediately see a reason why this couldn't be done as an
isolated change.  Suppose that we change the server to allow a cancel
request to be either 16 bytes or 20 bytes, rather than always 16 bytes
as they are currently.  Clients will need to be careful not to send
the new type of cancel request to a server that is too old to
understand it, but since they've got an active connection,
server_version will have been previously reported.

More generally, as long as new protocol bits are client-initiated, I
don't think we really need to bump the protocol version.  If we want
to change the kinds of responses the server sends or structurally
change the format of protocol messages or deprecate messages that
exist now, then we do.

-- 
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

Re: [HACKERS] Proposed change to make cancellations safe

[Tom Lane]

Shay Rojansky  writes:
>> We really do need "cancel up to" semantics for reliable behavior.
>> Consider the case where the client has sent the query (or thinks it has)
>> but the server hasn't received it yet.  If the cancel request can arrive
>> at the server before the query fully arrives, and we don't have "cancel
>> all messages up through N" semantics, the cancel will not do what the
>> client expects it to.

> Keep in mind that in the case of a cancellation arriving really too early,
> i.e. before any messages have been received by the server, it will be
> totally ignored since at the time of reception there's nothing for the
> server to cancel yet.

Right, that's how it works today ...

> This may seem a bit exotic, although if you really
> want to provide air-tight cancellation semantics you could have the server
> track unfulfilled cancellation requests. In other words, if the server
> receives "cancel up to X" and is now processing X-5, the cancellation
> request is kept in memory until X has been duly cancelled.

Exactly my point.  If we're trying to make cancel semantics less squishy,
I think we need to do that; errors in this direction are just as bad as
the late-cancel-arrival case.

regards, tom lane


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

Re: [HACKERS] Proposed change to make cancellations safe

[Shay Rojansky]

>
> We really do need "cancel up to" semantics for reliable behavior.
> Consider the case where the client has sent the query (or thinks it has)
> but the server hasn't received it yet.  If the cancel request can arrive
> at the server before the query fully arrives, and we don't have "cancel
> all messages up through N" semantics, the cancel will not do what the
> client expects it to.
>

Keep in mind that in the case of a cancellation arriving really too early,
i.e. before any messages have been received by the server, it will be
totally ignored since at the time of reception there's nothing for the
server to cancel yet. This may seem a bit exotic, although if you really
want to provide air-tight cancellation semantics you could have the server
track unfulfilled cancellation requests. In other words, if the server
receives "cancel up to X" and is now processing X-5, the cancellation
request is kept in memory until X has been duly cancelled.

Re: [HACKERS] Proposed change to make cancellations safe

[Alvaro Herrera]

Shay Rojansky wrote:

> BTW it seems it's no longer possible for anyone to add things to the TODO
> (no editor privileges).

You need to ask for editor privs on pgsql-www.  Make sure to mention
your community user name.

-- 
Álvaro Herrerahttp://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

Re: [HACKERS] Proposed change to make cancellations safe

[Tom Lane]

Shay Rojansky  writes:
>> Issuing bulk cancellations sounds like a bad plan.

> I'm not sure why, but at the very least it's a useful thing to have when
> batching several statements together and then wanting to cancel them all.

We really do need "cancel up to" semantics for reliable behavior.
Consider the case where the client has sent the query (or thinks it has)
but the server hasn't received it yet.  If the cancel request can arrive
at the server before the query fully arrives, and we don't have "cancel
all messages up through N" semantics, the cancel will not do what the
client expects it to.

regards, tom lane


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

Re: [HACKERS] Proposed change to make cancellations safe

[Shay Rojansky]

>
> I definitely agree that simply tracking message sequence numbers on both
>> sides is better. It's also a powerful feature to be able to cancel all
>> messages "up to N" - I'm thinking of a scenario where, for example, many
>> simple queries are sent and the whole process needs to be cancelled.
>>
>
> For security, I think any non-matching cancellation would be ignored so
> that only someone with proper context could issue the cancel.
>

Isn't security taken care of by the secret key?


> Issuing bulk cancellations sounds like a bad plan.
>

I'm not sure why, but at the very least it's a useful thing to have when
batching several statements together and then wanting to cancel them all.


> Yes, this has been happening to some Npgsql users, it's not very frequent
>> but it does happen from time to time. I also bumped into this in some
>> automated testing scenarios. It's not the highest-value feature, but it is
>> an improvement to have if you plan on working on a new protocol version.
>>
>> Let me know if you'd like me to update the TODO.
>>
>
> If you've got an itch, expecting someone else to scratch it is less likely
> to succeed.
>

Sure. I'd consider sending in a patch, but as this is a protocol-changing
feature it seems like working on this before the team "officially" starts
working on a new protocol might be a waste of time. Once there's critical
mass for a new protocol and agreement that PostgreSQL is going for it I'd
be happy to work on it.

BTW it seems it's no longer possible for anyone to add things to the TODO
(no editor privileges).

Re: [HACKERS] Proposed change to make cancellations safe

[Craig Ringer]

On 24 April 2016 at 23:11, Tom Lane  wrote:


> Have you seen this to be a problem in practice, or is it just
> theoretical?  I do not recall many, if any, field complaints
> about the issue.
>

It's caused pain for me when working with JDBC in the past.

If libpq gets pipelined query support in future it'll become much more
noticeable. Right now libpq won't be too bothered.

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

Re: [HACKERS] Proposed change to make cancellations safe

[Tom Lane]

Simon Riggs  writes:
> On 24 April 2016 at 17:54, Shay Rojansky  wrote:
>> I definitely agree that simply tracking message sequence numbers on both
>> sides is better. It's also a powerful feature to be able to cancel all
>> messages "up to N" - I'm thinking of a scenario where, for example, many
>> simple queries are sent and the whole process needs to be cancelled.

> For security, I think any non-matching cancellation would be ignored so
> that only someone with proper context could issue the cancel.

Well, we already have a random cancel key in the requests.  As a separate
matter for a protocol change, it might be nice to consider widening the
cancel key to make it harder to brute-force; but I disagree that the
current proposal has anything whatever to do with security.

regards, tom lane


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

Re: [HACKERS] Proposed change to make cancellations safe

[Simon Riggs]

On 24 April 2016 at 17:54, Shay Rojansky  wrote:


> I definitely agree that simply tracking message sequence numbers on both
> sides is better. It's also a powerful feature to be able to cancel all
> messages "up to N" - I'm thinking of a scenario where, for example, many
> simple queries are sent and the whole process needs to be cancelled.
>

For security, I think any non-matching cancellation would be ignored so
that only someone with proper context could issue the cancel.

Issuing bulk cancellations sounds like a bad plan.

Yes, this has been happening to some Npgsql users, it's not very frequent
> but it does happen from time to time. I also bumped into this in some
> automated testing scenarios. It's not the highest-value feature, but it is
> an improvement to have if you plan on working on a new protocol version.
>
> Let me know if you'd like me to update the TODO.
>

If you've got an itch, expecting someone else to scratch it is less likely
to succeed.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Proposed change to make cancellations safe

[Shay Rojansky]

I definitely agree that simply tracking message sequence numbers on both
sides is better. It's also a powerful feature to be able to cancel all
messages "up to N" - I'm thinking of a scenario where, for example, many
simple queries are sent and the whole process needs to be cancelled.

Yes, this has been happening to some Npgsql users, it's not very frequent
but it does happen from time to time. I also bumped into this in some
automated testing scenarios. It's not the highest-value feature, but it is
an improvement to have if you plan on working on a new protocol version.

Let me know if you'd like me to update the TODO.

On Sun, Apr 24, 2016 at 6:11 PM, Tom Lane  wrote:

> Shay Rojansky  writes:
> > The issue I'd like to tackle is the fact that it's not possible to make
> > sure a cancellation request affects a specific query.
>
> Right ...
>
> > A simple fix for this would be to have a sequence number returned in the
> > BindComplete message.
>
> First, that doesn't fix anything for simple query protocol, which doesn't
> use such a message.
>
> Second, making sure that the BindComplete gets delivered soon enough to be
> useful would require issuing a Sync between Bind and Execute.  At the
> least that doubles the number of network packets for small query results
> (so I dispute your assumption that the added network load would be
> negligible).  But a bigger issue is that it would have subtle effects
> on error handling.  An application could not blindly fire out
> Parse/Bind/Sync/Execute/Sync as one packet; it would have to wait for
> the Bind result to come back before it could know if it's safe to send
> Execute, unlike the case where it sends Parse/Bind/Execute/Sync.  (The
> server's skip-to-Sync-after-error rule is critical here.)  So actually,
> making use of this feature would double the number of network round trips,
> as well as requiring carefully thought-through changes to application
> error handling.
>
> Third, if a query is not cancellable till Execute, that prevents
> cancellation of scenarios where the parser or planner takes a long time
> for some reason.  Long planner runtimes are certainly not uncommon.
>
>
> So this is worth thinking about, but that doesn't sound like much of
> a solution to me.
>
> I wonder though why we need the server to tell the client a sequence
> number at all.  Surely both sides of the connection could easily count
> the number of messages the client has transmitted.  We could simply extend
> the cancel packet to include a sequence number field, with the semantics
> "this should only take effect if you're still working on message N".
> Or I guess maybe it should read "working on message N or before", so
> that in a case like Parse/Bind/Execute/Sync you would mention the number
> of the Execute message and still get cancellation if the query was hung
> up in Parse.
>
> Have you seen this to be a problem in practice, or is it just
> theoretical?  I do not recall many, if any, field complaints
> about the issue.
>
> regards, tom lane
>

Re: [HACKERS] Proposed change to make cancellations safe

[Simon Riggs]

On 24 April 2016 at 13:04, Shay Rojansky  wrote:

The issue I'd like to tackle is the fact that it's not possible to make
> sure a cancellation request affects a specific query. Since cancellations
> are totally asynchronous and occur on another socket, they affect whatever
> query is currently being processed. This makes it possible to inadvertently
> kill a later query, because by the time the cancellation request is
> delivered and "applied" an the intended query already completed and a later
> one gets killed.
>
> A simple fix for this would be to have a sequence number returned in the
> BindComplete message. When submitting a cancellation request, the frontend
> would have the option (but not the obligation) to pass such as sequence
> number to the backend. When cancelling, the backend would make sure the
> sequence number corresponds to the currently running query.
>
> The only drawback seems to be the increased payload of the BindComplete
> message (4 bytes, or possibly less if we're really worried about it).
>
> If this makes sense, I'll add it to the TODO wiki.
>

The topic makes sense, an alternate solution may be better.

If things are sequential, both sides can +1 as appropriate. So an extra
message isn't required to confirm that. We can resync with optional
additional info at various points. Requesting cancellation of a former
sequence number can still be blocked.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Proposed change to make cancellations safe

[Tom Lane]

Shay Rojansky  writes:
> The issue I'd like to tackle is the fact that it's not possible to make
> sure a cancellation request affects a specific query.

Right ...

> A simple fix for this would be to have a sequence number returned in the
> BindComplete message.

First, that doesn't fix anything for simple query protocol, which doesn't
use such a message.

Second, making sure that the BindComplete gets delivered soon enough to be
useful would require issuing a Sync between Bind and Execute.  At the
least that doubles the number of network packets for small query results
(so I dispute your assumption that the added network load would be
negligible).  But a bigger issue is that it would have subtle effects
on error handling.  An application could not blindly fire out
Parse/Bind/Sync/Execute/Sync as one packet; it would have to wait for
the Bind result to come back before it could know if it's safe to send
Execute, unlike the case where it sends Parse/Bind/Execute/Sync.  (The
server's skip-to-Sync-after-error rule is critical here.)  So actually,
making use of this feature would double the number of network round trips,
as well as requiring carefully thought-through changes to application
error handling.

Third, if a query is not cancellable till Execute, that prevents
cancellation of scenarios where the parser or planner takes a long time
for some reason.  Long planner runtimes are certainly not uncommon.


So this is worth thinking about, but that doesn't sound like much of
a solution to me.

I wonder though why we need the server to tell the client a sequence
number at all.  Surely both sides of the connection could easily count
the number of messages the client has transmitted.  We could simply extend
the cancel packet to include a sequence number field, with the semantics
"this should only take effect if you're still working on message N".
Or I guess maybe it should read "working on message N or before", so
that in a case like Parse/Bind/Execute/Sync you would mention the number
of the Execute message and still get cancellation if the query was hung
up in Parse.

Have you seen this to be a problem in practice, or is it just
theoretical?  I do not recall many, if any, field complaints
about the issue.

regards, tom lane


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