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
