Thanks for all the feedback. I attached a new patch that I think
addresses all of it. Below some additional info.
> On the whole it feels like a mistake to have two separate kinds of
> PGconn with fundamentally different behaviors and yet no distinction
> in the API. I think I'd recommend having a separate struct type
> (which might internally contain little more than a pointer to a
> cloned PGconn), and provide only a limited set of operations on it.
In my first version of this patch, this is exactly what I did. But then
I got this feedback from Jacob, so I changed it to reusing PGconn:
> > /* 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?
I changed it back to use PGcancelConn as per your suggestion and I
agree that the API got better because of it.
> + CONNECTION_CANCEL_FINISHED,
> /* Non-blocking mode only below here */
>
> is an absolute non-starter: it breaks ABI for every libpq client,
> even ones that aren't using this facility.
I removed this now. The main reason was so it was clear that no
queries could be sent over the connection, like is normally the case
when CONNECTION_OK happens. I don't think this is as useful anymore
now that this patch has a dedicated PGcancelStatus function.
NOTE: The CONNECTION_STARTING ConnStatusType is still necessary.
But to keep ABI compatibility I moved it to the end of the enum.
> Alternatively, we could teach libpq_pipeline
> to do getenv("PG_TEST_TIMEOUT_DEFAULT") with a fallback to 180,
> but that feels like it might be overly familiar with the innards
> of Utils.pm.
I went with this approach, because this environment variable was
already used in 2 other places than Utils.pm:
- contrib/test_decoding/sql/twophase.sql
- src/test/isolation/isolationtester.c
So, one more place seemed quite harmless.
P.S. I noticed a logical conflict between this patch and my libpq load
balancing patch. Because this patch depends on the connhost array
is constructed the exact same on the second invocation of connectOptions2.
But the libpq loadbalancing patch breaks this assumption. I'm making
a mental (and public) note that whichever of these patches gets merged last
should address this issue.
