Re: [HACKERS] pgindent fixups

[Tom Lane]

Robert Haas  writes:
> So I really would like to get a pgindent run done.  Any objections to
> doing it sometime RSN?  It is of course possible that it might make
> something that we want to revert later harder to revert, but I think
> we should just accept that risk and move forward.

Now that we bit the bullet on 137805f89, I do not think there's anything
else with a really high probability of being reverted.  Might as well do
the run.  Please note that typedefs.list is already out of date.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent fixups

[Robert Haas]

On Tue, May 3, 2016 at 11:31 AM, Robert Haas  wrote:
> On Tue, May 3, 2016 at 11:23 AM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> OK, I committed this.  Barring objections, I'll go ahead and pgindent
>>> the whole tree tomorrow.  If we're going to revert anything big then
>>> we might want to hold off, but otherwise I think its better to get
>>> this done sooner rather than later.
>>
>> Well, there are at least two patchsets we're actively discussing
>> reverting, so I think this should wait till those decisions are resolved.
>
> OK, but that may well mean we don't get this done before beta1, which
> I think is a bummer, but oh well.

So I really would like to get a pgindent run done.  Any objections to
doing it sometime RSN?  It is of course possible that it might make
something that we want to revert later harder to revert, but I think
we should just accept that risk and move forward.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent fixups

[Tom Lane]

Robert Haas  writes:
> On Tue, May 3, 2016 at 11:23 AM, Tom Lane  wrote:
>> Well, there are at least two patchsets we're actively discussing
>> reverting, so I think this should wait till those decisions are resolved.

> OK, but that may well mean we don't get this done before beta1, which
> I think is a bummer, but oh well.

pgindent is reliable enough that I'm not really worried about running
it post-beta.  Obviously sooner is better than later, to give authors
of 9.7 patches more time to rebase; but I do not think we should give
ourselves extra work just to do it a little sooner.

> There are a lot more than 2 patchsets that are busted at the moment,
> unfortunately, but I assume you are referring to "snapshot too old"
> and "Use Foreign Key relationships to infer multi-column join
> selectivity".

Yeah, those are the ones I'm thinking of.  I've not heard serious
proposals to revert any others, have you?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent fixups

[Robert Haas]

On Tue, May 3, 2016 at 11:23 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> OK, I committed this.  Barring objections, I'll go ahead and pgindent
>> the whole tree tomorrow.  If we're going to revert anything big then
>> we might want to hold off, but otherwise I think its better to get
>> this done sooner rather than later.
>
> Well, there are at least two patchsets we're actively discussing
> reverting, so I think this should wait till those decisions are resolved.

OK, but that may well mean we don't get this done before beta1, which
I think is a bummer, but oh well.

There are a lot more than 2 patchsets that are busted at the moment,
unfortunately, but I assume you are referring to "snapshot too old"
and "Use Foreign Key relationships to infer multi-column join
selectivity".

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent fixups

[Tom Lane]

Robert Haas  writes:
> OK, I committed this.  Barring objections, I'll go ahead and pgindent
> the whole tree tomorrow.  If we're going to revert anything big then
> we might want to hold off, but otherwise I think its better to get
> this done sooner rather than later.

Well, there are at least two patchsets we're actively discussing
reverting, so I think this should wait till those decisions are resolved.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent fixups

[Robert Haas]

On Tue, May 3, 2016 at 9:40 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> 1. Is pgindent supposed to touch DATA() lines?  Because it does.
>
> It always has.
>
>> 2. CustomPathMethods is not in the buildfarm's typedefs.list.  Why not?
>
> Probably because there are no variables, parameters, or fields declared
> with that typedef, so it doesn't get into the debugger symbol table of
> any .o file.  Grep says that the single use of the struct doesn't use
> the typedef; it's
> const struct CustomPathMethods *methods;
> So you'd need to change that, or else tweak some code somewhere to have
> a variable explicitly declared using the typedef.

Ah.  It's a minor issue, so probably not worth worrying about.

>> - In src/backend/executor/execParallel.c, it dodges two cases where
>> pgindent does stupid things with offsetof.
>
> Well, it's also pretty stupid about sizeof, or casts generally, so
> I'm not really convinced you need to get exercised about these two
> places in particular.  But no objection to tweaking them if you
> want to.

OK, I committed this.  Barring objections, I'll go ahead and pgindent
the whole tree tomorrow.  If we're going to revert anything big then
we might want to hold off, but otherwise I think its better to get
this done sooner rather than later.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent fixups

[Tom Lane]

Robert Haas  writes:
> 1. Is pgindent supposed to touch DATA() lines?  Because it does.

It always has.

> 2. CustomPathMethods is not in the buildfarm's typedefs.list.  Why not?

Probably because there are no variables, parameters, or fields declared
with that typedef, so it doesn't get into the debugger symbol table of
any .o file.  Grep says that the single use of the struct doesn't use
the typedef; it's
const struct CustomPathMethods *methods;
So you'd need to change that, or else tweak some code somewhere to have
a variable explicitly declared using the typedef.

> - In src/backend/executor/execParallel.c, it dodges two cases where
> pgindent does stupid things with offsetof.

Well, it's also pretty stupid about sizeof, or casts generally, so
I'm not really convinced you need to get exercised about these two
places in particular.  But no objection to tweaking them if you
want to.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent fixups

[Andrew Dunstan]

On 05/02/2016 10:56 PM, Robert Haas wrote:

I spent some time going through the output of a trial pgindent run
today.  Some questions/problems:

1. Is pgindent supposed to touch DATA() lines?  Because it does.



Apart from being detabbed/entabbed, no. pgindent protects (or tries to 
protect) DATA and CATALOG lines by enclosng them in comments which it 
later removes.





2. CustomPathMethods is not in the buildfarm's typedefs.list.  Why not?



Because it's not used anywhere. typdefs get into the list by being used 
and thus having a typedef symbol in the compiled binary. Just creating a 
typedef isn't enough. This has happened before.


You can get some insight into how the typedefs list is generated by 
looking here: 



cheers

andrew




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] pgindent fixups

[Robert Haas]

I spent some time going through the output of a trial pgindent run
today.  Some questions/problems:

1. Is pgindent supposed to touch DATA() lines?  Because it does.

2. CustomPathMethods is not in the buildfarm's typedefs.list.  Why not?

I'm attaching a patch that fixes up a few other problems that I found,
which I'll commit if there are not objections.  In detail:

- In contrib/pageinspect/heapfuncs.c, it separates the declaration of
bits_len from the initialization to avoid an awkward line-wrap.

- In src/backend/executor/execParallel.c, it dodges two cases where
pgindent does stupid things with offsetof.  Apparently, pgindent
thinks that you should write "offsetof(a, b) +c" rather than
"offsetof(a, b) + c".  In one case, I talked it out of it by putting
the + at the end of the first line rather than the start of the
continuation line.  The other statement was all on one line so I
changed it to say "c + offsetof(a, b)" instead.

- In nodeAgg.c, to_ts_any.c, and tsvector_op.c, I moved end-of-line
comments to their own separate lines, because they were getting broken
up into multiple lines in ways that seemed awkward.  In tsginidx.c, I
left a similar comment inline but fiddled the whitespace and comment
text to avoid getting a line break in mid-comment.

- In spell.c, I added  markers around a comment to prevent
pgindent from messing with the whitespace (entab still adjusts it, but
that should look the same if you have your tab stops set right).

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


pgindent-cleanup.patch
Description: application/download

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent

[Tom Lane]

Robert Haas  writes:
> I compared the result of running pgindent with the typedefs.list file
> as updated by me manually with the result of running pgindent using
> the buildfarm list and ... the buildfarm list is better.  Shows what I
> know.  Should we go ahead and commit the current version of that file
> as src/tools/pgindent/typedefs.list, then?

Yes, if it's what you plan to use for pgindent'ing.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent

[Robert Haas]

On Thu, Apr 28, 2016 at 7:39 AM, Bruce Momjian  wrote:
> On Wed, Apr 27, 2016 at 11:38:57PM -0400, Robert Haas wrote:
>> > On what grounds do you claim the buildfarm result is unstable?
>> > I've been using that for a long time and it works fine.  Moreover,
>> > ignoring that data is a bad idea because it reflects platform-specific
>> > variations in the set of typedefs that are known.  If you build a
>> > typedefs list based only on what works on your machine, it likely
>> > won't work for other people.
>>
>> /me shrugs
>>
>> Well, let's get the list, then, and compare it to what's in the file
>> now.  How do we do that exactly?
>
> The URL is in the file src/tools/pgindent/README:
>
>   5) Download the typedef file from the buildfarm:
>
> wget -O src/tools/pgindent/typedefs.list 
> http://buildfarm.postgresql.org/cgi-bin/typedefs.pl
>
>(see http://www.pgbuildfarm.org/cgi-bin/typedefs.pl?show_list for a full 
> list of typedefs,
> also 
> http://adpgtech.blogspot.com/2015/05/running-pgindent-on-non-core-code-or.html)

I compared the result of running pgindent with the typedefs.list file
as updated by me manually with the result of running pgindent using
the buildfarm list and ... the buildfarm list is better.  Shows what I
know.  Should we go ahead and commit the current version of that file
as src/tools/pgindent/typedefs.list, then?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent

[Bruce Momjian]

On Wed, Apr 27, 2016 at 11:38:57PM -0400, Robert Haas wrote:
> > On what grounds do you claim the buildfarm result is unstable?
> > I've been using that for a long time and it works fine.  Moreover,
> > ignoring that data is a bad idea because it reflects platform-specific
> > variations in the set of typedefs that are known.  If you build a
> > typedefs list based only on what works on your machine, it likely
> > won't work for other people.
> 
> /me shrugs
> 
> Well, let's get the list, then, and compare it to what's in the file
> now.  How do we do that exactly?

The URL is in the file src/tools/pgindent/README:

  5) Download the typedef file from the buildfarm:

wget -O src/tools/pgindent/typedefs.list 
http://buildfarm.postgresql.org/cgi-bin/typedefs.pl

   (see http://www.pgbuildfarm.org/cgi-bin/typedefs.pl?show_list for a full 
list of typedefs,
also 
http://adpgtech.blogspot.com/2015/05/running-pgindent-on-non-core-code-or.html)

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent

[Robert Haas]

On Wed, Apr 27, 2016 at 6:10 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Wed, Apr 27, 2016 at 2:23 PM, Tom Lane  wrote:
>>> Um, we normally take the buildfarm's list of typedefs, not anything
>>> manually created.
>
>> Well, we can still do that, but I don't see much advantage in it.  It
>> just churns the file to the extent that manual review of the changes
>> is impossible, and then when pgindent does the wrong thing it only
>> gets reported after the fact.  How is that better than making sure
>> that the contents of the file are such as to actually produce good
>> output from pgindent?
>
> On what grounds do you claim the buildfarm result is unstable?
> I've been using that for a long time and it works fine.  Moreover,
> ignoring that data is a bad idea because it reflects platform-specific
> variations in the set of typedefs that are known.  If you build a
> typedefs list based only on what works on your machine, it likely
> won't work for other people.

/me shrugs

Well, let's get the list, then, and compare it to what's in the file
now.  How do we do that exactly?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent

[Tom Lane]

Robert Haas  writes:
> On Wed, Apr 27, 2016 at 2:23 PM, Tom Lane  wrote:
>> Um, we normally take the buildfarm's list of typedefs, not anything
>> manually created.

> Well, we can still do that, but I don't see much advantage in it.  It
> just churns the file to the extent that manual review of the changes
> is impossible, and then when pgindent does the wrong thing it only
> gets reported after the fact.  How is that better than making sure
> that the contents of the file are such as to actually produce good
> output from pgindent?

On what grounds do you claim the buildfarm result is unstable?
I've been using that for a long time and it works fine.  Moreover,
ignoring that data is a bad idea because it reflects platform-specific
variations in the set of typedefs that are known.  If you build a
typedefs list based only on what works on your machine, it likely
won't work for other people.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent

[Bruce Momjian]

On Wed, Apr 27, 2016 at 05:01:14PM -0400, Bruce Momjian wrote:
> On Wed, Apr 27, 2016 at 02:54:35PM -0400, Robert Haas wrote:
> > On Wed, Apr 27, 2016 at 2:23 PM, Tom Lane  wrote:
> > > Robert Haas  writes:
> > >> I think it's about time for us to run pgindent.  I did a trial run
> > >> today of pgindent today and came up with the attached patch for
> > >> typedefs.list, which I'd like to commit more or less immediately,
> > >> barring objections.
> > >
> > > Um, we normally take the buildfarm's list of typedefs, not anything
> > > manually created.
> > 
> > Well, we can still do that, but I don't see much advantage in it.  It
> > just churns the file to the extent that manual review of the changes
> > is impossible, and then when pgindent does the wrong thing it only
> > gets reported after the fact.  How is that better than making sure
> > that the contents of the file are such as to actually produce good
> > output from pgindent?
> 
> Using the buildfarm typedefs assures they are always generated in a
> consistent way.

Oh, and as I remember the buildfarm merges several platforms, including
Windows, to make that list, so I suggest you use that one.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent

[Bruce Momjian]

On Wed, Apr 27, 2016 at 02:54:35PM -0400, Robert Haas wrote:
> On Wed, Apr 27, 2016 at 2:23 PM, Tom Lane  wrote:
> > Robert Haas  writes:
> >> I think it's about time for us to run pgindent.  I did a trial run
> >> today of pgindent today and came up with the attached patch for
> >> typedefs.list, which I'd like to commit more or less immediately,
> >> barring objections.
> >
> > Um, we normally take the buildfarm's list of typedefs, not anything
> > manually created.
> 
> Well, we can still do that, but I don't see much advantage in it.  It
> just churns the file to the extent that manual review of the changes
> is impossible, and then when pgindent does the wrong thing it only
> gets reported after the fact.  How is that better than making sure
> that the contents of the file are such as to actually produce good
> output from pgindent?

Using the buildfarm typedefs assures they are always generated in a
consistent way.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent

[Robert Haas]

On Wed, Apr 27, 2016 at 2:23 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> I think it's about time for us to run pgindent.  I did a trial run
>> today of pgindent today and came up with the attached patch for
>> typedefs.list, which I'd like to commit more or less immediately,
>> barring objections.
>
> Um, we normally take the buildfarm's list of typedefs, not anything
> manually created.

Well, we can still do that, but I don't see much advantage in it.  It
just churns the file to the extent that manual review of the changes
is impossible, and then when pgindent does the wrong thing it only
gets reported after the fact.  How is that better than making sure
that the contents of the file are such as to actually produce good
output from pgindent?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent

[Tom Lane]

Robert Haas  writes:
> I think it's about time for us to run pgindent.  I did a trial run
> today of pgindent today and came up with the attached patch for
> typedefs.list, which I'd like to commit more or less immediately,
> barring objections.

Um, we normally take the buildfarm's list of typedefs, not anything
manually created.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent

[Robert Haas]

On Wed, Apr 27, 2016 at 11:57 AM, Bruce Momjian  wrote:
> On Wed, Apr 27, 2016 at 11:51:42AM -0400, Robert Haas wrote:
>> >> It mostly just adds new typedefs that have
>> >> appeared over the last year, but it also realphabetizes the file -
>> >> some things that were added incrementally seem to have ended up in
>> >> what is, at least according to what sort likes to do on my machine,
>> >> the wrong place in the file.
>> >
>> > Is it just me, or is the sort order in that file a bit confusing? The
>> > whole thing about upper and lower case being separated seems to make it
>> > much harder than necessary to manually insert something in the right
>> > place..
>>
>> Except for recently-manually-added entries, it seems to match what
>> sort wants to do on my system exactly.  Which seems good.
>
> Well, sort ordering is all related to your collation setting:
>
> $ echo "a
> > A" | LC_COLLATE="" sort
> a
> A
>
> $ echo "a
> A" | LC_COLLATE="en_US" sort
> A
> a

Sure.  I guess we could resort that file with LC_COLLATE=C.  But my
point was mostly just that the ordering is hardly haphazard.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent

[Bruce Momjian]

On Wed, Apr 27, 2016 at 11:51:42AM -0400, Robert Haas wrote:
> >> It mostly just adds new typedefs that have
> >> appeared over the last year, but it also realphabetizes the file -
> >> some things that were added incrementally seem to have ended up in
> >> what is, at least according to what sort likes to do on my machine,
> >> the wrong place in the file.
> >
> > Is it just me, or is the sort order in that file a bit confusing? The
> > whole thing about upper and lower case being separated seems to make it
> > much harder than necessary to manually insert something in the right
> > place..
> 
> Except for recently-manually-added entries, it seems to match what
> sort wants to do on my system exactly.  Which seems good.

Well, sort ordering is all related to your collation setting:

$ echo "a
> A" | LC_COLLATE="" sort
a
A

$ echo "a
A" | LC_COLLATE="en_US" sort
A
a

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent

[Robert Haas]

On Wed, Apr 27, 2016 at 10:57 AM, Bruce Momjian  wrote:
> Agreed.  Sounds like a good plan --- a better plan than I have used in
> the past.

Thinking about this a bit more, what I am inclined to do is:

1. Run pgindent.

2. Read the diff and revert any changes that look icky.

3. Commit the rest.

4. Run pgindent again and post the diff.  Discuss how to fix the
ickiness contained therein, then proceed accordingly.

How does that sound?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent

[Robert Haas]

On Wed, Apr 27, 2016 at 11:45 AM, Andres Freund  wrote:
> Yes, that makes sense. That way other can easily look at "their" code,
> to see whether it can be made more pgindent resistant ;)

Right.

>> It mostly just adds new typedefs that have
>> appeared over the last year, but it also realphabetizes the file -
>> some things that were added incrementally seem to have ended up in
>> what is, at least according to what sort likes to do on my machine,
>> the wrong place in the file.
>
> Is it just me, or is the sort order in that file a bit confusing? The
> whole thing about upper and lower case being separated seems to make it
> much harder than necessary to manually insert something in the right
> place..

Except for recently-manually-added entries, it seems to match what
sort wants to do on my system exactly.  Which seems good.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent

[Andres Freund]

On 2016-04-27 10:51:55 -0400, Robert Haas wrote:
> I think it's about time for us to run pgindent.  Sounds reasonable.

> I did a trial run
> today of pgindent today and came up with the attached patch for
> typedefs.list, which I'd like to commit more or less immediately,
> barring objections.

Yes, that makes sense. That way other can easily look at "their" code,
to see whether it can be made more pgindent resistant ;)

> It mostly just adds new typedefs that have
> appeared over the last year, but it also realphabetizes the file -
> some things that were added incrementally seem to have ended up in
> what is, at least according to what sort likes to do on my machine,
> the wrong place in the file.

Is it just me, or is the sort order in that file a bit confusing? The
whole thing about upper and lower case being separated seems to make it
much harder than necessary to manually insert something in the right
place..

- Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] pgindent

[Robert Haas]

I think it's about time for us to run pgindent.  I did a trial run
today of pgindent today and came up with the attached patch for
typedefs.list, which I'd like to commit more or less immediately,
barring objections.  It mostly just adds new typedefs that have
appeared over the last year, but it also realphabetizes the file -
some things that were added incrementally seem to have ended up in
what is, at least according to what sort likes to do on my machine,
the wrong place in the file.

With this applied, I get a fairly clean pgindent run.  There are some
problems with comments getting mangled, and in a couple of cases
function definitions getting mangled, that need more investigation.
I'll try to find time to look into that soon and follow up, unless
somebody else beats me to it.  As far as possible, I think it's
desirable to clean up those things before rather than after running
pgindent, because unmangling ASCII art that pgindent has stepped on is
a thankless chore.

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


typedefs-cleanup.patch
Description: application/download

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent

[Bruce Momjian]

On Wed, Apr 27, 2016 at 10:51:55AM -0400, Robert Haas wrote:
> I think it's about time for us to run pgindent.  I did a trial run
> today of pgindent today and came up with the attached patch for
> typedefs.list, which I'd like to commit more or less immediately,
> barring objections.  It mostly just adds new typedefs that have
> appeared over the last year, but it also realphabetizes the file -
> some things that were added incrementally seem to have ended up in
> what is, at least according to what sort likes to do on my machine,
> the wrong place in the file.
> 
> With this applied, I get a fairly clean pgindent run.  There are some
> problems with comments getting mangled, and in a couple of cases
> function definitions getting mangled, that need more investigation.
> I'll try to find time to look into that soon and follow up, unless
> somebody else beats me to it.  As far as possible, I think it's
> desirable to clean up those things before rather than after running
> pgindent, because unmangling ASCII art that pgindent has stepped on is
> a thankless chore.

Agreed.  Sounds like a good plan --- a better plan than I have used in
the past.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent-polluted commits

[Noah Misch]

On Sat, Jan 16, 2016 at 09:57:45AM +, Simon Riggs wrote:
> On 16 January 2016 at 02:10, Noah Misch  wrote:
> > On Wed, Jan 13, 2016 at 12:13:11PM -0500, Tom Lane wrote:
> > > Basically this is trading off convenience of the committer (all of the
> > > alternatives Noah mentions are somewhat annoying) versus the convenience
> > > of post-commit reviewers.  I'm not sure that his recommendation is the
> > > best trade-off, nor that the situation is precisely comparable to
> > > pre-commit review.  There definitely will be pre-commit review, there
> > > may or may not be any post-commit review.
> >
> > That's a good summary.

> My objective in committing patches to PostgreSQL is to develop the Open
> Source version of PostgreSQL as a standalone product and I encourage others
> to do the same.
> 
> PostgreSQL is open source and therefore usable for various additional
> purposes, one of which is modified versions of PostgreSQL.
> 
> I will not go out of my way to cause problems for the secondary users of
> the code. I will try to implement one of the suggestions for whitespace
> handling, though may make mistakes in that, nobody being perfect.

Thanks.  Clean commits help so many audiences, including immediate post-commit
reviewers, intensive beta testers, fork maintainers, and hackers performing
root cause analysis on the bugs to be discovered in future years.  For what
it's worth, most committers already have been using some mix of strategy 2
(leave pgindent entirely to Bruce) and strategy 1 (neither add nor remove work
for the next whole-tree pgindent to do).  If you're already in that majority,
I advise no change.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent-polluted commits

[Simon Riggs]

On 16 January 2016 at 02:10, Noah Misch  wrote:

> On Wed, Jan 13, 2016 at 12:13:11PM -0500, Tom Lane wrote:
> > Simon Riggs  writes:
> > > On 13 January 2016 at 14:48, Noah Misch  wrote:
> > >> I've noticed commits, from a few of you, carrying pgindent changes to
> lines
> > >> the patch would not otherwise change.
> >
> > > Could we review again why this matters?
> >
> > Basically this is trading off convenience of the committer (all of the
> > alternatives Noah mentions are somewhat annoying) versus the convenience
> > of post-commit reviewers.  I'm not sure that his recommendation is the
> > best trade-off, nor that the situation is precisely comparable to
> > pre-commit review.  There definitely will be pre-commit review, there
> > may or may not be any post-commit review.
>
> That's a good summary.
>
> > I'm willing to go with the "separate commit to reindent individual files"
> > approach if there's a consensus that that makes for a cleaner git
> history.
> > But I'm not 100% convinced it matters.
>
> Thanks.
>

My objective in committing patches to PostgreSQL is to develop the Open
Source version of PostgreSQL as a standalone product and I encourage others
to do the same.

PostgreSQL is open source and therefore usable for various additional
purposes, one of which is modified versions of PostgreSQL.

I will not go out of my way to cause problems for the secondary users of
the code. I will try to implement one of the suggestions for whitespace
handling, though may make mistakes in that, nobody being perfect.

The secondary purposes of the code can only occur if the core code lives
and breathes, so I expect such users to make positive contributions to core
directly and not to block or slow down inclusion of features by others.
Quid pro quo.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] pgindent-polluted commits

[Noah Misch]

On Wed, Jan 13, 2016 at 12:13:11PM -0500, Tom Lane wrote:
> Simon Riggs  writes:
> > On 13 January 2016 at 14:48, Noah Misch  wrote:
> >> I've noticed commits, from a few of you, carrying pgindent changes to lines
> >> the patch would not otherwise change.
> 
> > Could we review again why this matters?
> 
> Basically this is trading off convenience of the committer (all of the
> alternatives Noah mentions are somewhat annoying) versus the convenience
> of post-commit reviewers.  I'm not sure that his recommendation is the
> best trade-off, nor that the situation is precisely comparable to
> pre-commit review.  There definitely will be pre-commit review, there
> may or may not be any post-commit review.

That's a good summary.

> I'm willing to go with the "separate commit to reindent individual files"
> approach if there's a consensus that that makes for a cleaner git history.
> But I'm not 100% convinced it matters.

Thanks.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent-polluted commits

[Andrew Dunstan]

On 01/13/2016 12:13 PM, Tom Lane wrote:

Simon Riggs  writes:

On 13 January 2016 at 14:48, Noah Misch  wrote:

I've noticed commits, from a few of you, carrying pgindent changes to lines
the patch would not otherwise change.

Could we review again why this matters?

Basically this is trading off convenience of the committer (all of the
alternatives Noah mentions are somewhat annoying) versus the convenience
of post-commit reviewers.  I'm not sure that his recommendation is the
best trade-off, nor that the situation is precisely comparable to
pre-commit review.  There definitely will be pre-commit review, there
may or may not be any post-commit review.

I'm willing to go with the "separate commit to reindent individual files"
approach if there's a consensus that that makes for a cleaner git history.
But I'm not 100% convinced it matters.






I do think it makes life easier when going through the git history if 
semantic changes are separated from formatting changes.


cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent-polluted commits

[Robert Haas]

On Thu, Jan 14, 2016 at 11:25 AM, Andrew Dunstan  wrote:
> I do think it makes life easier when going through the git history if
> semantic changes are separated from formatting changes.

I agree.  And I agree with Mark Dilger's point, too.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent-polluted commits

[Mark Dilger]

> On Jan 13, 2016, at 9:13 AM, Tom Lane  wrote:
> 
> Simon Riggs  writes:
>> On 13 January 2016 at 14:48, Noah Misch  wrote:
>>> I've noticed commits, from a few of you, carrying pgindent changes to lines
>>> the patch would not otherwise change.
> 
>> Could we review again why this matters?
> 
> Basically this is trading off convenience of the committer (all of the
> alternatives Noah mentions are somewhat annoying) versus the convenience
> of post-commit reviewers.  I'm not sure that his recommendation is the
> best trade-off, nor that the situation is precisely comparable to
> pre-commit review.  There definitely will be pre-commit review, there
> may or may not be any post-commit review.
> 
> I'm willing to go with the "separate commit to reindent individual files"
> approach if there's a consensus that that makes for a cleaner git history.
> But I'm not 100% convinced it matters.

