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

2016-03-29 Thread Michael Paquier 
On Tue, Mar 29, 2016 at 11:20 PM, Tom Lane  wrote:
> Christian Ullrich  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

2016-03-29 Thread Tom Lane 
Christian Ullrich  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

2016-03-29 Thread Christian Ullrich 

* Tom Lane wrote:


Christian Ullrich  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

diff --git a/src/timezone/zic.c b/src/timezone/zic.c
new file mode 100644
index 8d4347a..f9cbac9
*** a/src/timezone/zic.c
--- b/src/timezone/zic.c
*** int
*** 3485,3491 
--- 3485,3494 
  link(const char *oldpath, const char *newpath)
  {
if (!CopyFile(oldpath, newpath, false))
+   {
+   _dosmaperr(GetLastError());
return -1;
+   }
return 0;
  }
  #endif

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

2016-03-29 Thread Tom Lane 
Christian Ullrich  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

2016-03-29 Thread Christian Ullrich 

* Christian Ullrich wrote:


* Tom Lane wrote:



Christian Ullrich  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

diff --git a/src/timezone/zic.c b/src/timezone/zic.c
new file mode 100644
index 8d4347a..ce30740
*** a/src/timezone/zic.c
--- b/src/timezone/zic.c
*** int
*** 3485,3491 
--- 3485,3496 
  link(const char *oldpath, const char *newpath)
  {
if (!CopyFile(oldpath, newpath, false))
+   {
+   DWORD err = GetLastError();
+   if (err == ERROR_PATH_NOT_FOUND || err == ERROR_FILE_NOT_FOUND)
+   errno = ENOENT;
return -1;
+   }
return 0;
  }
  #endif

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

2016-03-29 Thread Christian Ullrich 

* Tom Lane wrote:


Christian Ullrich  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



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

2016-03-29 Thread Michael Paquier 
On Mon, Mar 28, 2016 at 10:09 PM, Tom Lane  wrote:
> Christian Ullrich  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


zic-ms-fix.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

2016-03-28 Thread Tom Lane 
Christian Ullrich  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

2016-03-28 Thread Michael Paquier 
On Tue, Mar 29, 2016 at 1:36 PM, Christian Ullrich  wrote:
> * Tom Lane wrote:
>
>> Michael Paquier  writes:
>>>
>>> Buildfarm-not-being-happy-status: woodloose, mastodon, thrips, jacana.
>>>
>>> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=woodlouse=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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

2016-03-28 Thread Christian Ullrich 

* Tom Lane wrote:


Michael Paquier  writes:

Buildfarm-not-being-happy-status: woodloose, mastodon, thrips, jacana.
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=woodlouse=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:


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

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


--
Christian



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

2016-03-28 Thread Michael Paquier 
On Tue, Mar 29, 2016 at 6:19 AM, Tom Lane  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=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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers