Re: Reducing WaitEventSet syscall churn

Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
>> On 13 Mar 2020, at 08:21, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> wrote:
>> The attached are:
>> 0001-0004 Not changed
>> 0005 Fix interface of PQregisterEventProc
>> 0006 Add new libpq event for this use.
>> 0007 Another version of "0006 Reuse a WaitEventSet in
>> libpqwalreceiver.c" based on libpq event.
>> 0008-0011 Not changed (old 0007-0010, blindly appended)

> Since 0001 has been applied already in 9b8aa0929390a, the patchtester is unable
> to make heads or tails with trying this patchset. Can you please submit a
> rebased version without the already applied changes?

While I've not really looked at this patchset, I did happen to notice
0005, and I think that's utterly unacceptable as written. You can't
break API/ABI on a released libpq function. I suppose the patch is
assuming that an enum return value is ABI-equivalent to int, but that
assumption is faulty. Moreover, what's the point? None of the later
patches require this AFAICS. (The patch could probably be salvaged
ABI-wise by making the error codes be integer #define's not an enum,
but I fail to see the point of changing this at all. I also don't
much like the idea of allowing callers to assume that there is a fixed
set of possible failure conditions for PQregisterEventProc. If we
wanted to return error details, it'd likely be better to say that an
error message is left in conn->errorMessage.)

0006 is an even more egregious ABI break; you can't renumber existing
enum values without breaking applications. That in itself could be
fixed by putting the new value at the end. But I'd still object very
strongly to 0006, because I do not think that it's acceptable to
have pqDropConnection correspond to an application-visible event.
We use that all over the place for cases that should not be
application-visible, for example when deciding that a connection attempt
has failed and moving on to the next candidate host. We already have
PGEVT_CONNRESET and PGEVT_CONNDESTROY as application-visible connection
state change events, and I don't see why those aren't sufficient.

(BTW, even if these weren't ABI breaks, where are the documentation
changes to go with them?)

regards, tom lane