Re: Add non-blocking version of PQcancel

On Thu, 2022-01-13 at 14:51 +0000, Jelte Fennema wrote:
> Attached is an updated patch which I believe fixes windows and the other test failures.
> At least on my machine make check-world passes now when compiled with --enable-tap-tests
>
> I also included a second patch which adds some basic documentation for the libpq tests.

This is not a full review by any means, but here are my thoughts so
far:

> NOTE: I have not tested this with GSS for the moment. My expectation is
> that using this new API with a GSS connection will result in a
> CONNECTION_BAD status when calling PQcancelStatus. The reason for this
> is that GSS reads will also need to communicate back that an EOF was
> found, just like I've done for TLS reads and unencrypted reads.

For what it's worth, I did a smoke test with a Kerberos environment via

./libpq_pipeline cancel '... gssencmode=require'

and the tests claim to pass.

> 2. Cancel connections benefit automatically from any improvements made
> to the normal connection establishment codepath. Examples of things
> that it currently gets for free currently are TLS support and
> keepalive settings.

This seems like a big change compared to PQcancel(); one that's not
really hinted at elsewhere. Having the async version of an API open up
a completely different code path with new features is pretty surprising
to me.

And does the backend actually handle cancel requests via TLS (or GSS)?
It didn't look that way from a quick scan, but I may have missed
something.

> @@ -1555,6 +1665,7 @@ print_test_list(void)
> printf("singlerow\n");
> printf("transaction\n");
> printf("uniqviol\n");
> + printf("cancel\n");
> }

This should probably go near the top; it looks like the existing list
is alphabetized.

The new cancel tests don't print any feedback. It'd be nice to get the
same sort of output as the other tests.

> /* issue a cancel request */
> extern int PQcancel(PGcancel *cancel, char *errbuf, int errbufsize);
> +extern PGcancelConn * PQcancelConnectStart(PGconn *conn);
> +extern PGcancelConn * PQcancelConnect(PGconn *conn);
> +extern PostgresPollingStatusType PQcancelConnectPoll(PGcancelConn * cancelConn);
> +extern ConnStatusType PQcancelStatus(const PGcancelConn * cancelConn);
> +extern int PQcancelSocket(const PGcancelConn * cancelConn);
> +extern char *PQcancelErrorMessage(const PGcancelConn * cancelConn);
> +extern void PQcancelFinish(PGcancelConn * cancelConn);

That's a lot of new entry points, most of which don't do anything
except call their twin after a pointer cast. How painful would it be to
just use the existing APIs as-is, and error out when calling
unsupported functions if conn->cancelRequest is true?

--Jacob