From: Jelte Fennema <Jelte.Fennema@microsoft.com>
To: Tom Lane <tgl@sss.pgh.pa.us>, Robert Haas <robertmhaas@gmail.com>
CC: Andres Freund <andres@anarazel.de>, Jacob Champion <pchampion@vmware.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: [EXTERNAL] Re: Add non-blocking version of PQcancel
Thread-Topic: [EXTERNAL] Re: Add non-blocking version of PQcancel
Thread-Index: 
 AQHYCI0SwADhK3qLxkSGSoq0ynWo4ay2h+AAgBj2+YCAABLLgIABSy8AgAADfYCAAAoEAIAAA1sAgAPwyy0=
Date: Mon, 28 Mar 2022 09:28:19 +0000
Message-ID: 
 <HE1PR8303MB00732AF8BEF2E45DACFE6633F71D9@HE1PR8303MB0073.EURPRD83.prod.outlook.com>
References: 
 <AM5PR83MB0178D3B31CA1B6EC4A8ECC42F7529@AM5PR83MB0178.EURPRD83.prod.outlook.com>
 <20220113004411.3tdgiqdmmo4musb5@alap3.anarazel.de>
 <AM5PR83MB0178CE0CEB690F1C041355B7F7539@AM5PR83MB0178.EURPRD83.prod.outlook.com>
 <b6e02c2ef43a4c8dcbca03dd677ae8331c892175.camel@vmware.com>
 <3150365.1648158113@sss.pgh.pa.us>
 <20220324224909.4tnubqv2owmfoyoc@alap3.anarazel.de>
 <CA+Tgmoav-v8HVuA-kLJM3Kf6XHtVfOPajQtyAkb5ejs7C_Aw2A@mail.gmail.com>
 <3453535.1648234019@sss.pgh.pa.us>
 <CA+TgmoZUb5yEFNi7pLpoqoOxfwyrCYfczjxeqYor21Cy+Ozasg@mail.gmail.com>
 <3457746.1648236890@sss.pgh.pa.us>
In-Reply-To: <3457746.1648236890@sss.pgh.pa.us>
Accept-Language: en-US
Content-Language: en-US
msip_labels: 
 MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Enabled=True;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SiteId=72f988bf-86f1-41af-91ab-2d7cd011db47;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SetDate=2022-03-28T09:28:18.259Z;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Name=General;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_ContentBits=0;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Method=Standard;
authentication-results: dkim=none (message not signed)
 header.d=none;dmarc=none action=none header.from=microsoft.com;
x-ms-publictraffictype: Email
x-ms-office365-filtering-correlation-id: 49a0b1bf-1844-4e7d-9887-08da109d4c13
x-ms-traffictypediagnostic: AM6PR83MB0230:EE_
x-ms-exchange-atpmessageproperties: SA|SL
x-microsoft-antispam-prvs: 
 <AM6PR83MB02300BE4A3CAA573B62EED23F71D9@AM6PR83MB0230.EURPRD83.prod.outlook.com>
x-ms-exchange-senderadcheck: 1
x-ms-exchange-antispam-relay: 0
x-microsoft-antispam: BCL:0;
x-microsoft-antispam-message-info: 
 VakInEigfD5k4ccjPkkdSzkFX0dAhefvQdeanjt8tIxe4SIBIgX9ks33BaxneP6ib2zkGWpP9f/sdOh+WmbbPCXuOB8CS2Dysk1TdwWz76LAhqwPstydtxA7rXDAekqLTAeoPAPecyZjhtsX0B448Zrjjx8PuTyGt/cpvCBQRky92P2Jm2rzGWJy9PJtgG/sC/UHd7GTAWhwsuQ5bDd4kKkl2ytkPs8NWFG0HQ+faStM5cCxNF6KZUnbUkzoR0iDHg1W9RcYadEPQduKqW23BDirfKTgkg9H8Eo7NkuqBhi8yr2PJ+XXHTy8M06H69uh887seBM17ge0+WyCi28SGQdQe/VF73VnAUGz/GARgqXvpyJvb/8EnDw/hym/TKgd1T3aAIp7hpH/c3jk+0ZIdknWUJqnc2/69EX/k3uLn0nxmUHOfXQ3Immz4YMgOSbNW6p/4iH//yNGBd/BGoZIf2sRocg9Toflc8HbF7b9NaX0y5Sq7qzgpR30WlokT0kM98KW14zqyyK73+jKrf8IZWUPp0jxc2RKwuw4XpVLU0pgbTueA+WSCRGU8DYUgqfuOzbIRgjRJjoz+Dm556U2oSESLLCDpLRlqQyUhH07MJFfTZcZ9BHcFskfmPus3SaIO5s6+ddLgdGB4WcwyAM5E50BdWrMeqNADWBP2ekoJH1CrZmW7qNJGxF4oXMoUazKxBnaDzv8th2qZ96gUE9uU96OeIFzqXpw/k4Zl61K8wymnikdeRpWsJLxm6opI1XF
