public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 21/23] cirrus/warnings: use ./configure cache in headerscheck..
6+ messages / 3 participants
[nested] [flat]
* [PATCH 21/23] cirrus/warnings: use ./configure cache in headerscheck..
@ 2022-07-19 17:38 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Justin Pryzby @ 2022-07-19 17:38 UTC (permalink / raw)
This is desirable since since configure is slow. It's necessary for the
environment variables to match, so disable ccache a different way.
ci-os-only: warnings
---
.cirrus.yml | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/.cirrus.yml b/.cirrus.yml
index 4f0695e455d..6f743ee03c0 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -633,11 +633,13 @@ task:
###
always:
headers_headerscheck_script: |
+ export CCACHE_DISABLE=1
time ./configure \
+ --cache gcc.cache \
${LINUX_CONFIGURE_FEATURES} \
--without-icu \
--quiet \
- CC="gcc" CXX"=g++" CLANG="clang"
+ CC="ccache gcc" CXX="ccache g++" CLANG="ccache clang"
make -s -j${BUILD_JOBS} clean
time make -s headerscheck EXTRAFLAGS='-fmax-errors=10'
headers_cpluspluscheck_script: |
--
2.17.1
--tKkaNMvYmhQvRCRK
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0022-cirrus-warnings-move-use-a-single-common-always-bloc.patch"
^ permalink raw reply [nested|flat] 6+ messages in thread
* [PATCH 24/25] cirrus/warnings: use ./configure cache in headerscheck..
@ 2022-07-19 17:38 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Justin Pryzby @ 2022-07-19 17:38 UTC (permalink / raw)
This is desirable since since configure is slow. It's necessary for the
environment variables to match, so disable ccache a different way.
ci-os-only: warnings
---
.cirrus.yml | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/.cirrus.yml b/.cirrus.yml
index 0421d56ca70..81822c58bec 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -627,11 +627,13 @@ task:
###
always:
headers_headerscheck_script: |
+ export CCACHE_DISABLE=1
time ./configure \
+ --cache gcc.cache \
${LINUX_CONFIGURE_FEATURES} \
--without-icu \
--quiet \
- CC="gcc" CXX"=g++" CLANG="clang"
+ CC="ccache gcc" CXX="ccache g++" CLANG="ccache clang"
make -s -j${BUILD_JOBS} clean
time make -s headerscheck EXTRAFLAGS='-fmax-errors=10'
headers_cpluspluscheck_script: |
--
2.17.1
--IS0zKkzwUGydFO0o
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0025-cirrus-warnings-move-use-a-single-common-always-bloc.patch"
^ permalink raw reply [nested|flat] 6+ messages in thread
* [PATCH 21/23] cirrus/warnings: use ./configure cache in headerscheck..
@ 2022-07-19 17:38 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Justin Pryzby @ 2022-07-19 17:38 UTC (permalink / raw)
This is desirable since since configure is slow. It's necessary for the
environment variables to match, so disable ccache a different way.
ci-os-only: warnings
---
.cirrus.yml | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/.cirrus.yml b/.cirrus.yml
index 4f0695e455d..6f743ee03c0 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -633,11 +633,13 @@ task:
###
always:
headers_headerscheck_script: |
+ export CCACHE_DISABLE=1
time ./configure \
+ --cache gcc.cache \
${LINUX_CONFIGURE_FEATURES} \
--without-icu \
--quiet \
- CC="gcc" CXX"=g++" CLANG="clang"
+ CC="ccache gcc" CXX="ccache g++" CLANG="ccache clang"
make -s -j${BUILD_JOBS} clean
time make -s headerscheck EXTRAFLAGS='-fmax-errors=10'
headers_cpluspluscheck_script: |
--
2.17.1
--tKkaNMvYmhQvRCRK
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0022-cirrus-warnings-move-use-a-single-common-always-bloc.patch"
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: thread-safety: gmtime_r(), localtime_r()
@ 2024-07-23 10:51 Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Peter Eisentraut @ 2024-07-23 10:51 UTC (permalink / raw)
To: Heikki Linnakangas <[email protected]>; pgsql-hackers
On 04.07.24 18:36, Heikki Linnakangas wrote:
> The Linux man page for localtime_r() says:
>
>> According to POSIX.1-2001, localtime() is required to behave as
>> though tzset(3) was called, while localtime_r() does not have this
>> requirement. For portable code, tzset(3) should be called before
>> localtime_r().
>
> It's not clear to me what happens if tzset() has not been called and the
> localtime_r() implementation does not call it either. I guess some
> implementation default timezone is used.
>
> In the libpq traces, I don't think we care much. In ecpg, I'm not sure
> what the impact is if the application has not previously called tzset().
> I'd suggest that we just document that an ecpg application should call
> tzset() before calling the functions that are sensitive to local
> timezone setting.
I have been studying this question. It appears that various libc
implementers have been equally puzzled by this; there are various
comments like "it's unclear what POSIX wants here" in the sources. (I
have checked glibc, FreeBSD, and Solaris.)
Consider if a program calls localtime() or localtime_r() twice:
localtime(...);
...
localtime(...);
or
localtime_r(...);
...
localtime_r(...);
The question here is, how many times does this effectively (internally)
call tzset(). There are three possible answers: 0, 1, or 2.
For localtime(), the answer is clear. localtime() is required to call
tzset() every time, so the answer is 2.
For localtime_r(), it's unclear. What you are wondering, and I have
been wondering, is whether the answer is 0 or non-zero (and possibly, if
it's 0, will these calls misbehave badly). What the libc implementers
are wondering is whether the answer is 1 or 2. The implementations
whose source code I have checked think it should be 1. They never
consider that it could be 0 and it's okay to misbehave.
Where this difference appears it practice would be something like
setenv("TZ", "foo");
localtime(...); // uses TZ foo
setenv("TZ", "bar");
localtime(...); // uses TZ bar
versus
setenv("TZ", "foo");
localtime_r(...); // uses TZ foo if first call in program
setenv("TZ", "bar");
localtime_r(...); // probably does not use new TZ
If you want the second case to pick up the changed TZ setting, you must
explicitly call tzset() to be sure.
I think, considering this, the proposed patch should be okay. At least,
the libraries are not going to misbehave if tzset() hasn't been called
explicitly. It will be called internally the first time it's needed. I
don't think we need to cater to cases like my example where the
application changes the TZ environment variable but neglects to call
tzset() itself.
>> There is one affected call in the backend. Most of the backend
>> otherwise uses the custom functions pg_gmtime() and pg_localtime(),
>> which are implemented differently.
>
> Do we need to call tzset(3) somewhere in backend startup? Or could we
> replace that localtime() call in the backend with pg_localtime()?
Let's look at what this code actually does. It just takes the current
time and then loops through all possible weekdays and months to collect
and cache the localized names. The current time or time zone doesn't
actually matter for this, we just need to fill in the struct tm a bit
for strftime() to be happy. We could probably replace this with
gmtime() to avoid the question about time zone state. (We probably
don't even need to call time() beforehand, we could just use time zero.
But the comment says "We use times close to current time as data for
strftime().", which is probably prudent.) (Since we are caching the
results for the session, we're already not dealing with the hilarious
hypothetical situation where the weekday and month names depend on the
actual time, in case that is a concern.)
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: thread-safety: gmtime_r(), localtime_r()
@ 2024-08-16 13:09 Thomas Munro <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Munro @ 2024-08-16 13:09 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; pgsql-hackers
On Tue, Jul 23, 2024 at 10:52 PM Peter Eisentraut <[email protected]> wrote:
> Let's look at what this code actually does. It just takes the current
> time and then loops through all possible weekdays and months to collect
> and cache the localized names. The current time or time zone doesn't
> actually matter for this, we just need to fill in the struct tm a bit
> for strftime() to be happy. We could probably replace this with
> gmtime() to avoid the question about time zone state. (We probably
> don't even need to call time() beforehand, we could just use time zero.
> But the comment says "We use times close to current time as data for
> strftime().", which is probably prudent.) (Since we are caching the
> results for the session, we're already not dealing with the hilarious
> hypothetical situation where the weekday and month names depend on the
> actual time, in case that is a concern.)
I think you could even just use a struct tm filled in with an example
date? Hmm, but it's annoying to choose one, and annoying that POSIX
says there may be other members of the struct, so yeah, I think
gmtime_r(0, tm) makes sense. It can't be that important, because we
aren't even using dates consistent with tm_wday, so we're assuming
that strftime("%a") only looks at tm_wday.
This change complements CF #5170's change strftime()->strftime_l(), to
make the function fully thread-safe.
Someone could also rewrite it to call
nl_langinfo_l({ABDAY,ABMON,DAY,MON}_1 + n, locale) directly, or
GetLocaleInfoEx(locale_name,
LOCALE_S{ABBREVDAY,ABBREVMONTH,DAY,MONTH}NAME1 + n, ...) on Window,
but that'd be more code churn.
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: thread-safety: gmtime_r(), localtime_r()
@ 2024-08-16 13:12 Thomas Munro <[email protected]>
parent: Thomas Munro <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Munro @ 2024-08-16 13:12 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; pgsql-hackers
On Sat, Aug 17, 2024 at 1:09 AM Thomas Munro <[email protected]> wrote:
> This change complements CF #5170's change strftime()->strftime_l(), to
> make the function fully thread-safe.
(Erm, I meant its standard library... of course it has its own global
variables to worry about still.)
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-08-16 13:12 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-07-19 17:38 [PATCH 21/23] cirrus/warnings: use ./configure cache in headerscheck.. Justin Pryzby <[email protected]>
2022-07-19 17:38 [PATCH 24/25] cirrus/warnings: use ./configure cache in headerscheck.. Justin Pryzby <[email protected]>
2022-07-19 17:38 [PATCH 21/23] cirrus/warnings: use ./configure cache in headerscheck.. Justin Pryzby <[email protected]>
2024-07-23 10:51 Re: thread-safety: gmtime_r(), localtime_r() Peter Eisentraut <[email protected]>
2024-08-16 13:09 ` Re: thread-safety: gmtime_r(), localtime_r() Thomas Munro <[email protected]>
2024-08-16 13:12 ` Re: thread-safety: gmtime_r(), localtime_r() Thomas Munro <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox