Обсуждение: Patches applied; initdb time!

I've applied patches to implement an int64-based data/time storage
scheme. I've also accumulated some other minor fixes, which result in an
initdb being required (sorry!).

Note that the *default* timestamp type is now TIMESTAMP WITHOUT TIME
ZONE. This is what we discussed previously for the transition to SQL9x
compliance.

Full cvs log entry is included below.
                  - Thomas

Support alternate storage scheme of 64-bit integer for date/time types.Use "--enable-integer-datetimes" in
configurationto use this ratherthan the original float8 storage. I would recommend the integer-basedstorage for any
platformon which it is available. We perhaps shouldmake this the default for the production release.
 
Change timezone(timestamptz) results to return timestamp rather thana character string. Formerly, we didn't have a way
torepresenttimestamps with an explicit time zone other than freezing the info intoa string. Now, we can reasonably omit
theexplicit time zone from theresult and return a timestamp with values appropriate for the specifiedtime zone. Much
cleaner,and if you need the time zone in the resultyou can put it into a character string pretty easily anyway.
 
Allow fractional seconds in date/time types even for dates prior to 1BC.
Limit timestamp data types to 6 decimal places of precision. Just rightfor a micro-second storage of int8 date/time
types,and reduces thenumber of places ad-hoc rounding was occuring for the float8-based
 
types.
Use lookup tables for precision/rounding calculations for timestamp andinterval types.  Formerly used pow() to
calculatethe desired value butwith a more limited range there is no reason to not type in a lookuptable. Should be
*much*better performance, though formerly there weresome optimizations to help minimize the number of times pow() was
 
called.
Define a HAVE_INT64_TIMESTAMP variable. Based on the configure option"--enable-integer-datetimes" and the existing
internalINT64_IS_BUSTED.
 
Add explicit date/interval operators and functions for addition andsubtraction. Formerly relied on implicit type
promotionfrom date totimestamp with time zone.
 
Change timezone conversion functions for the timetz type from "timetz()"to "timezone()". This is consistant with other
timezone coersionfunctions for other types.
 
Bump the catalog version to 200204201.
Fix up regression tests to reflect changes in fractional secondsrepresentation for date/times in BC eras.
All regression tests pass on my Linux box.

btw, I've updated the regression tests and results for my platform, but
other platforms (e.g. Solaris) will need their results files updated...
                  - Thomas

I'm seeing half a dozen gcc warnings as a result of these patches.
Do you want to fix 'em, or shall I?
        regards, tom lane

> I'm seeing half a dozen gcc warnings as a result of these patches.
> Do you want to fix 'em, or shall I?

Where are they? I haven't noticed anything in the files I have changes;
are the warnings elsewhere?
                    - Thomas

Thomas Lockhart <thomas@fourpalms.org> writes:
>> I'm seeing half a dozen gcc warnings as a result of these patches.
>> Do you want to fix 'em, or shall I?

> Where are they?

With fairly vanilla configure options, I get

make[3]: Entering directory `/home/postgres/pgsql/src/backend/parser'
gcc -O1 -Wall -Wmissing-prototypes -Wmissing-declarations -g -I../../../src/include   -c -o gram.o gram.c
gram.y:6688: warning: `set_name_needs_quotes' defined but not used

