Re: pgindent run coming

It is time to run pgindent on CVS HEAD for 8.4. I am thinking of
running it at zero-hour GMT tomorrow, meaning five hours from now.
Any objections?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> It is time to run pgindent on CVS HEAD for 8.4. I am thinking of
> running it at zero-hour GMT tomorrow, meaning five hours from now.
> Any objections?

I ran pgindent and was concerned enough about the results so I am
posting here rather than applying any changes. I used the old way of
generating typedefs and the new buildfarm method output from diffs from
http://www.pgbuildfarm.org/cgi-bin/typedefs.pl. The typedef lists and
diff are here:

http://momjian.us/expire/pgindent/

You can see the typedef lists are of similar size:

2775 typedefs.old
2123 typedefs.new

and the diffs generates are a similar number of lines:

133657 diff.old_typedefs
135042 diff.new_typedefs

I saw a few odd things. Most importantly, it seems 'stat' was
introduced as a typedef on _both_ lists, yielding weird changes like:

- ret = stat(indir, &st);
+ ret = stat (indir, &st);

and even odder:

- stat->weight |= 1 << 3;
+ stat ->weight |= 1 << 3;

stat was not a typedef in 8.3 or pgindent would have done this for 8.3,
but I can't figure out what has changed to make it appear for 8.4. I
see this in the objdump output (my OS has not changed from 8.3):

31357 EXCL 0 0 00003e64 97648 /usr/include/time.h
31358 EXCL 0 0 00007638 97624 /usr/include/sys/time.h
31359 EXCL 0 0 00000000 97648 /usr/include/time.h
31360 EXCL 0 0 00001dff 25540 /usr/include/fcntl.h
31361 BINCL 0 0 000144be 449667 /usr/include/sys/stat.h

31362 LSYM 0 0 00000000 449691 ostat:T(51,1)=s64st_dev:(0,9),0,16;st_ino:(9,20),32,32;st_mode:(9,22),64,16;st_nlink:(9,23),80,16;st_u
id:(0,9),96,16;st_gid:(0,9),112,16;st_rdev:(0,9),128,16;st_size:(0,3),160,32;st_atimespec:(48,2),192,64;st_mtimespec:(48,2),256,64;st_ctimespec:(4
8,2),320,64;st_blksize:(0,3),384,32;st_blocks:(0,3),416,32;st_flags:(0,5),448,32;st_gen:(0,5),480,32;;

31363 LSYM 0 0 00000000 450042 stat32:T(51,2)=s96st_dev:(9,17),0,32;st_ino:(9,20),32,32;st_mode:(9,22),64,16;st_nlink:(9,23),80,16;st
_uid:(9,28),96,32;st_gid:(9,19),128,32;st_rdev:(9,17),160,32;st_atimespec:(48,2),192,64;st_mtimespec:(48,2),256,64;st_ctimespec:(48,2),320,64;st_s
ize:(0,3),384,32;st_size1:(0,3),416,32;st_blocks:(0,3),448,32;st_blocks1:(0,3),480,32;st_blksize:(0,5),512,32;st_flags:(0,5),544,32;st_gen:(0,5),5
76,32;st_lspare:(0,3),608,32;st_qspare:(51,3)=ar(0,1);0;3;(0,3),640,128;;

--> 31364 LSYM 0 0 00000000 450510 stat:T(51,4)=s96st_dev:(9,17),0,32;st_ino:(9,20),32,32;st_mode:(9,22),64,16;st_nlink:(9,23),80,16;st_u
id:(9,28),96,32;st_gid:(9,19),128,32;st_rdev:(9,17),160,32;st_atimespec:(48,2),192,64;st_mtimespec:(48,2),256,64;st_ctimespec:(48,2),320,64;st_siz
e:(9,24),384,64;st_blocks:(9,8),448,64;st_blksize:(0,5),512,32;st_flags:(0,5),544,32;st_gen:(0,5),576,32;st_lspare:(0,3),608,32;st_qspare:(51,5)=a
r(0,1);0;1;(9,8),640,128;;

It is coming from the postgres binary.

The typedef is coming from the indicated line, and from
/usr/include/sys/stat.h, where there is no typedef for stat. Obviously
Linux or the buildfarm is finding the same issue, but I have no idea
why.

My only guess right now is that we are linking postgres differently than
we did for 8.3 and that is bringing in new wrong typedef symbols.

I will have to research this further tomorrow.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> The typedef is coming from the indicated line, and from
> /usr/include/sys/stat.h, where there is no typedef for stat. Obviously
> Linux or the buildfarm is finding the same issue, but I have no idea
> why.
>
> My only guess right now is that we are linking postgres differently than
> we did for 8.3 and that is bringing in new wrong typedef symbols.
>
> I will have to research this further tomorrow.

I was able to reproduce the incorrect stat typedef here in a small test
program by just including "sys/stat.h" so I will research tomorrow how
to fix this.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> I saw a few odd things. Most importantly, it seems 'stat' was
> introduced as a typedef on _both_ lists, yielding weird changes like:

