Re: [COMMITTERS] pgsql: Sigh ...

Log Message:
-----------
Sigh ... pg_config.h.win32 needs to define BLCKSZ and RELSEG_SIZE now.

Modified Files:
--------------
pgsql/src/include:
pg_config.h.win32 (r1.53 -> r1.54)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/pg_config.h.win32?r1=1.53&r2=1.54)

Tom Lane wrote:
> Log Message:
> -----------
> Sigh ... pg_config.h.win32 needs to define BLCKSZ and RELSEG_SIZE now.
>

Tom,

this reminds me a bit of Talleyrand's reply to the beggar who said to
him "Monseigneur, il faut que je vive," ... "Je n'en vois pas la
nécessité.".

This fix is surely wrong for several reasons:
. the configure changes only broke MSVC builds, not all Windows builds
(see narwhal, for example), but this change applies to both.
. fixing a change that adds a configure option by hardcoding it in
pg_config.h.win32 is simply the wrong fix - the right fix is to add the
equivalent logic to src/tools/Solution.pm.

I don't mind if you ask someone (realistically that will usually be
Magnus or me) to unbreak MSVC builds due to a configure change, because
you are not set up to test it yourself. But I do mind the wrong solution
being applied just to unbreak the buildfarm.

I make corrective surgery in the morning.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> This fix is surely wrong for several reasons:
> . the configure changes only broke MSVC builds, not all Windows builds
> (see narwhal, for example), but this change applies to both.
> . fixing a change that adds a configure option by hardcoding it in
> pg_config.h.win32 is simply the wrong fix - the right fix is to add the
> equivalent logic to src/tools/Solution.pm.

Well, maybe the right answer is to take a step back and figure out what
pg_config.h.win32's excuse for living is at all. I do not understand
our Windows configuration setup, and what I do understand is is that
it's a pile of horrid kluges that break anytime anyone looks at them
sideways.

I will be quite happy to never touch any Windows configuration stuff
again. If you want me to, you had better redesign and/or document it
so that people other than you and Magnus have some idea of what connects
to what.

regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>> This fix is surely wrong for several reasons:
>> . the configure changes only broke MSVC builds, not all Windows builds
>> (see narwhal, for example), but this change applies to both.
>> . fixing a change that adds a configure option by hardcoding it in
>> pg_config.h.win32 is simply the wrong fix - the right fix is to add the
>> equivalent logic to src/tools/Solution.pm.
>>
>
> Well, maybe the right answer is to take a step back and figure out what
> pg_config.h.win32's excuse for living is at all. I do not understand
> our Windows configuration setup, and what I do understand is is that
> it's a pile of horrid kluges that break anytime anyone looks at them
> sideways.
>
> I will be quite happy to never touch any Windows configuration stuff
> again. If you want me to, you had better redesign and/or document it
> so that people other than you and Magnus have some idea of what connects
> to what.
>
>
>

Oops, the file is only used by MSVC/BCC, not by MinGW. Sorry for the
mistake about that.

However, all the values are hardcoded, so nothing in it should relate to
settings that come from configure, I believe. These should be dealt with
in src/tools/msvc/Solution.pm (mostly in GenerateFiles() ).

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Tom Lane wrote:
>> Well, maybe the right answer is to take a step back and figure out what
>> pg_config.h.win32's excuse for living is at all.

> Oops, the file is only used by MSVC/BCC, not by MinGW. Sorry for the
> mistake about that.

Well, I'm still wondering why it is an input for MSVC. If we have a
configure substitute for MSVC, why isn't it working off pg_config.h.in?

The original idea of pg_config.h.win32 was to support the pre-8.0
method for building native libpq.dll. I'd like to see us obsolete that
method altogether and get rid of pg_config.h.win32.

> However, all the values are hardcoded, so nothing in it should relate to
> settings that come from configure, I believe. These should be dealt with
> in src/tools/msvc/Solution.pm (mostly in GenerateFiles() ).

FYI, I'm about to commit changes moving XLOG_BLCKSZ and XLOG_SEG_SIZE
into the domain of configurable stuff, too.

regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>> Tom Lane wrote:
>>
>>> Well, maybe the right answer is to take a step back and figure out what
>>> pg_config.h.win32's excuse for living is at all.
>>>
>
>
>> Oops, the file is only used by MSVC/BCC, not by MinGW. Sorry for the
>> mistake about that.
>>
>
> Well, I'm still wondering why it is an input for MSVC. If we have a
> configure substitute for MSVC, why isn't it working off pg_config.h.in?
>
> The original idea of pg_config.h.win32 was to support the pre-8.0
> method for building native libpq.dll. I'd like to see us obsolete that
> method altogether and get rid of pg_config.h.win32.
>

That makes sense - I'll look at that after I have cleaned up the current
stuff.

>
>> However, all the values are hardcoded, so nothing in it should relate to
>> settings that come from configure, I believe. These should be dealt with
>> in src/tools/msvc/Solution.pm (mostly in GenerateFiles() ).
>>
>
> FYI, I'm about to commit changes moving XLOG_BLCKSZ and XLOG_SEG_SIZE
> into the domain of configurable stuff, too.
>
>
>

So I gathered. I'll pick those up as part of the fixes I'm currently coding.

cheers

andrew

Tom Lane wrote:
>> However, all the values are hardcoded, so nothing in it should relate to
>> settings that come from configure, I believe. These should be dealt with
>> in src/tools/msvc/Solution.pm (mostly in GenerateFiles() ).
>>
>
> FYI, I'm about to commit changes moving XLOG_BLCKSZ and XLOG_SEG_SIZE
> into the domain of configurable stuff, too.
>
>
>

