Re: Re: [COMMITTERS] pgsql: Sync tzload() and tzparse() APIs with IANA release tzcode2016c.

Sync tzload() and tzparse() APIs with IANA release tzcode2016c.

This brings us a bit closer to matching upstream, but since it affects
files outside src/timezone/, we might choose not to back-patch it.
Hence keep it separate from the main update patch.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/1f4e9da624a0caf78bcb526f6b05f5993e26f2c7

Modified Files
--------------
src/bin/initdb/findtimezone.c | 6 +++---
src/timezone/localtime.c | 41 +++++++++++++++++++----------------------
src/timezone/pgtz.c | 4 ++--
src/timezone/pgtz.h | 4 ++--
4 files changed, 26 insertions(+), 29 deletions(-)

On Tue, Mar 29, 2016 at 6:19 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Sync tzload() and tzparse() APIs with IANA release tzcode2016c.
>
> This brings us a bit closer to matching upstream, but since it affects
> files outside src/timezone/, we might choose not to back-patch it.
> Hence keep it separate from the main update patch.

Buildfarm-not-being-happy-status: woodloose, mastodon, thrips, jacana.
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=woodlouse&dt=2016-03-29%2000%3A42%3A08

The origin of the problem is that, which prevents all the subsequent
queries to fail:
SET TimeZone to 'UTC';
+ ERROR: invalid value for parameter "TimeZone": "UTC"
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> Buildfarm-not-being-happy-status: woodloose, mastodon, thrips, jacana.
> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=woodlouse&dt=2016-03-29%2000%3A42%3A08
> The origin of the problem is that, which prevents all the subsequent
> queries to fail:
> SET TimeZone to 'UTC';
> + ERROR: invalid value for parameter "TimeZone": "UTC"

Yeah. I've been staring at that for awhile, but it's not clear where
the problem is. There are a bunch of other SET TIME ZONE commands in
the regression tests, how is it that this trivial case fails on the
Windows critters?

Probably the first thing to eliminate is whether it's zic that is
messing up (and writing a bad zone file) or the backend. Can someone
compare the $INSTALLDIR/share/timezone/UTC files between a working
build and a failing one? They should be bitwise identical (but note
I'm not expecting them to be identical to files from before the tzcode
merge commit).

regards, tom lane

* Tom Lane wrote:

> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>> Buildfarm-not-being-happy-status: woodloose, mastodon, thrips, jacana.
>> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=woodlouse&dt=2016-03-29%2000%3A42%3A08
>> The origin of the problem is that, which prevents all the subsequent
>> queries to fail:
>> SET TimeZone to 'UTC';
>> + ERROR: invalid value for parameter "TimeZone": "UTC"
>
> Yeah. I've been staring at that for awhile, but it's not clear where
> the problem is. There are a bunch of other SET TIME ZONE commands in
> the regression tests, how is it that this trivial case fails on the
> Windows critters?

I think this is the reason, from the check log on woodlouse (jacana says
the same in make style):

Generating timezone files...release\zic\zic: Can't create
C:/buildfarm/buildenv/HEAD/pgsql.build/tmp_install/share/timezone/US/Pacific-New:
No such file or directory

zic aborts somewhere between writing Etc/UTC and UTC. This is how the
toplevel directory ends up after the error:

<DIR> Africa
<DIR> America
<DIR> Antarctica
<DIR> Arctic
<DIR> Asia
<DIR> Atlantic
<DIR> Australia
2.102 CET
2.294 CST6CDT
1.876 EET
127 EST
2.294 EST5EDT
<DIR> Etc
<DIR> Europe
264 Factory
128 HST
<DIR> Indian
2.102 MET
127 MST
2.294 MST7MDT
<DIR> Pacific
2.294 PST8PDT
1.873 WET

After I manually created the "US" directory, zic had no further trouble
putting files into it.

--
Christian

On Tue, Mar 29, 2016 at 1:36 PM, Christian Ullrich <chris(at)chrullrich(dot)net> wrote:
> * Tom Lane wrote:
>
>> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>>>
>>> Buildfarm-not-being-happy-status: woodloose, mastodon, thrips, jacana.
>>>
>>> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=woodlouse&dt=2016-03-29%2000%3A42%3A08
>>> The origin of the problem is that, which prevents all the subsequent
>>> queries to fail:
>>> SET TimeZone to 'UTC';
>>> + ERROR: invalid value for parameter "TimeZone": "UTC"
>>
>>
>> Yeah. I've been staring at that for awhile, but it's not clear where
>> the problem is. There are a bunch of other SET TIME ZONE commands in
>> the regression tests, how is it that this trivial case fails on the
>> Windows critters?
>
>
> I think this is the reason, from the check log on woodlouse (jacana says the
> same in make style):
>
> Generating timezone files...release\zic\zic: Can't create
> C:/buildfarm/buildenv/HEAD/pgsql.build/tmp_install/share/timezone/US/Pacific-New:
> No such file or directory

Yes, I have bumped into that when running the build. And I think that
the error is in zic.c, in dolink() when performing a Link operation
because this parent directory is not created because of that:
if (link_errno == ENOENT || link_errno == ENOTSUP)
{
if (!mkdirs(toname))
exit(EXIT_FAILURE);
retry_if_link_supported = true;
}
I think that we'd want here to check as well on EISDIR or EACCES...
Haven't checked yet though. I'll update this thread with hopefully a
patch.
--
Michael

Christian Ullrich <chris(at)chrullrich(dot)net> writes:
> * Tom Lane wrote:
>> Yeah. I've been staring at that for awhile, but it's not clear where
>> the problem is. There are a bunch of other SET TIME ZONE commands in
>> the regression tests, how is it that this trivial case fails on the
>> Windows critters?

> zic aborts somewhere between writing Etc/UTC and UTC.

Huh ... I would not have guessed that. Can you track down exactly
where it's failing?

We absorbed some new code in zic.c for creating subdirectories of the
target timezone directory, and said code seemed a bit odd to me,
but I supposed that the IANA guys had debugged it already. Maybe not.
Or maybe the problem is with some specific input file that gets
reached somewhere in that range?

regards, tom lane

On Mon, Mar 28, 2016 at 10:09 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Christian Ullrich <chris(at)chrullrich(dot)net> writes:
>> * Tom Lane wrote:
>>> Yeah. I've been staring at that for awhile, but it's not clear where
>>> the problem is. There are a bunch of other SET TIME ZONE commands in
>>> the regression tests, how is it that this trivial case fails on the
>>> Windows critters?
>
>> zic aborts somewhere between writing Etc/UTC and UTC.
>
> Huh ... I would not have guessed that. Can you track down exactly
> where it's failing?

It is failing when attempting to create the link for Pacific_new, and
so the rest is not created at all.

> We absorbed some new code in zic.c for creating subdirectories of the
> target timezone directory, and said code seemed a bit odd to me,
> but I supposed that the IANA guys had debugged it already. Maybe not.
> Or maybe the problem is with some specific input file that gets
> reached somewhere in that range?

The issue is in dolink() visibly. The first time link() is called it
fails on EEXIST (?), but there is no retry because the target link,
US/Pacific-new is not a directory. But note that contrary to the
previous version of the code, link() is not attempted again, symlink
is called instead (HAVE_SYMLINK is actually missing here!) or an
open/getc/close is done to do a copy of the file.

With the WIP patch attached, I am still getting 4 warnings "symbolic
link used because hard link failed", at least it fixes the issue.
--
Michael

* Tom Lane wrote:

> Christian Ullrich <chris(at)chrullrich(dot)net> writes:

>> zic aborts somewhere between writing Etc/UTC and UTC.
>
> Huh ... I would not have guessed that. Can you track down exactly
> where it's failing?

I'd love to, but with 656ee84 I cannot reproduce on my Windows 10
system. I can try on the animals where it actually failed, but now that
there's a fix, that won't be necessary, right?