make[3]: Entering directory `/home/postgres/pgsql/src/backend/commands'
gcc -O1 -Wall -Wmissing-prototypes -Wmissing-declarations -g -I../../../src/include   -c -o sequence.o sequence.c
In file included from sequence.c:25:
../../../src/include/utils/int8.h:33: warning: `INT64CONST' redefined
../../../src/include/utils/pg_crc.h:83: warning: this is the location of the previous definition
gcc -O1 -Wall -Wmissing-prototypes -Wmissing-declarations -g -I../../../src/include   -c -o variable.o variable.c
variable.c: In function `parse_datestyle':
variable.c:262: warning: `rstat' might be used uninitialized in this function
variable.c:264: warning: `value' might be used uninitialized in this function

make[4]: Entering directory `/home/postgres/pgsql/src/backend/utils/adt'
gcc -O1 -Wall -Wmissing-prototypes -Wmissing-declarations -g -I../../../../src/include   -c -o selfuncs.o selfuncs.c
In file included from selfuncs.c:95:
../../../../src/include/utils/int8.h:33: warning: `INT64CONST' redefined
../../../../src/include/utils/pg_crc.h:83: warning: this is the location of the previous definition

Seems not good to have INT64CONST separately defined in int8.h and 
pg_crc.h.  Offhand I'd either move it into c.h, or else consider that
int8.h is the Right Place for it and make pg_crc.h include int8.h.
        regards, tom lane

> > I'm seeing half a dozen gcc warnings as a result of these patches.
> Where are they?

More specifically, the *only* compiler warning I see (other than the
usual yacc/lex symbol warnings) is that a routine in gram.y,
set_name_needs_quotes(), is defined but not used. Don't know where that
routine came from, and afaik I didn't accidentally remove a reference
when trying to merge changes...
                  - Thomas

> With fairly vanilla configure options, I get...

Please be specific on the options and platform. I do *not* see these
warnings here with my "fairly vanilla configure options" ;)

Can't fix what I can't see, and we should track down what interactions
are happening to get these variables exposed...

btw, the INT64CONST must be defined for int8 (which is where I get the
definition for the date/time stuff); not sure why it appears in two
separate places and not sure why my compiler (gcc-2.96.xxx) does not
notice it.
                 - Thomas

Thomas Lockhart <thomas@fourpalms.org> writes:
> btw, I've updated the regression tests and results for my platform, but
> other platforms (e.g. Solaris) will need their results files updated...

I committed a fix for HPUX's horology file, and did some extrapolation
to produce a Solaris version; someone please verify that it's OK.
        regards, tom lane

Thomas Lockhart <thomas@fourpalms.org> writes:
>> With fairly vanilla configure options, I get...
> Please be specific on the options and platform.

HPUX 10.20,

./configure --with-CXX --with-tcl --enable-cassert
        regards, tom lane

Thomas Lockhart wrote:
>>With fairly vanilla configure options, I get...
> 
> 
> Please be specific on the options and platform. I do *not* see these
> warnings here with my "fairly vanilla configure options" ;)
> 
> Can't fix what I can't see, and we should track down what interactions
> are happening to get these variables exposed...
> 
> btw, the INT64CONST must be defined for int8 (which is where I get the
> definition for the date/time stuff); not sure why it appears in two
> separate places and not sure why my compiler (gcc-2.96.xxx) does not
> notice it.
> 

I just built from cvs tip using:
./configure --enable-integer-datetimes --enable-locale  --enable-debug 
--enable-cassert --enable-multibyte --enable-syslog --enable-nls 
--enable-depend

and got:

gram.y:6688: warning: `set_name_needs_quotes' defined but not used

variable.c: In function `parse_datestyle':
variable.c:262: warning: `rstat' might be used uninitialized in this 
function
variable.c:264: warning: `value' might be used uninitialized in this 
function

-- and the usual lexer related warnings --

pgc.c: In function `yylex':
pgc.c:1249: warning: label `find_rule' defined but not used
pgc.l: At top level:
pgc.c:3073: warning: `yy_flex_realloc' defined but not used
and

