Обсуждение: pgsql: instr_time: Represent time as an int64 on all platforms
instr_time: Represent time as an int64 on all platforms Until now we used struct timespec for instr_time on all platforms but windows. Using struct timespec causes a fair bit of memory (struct timeval is 16 bytes) and runtime overhead (much more complicated additions). Instead we can convert the time to nanoseconds in INSTR_TIME_SET_CURRENT(), making the remaining operations cheaper. Representing time as int64 nanoseconds provides sufficient range, ~292 years relative to a starting point (depending on clock source, relative to the unix epoch or the system's boot time). That'd not be sufficient for calendar time stored on disk, but is plenty for runtime interval time measurement. On windows instr_time already is represented as cycles. It might make sense to represent time as cycles on other platforms as well, as using cycle acquisition instructions like rdtsc directly can reduce the overhead of time acquisition substantially. This could be done in a fairly localized manner as the code stands after this commit. Because the windows and non-windows paths are now more similar, use a common set of macros. To make that possible, most of the use of LARGE_INTEGER had to be removed, which looks nicer anyway. To avoid users of the API relying on the integer representation, we wrap the 64bit integer inside struct struct instr_time. Author: Andres Freund <andres@anarazel.de> Author: Lukas Fittl <lukas@fittl.com> Author: David Geier <geidav.pg@gmail.com> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/20230113195547.k4nlrmawpijqwlsa@awork3.anarazel.de Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/03023a2664f8950ad522385ff75ce004bc932a7c Modified Files -------------- src/include/portability/instr_time.h | 162 +++++++++++++++++++---------------- 1 file changed, 86 insertions(+), 76 deletions(-)
Hi Andres, On Sat, Jan 21, 2023 at 05:25:19AM +0000, Andres Freund wrote: > instr_time: Represent time as an int64 on all platforms > > Until now we used struct timespec for instr_time on all platforms but > windows. Using struct timespec causes a fair bit of memory (struct timeval is > 16 bytes) and runtime overhead (much more complicated additions). Instead we > can convert the time to nanoseconds in INSTR_TIME_SET_CURRENT(), making the > remaining operations cheaper. > > Representing time as int64 nanoseconds provides sufficient range, ~292 years > relative to a starting point (depending on clock source, relative to the unix > epoch or the system's boot time). That'd not be sufficient for calendar time > stored on disk, but is plenty for runtime interval time measurement. > > On windows instr_time already is represented as cycles. It might make sense to > represent time as cycles on other platforms as well, as using cycle > acquisition instructions like rdtsc directly can reduce the overhead of time > acquisition substantially. This could be done in a fairly localized manner as > the code stands after this commit. > > Because the windows and non-windows paths are now more similar, use a common > set of macros. To make that possible, most of the use of LARGE_INTEGER had to > be removed, which looks nicer anyway. > > To avoid users of the API relying on the integer representation, we wrap the > 64bit integer inside struct struct instr_time. > > Author: Andres Freund <andres@anarazel.de> > Author: Lukas Fittl <lukas@fittl.com> hoverfly is unhappy since this went in: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hoverfly&dt=2023-01-23%2005%3A01%3A44 "../../../src/include/portability/instr_time.h", line 116.9: 1506-304 (I) No function prototype given for "clock_gettime". "../../../src/include/portability/instr_time.h", line 116.23: 1506-045 (S) Undeclared identifier CLOCK_REALTIME. <builtin>: recipe for target 'plpy_cursorobject.o' failed Thanks, -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> writes: > On Sat, Jan 21, 2023 at 05:25:19AM +0000, Andres Freund wrote: >> instr_time: Represent time as an int64 on all platforms > hoverfly is unhappy since this went in: > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hoverfly&dt=2023-01-23%2005%3A01%3A44 Yeah, there was some discussion about that already: https://www.postgresql.org/message-id/20230121190303.7xjiwdg3gvb62lu3@awork3.anarazel.de I'm inclined to think that we should fix the plpython code to be rigorous about including everything else we need before including the Python headers. That's ugly, but it's not our fault that Python thinks it can redefine _POSIX_C_SOURCE. regards, tom lane
Hi, On 2023-01-23 01:20:54 -0500, Tom Lane wrote: > Michael Paquier <michael@paquier.xyz> writes: > > On Sat, Jan 21, 2023 at 05:25:19AM +0000, Andres Freund wrote: > >> instr_time: Represent time as an int64 on all platforms > > > hoverfly is unhappy since this went in: > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hoverfly&dt=2023-01-23%2005%3A01%3A44 > > Yeah, there was some discussion about that already: > > https://www.postgresql.org/message-id/20230121190303.7xjiwdg3gvb62lu3@awork3.anarazel.de I was thinking of starting a starting a separate thread about it - it's mostly a plpython issue, the fact that my commit caused the compilation failure is somewhat random. Although I now wonder if we could solve the issue of the compilation failure in a localized way, separately from fixing plpython. There's really no need for execnodes to include instrumentation.h (and thus instr_time). With a forward define of struct Instrumentation and WorkerInstrumentation (and using it in the file), plpython builds just fine with an intentionally broken instr_time.h. > I'm inclined to think that we should fix the plpython code to be rigorous > about including everything else we need before including the Python > headers. That's ugly, but it's not our fault that Python thinks it can > redefine _POSIX_C_SOURCE. Another approach could be to figure out that we ought to define _POSIX_C_SOURCE when building files that involve plpython (e.g. by querying the define during configure). But I'm a bit worried about that breaking assumptions we make - we do define _GNU_SOURCE on linux via CPPFLAGS, and IIRC there are some weird conflicts between the two. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > On 2023-01-23 01:20:54 -0500, Tom Lane wrote: >> Yeah, there was some discussion about that already: >> https://www.postgresql.org/message-id/20230121190303.7xjiwdg3gvb62lu3@awork3.anarazel.de > I was thinking of starting a starting a separate thread about it - it's > mostly a plpython issue, the fact that my commit caused the compilation > failure is somewhat random. True. It also seems odd to me that per your analysis, we fixed the _POSIX_C_SOURCE conflict on 4 Aug 2011 and then broke it again on 18 Dec 2011, yet nobody has noticed for nigh a dozen years --- there has to be some other element in there. regards, tom lane
Hi, On 2023-01-23 01:55:19 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2023-01-23 01:20:54 -0500, Tom Lane wrote: > >> Yeah, there was some discussion about that already: > >> https://www.postgresql.org/message-id/20230121190303.7xjiwdg3gvb62lu3@awork3.anarazel.de > > > I was thinking of starting a starting a separate thread about it - it's > > mostly a plpython issue, the fact that my commit caused the compilation > > failure is somewhat random. > > True. It also seems odd to me that per your analysis, we fixed > the _POSIX_C_SOURCE conflict on 4 Aug 2011 and then broke it again > on 18 Dec 2011, yet nobody has noticed for nigh a dozen years --- > there has to be some other element in there. Well, we didn't *fully* break - all the system library headers included via postgres.h are still included first. It's "just" stuff like <time.h>, that are included later / indirectly, where we broke it. It's not too hard to believe that changing _POSIX_C_SOURCE won't cause immediately visible problems oustide of a few headers that we already included via c.h. Greetings, Andres Freund
Hi, On 2023-01-23 01:20:54 -0500, Tom Lane wrote: > I'm inclined to think that we should fix the plpython code to be rigorous > about including everything else we need before including the Python > headers. I tried that, but I think it's hard with the current split of plpython headers. Several of the plpy* headers, most significantly plpy_typeio.h, include "postgres" headers. Which means that .c files can't include plpy_typeio.h and still maintain the ordering of plpython headers coming last. The only way I see that maintains the split of the plpython headers is to institute the rule that plpy_* can't have any includes other than plpython.h, and all their dependencies have to come from plpython.h. I'll go and start a dedicated thread, this is too big a mess to discuss just on -committers. Greetings, Andres Freund