> We absorbed some new code in zic.c for creating subdirectories of the
> target timezone directory, and said code seemed a bit odd to me,
> but I supposed that the IANA guys had debugged it already. Maybe not.
> Or maybe the problem is with some specific input file that gets
> reached somewhere in that range?

My first guess was that it stumbles over the two-character directory
name, but according to Michael Paquier's fix, that guess was wrong.

--
Christian

* Christian Ullrich wrote:

> * Tom Lane wrote:

>> Christian Ullrich <chris(at)chrullrich(dot)net> writes:

>>> zic aborts somewhere between writing Etc/UTC and UTC.
>>
>> Huh ... I would not have guessed that. Can you track down exactly
>> where it's failing?
>
> I'd love to, but with 656ee84 I cannot reproduce on my Windows 10
> system. I can try on the animals where it actually failed, but now that
> there's a fix, that won't be necessary, right?

Weird. vcregress check works, install fails. Perhaps the directories are
precreated for check?

Anyway, I think Michael's fix is wrong. The bug is that the Win32
version of link() (at the bottom of zic.c) does not set errno if its
attempt to copy the file fails, so what dolink() puts into link_errno is
bogus.

The additional mkdirs() call just papers over the actual bug; the
existing one in line 802 will do nicely once it actually runs.

Patch attached.

--
Christian

Christian Ullrich <chris(at)chrullrich(dot)net> writes:
> Anyway, I think Michael's fix is wrong. The bug is that the Win32
> version of link() (at the bottom of zic.c) does not set errno if its
> attempt to copy the file fails, so what dolink() puts into link_errno is
> bogus.

Ah-hah, that explains things nicely. The previous coding in dolink()
wasn't so dependent on link() returning a valid errno on failure.

> Patch attached.

But then, should not this code make sure that errno *always* gets set?
I'd be inclined to think we should use _dosmaperr(), too, rather than
hand-coding it.

regards, tom lane

* Tom Lane wrote:

> Christian Ullrich <chris(at)chrullrich(dot)net> writes:

>> Anyway, I think Michael's fix is wrong. The bug is that the Win32
>> version of link() (at the bottom of zic.c) does not set errno if its
>> attempt to copy the file fails, so what dolink() puts into link_errno is
>> bogus.
>
> Ah-hah, that explains things nicely. The previous coding in dolink()
> wasn't so dependent on link() returning a valid errno on failure.
>
>> Patch attached.
>
> But then, should not this code make sure that errno *always* gets set?

A library function that does not fail does not touch errno. This link()
replacement is an honorary library function, so neither should it.

> I'd be inclined to think we should use _dosmaperr(), too, rather than
> hand-coding it.

Yes, of course. If only I had known about it ...

New patch attached.

--
Christian

Christian Ullrich <chris(at)chrullrich(dot)net> writes:
> * Tom Lane wrote:
>> But then, should not this code make sure that errno *always* gets set?

> A library function that does not fail does not touch errno.

Right, I meant "always in the error path".

>> I'd be inclined to think we should use _dosmaperr(), too, rather than
>> hand-coding it.

> Yes, of course. If only I had known about it ...
> New patch attached.

This looks good, will push shortly.

regards, tom lane

On Tue, Mar 29, 2016 at 11:20 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Christian Ullrich <chris(at)chrullrich(dot)net> writes:
>> * Tom Lane wrote:
>>> But then, should not this code make sure that errno *always* gets set?
>
>> A library function that does not fail does not touch errno.
>
> Right, I meant "always in the error path".
>
>>> I'd be inclined to think we should use _dosmaperr(), too, rather than
>>> hand-coding it.
>
>> Yes, of course. If only I had known about it ...
>> New patch attached.
>
> This looks good, will push shortly.

Thanks for digging more than I did regarding this issue, things are
fixed on my side, and the buildfarm looks happy. Seeing EEXIST being
returned for a non-existing entry when calling link() really bugged
me, and the errno my patch looked at was set from another function
call and not link() itself...
--
Michael