Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Latest consolidated typedef list from buildfarm.

  1. pgindent weirdness

    Robert Haas <robertmhaas@gmail.com> — 2011-04-20T03:50:23Z

    pgindent seems to have muffed it when it comes to BulkInsertStateData:

diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index 2849992..72a69e5 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -150,7 +150,7 @@ ReadBufferBI(Relation relation, BlockNumber targetBlock,
 Buffer
 RelationGetBufferForTuple(Relation relation, Size len,
                                                  Buffer otherBuffer,
int options,
-                                                 struct
BulkInsertStateData *bistate)
+                                                 struct
BulkInsertStateData * bistate)
 {
        bool            use_fsm = !(options & HEAP_INSERT_SKIP_FSM);
        Buffer          buffer = InvalidBuffer;

Not sure what happened here exactly...

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  2. Re: pgindent weirdness

    Bruce Momjian <bruce@momjian.us> — 2011-04-20T09:48:34Z

    Robert Haas wrote:
> pgindent seems to have muffed it when it comes to BulkInsertStateData:
> 
> diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
> index 2849992..72a69e5 100644
> --- a/src/backend/access/heap/hio.c
> +++ b/src/backend/access/heap/hio.c
> @@ -150,7 +150,7 @@ ReadBufferBI(Relation relation, BlockNumber targetBlock,
>  Buffer
>  RelationGetBufferForTuple(Relation relation, Size len,
>                                                   Buffer otherBuffer,
> int options,
> -                                                 struct
> BulkInsertStateData *bistate)
> +                                                 struct
> BulkInsertStateData * bistate)
>  {
>         bool            use_fsm = !(options & HEAP_INSERT_SKIP_FSM);
>         Buffer          buffer = InvalidBuffer;
> 
> Not sure what happened here exactly...

BulkInsertStateData is not listed in the typedef list supplied by
Andrew; see src/tools/pgindent/typedefs.list.  CC'ing him.  This might
be because the typdef is listed in two files:

/*
 * state for bulk inserts --- private to heapam.c and hio.c
 *
 * If current_buf isn't InvalidBuffer, then we are holding an extra pin
 * on that buffer.
 *
 * "typedef struct BulkInsertStateData *BulkInsertState" is in heapam.h
 */


-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +

  3. Re: pgindent weirdness

    Andrew Dunstan <andrew@dunslane.net> — 2011-04-20T15:15:55Z

    
On 04/20/2011 05:48 AM, Bruce Momjian wrote:
> Robert Haas wrote:
>> pgindent seems to have muffed it when it comes to BulkInsertStateData:
>>
>> diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
>> index 2849992..72a69e5 100644
>> --- a/src/backend/access/heap/hio.c
>> +++ b/src/backend/access/heap/hio.c
>> @@ -150,7 +150,7 @@ ReadBufferBI(Relation relation, BlockNumber targetBlock,
>>   Buffer
>>   RelationGetBufferForTuple(Relation relation, Size len,
>>                                                    Buffer otherBuffer,
>> int options,
>> -                                                 struct
>> BulkInsertStateData *bistate)
>> +                                                 struct
>> BulkInsertStateData * bistate)
>>   {
>>          bool            use_fsm = !(options&  HEAP_INSERT_SKIP_FSM);
>>          Buffer          buffer = InvalidBuffer;
>>
>> Not sure what happened here exactly...
> BulkInsertStateData is not listed in the typedef list supplied by
> Andrew; see src/tools/pgindent/typedefs.list.  CC'ing him.  This might
> be because the typdef is listed in two files:
>
> /*
>   * state for bulk inserts --- private to heapam.c and hio.c
>   *
>   * If current_buf isn't InvalidBuffer, then we are holding an extra pin
>   * on that buffer.
>   *
>   * "typedef struct BulkInsertStateData *BulkInsertState" is in heapam.h
>   */
>
>


It's tagged as a structure type by objdump, but not as a typedef:

    <1><40055>: Abbrev Number: 4 (DW_TAG_typedef)
    <40056>   DW_AT_name        : (indirect string, offset: 0x6bf6):
    BulkInsertState
    <4005a>   DW_AT_decl_file   : 30
    <4005b>   DW_AT_decl_line   : 32
    <4005c>   DW_AT_type        : <0x40060>
    <1><40060>: Abbrev Number: 7 (DW_TAG_pointer_type)
    <40061>   DW_AT_byte_size   : 8
    <40062>   DW_AT_type        : <0x40066>
    <1><40066>: Abbrev Number: 13 (DW_TAG_structure_type)
    <40067>   DW_AT_name        : (indirect string, offset: 0x66bf):
    BulkInsertStateData
    <4006b>   DW_AT_byte_size   : 16
    <4006c>   DW_AT_decl_file   : 31
    <4006d>   DW_AT_decl_line   : 30
    <4006e>   DW_AT_sibling     : <0x4008b>

I can pull out those too if you want them in the list, but it would 
possibly add a LOT of names to the list.

I did carefully warn you about the need to check the effects of the 
changes when I committed the new list.

It looks like quite a few of the deletions come into this category, for 
example just looking at the diff here 
<https://github.com/postgres/postgres/commit/fe1438da8aa8a45f2cee816eb54841f97d3b2f22#src/tools/pgindent/typedefs.list> 
I see AggHashEntryData, AggStatePerAggData, AllocBlockData, and 
AllocChunkData from among the first few that were deleted and all are in 
the same category.

I wondered if this is some sort of optimizer effect, but building with 
-O0 doesn't seem to affect it.

Note that the list we're using is a composite of dumps from four 
platforms: Linux, FreeBSD, MinGW and Cygwin. What they have in common is 
that they are all using gcc, and a fairly modern version of gcc at that, 
and fairly modern versions of objdump too.

Attached is a partial list of new candidate symbols if we want to pick 
up these extras.

cheers

andrew

  4. Re: pgindent weirdness

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-04-20T15:43:48Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 04/20/2011 05:48 AM, Bruce Momjian wrote:
>> BulkInsertStateData is not listed in the typedef list supplied by
>> Andrew; see src/tools/pgindent/typedefs.list.  CC'ing him.  This might
>> be because the typdef is listed in two files:

> It's tagged as a structure type by objdump, but not as a typedef:

Hmm.  hio.h clearly declares it as both, but many object files probably
include only heapam.h, which exposes only the struct name.  I'm guessing
that you are merging the results from objdump'ing different files in a
way that fails to consider the possibility of some files knowing more
versions of a symbol than others.

Now having said that, there seems to be a pgindent bug here too, in that
it's throwing a space into

Buffer
RelationGetBufferForTuple(Relation relation, Size len,
                          Buffer otherBuffer, int options,
                          struct BulkInsertStateData * bistate)

Whether BulkInsertStateData is flagged as a typedef or not, surely it
ought to understand that "struct BulkInsertStateData" is a type name.

			regards, tom lane

  5. Re: pgindent weirdnessf

    Bruce Momjian <bruce@momjian.us> — 2011-04-20T15:48:49Z

    Andrew Dunstan wrote:
> It's tagged as a structure type by objdump, but not as a typedef:
> 
>     <1><40055>: Abbrev Number: 4 (DW_TAG_typedef)
>     <40056>   DW_AT_name        : (indirect string, offset: 0x6bf6):
>     BulkInsertState
>     <4005a>   DW_AT_decl_file   : 30
>     <4005b>   DW_AT_decl_line   : 32
>     <4005c>   DW_AT_type        : <0x40060>
>     <1><40060>: Abbrev Number: 7 (DW_TAG_pointer_type)
>     <40061>   DW_AT_byte_size   : 8
>     <40062>   DW_AT_type        : <0x40066>
>     <1><40066>: Abbrev Number: 13 (DW_TAG_structure_type)
>     <40067>   DW_AT_name        : (indirect string, offset: 0x66bf):
>     BulkInsertStateData
>     <4006b>   DW_AT_byte_size   : 16
>     <4006c>   DW_AT_decl_file   : 31
>     <4006d>   DW_AT_decl_line   : 30
>     <4006e>   DW_AT_sibling     : <0x4008b>
> 
> I can pull out those too if you want them in the list, but it would 
> possibly add a LOT of names to the list.
> 
> I did carefully warn you about the need to check the effects of the 
> changes when I committed the new list.
> 
> It looks like quite a few of the deletions come into this category, for 
> example just looking at the diff here 
> <https://github.com/postgres/postgres/commit/fe1438da8aa8a45f2cee816eb54841f97d3b2f22#src/tools/pgindent/typedefs.list> 
> I see AggHashEntryData, AggStatePerAggData, AllocBlockData, and 
> AllocChunkData from among the first few that were deleted and all are in 
> the same category.
> 
> I wondered if this is some sort of optimizer effect, but building with 
> -O0 doesn't seem to affect it.

I assume you are using -g, right?

The BulkInsertStateData typedef looks pretty normal:

	typedef struct BulkInsertStateData
	{
	    BufferAccessStrategy strategy;      /* our BULKWRITE strategy object */
	    Buffer      current_buf;    /* current insertion target page */
	}   BulkInsertStateData;

I tested my BSD machine using src/tools/find_typedefs and it does show
BulkInsertStateData.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +

  6. Re: pgindent weirdness

    Bruce Momjian <bruce@momjian.us> — 2011-04-20T15:50:59Z

    Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
> > On 04/20/2011 05:48 AM, Bruce Momjian wrote:
> >> BulkInsertStateData is not listed in the typedef list supplied by
> >> Andrew; see src/tools/pgindent/typedefs.list.  CC'ing him.  This might
> >> be because the typdef is listed in two files:
> 
> > It's tagged as a structure type by objdump, but not as a typedef:
> 
> Hmm.  hio.h clearly declares it as both, but many object files probably
> include only heapam.h, which exposes only the struct name.  I'm guessing
> that you are merging the results from objdump'ing different files in a
> way that fails to consider the possibility of some files knowing more
> versions of a symbol than others.
> 
> Now having said that, there seems to be a pgindent bug here too, in that
> it's throwing a space into
> 
> Buffer
> RelationGetBufferForTuple(Relation relation, Size len,
>                           Buffer otherBuffer, int options,
>                           struct BulkInsertStateData * bistate)
> 
> Whether BulkInsertStateData is flagged as a typedef or not, surely it
> ought to understand that "struct BulkInsertStateData" is a type name.

Uh, I think we have this listed as a known bug at the top of the
pgindent script:

	# Known bugs:
	#
	# Blank line is added after parentheses; seen as a function definition, no space
	# after *:
	#       y = (int) x *y;
	#
	# Structure/union pointers in function prototypes and definitions have an extra
	# space after the asterisk:
	#
	#       void x(struct xxc * a);

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +

  7. Re: pgindent weirdness

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-04-20T16:04:16Z

    Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> Now having said that, there seems to be a pgindent bug here too, in that
>> it's throwing a space into
>> 
>> Buffer
>> RelationGetBufferForTuple(Relation relation, Size len,
>> Buffer otherBuffer, int options,
>> struct BulkInsertStateData * bistate)
>> 
>> Whether BulkInsertStateData is flagged as a typedef or not, surely it
>> ought to understand that "struct BulkInsertStateData" is a type name.

> Uh, I think we have this listed as a known bug at the top of the
> pgindent script:

Hm.  I guess the third observation is that in the current state of the
code, there's no very good reason to be using "struct" in
RelationGetBufferForTuple's prototype anyway, since the typedef is
declared right above it.  Maybe we should just change that and not
risk fooling with pgindent.

			regards, tom lane

  8. Re: pgindent weirdness

    Bruce Momjian <bruce@momjian.us> — 2011-04-20T16:13:08Z

    Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Tom Lane wrote:
> >> Now having said that, there seems to be a pgindent bug here too, in that
> >> it's throwing a space into
> >> 
> >> Buffer
> >> RelationGetBufferForTuple(Relation relation, Size len,
> >> Buffer otherBuffer, int options,
> >> struct BulkInsertStateData * bistate)
> >> 
> >> Whether BulkInsertStateData is flagged as a typedef or not, surely it
> >> ought to understand that "struct BulkInsertStateData" is a type name.
> 
> > Uh, I think we have this listed as a known bug at the top of the
> > pgindent script:
> 
> Hm.  I guess the third observation is that in the current state of the
> code, there's no very good reason to be using "struct" in
> RelationGetBufferForTuple's prototype anyway, since the typedef is
> declared right above it.  Maybe we should just change that and not
> risk fooling with pgindent.

Probably.  :-(

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +

  9. Re: pgindent weirdness

    Andrew Dunstan <andrew@dunslane.net> — 2011-04-20T16:23:11Z

    
On 04/20/2011 11:43 AM, Tom Lane wrote:
> Andrew Dunstan<andrew@dunslane.net>  writes:
>> On 04/20/2011 05:48 AM, Bruce Momjian wrote:
>>> BulkInsertStateData is not listed in the typedef list supplied by
>>> Andrew; see src/tools/pgindent/typedefs.list.  CC'ing him.  This might
>>> be because the typdef is listed in two files:
>> It's tagged as a structure type by objdump, but not as a typedef:
> Hmm.  hio.h clearly declares it as both, but many object files probably
> include only heapam.h, which exposes only the struct name.  I'm guessing
> that you are merging the results from objdump'ing different files in a
> way that fails to consider the possibility of some files knowing more
> versions of a symbol than others.


We don't run objdump against individual object files, we run it against 
the linked binaries, i.e. the contents of the installed bin and lib 
directories.

But in any case, *none* of the individual files knows about 
BulkInsertStateData as a typedef:

    [andrew@emma backend]$ for f in ./access/heap/hio.o
    ./access/heap/heapam.o ./commands/tablecmds.o ./commands/copy.o
    ./executor/execMain.o ; do objdump -W $f ; done | egrep -A3
    DW_TAG_typedef | grep BulkInsertState
    <1811>   DW_AT_name        : (indirect string, offset: 0x1cc9):
    BulkInsertState
    <1f3c>   DW_AT_name        : (indirect string, offset: 0x296c):
    BulkInsertState
    <1fac>   DW_AT_name        : (indirect string, offset: 0x5c5b):
    BulkInsertState
    <211b>   DW_AT_name        : (indirect string, offset: 0x35ad):
    BulkInsertState
    <2530>   DW_AT_name        : (indirect string, offset: 0x3c93):
    BulkInsertState

And the reason is actually fairly obvious on closer inspection. The only 
place we actually use the BulkInsertStateData typedef (as opposed to the 
struct declaration) is here:

    ./backend/access/heap/heapam.c:    bistate = (BulkInsertState)
    palloc(sizeof(BulkInsertStateData));

and that sizeof operation will be resolved at compile time and never hit 
the symbol table.

So I suspect that the typedef finding code is actually working just fine.

It looks like the real problem is in how pgindent handles the use of 
'struct foo' in various places.

cheers

andrew

  10. Re: pgindent weirdness

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-04-20T16:29:02Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> But in any case, *none* of the individual files knows about 
> BulkInsertStateData as a typedef:
> ...
> And the reason is actually fairly obvious on closer inspection. The only 
> place we actually use the BulkInsertStateData typedef (as opposed to the 
> struct declaration) is here:

>     ./backend/access/heap/heapam.c:    bistate = (BulkInsertState)
>     palloc(sizeof(BulkInsertStateData));

> and that sizeof operation will be resolved at compile time and never hit 
> the symbol table.

Oh, interesting.  So you're saying that for this mechanism to know that
"foo" is a typedef, there has to be at least one variable in the code
that's declared as being of type foo or foo *?  (Where "variable" would
include function parameters, fields of other structs, etc.)

That's probably fine, because otherwise we'd have the typedef list
cluttered with junk we don't care about from system headers.

So in the case at hand, we actually *need* to remove the "struct" from
RelationGetBufferForTuple's declaration, so that BulkInsertStateData
gets used as a typedef name in that way.

			regards, tom lane

  11. Re: pgindent weirdnessf

    Andrew Dunstan <andrew@dunslane.net> — 2011-04-20T16:33:26Z

    
On 04/20/2011 11:48 AM, Bruce Momjian wrote:
> I assume you are using -g, right?


Of course I did. I wouldn't have any symbols at all if I didn't.

> The BulkInsertStateData typedef looks pretty normal:
>
> 	typedef struct BulkInsertStateData
> 	{
> 	    BufferAccessStrategy strategy;      /* our BULKWRITE strategy object */
> 	    Buffer      current_buf;    /* current insertion target page */
> 	}   BulkInsertStateData;
>
> I tested my BSD machine using src/tools/find_typedefs and it does show
> BulkInsertStateData.


You can contribute to the list by running a buildfarm animal on your 
machine and running its find_typedefs occasionally. This is not just 
about me. I have asked on numerous occasions for other people to 
contribute, and the response has been deafening silence. The  reason we 
got to this place is that people complained that your list was 
insufficiently complete, so I added a facility for buildfarm animals to 
generate their own lists, so we could get wider platform coverage. So my 
response to anyone who says "well, it works on my box" is "then why 
isn't your box doing it for the buildfarm?"

cheers

andrew

  12. Re: pgindent weirdness

    Andrew Dunstan <andrew@dunslane.net> — 2011-04-20T16:38:57Z

    
On 04/20/2011 12:29 PM, Tom Lane wrote:
> Andrew Dunstan<andrew@dunslane.net>  writes:
>> But in any case, *none* of the individual files knows about
>> BulkInsertStateData as a typedef:
>> ...
>> And the reason is actually fairly obvious on closer inspection. The only
>> place we actually use the BulkInsertStateData typedef (as opposed to the
>> struct declaration) is here:
>>      ./backend/access/heap/heapam.c:    bistate = (BulkInsertState)
>>      palloc(sizeof(BulkInsertStateData));
>> and that sizeof operation will be resolved at compile time and never hit
>> the symbol table.
> Oh, interesting.  So you're saying that for this mechanism to know that
> "foo" is a typedef, there has to be at least one variable in the code
> that's declared as being of type foo or foo *?  (Where "variable" would
> include function parameters, fields of other structs, etc.)

I believe so. I don't see how it could get tagged in the tables otherwise.

> That's probably fine, because otherwise we'd have the typedef list
> cluttered with junk we don't care about from system headers.


Well, yes, except that I'm a tiny bit smarter than that :-) After we 
generate the list of symbols we check that they actually occur in our 
sources and filter them out if they don't. That reduces the list by 
quite a lot.

> So in the case at hand, we actually *need* to remove the "struct" from
> RelationGetBufferForTuple's declaration, so that BulkInsertStateData
> gets used as a typedef name in that way.
>
> 			

That sounds right.

cheers

andrew

  13. Re: pgindent weirdness

    Aidan Van Dyk <aidan@highrise.ca> — 2011-04-20T17:03:14Z

    On Wed, Apr 20, 2011 at 12:38 PM, Andrew Dunstan <andrew@dunslane.net> wrote:

>> So in the case at hand, we actually *need* to remove the "struct" from
>> RelationGetBufferForTuple's declaration, so that BulkInsertStateData
>> gets used as a typedef name in that way.

Since the general form seems to be to declare things as:
   typedef struct foo { ... } foo;

Is there any reason why we see any struct foo in the sources other
than in the typedef line?

"Legacy" and "invasive patch" are good enough reasons, if they are it...

a.


-- 
Aidan Van Dyk                                             Create like a god,
aidan@highrise.ca                                       command like a king,
http://www.highrise.ca/                                   work like a slave.

  14. Re: pgindent weirdness

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-04-20T17:10:05Z

    Aidan Van Dyk <aidan@highrise.ca> writes:
> Since the general form seems to be to declare things as:
>    typedef struct foo { ... } foo;

> Is there any reason why we see any struct foo in the sources other
> than in the typedef line?

It gives an escape hatch in case you need a forward reference to the
struct, ie you can do "struct foo *" even before this.  But I agree that
90% of those struct tags are useless, and so the habit of tagging every
typedef this way is mostly legacy.

			regards, tom lane

  15. Re: pgindent weirdness

    Robert Haas <robertmhaas@gmail.com> — 2011-04-20T17:12:47Z

    On Wed, Apr 20, 2011 at 11:15 AM, Andrew Dunstan <andrew@dunslane.net> wrote:
> I did carefully warn you about the need to check the effects of the changes
> when I committed the new list.
>
> It looks like quite a few of the deletions come into this category, for
> example just looking at the diff here
> <https://github.com/postgres/postgres/commit/fe1438da8aa8a45f2cee816eb54841f97d3b2f22#src/tools/pgindent/typedefs.list>
> I see AggHashEntryData, AggStatePerAggData, AllocBlockData, and
> AllocChunkData from among the first few that were deleted and all are in the
> same category.

This implies to me that we changed something about how we handle this
since we did the 9.0 runs, but I don't know what it was.  Should I?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  16. Re: pgindent weirdness

    Bruce Momjian <bruce@momjian.us> — 2011-04-20T17:16:50Z

    Robert Haas wrote:
> On Wed, Apr 20, 2011 at 11:15 AM, Andrew Dunstan <andrew@dunslane.net> wrote:
> > I did carefully warn you about the need to check the effects of the changes
> > when I committed the new list.
> >
> > It looks like quite a few of the deletions come into this category, for
> > example just looking at the diff here
> > <https://github.com/postgres/postgres/commit/fe1438da8aa8a45f2cee816eb54841f97d3b2f22#src/tools/pgindent/typedefs.list>
> > I see AggHashEntryData, AggStatePerAggData, AllocBlockData, and
> > AllocChunkData from among the first few that were deleted and all are in the
> > same category.
> 
> This implies to me that we changed something about how we handle this
> since we did the 9.0 runs, but I don't know what it was.  Should I?

I think Andrew also supplied the typedef list for the 9.0 run.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +

  17. Re: pgindent weirdness

    Andrew Dunstan <andrew@dunslane.net> — 2011-04-20T17:17:50Z

    
On 04/20/2011 01:12 PM, Robert Haas wrote:
> On Wed, Apr 20, 2011 at 11:15 AM, Andrew Dunstan<andrew@dunslane.net>  wrote:
>> I did carefully warn you about the need to check the effects of the changes
>> when I committed the new list.
>>
>> It looks like quite a few of the deletions come into this category, for
>> example just looking at the diff here
>> <https://github.com/postgres/postgres/commit/fe1438da8aa8a45f2cee816eb54841f97d3b2f22#src/tools/pgindent/typedefs.list>
>> I see AggHashEntryData, AggStatePerAggData, AllocBlockData, and
>> AllocChunkData from among the first few that were deleted and all are in the
>> same category.
> This implies to me that we changed something about how we handle this
> since we did the 9.0 runs, but I don't know what it was.  Should I?
>

We have newer compilers which probably construct the symbol tables 
slightly differently.

cheers

andrew

  18. Re: pgindent weirdnessf

    Robert Haas <robertmhaas@gmail.com> — 2011-04-20T17:25:05Z

    On Wed, Apr 20, 2011 at 12:33 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
> You can contribute to the list by running a buildfarm animal on your machine
> and running its find_typedefs occasionally. This is not just about me. I
> have asked on numerous occasions for other people to contribute, and the
> response has been deafening silence. The  reason we got to this place is
> that people complained that your list was insufficiently complete, so I
> added a facility for buildfarm animals to generate their own lists, so we
> could get wider platform coverage. So my response to anyone who says "well,
> it works on my box" is "then why isn't your box doing it for the buildfarm?"

This is all well and good up to a point, but if Bruce's ancient BSDi
machine is the only one that can properly find these symbols, then we
are courting disaster by relying on it, even if he does run a
buildfarm animal there.  I can't help thinking there must be some
other explanation for this change.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  19. Re: pgindent weirdness

    Andrew Dunstan <andrew@dunslane.net> — 2011-04-20T17:29:36Z

    
On 04/20/2011 01:16 PM, Bruce Momjian wrote:
>>
>> This implies to me that we changed something about how we handle this
>> since we did the 9.0 runs, but I don't know what it was.  Should I?
> I think Andrew also supplied the typedef list for the 9.0 run.
>

Yes. But in November, the server where all my animals were running died. 
The rebuilt machines all used newer versions of the OS, new compilers 
and newer tools such as objdump. As I pointed out at the time I 
committed the new typedefs list, that accounts for a lot of the changes.

cheers

andrew

  20. Re: pgindent weirdnessf

    Bruce Momjian <bruce@momjian.us> — 2011-04-20T17:59:53Z

    Robert Haas wrote:
> On Wed, Apr 20, 2011 at 12:33 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
> > You can contribute to the list by running a buildfarm animal on your machine
> > and running its find_typedefs occasionally. This is not just about me. I
> > have asked on numerous occasions for other people to contribute, and the
> > response has been deafening silence. The ?reason we got to this place is
> > that people complained that your list was insufficiently complete, so I
> > added a facility for buildfarm animals to generate their own lists, so we
> > could get wider platform coverage. So my response to anyone who says "well,
> > it works on my box" is "then why isn't your box doing it for the buildfarm?"
> 
> This is all well and good up to a point, but if Bruce's ancient BSDi
> machine is the only one that can properly find these symbols, then we
> are courting disaster by relying on it, even if he does run a
> buildfarm animal there.  I can't help thinking there must be some
> other explanation for this change.

Uh, just a reality check, but our "courting disaster" means we will have
an extra space after some asterisks in the source code.  ;-)

I do, agree, though, it would be nice to find out what changed that
caused this.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +

  21. Re: pgindent weirdnessf

    Andrew Dunstan <andrew@dunslane.net> — 2011-04-20T18:13:36Z

    
On 04/20/2011 01:59 PM, Bruce Momjian wrote:
> I do, agree, though, it would be nice to find out what changed that
> caused this.
>

I am 100% certain that it's the tools that have changed. I bet if I were 
to hunt in my pile of old DVDs and find installation media for Fedora 6 
or thereabouts and set it up on a VM I'd be able to reproduce the old 
list. But it would be a serious waste of my tolerably precious time.

cheers

andrew

  22. Re: pgindent weirdness

    Andrew Dunstan <andrew@dunslane.net> — 2011-04-20T18:25:47Z

    
On 04/20/2011 01:10 PM, Tom Lane wrote:
> Aidan Van Dyk<aidan@highrise.ca>  writes:
>> Since the general form seems to be to declare things as:
>>     typedef struct foo { ... } foo;
>> Is there any reason why we see any struct foo in the sources other
>> than in the typedef line?
> It gives an escape hatch in case you need a forward reference to the
> struct, ie you can do "struct foo *" even before this.  But I agree that
> 90% of those struct tags are useless, and so the habit of tagging every
> typedef this way is mostly legacy.
>
> 			

Yeah, I think it would be reasonable to remove lots of them, especially 
in argument lists where I think they're a bit ugly anyway.

I'm not sure if now is a good time to be doing that sort of cleanup - 
maybe we should just add the typedefs we think we're missing to the 
typedefs list and try another pgindent run, and then make these changes 
early in 9.2 dev cycle.


cheers

andrew

  23. Re: pgindent weirdness

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-04-20T19:50:25Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 04/20/2011 01:16 PM, Bruce Momjian wrote:
>>> This implies to me that we changed something about how we handle this
>>> since we did the 9.0 runs, but I don't know what it was.  Should I?

>> I think Andrew also supplied the typedef list for the 9.0 run.

> Yes. But in November, the server where all my animals were running died. 
> The rebuilt machines all used newer versions of the OS, new compilers 
> and newer tools such as objdump. As I pointed out at the time I 
> committed the new typedefs list, that accounts for a lot of the changes.

It wouldn't surprise me in the least to find out that newer gcc's
stopped emitting symbol table entries for unreferenced typedefs.

In fact, using HEAD, I get this on my old HPUX box:

(gdb) p sizeof(BulkInsertStateData)
$65 = 8

and this on my Fedora 13 box:

(gdb) p sizeof(BulkInsertStateData)
No symbol "BulkInsertStateData" in current context.

(gcc 2.95.3 and 4.4.5 respectively)  So the tools definitely changed
sometime in the last N years.

			regards, tom lane

  24. Re: pgindent weirdness

    Bruce Momjian <bruce@momjian.us> — 2011-04-20T20:28:34Z

    Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
> > On 04/20/2011 01:16 PM, Bruce Momjian wrote:
> >>> This implies to me that we changed something about how we handle this
> >>> since we did the 9.0 runs, but I don't know what it was.  Should I?
> 
> >> I think Andrew also supplied the typedef list for the 9.0 run.
> 
> > Yes. But in November, the server where all my animals were running died. 
> > The rebuilt machines all used newer versions of the OS, new compilers 
> > and newer tools such as objdump. As I pointed out at the time I 
> > committed the new typedefs list, that accounts for a lot of the changes.
> 
> It wouldn't surprise me in the least to find out that newer gcc's
> stopped emitting symbol table entries for unreferenced typedefs.
> 
> In fact, using HEAD, I get this on my old HPUX box:
> 
> (gdb) p sizeof(BulkInsertStateData)
> $65 = 8
> 
> and this on my Fedora 13 box:
> 
> (gdb) p sizeof(BulkInsertStateData)
> No symbol "BulkInsertStateData" in current context.
> 
> (gcc 2.95.3 and 4.4.5 respectively)  So the tools definitely changed
> sometime in the last N years.

So the list of possible additions Andrew supplied are cases where we
never reference those typedefs --- seems like a cleanup opportunity.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +

  25. Re: pgindent weirdness

    Andrew Dunstan <andrew@dunslane.net> — 2011-04-20T21:05:51Z

    
On 04/20/2011 04:28 PM, Bruce Momjian wrote:
> So the list of possible additions Andrew supplied are cases where we
> never reference those typedefs --- seems like a cleanup opportunity.
>

I think the best cleanup idea is Aidan's, namely is we have declared 
"typdef struct foo { ... } foo;" we should use "foo" in the code  
instead of "struct foo". Then the typedef will be referenced, and the 
code will be cleaner, and we won't run into the pgindent "struct" bug 
either, so it's a win/win/win.

cheers

andrew

  26. Re: pgindent weirdness

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-04-20T21:29:20Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 04/20/2011 04:28 PM, Bruce Momjian wrote:
>> So the list of possible additions Andrew supplied are cases where we
>> never reference those typedefs --- seems like a cleanup opportunity.

> I think the best cleanup idea is Aidan's, namely is we have declared 
> "typdef struct foo { ... } foo;" we should use "foo" in the code  
> instead of "struct foo". Then the typedef will be referenced, and the 
> code will be cleaner, and we won't run into the pgindent "struct" bug 
> either, so it's a win/win/win.

We want to do that in any case.  I think that Bruce was suggesting going
further and actively removing unreferenced struct tags from the
declaration sites.  I'm less enthused about that.  It would save nothing
except some probably-unmeasurable amount of compile time, and it'd
result in a lot of diffs that might come back to bite future
back-patching efforts.

			regards, tom lane

  27. Re: pgindent weirdness

    Bruce Momjian <bruce@momjian.us> — 2011-04-20T21:43:11Z

    Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
> > On 04/20/2011 04:28 PM, Bruce Momjian wrote:
> >> So the list of possible additions Andrew supplied are cases where we
> >> never reference those typedefs --- seems like a cleanup opportunity.
> 
> > I think the best cleanup idea is Aidan's, namely is we have declared 
> > "typdef struct foo { ... } foo;" we should use "foo" in the code  
> > instead of "struct foo". Then the typedef will be referenced, and the 
> > code will be cleaner, and we won't run into the pgindent "struct" bug 
> > either, so it's a win/win/win.
> 
> We want to do that in any case.  I think that Bruce was suggesting going
> further and actively removing unreferenced struct tags from the
> declaration sites.  I'm less enthused about that.  It would save nothing
> except some probably-unmeasurable amount of compile time, and it'd
> result in a lot of diffs that might come back to bite future
> back-patching efforts.

No, I wasn't thinking that far;  just finding the cases where we don't
reference a typedef and instead use 'struct structname'.  I think Andrew
has supplied that list, almost.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +

  28. Re: pgindent weirdness

    Andrew Dunstan <andrew@dunslane.net> — 2011-04-20T22:13:29Z

    
On 04/20/2011 05:29 PM, Tom Lane wrote:
> Andrew Dunstan<andrew@dunslane.net>  writes:
>> On 04/20/2011 04:28 PM, Bruce Momjian wrote:
>>> So the list of possible additions Andrew supplied are cases where we
>>> never reference those typedefs --- seems like a cleanup opportunity.
>> I think the best cleanup idea is Aidan's, namely is we have declared
>> "typdef struct foo { ... } foo;" we should use "foo" in the code
>> instead of "struct foo". Then the typedef will be referenced, and the
>> code will be cleaner, and we won't run into the pgindent "struct" bug
>> either, so it's a win/win/win.
> We want to do that in any case.  I think that Bruce was suggesting going
> further and actively removing unreferenced struct tags from the
> declaration sites.  I'm less enthused about that.  It would save nothing
> except some probably-unmeasurable amount of compile time, and it'd
> result in a lot of diffs that might come back to bite future
> back-patching efforts.
>
> 			

Well he says not, but in any case I agree there's no great gain from it. 
It's a well established C idiom, and as you pointed out upthread the 
struct tag is just about required for defining recursive structs anyway.

cheers

andrew

  29. Re: pgindent weirdness

    Bruce Momjian <bruce@momjian.us> — 2011-10-12T20:54:23Z

    Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Tom Lane wrote:
> >> Now having said that, there seems to be a pgindent bug here too, in that
> >> it's throwing a space into
> >> 
> >> Buffer
> >> RelationGetBufferForTuple(Relation relation, Size len,
> >> Buffer otherBuffer, int options,
> >> struct BulkInsertStateData * bistate)
> >> 
> >> Whether BulkInsertStateData is flagged as a typedef or not, surely it
> >> ought to understand that "struct BulkInsertStateData" is a type name.
> 
> > Uh, I think we have this listed as a known bug at the top of the
> > pgindent script:
> 
> Hm.  I guess the third observation is that in the current state of the
> code, there's no very good reason to be using "struct" in
> RelationGetBufferForTuple's prototype anyway, since the typedef is
> declared right above it.  Maybe we should just change that and not
> risk fooling with pgindent.

Changed as you suggested.  I didn't see any other obvious cases.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +