Обсуждение: pgindent weirdness
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,BufferRelationGetBufferForTuple(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
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
anextra 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. +
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
Вложения
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
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. +
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. +
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
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. +
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 : (indirectstring, 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
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
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
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
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.
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
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
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. +
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
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
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
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. +
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
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
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
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. +
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
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
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. +
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
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. + diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c new file mode 100644 index 26db1e3..beecc90 *** a/src/backend/access/heap/hio.c --- b/src/backend/access/heap/hio.c *************** GetVisibilityMapPins(Relation relation, *** 213,219 **** Buffer RelationGetBufferForTuple(Relation relation, Size len, Buffer otherBuffer, int options, ! struct BulkInsertStateData * bistate, Buffer *vmbuffer, Buffer *vmbuffer_other) { bool use_fsm = !(options & HEAP_INSERT_SKIP_FSM); --- 213,219 ---- Buffer RelationGetBufferForTuple(Relation relation, Size len, Buffer otherBuffer, int options, ! BulkInsertState bistate, Buffer *vmbuffer, Buffer *vmbuffer_other) { bool use_fsm = !(options & HEAP_INSERT_SKIP_FSM); diff --git a/src/include/access/hio.h b/src/include/access/hio.h new file mode 100644 index 6eac97e..aefd679 *** a/src/include/access/hio.h --- b/src/include/access/hio.h *************** extern void RelationPutHeapTuple(Relatio *** 38,44 **** HeapTuple tuple); extern Buffer RelationGetBufferForTuple(Relation relation, Size len, Buffer otherBuffer, int options, ! struct BulkInsertStateData * bistate, Buffer *vmbuffer, Buffer *vmbuffer_other); #endif /* HIO_H */ --- 38,44 ---- HeapTuple tuple); extern Buffer RelationGetBufferForTuple(Relation relation, Size len, Buffer otherBuffer, int options, ! BulkInsertState bistate, Buffer *vmbuffer, Buffer *vmbuffer_other); #endif /* HIO_H */