As somebody who maintains a fork of the code base, I can say it is easier
to deal with merge conflicts when work is broken out into smaller commits.
Separating whitespace and formatting changes into their own commits
would make my life a little easier.

OTOH, I don't know if the core developer community cares about the ease
of maintaining code forks.


mark



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] pgindent-polluted commits

[Noah Misch]

I've noticed commits, from a few of you, carrying pgindent changes to lines
the patch would not otherwise change.  (That is to say, the next pgindent run
would have made the same changes anyway.)  From
https://wiki.postgresql.org/wiki/Submitting_a_Patch#Reasons_your_patch_might_be_returned:

  The fastest way to get your patch rejected is to make unrelated changes.
  Reformatting lines that haven't changed, changing unrelated comments you
  felt were poorly worded, touching code not necessary to your change, etc.

Commits should follow the same high standards we ask of submissions.  Several
pgindent strategies do conform:

1) Run pgindent, then manually reduce its changes to the subset that your
   patch caused.
2) Don't run pgindent yourself; commit code that pgindent may later change.
3) Push a commit containing nothing but a pgindent run of the files you care
   about, then push a second commit for your feature/bugfix.

Please use of one of those next time you'd run pgindent.

Thanks,
nm


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent-polluted commits

[Tom Lane]

Simon Riggs  writes:
> On 13 January 2016 at 14:48, Noah Misch  wrote:
>> I've noticed commits, from a few of you, carrying pgindent changes to lines
>> the patch would not otherwise change.

> Could we review again why this matters?

Basically this is trading off convenience of the committer (all of the
alternatives Noah mentions are somewhat annoying) versus the convenience
of post-commit reviewers.  I'm not sure that his recommendation is the
best trade-off, nor that the situation is precisely comparable to
pre-commit review.  There definitely will be pre-commit review, there
may or may not be any post-commit review.

I'm willing to go with the "separate commit to reindent individual files"
approach if there's a consensus that that makes for a cleaner git history.
But I'm not 100% convinced it matters.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent-polluted commits

[Simon Riggs]

On 13 January 2016 at 14:48, Noah Misch  wrote:


> I've noticed commits, from a few of you, carrying pgindent changes to lines
> the patch would not otherwise change.


Could we review again why this matters?

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

[HACKERS] pgindent vs emacs

[Andrew Dunstan]

One of the annoying inconsistencies between emacs and pgindent is that 
emacs refuses to offset a block following a case label, while pgindent 
does. Is there anything we can do to induce emacs to do what pgindent does?


cheers

andrew



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent vs emacs

[Andres Freund]

On 2015-05-29 13:37:40 -0400, Andrew Dunstan wrote:
 One of the annoying inconsistencies between emacs and pgindent is that emacs
 refuses to offset a block following a case label, while pgindent does. Is
 there anything we can do to induce emacs to do what pgindent does?

Are you using the logic from src/tools/editors/emacs.samples

I don't see that problem here. I've further tuned my emacs for pPG, but
afaics nothing but relevant for this but the above.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent vs emacs

[Andrew Dunstan]

On 05/29/2015 01:49 PM, Andres Freund wrote:

On 2015-05-29 13:37:40 -0400, Andrew Dunstan wrote:

One of the annoying inconsistencies between emacs and pgindent is that emacs
refuses to offset a block following a case label, while pgindent does. Is
there anything we can do to induce emacs to do what pgindent does?

Are you using the logic from src/tools/editors/emacs.samples

I don't see that problem here. I've further tuned my emacs for pPG, but
afaics nothing but relevant for this but the above.




Hmm, yes, you're right, I was missing something. It also turns out it 
depends on stuff we can't put in .dir-locals.el.


Sorry for the noise.

cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] pgindent weirdness

[Robert Haas]

In contrib/test_shm_mq/test.c, the 9.4 pgindent run
(0a7832005792fa6dad171f9cadb8d587fe0dd800) did this:

-PG_MODULE_MAGIC;
-PG_FUNCTION_INFO_V1(test_shm_mq);
+PG_MODULE_MAGIC; PG_FUNCTION_INFO_V1(test_shm_mq);
 PG_FUNCTION_INFO_V1(test_shm_mq_pipelined);

This is obviously not an improvement.  Is there some formatting rule
that I violated in the original code that lead to this, or what?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Bruce Momjian]

On Thu, Jul 10, 2014 at 12:02:50PM -0400, Robert Haas wrote:
 In contrib/test_shm_mq/test.c, the 9.4 pgindent run
 (0a7832005792fa6dad171f9cadb8d587fe0dd800) did this:
 
 -PG_MODULE_MAGIC;
 -PG_FUNCTION_INFO_V1(test_shm_mq);
 +PG_MODULE_MAGIC; PG_FUNCTION_INFO_V1(test_shm_mq);
  PG_FUNCTION_INFO_V1(test_shm_mq_pipelined);
 
 This is obviously not an improvement.  Is there some formatting rule
 that I violated in the original code that lead to this, or what?

Uh, ever other case of PG_MODULE_MAGIC had blank lines before/after this
define.  I went and added that to test_shm_mq/test.c, and adjusted other
blank lines to be consistent.  I did not modify 9.4 as this cosmetic.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent run

[Bruce Momjian]

On Sat, May  3, 2014 at 02:20:27PM -0400, Bruce Momjian wrote:
 I am planning to run pgindent in a few days to prepare for beta.  Does
 anyone have major patches that you are planning to apply soon?  If so, I
 can delay pgindent until you are done.  
 
 This run will also have a tabs-in-comments removal phase which will also
 be run on supported back branches.

Hearing nothing, I plan to pgindent head in an hour, around 14:00 GMT. 
I will also be removing some tabs in comments in all supported back
branches.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent run

[Bruce Momjian]

On Tue, May  6, 2014 at 08:55:07AM -0400, Bruce Momjian wrote:
 On Sat, May  3, 2014 at 02:20:27PM -0400, Bruce Momjian wrote:
  I am planning to run pgindent in a few days to prepare for beta.  Does
  anyone have major patches that you are planning to apply soon?  If so, I
  can delay pgindent until you are done.  
  
  This run will also have a tabs-in-comments removal phase which will also
  be run on supported back branches.
 
 Hearing nothing, I plan to pgindent head in an hour, around 14:00 GMT. 
 I will also be removing some tabs in comments in all supported back
 branches.

All done.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] pgindent run

[Bruce Momjian]

I am planning to run pgindent in a few days to prepare for beta.  Does
anyone have major patches that you are planning to apply soon?  If so, I
can delay pgindent until you are done.  

This run will also have a tabs-in-comments removal phase which will also
be run on supported back branches.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent behavior we could do without

[Bruce Momjian]

On Thu, Jan 30, 2014 at 11:44:31PM -0500, Tom Lane wrote:
 Bruce Momjian br...@momjian.us writes:
  OK, seven hours later, I have fixed pg_bsd_indent to no longer insert
  blank lines above #elif/#else/#endif, and therefore removed the special
  case code from pgindent.
 
  You will need to download version 1.3 of pg_bsd_indent for this to work,
  and pgindent will complain if it doesn't find the right pg_bsd_indent
  version.
 
 Cool.
 
  Do we need to go an clean up any places where pgindent has suppressed
  blank lines above #elif/#else/#endif in the past?
 
 Not sure it's worth making a massive change for this, but I appreciate the
 fact that pgindent won't mess up such code in the future.

Yes, it is a shame pgindent has removed many proper empty lines in the
past and there is no way to re-add them without causing backpatching
problems.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent behavior we could do without

[Bruce Momjian]

On Fri, Jan 31, 2014 at 11:18:17AM -0500, Bruce Momjian wrote:
 Yes, it is a shame pgindent has removed many proper empty lines in the
 past and there is no way to re-add them without causing backpatching
 problems.

FYI, the original BSD indent code that added the blank lines kind of
made sense.  If you defined a block of variables or a function, BSD
indent wanted a blank line after that.  When it saw a CPP directive, it
knew that wasn't a blank line, so it forced one.

In our coding, we often want CPP directives with no blank space, hence
the problem.  pg_bsd_indent 1.3 will not longer force a blank line
when it sees a CPP directive, and pgindent will no longer remove those
blank lines.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] pgindent wishlist item

[Andrew Dunstan]

While Bruce is working on pgindent, let me register a small wishlist 
item. It would be quite useful to be able to supply extra typedefs on 
the command line to supplement a typedefs file downloaded from the 
buildfarm or constructed however. A concrete example: in the code I have 
been recently working on, there are typedefs for Jsonb and JsonbValue. 
If I run pgindent as normal on the new code these items are not treated 
properly. What I had to do was take a special copy of the typedefs list 
and add those two items. If we could pass a list of extra typedefs to 
supplement the typedefs file that would be very useful. Then I could do 
something like:


   pgindent --typedef Jsonb --typedef JsonbValue
   src/backend/utils/adt/jsonfuncs.c

without having to mangle a typedefs file.

This would make using pgindent nicer to use during development, since 
any significant development is just about guaranteed to have some new 
typedefs the buildfarm can't have.


cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent wishlist item

[Tom Lane]

Andrew Dunstan and...@dunslane.net writes:
 While Bruce is working on pgindent, let me register a small wishlist 
 item. It would be quite useful to be able to supply extra typedefs on 
 the command line to supplement a typedefs file downloaded from the 
 buildfarm or constructed however. A concrete example: in the code I have 
 been recently working on, there are typedefs for Jsonb and JsonbValue. 
 If I run pgindent as normal on the new code these items are not treated 
 properly. What I had to do was take a special copy of the typedefs list 
 and add those two items. If we could pass a list of extra typedefs to 
 supplement the typedefs file that would be very useful. Then I could do 
 something like:

 pgindent --typedef Jsonb --typedef JsonbValue
 src/backend/utils/adt/jsonfuncs.c

 without having to mangle a typedefs file.

Personally, I always just edit the downloaded file to add whatever
typedefs the patch I'm working on adds.  I wouldn't use a command
line switch even if there was one, because then I'd have to remember
which typedef names to add each time I run pgindent.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent wishlist item

[Andres Freund]

On 2014-01-31 12:29:52 -0500, Andrew Dunstan wrote:
 While Bruce is working on pgindent

If it's christmas, let me wish for a not completly broken formatting of
function typedefs. E.g.
typedef ForeignScan *(*GetForeignPlan_function) (PlannerInfo *root,
 RelOptInfo *baserel,
  Oid foreigntableid,
  ForeignPath *best_path,
 List *tlist,
 List *scan_clauses);
is ridiculous. It can't be convinced to add a newline at any helpful
place afaik.


Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent wishlist item

[Bruce Momjian]

On Fri, Jan 31, 2014 at 12:44:22PM -0500, Tom Lane wrote:
 Andrew Dunstan and...@dunslane.net writes:
  While Bruce is working on pgindent, let me register a small wishlist 
  item. It would be quite useful to be able to supply extra typedefs on 
  the command line to supplement a typedefs file downloaded from the 
  buildfarm or constructed however. A concrete example: in the code I have 
  been recently working on, there are typedefs for Jsonb and JsonbValue. 
  If I run pgindent as normal on the new code these items are not treated 
  properly. What I had to do was take a special copy of the typedefs list 
  and add those two items. If we could pass a list of extra typedefs to 
  supplement the typedefs file that would be very useful. Then I could do 
  something like:
 
  pgindent --typedef Jsonb --typedef JsonbValue
  src/backend/utils/adt/jsonfuncs.c
 
  without having to mangle a typedefs file.
 
 Personally, I always just edit the downloaded file to add whatever
 typedefs the patch I'm working on adds.  I wouldn't use a command
 line switch even if there was one, because then I'd have to remember
 which typedef names to add each time I run pgindent.

Easily added, so done with the attached, applied patch.  You use it
like this:

pgindent --list-of-typedefs 'abc def'

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +
diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
new file mode 100755
index 8e45b18..2de7a53
*** a/src/tools/pgindent/pgindent
--- b/src/tools/pgindent/pgindent
*** my $indent_opts =
*** 22,31 
  # indent-dependant settings
  my $extra_opts = ;
  
