MIME-Version: 1.0
References: <CAFCRh-8hf=7V8UoF63aLxSkeFmXX8-1O5tRxHL61Pngb7V9rcw@mail.gmail.com>
 <740207.1718036299@sss.pgh.pa.us>
In-Reply-To: <740207.1718036299@sss.pgh.pa.us>
From: Dominique Devienne <ddevienne@gmail.com>
Date: Mon, 10 Jun 2024 20:43:28 +0200
Message-ID: <CAFCRh-8zngmAsYfLyrmz6mDhPPF5uh8YRBCcMJjdUidKBeFSxw@mail.gmail.com>
Subject: Re: libpq v17 PQsocketPoll timeout is not granular enough
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-general@postgresql.org
Content-Type: multipart/alternative; boundary="00000000000011b45f061a8d87b7"
Archived-At: <https://www.postgresql.org/message-id/CAFCRh-8zngmAsYfLyrmz6mDhPPF5uh8YRBCcMJjdUidKBeFSxw%40mail.gmail.com>
Precedence: bulk

--00000000000011b45f061a8d87b7
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Bummer=E2=80=A6 I didn=E2=80=99t presume to suggest an api before, but simp=
ly adding an
extra int with the milliseconds offset from the time_t is simple, and
trivial to plug into the implementation I saw. Callers who don=E2=80=99t ca=
re can
simply pass zero. while I could pass a computed time_t and ms offset using
<chrono>. No need for fancy types imho. Aren=E2=80=99t betas precisely for =
the
purpose of exposing apis to those like myself to vet them? This is also
beta1, I,e, the first one. My =E2=82=AC0.02

On Mon, Jun 10, 2024 at 6:18=E2=80=AFPM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Dominique Devienne <ddevienne@gmail.com> writes:
> > PQsocketPoll() being based on time_t, it has only second resolution,
> AFAIK.
> > Despite the [underlying implementation in fe-misc.c][2] supporting at
> > least milliseconds.
> > ...
> > But I think it would a pity if that unreleased API couldn't be made to
> > accomodate sub-second timeouts and more use-cases, like above.
> > Especially given that libpq v17 isn't out yet. I may come late to the
> > game, but hopefully it is not too late.
>
> This is an interesting suggestion, but I think yes it's too late.
> We're already past beta1 and this'd require some fairly fundamental
> rethinking, since there's no easy substitute for type time_t that has
> millisecond resolution.  (The callers do want to specify an end time
> not a timeout interval, since some of them loop around PQsocketPoll
> and don't want the end time to slip.)
>
> I guess conceivably we could use gettimeofday() and struct timeval
> instead of time() and time_t, but it'd touch a lot of places in
> libpq and it'd make some of the calculations a lot more complex.
>
> Maybe a better idea would be to convert to using our
> src/include/portability/instr_time.h abstraction?  But that
> would be problematic for outside callers.
>
> In any case this doesn't seem like a sane thing to be redesigning
> post-beta.  A few months ago maybe we'd have done it, but ...
>
>                         regards, tom lane
>

--00000000000011b45f061a8d87b7
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div><div dir=3D"auto">Bummer=E2=80=A6 I didn=E2=80=99t presume to suggest =
an api before, but simply adding an extra int with the milliseconds offset =
from the time_t is simple, and trivial to plug into the implementation I sa=
w. Callers who don=E2=80=99t care can simply pass zero. while I could pass =
a computed time_t and ms offset using &lt;chrono&gt;. No need for fancy typ=
es imho. Aren=E2=80=99t betas precisely for the purpose of exposing apis to=
 those like myself to vet them? This is also beta1, I,e, the first one. My =
=E2=82=AC0.02</div></div><div><br><div class=3D"gmail_quote"><div dir=3D"lt=
r" class=3D"gmail_attr">On Mon, Jun 10, 2024 at 6:18=E2=80=AFPM Tom Lane &l=
t;<a href=3D"mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>&gt; wrote:<br>=
</div><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-l=
eft:1px #ccc solid;padding-left:1ex">Dominique Devienne &lt;<a href=3D"mail=
to:ddevienne@gmail.com" target=3D"_blank">ddevienne@gmail.com</a>&gt; write=
s:<br>
&gt; PQsocketPoll() being based on time_t, it has only second resolution, A=
FAIK.<br>
&gt; Despite the [underlying implementation in fe-misc.c][2] supporting at<=
br>
&gt; least milliseconds.<br>
&gt; ...<br>
&gt; But I think it would a pity if that unreleased API couldn&#39;t be mad=
e to<br>
&gt; accomodate sub-second timeouts and more use-cases, like above.<br>
&gt; Especially given that libpq v17 isn&#39;t out yet. I may come late to =
the<br>
&gt; game, but hopefully it is not too late.<br>
<br>
This is an interesting suggestion, but I think yes it&#39;s too late.<br>
We&#39;re already past beta1 and this&#39;d require some fairly fundamental=
<br>
rethinking, since there&#39;s no easy substitute for type time_t that has<b=
r>
millisecond resolution.=C2=A0 (The callers do want to specify an end time<b=
r>
not a timeout interval, since some of them loop around PQsocketPoll<br>
and don&#39;t want the end time to slip.)<br>
<br>
I guess conceivably we could use gettimeofday() and struct timeval<br>
instead of time() and time_t, but it&#39;d touch a lot of places in<br>
libpq and it&#39;d make some of the calculations a lot more complex.<br>
<br>
Maybe a better idea would be to convert to using our<br>
src/include/portability/instr_time.h abstraction?=C2=A0 But that<br>
would be problematic for outside callers.<br>
<br>
In any case this doesn&#39;t seem like a sane thing to be redesigning<br>
post-beta.=C2=A0 A few months ago maybe we&#39;d have done it, but ...<br>
<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 regards, tom lane<br>
</blockquote></div></div>

--00000000000011b45f061a8d87b7--