The standard headers do define "struct stat". I wonder whether the
objdump kluge we are using is unable to distinguish typedef names
from struct tags.

> I will have to research this further tomorrow.

We don't have a lot of time for research. Maybe the best thing is to
just manually remove stat from the typedef list (along with anything
else that clearly shouldn't be there)?

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > I saw a few odd things. Most importantly, it seems 'stat' was
> > introduced as a typedef on _both_ lists, yielding weird changes like:
>
> The standard headers do define "struct stat". I wonder whether the
> objdump kluge we are using is unable to distinguish typedef names
> from struct tags.
>
> > I will have to research this further tomorrow.
>
> We don't have a lot of time for research. Maybe the best thing is to
> just manually remove stat from the typedef list (along with anything
> else that clearly shouldn't be there)?

The problem is that there are other symbols I don't know about and the
diff is very large. I have found that the problem was caused when we
added Linux support to find_typedef and I have a way to get an accurate
list on my machine.

I will generate a proper list on my machine tomorrow then test Linux
here to see if I can get it to generate the right list too. But odds
are we are not going to have time to re-run the list on the build farm
even if I can get Linux working here.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > I saw a few odd things. Most importantly, it seems 'stat' was
> > introduced as a typedef on _both_ lists, yielding weird changes like:
>
> The standard headers do define "struct stat". I wonder whether the
> objdump kluge we are using is unable to distinguish typedef names
> from struct tags.
>
> > I will have to research this further tomorrow.
>
> We don't have a lot of time for research. Maybe the best thing is to
> just manually remove stat from the typedef list (along with anything
> else that clearly shouldn't be there)?

Do you want me to just run with my old typedef list now and apply it?
We an always rerun tomorrow if we get a better typedef list.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Tom Lane wrote:
>> We don't have a lot of time for research. Maybe the best thing is to
>> just manually remove stat from the typedef list (along with anything
>> else that clearly shouldn't be there)?

> Do you want me to just run with my old typedef list now and apply it?
> We an always rerun tomorrow if we get a better typedef list.

I'd rather have *one* run with the final typedef list. If you don't
have that list yet, wait till you do.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Tom Lane wrote:
> >> We don't have a lot of time for research. Maybe the best thing is to
> >> just manually remove stat from the typedef list (along with anything
> >> else that clearly shouldn't be there)?
>
> > Do you want me to just run with my old typedef list now and apply it?
> > We an always rerun tomorrow if we get a better typedef list.
>
> I'd rather have *one* run with the final typedef list. If you don't
> have that list yet, wait till you do.

OK, Andrew, would you use the find_typedef file that is in CVS HEAD and
run that. I think that will fix our problem and then I can use the
buildfarm version. How often does that run and does it pull the script
from CVS HEAD?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> OK, Andrew, would you use the find_typedef file that is in CVS HEAD and
> run that. I think that will fix our problem and then I can use the
> buildfarm version. How often does that run and does it pull the script
> from CVS HEAD?
>
>

The buildfarm does not run the find-typedefs script. Its code for this
is below. My Unix machine runs this once a day. I can do runs on Windows
and Cygwin manually.

If there is changed logic tell me what it is and I'll try to get it done.

cheers

andrew

-----------------
sub find_typedefs
{
my @err = `objdump -W 2>&1`;
@err = () if `uname -s 2>&1` =~ /CYGWIN/i;
my %syms;
my @dumpout;
my @flds;
foreach my $bin (glob("$installdir/bin/*"),
glob("$installdir/lib/*"),
glob("$installdir/lib/postgresql/*"))
{
next if $bin =~ m!bin/(ipcclean|pltcl_)!;
next unless -f $bin;
if (@err == 1) # Linux
{
@dumpout = `objdump -W $bin 2>/dev/null | egrep -A3
'(DW_TAG_typedef|DW_TAG_structure_type|DW_TAG_union_type)' 2>/dev/null`;
foreach (@dumpout)
{
@flds = split;
next if (($flds[0] ne 'DW_AT_name' && $flds[1] ne 'DW_AT_name' ) ||
$flds[-1] =~ /^DW_FORM_str/);
$syms{$flds[-1]} =1;
}
}
else
{
@dumpout = `objdump --stabs $bin 2>/dev/null`;
foreach (@dumpout)
{
@flds = split;
next if (@flds < 7);
next if ($flds[1] ne 'LSYM' || $flds[6] !~ /([^:]+):[tT]/);
$syms{$1} =1;
}
}
}
my @badsyms = grep { /\s/ } keys %syms;
push(@badsyms,'date','interval','timestamp','ANY');
delete @syms{(at)badsyms};

my @goodsyms = sort keys %syms;
my @foundsyms;

my %foundwords;

my $setfound = sub
{
return unless (-f $_ && /^.*\.[chly]\z/);
my @lines;
my $handle;
open ($handle,$_);
while (my $line=<$handle>)
{
foreach my $word (split(/\W+/,$line))
{
$foundwords{$word} = 1;
}
}
close($handle);
};

File::Find::find($setfound,"$branch_root/pgsql");

foreach my $sym (@goodsyms)
{
push(@foundsyms,"$sym\n") if exists $foundwords{$sym};
}

writelog('typedefs',\(at)foundsyms);
$steps_completed .= " find-typedefs";
}

On Tue, 2009-06-09 at 13:21 -0400, Bruce Momjian wrote:

> It is time to run pgindent on CVS HEAD for 8.4. I am thinking of
> running it at zero-hour GMT tomorrow, meaning five hours from now.
> Any objections?

Why don't we do this automatically after each individual commit? That
way each commit would be associated directly with any required tidy-up
that must occur because of it.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Andrew Dunstan wrote:
>
>
> Bruce Momjian wrote:
> > OK, Andrew, would you use the find_typedef file that is in CVS HEAD and
> > run that. I think that will fix our problem and then I can use the
> > buildfarm version. How often does that run and does it pull the script
> > from CVS HEAD?
> >
> >
>
> The buildfarm does not run the find-typedefs script. Its code for this
> is below. My Unix machine runs this once a day. I can do runs on Windows
> and Cygwin manually.
>
> If there is changed logic tell me what it is and I'll try to get it done.

Yes, please make the attached change, and rerun the script. This change
reflects the changes I made in src/tools/find_typedef.

What value is there if the URL I have just runs on Linux? It probably
has the same coverage I have for BSD. I asked for something that was
automated for more platforms a year ago and I still don't have it? I
might as well just use the same method I have have used for years.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On Tue, 2009-06-09 at 13:21 -0400, Bruce Momjian wrote:
>> It is time to run pgindent on CVS HEAD for 8.4. I am thinking of
>> running it at zero-hour GMT tomorrow, meaning five hours from now.

> Why don't we do this automatically after each individual commit?

It's not very practical until the typedef extraction stuff is fully
automatic and fully trustworthy; right now some manual review and
intervention still seems like a good idea.

Also, that would risk breaking series of interdependent patches.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > I saw a few odd things. Most importantly, it seems 'stat' was
> > introduced as a typedef on _both_ lists, yielding weird changes like:
>
> The standard headers do define "struct stat". I wonder whether the
> objdump kluge we are using is unable to distinguish typedef names
> from struct tags.
>
> > I will have to research this further tomorrow.
>
> We don't have a lot of time for research. Maybe the best thing is to
> just manually remove stat from the typedef list (along with anything
> else that clearly shouldn't be there)?

I agree we are running out of time so I will be running pgindent in one
hour using the same BSD-based typedef method I have used in the past.
If a better typedef list appears, we can always rerun pgindent.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Tom Lane wrote:
>> We don't have a lot of time for research. Maybe the best thing is to
>> just manually remove stat from the typedef list (along with anything
>> else that clearly shouldn't be there)?

> I agree we are running out of time so I will be running pgindent in one
> hour using the same BSD-based typedef method I have used in the past.
> If a better typedef list appears, we can always rerun pgindent.

Why not compare the BSD list with the new one and adopt anything that
seems sane? We know that your old method misses quite a lot of typedefs.

regards, tom lane

Bruce Momjian wrote:
> Andrew Dunstan wrote:
>
>> Bruce Momjian wrote:
>>
>>> OK, Andrew, would you use the find_typedef file that is in CVS HEAD and
>>> run that. I think that will fix our problem and then I can use the
>>> buildfarm version. How often does that run and does it pull the script
>>> from CVS HEAD?
>>>
>>>
>>>
>> The buildfarm does not run the find-typedefs script. Its code for this
>> is below. My Unix machine runs this once a day. I can do runs on Windows
>> and Cygwin manually.
>>
>> If there is changed logic tell me what it is and I'll try to get it done.
>>
>
> Yes, please make the attached change, and rerun the script. This change
> reflects the changes I made in src/tools/find_typedef.
>
> What value is there if the URL I have just runs on Linux? It probably
> has the same coverage I have for BSD. I asked for something that was
> automated for more platforms a year ago and I still don't have it? I
> might as well just use the same method I have have used for years.
>
>

Well, sometimes I build it and they don't come ;-).

I don't have every platform under the sun that I can run this on,
although I do now have an FBSD VM that I didn't have when I worked on
this previously. If you're actually going to use it I'll set it up as a
buildfarm member and run the find-typedefs there.

I will make the change you request and rerun the stuff on the platforms
I have that are currently buildfarm members, but AFAICT the Linux output
will still include the stat symbol (your change doesn't affect the Linux
branch at all).

Both my version and yours contains provision for filtering out certain
symbols ('timestamp' etc). Maybe we need to add 'stat' to that list.

cheers

andrew
> ------------------------------------------------------------------------
>
> *** ./script.pl.orig Wed Jun 10 09:39:27 2009
> --- ./script.pl Wed Jun 10 09:39:39 2009
> ***************
> *** 30,36 ****
> {
> @flds = split;
> next if (@flds < 7);
> ! next if ($flds[1] ne 'LSYM' || $flds[6] !~ /([^:]+):[tT]/);
> $syms{$1} =1;
> }
> }
> --- 30,36 ----
> {
> @flds = split;
> next if (@flds < 7);
> ! next if ($flds[1] ne 'LSYM' || $flds[6] !~ /([^:]+):t/);
> $syms{$1} =1;
> }
> }
>

Andrew Dunstan wrote:
> Well, sometimes I build it and they don't come ;-).
>
> I don't have every platform under the sun that I can run this on,
> although I do now have an FBSD VM that I didn't have when I worked on
> this previously. If you're actually going to use it I'll set it up as a
> buildfarm member and run the find-typedefs there.
>
> I will make the change you request and rerun the stuff on the platforms
> I have that are currently buildfarm members, but AFAICT the Linux output
> will still include the stat symbol (your change doesn't affect the Linux
> branch at all).

Good point. Here is another diff I need you to make to the pl file.
If you want to make your pl file the official version and replace the
shell script in CVS, that is fine with me. Do you want me to do that?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
>
> Good point. Here is another diff I need you to make to the pl file.
>

Done. Linux run under way.

> If you want to make your pl file the official version and replace the
> shell script in CVS, that is fine with me. Do you want me to do that?
>
>

It needs to be done in two pieces, I think: a perl module that exports a
function that can be called from the buildfarm, and program that calls
it so it can be run standalone. The function needs two inputs: an
installation directory and a source directory. Then we will be able to
stay in sync nicely. I will try to do that over the next day or so if
you like.

Or we can just wing it for now and do this after the release.

cheers

andrew

Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > > I saw a few odd things. Most importantly, it seems 'stat' was
> > > introduced as a typedef on _both_ lists, yielding weird changes like:
> >
> > The standard headers do define "struct stat". I wonder whether the
> > objdump kluge we are using is unable to distinguish typedef names
> > from struct tags.
> >
> > > I will have to research this further tomorrow.
> >
> > We don't have a lot of time for research. Maybe the best thing is to
> > just manually remove stat from the typedef list (along with anything
> > else that clearly shouldn't be there)?
>
> The problem is that there are other symbols I don't know about and the
> diff is very large. I have found that the problem was caused when we
> added Linux support to find_typedef and I have a way to get an accurate
> list on my machine.

OK, I have found the cause of the script error, and it was my fault. A
month after we ran pgindent for 8.3 (December 2007), I received this
issue from Tom:

http://archives.postgresql.org/pgsql-hackers/2007-12/msg00800.php
> Something I noticed the other day is that pgindent doesn't seem to treat
> "struct foo" or "union foo" as a type name, which is pretty silly
> because no context is needed at all to recognize that. We tend not to
> do that too much --- the project style is to use a typedef name --- but
> there are some places that do it, particularly the regex code. For
> instance there are extra spaces here:
>
> static void
> cmtreefree(struct colormap * cm,
> union tree * tree,
> int level) /* level number (top == 0) of this
> block */
> {
>
> Fixable?

Not understanding the ramifications of adding struct and union tags to
the typedef list, I modified the BSD code:

http://archives.postgresql.org/pgsql-hackers/2007-12/msg00810.php
> Yes, I found those are 't' STABS rather than "T" which are used in cases
> where you do typedef struct {} name. The next pgindent will have those
> typedefs you want.

and that modification was propogated to the Linux code.

This has now been fixed in the BSD and Linux code (and Perl script) and
we can move forward with running pgindent once Andrew has lists for all
the platforms he wants.

As for Tom's original complaint about, it seems BSD indent is just not
smart enough about struct/union tags. I will look into fixing that
after 8.4 pgindent is run.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Andrew Dunstan wrote:
>
>
> Bruce Momjian wrote:
> >
> > Good point. Here is another diff I need you to make to the pl file.
> >
>
> Done. Linux run under way.
>
> > If you want to make your pl file the official version and replace the
> > shell script in CVS, that is fine with me. Do you want me to do that?
> >
> >
>
> It needs to be done in two pieces, I think: a perl module that exports a
> function that can be called from the buildfarm, and program that calls
> it so it can be run standalone. The function needs two inputs: an
> installation directory and a source directory. Then we will be able to
> stay in sync nicely. I will try to do that over the next day or so if
> you like.
>
> Or we can just wing it for now and do this after the release.

We have to wing it for now. Please let me know when whatever lists you
can generate are ready, hopefully in the next few hours.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> OK, I have found the cause of the script error, and it was my fault. A
> month after we ran pgindent for 8.3 (December 2007), I received this
> issue from Tom:

> http://archives.postgresql.org/pgsql-hackers/2007-12/msg00800.php
>> Something I noticed the other day is that pgindent doesn't seem to treat
>> "struct foo" or "union foo" as a type name, which is pretty silly
>> because no context is needed at all to recognize that.

Ah. So really the point here is that we want to specifically exclude
"struct stat" because there are too many places in our code where "stat"
is used as a regular identifier. Are there any other special cases like
that?

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > OK, I have found the cause of the script error, and it was my fault. A
> > month after we ran pgindent for 8.3 (December 2007), I received this
> > issue from Tom:
>
> > http://archives.postgresql.org/pgsql-hackers/2007-12/msg00800.php
> >> Something I noticed the other day is that pgindent doesn't seem to treat
> >> "struct foo" or "union foo" as a type name, which is pretty silly
> >> because no context is needed at all to recognize that.
>
> Ah. So really the point here is that we want to specifically exclude
> "struct stat" because there are too many places in our code where "stat"
> is used as a regular identifier. Are there any other special cases like
> that?

Yep, lots. I see "option" also doing strange things, and some others.
You can see the diff here:

http://momjian.us/expire/pgindent/http://momjian.us/expire/pgindent/

Basically that list is meant for typedefs, not struct or union tags.
The BSD indent manual page says:

-Ttypename Adds typename to the list of type keywords. Names accu-
mulate: -T can be specified more than once. You need to
specify all the typenames that appear in your program
that are defined by typedef - nothing will be harmed if
you miss a few, but the program won't be formatted as
nicely as it should. This sounds like a painful thing to
have to do, but it's really a symptom of a problem in C:
typedef causes a syntactic change in the language and
indent can't find all instances of typedef.

I am unclear why struct pointers are not being formatted properly in
function headers but will research it.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> I am unclear why struct pointers are not being formatted properly in
> function headers but will research it.

Yeah, if we can fix that directly without adding the names to the
typedef list, it would be better. But not something to do right now.

Have you started the pgindent run yet? I have a patch ready for
the cursor stability issue, but will hold off committing if it might
create a merge problem for you.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > I am unclear why struct pointers are not being formatted properly in
> > function headers but will research it.
>
> Yeah, if we can fix that directly without adding the names to the
> typedef list, it would be better. But not something to do right now.
>
> Have you started the pgindent run yet? I have a patch ready for
> the cursor stability issue, but will hold off committing if it might
> create a merge problem for you.

I am waiting for Andrew to tell me he is ready with updated lists for
his platforms. His CGI output is now _not_ showing stat so I am
comparing the lists now.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
>> Have you started the pgindent run yet? I have a patch ready for
>> the cursor stability issue, but will hold off committing if it might
>> create a merge problem for you.

> I am waiting for Andrew to tell me he is ready with updated lists for
> his platforms. His CGI output is now _not_ showing stat so I am
> comparing the lists now.

OK, I'll commit now and then stay out of your way.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
>
>> I am unclear why struct pointers are not being formatted properly in
>> function headers but will research it.
>>
>
> Yeah, if we can fix that directly without adding the names to the
> typedef list, it would be better. But not something to do right now.
>
> Have you started the pgindent run yet? I have a patch ready for
> the cursor stability issue, but will hold off committing if it might
> create a merge problem for you.
>
>
>

I am doing runs as requested on various platforms to extract the typedef
lists. Linux is done, Windows (mingw) is running, FBSD and Cygwin to come.

Results in a few hours. The buildfarm will have a consolidated list.

cheers

andrew

Andrew Dunstan wrote:
>
>
> Tom Lane wrote:
> > Bruce Momjian <bruce(at)momjian(dot)us> writes:
> >
> >> I am unclear why struct pointers are not being formatted properly in
> >> function headers but will research it.
> >>
> >
> > Yeah, if we can fix that directly without adding the names to the
> > typedef list, it would be better. But not something to do right now.
> >
> > Have you started the pgindent run yet? I have a patch ready for
> > the cursor stability issue, but will hold off committing if it might
> > create a merge problem for you.
> >
> >
> >
>
> I am doing runs as requested on various platforms to extract the typedef
> lists. Linux is done, Windows (mingw) is running, FBSD and Cygwin to come.
>
> Results in a few hours. The buildfarm will have a consolidated list.

OK, good. However, I am seeing missing typedefs in the Linux list you
posted at:

http://www.pgbuildfarm.org/cgi-bin/typedefs.pl

I pulled this list 20 minutes ago. I compared your list to the BSD list
I usually used and found that the Linux list had fewer typedefs:

2270 typedefs.old
1848 typedefs.new

I did a diff, attached, and found some typedefs that don't appear, like
PortalData. That is defined in our code as:

typedef struct PortalData *Portal;

typedef struct PortalData
{
/* Bookkeeping data */
...
bool visible; /* include this portal in pg_cursors? */
} PortalData;

I will try to build on Linux here and see how objdump displays that,
unless you can get me the output.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Out of curiosity how different is the output if we don't pass the
typedef list at all? I'm wondering if the formatting differences are
things we actually care much about anyways.

Bruce Momjian wrote:
> I did a diff, attached, and found some typedefs that don't appear, like
> PortalData. That is defined in our code as:
>
> typedef struct PortalData *Portal;
>
> typedef struct PortalData
> {
> /* Bookkeeping data */
> ...
> bool visible; /* include this portal in pg_cursors? */
> } PortalData;
>
> I will try to build on Linux here and see how objdump displays that,
> unless you can get me the output.
>

Well, that is almost certainly a result of the change you asked me to
make :-)

The symbol is in the run done early this morning before those changes.
See
<http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=dungbeetle&dt=2009-06-10%20064401&stg=typedefs>

So we need to look and see what tag that symbol has in the objdump output.

cheers

andrew

Greg Stark <stark(at)enterprisedb(dot)com> writes:
> Out of curiosity how different is the output if we don't pass the
> typedef list at all? I'm wondering if the formatting differences are
> things we actually care much about anyways.

It tends to put extra spaces in variable declarations that are using
the typedef. Not sure about other effects, but it is kinda ugly when
you are used to it being right.

regards, tom lane

On Tuesday 09 June 2009 20:21:35 Bruce Momjian wrote:
> It is time to run pgindent on CVS HEAD for 8.4. I am thinking of
> running it at zero-hour GMT tomorrow, meaning five hours from now.
> Any objections?

Btw., can you make pgindent remove whitespace at the end of lines?

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Btw., can you make pgindent remove whitespace at the end of lines?

I think it usually does that already ...

regards, tom lane

Andrew Dunstan wrote:
>
>
> Bruce Momjian wrote:
> > I did a diff, attached, and found some typedefs that don't appear, like
> > PortalData. That is defined in our code as:
> >
> > typedef struct PortalData *Portal;
> >
> > typedef struct PortalData
> > {
> > /* Bookkeeping data */
> > ...
> > bool visible; /* include this portal in pg_cursors? */
> > } PortalData;
> >
> > I will try to build on Linux here and see how objdump displays that,
> > unless you can get me the output.
> >
>
>
> Well, that is almost certainly a result of the change you asked me to
> make :-)
>
> The symbol is in the run done early this morning before those changes.
> See
> <http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=dungbeetle&dt=2009-06-10%20064401&stg=typedefs>
>
> So we need to look and see what tag that symbol has in the objdump output.

OK, I got the answer for Linux. I built 8.4 RC2 and ran:

objdump -W postgres |grep -3 PortalData

and the output is attached. You will notice every mention of PortalData
has 'DW_TAG_structure_type' (stucture member) above it, and none have
DW_TAG_typedef (typedef tag). This is caused by this documented
behavior from find_typedef:

# Linux
# Unfortunately the Linux version doesn't show unreferenced typedefs.
# The problem is that they are still in the source code so should be
# indented properly. However, I think pgindent only cares about
# the typedef references, not the definitions, so I think it might
# be fine

So that is why the Linux list is shorter, but again, I think that is fine.
Andrew, let me know when your list is ready.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > Btw., can you make pgindent remove whitespace at the end of lines?
>
> I think it usually does that already ...

Yes.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Tom Lane wrote:
> Greg Stark <stark(at)enterprisedb(dot)com> writes:
> > Out of curiosity how different is the output if we don't pass the
> > typedef list at all? I'm wondering if the formatting differences are
> > things we actually care much about anyways.
>
> It tends to put extra spaces in variable declarations that are using
> the typedef. Not sure about other effects, but it is kinda ugly when
> you are used to it being right.

Yea, I used a short pgindent typedef list once and people noticed right
away so I had to rerun.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Wednesday 10 June 2009 22:50:15 Bruce Momjian wrote:
> Tom Lane wrote:
> > Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > > Btw., can you make pgindent remove whitespace at the end of lines?
> >
> > I think it usually does that already ...
>
> Yes.

Um, attached you will find a bunch of counterexamples.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>>> I think it usually does that already ...

> Um, attached you will find a bunch of counterexamples.

At a quick look, I'm not sure that any of these are in code that hasn't
been edited since the 8.3 pgindent run.

regards, tom lane

On Wed, Jun 10, 2009 at 9:54 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>>>> I think it usually does that already ...
>
>> Um, attached you will find a bunch of counterexamples.
>
> At a quick look, I'm not sure that any of these are in code that hasn't
> been edited since the 8.3 pgindent run.

On a tangent: git warns about or fixes up white-space problems like
that when you're applying a patch.

I've actually found this to be a bit of a dilemma though. For code i
want it to just go ahead and fix up anything it finds. But for
regression test expected output files I don't want it to. I don't
think you can control it per-directory though.

--
Gregory Stark
http://mit.edu/~gsstark/resume.pdf

Andrew Dunstan wrote:
>
>
> I am doing runs as requested on various platforms to extract the
> typedef lists. Linux is done, Windows (mingw) is running, FBSD and
> Cygwin to come.
>
> Results in a few hours. The buildfarm will have a consolidated list.
>
>

The consolidated list comes from Windows(mingw) and Linux. My Cygwin
run broke for some reason, and 'objdump --stabs' doesn't seem to do what
we need on FBSD, so the output there was empty. If someone knows how to
get the typedefs out via objdump on FBSD would they please let us know ASAP?

Bruce, I think that's the best I can do today.

thanks

andrew

Andrew Dunstan wrote:
>
>
> Andrew Dunstan wrote:
> >
> >
> > I am doing runs as requested on various platforms to extract the
> > typedef lists. Linux is done, Windows (mingw) is running, FBSD and
> > Cygwin to come.
> >
> > Results in a few hours. The buildfarm will have a consolidated list.
> >
> >
>
> The consolidated list comes from Windows(mingw) and Linux. My Cygwin
> run broke for some reason, and 'objdump --stabs' doesn't seem to do what
> we need on FBSD, so the output there was empty. If someone knows how to
> get the typedefs out via objdump on FBSD would they please let us know ASAP?

I will check on our Postgres shell server right away.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
>> The consolidated list comes from Windows(mingw) and Linux. My Cygwin
>> run broke for some reason, and 'objdump --stabs' doesn't seem to do what
>> we need on FBSD, so the output there was empty. If someone knows how to
>> get the typedefs out via objdump on FBSD would they please let us know ASAP?
>>
>
> I will check on our Postgres shell server right away.
>

OK, so we got that working, and the consolidated list now contains FBSD
data as well.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Bruce Momjian wrote:
>> I will check on our Postgres shell server right away.

> OK, so we got that working, and the consolidated list now contains FBSD
> data as well.

Um, let's *go* guys. RC1 wrap is scheduled for 18 hours from now.
That means it is already too late to be sure that we'll have a full
cycle of buildfarm checks on the pgindent run. Quit fooling around
and get it done.

regards, tom lane

Andrew Dunstan wrote:
>
>
> Bruce Momjian wrote:
> >> The consolidated list comes from Windows(mingw) and Linux. My Cygwin
> >> run broke for some reason, and 'objdump --stabs' doesn't seem to do what
> >> we need on FBSD, so the output there was empty. If someone knows how to
> >> get the typedefs out via objdump on FBSD would they please let us know ASAP?
> >>
> >
> > I will check on our Postgres shell server right away.
> >
>
> OK, so we got that working, and the consolidated list now contains FBSD
> data as well.

And where do I get the consolodated list? From?

http://www.pgbuildfarm.org/cgi-bin/typedefs.pl

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Andrew Dunstan wrote:
>
>> Bruce Momjian wrote:
>>
>>>> The consolidated list comes from Windows(mingw) and Linux. My Cygwin
>>>> run broke for some reason, and 'objdump --stabs' doesn't seem to do what
>>>> we need on FBSD, so the output there was empty. If someone knows how to
>>>> get the typedefs out via objdump on FBSD would they please let us know ASAP?
>>>>
>>>>
>>> I will check on our Postgres shell server right away.
>>>
>>>
>> OK, so we got that working, and the consolidated list now contains FBSD
>> data as well.
>>
>
> And where do I get the consolodated list? From?
>
> http://www.pgbuildfarm.org/cgi-bin/typedefs.pl
>

yes. As previously advertised.

If you want to see the individual components, use
<http://www.pgbuildfarm.org/cgi-bin/typedefs.pl?show_list=1>

cheers

andrew

Andrew Dunstan wrote:
>
>
> Bruce Momjian wrote:
> > Andrew Dunstan wrote:
> >
> >> Bruce Momjian wrote:
> >>
> >>>> The consolidated list comes from Windows(mingw) and Linux. My Cygwin
> >>>> run broke for some reason, and 'objdump --stabs' doesn't seem to do what
> >>>> we need on FBSD, so the output there was empty. If someone knows how to
> >>>> get the typedefs out via objdump on FBSD would they please let us know ASAP?
> >>>>
> >>>>
> >>> I will check on our Postgres shell server right away.
> >>>
> >>>
> >> OK, so we got that working, and the consolidated list now contains FBSD
> >> data as well.
> >>
> >
> > And where do I get the consolodated list? From?
> >
> > http://www.pgbuildfarm.org/cgi-bin/typedefs.pl
> >
>
> yes. As previously advertised.
>
> If you want to see the individual components, use
> <http://www.pgbuildfarm.org/cgi-bin/typedefs.pl?show_list=1>

OK, pgindent run with updated list and applied to CVS HEAD. I eyeballed
the patch and it looked clean, and it tested successfully. Thanks.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> OK, pgindent run with updated list and applied to CVS HEAD. I eyeballed
> the patch and it looked clean, and it tested successfully. Thanks.

Do we have any TODO items concerning pgindent at this point? You had
mentioned wanting to research its behavior for 'struct foo var'
declarations, and I'm not too clear about whether we're happy with the
typedef collection code yet.

regards, tom lane

Tom Lane wrote:
> Do we have any TODO items concerning pgindent at this point? Y
>

Yes, we will make the buildfarm and standalone find-typedefs run from a
common pieces of code so they are always in sync.

cheers

andrew

Andrew Dunstan wrote:
>
>
> Tom Lane wrote:
>> Do we have any TODO items concerning pgindent at this point? Y
>>
>
> Yes, we will make the buildfarm and standalone find-typedefs run from a
> common pieces of code so they are always in sync.

BTW if we had an "official" typedef list that could be used for the
length of a whole major release, we could run pgindent on a regular
basis (say fortnightly or monthly); patch submitters would just need to
run it on their own trees to avoid merge conflicts.

(Hmm, but I'm unsure how would that work with git merge etc).

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:
> BTW if we had an "official" typedef list that could be used for the
> length of a whole major release, we could run pgindent on a regular
> basis (say fortnightly or monthly); patch submitters would just need to
> run it on their own trees to avoid merge conflicts.
>
> (Hmm, but I'm unsure how would that work with git merge etc).
>
>

Well, step one was finding a way to get a more comprehensive typedef
list. Now we have some sort of mechanism for doing that, we can look at
regularly pulling the list into the source, I guess. Maybe then we could
look at either regular runs or even making it a part of a commit to the
central repo, if we trust it enough. That would have the advantage of
avoiding any later merge problems.

After the release, we can play around some more.

cheers

andrew

Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
> I confirmed the aix problem on 4.3.3. Installed the patches and
> updated postgres 8.4b2 removing the aix hack. Server starts fine:

> $ LOG: could not bind IPv6 socket: Addr family not supported by protocol
> HINT: Is another postmaster already running on port 5432? If not,
> wait a few seconds and retry.
> LOG: database system was shut down at 2009-06-11 13:23:32 EDT
> LOG: autovacuum launcher started
> LOG: database system is ready to accept connection

> psql logs in. I don't know if the problem we identified this sprint
> is still present...not too concered about that. pull the AIX
> getaddrinfo check and go with it... no problems as best I can tell.
> (out of time for this today).

OK, we'll go with that for RC1. I'll put a nonspecific note into the
docs suggesting a system update if trouble is encountered, but it would
be nice if we could add some information about exactly which fixes are
needed.

regards, tom lane

On Thu, Jun 11, 2009 at 2:13 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
>> I confirmed the aix problem on 4.3.3. Installed the patches and
>> updated postgres 8.4b2 removing the aix hack.  Server starts fine:
>
>> $ LOG:  could not bind IPv6 socket: Addr family not supported by protocol
>> HINT:  Is another postmaster already running on port 5432? If not,
>> wait a few seconds and retry.
>> LOG:  database system was shut down at 2009-06-11 13:23:32 EDT
>> LOG:  autovacuum launcher started
>> LOG:  database system is ready to accept connection
>
>> psql logs in.  I don't know if the problem we identified this sprint
>> is still present...not too concered about that.  pull the AIX
>> getaddrinfo check and go with it... no problems as best I can tell.
>> (out of time for this today).
>
> OK, we'll go with that for RC1.  I'll put a nonspecific note into the
> docs suggesting a system update if trouble is encountered, but it would
> be nice if we could add some information about exactly which fixes are
> needed.

I just pulled the aix rollup pack...and would expect others to do the
same. It hasn't been updated since 2004-ish.

merlin

On Wednesday 10 June 2009 23:54:41 Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> >>> I think it usually does that already ...
> >
> > Um, attached you will find a bunch of counterexamples.
>
> At a quick look, I'm not sure that any of these are in code that hasn't
> been edited since the 8.3 pgindent run.

So what does that mean then? Surely pgindent doesn't keep track of what has
been edited when?

Peter Eisentraut wrote:
> On Wednesday 10 June 2009 23:54:41 Tom Lane wrote:
>
>> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>>
>>>>> I think it usually does that already ...
>>>>>
>>> Um, attached you will find a bunch of counterexamples.
>>>
>> At a quick look, I'm not sure that any of these are in code that hasn't
>> been edited since the 8.3 pgindent run.
>>
>
> So what does that mean then? Surely pgindent doesn't keep track of what has
> been edited when?
>
>

If the code has been edited since the last pgindent run, then pgindent
hasn't had a chance to adjust it, no?

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Peter Eisentraut wrote:
>> On Wednesday 10 June 2009 23:54:41 Tom Lane wrote:
>>> At a quick look, I'm not sure that any of these are in code that hasn't
>>> been edited since the 8.3 pgindent run.
>>
>> So what does that mean then? Surely pgindent doesn't keep track of what has
>> been edited when?

> If the code has been edited since the last pgindent run, then pgindent
> hasn't had a chance to adjust it, no?

Right. Those extra spaces all represent manual editing sloppiness.

I have not done a complete check, but I looked at the first couple of
examples you cited and verified that pgindent did remove those spaces.

regards, tom lane