pl_scan.c: In function `plpgsql_base_yylex':
pl_scan.c:1020: warning: label `find_rule' defined but not used
scan.l: At top level:
pl_scan.c:2321: warning: `yy_flex_realloc' defined but not used

but did *not* get the INT64CONST warning that Tom did. I'm using an 
updated Red Hat 7.2 box.

HTH,

Joe

Thomas Lockhart <thomas@fourpalms.org> writes:
> More specifically, the *only* compiler warning I see (other than the
> usual yacc/lex symbol warnings) is that a routine in gram.y,
> set_name_needs_quotes(), is defined but not used. Don't know where that
> routine came from, and afaik I didn't accidentally remove a reference
> when trying to merge changes...

Yeah, you did.  However the routine could possibly go away now.
It was a hack I put in recently to handle cases like

regression=# create schema "MySchema";
CREATE
regression=# create schema "MyOtherSchema";
CREATE
regression=# set search_path TO "MySchema", "MyOtherSchema";
ERROR:  SET takes only one argument for this parameter

Formerly gram.y merged the list items into a single string, and so it
needed to double-quote mixed-case names to prevent case folding when
the string got re-parsed later.

This example worked last week, and probably would work again if the
system were applying your new list-argument logic for search_path ...
but I'm not sure where to look to learn about that.
        regards, tom lane

> >> With fairly vanilla configure options, I get...
> > Please be specific on the options and platform.
> HPUX 10.20,
> ./configure --with-CXX --with-tcl --enable-cassert

Boy, how plain-vanilla. *My* configure line is all of

./configure --prefix=/home/thomas/local

But I do override some parameters in my Makefile.custom:

CFLAGS+= -g -O0 -DUSE_ASSERT_CHECKING
CFLAGS+= -DCOPY_PARSE_PLAN_TREES

Which gives me (except for the plan tree thing) something very similar.

I've looked a bit more, and the set_name_needs_quotes() is probably
obsoleted by my update, which generalizes parameter handling in SET
variables. I'll rip it out unless we get a test case in the regression
tests which demonstrates a problem. I'm pretty sure that it may have
allowed 
 SET key='par1 w space,par2';

but that would be handled now by
 SET key='par1 w space',par2;

for cases in which "key" would accept multiple values. We now can allow
single parameters with embedded commas *and* whitespace, which would
have been impossible before. Not sure why white space is desirable
however, so the new behavior seems adequate to me.

I'm still not sure why the INT64CONST conflict does not show up as a
warning on my machine, but looking at the code I'm not sure why we would
ever have had two versions in the first place. Anyone want to take
responsibility for consolidating it into The Right Place? If not, I'll
go ahead and do it...
                   - Thomas

Joe Conway <mail@joeconway.com> writes:
> but did *not* get the INT64CONST warning that Tom did. I'm using an 
> updated Red Hat 7.2 box.

Probably it depends on compiler version?  I'm using gcc 2.95.3.
        regards, tom lane

Thomas Lockhart <thomas@fourpalms.org> writes:
> I'm still not sure why the INT64CONST conflict does not show up as a
> warning on my machine, but looking at the code I'm not sure why we would
> ever have had two versions in the first place. Anyone want to take
> responsibility for consolidating it into The Right Place? If not, I'll
> go ahead and do it...

I think it was originally needed only for the CRC code, so we put it
there to begin with.  Clearly should be in a more widely used place now.
Do you have any opinion whether c.h or int8.h is the Right Place?
I'm still dithering about that.
        regards, tom lane

Tom Lane wrote:
> Joe Conway <mail@joeconway.com> writes:
> 
>>but did *not* get the INT64CONST warning that Tom did. I'm using an 
>>updated Red Hat 7.2 box.
> 
> 
> Probably it depends on compiler version?  I'm using gcc 2.95.3.
> 

could be:
[postgres@jec-linux pgsql]$ gcc -v
Reading specs from /usr/lib/gcc-lib/i386-redhat-linux/2.96/specs
gcc version 2.96 20000731 (Red Hat Linux 7.1 2.96-98)

Joe

> I think it was originally needed only for the CRC code, so we put it
> there to begin with.  Clearly should be in a more widely used place now.
> Do you have any opinion whether c.h or int8.h is the Right Place?
> I'm still dithering about that.

In looking at the code, istm that the versions should be merged with
features from both. The generated constants should be surrounded in
parens, but the explicit coersion to (int64) should be omitted at least
with the "LL" version.

I've got some other "int64" pushups to worry about; let's try fixing
those too (though afaict they may need to happen in different places).
At the moment, we have INT64_IS_BUSTED as an amalgam of other conditions
or undefined variables. I've also got a HAVE_INT64_TIMESTAMP which comes
from a configured variable USE_INTEGER_DATETIMES and an undefined
INT64_IS_BUSTED. This is now housed in c.h, but istm that we *should*
check for conflicting settings in configure itself, and carry forward a
consistant set of parameters from there.

Anyway, at the moment some of this stuff is in c.h, and that is probably
the right place to put the INT64CONST definitions, at least until things
sort out differently.

btw, I've updated gram.y and variable.c to suppress the reported
warnings (which I *still* don't see here; that is very annoying).
                  - Thomas

Thomas Lockhart wrote:
> 
> btw, I've updated gram.y and variable.c to suppress the reported
> warnings (which I *still* don't see here; that is very annoying).
> 

FWIW, I'm still seeing:
gram.y:99: warning: `set_name_needs_quotes' declared `static' but never 
defined