This patch should fix things for both sets of changes. And it
demonstrates pretty much what you need to do for config options for MSVC.

If there's no objection I will apply shortly.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> ! print O "#define RELSEG_SIZE ",
> ! (1024 / $self->{options}->{blocksize}) *
> ! $self->{options}->{segsize} * 1024, "\n";

This doesn't look quite right; unless the arithmetic is being done in
floating point? I had it like this in configure.in:

RELSEG_SIZE=`expr '(' 1024 '*' ${segsize} / ${blocksize} ')' '*' 1024`

Also it looks like you missed adding segsize to the config.pl comments.

regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>> ! print O "#define RELSEG_SIZE ",
>> ! (1024 / $self->{options}->{blocksize}) *
>> ! $self->{options}->{segsize} * 1024, "\n";
>>
>
> This doesn't look quite right; unless the arithmetic is being done in
> floating point? I had it like this in configure.in:
>
> RELSEG_SIZE=`expr '(' 1024 '*' ${segsize} / ${blocksize} ')' '*' 1024`
>

blocksize is one of (1,2,4,8,16,32) so it should always be a factor of
1024 unless my arithmetic is awry. I did it that way because I dislike
expressions with unbracketed mixed operations - they make me think too
much.

> Also it looks like you missed adding segsize to the config.pl comments.
>
>
>

That's deliberate - we are currently only allowing a value of 1 here, so
I don't see any point in putting it in the sample config file, even as a
comment. When we enable other seg sizes we can add it to the sample file.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Tom Lane wrote:
>> This doesn't look quite right; unless the arithmetic is being done in
>> floating point? I had it like this in configure.in:
>>
>> RELSEG_SIZE=`expr '(' 1024 '*' ${segsize} / ${blocksize} ')' '*' 1024`

> blocksize is one of (1,2,4,8,16,32) so it should always be a factor of
> 1024 unless my arithmetic is awry. I did it that way because I dislike
> expressions with unbracketed mixed operations - they make me think too
> much.

Well, if you dislike the original on style grounds, you should change it
to match. Doing the same thing in two different ways in two places
isn't good.

>> Also it looks like you missed adding segsize to the config.pl comments.

> That's deliberate - we are currently only allowing a value of 1 here, so
> I don't see any point in putting it in the sample config file, even as a
> comment. When we enable other seg sizes we can add it to the sample file.

Ah, OK.

regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>> Tom Lane wrote:
>>
>>> This doesn't look quite right; unless the arithmetic is being done in
>>> floating point? I had it like this in configure.in:
>>>
>>> RELSEG_SIZE=`expr '(' 1024 '*' ${segsize} / ${blocksize} ')' '*' 1024`
>>>
>
>
>> blocksize is one of (1,2,4,8,16,32) so it should always be a factor of
>> 1024 unless my arithmetic is awry. I did it that way because I dislike
>> expressions with unbracketed mixed operations - they make me think too
>> much.
>>
>
> Well, if you dislike the original on style grounds, you should change it
> to match. Doing the same thing in two different ways in two places
> isn't good.
>

OK, done. Patch applied with that addition (it was time I deployed
autoconf 2.61 anyway).

cheers

andrew

Andrew Dunstan wrote:
> This patch should fix things for both sets of changes. And it
> demonstrates pretty much what you need to do for config options for MSVC.

Btw., it is quite easily possible to use the autom4te tracing facility to
parse the configure.ac file, in case you are interested in generating some of
the Windows build code automatically.

For example:

$ autoconf -t 'AC_ARG_ENABLE:$1' configure.in
integer-datetimes
nls
shared
rpath
spinlocks
debug
profiling
dtrace
segmented-files
depend
cassert
thread-safety
thread-safety
thread-safety-force
largefile

Let me know if you want to do something with that.

Peter Eisentraut wrote:
> Andrew Dunstan wrote:
>
>> This patch should fix things for both sets of changes. And it
>> demonstrates pretty much what you need to do for config options for MSVC.
>>
>
> Btw., it is quite easily possible to use the autom4te tracing facility to
> parse the configure.ac file, in case you are interested in generating some of
> the Windows build code automatically.
>
> For example:
>
> $ autoconf -t 'AC_ARG_ENABLE:$1' configure.in
> integer-datetimes
> nls
> shared
> rpath
> spinlocks
> debug
> profiling
> dtrace
> segmented-files
> depend
> cassert
> thread-safety
> thread-safety
> thread-safety-force
> largefile
>
> Let me know if you want to do something with that.
>
>

Yeah, maybe. Let's fix the issue pg_config.h.win32 that Tom raised first.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Peter Eisentraut wrote:
>> Btw., it is quite easily possible to use the autom4te tracing facility to
>> parse the configure.ac file, in case you are interested in generating some of
>> the Windows build code automatically.

> Yeah, maybe. Let's fix the issue pg_config.h.win32 that Tom raised first.

+1 for both. We really need to eliminate as much redundancy as we can
between the two build systems, because it'll keep biting us until we do.

regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> Peter Eisentraut wrote:
>>> Btw., it is quite easily possible to use the autom4te tracing facility to
>>> parse the configure.ac file, in case you are interested in generating some of
>>> the Windows build code automatically.
>
>> Yeah, maybe. Let's fix the issue pg_config.h.win32 that Tom raised first.
>
> +1 for both. We really need to eliminate as much redundancy as we can
> between the two build systems, because it'll keep biting us until we do.

+1 from here as well, if that wasn' obvious :-)

(will get back to the actual issues at hand when I get back in a more
connected mode in a couple of days, unless they are already solved by then)

//Magnus