x-forefront-antispam-report: 
 CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:HE1PR8303MB0073.EURPRD83.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230001)(4636009)(366004)(451199009)(110136005)(6506007)(7696005)(54906003)(316002)(9686003)(26005)(186003)(55016003)(71200400001)(33656002)(508600001)(38070700005)(2906002)(8990500004)(86362001)(76116006)(91956017)(66446008)(64756008)(66946007)(4326008)(66556008)(66476007)(8676002)(83380400001)(38100700002)(10290500003)(82960400001)(122000001)(52536014)(5660300002)(8936002)(82950400001);DIR:OUT;SFP:1102;
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: 
 =?iso-8859-1?Q?F6DH9Pk70BGwBco1RI0IKb0nYOWZl3OtPLIILzVmf7MtD/YkH1N29RCjlm?=
 =?iso-8859-1?Q?oEBNkKmycFf7LGYkKViww2vUKZECxJTum5Fil9NaNgh88L8SI0RxzOS8FL?=
 =?iso-8859-1?Q?ejkug4V2yWQOLEKnPENUNDxg6MDz4L/QfGOqztbdubvqcs2O0fPHfiBXeq?=
 =?iso-8859-1?Q?9GvAtFW5yZUbOJ5MwupetiO/yTLAaxfJdaiNPRfZsN1iWO9mpqgNmLWfiD?=
 =?iso-8859-1?Q?MI0hiRSEf12hmmHRjEHyqHNohqcjzDAwi94rr6DhbWCucTYe25NfUO260O?=
 =?iso-8859-1?Q?u4Le/rGvCdgY/+sR67DSSOB8+HKvoCzkvs+2jHeYyhTGuJ18HmDSpD/nxq?=
 =?iso-8859-1?Q?qnwEcHrEOQKLNkbQcI4vwh3IfIlShQzKpMbYwFQv+PsljmoiLALs/rcD1x?=
 =?iso-8859-1?Q?LupJZ8bZoe5jG9fc0E1+HO85L1K9csC2b2sVDJ7RH2pomcML1Rch+YKMbP?=
 =?iso-8859-1?Q?TJeLUDtEk4STwsmrVPbsT5MidB7Nz+sNco6irsD23zUIIXRjrLEkXTPTHl?=
 =?iso-8859-1?Q?XD1h5LMBJdMntEfYTCNftzHQgLbgVcDzwah6FX3UfwsJ5eDmxn6cNAEhi1?=
 =?iso-8859-1?Q?6VBX1qJKyR/h+Ozvnr1aRMmJhANMReqTBL+tUPTQe8PCqcBRGDhRwLqLrb?=
 =?iso-8859-1?Q?/wzLmXl09yd/SOOUiNDXp3FZnpPDC+ZIOqQlwKowBSVg4F9AYaC0fS9+Y/?=
 =?iso-8859-1?Q?TEoMINrr39kRg9efvb8nrQ1rnLZCBseU9UvQ1nnQUMDSgbNe++lPUVM+D2?=
 =?iso-8859-1?Q?vqdkveLLUwQdBP/yLkvwIVKfvefdPIsER5iOjQZYXywbHKTuUisu7zIJSQ?=
 =?iso-8859-1?Q?l36oixQ15KopWEbzgmZn4ZLkmU6JQmS2PyyBAbbvokwAxjYZhF72BML62T?=
 =?iso-8859-1?Q?xacNZHfna/q28aAbUV2/fvGDrY991g61Mo2a8GeU27Rp+WmvXcwTp9w5Yo?=
 =?iso-8859-1?Q?B55RwJXxTnh3Gv3+SBrLokJw2AHjOxhCtNzszMZNfQq+nIv9scCN3+eBT5?=
 =?iso-8859-1?Q?ddmiFBCwzvYHABJvaYluciaVB9gBl+1LQpAzzTXFADNmxYXt8t0U5S2cWh?=
 =?iso-8859-1?Q?16Pkv91j93xtqXFtiUtKMYX2ZKBxNbsCId7i0oZ43rrA866+btbbU4MejC?=
 =?iso-8859-1?Q?pSUGgGRvqxkL/X6ePJRXxIfJHmLhzp76ytFFVdgAz6HXPU76jxqJssFsV6?=
 =?iso-8859-1?Q?g5IcN9NILyyPmQXxH3gLpLU39h/3UrQ+cDDnx23ljSGUlr7P8caOAVIkYx?=
 =?iso-8859-1?Q?6mEMD4/tnLcH0VQcy7HIJc0Sw/KC0NCRRllbaRg8cq/PphDGnFQE9YdOcp?=
 =?iso-8859-1?Q?eBsXm4y2VeecmeaDFVL9fYGcOIJRdZwl4kSO//02lvEvR2e/rdsTpdrqZ8?=
 =?iso-8859-1?Q?j+bcEvG+VdfekODOXk8Os/IkpJwYV54yxtSesWoE108m5SrBpdLdzeUQEk?=
 =?iso-8859-1?Q?ktp5ZeX5YkkIXJ2vP91bOszmULdpp0SR+z8ZcvVUdo1kW13LJmZ+/Hzko2?=
 =?iso-8859-1?Q?Z6pIj9lgvnJYGaTtbUK9cUOnanEFyRiDjTTRftzC3abMxkZcgq0U9dovA1?=
 =?iso-8859-1?Q?j85ILp4ZeIDF1/luRLngSq1VY5ZWTMIk/LhqlDm/WvXkykWzjZNLz5uScC?=
 =?iso-8859-1?Q?7taZQrfyI2FingEYS6lcf3tvtY50bS9vynmmUYXBBoKJ1yNjuGVodvw13X?=
 =?iso-8859-1?Q?LeLUYRF9HN9ORRwkQnw+uvH1TEi7HOqS11srxSzahCY/mM3tjHvTUlAiHb?=
 =?iso-8859-1?Q?Tq4Ak2SVgNKP71ECSpHRZdaLIes5gA17jx5pXacsIkeXTf6BSosBWSprNH?=
 =?iso-8859-1?Q?33rkvu0eQQ=3D=3D?=
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: microsoft.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: 
 HE1PR8303MB0073.EURPRD83.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 
 49a0b1bf-1844-4e7d-9887-08da109d4c13
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Mar 2022 09:28:19.5859
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 
 kF4GXwSzxRTF9e8FWd0m9fas4OJ3XIpMSxj43IIX4pAHeAmiJ8JvLSNS69QwVskzZrd1bNr4I7HgYulQ7W5Lnw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR83MB0230
List-Id: <pgsql-hackers.lists.postgresql.org>
List-Help: <https://lists.postgresql.org/manage/>
List-Subscribe: <https://lists.postgresql.org/manage/>
List-Post: <mailto:pgsql-hackers@lists.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@lists.postgresql.org>
List-Archive: <https://www.postgresql.org/list/pgsql-hackers>
Archived-At: 
 <https://www.postgresql.org/message-id/HE1PR8303MB00732AF8BEF2E45DACFE6633F71D9%40HE1PR8303MB0073.EURPRD83.prod.outlook.com>
Precedence: bulk

Thanks for all the feedback everyone. I'll try to send a new patch=0A=
later this week that includes user facing docs and a simplified API.=0A=
For now a few responses:=0A=
=0A=
> Yeah.  We need to choose a name for these new function(s) that is=0A=
> sufficiently different from "PQcancel" that people won't expect them=0A=
> to behave exactly the same as that does.  I lack any good ideas about=0A=
> that, how about you?=0A=
=0A=
So I guess the names I proposed were not great, since everyone seems to be =
falling over them. =0A=
But I'd like to make my intention clear with the current naming. After this=
 patch there would be =0A=
four different APIs for starting a cancelation:=0A=
1. PQrequestCancel: deprecated+old, not signal-safe function for requesting=
 query cancellation, only uses a specific set of connection options=0A=