Joe

> FWIW, I'm still seeing:
> gram.y:99: warning: `set_name_needs_quotes' declared `static' but never
> defined

Ack. Sloppy patching. Should be fixed now...
                 - Thomas

Thomas Lockhart writes:

> But I do override some parameters in my Makefile.custom:
>
> CFLAGS+= -g -O0 -DUSE_ASSERT_CHECKING

If you use -O0 then you miss most of the interesting warnings.

-- 
Peter Eisentraut   peter_e@gmx.net

> > But I do override some parameters in my Makefile.custom:
> > CFLAGS+= -g -O0 -DUSE_ASSERT_CHECKING
> If you use -O0 then you miss most of the interesting warnings.

?? Not in this case. afaik -O0 suppresses most optimizations (and hence
does not reorder instructions, which is why I use it for debugging; I
know, debuggers nowadays work pretty well even with instruction
reordering, but...).

Anyway, compiling with "-O2" on variable.c still does not show the
warnings with my 2.96.x compiler...
                 - Thomas

Thomas Lockhart <thomas@fourpalms.org> writes:
> But I do override some parameters in my Makefile.custom:
> CFLAGS+= -g -O0 -DUSE_ASSERT_CHECKING
>> If you use -O0 then you miss most of the interesting warnings.

> ?? Not in this case. afaik -O0 suppresses most optimizations

In particular, you don't get "unused variable" and "variable may not
have been set before being used" warnings at -O0, because the
control-flow analysis needed to emit those warnings is not done at -O0.

I generally use -O1 for development; it's sometimes a little hairy
stepping through the generated code, but usually gcc works well enough
at -O1, and I get the important warnings.
        regards, tom lane

Thomas Lockhart wrote:
>>FWIW, I'm still seeing:
>>gram.y:99: warning: `set_name_needs_quotes' declared `static' but never
>>defined
> 
> 
> Ack. Sloppy patching. Should be fixed now...
> 
>                   - Thomas

Yup, did the trick.

Thanks,

Joe

...
> In particular, you don't get "unused variable" and "variable may not
> have been set before being used" warnings at -O0, because the
> control-flow analysis needed to emit those warnings is not done at -O0.

Right. The point is that I don't get those (apparently) with -O2 either,
with my particular compiler. Hmm. Actually, I *do* get those if I make
sure that some of the other options are set too; my quick test added -O2
but left out some of the -w switches. OK, never mind...
                     - Thomas

Thomas Lockhart wrote:

>>>But I do override some parameters in my Makefile.custom:
>>>CFLAGS+= -g -O0 -DUSE_ASSERT_CHECKING
>>>
>>If you use -O0 then you miss most of the interesting warnings.
>>
>
>?? Not in this case. afaik -O0 suppresses most optimizations (and hence
>does not reorder instructions, which is why I use it for debugging; I
>know, debuggers nowadays work pretty well even with instruction
>reordering, but...).
>
>Anyway, compiling with "-O2" on variable.c still does not show the
>warnings with my 2.96.x compiler...
>
It's actually the optimiser that allows a large number of the warnings 
to be uncovered.  It generates extra code-path and coverage information, 
as well as other things, that are needed for the guts of GCC to squawk 
about a number of odd behaviours.

> Right. The point is that I don't get those (apparently) with -O2 either,
> with my particular compiler. Hmm. Actually, I *do* get those if I make
> sure that some of the other options are set too; my quick test added -O2
> but left out some of the -w switches. OK, never mind...

btw, now that I've started using "-O2", my geometry regression test now
passes as though it were the "standard linux result". It's been a *long*
time since that test passed for me, which probably says that it has been
quite a while since I didn't force a "-O0"...
                 - Thomas