Horton

Thread

Commits

  1. Dodge a compiler bug affecting timetz_zone/timetz_izone.

  2. Back-patch test cases for timetz_zone/timetz_izone.

  3. Add regression test coverage for timetz_izone().

  4. Add support for AT LOCAL

  5. doc: Mention timezone(zone, time) in section for AT TIME ZONE

  6. Use HASH_BLOBS for xidhash.

  1. Add support for AT LOCAL

    Vik Fearing <vik@postgresfriends.org> — 2023-06-06T03:13:52Z

    The Standard defines time zone conversion as follows:

<datetime factor> ::=
   <datetime primary> [ <time zone> ]

<time zone> ::=
   AT <time zone specifier>

<time zone specifier> ::=
     LOCAL
   | TIME ZONE <interval primary>


While looking at something else, I noticed we do not support AT LOCAL. 
The local time zone is defined as that of *the session*, not the server, 
which can make this quite interesting in views where the view will 
automatically adjust to the session's time zone.

Patch against 3f1aaaa180 attached.
-- 
Vik Fearing

  2. Re: Add support for AT LOCAL

    Laurenz Albe <laurenz.albe@cybertec.at> — 2023-06-06T07:56:42Z

    On Mon, 2023-06-05 at 23:13 -0400, Vik Fearing wrote:
> The Standard defines time zone conversion as follows:
> 
> <datetime factor> ::=
>    <datetime primary> [ <time zone> ]
> 
> <time zone> ::=
>    AT <time zone specifier>
> 
> <time zone specifier> ::=
>      LOCAL
>    | TIME ZONE <interval primary>
> 
> 
> While looking at something else, I noticed we do not support AT LOCAL. 
> The local time zone is defined as that of *the session*, not the server, 
> which can make this quite interesting in views where the view will 
> automatically adjust to the session's time zone.
> 
> Patch against 3f1aaaa180 attached.

+1 on the idea; it should be faily trivial, if not very useful.

At a quick glance, it looks like you resolve "timezone" at the time
the query is parsed.  Shouldn't the resolution happen at query
execution time?

Yours,
Laurenz Albe

  3. Re: Add support for AT LOCAL

    Vik Fearing <vik@postgresfriends.org> — 2023-06-06T08:24:55Z

    On 6/6/23 03:56, Laurenz Albe wrote:
> On Mon, 2023-06-05 at 23:13 -0400, Vik Fearing wrote:
>> The Standard defines time zone conversion as follows:
>>
>> <datetime factor> ::=
>>     <datetime primary> [ <time zone> ]
>>
>> <time zone> ::=
>>     AT <time zone specifier>
>>
>> <time zone specifier> ::=
>>       LOCAL
>>     | TIME ZONE <interval primary>
>>
>>
>> While looking at something else, I noticed we do not support AT LOCAL.
>> The local time zone is defined as that of *the session*, not the server,
>> which can make this quite interesting in views where the view will
>> automatically adjust to the session's time zone.
>>
>> Patch against 3f1aaaa180 attached.
> 
> +1 on the idea; it should be faily trivial, if not very useful.

Thanks.

> At a quick glance, it looks like you resolve "timezone" at the time
> the query is parsed.  Shouldn't the resolution happen at query
> execution time?

current_setting(text) is stable, and my tests show that it is calculated 
at execution time.


postgres=# prepare x as values (now() at local);
PREPARE
postgres=# set timezone to 'UTC';
SET
postgres=# execute x;
           column1
----------------------------
  2023-06-06 08:23:02.088634
(1 row)

postgres=# set timezone to 'Asia/Pyongyang';
SET
postgres=# execute x;
           column1
----------------------------
  2023-06-06 17:23:14.837219
(1 row)


Am I missing something?
-- 
Vik Fearing

  4. Re: Add support for AT LOCAL

    Laurenz Albe <laurenz.albe@cybertec.at> — 2023-06-06T17:02:26Z

    On Tue, 2023-06-06 at 04:24 -0400, Vik Fearing wrote:
> > At a quick glance, it looks like you resolve "timezone" at the time
> > the query is parsed.  Shouldn't the resolution happen at query
> > execution time?
> 
> current_setting(text) is stable, and my tests show that it is calculated 
> at execution time.

Ah, ok, then sorry for the noise.  I misread the code then.

Yours,
Laurenz Albe

  5. Re: Add support for AT LOCAL

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-06-12T15:37:07Z

    On 2023-Jun-06, Laurenz Albe wrote:

> At a quick glance, it looks like you resolve "timezone" at the time
> the query is parsed.  Shouldn't the resolution happen at query
> execution time?

Sounds like it -- consider the case where the timestamp value is a
partition key and one of the partition boundaries falls in between two
timezone offsets for some particular ts value; then you use a prepared
query to read from a view defined with AT LOCAL.  Partition pruning
would need to compute partitions to read from at runtime, not plan time.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

  6. Re: Add support for AT LOCAL

    Vik Fearing <vik@postgresfriends.org> — 2023-06-25T00:14:52Z

    On 6/12/23 17:37, Alvaro Herrera wrote:
> On 2023-Jun-06, Laurenz Albe wrote:
> 
>> At a quick glance, it looks like you resolve "timezone" at the time
>> the query is parsed.  Shouldn't the resolution happen at query
>> execution time?
> 
> Sounds like it -- consider the case where the timestamp value is a
> partition key and one of the partition boundaries falls in between two
> timezone offsets for some particular ts value; then you use a prepared
> query to read from a view defined with AT LOCAL.  Partition pruning
> would need to compute partitions to read from at runtime, not plan time.


Can you show me an example of that happening with my patch?
-- 
Vik Fearing

  7. Re: Add support for AT LOCAL

    Daniel Gustafsson <daniel@yesql.se> — 2023-07-03T13:42:44Z

    > On 6 Jun 2023, at 05:13, Vik Fearing <vik@postgresfriends.org> wrote:

> Patch against 3f1aaaa180 attached.

This patch fails to compile, the declaration of variables in the switch block
needs to be scoped within a { } block.  I've fixed this trivial error in the
attached v2 and also reflowed the comments which now no longer fit.

--
Daniel Gustafsson

  8. Re: Add support for AT LOCAL

    Vik Fearing <vik@postgresfriends.org> — 2023-07-03T14:23:50Z

    On 7/3/23 15:42, Daniel Gustafsson wrote:
>> On 6 Jun 2023, at 05:13, Vik Fearing <vik@postgresfriends.org> wrote:
> 
>> Patch against 3f1aaaa180 attached.
> 
> This patch fails to compile, the declaration of variables in the switch block
> needs to be scoped within a { } block.


Interesting.  It compiles for me.


> I've fixed this trivial error in the
> attached v2 and also reflowed the comments which now no longer fit.


Thank you.
-- 
Vik Fearing

  9. Re: Add support for AT LOCAL

    cary huang <hcary328@gmail.com> — 2023-09-22T21:46:41Z

    The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

Hello

I think this feature can be a useful addition in dealing with time zones. I have applied and tried out the patch, The feature works as described and seems promising. The problem with compilation failure was probably reported on CirrusCI when compiled on different platforms. I have run the latest patch on my own Cirrus CI environment and everything checked out fine. 

Thank you

Cary Huang
------------------
Highgo Software Canada
www.highgo.ca

  10. Re: Add support for AT LOCAL

    Vik Fearing <vik@postgresfriends.org> — 2023-09-22T22:54:01Z

    On 9/22/23 23:46, cary huang wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:       tested, passed
> Spec compliant:           tested, passed
> Documentation:            tested, passed
> 
> Hello
> 
> I think this feature can be a useful addition in dealing with time zones. I have applied and tried out the patch, The feature works as described and seems promising. The problem with compilation failure was probably reported on CirrusCI when compiled on different platforms. I have run the latest patch on my own Cirrus CI environment and everything checked out fine.
> 
> Thank you

Thank you for reviewing!
-- 
Vik Fearing

  11. Re: Add support for AT LOCAL

    Michael Paquier <michael@paquier.xyz> — 2023-09-29T07:27:52Z

    On Sat, Sep 23, 2023 at 12:54:01AM +0200, Vik Fearing wrote:
> On 9/22/23 23:46, cary huang wrote:
>> I think this feature can be a useful addition in dealing with time
>> zones. I have applied and tried out the patch, The feature works as
>> described and seems promising. The problem with compilation failure
>> was probably reported on CirrusCI when compiled on different
>> platforms. I have run the latest patch on my own Cirrus CI environment
>> and everything checked out fine. 
> 
> Thank you for reviewing!

+            | a_expr AT LOCAL                        %prec AT
+                {
+                    /* Use the value of the session's time zone */
+                    FuncCall *tz = makeFuncCall(SystemFuncName("current_setting"),
+                                                list_make1(makeStringConst("TimeZone", -1)),
+                                                COERCE_SQL_SYNTAX,
+                                                -1);
+                    $$ = (Node *) makeFuncCall(SystemFuncName("timezone"),
+                                               list_make2(tz, $1),
+                                               COERCE_SQL_SYNTAX,
+                                               @2);

As the deparsing code introduced by this patch is showing, this leads
to a lot of extra complexity.  And, actually, this can be quite
expensive as well with these two layers of functions.  Note also that
in comparison to SQLValueFunctions, COERCE_SQL_SYNTAX does less
inlining.  So here comes my question: why doesn't this stuff just use 
one underlying function to do this job?
--
Michael

  12. Re: Add support for AT LOCAL

    Vik Fearing <vik@postgresfriends.org> — 2023-10-03T00:10:48Z

    On 9/29/23 09:27, Michael Paquier wrote:
> On Sat, Sep 23, 2023 at 12:54:01AM +0200, Vik Fearing wrote:
>> On 9/22/23 23:46, cary huang wrote:
>>> I think this feature can be a useful addition in dealing with time
>>> zones. I have applied and tried out the patch, The feature works as
>>> described and seems promising. The problem with compilation failure
>>> was probably reported on CirrusCI when compiled on different
>>> platforms. I have run the latest patch on my own Cirrus CI environment
>>> and everything checked out fine.
>>
>> Thank you for reviewing!
> 
> +            | a_expr AT LOCAL                        %prec AT
> +                {
> +                    /* Use the value of the session's time zone */
> +                    FuncCall *tz = makeFuncCall(SystemFuncName("current_setting"),
> +                                                list_make1(makeStringConst("TimeZone", -1)),
> +                                                COERCE_SQL_SYNTAX,
> +                                                -1);
> +                    $$ = (Node *) makeFuncCall(SystemFuncName("timezone"),
> +                                               list_make2(tz, $1),
> +                                               COERCE_SQL_SYNTAX,
> +                                               @2);
> 
> As the deparsing code introduced by this patch is showing, this leads
> to a lot of extra complexity.  And, actually, this can be quite
> expensive as well with these two layers of functions.  Note also that
> in comparison to SQLValueFunctions, COERCE_SQL_SYNTAX does less
> inlining.  So here comes my question: why doesn't this stuff just use
> one underlying function to do this job?

I guess I don't understand the question.  Why do you think a single 
function that repeats what these functions do would be preferable?  I am 
not sure how doing it a different way would be better.
-- 
Vik Fearing

  13. Re: Add support for AT LOCAL

    Michael Paquier <michael@paquier.xyz> — 2023-10-03T23:28:02Z

    On Tue, Oct 03, 2023 at 02:10:48AM +0200, Vik Fearing wrote:
> On 9/29/23 09:27, Michael Paquier wrote:
>> As the deparsing code introduced by this patch is showing, this leads
>> to a lot of extra complexity.  And, actually, this can be quite
>> expensive as well with these two layers of functions.  Note also that
>> in comparison to SQLValueFunctions, COERCE_SQL_SYNTAX does less
>> inlining.  So here comes my question: why doesn't this stuff just use
>> one underlying function to do this job?
> 
> I guess I don't understand the question.  Why do you think a single function
> that repeats what these functions do would be preferable?  I am not sure how
> doing it a different way would be better.

Leaving aside the ruleutils.c changes introduced by the patch that are
quite confusing, having one function in the executor stack is going to
be more efficient than two (aka less ExecInitFunc), and this syntax
could be used in SQL queries where the operations is repeated a lot.
This patch introduces two COERCE_SQL_SYNTAX, meaning that we would do
twice the ACL check, twice the function hook, etc, so this could lead
to significant differences.  I think that we should be careful with
the approach taken, and do benchmarks to choose an efficient approach
from the start.  See for example:
https://www.postgresql.org/message-id/ZGHBGE45jKW8FEpe@paquier.xyz
--
Michael

  14. Re: Add support for AT LOCAL

    Vik Fearing <vik@postgresfriends.org> — 2023-10-04T14:49:03Z

    On 9/29/23 09:27, Michael Paquier wrote:
> On Sat, Sep 23, 2023 at 12:54:01AM +0200, Vik Fearing wrote:
>> On 9/22/23 23:46, cary huang wrote:
>>> I think this feature can be a useful addition in dealing with time
>>> zones. I have applied and tried out the patch, The feature works as
>>> described and seems promising. The problem with compilation failure
>>> was probably reported on CirrusCI when compiled on different
>>> platforms. I have run the latest patch on my own Cirrus CI environment
>>> and everything checked out fine.
>>
>> Thank you for reviewing!
> 
> +            | a_expr AT LOCAL                        %prec AT
> +                {
> +                    /* Use the value of the session's time zone */
> +                    FuncCall *tz = makeFuncCall(SystemFuncName("current_setting"),
> +                                                list_make1(makeStringConst("TimeZone", -1)),
> +                                                COERCE_SQL_SYNTAX,
> +                                                -1);
> +                    $$ = (Node *) makeFuncCall(SystemFuncName("timezone"),
> +                                               list_make2(tz, $1),
> +                                               COERCE_SQL_SYNTAX,
> +                                               @2);
> 
> As the deparsing code introduced by this patch is showing, this leads
> to a lot of extra complexity.  And, actually, this can be quite
> expensive as well with these two layers of functions.  Note also that
> in comparison to SQLValueFunctions, COERCE_SQL_SYNTAX does less
> inlining.  So here comes my question: why doesn't this stuff just use
> one underlying function to do this job?

Okay.  Here is a v3 using that approach.
-- 
Vik Fearing

  15. Re: Add support for AT LOCAL

    Michael Paquier <michael@paquier.xyz> — 2023-10-06T06:05:12Z

    On Wed, Oct 04, 2023 at 03:49:03PM +0100, Vik Fearing wrote:
> Okay.  Here is a v3 using that approach.

You have not posted any numbers to show if there's a difference in
performance, so I have run a simple test:
PREPARE test AS SELECT TIMESTAMP '1978-07-07 19:38' AT LOCAL;
DO $$ BEGIN
  FOR i IN 1..1000000 LOOP
    EXECUTE 'EXECUTE test';
  END LOOP;
END $$;

On a medium-ish benchmark machine I have (16 vCPUs, 32GB of memory,
-O2, no asserts), this DO block takes in average 4.3s to run with v2,
versus 3.6s with v3.  So yes, that's faster.

I haven't yet finished my review of the patch, still looking at it.
--
Michael

  16. Re: Add support for AT LOCAL

    Vik Fearing <vik@postgresfriends.org> — 2023-10-07T01:35:06Z

    On 10/6/23 07:05, Michael Paquier wrote:
> 
> I haven't yet finished my review of the patch, still looking at it.

I realized that my regression tests are not exercising what I originally 
intended them to after this change.  They are supposed to show that 
calling the function explicitly or using AT LOCAL is correctly 
reproduced by ruleutils.c.

The attached v4 changes the regression tests (and nothing else).
-- 
Vik Fearing

  17. Re: Add support for AT LOCAL

    Michael Paquier <michael@paquier.xyz> — 2023-10-10T04:34:32Z

    On Sat, Oct 07, 2023 at 02:35:06AM +0100, Vik Fearing wrote:
> I realized that my regression tests are not exercising what I originally
> intended them to after this change.  They are supposed to show that calling
> the function explicitly or using AT LOCAL is correctly reproduced by
> ruleutils.c.

Yes, I don't really see the point in adding more tests for the
deparsing of AT TIME ZONE in this context.  I would not expect one to
call directly timezone() with the grammar in place, but I have no
objections either to do that in the view for the regression tests as
you are suggesting in v4.  The minimal set of changes to test is to
make sure that both paths (ts->tstz and tstz->tz) are exercised, and
that's what you do.

Anyway, upon review, I have a few issues with this patch.  First is
the documentation that I find too light:
- There is no description for AT LOCAL and what kind of result one
gets back depending on the input given, while AT TIME ZONE has more
details about the arguments that can be used and the results
obtained.
- The function timezone(zone, timestamp) is mentioned, and I think
that we should do the same with timezone(timestamp) for AT LOCAL.

Another thing that I have been surprised with is the following, which
is inconsistent with AT TIME ZONE because we are lacking one system
function:
=# select time with time zone '05:34:17-05' at local;
ERROR:  42883: function pg_catalog.timezone(time with time zone) does not exist

I think that we should include that to have a full set of operations
supported, similar to AT TIME ZONE (see \df+ timezone).  It looks like
this would need one extra timetz_at_local(), which would require a bit
more refactoring in date.c so as an equivalent of timetz_zone() could
feed on the current session's TimeZone instead.  I guess that you
could just have an timetz_zone_internal() that optionally takes a
timezone provided by the user or gets the current session's Timezone
(session_timezone).  Am I missing something?

I am attaching a v5 that addresses the documentation bits, could you
look at the business with date.c?
--
Michael

  18. Re: Add support for AT LOCAL

    Vik Fearing <vik@postgresfriends.org> — 2023-10-13T00:20:59Z

    On 10/10/23 05:34, Michael Paquier wrote:
> I am attaching a v5 that addresses the documentation bits, could you
> look at the business with date.c?

Here is a v6 which hopefully addresses all of your concerns.
-- 
Vik Fearing

  19. Re: Add support for AT LOCAL

    Michael Paquier <michael@paquier.xyz> — 2023-10-13T04:07:00Z

    On Fri, Oct 13, 2023 at 02:20:59AM +0200, Vik Fearing wrote:
> On 10/10/23 05:34, Michael Paquier wrote:
> > I am attaching a v5 that addresses the documentation bits, could you
> > look at the business with date.c?
> 
> Here is a v6

Thanks for the new version.

> which hopefully addresses all of your concerns.

Mostly ;)

The first thing I did was to extract the doc bits about timezone(zone,
time) for AT TIME ZONE from v6 and applied it independently.

I have then looked at the rest and it looked mostly OK to me,
including the extra description you have added for the fifth example
in the docs.  I have tweaked a few things: the regression tests to
make the views a bit more appealing to the eye, an indentation to not
have koel complain and did a catalog bump.  Then applied it.
--
Michael

  20. Re: Add support for AT LOCAL

    Vik Fearing <vik@postgresfriends.org> — 2023-10-13T05:03:20Z

    On 10/13/23 05:07, Michael Paquier wrote:
> On Fri, Oct 13, 2023 at 02:20:59AM +0200, Vik Fearing wrote:
>> On 10/10/23 05:34, Michael Paquier wrote:
>>> I am attaching a v5 that addresses the documentation bits, could you
>>> look at the business with date.c?
>>
>> Here is a v6
> 
> Thanks for the new version.
> 
>> which hopefully addresses all of your concerns.
> 
> Mostly ;)
> 
> The first thing I did was to extract the doc bits about timezone(zone,
> time) for AT TIME ZONE from v6 and applied it independently.
> 
> I have then looked at the rest and it looked mostly OK to me,
> including the extra description you have added for the fifth example
> in the docs.  I have tweaked a few things: the regression tests to
> make the views a bit more appealing to the eye, an indentation to not
> have koel complain and did a catalog bump.  Then applied it.


Thank you, Michael君!
-- 
Vik Fearing

  21. Re: Add support for AT LOCAL

    Michael Paquier <michael@paquier.xyz> — 2023-10-13T05:07:27Z

    On Fri, Oct 13, 2023 at 07:03:20AM +0200, Vik Fearing wrote:
> Thank you, Michael君!

No pb, ヴィックさん。
--
Michael

  22. Re: Add support for AT LOCAL

    Thomas Munro <thomas.munro@gmail.com> — 2023-10-15T21:16:47Z

    One of the AIX animals gave a strange result here:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hornet&dt=2023-10-15%2011%3A40%3A01

If you ignore the diffs due to change in column width, the interesting
change seems to be:

-  23:59:00-07    | 06:59:00+00    | 06:59:00+00    | 06:59:00+00
-  23:59:59.99-07 | 06:59:59.99+00 | 06:59:59.99+00 | 06:59:59.99+00
+  23:59:00-07    | 4294966103:4294967295:00+00 |
4294966103:4294967295:00+00 | 4294966103:4294967295:00+00
+  23:59:59.99-07 | 4294966103:00:00.01+00      |
4294966103:00:00.01+00      | 4294966103:00:00.01+00

But the other AIX animal 'sungazer' was OK with it.  They're both on
the same AIX7.1 host IIRC, both 64 bit builds, but the former is using
xlc and the latter gcc.  I don't immediately see what would cause that
underflow on that old compiler but not elsewhere.  I have a shell
there (cfarm111) if someone has an idea...

  23. Re: Add support for AT LOCAL

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-15T21:57:51Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> One of the AIX animals gave a strange result here:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hornet&dt=2023-10-15%2011%3A40%3A01

> If you ignore the diffs due to change in column width, the interesting
> change seems to be:

> -  23:59:00-07    | 06:59:00+00    | 06:59:00+00    | 06:59:00+00
> -  23:59:59.99-07 | 06:59:59.99+00 | 06:59:59.99+00 | 06:59:59.99+00
> +  23:59:00-07    | 4294966103:4294967295:00+00 |
> 4294966103:4294967295:00+00 | 4294966103:4294967295:00+00
> +  23:59:59.99-07 | 4294966103:00:00.01+00      |
> 4294966103:00:00.01+00      | 4294966103:00:00.01+00

> But the other AIX animal 'sungazer' was OK with it.  They're both on
> the same AIX7.1 host IIRC, both 64 bit builds, but the former is using
> xlc and the latter gcc.  I don't immediately see what would cause that
> underflow on that old compiler but not elsewhere.  I have a shell
> there (cfarm111) if someone has an idea...

Hmm.  Seems like the error has to be creeping in during this part
of timetz_zone():

	result->time = t->time + (t->zone - tz) * USECS_PER_SEC;
	while (result->time < INT64CONST(0))
		result->time += USECS_PER_DAY;
	while (result->time >= USECS_PER_DAY)
		result->time -= USECS_PER_DAY;

According to my machine, the initial computation of result->time
(for the '23:59:59.99-07' input) yields 111599990000, and then we
iterate the second loop once to get 25199990000, which is the right
answer.  If I force a second iteration to get -61200010000, I get

# select '23:59:59.99-07'::timetz at local;
        timezone        
------------------------
 4294967279:00:00.01+00
(1 row)

which doesn't quite match hornet's result but it seems
suggestively close.

Another line of thought is that while the time fields are int64,
t->zone and tz are only int32.  Multiplying by the INT64CONST
USECS_PER_SEC ought to be enough to make the compiler widen
the subtraction result to int64, but maybe that's screwing up?
I'm tempted to wonder if this helps:

-	result->time = t->time + (t->zone - tz) * USECS_PER_SEC;
+	result->time = t->time + (int64) (t->zone - tz) * USECS_PER_SEC;

Forcing the wrong thing to happen there doesn't produce a match
to hornet's result either, so I don't have a lot of hope for that
theory, but it seems like the explanation has to be somewhere here.

			regards, tom lane

  24. Re: Add support for AT LOCAL

    Thomas Munro <thomas.munro@gmail.com> — 2023-10-15T22:24:13Z

    On Mon, Oct 16, 2023 at 10:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm tempted to wonder if this helps:
>
> -       result->time = t->time + (t->zone - tz) * USECS_PER_SEC;
> +       result->time = t->time + (int64) (t->zone - tz) * USECS_PER_SEC;

I wanted to be able to try this and any other theories and managed to
build the tip of master on cfarm111 with the same CC and CFLAGS as
Noah used, but the problem didn't reproduce!  Hmm, I didn't enable any
extra options, so now I'm wondering if something in some random header
somewhere is involved here...  trying again with more stuff turned
on...

  25. Re: Add support for AT LOCAL

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-15T22:47:04Z

    Another possibly interesting factoid: it appears that before
97957fdba, we had zero regression test coverage of timetz_zone ---
and we still have none of timetz_izone, which contains essentially
the same code.  So if there is a problem here, whether it's ours or
the compiler's, it's not hard to see why we didn't notice.

			regards, tom lane

  26. Re: Add support for AT LOCAL

    Thomas Munro <thomas.munro@gmail.com> — 2023-10-15T22:50:08Z

    On Mon, Oct 16, 2023 at 11:24 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Mon, Oct 16, 2023 at 10:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I'm tempted to wonder if this helps:
> >
> > -       result->time = t->time + (t->zone - tz) * USECS_PER_SEC;
> > +       result->time = t->time + (int64) (t->zone - tz) * USECS_PER_SEC;
>
> I wanted to be able to try this and any other theories and managed to
> build the tip of master on cfarm111 with the same CC and CFLAGS as
> Noah used, but the problem didn't reproduce!  Hmm, I didn't enable any
> extra options, so now I'm wondering if something in some random header
> somewhere is involved here...  trying again with more stuff turned
> on...

Oh, I can't use any of the handrolled packages in ~nm due to
permissions.  I tried enabling perl from /opt/freeware (perl is my
usual first guess for who is !@#$ing with the system headers), but the
test passes.

  27. Re: Add support for AT LOCAL

    Michael Paquier <michael@paquier.xyz> — 2023-10-16T01:28:27Z

    On Sun, Oct 15, 2023 at 06:47:04PM -0400, Tom Lane wrote:
> Another possibly interesting factoid: it appears that before
> 97957fdba, we had zero regression test coverage of timetz_zone ---
> and we still have none of timetz_izone, which contains essentially
> the same code.  So if there is a problem here, whether it's ours or
> the compiler's, it's not hard to see why we didn't notice.

Right.  This one is just a lucky, or say unlucky find.  I didn't
notice that this path was entirely missing coverage, planting an
assertion in the middle of timetz_zone() passes check-world.
--
Michael

  28. Re: Add support for AT LOCAL

    Michael Paquier <michael@paquier.xyz> — 2023-10-16T01:58:03Z

    On Mon, Oct 16, 2023 at 11:50:08AM +1300, Thomas Munro wrote:
> On Mon, Oct 16, 2023 at 11:24 AM Thomas Munro <thomas.munro@gmail.com> wrote:
>> On Mon, Oct 16, 2023 at 10:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I'm tempted to wonder if this helps:
>>>
>>> -       result->time = t->time + (t->zone - tz) * USECS_PER_SEC;
>>> +       result->time = t->time + (int64) (t->zone - tz) * USECS_PER_SEC;

All that should use TZNAME_FIXED_OFFSET as timezone type, and I don't
really see why this would overflow..

Perhaps a more aggressive (int64) ((t->zone - (int64) tz) *
USECS_PER_SEC) would help?

>> I wanted to be able to try this and any other theories and managed to
>> build the tip of master on cfarm111 with the same CC and CFLAGS as
>> Noah used, but the problem didn't reproduce!  Hmm, I didn't enable any
>> extra options, so now I'm wondering if something in some random header
>> somewhere is involved here...  trying again with more stuff turned
>> on...
> 
> Oh, I can't use any of the handrolled packages in ~nm due to
> permissions.  I tried enabling perl from /opt/freeware (perl is my
> usual first guess for who is !@#$ing with the system headers), but the
> test passes.

Another theory would be one of these weird compiler optimization issue
from xlc?  In recent history, there was 8d2a01ae12cd.
--
Michael

  29. Re: Add support for AT LOCAL

    Thomas Munro <thomas.munro@gmail.com> — 2023-10-16T02:53:56Z

    On Mon, Oct 16, 2023 at 2:58 PM Michael Paquier <michael@paquier.xyz> wrote:
> Another theory would be one of these weird compiler optimization issue
> from xlc?  In recent history, there was 8d2a01ae12cd.

Yeah, there are more like that too.  xlc 12.1 is dead (like the OS
version it shipped with).  New versions are available on cfarm if we
care about this target.  But I am conscious of the cosmic law that if
you blame the compiler too soon you can cause the bug to move into
your code...

  30. Re: Add support for AT LOCAL

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-16T03:02:45Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Mon, Oct 16, 2023 at 2:58 PM Michael Paquier <michael@paquier.xyz> wrote:
>> Another theory would be one of these weird compiler optimization issue
>> from xlc?  In recent history, there was 8d2a01ae12cd.

> Yeah, there are more like that too.  xlc 12.1 is dead (like the OS
> version it shipped with).  New versions are available on cfarm if we
> care about this target.  But I am conscious of the cosmic law that if
> you blame the compiler too soon you can cause the bug to move into
> your code...

I'm having a hard time not believing that this is a compiler bug.
Looking back at 8d2a01ae12cd and its speculation that xlc is overly
liberal about reordering code around sequence points ... I wonder
if it'd help to do this calculation in a local variable, and only
assign the final value to result->time ?  But we have to reproduce
the problem first.

			regards, tom lane

  31. Re: Add support for AT LOCAL

    Thomas Munro <thomas.munro@gmail.com> — 2023-10-16T03:07:20Z

    On Mon, Oct 16, 2023 at 4:02 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm having a hard time not believing that this is a compiler bug.
> Looking back at 8d2a01ae12cd and its speculation that xlc is overly
> liberal about reordering code around sequence points ... I wonder
> if it'd help to do this calculation in a local variable, and only
> assign the final value to result->time ?  But we have to reproduce
> the problem first.

If that can be shown I would vote for switching to /opt/IBM/xlc/16.1.0
and not changing a single bit of PostgreSQL.

  32. Re: Add support for AT LOCAL

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-16T03:30:17Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Mon, Oct 16, 2023 at 4:02 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm having a hard time not believing that this is a compiler bug.
>> Looking back at 8d2a01ae12cd and its speculation that xlc is overly
>> liberal about reordering code around sequence points ... I wonder
>> if it'd help to do this calculation in a local variable, and only
>> assign the final value to result->time ?  But we have to reproduce
>> the problem first.

> If that can be shown I would vote for switching to /opt/IBM/xlc/16.1.0
> and not changing a single bit of PostgreSQL.

If switching to 16.1 removes the failure, I'd agree.  It's hard
to believe that any significant number of users still care about
building PG with xlc 12.

			regards, tom lane

  33. Re: Add support for AT LOCAL

    Michael Paquier <michael@paquier.xyz> — 2023-10-16T04:22:54Z

    On Sun, Oct 15, 2023 at 11:30:17PM -0400, Tom Lane wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
>> If that can be shown I would vote for switching to /opt/IBM/xlc/16.1.0
>> and not changing a single bit of PostgreSQL.
> 
> If switching to 16.1 removes the failure, I'd agree.  It's hard
> to believe that any significant number of users still care about
> building PG with xlc 12.

FWIW, I really wish that we were less conservative here and just drop
that rather than waste resources in debugging things.

Now, I'm also OK to put this one aside and put a WHERE clause to
timetz_local_view to only fetch one value, as the test has the same
value as long as we check that AT LOCAL converts the result to UTC for
the three expression patterns tested.
--
Michael

  34. Re: Add support for AT LOCAL

    Noah Misch <noah@leadboat.com> — 2023-10-16T04:58:04Z

    On Sun, Oct 15, 2023 at 11:30:17PM -0400, Tom Lane wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > On Mon, Oct 16, 2023 at 4:02 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I'm having a hard time not believing that this is a compiler bug.
> >> Looking back at 8d2a01ae12cd and its speculation that xlc is overly
> >> liberal about reordering code around sequence points ... I wonder
> >> if it'd help to do this calculation in a local variable, and only
> >> assign the final value to result->time ?  But we have to reproduce
> >> the problem first.
> 
> > If that can be shown I would vote for switching to /opt/IBM/xlc/16.1.0
> > and not changing a single bit of PostgreSQL.
> 
> If switching to 16.1 removes the failure, I'd agree.  It's hard
> to believe that any significant number of users still care about
> building PG with xlc 12.

Works for me.  I've started a test run with the xlc version change.

  35. Re: Add support for AT LOCAL

    Noah Misch <noah@leadboat.com> — 2023-10-16T05:50:51Z

    On Sun, Oct 15, 2023 at 09:58:04PM -0700, Noah Misch wrote:
> On Sun, Oct 15, 2023 at 11:30:17PM -0400, Tom Lane wrote:
> > Thomas Munro <thomas.munro@gmail.com> writes:
> > > On Mon, Oct 16, 2023 at 4:02 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >> I'm having a hard time not believing that this is a compiler bug.
> > >> Looking back at 8d2a01ae12cd and its speculation that xlc is overly
> > >> liberal about reordering code around sequence points ... I wonder
> > >> if it'd help to do this calculation in a local variable, and only
> > >> assign the final value to result->time ?  But we have to reproduce
> > >> the problem first.
> > 
> > > If that can be shown I would vote for switching to /opt/IBM/xlc/16.1.0
> > > and not changing a single bit of PostgreSQL.
> > 
> > If switching to 16.1 removes the failure, I'd agree.  It's hard
> > to believe that any significant number of users still care about
> > building PG with xlc 12.
> 
> Works for me.  I've started a test run with the xlc version change.

It failed similarly:

+  23:59:00-07    | 4294966103:4294967295:00+00 | 4294966103:4294967295:00+00 | 4294966103:4294967295:00+00
+  23:59:59.99-07 | 4294966103:00:00.01+00      | 4294966103:00:00.01+00      | 4294966103:00:00.01+00

  36. Re: Add support for AT LOCAL

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-16T05:54:23Z

    Noah Misch <noah@leadboat.com> writes:
> On Sun, Oct 15, 2023 at 09:58:04PM -0700, Noah Misch wrote:
>> Works for me.  I've started a test run with the xlc version change.

> It failed similarly:

> +  23:59:00-07    | 4294966103:4294967295:00+00 | 4294966103:4294967295:00+00 | 4294966103:4294967295:00+00
> +  23:59:59.99-07 | 4294966103:00:00.01+00      | 4294966103:00:00.01+00      | 4294966103:00:00.01+00

Ugh.  So if the failure is robust enough to persist across
several major xlc versions, why couldn't Thomas reproduce it?

			regards, tom lane

  37. Re: Add support for AT LOCAL

    Noah Misch <noah@leadboat.com> — 2023-10-16T06:05:10Z

    On Mon, Oct 16, 2023 at 01:54:23AM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > On Sun, Oct 15, 2023 at 09:58:04PM -0700, Noah Misch wrote:
> >> Works for me.  I've started a test run with the xlc version change.
> 
> > It failed similarly:
> 
> > +  23:59:00-07    | 4294966103:4294967295:00+00 | 4294966103:4294967295:00+00 | 4294966103:4294967295:00+00
> > +  23:59:59.99-07 | 4294966103:00:00.01+00      | 4294966103:00:00.01+00      | 4294966103:00:00.01+00
> 
> Ugh.  So if the failure is robust enough to persist across
> several major xlc versions, why couldn't Thomas reproduce it?

Beats me.  hornet wipes its starting environment down to OBJECT_MODE=32_64
PERL5LIB=/home/nm/sw/cpan64/lib/perl5 SPECIES=xlc64 PATH=/usr/bin, then
applies all the environment settings seen in buildfarm logs.

  38. Re: Add support for AT LOCAL

    Michael Paquier <michael@paquier.xyz> — 2023-10-16T07:29:56Z

    On Sun, Oct 15, 2023 at 11:05:10PM -0700, Noah Misch wrote:
> On Mon, Oct 16, 2023 at 01:54:23AM -0400, Tom Lane wrote:
>> Ugh.  So if the failure is robust enough to persist across
>> several major xlc versions, why couldn't Thomas reproduce it?
> 
> Beats me.  hornet wipes its starting environment down to OBJECT_MODE=32_64
> PERL5LIB=/home/nm/sw/cpan64/lib/perl5 SPECIES=xlc64 PATH=/usr/bin, then
> applies all the environment settings seen in buildfarm logs.

Perhaps that's a stupid question..  But a server running under this
environment fails the two following queries even for older branches,
right?
select timezone('UTC', '23:59:59.99-07'::timetz);
select timezone('UTC', '23:59:00-07'::timetz);
--
Michael

  39. Re: Add support for AT LOCAL

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-16T13:54:41Z

    Michael Paquier <michael@paquier.xyz> writes:
> Perhaps that's a stupid question..  But a server running under this
> environment fails the two following queries even for older branches,
> right?
> select timezone('UTC', '23:59:59.99-07'::timetz);
> select timezone('UTC', '23:59:00-07'::timetz);

One would expect, since the AT LOCAL syntax is just sugar for that.

I'm mighty tempted though to (a) add coverage for timetz_izone
to HEAD, and (b) backpatch the new tests, sans the AT LOCAL case,
to the back branches (maybe not v11).

			regards, tom lane

  40. Re: Add support for AT LOCAL

    Michael Paquier <michael@paquier.xyz> — 2023-10-16T23:25:29Z

    On Mon, Oct 16, 2023 at 09:54:41AM -0400, Tom Lane wrote:
> I'm mighty tempted though to (a) add coverage for timetz_izone
> to HEAD, and (b) backpatch the new tests, sans the AT LOCAL case,
> to the back branches (maybe not v11).

I see that you've already done (a) with 2f04720307.  I'd be curious to
see what happens for (b), as well, once (a) is processed on hornet
once..
--
Michael

  41. Re: Add support for AT LOCAL

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-17T02:11:41Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Oct 16, 2023 at 09:54:41AM -0400, Tom Lane wrote:
>> I'm mighty tempted though to (a) add coverage for timetz_izone
>> to HEAD, and (b) backpatch the new tests, sans the AT LOCAL case,
>> to the back branches (maybe not v11).

> I see that you've already done (a) with 2f04720307.  I'd be curious to
> see what happens for (b), as well, once (a) is processed on hornet
> once..

Sure enough, timetz_izone has exactly the same behavior [1].

I'd kind of decided that back-patching wouldn't be worth the trouble;
do you foresee that it'd teach us anything new?

			regards, tom lane

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hornet&dt=2023-10-17%2000%3A07%3A36

  42. Re: Add support for AT LOCAL

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-17T05:40:18Z

    Noah Misch <noah@leadboat.com> writes:
> On Mon, Oct 16, 2023 at 01:54:23AM -0400, Tom Lane wrote:
>> Ugh.  So if the failure is robust enough to persist across
>> several major xlc versions, why couldn't Thomas reproduce it?

> Beats me.  hornet wipes its starting environment down to OBJECT_MODE=32_64
> PERL5LIB=/home/nm/sw/cpan64/lib/perl5 SPECIES=xlc64 PATH=/usr/bin, then
> applies all the environment settings seen in buildfarm logs.

I was able to reproduce the failure on cfarm111 after adopting
these settings from hornet's configuration:

export OBJECT_MODE=64
export CC='xlc_r -D_LARGE_FILES=1 '
export CFLAGS='-O2 -qmaxmem=33554432 -qsuppress=1500-010:1506-995 -qsuppress=1506-010:1506-416:1506-450:1506-480:1506-481:1506-492:1506-944:1506-1264 -qinfo=all:nocnd:noeff:noext:nogot:noini:noord:nopar:noppc:norea:nouni:nouse -qsuppress=1506-374:1506-419:1506-434:1506-438:1506-451:1506-452:1506-453:1506-495:1506-786'

and doing

./configure --enable-cassert --enable-debug --without-icu --prefix=/home/tgl/testversion

etc etc.

It is absolutely, gold-platedly, a compiler bug, because inserting
a debug printf into the loop

	while (result->time >= USECS_PER_DAY)
		result->time -= USECS_PER_DAY;

makes the failure go away.  Unfortunately, I've not yet found another
way to make it go away :-(.  My upthread idea of using a local variable
instead of result->time is no help, and some other random code
alterations didn't change the results either.

Not sure where we go from here.  While I don't have much hesitation
about blowing off xlc_r 12.1, it would be sad if their latest
toolchain doesn't work either.  (I didn't try permuting the code
while using the newer compiler, though.)

			regards, tom lane

  43. Re: Add support for AT LOCAL

    Michael Paquier <michael@paquier.xyz> — 2023-10-17T06:25:14Z

    On Tue, Oct 17, 2023 at 01:40:18AM -0400, Tom Lane wrote:
> makes the failure go away.  Unfortunately, I've not yet found another
> way to make it go away :-(.  My upthread idea of using a local variable
> instead of result->time is no help, and some other random code
> alterations didn't change the results either.

That may be a long shot, but even a modulo?  Say in these two code
paths:
-   while (result->time >= USECS_PER_DAY)
-       result->time -= USECS_PER_DAY;
+   if (result->time >= USECS_PER_DAY)
+       result->time %= USECS_PER_DAY;

> Not sure where we go from here.  While I don't have much hesitation
> about blowing off xlc_r 12.1, it would be sad if their latest
> toolchain doesn't work either.  (I didn't try permuting the code
> while using the newer compiler, though.)

We've spent a lot of time on that.  I'm OK to just give up, trim the
values of the view with a qual, and call it a day.
--
Michael

  44. Re: Add support for AT LOCAL

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-17T15:31:06Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Tue, Oct 17, 2023 at 01:40:18AM -0400, Tom Lane wrote:
>> makes the failure go away.  Unfortunately, I've not yet found another
>> way to make it go away :-(.  My upthread idea of using a local variable
>> instead of result->time is no help, and some other random code
>> alterations didn't change the results either.

> That may be a long shot, but even a modulo?

Yeah, the same thing occurred to me in the shower this morning, and it
does seem to work!  We can replace both loops with a %= operator, at
least if we're willing to assume C99 division semantics, which seems
pretty safe in 2023.  Your idea of doing a range check to skip the
division in typical cases is a refinement I'd not thought of, but
it seems like a good idea for performance.

(I see that the negative-starting-point case isn't covered in the
current regression tests, so maybe we better add a test for that.)

			regards, tom lane

  45. Re: Add support for AT LOCAL

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-17T16:45:28Z

    I wrote:
> Yeah, the same thing occurred to me in the shower this morning, and it
> does seem to work!  We can replace both loops with a %= operator, at
> least if we're willing to assume C99 division semantics, which seems
> pretty safe in 2023.

Whoops, no: for negative starting values we'd need truncate-towards-
minus-infinity division whereas C99 specifies truncate-towards-zero.
However, the attached does pass for me on cfarm111 as well as my
usual dev machine.

Presumably this is a pre-existing bug that also appears in back
branches.  But in the interests of science I propose that we
back-patch only the test case and see which machine(s) fail it
before back-patching the code change.

			regards, tom lane

  46. Re: Add support for AT LOCAL

    Thomas Munro <thomas.munro@gmail.com> — 2023-10-17T22:38:58Z

    Hmm, I guess I must have missed some important flag or environment
variable when trying to reproduce it, sorry.

Given that IBM describes xlc as "legacy" (replaced by xlclang, but
still supported for some unspecified period of time for the benefit of
people who need C++ ABI compatibility with old code), I wonder how
long we plan to support it...  Anecdotally, from a time 1-2 decades
ago when I used AIX daily, I can report that vast amounts of open
source stuff couldn't build with xlc, so gcc was used for pretty much
anything that didn't have a C++ ABI requirement.  I kinda wonder if a
single person in the entire world appreciates that we support this.

  47. Re: Add support for AT LOCAL

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-17T22:54:33Z

    Thomas Munro <thomas.munro@gmail.com> writes:

> Given that IBM describes xlc as "legacy" (replaced by xlclang, but
> still supported for some unspecified period of time for the benefit of
> people who need C++ ABI compatibility with old code), I wonder how
> long we plan to support it...

Should we be testing against xlclang instead?

			regards, tom lane

  48. Re: Add support for AT LOCAL

    Thomas Munro <thomas.munro@gmail.com> — 2023-10-17T23:11:35Z

    On Wed, Oct 18, 2023 at 11:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
>
> > Given that IBM describes xlc as "legacy" (replaced by xlclang, but
> > still supported for some unspecified period of time for the benefit of
> > people who need C++ ABI compatibility with old code), I wonder how
> > long we plan to support it...
>
> Should we be testing against xlclang instead?

I hesitated to suggest it because it's not my animal/time we're
talking about but it seems to make more sense.  It appears to be IBM's
answer to the nothing-builds-with-this-thing phenomenon, since it
accepts a lot of GCCisms via Clang's adoption of them.  From a quick
glance at [1], it lacks the atomics builtins but we have our own
assembler magic for POWER.  So maybe it'd all just work™.

[1] https://www.ibm.com/docs/en/xl-c-and-cpp-aix/16.1?topic=migration-checklist-when-moving-from-xl-based-front-end-clang-based-front-end

  49. Re: Add support for AT LOCAL

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-17T23:32:05Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Wed, Oct 18, 2023 at 11:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Should we be testing against xlclang instead?

> I hesitated to suggest it because it's not my animal/time we're
> talking about but it seems to make more sense.  It appears to be IBM's
> answer to the nothing-builds-with-this-thing phenomenon, since it
> accepts a lot of GCCisms via Clang's adoption of them.  From a quick
> glance at [1], it lacks the atomics builtins but we have our own
> assembler magic for POWER.  So maybe it'd all just work™.

Discounting the Windows animals, it looks like the xlc animals are
our only remaining ones that use anything except gcc or clang.
That feels uncomfortably like a compiler monoculture to me, so
I can understand the reasoning for keeping hornet/mandrill going.
Still, maybe we should just accept the fact that gcc/clang have
outcompeted everything else in the C compiler universe.  It's
getting hard to imagine that anyone would bring out some new product
that didn't try to be bug-compatible with gcc, for precisely the
reason you mention.

			regards, tom lane

  50. Re: Add support for AT LOCAL

    Michael Paquier <michael@paquier.xyz> — 2023-10-18T00:02:13Z

    On Tue, Oct 17, 2023 at 12:45:28PM -0400, Tom Lane wrote:
> Whoops, no: for negative starting values we'd need truncate-towards-
> minus-infinity division whereas C99 specifies truncate-towards-zero.
> However, the attached does pass for me on cfarm111 as well as my
> usual dev machine.

I guess that the following trick could be used for the negative case,
with one modulo followed by one extra addition: 
if (result->time < INT64CONST(0))
{
    result->time %= USECS_PER_DAY;
    result->time += USECS_PER_DAY;
}

> Presumably this is a pre-existing bug that also appears in back
> branches.  But in the interests of science I propose that we
> back-patch only the test case and see which machine(s) fail it
> before back-patching the code change.

Sure, as you see fit.
--
Michael

  51. Re: Add support for AT LOCAL

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-18T03:15:14Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Wed, Oct 18, 2023 at 11:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Should we be testing against xlclang instead?

> I hesitated to suggest it because it's not my animal/time we're
> talking about but it seems to make more sense.  It appears to be IBM's
> answer to the nothing-builds-with-this-thing phenomenon, since it
> accepts a lot of GCCisms via Clang's adoption of them.  From a quick
> glance at [1], it lacks the atomics builtins but we have our own
> assembler magic for POWER.  So maybe it'd all just work™.

FWIW, I tried a test build with xlclang 16.1 on cfarm111, and
it does seem like it Just Works, modulo a couple of oddities:

* <netinet/tcp.h> fails to compile, due to references to struct
in6_addr, unless <netinet/in.h> is included first.  Most of our
references to tcp.h already do that, but not libpq-be.h and
fe-protocol3.c.  I'm a bit at a loss why we've not seen this
with the existing BF animals on this machine, because AFAICS
they're all using the same /usr/include tree.

* configure recognizes this as gcc but not Clang, which may or may
not be fine:
...
checking whether we are using the GNU C compiler... yes
...
checking whether xlclang is Clang... no
...
This doesn't seem to break anything, but it struck me as odd.
configure seems to pick a sane set of compiler options anyway.

Interestingly, xlclang shows the same failure with the pre-19fa97731
versions of timetz_zone/timetz_izone as plain xlc does.  I guess
this is not so astonishing since they presumably share the same
codegen backend.  But maybe somebody ought to file a bug with IBM?

			regards, tom lane

  52. Re: Add support for AT LOCAL

    Robert Haas <robertmhaas@gmail.com> — 2023-10-18T15:38:18Z

    On Tue, Oct 17, 2023 at 7:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > On Wed, Oct 18, 2023 at 11:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Should we be testing against xlclang instead?
>
> > I hesitated to suggest it because it's not my animal/time we're
> > talking about but it seems to make more sense.  It appears to be IBM's
> > answer to the nothing-builds-with-this-thing phenomenon, since it
> > accepts a lot of GCCisms via Clang's adoption of them.  From a quick
> > glance at [1], it lacks the atomics builtins but we have our own
> > assembler magic for POWER.  So maybe it'd all just work™.
>
> Discounting the Windows animals, it looks like the xlc animals are
> our only remaining ones that use anything except gcc or clang.
> That feels uncomfortably like a compiler monoculture to me, so
> I can understand the reasoning for keeping hornet/mandrill going.
> Still, maybe we should just accept the fact that gcc/clang have
> outcompeted everything else in the C compiler universe.  It's
> getting hard to imagine that anyone would bring out some new product
> that didn't try to be bug-compatible with gcc, for precisely the
> reason you mention.

After some research I determined that the release date for xlc 12.1
seems to be June 1, 2012. At that time, clang 3.1 was current and just
after, GCC release version 4.7.1 was released. The oldest version of
clang that I find in the buildfarm is 3.9, and the oldest version of
gcc I find in the buildfarm is 4.6.3. So, somewhat to my surprise, xlc
is not the oldest compiler that we're still supporting in the
buildfarm. But it is very old, and it seems like that gcc and clang
are going to continue to gain ground against gcc and other proprietary
compilers for some time to come. I think it's reasonable to ask
ourselves whether we really want to go to the trouble of maintaining
something that is likely to get so little real-world usage.

To be honest, I'm not particularly concerned about the need to adjust
compiler and linker options from time to time, even though I know that
probably annoys Andres. What does concern me is finding and coding
around compiler bugs. 19fa977311b9da9c6c84f0108600e78213751a38 is just
ridiculous, IMHO. If an end-of-life compiler for an end-of-life
operating system has bugs that mean that C code that's doing nothing
more than a bit of arithmetic isn't compiling properly, it's time to
pull the plug. Nor is this the first example of working around a bug
that only manifests in ancient xlc.

I think that, when there was more real diversity in the software
ecosystem, testing on a lot of platforms was a good way of finding out
whether you'd done something that was in general correct or just
something that happened to work on the machine you had in front of
you. But hornet and mandrill are not telling us about things we've
done that are incorrect in general yet happen to work on gcc and
clang. What they seem to be telling us about, in this case and some
others, are things that are CORRECT in general yet happen NOT to work
on ancient xlc. And that's an important difference, because if we were
finding real mistakes for which other platforms were not punishing us,
then we could hope that fixing those mistakes would improve
compatibility with other, equally niche platforms, potentially
including future platforms that haven't come along yet. As it is, it's
hard to believe that any work we put into this is going to have any
benefit on any system other than ancient AIX. If there are other niche
systems out there that have a similar number of bugs, they'll probably
be *different* bugs.

Sources for release dates:

https://www.ibm.com/support/pages/fix-list-xl-c-aix
https://releases.llvm.org/
https://gcc.gnu.org/releases.html

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  53. Re: Add support for AT LOCAL

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-18T16:02:28Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Oct 17, 2023 at 7:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Discounting the Windows animals, it looks like the xlc animals are
>> our only remaining ones that use anything except gcc or clang.

> After some research I determined that the release date for xlc 12.1
> seems to be June 1, 2012. At that time, clang 3.1 was current and just
> after, GCC release version 4.7.1 was released. The oldest version of
> clang that I find in the buildfarm is 3.9, and the oldest version of
> gcc I find in the buildfarm is 4.6.3. So, somewhat to my surprise, xlc
> is not the oldest compiler that we're still supporting in the
> buildfarm. But it is very old, and it seems like that gcc and clang
> are going to continue to gain ground against gcc and other proprietary
> compilers for some time to come.

Probably.  Independent of that, it's fair to ask why we're still
testing against xlc 12.1 and not the considerably-more-recent xlclang,
or at least xlc 16.1.  (I also wonder why we're still testing AIX 7.1
rather than an OS version that's not EOL.)

> What does concern me is finding and coding
> around compiler bugs. 19fa977311b9da9c6c84f0108600e78213751a38 is just
> ridiculous, IMHO.

I would agree, except for the downthread discovery that the bug is
still present in current xlc and xlclang.  Short of blowing off AIX
altogether, it seems like we need to do something about it.

			regards, tom lane

  54. Re: Add support for AT LOCAL

    Robert Haas <robertmhaas@gmail.com> — 2023-10-18T17:01:42Z

    On Wed, Oct 18, 2023 at 12:02 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Probably.  Independent of that, it's fair to ask why we're still
> testing against xlc 12.1 and not the considerably-more-recent xlclang,
> or at least xlc 16.1.  (I also wonder why we're still testing AIX 7.1
> rather than an OS version that's not EOL.)

Well, according to Wikipedia, AIX 7.3 (released in 2021) requires
POWER8. AIX 7.2 (released 2015) only requires POWER7, and according to
the buildfarm page, this machine is POWER7. So it could possibly be
upgraded from 7.1 to 7.2, supposing that it is indeed compatible with
that release and that Noah's willing to do it and that there's not an
exorbitant fee and so on, but that still leaves you running an OS
version that is almost certainly closer to EOL than it is to the
original release date. Anything newer would require buying new
hardware, or so I guess.

Put otherwise, I think the reason we're testing on this AIX rather
than anything else is probably that there is exactly 1 person
associated with the project who has >0 pieces of hardware that can run
AIX, and that person has one, so we're testing on that one. That might
be a reason to question whether that particular strain of hardware has
a bright future, at least in terms of PostgreSQL support.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  55. Re: Add support for AT LOCAL

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-18T20:45:46Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Oct 18, 2023 at 12:02 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Probably.  Independent of that, it's fair to ask why we're still
>> testing against xlc 12.1 and not the considerably-more-recent xlclang,
>> or at least xlc 16.1.  (I also wonder why we're still testing AIX 7.1
>> rather than an OS version that's not EOL.)

> Well, according to Wikipedia, AIX 7.3 (released in 2021) requires
> POWER8. AIX 7.2 (released 2015) only requires POWER7, and according to
> the buildfarm page, this machine is POWER7. So it could possibly be
> upgraded from 7.1 to 7.2, supposing that it is indeed compatible with
> that release and that Noah's willing to do it and that there's not an
> exorbitant fee and so on, but that still leaves you running an OS
> version that is almost certainly closer to EOL than it is to the
> original release date. Anything newer would require buying new
> hardware, or so I guess.

The machine belongs to OSU (via the gcc compile farm), and I see
that they have another one that's POWER8 and is running AIX 7.3 [1].
So in principle the buildfarm animals could just be moved over
to that one.

Perhaps Noah has some particular attachment to 7.1, but now that that's
EOL it seems like we shouldn't be paying so much attention to it.
My guess is that it's still there in the compile farm because the gcc
people think it's still useful to have access to POWER7 hardware; but
I doubt there's enough difference for our purposes to be worth dealing
with a dead OS and ancient compiler.

			regards, tom lane

[1] https://portal.cfarm.net/machines/list/

  56. Re: Add support for AT LOCAL

    Noah Misch <noah@leadboat.com> — 2023-10-18T23:33:20Z

    On Wed, Oct 18, 2023 at 04:45:46PM -0400, Tom Lane wrote:
> > On Wed, Oct 18, 2023 at 12:02 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Probably.  Independent of that, it's fair to ask why we're still
> >> testing against xlc 12.1 and not the considerably-more-recent xlclang,
> >> or at least xlc 16.1.  (I also wonder why we're still testing AIX 7.1
> >> rather than an OS version that's not EOL.)

> The machine belongs to OSU (via the gcc compile farm), and I see
> that they have another one that's POWER8 and is running AIX 7.3 [1].
> So in principle the buildfarm animals could just be moved over
> to that one.
> 
> Perhaps Noah has some particular attachment to 7.1, but now that that's
> EOL it seems like we shouldn't be paying so much attention to it.
> My guess is that it's still there in the compile farm because the gcc
> people think it's still useful to have access to POWER7 hardware; but
> I doubt there's enough difference for our purposes to be worth dealing
> with a dead OS and ancient compiler.

No particular attachment.  From 2019 to 2023-08, hoverfly tested xlc16 on AIX
7.2; its run ended when cfarm119's owner replaced cfarm119 with an AIX 7.3,
ibm-clang v17.1.1 machine.  Since 2015, hornet and mandrill have tested xlc12
on AIX 7.1.  That said, given your finding that later xlc versions have the
same code generation bug, the choice of version is a side issue.  A migration
to ibm-clang wouldn't have prevented this week's xlc-prompted commits.

I feel the gravity and longevity of xlc bugs has been out of proportion with
the compiler's contribution to PostgreSQL.  I would find it reasonable to
revoke xlc support in v17+, leaving AIX gcc support in place.  The main
contribution of AIX has been to find the bug behind commit a1b8aa1.  That
benefited from the AIX kernel, not from any particular compiler.  hornet and
mandrill would continue to test v16-.

By the way, I once tried to report an xlc bug.  Their system was tailored to
accept bugs from paid support customers only.  I submitted it via some sales
inquiry form, just in case, but never heard back.

  57. Re: Add support for AT LOCAL

    Robert Haas <robertmhaas@gmail.com> — 2023-10-19T14:38:14Z

    On Wed, Oct 18, 2023 at 7:33 PM Noah Misch <noah@leadboat.com> wrote:
> I feel the gravity and longevity of xlc bugs has been out of proportion with
> the compiler's contribution to PostgreSQL.  I would find it reasonable to
> revoke xlc support in v17+, leaving AIX gcc support in place.

+1 for this proposal. I just think this is getting silly. We're saying
that we only have access to 1 or 2 AIX machines, and most of us have
access to none, and the compiler has serious code generation bugs that
are present in both a release 11 years old and also a release current
release, meaning they went unfixed for 10 years, and we can't report
bugs or get them fixed when we find them, and the use of this
particular compiler in the buildfarm isn't finding any issues that
matter anywhere else.

To be honest, I'm not entirely sure that even AIX gcc support is
delivering enough value per unit work to justify keeping it around.
But the xlc situation is worse.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  58. Re: Add support for AT LOCAL

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-19T14:46:14Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Oct 18, 2023 at 7:33 PM Noah Misch <noah@leadboat.com> wrote:
>> I feel the gravity and longevity of xlc bugs has been out of proportion with
>> the compiler's contribution to PostgreSQL.  I would find it reasonable to
>> revoke xlc support in v17+, leaving AIX gcc support in place.

> +1 for this proposal.

WFM, too.

			regards, tom lane

  59. Re: Add support for AT LOCAL

    Andres Freund <andres@anarazel.de> — 2023-10-21T05:18:33Z

    Hi,

On 2023-10-19 10:38:14 -0400, Robert Haas wrote:
> On Wed, Oct 18, 2023 at 7:33 PM Noah Misch <noah@leadboat.com> wrote:
> > I feel the gravity and longevity of xlc bugs has been out of proportion with
> > the compiler's contribution to PostgreSQL.  I would find it reasonable to
> > revoke xlc support in v17+, leaving AIX gcc support in place.
> 
> +1 for this proposal. I just think this is getting silly. We're saying
> that we only have access to 1 or 2 AIX machines, and most of us have
> access to none, and the compiler has serious code generation bugs that
> are present in both a release 11 years old and also a release current
> release, meaning they went unfixed for 10 years, and we can't report
> bugs or get them fixed when we find them, and the use of this
> particular compiler in the buildfarm isn't finding any issues that
> matter anywhere else.

+1.


> To be honest, I'm not entirely sure that even AIX gcc support is
> delivering enough value per unit work to justify keeping it around.
> But the xlc situation is worse.

Agreed with both. If it were just a platform that didn't need special casing
in a bunch of places, it'd be one thing, but it's linkage model is so odd that
it makes no sense to keep AIX support around. But I'll take what I can get...

Greetings,

Andres Freund

  60. Re: Add support for AT LOCAL

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-21T05:27:58Z

    Andres Freund <andres@anarazel.de> writes:
> On 2023-10-19 10:38:14 -0400, Robert Haas wrote:
>> To be honest, I'm not entirely sure that even AIX gcc support is
>> delivering enough value per unit work to justify keeping it around.
>> But the xlc situation is worse.

> Agreed with both. If it were just a platform that didn't need special casing
> in a bunch of places, it'd be one thing, but it's linkage model is so odd that
> it makes no sense to keep AIX support around. But I'll take what I can get...

The other thread recently referred to:

https://www.postgresql.org/message-id/flat/20220702183354.a6uhja35wta7agew%40alap3.anarazel.de

was mostly about how AIX's choice that alignof(double) < alignof(int64)
breaks a whole bunch of assumptions in our code.  AFAICS we've done
nothing to resolve that, and nobody really wants to deal with it,
and there's no good reason to think that fixing it would improve
portability to any other platform.  So maybe there's an adequate
case for just nuking AIX support altogether?  I can't recall the
last time I saw a report from an actual AIX end user.

			regards, tom lane