2. PQcancel: Cancel queries in a signal safe way, to be signal-safe it only=
 uses a limited set of connection options=0A=
3. PQcancelConnect: Cancel queries in a non-signal safe way that uses all c=
onnection options=0A=
4. PQcancelConnectStart: Cancel queries in a non-signal safe and non-blocki=
ng way that uses all connection options=0A=
=0A=
So the idea was that you should not look at PQcancelConnectStart as the non=
-blocking=0A=
version of PQcancel, but as the non-blocking version of PQcancelConnect. I'=
ll try to =0A=
think of some different names too, but IMHO these names could be acceptable=
=0A=
when their differences are addressed sufficiently in the documentation. =0A=
=0A=
One other approach to naming that comes to mind now is repurposing PQreques=
tCancel:=0A=
1. PQrequestCancel: Cancel queries in a non-signal safe way that uses all c=
onnection options=0A=
2. PQrequestCancelStart: Cancel queries in a non-signal safe and non-blocki=
ng way that uses all connection options=0A=
3. PQcancel: Cancel queries in a signal safe way, to be signal-safe it only=
 uses a limited set of connection options=0A=
=0A=
> I think it's probably a=0A=
> bad sign that this function is tinkering with logic in this sort of=0A=
> low-level function anyway. pqReadData() is a really general function=0A=
> that manages to work with non-blocking I/O already, so why does=0A=
> non-blocking query cancellation need to change its return values, or=0A=
> whether or not it drops data in certain cases?=0A=
=0A=
The reason for this low level change is that the cancellation part of the=
=0A=
Postgres protocol is following a different, much more simplistic design =0A=
than all the other parts. The client does not expect a response message bac=
k =0A=
from the server after sending the cancellation request. The expectation =0A=
is that the server signals completion by closing the connection, i.e. sendi=
ng EOF. =0A=
For all other parts of the protocol, connection termination should be initi=
ated=0A=
client side by sending a Terminate message. So the server closing (sending=
=0A=
EOF) is always unexpected and is thus currently considered an error by pqRe=
adData.=0A=
=0A=
But since this is not the case for the cancellation protocol, the result is=
=0A=
changed to -2 in case of EOF to make it possible to distinguish between=0A=
an EOF and an actual error.=0A=
=0A=
> And it updates that function to return -2 when the is=0A=
> ECONNRESET, which seems to fly in the face of the comment's idea that=0A=
> this is the "clean connection closure" case. =0A=
=0A=
The diff sadly does not include the very relevant comment right above these=
=0A=
lines. Pasting the whole case statement here to clear up this confusion:=0A=
=0A=
case SSL_ERROR_ZERO_RETURN:=0A=
=0A=
	/*=0A=
	 * Per OpenSSL documentation, this error code is only returned for=0A=
	 * a clean connection closure, so we should not report it as a=0A=
	 * server crash.=0A=
	 */=0A=
	appendPQExpBufferStr(&conn->errorMessage,=0A=
						 libpq_gettext("SSL connection has been closed unexpectedly\n"));=0A=
	result_errno =3D ECONNRESET;=0A=
	n =3D -2;=0A=
	break;=0A=
=0A=
=0A=
> For example, it updates the=0A=
> comment for pqsecure_read() to say "Returns -1 in case of failures,=0A=
> except in the case of clean connection closure then it returns -2."=0A=
> But that function calls any of three different implementation=0A=
> functions depending on the situation and the patch only updates one of=0A=
> them. =0A=
=0A=
That comment is indeed not describing what is happening correctly and I'll =
=0A=
try to make it clearer. The main reason for it being incorrect is coming fr=
om =0A=
the fact that receiving EOFs is handled in different places based on the =
=0A=
encryption method:=0A=
=0A=
1. Unencrypted TCP: EOF is not returned as an error by pqsecure_read, but d=
etected by pqReadData (see comments related to definitelyEOF)=0A=
2. OpenSSL: EOF is returned as an error by pqsecure_read (see copied case s=
tatement above)=0A=
3. GSS: When writing the patch I was not sure how EOF handling worked here,=
 but given that the tests passed for Jacob on GSS, I'm guessing it works th=
e same as unencrypted TCP.=0A=