pgindent weirdness

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. +

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&gt;
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&gt;
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 : (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

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&gt;
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&gt;
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&gt;
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. +