! my ($typedefs_file, $code_base, $excludes, $indent, $build);
  
  my %options = (
  	typedefs=s  = \$typedefs_file,
  	code-base=s = \$code_base,
  	excludes=s  = \$excludes,
  	indent=s= \$indent,
--- 22,32 
  # indent-dependant settings
  my $extra_opts = ;
  
! my ($typedefs_file, $typedef_str, $code_base, $excludes, $indent, $build);
  
  my %options = (
  	typedefs=s  = \$typedefs_file,
+ 	list-of-typedefs=s  = \$typedef_str,
  	code-base=s = \$code_base,
  	excludes=s  = \$excludes,
  	indent=s= \$indent,
*** sub load_typedefs
*** 125,130 
--- 126,138 
  	  || die cannot open typedefs file \$typedefs_file\: $!\n;
  	my @typedefs = $typedefs_fh;
  	close($typedefs_fh);
+ 	if (defined($typedef_str))
+ 	{
+ 		foreach my $typedef (split(m/[, \t\n]+/, $typedef_str))
+ 		{
+ 			push(@typedefs, $typedef . \n);
+ 		}
+ 	}
  
  	# remove certain entries
  	@typedefs =

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent wishlist item

[Bruce Momjian]

On Fri, Jan 31, 2014 at 07:15:05PM +0100, Andres Freund wrote:
 On 2014-01-31 12:29:52 -0500, Andrew Dunstan wrote:
  While Bruce is working on pgindent
 
 If it's christmas, let me wish for a not completly broken formatting of
 function typedefs. E.g.
 typedef ForeignScan *(*GetForeignPlan_function) (PlannerInfo *root,
  RelOptInfo *baserel,
   Oid foreigntableid,
   ForeignPath *best_path,
  List *tlist,
  List *scan_clauses);
 is ridiculous. It can't be convinced to add a newline at any helpful
 place afaik.

Uh, not sure how to help here.  :-(

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent behavior we could do without

[Bruce Momjian]

On Thu, Jul 18, 2013 at 12:27:21AM -0400, Tom Lane wrote:
 It's always annoyed me that pgindent insists on adjusting vertical
 whitespace around #else and related commands.  This has, for example,
 rendered src/include/storage/barrier.h nigh illegible: you get things
 like
 
 /*
  * lwsync orders loads with respect to each other, and similarly with stores.
  * But a load can be performed before a subsequent store, so sync must be used
  * for a full memory barrier.
  */
 #define pg_memory_barrier() __asm__ __volatile__ (sync : : : memory)
 #define pg_read_barrier()   __asm__ __volatile__ (lwsync : : : memory)
 #define pg_write_barrier()  __asm__ __volatile__ (lwsync : : : memory)
 #elif defined(__alpha) || defined(__alpha__)/* Alpha */
 
 which makes it look like this block of code has something to do with
 Alpha.
 
 By chance, I noticed today that this misbehavior comes from a discretely
 identifiable spot, to wit lines 289-290 in src/tools/pgindent/pgindent:
 
   # Remove blank line(s) before #else, #elif, and #endif
   $source =~ s!\n\n+(\#else|\#elif|\#endif)!\n$1!g;
 
 This seems pretty broken to me: why exactly is whitespace there such a
 bad idea?  Not only that, but the next action is concerned with undoing
 some of the damage this rule causes:
 
   # Add blank line before #endif if it is the last line in the file
   $source =~ s!\n(#endif.*)\n\z!\n\n$1\n!;
 
 I assert that we should simply remove both of these bits of code, as
 just about every committer on the project is smarter about when to use
 vertical whitespace than this program is.

OK, I have developed the attached patch that shows the code change of
removing the test for #else/#elif/#endif.  You will see that the new
output has odd blank lines for cases like:

#ifndef WIN32
static intcopy_file(const char *fromfile, const char *tofile, bool 
force);
--
#else
static intwin32_pghardlink(const char *src, const char *dst);
--
#endif

I can't think of a way to detect tight blocks like the above from cases
where there are sizable blocks, like:

#ifndef WIN32

static intcopy_file(const char *fromfile, const char *tofile, bool 
force);
...
...
...

#else

static intwin32_pghardlink(const char *src, const char *dst);
...
...
...

#endif

Ideas?

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent behavior we could do without

[Tom Lane]

Bruce Momjian br...@momjian.us writes:
 On Thu, Jul 18, 2013 at 12:27:21AM -0400, Tom Lane wrote:
 I assert that we should simply remove both of these bits of code, as
 just about every committer on the project is smarter about when to use
 vertical whitespace than this program is.

 OK, I have developed the attached patch that shows the code change of
 removing the test for #else/#elif/#endif.  You will see that the new
 output has odd blank lines for cases like:

   #ifndef WIN32
   static intcopy_file(const char *fromfile, const char *tofile, bool 
 force);
 --
   #else
   static intwin32_pghardlink(const char *src, const char *dst);
 --
   #endif

Hm.  So actually, that code is trying to undo excess vertical whitespace
that something earlier in pgindent added?  Where is that happening?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent behavior we could do without

[Bruce Momjian]

On Thu, Jan 30, 2014 at 01:46:34PM -0500, Tom Lane wrote:
 Bruce Momjian br...@momjian.us writes:
  On Thu, Jul 18, 2013 at 12:27:21AM -0400, Tom Lane wrote:
  I assert that we should simply remove both of these bits of code, as
  just about every committer on the project is smarter about when to use
  vertical whitespace than this program is.
 
  OK, I have developed the attached patch that shows the code change of
  removing the test for #else/#elif/#endif.  You will see that the new
  output has odd blank lines for cases like:
 
  #ifndef WIN32
  static intcopy_file(const char *fromfile, const char *tofile, bool 
  force);
  --
  #else
  static intwin32_pghardlink(const char *src, const char *dst);
  --
  #endif
 
 Hm.  So actually, that code is trying to undo excess vertical whitespace
 that something earlier in pgindent added?  Where is that happening?

I am afraid it is _inside_ BSD indent, and if ever looked at that code,
you would not want to go in there.  :-O

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent behavior we could do without

[Bruce Momjian]

On Thu, Jan 30, 2014 at 01:52:55PM -0500, Bruce Momjian wrote:
 On Thu, Jan 30, 2014 at 01:46:34PM -0500, Tom Lane wrote:
  Bruce Momjian br...@momjian.us writes:
   On Thu, Jul 18, 2013 at 12:27:21AM -0400, Tom Lane wrote:
   I assert that we should simply remove both of these bits of code, as
   just about every committer on the project is smarter about when to use
   vertical whitespace than this program is.
  
   OK, I have developed the attached patch that shows the code change of
   removing the test for #else/#elif/#endif.  You will see that the new
   output has odd blank lines for cases like:
  
 #ifndef WIN32
 static intcopy_file(const char *fromfile, const char *tofile, bool 
   force);
   --
 #else
 static intwin32_pghardlink(const char *src, const char *dst);
   --
 #endif
  
  Hm.  So actually, that code is trying to undo excess vertical whitespace
  that something earlier in pgindent added?  Where is that happening?
 
 I am afraid it is _inside_ BSD indent, and if ever looked at that code,
 you would not want to go in there.  :-O

OK, seven hours later, I have fixed pg_bsd_indent to no longer insert
blank lines above #elif/#else/#endif, and therefore removed the special
case code from pgindent.

You will need to download version 1.3 of pg_bsd_indent for this to work,
and pgindent will complain if it doesn't find the right pg_bsd_indent
version.

Do we need to go an clean up any places where pgindent has suppressed
blank lines above #elif/#else/#endif in the past?

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent behavior we could do without

[Tom Lane]

Bruce Momjian br...@momjian.us writes:
 OK, seven hours later, I have fixed pg_bsd_indent to no longer insert
 blank lines above #elif/#else/#endif, and therefore removed the special
 case code from pgindent.

 You will need to download version 1.3 of pg_bsd_indent for this to work,
 and pgindent will complain if it doesn't find the right pg_bsd_indent
 version.

Cool.

 Do we need to go an clean up any places where pgindent has suppressed
 blank lines above #elif/#else/#endif in the past?

Not sure it's worth making a massive change for this, but I appreciate the
fact that pgindent won't mess up such code in the future.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent behavior we could do without

[Andrew Dunstan]

On 07/18/2013 11:30 PM, Tom Lane wrote:

Noah Misch n...@leadboat.com writes:

On Thu, Jul 18, 2013 at 12:27:21AM -0400, Tom Lane wrote:

It's always annoyed me that pgindent insists on adjusting vertical
whitespace around #else and related commands.  This has, for example,
rendered src/include/storage/barrier.h nigh illegible: you get things
like

Agreed.  I've similarly disliked how pgindent adds a blank line between an #if
and a multi-line comment, like at the top of get_restricted_token().

AFAICT it forces a blank line before a multiline comment in most
contexts; #if isn't special (though it does seem to avoid doing that
just after a left brace).  I too don't much care for that behavior,
although it's not as detrimental to readability as removing blank lines
can be.

In general, pgindent isn't nearly as good about managing vertical
whitespace as it is about horizontal spacing ...



I agree.

I should perhaps note that when I rewrote pgindent, I deliberately 
didn't editorialize about its behaviour - but I did intend to make it 
more maintainable and simpler to change stuff like this, and so it is :-)


cheers

andrew



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent behavior we could do without

[Bruce Momjian]

On Thu, Jul 18, 2013 at 12:27:21AM -0400, Tom Lane wrote:
 It's always annoyed me that pgindent insists on adjusting vertical
 whitespace around #else and related commands.  This has, for example,
 rendered src/include/storage/barrier.h nigh illegible: you get things
 like
 
 /*
  * lwsync orders loads with respect to each other, and similarly with stores.
  * But a load can be performed before a subsequent store, so sync must be used
  * for a full memory barrier.
  */
 #define pg_memory_barrier() __asm__ __volatile__ (sync : : : memory)
 #define pg_read_barrier()   __asm__ __volatile__ (lwsync : : : memory)
 #define pg_write_barrier()  __asm__ __volatile__ (lwsync : : : memory)
 #elif defined(__alpha) || defined(__alpha__)/* Alpha */
 
 which makes it look like this block of code has something to do with
 Alpha.
 
 By chance, I noticed today that this misbehavior comes from a discretely
 identifiable spot, to wit lines 289-290 in src/tools/pgindent/pgindent:
 
   # Remove blank line(s) before #else, #elif, and #endif
   $source =~ s!\n\n+(\#else|\#elif|\#endif)!\n$1!g;
 
 This seems pretty broken to me: why exactly is whitespace there such a
 bad idea?  Not only that, but the next action is concerned with undoing
 some of the damage this rule causes:
 
   # Add blank line before #endif if it is the last line in the file
   $source =~ s!\n(#endif.*)\n\z!\n\n$1\n!;
 
 I assert that we should simply remove both of these bits of code, as
 just about every committer on the project is smarter about when to use
 vertical whitespace than this program is.
 
 Thoughts?

Interesting.  I can't remember fully but the problem might be that BSD
indent was adding extra spacing around those, and I needed to remove it.
Would you like me to test a run and show you the output?  I can do that
next week when I return from vacation, or you can give it a try.  I am
fine with removing that code.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent behavior we could do without

[Noah Misch]

On Thu, Jul 18, 2013 at 12:27:21AM -0400, Tom Lane wrote:
 It's always annoyed me that pgindent insists on adjusting vertical
 whitespace around #else and related commands.  This has, for example,
 rendered src/include/storage/barrier.h nigh illegible: you get things
 like
 
 /*
  * lwsync orders loads with respect to each other, and similarly with stores.
  * But a load can be performed before a subsequent store, so sync must be used
  * for a full memory barrier.
  */
 #define pg_memory_barrier() __asm__ __volatile__ (sync : : : memory)
 #define pg_read_barrier()   __asm__ __volatile__ (lwsync : : : memory)
 #define pg_write_barrier()  __asm__ __volatile__ (lwsync : : : memory)
 #elif defined(__alpha) || defined(__alpha__)/* Alpha */
 
 which makes it look like this block of code has something to do with
 Alpha.

Agreed.  I've similarly disliked how pgindent adds a blank line between an #if
and a multi-line comment, like at the top of get_restricted_token().

-- 
Noah Misch
EnterpriseDB http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent behavior we could do without

[Tom Lane]

Noah Misch n...@leadboat.com writes:
 On Thu, Jul 18, 2013 at 12:27:21AM -0400, Tom Lane wrote:
 It's always annoyed me that pgindent insists on adjusting vertical
 whitespace around #else and related commands.  This has, for example,
 rendered src/include/storage/barrier.h nigh illegible: you get things
 like

 Agreed.  I've similarly disliked how pgindent adds a blank line between an #if
 and a multi-line comment, like at the top of get_restricted_token().

AFAICT it forces a blank line before a multiline comment in most
contexts; #if isn't special (though it does seem to avoid doing that
just after a left brace).  I too don't much care for that behavior,
although it's not as detrimental to readability as removing blank lines
can be.

In general, pgindent isn't nearly as good about managing vertical
whitespace as it is about horizontal spacing ...

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] pgindent behavior we could do without

[Tom Lane]

It's always annoyed me that pgindent insists on adjusting vertical
whitespace around #else and related commands.  This has, for example,
rendered src/include/storage/barrier.h nigh illegible: you get things
like

/*
 * lwsync orders loads with respect to each other, and similarly with stores.
 * But a load can be performed before a subsequent store, so sync must be used
 * for a full memory barrier.
 */
#define pg_memory_barrier() __asm__ __volatile__ (sync : : : memory)
#define pg_read_barrier()   __asm__ __volatile__ (lwsync : : : memory)
#define pg_write_barrier()  __asm__ __volatile__ (lwsync : : : memory)
#elif defined(__alpha) || defined(__alpha__)/* Alpha */

which makes it look like this block of code has something to do with
Alpha.

By chance, I noticed today that this misbehavior comes from a discretely
identifiable spot, to wit lines 289-290 in src/tools/pgindent/pgindent:

# Remove blank line(s) before #else, #elif, and #endif
$source =~ s!\n\n+(\#else|\#elif|\#endif)!\n$1!g;

This seems pretty broken to me: why exactly is whitespace there such a
bad idea?  Not only that, but the next action is concerned with undoing
some of the damage this rule causes:

# Add blank line before #endif if it is the last line in the file
$source =~ s!\n(#endif.*)\n\z!\n\n$1\n!;

I assert that we should simply remove both of these bits of code, as
just about every committer on the project is smarter about when to use
vertical whitespace than this program is.

Thoughts?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent README correction

[Bruce Momjian]

On Mon, Jan  9, 2012 at 11:31:02AM -0600, Kevin Grittner wrote:
 I found that I needed to adjust the command given in the README file
 for pgindent.  Trivial patch attached.
  
 The one other issue I ran into in following the latest pgindent
 instructions was that I had to add #include stdlib.h to the
 parse.c file (as included in the pg_bsd_indent-1.1.tar.gz file at
 ftp://ftp.postgresql.org/pub/dev ).  Without it I got this:
  
 parse.c: In function *parse*:
 parse.c:236:6: warning: implicit declaration of function *exit*
 parse.c:236:6: warning: incompatible implicit declaration of built-in
 function *exit*
  
 Can someone fix that and put up a 1.2 version?

Done.  Please give the ftp mirrors a little while to update, but you can
get pg_bsd_indent 1.2 now at ftpmaster.postgresql.org.  pgindent was
also updated to require the 1.2 version.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent README correction

[Magnus Hagander]

On Wednesday, February 8, 2012, Bruce Momjian wrote:

 On Mon, Jan 09, 2012 at 01:32:49PM -0500, Robert Haas wrote:
   The one other issue I ran into in following the latest pgindent
   instructions was that I had to add #include stdlib.h to the
   parse.c file (as included in the pg_bsd_indent-1.1.tar.gz file at
   ftp://ftp.postgresql.org/pub/dev ).  Without it I got this:
  
   parse.c: In function *parse*:
   parse.c:236:6: warning: implicit declaration of function *exit*
   parse.c:236:6: warning: incompatible implicit declaration of built-in
   function *exit*
  
   Can someone fix that and put up a 1.2 version?
 
  Sounds like a job for Mr. Momjian.

 What server do I log into to update the ftp pgindent tgz file?


The ftp master server is fornax.postgresql.org.

//Magnus



-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [pgsql-www] [HACKERS] pgindent README correction

[Dave Page]

On Wed, Feb 8, 2012 at 8:13 AM, Magnus Hagander mag...@hagander.net wrote:
 On Wednesday, February 8, 2012, Bruce Momjian wrote:

 On Mon, Jan 09, 2012 at 01:32:49PM -0500, Robert Haas wrote:
   The one other issue I ran into in following the latest pgindent
   instructions was that I had to add #include stdlib.h to the
   parse.c file (as included in the pg_bsd_indent-1.1.tar.gz file at
   ftp://ftp.postgresql.org/pub/dev ).  Without it I got this:
  
   parse.c: In function *parse*:
   parse.c:236:6: warning: implicit declaration of function *exit*
   parse.c:236:6: warning: incompatible implicit declaration of built-in
   function *exit*
  
   Can someone fix that and put up a 1.2 version?
 
  Sounds like a job for Mr. Momjian.

 What server do I log into to update the ftp pgindent tgz file?


 The ftp master server is fornax.postgresql.org.

Though, you should bookmark ftpmaster.postgresql.org which will always
point to the master FTP server even if it moves to a different host.

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent README correction

[Bruce Momjian]

On Mon, Jan 09, 2012 at 01:32:49PM -0500, Robert Haas wrote:
  The one other issue I ran into in following the latest pgindent
  instructions was that I had to add #include stdlib.h to the
  parse.c file (as included in the pg_bsd_indent-1.1.tar.gz file at
  ftp://ftp.postgresql.org/pub/dev ).  Without it I got this:
 
  parse.c: In function *parse*:
  parse.c:236:6: warning: implicit declaration of function *exit*
  parse.c:236:6: warning: incompatible implicit declaration of built-in
  function *exit*
 
  Can someone fix that and put up a 1.2 version?
 
 Sounds like a job for Mr. Momjian.

What server do I log into to update the ftp pgindent tgz file?

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] pgindent README correction

[Kevin Grittner]

I found that I needed to adjust the command given in the README file
for pgindent.  Trivial patch attached.
 
The one other issue I ran into in following the latest pgindent
instructions was that I had to add #include stdlib.h to the
parse.c file (as included in the pg_bsd_indent-1.1.tar.gz file at
ftp://ftp.postgresql.org/pub/dev ).  Without it I got this:
 
parse.c: In function *parse*:
parse.c:236:6: warning: implicit declaration of function *exit*
parse.c:236:6: warning: incompatible implicit declaration of built-in
function *exit*
 
Can someone fix that and put up a 1.2 version?
 
-Kevin

diff --git a/src/tools/pgindent/README b/src/tools/pgindent/README
index d88c201..a47b809 100644
--- a/src/tools/pgindent/README
+++ b/src/tools/pgindent/README
@@ -22,7 +22,7 @@ This can format all PostgreSQL *.c and *.h files, but 
excludes *.y, and
 
find . -name '*.[ch]' -type f -print | \
egrep -v -f src/tools/pgindent/exclude_file_patterns | \
-   xargs -n100 pgindent src/tools/pgindent/typedefs.list
+   xargs -n100 src/tools/pgindent/pgindent src/tools/pgindent/typedefs.list
 
 6) Remove any files that generate errors and restore their original
versions.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent README correction

[Robert Haas]

On Mon, Jan 9, 2012 at 12:31 PM, Kevin Grittner
kevin.gritt...@wicourts.gov wrote:
 I found that I needed to adjust the command given in the README file
 for pgindent.  Trivial patch attached.

Committed.

 The one other issue I ran into in following the latest pgindent
 instructions was that I had to add #include stdlib.h to the
 parse.c file (as included in the pg_bsd_indent-1.1.tar.gz file at
 ftp://ftp.postgresql.org/pub/dev ).  Without it I got this:

 parse.c: In function *parse*:
 parse.c:236:6: warning: implicit declaration of function *exit*
 parse.c:236:6: warning: incompatible implicit declaration of built-in
 function *exit*

 Can someone fix that and put up a 1.2 version?

Sounds like a job for Mr. Momjian.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Bruce Momjian]

Tom Lane wrote:
 Bruce Momjian br...@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  br...@momjian.ushttp://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 */

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] pgindent messing up translator: comments

[Alvaro Herrera]

I just noticed that this comment got reindented by pgindent
(xlog.c, line 3226 in REL9_1_STABLE):
/*
 * translator: First %s represents a recovery.conf parameter 
name like
 * recovery_end_command, and the 2nd is the value of that 
parameter.
 */
ereport((signaled  failOnSignal) ? FATAL : WARNING,
(errmsg(%s \%s\: return code %d, 
commandName,
command, rc)));

Sure enough, the resulting POT entry does not have the necessary
comment:

#: /pgsql/source/REL9_1_STABLE/src/backend/access/transam/xlog.c:3230
#, c-format
msgid %s \%s\: return code %d
msgstr 

I think the proper fix would be to use the /* trick, such as in
postmaster.c:

/*--
  translator: %s is a noun phrase describing a child process, 
such as
  server process */
(errmsg(%s (PID %d) exited with exit code %d,
procname, pid, 
WEXITSTATUS(exitstatus;

It seems to me that we should alert if pgindent does anything to a
comment line containing translator:.

-- 
Álvaro Herrera alvhe...@alvh.no-ip.org

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent messing up translator: comments

[Bruce Momjian]

Alvaro Herrera wrote:
 I just noticed that this comment got reindented by pgindent
 (xlog.c, line 3226 in REL9_1_STABLE):
   /*
* translator: First %s represents a recovery.conf parameter 
 name like
* recovery_end_command, and the 2nd is the value of that 
 parameter.
*/
   ereport((signaled  failOnSignal) ? FATAL : WARNING,
   (errmsg(%s \%s\: return code %d, 
 commandName,
   command, rc)));
 
 Sure enough, the resulting POT entry does not have the necessary
 comment:
 
 #: /pgsql/source/REL9_1_STABLE/src/backend/access/transam/xlog.c:3230
 #, c-format
 msgid %s \%s\: return code %d
 msgstr 
 
 I think the proper fix would be to use the /* trick, such as in
 postmaster.c:
 
   /*--
 translator: %s is a noun phrase describing a child process, 
 such as
 server process */
   (errmsg(%s (PID %d) exited with exit code %d,
   procname, pid, 
 WEXITSTATUS(exitstatus;
 
 It seems to me that we should alert if pgindent does anything to a
 comment line containing translator:.

Well, the comment adjustments happen in the C code, which is hard to
modify.  We would need a wrapper that understood when it was in a C
command and add /*--- markers if the word 'translator:' appeared.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent messing up translator: comments

[Alvaro Herrera]

Excerpts from Alvaro Herrera's message of lun sep 05 16:21:38 -0300 2011:
 I just noticed that this comment got reindented by pgindent
 (xlog.c, line 3226 in REL9_1_STABLE):
 /*
  * translator: First %s represents a recovery.conf parameter name like
  * recovery_end_command, and the 2nd is the value of that parameter.
  */
 ereport((signaled  failOnSignal) ? FATAL : WARNING,
 (errmsg(%s \%s\: return code %d, commandName,
 command, rc)));

Actually, after I looked into Git history it turns out that this comment
was introduced in this way; it wasn't pgindent's fault.  I checked a
couple of diffs from pgindent runs, and I found no translator: comment
reindented destructively.  Still, it seems possible that it could happen
someday.

I will fix this one occurence.

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent messing up translator: comments

[Tom Lane]

Alvaro Herrera alvhe...@alvh.no-ip.org writes:
 I think the proper fix would be to use the /* trick, such as in
 postmaster.c:

   /*--
 translator: %s is a noun phrase describing a child process, 
 such as
 server process */
   (errmsg(%s (PID %d) exited with exit code %d,
   procname, pid, 
 WEXITSTATUS(exitstatus;

Ugh.  Are the gettext tools so broken that they force us to use that
(very ugly IMO) layout for translator: comments?  Why can't we get
the tools fixed instead?

By and large, the people who put in those comments don't know about any
specialized restrictions that gettext might have on the layout of the
comment; the only documentation I've ever seen just says that the
comment has to start with translator::
http://developer.postgresql.org/pgdocs/postgres/nls-programmer.html

I think that if gettext can't handle the comment as it stands, that's
a gettext bug, not something that both pgindent and the human code
authors ought to be subservient to.  Or at the very least, I want to see
an exact specification for what the allowed format is, and it had better
not be very magical.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent messing up translator: comments

[Alvaro Herrera]

Excerpts from Tom Lane's message of lun sep 05 16:43:32 -0300 2011:
 Alvaro Herrera alvhe...@alvh.no-ip.org writes:
  I think the proper fix would be to use the /* trick, such as in
  postmaster.c:
 
  /*--
translator: %s is a noun phrase describing a child process, such 
  as
server process */
  (errmsg(%s (PID %d) exited with exit code %d,
  procname, pid, WEXITSTATUS(exitstatus;
 
 Ugh.  Are the gettext tools so broken that they force us to use that
 (very ugly IMO) layout for translator: comments?  Why can't we get
 the tools fixed instead?
 
 By and large, the people who put in those comments don't know about any
 specialized restrictions that gettext might have on the layout of the
 comment; the only documentation I've ever seen just says that the
 comment has to start with translator::
 http://developer.postgresql.org/pgdocs/postgres/nls-programmer.html

Well, this is all the xgettext manpage says:

   -cTAG, --add-comments=TAG
  place comment blocks starting with TAG and preceding keyword 
lines in output file

I think nobody bothers to fix this because everyone else is using the
GNU indentation style, which would make the message look like this:

/* translator: %s is a noun phrase describing a child process,
such as server process */
errmsg( ... );


 I think that if gettext can't handle the comment as it stands, that's
 a gettext bug, not something that both pgindent and the human code
 authors ought to be subservient to.  Or at the very least, I want to see
 an exact specification for what the allowed format is, and it had better
 not be very magical.

Hmm.  I think the only other place than the above line in the manpage
where this is mentioned in the manual, is this:

http://www.gnu.org/software/gettext/manual/gettext.html#Bug-Report-Address

No mention of the format is done anywhere.

This seems related to this (unanswered) bug report:
http://savannah.gnu.org/bugs/?33451

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Bruce Momjian]

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)
  {
 booluse_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  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Andrew Dunstan]

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)
  {
 booluse_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:

   140055: 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
   140060: Abbrev Number: 7 (DW_TAG_pointer_type)
   40061   DW_AT_byte_size   : 8
   40062   DW_AT_type: 0x40066
   140066: 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
addrinfo
aff_struct
AggHashEntryData
AggStatePerAggData
AllocBlockData
AllocChunkData
arc
arcbatch
arcp
ArrayIteratorData
ASIdentifiers_st
ASN1_ENCODING_st
asn1_object_st
asn1_string_st
asn1_type_st
ASN1_VALUE_st
attrDefault
AUTHORITY_KEYID_st
BgWriterSlotMapping
bignum_ctx
bignum_st
bio_method_st
bio_st
bkend
bn_blinding_st
bn_gencb_st
bn_mont_ctx_st
BufferAccessStrategyData
buf_mem_st
buftag
BulkInsertStateData
cachedesc
carc
catcache
catcacheheader
catclist
catctup
cert_st
cname
cnfa
colordesc
colormap
colors
comp_ctx_st
comp_method_st
config_bool
config_enum
config_enum_entry
config_generic
config_int
config_real
config_string
ConnectionOption
constrCheck
crypto_ex_data_st
cvec
_DestReceiver
dfa
df_files
dh_method
dh_st
dirent
__dirstream
dropmsgstrings
dsa_method
DSA_SIG_st
dsa_st
dtls1_bitmap_st
dtls1_retransmit_state
dtls1_state_st
dtls1_timeout_st
encoding_match
engine_st
env_md_ctx_st
env_md_st
evp_cipher_ctx_st
evp_cipher_st
evp_pkey_asn1_method_st
evp_pkey_ctx_st
evp_pkey_st
fmgr_security_definer_cache
fns
fp_info
_FuncCandidateList
GinScanEntryData
GinScanKeyData
GlobalTransactionData
group
gss_buffer_desc_struct
gss_channel_bindings_struct
gss_cred_id_struct
gss_ctx_id_struct
gss_name_struct
gss_OID_desc_struct
guc_stack
guts
HashJoinTableData
HashJoinTupleData
HeapScanDescData
hmac_ctx_st
hm_header_st
ifaddrs
in6_addr
in_addr
_IndexList
IndexScanDescData
_IO_FILE
_IO_marker
ipc_perm
ISSUING_DIST_POINT_st
itimerval
__jmp_buf_tag
kssl_ctx_st
lconv
lc_time_T
ldap
ldapmsg
lhash_st_SSL_SESSION
__locale_data
__locale_struct
lsinfo
mbinterval
_MdfdVec
MergeJoinClauseData
NAME_CONSTRAINTS_st
nameData
nfa
NumericData
NumericLong
NumericShort
OldSerXidControlData
ONEXIT
opclasscacheent
pam_conv
pam_handle
pam_message
pam_response
ParamListInfoData
passwd
pg_encoding
pg_tm
pollfd
PortalData
portalhashent
_pqueue
PredXactListData
PredXactListElementData
ptrs

Re: [HACKERS] pgindent weirdness

[Tom Lane]

Andrew Dunstan and...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdnessf

[Bruce Momjian]

Andrew Dunstan wrote:
 It's tagged as a structure type by objdump, but not as a typedef:
 
 140055: 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
 140060: Abbrev Number: 7 (DW_TAG_pointer_type)
 40061   DW_AT_byte_size   : 8
 40062   DW_AT_type: 0x40066
 140066: 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  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Bruce Momjian]

Tom Lane wrote:
 Andrew Dunstan and...@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  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Tom Lane]

Bruce Momjian br...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Bruce Momjian]

Tom Lane wrote:
 Bruce Momjian br...@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  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Andrew Dunstan]

On 04/20/2011 11:43 AM, Tom Lane wrote:

Andrew Dunstanand...@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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Tom Lane]

Andrew Dunstan and...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdnessf

[Andrew Dunstan]

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Andrew Dunstan]

On 04/20/2011 12:29 PM, Tom Lane wrote:

Andrew Dunstanand...@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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Aidan Van Dyk]

On Wed, Apr 20, 2011 at 12:38 PM, Andrew Dunstan and...@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,
ai...@highrise.ca                                       command like a king,
http://www.highrise.ca/                                   work like a slave.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Tom Lane]

Aidan Van Dyk ai...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Robert Haas]

On Wed, Apr 20, 2011 at 11:15 AM, Andrew Dunstan and...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Bruce Momjian]

Robert Haas wrote:
 On Wed, Apr 20, 2011 at 11:15 AM, Andrew Dunstan and...@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  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Andrew Dunstan]

On 04/20/2011 01:12 PM, Robert Haas wrote:

On Wed, Apr 20, 2011 at 11:15 AM, Andrew Dunstanand...@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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdnessf

[Robert Haas]

On Wed, Apr 20, 2011 at 12:33 PM, Andrew Dunstan and...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Andrew Dunstan]

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdnessf

[Bruce Momjian]

Robert Haas wrote:
 On Wed, Apr 20, 2011 at 12:33 PM, Andrew Dunstan and...@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  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdnessf

[Andrew Dunstan]

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Andrew Dunstan]

On 04/20/2011 01:10 PM, Tom Lane wrote:

Aidan Van Dykai...@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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Tom Lane]

Andrew Dunstan and...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Bruce Momjian]

Tom Lane wrote:
 Andrew Dunstan and...@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  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Andrew Dunstan]

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Tom Lane]

Andrew Dunstan and...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Bruce Momjian]

Tom Lane wrote:
 Andrew Dunstan and...@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  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent weirdness

[Andrew Dunstan]

On 04/20/2011 05:29 PM, Tom Lane wrote:

Andrew Dunstanand...@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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] pgindent weirdness

[Robert Haas]

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)
 {
booluse_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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent

[Bruce Momjian]

Bruce Momjian wrote:
 Robert Haas wrote:
  On Fri, Apr 8, 2011 at 11:21 PM, Andrew Dunstan and...@dunslane.net wrote:
   We've got more work to do before that works, so I have committed what we
   have. Some symbols have disappeared, some because of code changes and some
   probably because Cygwin has changed the way it does objdump. This is
   probably harmless, but whoever does the pgindent run needs to look at the
   results carefully before committing them (as always).
  
  Well, that's normally Bruce.  Bruce?
 
 I can run it tonight, in 15 hours.

27 hours later, done.   I ran all the tests outlined in the pgindent
README.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers