Re: Lessons from commit fest

There has been talk of the lessons we learned during this commit fest,
but exactly what lessons did we learn? I am not clear on that so I
assume others are not as well. What are we going to do differently
during the next commit fest?

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

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

Bruce Momjian wrote:
> There has been talk of the lessons we learned during this commit fest,
> but exactly what lessons did we learn? I am not clear on that so I
> assume others are not as well. What are we going to do differently
> during the next commit fest?

As far as the Wiki page is concerned, it would be good to make sure the
entries have a bit more info than just a header line -- things such as
"author", who reviewed and what did the reviewer say about it.

Some of it is already there.

Something else we learned is that the archives are central (well, we
already knew that, but I don't think we had ever given them so broad
use), and we've been making changes to them so that they are more useful
to reviewers. Further changes are still needed on them, of course, to
address the remaining problems.

Lastly, I would say that pushing submitters to enter their sent patches
into the Wiki worked -- we need to ensure that they keep doing it.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Tue, Apr 15, 2008 at 2:45 AM, Alvaro Herrera wrote:
> As far as the Wiki page is concerned, it would be good to make sure the
> entries have a bit more info than just a header line -- things such as
> "author", who reviewed and what did the reviewer say about it.
>

In a wiki context, this sort of thing has got "Template" written all
over it. I've done a little editing on Wikipedia, so I've got some
idea about how to make wiki Templates work. I'm not claiming to be an
expert, but if nobody else has a particular yen for it, I can have a
go at setting up some simple templates to make it easier to add a
"patch" or a "review" in a structured way.

> Lastly, I would say that pushing submitters to enter their sent patches
> into the Wiki worked -- we need to ensure that they keep doing it.
>

+1. Although I wouldn't say that there has yet been a "push" for
submitters to enter their patches into the wiki =) I've started
adding my own patches to the wiki recently. The only thing about the
process that sucks is that I need a URL linking to the message in the
archives. I naturally want to add my patch to the wiki immediately
after sending my email to -patches, and it takes some material
interval of time for messages to show up on the archive. My solution
was to just pull the message ID out of the headers in gmail and fudge
the URL. So the URL I add to the wiki is actually borked until the
archives refresh themselves, which is less than awesome ...

Apart from the archive linkage thing, I found the process of queueing
my own patches smooth, straightforward and satisfying. I would
recommend it to other submitters, if for no other reason than to
reduce the amount of drudge work the core team has to do to keep
things in order.

Cheers,
BJ

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.7 (GNU/Linux)
Comment: http://getfiregpg.org

iD8DBQFIA5Jl5YBsbHkuyV0RAnL0AJ97+ZdXr71Xu6wMSlVGSvy1t4WqbgCgz58X
GHMaO7j4g8+WTmcNx2SKBnA=
=xPJ3
-----END PGP SIGNATURE-----

Brendan Jurd escribió:

> On Tue, Apr 15, 2008 at 2:45 AM, Alvaro Herrera wrote:
> > As far as the Wiki page is concerned, it would be good to make sure the
> > entries have a bit more info than just a header line -- things such as
> > "author", who reviewed and what did the reviewer say about it.
>
> In a wiki context, this sort of thing has got "Template" written all
> over it. I've done a little editing on Wikipedia, so I've got some
> idea about how to make wiki Templates work. I'm not claiming to be an
> expert, but if nobody else has a particular yen for it, I can have a
> go at setting up some simple templates to make it easier to add a
> "patch" or a "review" in a structured way.

Please have a go at it in a test page. The main principle we need is
that the thing must be as easy as possible to edit. (FWIW if you can
come up with a better markup than what we currently use, we'd welcome
the advise.)

> > Lastly, I would say that pushing submitters to enter their sent patches
> > into the Wiki worked -- we need to ensure that they keep doing it.
>
> +1. Although I wouldn't say that there has yet been a "push" for
> submitters to enter their patches into the wiki =)

Well, I pushed some authors via private email. Others did not seem to
need any pushing :-)

> I've started adding my own patches to the wiki recently. The only
> thing about the process that sucks is that I need a URL linking to the
> message in the archives. I naturally want to add my patch to the wiki
> immediately after sending my email to -patches, and it takes some
> material interval of time for messages to show up on the archive. My
> solution was to just pull the message ID out of the headers in gmail
> and fudge the URL.

Right, that's the way I use them too. Sorry we don't have anything
better :-( The archives are refreshed every 10 minutes, so it's not
like you need to wait for a week, either.

Of course, I've configured my email client so that Message-Ids are shown
all over the place, so I don't have to mess around clicking stuff.

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

Brendan Jurd wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On Tue, Apr 15, 2008 at 2:45 AM, Alvaro Herrera wrote:
> > As far as the Wiki page is concerned, it would be good to make sure the
> > entries have a bit more info than just a header line -- things such as
> > "author", who reviewed and what did the reviewer say about it.
> >
>
> In a wiki context, this sort of thing has got "Template" written all
> over it. I've done a little editing on Wikipedia, so I've got some
> idea about how to make wiki Templates work. I'm not claiming to be an
> expert, but if nobody else has a particular yen for it, I can have a
> go at setting up some simple templates to make it easier to add a
> "patch" or a "review" in a structured way.

One problem I saw is that people commenting in the wiki sometimes didn't
leave their names. It would be nice if that could be improved.

> > Lastly, I would say that pushing submitters to enter their sent patches
> > into the Wiki worked -- we need to ensure that they keep doing it.
> >
>
> +1. Although I wouldn't say that there has yet been a "push" for
> submitters to enter their patches into the wiki =) I've started
> adding my own patches to the wiki recently. The only thing about the
> process that sucks is that I need a URL linking to the message in the
> archives. I naturally want to add my patch to the wiki immediately
> after sending my email to -patches, and it takes some material
> interval of time for messages to show up on the archive. My solution
> was to just pull the message ID out of the headers in gmail and fudge
> the URL. So the URL I add to the wiki is actually borked until the
> archives refresh themselves, which is less than awesome ...

Yea, I have to wait to add URLs to the TODO list too, but as you
mentioned I can now use message-id URLs, though I try to avoid them
beccause they aren't as attractive --- doesn't matter for the wiki.

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

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

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> As far as the Wiki page is concerned, it would be good to make sure the
> entries have a bit more info than just a header line -- things such as
> "author", who reviewed and what did the reviewer say about it.

I think it'd be easy to go overboard there. One thing we learned from
Bruce's page is that a display with ten or more lines per work item is
not very helpful ... you start wishing you had a summary, and then the
whole design cycle repeats.

We should not try to make the wiki page be a substitute for reading the
linked-to discussions. (This is one of the reasons that dropped links
in the archives, such as the month-end problem, are so nasty.)

One idea is to make more effective use of horizontal space than we were
doing with the current wiki-page markup. I'd have no objection to
including the author's name and a status indicator if it all fit on the
same line as the patch title/link.

regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Tue, Apr 15, 2008 at 4:12 AM, Tom Lane wrote:
> Alvaro Herrera writes:
> > As far as the Wiki page is concerned, it would be good to make sure the
> > entries have a bit more info than just a header line -- things such as
> > "author", who reviewed and what did the reviewer say about it.
>
> We should not try to make the wiki page be a substitute for reading the
> linked-to discussions. (This is one of the reasons that dropped links
> in the archives, such as the month-end problem, are so nasty.)
>
> One idea is to make more effective use of horizontal space than we were
> doing with the current wiki-page markup. I'd have no objection to
> including the author's name and a status indicator if it all fit on the
> same line as the patch title/link.
>

When you say "horizontal space", I start thinking in terms of a table,
like the ancestor of the current wiki commitfest queue, ye olde
http://developer.postgresql.org/index.php/Todo:PatchStatus

However, the table format doesn't provide well for brief review
comments, such as we currently have for the 64bit pass-by-value patch
in the May CommitFest page.

Personally I think the review addenda are nice. As Tom says, it
shouldn't be a replacement for actually reading the thread, but it
helps to get people up to speed on the status of the patch. I think
it's a useful primer for those who may be interested in looking at the
patch in more detail.

Anyway, take a look at http://wiki.postgresql.org/wiki/Template:Patch,
and http://wiki.postgresql.org/wiki/User_talk:Direvus for an example
of how it is used. This is my first stab at writing a patch queue
entry template. It uses a similar structure to what's already on the
CommitFest pages.

To make this work, all a patch submitter needs to do is go to the
relevant CommitFest page and add {{patch|||}} to the Pending Patches
section.

If you guys find this useful, I'd be happy to add a "review" template
in a similar vein.

Cheers,
BJ
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.7 (GNU/Linux)
Comment: http://getfiregpg.org

iD8DBQFIA6eX5YBsbHkuyV0RAkzNAKDEbtZZOyUI1olsErxgp1o39dH3VQCfcBQN
b25k/nEy7qupYsthhPtU7eQ=
=uu+E
-----END PGP SIGNATURE-----

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> There has been talk of the lessons we learned during this commit fest,
> but exactly what lessons did we learn?

Actually, the *main* lesson we learned was "don't start with a
2000-email inbox". There were a couple of reasons that the queue
was so forbidding:

1. We were in feature freeze for 11 months and consequently built up
a pretty sizable backlog of stuff that had been postponed to 8.4.
We have to avoid ever doing that again. We've already made some
process changes to try to avoid getting stuck that way, and we have
to be willing to change some more if the current plan doesn't work.
But that wasn't a lesson of the commit fest, we already knew it was
broken :-(. This was just inevitable pain from our poor management
of the last release cycle.

2. A whole lot of the 2000 emails were not actually about reviewable
patches. I'm willing to take most of the blame here --- I pushed
Bruce to publish the list before he'd finished doing as much clean-up
as he wanted, and I also encouraged him to leave in some long design
discussion threads that seemed to me to warrant more discussion.
(And part of the reason I thought so was that I'd deliberately ignored
those same threads when they were active, because I was busy trying
to get 8.3 out the door; so again this was partly delayed pain from
the 8.3 mess.)

In hindsight we didn't get very much design discussion done during
the fest, and I think it's unlikely to happen in future either.
We should probably try to limit the focus of fests to consider only
actual patches, with design discussions handled "live" during normal
development, the way they've always been in the past.

A smaller lesson was that you can't start fest without a queue of
ready-to-work-on patches. We seem to be evolving towards a plan
where stuff gets dumped onto the wiki page more or less immediately
as it comes in. That should take care of that problem, though I'd
still like to see someone accept responsibility for making sure
patches get listed whether or not their author does it.

Other lessons that were already brought up:
* Bruce's mbox page was not a good status tool, despite his efforts
to improve it;
* If Bruce and I are the only ones doing anything, it's gonna be slow.

regards, tom lane

Tom Lane wrote:

> A smaller lesson was that you can't start fest without a queue of
> ready-to-work-on patches. We seem to be evolving towards a plan
> where stuff gets dumped onto the wiki page more or less immediately
> as it comes in. That should take care of that problem, though I'd
> still like to see someone accept responsibility for making sure
> patches get listed whether or not their author does it.

I'm on it. If someone wants to act as backup, he or she is more than
welcome.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Tom Lane wrote:
> the fest, and I think it's unlikely to happen in future either.
> We should probably try to limit the focus of fests to consider only
> actual patches, with design discussions handled "live" during normal
> development, the way they've always been in the past.

We have discouraged design discussions during feature freeze and beta ---
do we want to change that?

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

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

Tom Lane wrote:
> A smaller lesson was that you can't start fest without a queue of
> ready-to-work-on patches. We seem to be evolving towards a plan
> where stuff gets dumped onto the wiki page more or less immediately
> as it comes in. That should take care of that problem, though I'd
> still like to see someone accept responsibility for making sure
> patches get listed whether or not their author does it.
>
> Other lessons that were already brought up:
> * Bruce's mbox page was not a good status tool, despite his efforts
> to improve it;
> * If Bruce and I are the only ones doing anything, it's gonna be slow.

Even after the wiki was setup there was still limited involvement in
patch application except for me and Tom. The wiki is going to allow
people to see more easily what is outstanding, but it doesn't seem to
have translated into more people involved in finishing the commit fest.

Humorously, it is like televising a football game --- more people watch,
but it doesn't help the players on the field. :-(

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

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

Bruce Momjian wrote:
> Tom Lane wrote:
> > the fest, and I think it's unlikely to happen in future either.
> > We should probably try to limit the focus of fests to consider only
> > actual patches, with design discussions handled "live" during normal
> > development, the way they've always been in the past.
>
> We have discouraged design discussions during feature freeze and beta ---
> do we want to change that?

In fact, discussions were discouraged during the March commitfest too.

Perhaps this is a good idea -- discussions on new development should
be deferred until the end of the commitfest, if one is in progress.
We'll see what happens on the May commitfest, but my guess is that it
will be a lot shorter than the March one. If it takes 1.5-2 weeks, it's
not so bad, and it means people is eager to get the current patches done
as quickly as possible so that discussions on items they are interested
in can continue.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Bruce Momjian wrote:
> > Tom Lane wrote:
> > > the fest, and I think it's unlikely to happen in future either.
> > > We should probably try to limit the focus of fests to consider only
> > > actual patches, with design discussions handled "live" during normal
> > > development, the way they've always been in the past.
> >
> > We have discouraged design discussions during feature freeze and beta ---
> > do we want to change that?
>
> In fact, discussions were discouraged during the March commitfest too.
>
> Perhaps this is a good idea -- discussions on new development should
> be deferred until the end of the commitfest, if one is in progress.
> We'll see what happens on the May commitfest, but my guess is that it
> will be a lot shorter than the March one. If it takes 1.5-2 weeks, it's
> not so bad, and it means people is eager to get the current patches done
> as quickly as possible so that discussions on items they are interested
> in can continue.

If you do that then the discussions bunch up, which is part of the
reason we had 2k emails for the March commit fest. Tom is saying not to
delay those discussions.

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

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

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Tom Lane wrote:
>> We should probably try to limit the focus of fests to consider only
>> actual patches, with design discussions handled "live" during normal
>> development, the way they've always been in the past.

> We have discouraged design discussions during feature freeze and beta ---
> do we want to change that?

No, changing that wasn't what I meant to suggest. My point here is that
we'd dropped a number of big mushy discussions into the queue with the
idea that they'd be re-opened during commit fest, but new discussion
did not happen to any useful extent. Conclusion: don't bother putting
anything less concrete than a patch on the fest queue.

regards, tom lane

Bruce Momjian wrote:
> Alvaro Herrera wrote:

> > Perhaps this is a good idea -- discussions on new development should
> > be deferred until the end of the commitfest, if one is in progress.
> > We'll see what happens on the May commitfest, but my guess is that it
> > will be a lot shorter than the March one. If it takes 1.5-2 weeks, it's
> > not so bad, and it means people is eager to get the current patches done
> > as quickly as possible so that discussions on items they are interested
> > in can continue.

> If you do that then the discussions bunch up, which is part of the
> reason we had 2k emails for the March commit fest. Tom is saying not to
> delay those discussions.

The problem here was that the discussions bunched up during a full year.
Having them bunch up for a couple of weeks is not so bad.

And the problem with those discussions continuing is that the developers
participating in them are not working in the items in the queue. We
want to encourage them to review the things already done, not to get
distracted on new stuff.

Of course, it is cat-herding, so it's not going to work 100%.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Perhaps this is a good idea -- discussions on new development should
> be deferred until the end of the commitfest, if one is in progress.

Well, that was part of the original idea of a commit fest, on the theory
that everyone should be helping review/commit instead. But not that
many people seem to have bought into it ...

> We'll see what happens on the May commitfest, but my guess is that it
> will be a lot shorter than the March one. If it takes 1.5-2 weeks, it's
> not so bad, and it means people is eager to get the current patches done
> as quickly as possible so that discussions on items they are interested
> in can continue.

Yeah. This first one was going to be an aberration from the get-go,
just because of the backlog. I don't think we should draw too many
conclusions from it about how future ones will go. But they'd *better*
be shorter.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Tom Lane wrote:
> >> We should probably try to limit the focus of fests to consider only
> >> actual patches, with design discussions handled "live" during normal
> >> development, the way they've always been in the past.
>
> > We have discouraged design discussions during feature freeze and beta ---
> > do we want to change that?
>
> No, changing that wasn't what I meant to suggest. My point here is that
> we'd dropped a number of big mushy discussions into the queue with the
> idea that they'd be re-opened during commit fest, but new discussion
> did not happen to any useful extent. Conclusion: don't bother putting
> anything less concrete than a patch on the fest queue.

So when/how do those discussions get resolved?

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

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

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Other lessons that were already brought up:
> * Bruce's mbox page was not a good status tool, despite his efforts
> to improve it;
> * If Bruce and I are the only ones doing anything, it's gonna be slow.

How did you feel about the idea of having a Tom-free commitfest for May? You
would get to sit back and comment on other people's attempt to review patches,
just shouting if they seem to be headed in the wrong direction. And of course
work on your own ideas you've probably been itching to do since before 8.3
feature freeze.

I assume you realize it's not that I don't appreciate having you doing all
this work but I think it would be a good exercise for us to go through do
once. (And you certainly deserve a break!)

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's 24x7 Postgres support!

On Mon, 14 Apr 2008 21:12:59 +0100
Gregory Stark <stark(at)enterprisedb(dot)com> wrote:

> I assume you realize it's not that I don't appreciate having you
> doing all this work but I think it would be a good exercise for us to
> go through do once. (And you certainly deserve a break!)
>

Although I applaud the sentiment I think a more reasonable approach
would be (if Tom wanted) to have Tom pick 3-5 patches (or whatever) that
are his deal. That's it. No extra bubble activities for you. Except for
Buddha level oversight the rest falls on the rest of the community.

It isn't like we don't have at least 6 major contributors that can do
patch review... Alvaro, Greg, Neil, AndrewD, Heikki, and Magnus ... Not
to mention a slew of others who can at least lend a hand.

Sincerely,

Joshua D. Drake

--
The PostgreSQL Company since 1997: http://www.commandprompt.com/
PostgreSQL Community Conference: http://www.postgresqlconference.org/
United States PostgreSQL Association: http://www.postgresql.us/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Tom Lane wrote:
>> No, changing that wasn't what I meant to suggest. My point here is that
>> we'd dropped a number of big mushy discussions into the queue with the
>> idea that they'd be re-opened during commit fest, but new discussion
>> did not happen to any useful extent. Conclusion: don't bother putting
>> anything less concrete than a patch on the fest queue.

> So when/how do those discussions get resolved?

[ shrug... ] You can't force ideas to happen on a schedule.

If you don't want an issue to get forgotten, then make a TODO entry for
it. But the purpose of commit fest is to make sure we deal with things
that can be dealt with in a timely fashion. It's not going to cause
solutions to unsolved problems to appear from nowhere.

regards, tom lane

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> How did you feel about the idea of having a Tom-free commitfest for May?

Not a lot, though certainly I'd be willing to disengage from trivial
patches if someone else picked them up.

One problem with this fest was that a whole lot of the patches *weren't*
trivial; if they had been they'd not have gotten put off till 8.4. So
there weren't that many that I wanted to let go in without looking at
them. I guess that's just another way in which the 8.3 schedule problem
came home to roost during this fest.

In future fests we should have a much higher proportion of "little
things" that maybe more people would feel comfortable taking
responsibility for.

Perhaps it would be useful to try to rate pending patches by difficulty?

regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Tue, Apr 15, 2008 at 6:39 AM, Tom Lane wrote:
>
> Perhaps it would be useful to try to rate pending patches by difficulty?
>

Just a thought, but the file size of a context diff has a pretty good
correlation to the patch's intrusiveness / complexity. As a metric of
difficulty it's very naive, but it's also incredibly easy to measure
...

Cheers,
BJ
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.7 (GNU/Linux)
Comment: http://getfiregpg.org

iD8DBQFIA8JF5YBsbHkuyV0RAgZ4AKDOAnKFetNEAXvMUIZZS3MUgj2yagCfZrD9
ogJzfeRMbhX/Jc37wXLDDTk=
=wRkJ
-----END PGP SIGNATURE-----

Brendan Jurd escribió:

> Anyway, take a look at http://wiki.postgresql.org/wiki/Template:Patch,
> and http://wiki.postgresql.org/wiki/User_talk:Direvus for an example
> of how it is used. This is my first stab at writing a patch queue
> entry template. It uses a similar structure to what's already on the
> CommitFest pages.

Thanks, I changed a couple of patches to this method, and quickly hacked
up added a new template for reviews.

Re: horizontal whitespace in the patch template, I wonder if we can put
the author name and status in the same line as the patch name.
Separated from the patch name -- perhaps aligned right, if possible.

Maybe something like

<bold blue>Foobar the Bozos</bold blue> (<bold black>Status</bold black) <lotsa whitespace> Author

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Tue, Apr 15, 2008 at 7:00 AM, Alvaro Herrera wrote:
>
> Thanks, I changed a couple of patches to this method, and quickly hacked
> up added a new template for reviews.
>
> Re: horizontal whitespace in the patch template, I wonder if we can put
> the author name and status in the same line as the patch name.
> Separated from the patch name -- perhaps aligned right, if possible.
>
> Maybe something like
>
> Foobar the Bozos (Status Author
>

I've changed Template:patch as you suggest, using float: right to
shift the author over to the right hand side. Feedback welcome.

Cheers,
BJ
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.7 (GNU/Linux)
Comment: http://getfiregpg.org

iD8DBQFIA8lg5YBsbHkuyV0RAtfqAJ9I+tuemfMgwKDqzXFoJr0EptVS6QCfa6jw
pSfyiOfBxrSTgb6GU5oRWO4=
=yabv
-----END PGP SIGNATURE-----

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Tom Lane wrote:
> >> No, changing that wasn't what I meant to suggest. My point here is that
> >> we'd dropped a number of big mushy discussions into the queue with the
> >> idea that they'd be re-opened during commit fest, but new discussion
> >> did not happen to any useful extent. Conclusion: don't bother putting
> >> anything less concrete than a patch on the fest queue.
>
> > So when/how do those discussions get resolved?
>
> [ shrug... ] You can't force ideas to happen on a schedule.
>
> If you don't want an issue to get forgotten, then make a TODO entry for
> it. But the purpose of commit fest is to make sure we deal with things
> that can be dealt with in a timely fashion. It's not going to cause
> solutions to unsolved problems to appear from nowhere.

I need is to know if they are ideas worthy of TODO.

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

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Tue, Apr 15, 2008 at 2:45 AM, Alvaro Herrera wrote:
> Lastly, I would say that pushing submitters to enter their sent patches
> into the Wiki worked -- we need to ensure that they keep doing it.

I'd also suggest, if you want to get serious about encouraging
submitters to add their patches to the wiki:

* A Big Fat Link to the current commitfest page on the main page of the wiki.

* Something in the Developers' FAQ section about submitting patches
with a link to the wiki and some brief instructions about how to
register an account and add a patch to the queue.

Cheers,
BJ
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.7 (GNU/Linux)
Comment: http://getfiregpg.org

iD8DBQFIA9AN5YBsbHkuyV0RAm1TAKDK8Joz4zwhUx35fztOcwuNgKC6HACggj4l
nStLQbEg+IYvR0CYar6md9Q=
=1yGv
-----END PGP SIGNATURE-----

On Mon, Apr 14, 2008 at 6:45 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
[...]
> As far as the Wiki page is concerned, it would be good to make sure the
> entries have a bit more info than just a header line -- things such as
> "author", who reviewed and what did the reviewer say about it.
>
> Some of it is already there.
>
> Something else we learned is that the archives are central (well, we
> already knew that, but I don't think we had ever given them so broad
> use), and we've been making changes to them so that they are more useful
> to reviewers. Further changes are still needed on them, of course, to
> address the remaining problems.
>
> Lastly, I would say that pushing submitters to enter their sent patches
> into the Wiki worked -- we need to ensure that they keep doing it.

I think this should be explained nicely in developer FAQ. The whole
process preferably.

As a first time contributor ;) I must say I was (and still am, a bit)
confused about the process. The FAQ point 1.4 says to discuss
it on -hakers unless its a trivial patch.

I thought the patch would be trivial, sent it to -patches. Then, later
on I thought that perhaps it should be discussed on the -hackers
nonetheless, so I have written there also:
http://archives.postgresql.org/pgsql-hackers/2008-04/msg00147.php
then the patch got rejected, if I understand correctly.

Now assuming I want to prepare patch for something else, at what
point does Wiki come in? Should I send it to -patches and put it on
wiki? Or perhaps wait for some developer's suggestion "put it on
the wiki"? Should I start discussion on -hackers or is -patches enough?
I know that with time they look trivial -- but at least I felt quite uncertain
about them when sending first patch. .

Don't forget to update developer FAQ as well. :)

Regards,
Dawid

Dawid Kuroczko escribió:

> I thought the patch would be trivial, sent it to -patches. Then, later
> on I thought that perhaps it should be discussed on the -hackers
> nonetheless, so I have written there also:
> http://archives.postgresql.org/pgsql-hackers/2008-04/msg00147.php
> then the patch got rejected, if I understand correctly.

The problem is that the patch was initially trivial, but it turned into
a much larger redesign of command handling. I think that's a great
turnout for a submission.

> Don't forget to update developer FAQ as well. :)

Agreed -- the FAQs and other documents do not cover the processes we're
currently following. Mind you, the processes are quite young. (More
reason to have them better documented I guess.)

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

On Mon, 14 Apr 2008 21:25:28 -0400
Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:

> The problem is that the patch was initially trivial, but it turned
> into a much larger redesign of command handling. I think that's a
> great turnout for a submission.
>
> > Don't forget to update developer FAQ as well. :)
>
> Agreed -- the FAQs and other documents do not cover the processes
> we're currently following. Mind you, the processes are quite young.
> (More reason to have them better documented I guess.)

We can change the FAQ per commit fest as things grow and as each
commit fest starts, send out the policy for the commit fest on
announce.

Joshua D. Drake

--
The PostgreSQL Company since 1997: http://www.commandprompt.com/
PostgreSQL Community Conference: http://www.postgresqlconference.org/
United States PostgreSQL Association: http://www.postgresql.us/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

On Mon, Apr 14, 2008 at 05:15:51PM -0400, Bruce Momjian wrote:
> > > So when/how do those discussions get resolved?
> >
> > [ shrug... ] You can't force ideas to happen on a schedule.
>
> I need is to know if they are ideas worthy of TODO.

One thing I would like is if larger more complex patches where there
really was discussion would get a page in the wiki. For example the FSM
data structure discussion; I read the thread but due to all the
crossing threads I only have a have vague idea of what's actually been
decided. It would be helpful if there was a wiki page that described
the solution and why it is better than the others suggested.

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Please line up in a tree and maintain the heap invariant while
> boarding. Thank you for flying nlogn airlines.

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> One problem with this fest was that a whole lot of the patches *weren't*
> trivial; if they had been they'd not have gotten put off till 8.4. So
> there weren't that many that I wanted to let go in without looking at
> them. I guess that's just another way in which the 8.3 schedule problem
> came home to roost during this fest.

That's an issue I ran into when looking through the patches as well. I
can help review trivial patches but I don't generally have the
bandwidth to review alot of the pretty heavy patches which were in the
March commitfest. It took me a while to get going on the commitfest too
though, in part because I wasn't really confident I knew the right
places to look and the right things to do. I'm still not 100% sure,
honestly. Do we have guidelines up somewhere about reviewing, specific
to the process rather than about how to submit patches?

> In future fests we should have a much higher proportion of "little
> things" that maybe more people would feel comfortable taking
> responsibility for.

I'm certainly happy to look at little things and review those and at
least add my own comments. I think that was helpful for the patches I
did comment on in the past, though if not please let me know.

> Perhaps it would be useful to try to rate pending patches by difficulty?

I think alot of times this can be determined pretty quickly, and I'd
hate to have a situation where patch submitters are complaining about
"you rated my patch harder than his and that's not fair" kind of things.

Thanks,

Stephen

Martijn van Oosterhout napsal(a):
> On Mon, Apr 14, 2008 at 05:15:51PM -0400, Bruce Momjian wrote:
>>>> So when/how do those discussions get resolved?
>>> [ shrug... ] You can't force ideas to happen on a schedule.
>> I need is to know if they are ideas worthy of TODO.
>
> One thing I would like is if larger more complex patches where there
> really was discussion would get a page in the wiki. For example the FSM
> data structure discussion; I read the thread but due to all the
> crossing threads I only have a have vague idea of what's actually been
> decided. It would be helpful if there was a wiki page that described
> the solution and why it is better than the others suggested.

+1
Completely agree. Each huge page should have actual proposal/design on wiki. It
helps to improve speed of review and also it will be good knowledge base about
internals.

Zdenek

"Stephen Frost" <sfrost(at)snowman(dot)net> writes:

> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> One problem with this fest was that a whole lot of the patches *weren't*
>> trivial; if they had been they'd not have gotten put off till 8.4. So
>> there weren't that many that I wanted to let go in without looking at
>> them. I guess that's just another way in which the 8.3 schedule problem
>> came home to roost during this fest.
>
> That's an issue I ran into when looking through the patches as well. I
> can help review trivial patches but I don't generally have the
> bandwidth to review alot of the pretty heavy patches which were in the
> March commitfest. It took me a while to get going on the commitfest too
> though, in part because I wasn't really confident I knew the right
> places to look and the right things to do. I'm still not 100% sure,
> honestly. Do we have guidelines up somewhere about reviewing, specific
> to the process rather than about how to submit patches?

I don't think we know what a "typical review" really looks like. But basically
what worked for me in the (relatively trivial) patches I've looked at was
considering "if this was mine, what would I consider doing next?" I made a
bullet point list of things I would consider changing or think are missing.

One thing I found is that I'm not sure what to do if I don't find any changes
to propose. I tend to assume that means I just don't understand the code well
enough and end up just not posting anything.

>> Perhaps it would be useful to try to rate pending patches by difficulty?
>
> I think alot of times this can be determined pretty quickly, and I'd
> hate to have a situation where patch submitters are complaining about
> "you rated my patch harder than his and that's not fair" kind of things.

One thing which is conventional on linux-kernel is to include the output of
diffstat when you post the patch. That helps people to get an idea of whether
their favourite area of the code is being whacked around and it warrants a
look. It also helps get a quick overview of what to expect as you start to
read the patch proper.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's 24x7 Postgres support!

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> One thing I found is that I'm not sure what to do if I don't find any changes
> to propose. I tend to assume that means I just don't understand the code well
> enough and end up just not posting anything.

It's still worth adding an annotation to the wiki that you looked it
over and couldn't find anything wrong. A couple such annotations would
give the eventual committer a warm fuzzy feeling that he didn't need to
look at the patch too hard.

regards, tom lane

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> I don't think we know what a "typical review" really looks like.

A general comment is that in stuff I review, I frequently spend a lot of
time trying to make the patch "look like it belongs", that is make it
reasonably well-integrated with the surrounding code. This is important
because a code base that too obviously consists of layers upon layers
of independent patches soon ceases to be readable or maintainable.
Ideally, once your patch has gone in, someone looking at the code for
the first time wouldn't even suspect it hadn't always been there.

Typical sins in this area include not following the coding style or
commenting style of immediately adjacent code, failing to update
comments that your patch has rendered inaccurate, intentionally making
your edits "stand out", etc. While pg_indent will fix some of the
simpler transgressions, it's not very good with comment style, and
it certainly can't fix failures of omission. (I dislike committing code
that is far away from pg_indent style anyway, since even if it will get
fixed later, I'm still gonna have to look at it until then.)

Sometimes patch authors seem to prefer shorter patches over better
integrated ones --- this particularly happens when the "most natural"
way of adding something would require refactoring existing code.
I understand the reasons for preferring a smaller patch, but you
really need to take the long view about what the code will look like
later.

I guess this is coming off as more advice to patch authors than
reviewers, but it is definitely a big deal in my eyes --- I typically
spend about as much time on issues of this sort as on functional
correctness. And it is something that reviewers can fix if they
care to.

regards, tom lane

Tom Lane wrote:

> A general comment is that in stuff I review, I frequently spend a lot of
> time trying to make the patch "look like it belongs", that is make it
> reasonably well-integrated with the surrounding code. This is important
> because a code base that too obviously consists of layers upon layers
> of independent patches soon ceases to be readable or maintainable.

I did waste some time in the past complaining to submitters when the
style was off. At some point I stopped because I got the impression
that that style of comment was not useful: people seem to get the idea
that it's OK if the code does not follow our style; pgindent would fix
it later after all.

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

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Tom Lane wrote:
>> A general comment is that in stuff I review, I frequently spend a lot of
>> time trying to make the patch "look like it belongs", that is make it
>> reasonably well-integrated with the surrounding code. This is important
>> because a code base that too obviously consists of layers upon layers
>> of independent patches soon ceases to be readable or maintainable.

> I did waste some time in the past complaining to submitters when the
> style was off. At some point I stopped because I got the impression
> that that style of comment was not useful: people seem to get the idea
> that it's OK if the code does not follow our style; pgindent would fix
> it later after all.

pg_indent can fix some things, but a lot of what I'm thinking about here
is far beyond its ken. You also have to be aware that it can mangle
comments to the point of unreadability, if the comment style is not
what it expects --- even relatively simple things like using two stars
instead of one for block comment leader can look pretty ugly after
pg_indent.

I tend to just fix this stuff while committing, rather than lecture the
submitters about it, but it undoubtedly is a time sink.

regards, tom lane

Tom Lane wrote:

> I tend to just fix this stuff while committing, rather than lecture the
> submitters about it, but it undoubtedly is a time sink.

Lesson learned: a useful task for another reviewer to do is to grab the
patch, fix the style issues, and post the fixed version. That way, the
"higher level reviewer" does not have to waste time on a task that,
really, anybody can do.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

On Tue, 15 Apr 2008 12:12:24 -0400
Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:

> Tom Lane wrote:
>
> > I tend to just fix this stuff while committing, rather than lecture
> > the submitters about it, but it undoubtedly is a time sink.
>
> Lesson learned: a useful task for another reviewer to do is to grab
> the patch, fix the style issues, and post the fixed version. That
> way, the "higher level reviewer" does not have to waste time on a
> task that, really, anybody can do.

This reminds me of parenting. As a parent you have a tendency to do
things for your children, long past the time you should. Maybe it is
that you cut their food so they don't have to use a knife or that you
wash their face with their sleeve as they walk out the door. At some
point you have to let go otherwise the child will never learn to take
care of themselves and will end up living in your basement, unemployed
and yelling up the stairs for bagel bytes.

The idea that we "fix" stylistic issues on the fly is not sustainable.
We should offer help and mentorship to new patch submitters in all
areas (including stylistic) but they should do the work. It is the only
way we will mold them to submit patches in the proper way.

Sincerely,

Joshua D. Drake

--
The PostgreSQL Company since 1997: http://www.commandprompt.com/
PostgreSQL Community Conference: http://www.postgresqlconference.org/
United States PostgreSQL Association: http://www.postgresql.us/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Wed, Apr 16, 2008 at 2:17 AM, Joshua D. Drake wrote:
> On Tue, 15 Apr 2008 12:12:24 -0400
> Alvaro Herrera wrote:
>
> > Tom Lane wrote:
> >
> > > I tend to just fix this stuff while committing, rather than lecture
> > > the submitters about it, but it undoubtedly is a time sink.
> >
> > Lesson learned: a useful task for another reviewer to do is to grab
> > the patch, fix the style issues, and post the fixed version. That
> > way, the "higher level reviewer" does not have to waste time on a
> > task that, really, anybody can do.
>
> The idea that we "fix" stylistic issues on the fly is not sustainable.
> We should offer help and mentorship to new patch submitters in all
> areas (including stylistic) but they should do the work. It is the only
> way we will mold them to submit patches in the proper way.
>

I agree. As a submitter I would much rather get an email saying e.g.
"Hey, your patch is nice but the code style sticks out like a sore
thumb. Please adopt surrounding naming convention and fix your
indentation per the rules at [link]." than have it fixed silently on
its way to being committed.

With the former I learn something and get to improve my own work.
With the latter, my next patch is probably going to have the exact
same problem, which is in the long term just making extra work for the
reviewers.

Cheers,
BJ
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.7 (GNU/Linux)
Comment: http://getfiregpg.org

iD8DBQFIBZZ95YBsbHkuyV0RAjviAJ9pBG6I3DAySIx6ARp2cLnYe+bAdACg/VAR
Xgvpg4iyWsbNk4QKGI//diw=
=yDoO
-----END PGP SIGNATURE-----

On Apr 14, 2008, at 4:15 PM, Bruce Momjian wrote:
>> If you don't want an issue to get forgotten, then make a TODO
>> entry for
>> it. But the purpose of commit fest is to make sure we deal with
>> things
>> that can be dealt with in a timely fashion. It's not going to cause
>> solutions to unsolved problems to appear from nowhere.
>
> I need is to know if they are ideas worthy of TODO.

This is something I think we could really improve upon... For one, I
think the impression (right or wrong) is that Bruce is the person
that gets say on what goes on the TODO. We also don't have any good
mechanism for general users to provide feedback on what things are
important to them. Of course it's ultimately about a developer
scratching an itch, but I think it would be useful to have some idea
of how popular TODO items were to help guide development efforts, as
well as possibly target items that should just be removed.
--
Decibel!, aka Jim C. Nasby, Database Architect decibel(at)decibel(dot)org
Give your computer some brain candy! www.distributed.net Team #1828

Hi,

> The idea that we "fix" stylistic issues on the fly is not sustainable.
> > We should offer help and mentorship to new patch submitters in all
> > areas (including stylistic) but they should do the work. It is the only
> > way we will mold them to submit patches in the proper way.
> >
>
> I agree. As a submitter I would much rather get an email saying e.g.
> "Hey, your patch is nice but the code style sticks out like a sore
> thumb. Please adopt surrounding naming convention and fix your
> indentation per the rules at [link]." than have it fixed silently on
> its way to being committed.
>
> With the former I learn something and get to improve my own work.
> With the latter, my next patch is probably going to have the exact
> same problem, which is in the long term just making extra work for the
> reviewers.
>

I think, us patch-submitters should be asked to do a run of pg_indent on the
files that we have modified. That should take care of atleast the
indentation related issues. I looked at the README of src/tools/pgindent,
and it should be easy to run enough (or is it not?). Only one thing that
caught my eye was:

1) Build the source tree with _debug_ symbols and all possible configure
options

Can the above point be elaborated further? What all typical and possible
configure options should be used to get a clean and complete pg_indent run?

And I think adopting surrounding naming, commeting, coding conventions
should come naturally as it can aide in copy-pasting too :)

Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com

NikhilS wrote:
> Hi,
>
> > The idea that we "fix" stylistic issues on the fly is not
> > sustainable.
> > > We should offer help and mentorship to new patch submitters in
> > > all areas (including stylistic) but they should do the work. It
> > > is the only way we will mold them to submit patches in the proper
> > > way.
> > >
> >
> > I agree. As a submitter I would much rather get an email saying
> > e.g. "Hey, your patch is nice but the code style sticks out like a
> > sore thumb. Please adopt surrounding naming convention and fix your
> > indentation per the rules at [link]." than have it fixed silently on
> > its way to being committed.
> >
> > With the former I learn something and get to improve my own work.
> > With the latter, my next patch is probably going to have the exact
> > same problem, which is in the long term just making extra work for
> > the reviewers.
> >
>
> I think, us patch-submitters should be asked to do a run of pg_indent
> on the files that we have modified. That should take care of atleast
> the indentation related issues. I looked at the README of
> src/tools/pgindent, and it should be easy to run enough (or is it
> not?). Only one thing that caught my eye was:
>
> 1) Build the source tree with _debug_ symbols and all possible
> configure options
>
> Can the above point be elaborated further? What all typical and
> possible configure options should be used to get a clean and complete
> pg_indent run?
>
> And I think adopting surrounding naming, commeting, coding conventions
> should come naturally as it can aide in copy-pasting too :)

I think pg_indent has to be made a lot more portable and easy to use
before that can happen :-) I've run it once or twice on linux machines,
and it comes out with huge changes compared to what Bruce gets on his
machine. Other times, it doesn't :-) So yeah, it could be that it just
needs to be made easier to use, because I may certainly have done
something wrong.

//Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> I think pg_indent has to be made a lot more portable and easy to use
> before that can happen :-) I've run it once or twice on linux machines,
> and it comes out with huge changes compared to what Bruce gets on his
> machine.

Yeah, I've had no luck with it either.

Every so often there are discussions of going over to GNU indent
instead. Presumably that would solve the portability problem.
The last time we tried it (which was a long time ago) it seemed
to have too many bugs and idiosyncrasies of its own, but it would
be worth a fresh round of experimenting IMHO.

regards, tom lane

On Wed, Apr 16, 2008 at 1:07 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:

> I think pg_indent has to be made a lot more portable and easy to use
> before that can happen :-) I've run it once or twice on linux machines,
> and it comes out with huge changes compared to what Bruce gets on his
> machine. Other times, it doesn't :-) So yeah, it could be that it just
> needs to be made easier to use, because I may certainly have done
> something wrong.

Yeah, I recall trying it a while ago and not having happy memories. It
should be possible (and indeed, mandatory) for patch submitters to run
it over their code before submitting it - having core guys whose time
is so valuable dealing with indentation etc seems an incredible waste.
It won't fix everything, as Tom points out, but it's a better starting
point.

Cheers

Tom

tgl(at)sss(dot)pgh(dot)pa(dot)us (Tom Lane) writes:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> I think pg_indent has to be made a lot more portable and easy to use
>> before that can happen :-) I've run it once or twice on linux machines,
>> and it comes out with huge changes compared to what Bruce gets on his
>> machine.
>
> Yeah, I've had no luck with it either.
>
> Every so often there are discussions of going over to GNU indent
> instead. Presumably that would solve the portability problem.
> The last time we tried it (which was a long time ago) it seemed
> to have too many bugs and idiosyncrasies of its own, but it would
> be worth a fresh round of experimenting IMHO.

Well, GNU indent is now on version 2.2.9, and has evidently addressed
*some* problems with it.

Unfortunately, the pgindent README does not actually specify what any
of the actual problems with GNU indent are, thus making it pretty much
impossible to evaluate whether or not any of the subsequent releases
might have addressed any of those problems.

I doubt that the pgindent issues have been addressed.
--
output = reverse("ofni.sesabatadxunil" "@" "enworbbc")
http://linuxfinances.info/info/sgml.html
"In elementary school, in case of fire you have to line up quietly in
a single file line from smallest to tallest. What is the logic? Do
tall people burn slower?" -- Warren Hutcherson

Brendan Jurd wrote:
> > The idea that we "fix" stylistic issues on the fly is not sustainable.
> > We should offer help and mentorship to new patch submitters in all
> > areas (including stylistic) but they should do the work. It is the only
> > way we will mold them to submit patches in the proper way.
> >
>
> I agree. As a submitter I would much rather get an email saying e.g.
> "Hey, your patch is nice but the code style sticks out like a sore
> thumb. Please adopt surrounding naming convention and fix your
> indentation per the rules at [link]." than have it fixed silently on
> its way to being committed.
>
> With the former I learn something and get to improve my own work.
> With the latter, my next patch is probably going to have the exact
> same problem, which is in the long term just making extra work for the
> reviewers.

If the patch submitters hasn't read the developers' FAQ, we assume they
are not interested in improving the style of their patch.

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

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

Magnus Hagander wrote:
> > And I think adopting surrounding naming, commeting, coding conventions
> > should come naturally as it can aide in copy-pasting too :)
>
> I think pg_indent has to be made a lot more portable and easy to use
> before that can happen :-) I've run it once or twice on linux machines,
> and it comes out with huge changes compared to what Bruce gets on his
> machine. Other times, it doesn't :-) So yeah, it could be that it just
> needs to be made easier to use, because I may certainly have done
> something wrong.

Agreed, pgindent is too cumbersome to require patch submitters to use.
One idea would be to allow C files to be emailed and the indented
version automatically returned via email.

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

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

bruce(at)momjian(dot)us (Bruce Momjian) writes:
> Magnus Hagander wrote:
>> > And I think adopting surrounding naming, commeting, coding conventions
>> > should come naturally as it can aide in copy-pasting too :)
>>
>> I think pg_indent has to be made a lot more portable and easy to use
>> before that can happen :-) I've run it once or twice on linux machines,
>> and it comes out with huge changes compared to what Bruce gets on his
>> machine. Other times, it doesn't :-) So yeah, it could be that it just
>> needs to be made easier to use, because I may certainly have done
>> something wrong.
>
> Agreed, pgindent is too cumbersome to require patch submitters to use.
> One idea would be to allow C files to be emailed and the indented
> version automatically returned via email.

Would it be a terrible idea to...

- Draw the indent code from NetBSD into src/tools/pgindent
- Build it _in place_ inside the code tree (e.g. - don't assume
it will get installed in /usr/local/bin)
- Thus have the ability to run it in place?
--
let name="cbbrowne" and tld="linuxfinances.info" in name ^ "@" ^ tld;;
http://cbbrowne.com/info/lisp.html
"It worked about as well as sticking a blender in the middle of a lime
plantation and hoping they'll make margaritas out of themselves."
-- Frederick J. Polsky v1.0

Chris Browne <cbbrowne(at)acm(dot)org> writes:
> tgl(at)sss(dot)pgh(dot)pa(dot)us (Tom Lane) writes:
>> Every so often there are discussions of going over to GNU indent
>> instead. Presumably that would solve the portability problem.
>> The last time we tried it (which was a long time ago) it seemed
>> to have too many bugs and idiosyncrasies of its own, but it would
>> be worth a fresh round of experimenting IMHO.

> Well, GNU indent is now on version 2.2.9, and has evidently addressed
> *some* problems with it.

> Unfortunately, the pgindent README does not actually specify what any
> of the actual problems with GNU indent are, thus making it pretty much
> impossible to evaluate whether or not any of the subsequent releases
> might have addressed any of those problems.

This wouldn't be hard for someone to investigate. Check out our CVS
from immediately after the last pgindent run. Run GNU indent on it.
See what options result in the smallest diff, and what the diff looks
like in terms of making the code look nicer or less nice.

regards, tom lane

On Wed, 16 Apr 2008 12:36:50 -0400 (EDT)
Bruce Momjian <bruce(at)momjian(dot)us> wrote:

>
> If the patch submitters hasn't read the developers' FAQ, we assume
> they are not interested in improving the style of their patch.

I think that point is fairly flawed in consideration. I know for a fact
that I have had to repeat even to long time contributors source
references for information even though they new about it previously.
Yourself included.

People get in a hurry, they should be reminded with reference.

JD, thanks for the patch but you missed foo.. please check 1.5 of the
Developers FAQ.

Sincerely,

Joshua D. Drake

--
The PostgreSQL Company since 1997: http://www.commandprompt.com/
PostgreSQL Community Conference: http://www.postgresqlconference.org/
United States PostgreSQL Association: http://www.postgresql.us/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

Chris Browne wrote:
> tgl(at)sss(dot)pgh(dot)pa(dot)us (Tom Lane) writes:
> > Magnus Hagander <magnus(at)hagander(dot)net> writes:
> >> I think pg_indent has to be made a lot more portable and easy to use
> >> before that can happen :-) I've run it once or twice on linux machines,
> >> and it comes out with huge changes compared to what Bruce gets on his
> >> machine.
> >
> > Yeah, I've had no luck with it either.
> >
> > Every so often there are discussions of going over to GNU indent
> > instead. Presumably that would solve the portability problem.
> > The last time we tried it (which was a long time ago) it seemed
> > to have too many bugs and idiosyncrasies of its own, but it would
> > be worth a fresh round of experimenting IMHO.
>
> Well, GNU indent is now on version 2.2.9, and has evidently addressed
> *some* problems with it.
>
> Unfortunately, the pgindent README does not actually specify what any
> of the actual problems with GNU indent are, thus making it pretty much
> impossible to evaluate whether or not any of the subsequent releases
> might have addressed any of those problems.
>
> I doubt that the pgindent issues have been addressed.

What I did was to run pgindent on the source tree, make a copy, then run
GNU indent on it and diff -r. I can try that test again in the next few
months.

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

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

Chris Browne wrote:
> bruce(at)momjian(dot)us (Bruce Momjian) writes:
> > Magnus Hagander wrote:
> >> > And I think adopting surrounding naming, commeting, coding conventions
> >> > should come naturally as it can aide in copy-pasting too :)
> >>
> >> I think pg_indent has to be made a lot more portable and easy to use
> >> before that can happen :-) I've run it once or twice on linux machines,
> >> and it comes out with huge changes compared to what Bruce gets on his
> >> machine. Other times, it doesn't :-) So yeah, it could be that it just
> >> needs to be made easier to use, because I may certainly have done
> >> something wrong.
> >
> > Agreed, pgindent is too cumbersome to require patch submitters to use.
> > One idea would be to allow C files to be emailed and the indented
> > version automatically returned via email.
>
> Would it be a terrible idea to...
>
> - Draw the indent code from NetBSD into src/tools/pgindent
> - Build it _in place_ inside the code tree (e.g. - don't assume
> it will get installed in /usr/local/bin)
> - Thus have the ability to run it in place?

Yes, but it bloats our code and people still need to generate the
typedefs and follow the instructions. The other problem is if they run
it on a file they have modified, it is going to adjust places they
didn't touch, thereby making the patch harder to review.

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

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

Bruce Momjian wrote:
> Chris Browne wrote:

> > Would it be a terrible idea to...
> >
> > - Draw the indent code from NetBSD into src/tools/pgindent
> > - Build it _in place_ inside the code tree (e.g. - don't assume
> > it will get installed in /usr/local/bin)
> > - Thus have the ability to run it in place?
>
> Yes, but it bloats our code and people still need to generate the
> typedefs and follow the instructions.

I suggested to you awhile back that we could keep a typedef file on the
repository, saving one step.

I don't see the problem with keeping the BSD indent for now, until we
figure out the set of options GNU indent needs to behave properly. It's
not all that much code, after all.

> The other problem is if they run it on a file they have modified, it
> is going to adjust places they didn't touch, thereby making the patch
> harder to review.

That should be rare if pgindent is run frequently, I think. (And
anyway, there are tools to remove unwanted hunks from patches if the
author so desires.)

I guess the point here is that pgindent is a tool that should _help_
developers. Making it harder to run is not helping anyone.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

bruce(at)momjian(dot)us (Bruce Momjian) writes:
> Chris Browne wrote:
>> bruce(at)momjian(dot)us (Bruce Momjian) writes:
>> > Magnus Hagander wrote:
>> >> > And I think adopting surrounding naming, commeting, coding conventions
>> >> > should come naturally as it can aide in copy-pasting too :)
>> >>
>> >> I think pg_indent has to be made a lot more portable and easy to use
>> >> before that can happen :-) I've run it once or twice on linux machines,
>> >> and it comes out with huge changes compared to what Bruce gets on his
>> >> machine. Other times, it doesn't :-) So yeah, it could be that it just
>> >> needs to be made easier to use, because I may certainly have done
>> >> something wrong.
>> >
>> > Agreed, pgindent is too cumbersome to require patch submitters to use.
>> > One idea would be to allow C files to be emailed and the indented
>> > version automatically returned via email.
>>
>> Would it be a terrible idea to...
>>
>> - Draw the indent code from NetBSD into src/tools/pgindent
>> - Build it _in place_ inside the code tree (e.g. - don't assume
>> it will get installed in /usr/local/bin)
>> - Thus have the ability to run it in place?
>
> Yes, but it bloats our code and people still need to generate the
> typedefs and follow the instructions. The other problem is if they run
> it on a file they have modified, it is going to adjust places they
> didn't touch, thereby making the patch harder to review.

The bloat is 154K, on a project with something around 260MB of code.
I don't think this is a particlarly material degree of bloat.

If it is included in src/tools/pgindent, you can add in a Makefile
such that it is automatically built, so the cost of running it goes
way down, so that it could be run all the time rather than once in a
great long while.

If it was being run *all* the time, would we not expect to find that
we would be seeing relatively smaller sets of changes coming out of
it?

We are presently at the extreme position where pgindent is run once in
a very long time (~ once a year), at pretty considerable cost, and
with the associated cost that a whole lot of indentation problems are
managed by hand.

If we ran pgindent really frequently, there would admittedly be the
difference that there would be a lot of little cases of
changes-from-pgindent being committed along the way, but [1] might it
not be cheaper to accept that cost, with the concomittant benefit that
you could tell patchers "Hey, run pgindent before submitting that
patch, and that'll clean up a number of of the issues." Yes, it
doesn't address code changes like typedef generation, but that never
was an argument against running pgindent...

[1] In cases where the differences primarily fall from differences in
indentation, "cvs diff -u" can drop out those differences when reading
the effects of a patch...
--
let name="cbbrowne" and tld="cbbrowne.com" in name ^ "@" ^ tld;;
http://linuxdatabases.info/info/sap.html
"A study in the Washington Post says that women have better verbal
skills than men. I just want to say to the authors of that study:
Duh." -- Conan O' Brien

Chris Browne wrote:
> >> Would it be a terrible idea to...
> >>
> >> - Draw the indent code from NetBSD into src/tools/pgindent
> >> - Build it _in place_ inside the code tree (e.g. - don't assume
> >> it will get installed in /usr/local/bin)
> >> - Thus have the ability to run it in place?
> >
> > Yes, but it bloats our code and people still need to generate the
> > typedefs and follow the instructions. The other problem is if they run
> > it on a file they have modified, it is going to adjust places they
> > didn't touch, thereby making the patch harder to review.
>
> The bloat is 154K, on a project with something around 260MB of code.
> I don't think this is a particlarly material degree of bloat.

You mean 37kb vs 13MB, right? That is the tarball sizes I see.

> If we ran pgindent really frequently, there would admittedly be the
> difference that there would be a lot of little cases of
> changes-from-pgindent being committed along the way, but [1] might it
> not be cheaper to accept that cost, with the concomittant benefit that
> you could tell patchers "Hey, run pgindent before submitting that
> patch, and that'll clean up a number of of the issues." Yes, it
> doesn't address code changes like typedef generation, but that never
> was an argument against running pgindent...

That is much a more radical use of pgindent than it has had in the past
but it is certainly possible.

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

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

Chris Browne <cbbrowne(at)acm(dot)org> writes:
> Would it be a terrible idea to...
>
> - Draw the indent code from NetBSD into src/tools/pgindent

I am not real eager to become maintainers of our own indent fork, which
is what you propose. (Just for starters, what will we have to do to
make it run on non-BSD systems?)

> We are presently at the extreme position where pgindent is run once in
> a very long time (~ once a year), at pretty considerable cost, and
> with the associated cost that a whole lot of indentation problems are
> managed by hand.

Yeah. One reason for that is that the typedef problem makes it a pretty
manual process.

The main problem I see with "pgindent early and often" is that it only
works well if everyone is using exactly the same pgindent code (and
exactly the same typedef list). Otherwise you just get buried in
useless whitespace diffs.

It's bad enough that Bruce whacks around his copy from time to time :-(.
I would say that the single greatest annoyance for maintaining our back
branches is that patches tend to not back-patch cleanly, and well over
half the time it's because of random reformattings done by pgindent
to code that hadn't changed at all, but it had formatted differently
the prior year.

For the same reason, my take on your "random whitespace changes are
acceptable" theory is not no but hell no. It's gonna cost us,
permanently, in manual patch adjustments if we allow the repository to
get cluttered with content-free diffs.

I guess an advantage of maintaining our own fork is that there'd be Only
One True pgindent, thereby alleviating that problem; but I'm still not
eager to go there. If we were going to do it, I'd really wish that we
could standardize on a version that didn't need a typedef list. The
random whitespace changes you get after changing the typedef list are
another thorn in my side.

But in any case, this is all focusing on trivialities. The stuff
pgindent can fix is, by definition, stuff that committers don't really
have to worry about during patch review. The stuff I'm worried about
is at a higher level than that.

regards, tom lane

Tom Lane wrote:
> The main problem I see with "pgindent early and often" is that it only
> works well if everyone is using exactly the same pgindent code (and
> exactly the same typedef list). Otherwise you just get buried in
> useless whitespace diffs.
>
> It's bad enough that Bruce whacks around his copy from time to time :-(.
> I would say that the single greatest annoyance for maintaining our back
> branches is that patches tend to not back-patch cleanly, and well over
> half the time it's because of random reformatting done by pgindent
> to code that hadn't changed at all, but it had formatted differently
> the prior year.

I hate to say it but pgindent formatting changes are usually made in
cases where you or the community want pgindent to improve its indenting. :-)
So we improve pgindent but pay for it in backpatching difficulties. :-(

> I guess an advantage of maintaining our own fork is that there'd be Only
> One True pgindent, thereby alleviating that problem; but I'm still not
> eager to go there. If we were going to do it, I'd really wish that we
> could standardize on a version that didn't need a typedef list. The

I don't think that is possible. GNU indent 2.2.9 requires the same -T
option:

You must use the -T option to tell indent the name of all the
type names in your program that are defined by typedef. -T can be
specified more than once, and all names specified are used. For
example, if your program contains

typedef unsigned long CODE_ADDR;
typedef enum {red, blue, green} COLOR;

you would use the options -T CODE_ADDR -T COLOR

astyle doesn't seem to require it but perhaps it just mishandles them.
As I remember there isn't anything about the C grammar that can tell
identify a typedef when it needs to.

> But in any case, this is all focusing on trivialities. The stuff
> pgindent can fix is, by definition, stuff that committers don't really
> have to worry about during patch review. The stuff I'm worried about
> is at a higher level than that.

Agreed. Reformatting is small compared to understanding the patch,
adjusting in the comments, testing, documentation, etc.

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

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

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Tom Lane wrote:
>> It's bad enough that Bruce whacks around his copy from time to time :-(.

> I hate to say it but pgindent formatting changes are usually made in
> cases where you or the community want pgindent to improve its indenting. :-)
> So we improve pgindent but pay for it in backpatching difficulties. :-(

Yeah, I know ... it's why I've pretty much stopped complaining about
pgindent, even though there are lots of little things it does that
I don't especially care for.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:

> > I hate to say it but pgindent formatting changes are usually made in
> > cases where you or the community want pgindent to improve its indenting. :-)
> > So we improve pgindent but pay for it in backpatching difficulties. :-(
>
> Yeah, I know ... it's why I've pretty much stopped complaining about
> pgindent, even though there are lots of little things it does that
> I don't especially care for.

Maybe this means that we should give pgindent a run over the back
branches.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Tom Lane wrote:
> > Bruce Momjian <bruce(at)momjian(dot)us> writes:
>
> > > I hate to say it but pgindent formatting changes are usually made in
> > > cases where you or the community want pgindent to improve its indenting. :-)
> > > So we improve pgindent but pay for it in backpatching difficulties. :-(
> >
> > Yeah, I know ... it's why I've pretty much stopped complaining about
> > pgindent, even though there are lots of little things it does that
> > I don't especially care for.
>
> Maybe this means that we should give pgindent a run over the back
> branches.

Yea, that thought has crossed our minds, but the problem is that there
is little testing of back branches so it would be risky.

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

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

Bruce Momjian wrote:
> Alvaro Herrera wrote:

> > Maybe this means that we should give pgindent a run over the back
> > branches.
>
> Yea, that thought has crossed our minds, but the problem is that there
> is little testing of back branches so it would be risky.

That's a fair point, though I wonder how can a code indenter be so
broken that it broke code in ways not detected by our buildfarm.

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

bruce(at)momjian(dot)us (Bruce Momjian) writes:
> Chris Browne wrote:
>> >> Would it be a terrible idea to...
>> >>
>> >> - Draw the indent code from NetBSD into src/tools/pgindent
>> >> - Build it _in place_ inside the code tree (e.g. - don't assume
>> >> it will get installed in /usr/local/bin)
>> >> - Thus have the ability to run it in place?
>> >
>> > Yes, but it bloats our code and people still need to generate the
>> > typedefs and follow the instructions. The other problem is if they run
>> > it on a file they have modified, it is going to adjust places they
>> > didn't touch, thereby making the patch harder to review.
>>
>> The bloat is 154K, on a project with something around 260MB of code.
>> I don't think this is a particlarly material degree of bloat.
>
> You mean 37kb vs 13MB, right? That is the tarball sizes I see.

Hmm. I was apparently badly counting the size of the sources. I ran
a find | wc command that seemed to report 260MB of code. A "du" on a
"make distclean" tree gives me 104MB. At any rate, that's only out by
a bit more than a binary order of magnitude ;-).

I was thinking about the 154K of source code sitting in CVS, not the
(yes, lower) cost of it in a tarball.

Seems immaterial either way...

>> If we ran pgindent really frequently, there would admittedly be the
>> difference that there would be a lot of little cases of
>> changes-from-pgindent being committed along the way, but [1] might it
>> not be cheaper to accept that cost, with the concomittant benefit that
>> you could tell patchers "Hey, run pgindent before submitting that
>> patch, and that'll clean up a number of of the issues." Yes, it
>> doesn't address code changes like typedef generation, but that never
>> was an argument against running pgindent...
>
> That is much a more radical use of pgindent than it has had in the past
> but it is certainly possible.

Well, supposing you're cleaning up a patch after someone has generated
it in bad style, it would seem like rather less work to use pgindent
to impose style policy right away rather than simulating its effects
manually.

I'm not proposing anything *really* radical, like migrating away from
CVS, after all! ;-)
--
let name="cbbrowne" and tld="linuxfinances.info" in String.concat "@" [name;tld];;
http://linuxfinances.info/info/lisp.html
There was a young lady of Crewe
Whose limericks stopped at line two.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Bruce Momjian wrote:
>> Alvaro Herrera wrote:
>>> Maybe this means that we should give pgindent a run over the back
>>> branches.
>>
>> Yea, that thought has crossed our minds, but the problem is that there
>> is little testing of back branches so it would be risky.

> That's a fair point, though I wonder how can a code indenter be so
> broken that it broke code in ways not detected by our buildfarm.

Yeah, the buildfarm has changed that equation a little.

I think that if we were going to do something like switch to GNU indent,
we'd really have to do that (re-indent the back branches) to maintain
our sanity. And again anytime we moved to a new release of indent.

regards, tom lane

Chris Browne wrote:
> > That is much a more radical use of pgindent than it has had in the past
> > but it is certainly possible.
>
> Well, supposing you're cleaning up a patch after someone has generated
> it in bad style, it would seem like rather less work to use pgindent
> to impose style policy right away rather than simulating its effects
> manually.

I am reviewing the psql wrap patch and just used pgindent today to clean
it up. (pgindent did not add any extra spacing changes.) Patch
reviewers should probably be able to run pgindent.

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

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

"Alvaro Herrera" <alvherre(at)commandprompt(dot)com> writes:

> I suggested to you awhile back that we could keep a typedef file on the
> repository, saving one step.

That kind of sucks since it means you get conflicts when you update if you've
run it yourself.

Is there a reason we can't write makefiles which generate this file?

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's RemoteDBA services!

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> I am reviewing the psql wrap patch and just used pgindent today to clean
> it up. (pgindent did not add any extra spacing changes.) Patch
> reviewers should probably be able to run pgindent.

Well, that means nobody in the world can review except you, because
nobody else has ever reported success in duplicating your pgindent
results. I know I haven't been able to.

If you really believe the above then you need to try a bit harder
to find a portable version of indent that we all can use.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > I am reviewing the psql wrap patch and just used pgindent today to clean
> > it up. (pgindent did not add any extra spacing changes.) Patch
> > reviewers should probably be able to run pgindent.
>
> Well, that means nobody in the world can review except you, because
> nobody else has ever reported success in duplicating your pgindent
> results. I know I haven't been able to.
>
> If you really believe the above then you need to try a bit harder
> to find a portable version of indent that we all can use.

The source code of pgindent I use is on our ftp site. find_typedefs
should now work on Linux as well as BSD. Not sure what else would be a
problem.

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

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

Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > > I am reviewing the psql wrap patch and just used pgindent today to clean
> > > it up. (pgindent did not add any extra spacing changes.) Patch
> > > reviewers should probably be able to run pgindent.
> >
> > Well, that means nobody in the world can review except you, because
> > nobody else has ever reported success in duplicating your pgindent
> > results. I know I haven't been able to.
> >
> > If you really believe the above then you need to try a bit harder
> > to find a portable version of indent that we all can use.
>
> The source code of pgindent I use is on our ftp site. find_typedefs
> should now work on Linux as well as BSD. Not sure what else would be a
> problem.

Also I can put up a web page where you can upload or email your C file
and get a formatted version back.

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

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

Alvaro Herrera wrote:
> Bruce Momjian wrote:
>> Alvaro Herrera wrote:
>
>>> Maybe this means that we should give pgindent a run over the back
>>> branches.
>> Yea, that thought has crossed our minds, but the problem is that there
>> is little testing of back branches so it would be risky.
>
> That's a fair point, though I wonder how can a code indenter be so
> broken that it broke code in ways not detected by our buildfarm.

Something like this:

if (foo)
{
do something;
do something else;
}
...

->

if (foo)
do something;
do something else;
...

I wouldn't be too surprised if something like that happened with one of
the complex macros, like PG_TRY/CATCH.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Thu, Apr 17, 2008 at 09:11:12AM +0300, Heikki Linnakangas wrote:
> Something like this:
>
> if (foo)
> {
> do something;
> do something else;
> }
> ...
>
> ->
>
> if (foo)
> do something;
> do something else;
> ...

I doubt it, indent doesn't know nearly enough C to be able to anything
other than adjust whitespace. It surely won't remove braces...

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Please line up in a tree and maintain the heap invariant while
> boarding. Thank you for flying nlogn airlines.

Bruce Momjian wrote:
> Bruce Momjian wrote:
> > Tom Lane wrote:
> > > Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > > > I am reviewing the psql wrap patch and just used pgindent today
> > > > to clean it up. (pgindent did not add any extra spacing
> > > > changes.) Patch reviewers should probably be able to run
> > > > pgindent.
> > >
> > > Well, that means nobody in the world can review except you,
> > > because nobody else has ever reported success in duplicating your
> > > pgindent results. I know I haven't been able to.
> > >
> > > If you really believe the above then you need to try a bit harder
> > > to find a portable version of indent that we all can use.
> >
> > The source code of pgindent I use is on our ftp site. find_typedefs
> > should now work on Linux as well as BSD. Not sure what else would
> > be a problem.
>
> Also I can put up a web page where you can upload or email your C file
> and get a formatted version back.

IIRC, last time I tried it, the failure was because I couldn't get it
to build the proper typedefs. Any chance you could just put a regularly
updated typedefs file somewhere that I could wget down?

//Magnus

Martijn van Oosterhout wrote:
> I doubt it, indent doesn't know nearly enough C to be able to anything
> other than adjust whitespace. It surely won't remove braces...

I faintly recall that it does or at least did at some point.

Peter Eisentraut wrote:
> Martijn van Oosterhout wrote:
> > I doubt it, indent doesn't know nearly enough C to be able to anything
> > other than adjust whitespace. It surely won't remove braces...
>
> I faintly recall that it does or at least did at some point.

It used to remove braces around single-statement blocks, but that
feature was removed because it broke indentation of PG_TRY blocks.

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

Magnus Hagander wrote:
> Bruce Momjian wrote:
> > Bruce Momjian wrote:
> > > Tom Lane wrote:
> > > > Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > > > > I am reviewing the psql wrap patch and just used pgindent today
> > > > > to clean it up. (pgindent did not add any extra spacing
> > > > > changes.) Patch reviewers should probably be able to run
> > > > > pgindent.
> > > >
> > > > Well, that means nobody in the world can review except you,
> > > > because nobody else has ever reported success in duplicating your
> > > > pgindent results. I know I haven't been able to.
> > > >
> > > > If you really believe the above then you need to try a bit harder
> > > > to find a portable version of indent that we all can use.
> > >
> > > The source code of pgindent I use is on our ftp site. find_typedefs
> > > should now work on Linux as well as BSD. Not sure what else would
> > > be a problem.
> >
> > Also I can put up a web page where you can upload or email your C file
> > and get a formatted version back.
>
> IIRC, last time I tried it, the failure was because I couldn't get it
> to build the proper typedefs. Any chance you could just put a regularly
> updated typedefs file somewhere that I could wget down?

Have you tried the CVS version? It should support typedefs on Linux? I
can put a continually-updated version on my ftp site if people want it.

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

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

Bruce Momjian wrote:
> Magnus Hagander wrote:
> > Bruce Momjian wrote:
> > > Bruce Momjian wrote:
> > > > Tom Lane wrote:
> > > > > Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > > > > > I am reviewing the psql wrap patch and just used pgindent
> > > > > > today to clean it up. (pgindent did not add any extra
> > > > > > spacing changes.) Patch reviewers should probably be able
> > > > > > to run pgindent.
> > > > >
> > > > > Well, that means nobody in the world can review except you,
> > > > > because nobody else has ever reported success in duplicating
> > > > > your pgindent results. I know I haven't been able to.
> > > > >
> > > > > If you really believe the above then you need to try a bit
> > > > > harder to find a portable version of indent that we all can
> > > > > use.
> > > >
> > > > The source code of pgindent I use is on our ftp site.
> > > > find_typedefs should now work on Linux as well as BSD. Not
> > > > sure what else would be a problem.
> > >
> > > Also I can put up a web page where you can upload or email your C
> > > file and get a formatted version back.
> >
> > IIRC, last time I tried it, the failure was because I couldn't get
> > it to build the proper typedefs. Any chance you could just put a
> > regularly updated typedefs file somewhere that I could wget down?
>
> Have you tried the CVS version? It should support typedefs on
> Linux? I can put a continually-updated version on my ftp site if
> people want it.

Right, but it requires me to run configure with all modules enabled,
right? And I don't have all modules installed on all machines, etc etc

(And I didn't get it working properly either way, but that could be
because I simply didn't try hard enough)

//Magnus

Bruce Momjian wrote:
> Magnus Hagander wrote:

> > IIRC, last time I tried it, the failure was because I couldn't get it
> > to build the proper typedefs. Any chance you could just put a regularly
> > updated typedefs file somewhere that I could wget down?
>
> Have you tried the CVS version? It should support typedefs on Linux? I
> can put a continually-updated version on my ftp site if people want it.

A typedef file generated from a single build is no good. We need at
least one for a regular Unix and another one from Windows. The set of
typedefs is different.

If everyone is expected to generate the typedef on their local builds,
then the pgindent output will be different for every developer, thus
generating lots of spurious changes. This is not acceptable nor useful.

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

Magnus Hagander wrote:
> > > > Also I can put up a web page where you can upload or email your C
> > > > file and get a formatted version back.
> > >
> > > IIRC, last time I tried it, the failure was because I couldn't get
> > > it to build the proper typedefs. Any chance you could just put a
> > > regularly updated typedefs file somewhere that I could wget down?
> >
> > Have you tried the CVS version? It should support typedefs on
> > Linux? I can put a continually-updated version on my ftp site if
> > people want it.
>
> Right, but it requires me to run configure with all modules enabled,
> right? And I don't have all modules installed on all machines, etc etc

I don't enable all modules actually because I don't have everything
installed either, but I do my best.

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

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

Alvaro Herrera wrote:
> Bruce Momjian wrote:
> > Magnus Hagander wrote:
>
> > > IIRC, last time I tried it, the failure was because I couldn't get it
> > > to build the proper typedefs. Any chance you could just put a regularly
> > > updated typedefs file somewhere that I could wget down?
> >
> > Have you tried the CVS version? It should support typedefs on Linux? I
> > can put a continually-updated version on my ftp site if people want it.
>
> A typedef file generated from a single build is no good. We need at
> least one for a regular Unix and another one from Windows. The set of
> typedefs is different.
>
> If everyone is expected to generate the typedef on their local builds,
> then the pgindent output will be different for every developer, thus
> generating lots of spurious changes. This is not acceptable nor useful.

The source code is the same for both Unix and Windows but you are right
some typedefs are only visible on windows. I think most are from
EXEC_BACKEND so compiling with/without that should help but then you
have to merge the typedef lists, of course.

I count 2481 typedefs found on my build. I don't think we have to find
every single typedef in the system for pgindent to be useful, but if we
want people to be able to use this we should choose a single typedef
file and all use that. I am willing to create a standard one for
everyone and upload it daily to the community ftp server. It will not
be perfect but I can improve it as people make suggestions.

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

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

Bruce Momjian wrote:

> The source code is the same for both Unix and Windows but you are right
> some typedefs are only visible on windows. I think most are from
> EXEC_BACKEND so compiling with/without that should help but then you
> have to merge the typedef lists, of course.

The source code is the same, of course, but typedef generation uses
object files, not source code.

> I count 2481 typedefs found on my build. I don't think we have to find
> every single typedef in the system for pgindent to be useful, but if we
> want people to be able to use this we should choose a single typedef
> file and all use that. I am willing to create a standard one for
> everyone and upload it daily to the community ftp server. It will not
> be perfect but I can improve it as people make suggestions.

Please do.

What are we going to do about the duality of Windows vs. non-Windows?
Perhaps we could collect typedefs generated on the buildfarm.

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

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> What are we going to do about the duality of Windows vs. non-Windows?
> Perhaps we could collect typedefs generated on the buildfarm.

I think it's really not acceptable that pgindent misformats Windows-only
code (or any other part of the code that Bruce doesn't care enough about
to build on his system).

This whole business of extracting the typedef names from object files
sucks to begin with, and it will never not suck. If we have to have
such a list, we need to find a more platform-independent procedure
for getting it.

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > What are we going to do about the duality of Windows vs. non-Windows?
> > Perhaps we could collect typedefs generated on the buildfarm.
>
> I think it's really not acceptable that pgindent misformats Windows-only
> code (or any other part of the code that Bruce doesn't care enough about
> to build on his system).
>
> This whole business of extracting the typedef names from object files
> sucks to begin with, and it will never not suck. If we have to have
> such a list, we need to find a more platform-independent procedure
> for getting it.

Based on that reaction I am not going to bother uploading my copy of the
typedefs.

"If it can't be perfect, don't bother doing it" seems to be the
approach, and because pgindent will never be perfect, I don't understand
why we bother even running it. Not finding all typedefs is only part of
the imperfect-ness of pgindent.

I think we use pgindent and an imperfect typedef list because it has
proven to be "good enough" to be useful.

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

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

Bruce Momjian wrote:

> Based on that reaction I am not going to bother uploading my copy of the
> typedefs.

Please reconsider. Not having pgindent work at all is not better than
it working "only" 98%.

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

Alvaro Herrera wrote:
> Bruce Momjian wrote:
>
> > Based on that reaction I am not going to bother uploading my copy of the
> > typedefs.
>
> Please reconsider. Not having pgindent work at all is not better than
> it working "only" 98%.

That's what I thought, but Tom thinks my list is unacceptable. What do
others think?

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

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

tgl(at)sss(dot)pgh(dot)pa(dot)us (Tom Lane) writes:
> Chris Browne <cbbrowne(at)acm(dot)org> writes:
>> Would it be a terrible idea to...
>>
>> - Draw the indent code from NetBSD into src/tools/pgindent
>
> I am not real eager to become maintainers of our own indent fork, which
> is what you propose. (Just for starters, what will we have to do to
> make it run on non-BSD systems?)
>
>> We are presently at the extreme position where pgindent is run once in
>> a very long time (~ once a year), at pretty considerable cost, and
>> with the associated cost that a whole lot of indentation problems are
>> managed by hand.
>
> Yeah. One reason for that is that the typedef problem makes it a pretty
> manual process.

As I hear more about the "typedef problem," a part of me gets more and
more appalled... It seems like we're creating some problem for
ourselves in that the typedefs don't seem to be able to be consistent.

I don't have an answer, but it's looking like a sore tooth that
clearly needs attention.

> The main problem I see with "pgindent early and often" is that it only
> works well if everyone is using exactly the same pgindent code (and
> exactly the same typedef list). Otherwise you just get buried in
> useless whitespace diffs.
>
> It's bad enough that Bruce whacks around his copy from time to time :-(.
> I would say that the single greatest annoyance for maintaining our back
> branches is that patches tend to not back-patch cleanly, and well over
> half the time it's because of random reformattings done by pgindent
> to code that hadn't changed at all, but it had formatted differently
> the prior year.
>
> For the same reason, my take on your "random whitespace changes are
> acceptable" theory is not no but hell no. It's gonna cost us,
> permanently, in manual patch adjustments if we allow the repository to
> get cluttered with content-free diffs.

I don't want to be cavalier about it; I'm hoping that in the
discussion, some more stable answer may fall out. Though with the
typedef issues that have emerged, I'm not entirely sanguine...
--
(reverse (concatenate 'string "gro.mca" "@" "enworbbc"))
http://linuxfinances.info/info/internet.html
HEADLINE: Suicidal twin kills sister by mistake!

Alvaro Herrera wrote:
> Bruce Momjian wrote:
>
>
>> Based on that reaction I am not going to bother uploading my copy of the
>> typedefs.
>>
>
> Please reconsider. Not having pgindent work at all is not better than
> it working "only" 98%.
>
>

I have been thinking of pursuing your suggestion of having it as a
buildfarm option. We could provide a SOAP interface to collect the
typedefs and then consolidate them and put them in CVS. We could even do
it per release. That would include Windows, although only MinGW, not
MSVC, which doesn't have objdump.

I could probably get most of that done in a day or so.

Thoughts?

cheers

andrew

"Alvaro Herrera" <alvherre(at)commandprompt(dot)com> writes:

> Bruce Momjian wrote:
>
>> Based on that reaction I am not going to bother uploading my copy of the
>> typedefs.
>
> Please reconsider. Not having pgindent work at all is not better than
> it working "only" 98%.

I think I'm rescinding my objection to checking a canonical list of typedefs
into CVS. I didn't realize how hard it was to generate these typedefs or how
important it was to have everyone using the same version.

Since we really *don't* want individual developers rebuilding the list of
typedefs we don't have to worry about conflicts when the upstream list
changes.

Bruce's list of typedefs seems like a good start point for a "canonical" list
of typedefs. The idea of gathering the lists from the build farm and
consolidating them sounds like a good plan going forward.

The only thing is that if the whole point is to have patch submitters run
pgindent on their own added code it won't work since their own code will be
precisely the code with the missing typedefs. How easy is it to manually add a
handful of typedefs to the list?

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's RemoteDBA services!

Andrew Dunstan wrote:

> I have been thinking of pursuing your suggestion of having it as a
> buildfarm option. We could provide a SOAP interface to collect the
> typedefs and then consolidate them and put them in CVS. We could even do
> it per release. That would include Windows, although only MinGW, not
> MSVC, which doesn't have objdump.

That would rock.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Gregory Stark wrote:

> The only thing is that if the whole point is to have patch submitters run
> pgindent on their own added code it won't work since their own code will be
> precisely the code with the missing typedefs. How easy is it to manually add a
> handful of typedefs to the list?

The list is just a plain text file with one typedef per line, so it
should be trivial.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> I have been thinking of pursuing your suggestion of having it as a
> buildfarm option. We could provide a SOAP interface to collect the
> typedefs and then consolidate them and put them in CVS. We could even do
> it per release. That would include Windows, although only MinGW, not
> MSVC, which doesn't have objdump.

That would certainly be better than the current approach, since
presumably it would cover not only Windows but the other
conditionally-compiled stuff that Bruce chooses not to compile on
his own machine.

Note though that the existing find_typedef script only works on (some
variants of) Linux and BSD. Porting it to a wider set of platforms
might be pretty painful.

I still wish we could build the list directly from the source code,
but I have no suggestions for tools that would do it.

regards, tom lane

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> I have been thinking of pursuing your suggestion of having it as a
>> buildfarm option. We could provide a SOAP interface to collect the
>> typedefs and then consolidate them and put them in CVS. We could even do
>> it per release. That would include Windows, although only MinGW, not
>> MSVC, which doesn't have objdump.
>
> That would certainly be better than the current approach, since
> presumably it would cover not only Windows but the other
> conditionally-compiled stuff that Bruce chooses not to compile on
> his own machine.

It would, as someone said, rock. But it wouldn't really address the ability of
a developer to run pgindent on code he's about to send in, since it wouldn't
have any typedefs that developer just created.

> I still wish we could build the list directly from the source code,
> but I have no suggestions for tools that would do it.

If we wanted to do that I have a few questions:

1) I take it we feel safe guaranteeing that we won't use any fancy macros
inside typedefs. So no '#define pgtype(x) _pg_##x' or anythin like that.

2) How much information do we need about the typedefs? Just their name?

3) How would this work with typedefs which come from system or library
includes?

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Get trained by Bruce Momjian - ask me about EnterpriseDB's PostgreSQL training!

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> That would certainly be better than the current approach, since
>> presumably it would cover not only Windows but the other
>> conditionally-compiled stuff that Bruce chooses not to compile on
>> his own machine.

> It would, as someone said, rock. But it wouldn't really address the ability of
> a developer to run pgindent on code he's about to send in, since it wouldn't
> have any typedefs that developer just created.

Well, that list is just a simple text file listing typedef names,
so it'd hardly be difficult to add your own to the list.

>> I still wish we could build the list directly from the source code,
>> but I have no suggestions for tools that would do it.

> If we wanted to do that I have a few questions:

> 1) I take it we feel safe guaranteeing that we won't use any fancy macros
> inside typedefs. So no '#define pgtype(x) _pg_##x' or anythin like that.

Hmm ... we are fairly crawling with struct tags built that way:

/* Introduces a catalog's structure definition */
#define CATALOG(name,oid) typedef struct CppConcat(FormData_,name)

but offhand I can't think of any actual typedef names built with ##.
Does indent need a preset list of struct tags? Seems that would be
stupid ...

> 2) How much information do we need about the typedefs? Just their name?

Right.

> 3) How would this work with typedefs which come from system or library
> includes?

Ouch, that's a real good point. Maybe a certain amount of platform
dependence is inevitable.

regards, tom lane

* Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> [080417 20:11]:

> > 3) How would this work with typedefs which come from system or library
> > includes?
>
> Ouch, that's a real good point. Maybe a certain amount of platform
> dependence is inevitable.

I don't get it. If they are used as typedefs *anywhere* in the PG
source, they're needed in the typedef list. If they are not used in
the PG source, they aren't needed in the typedef list.

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

Aidan Van Dyk <aidan(at)highrise(dot)ca> writes:
> * Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> [080417 20:11]:
>> Ouch, that's a real good point. Maybe a certain amount of platform
>> dependence is inevitable.

> I don't get it. If they are used as typedefs *anywhere* in the PG
> source, they're needed in the typedef list.

Right, but if the only use is inside #ifdef __BRAND_X_PLATFORM__,
the only way for the proposed toolchain to extract that usage is
if someone runs it on BRAND_X_PLATFORM.

Of course, for seldom-used platforms maybe we don't care that much.

regards, tom lane

* Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> [080417 20:47]:

> Right, but if the only use is inside #ifdef __BRAND_X_PLATFORM__,
> the only way for the proposed toolchain to extract that usage is
> if someone runs it on BRAND_X_PLATFORM.
>
> Of course, for seldom-used platforms maybe we don't care that much.

But if we're talking about putting a "definitive list" of them in the
source, we can build that list in a few iterations, with any method we
want (including using Bruce's list as a starting point, some inspection,
some grep/awk/perl, etc). It's not something that will need to be
re-run on a continual basis and produce a definitive list each and every
run.

a.

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

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Gregory Stark <stark(at)enterprisedb(dot)com> writes:
>> "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>
>> 1) I take it we feel safe guaranteeing that we won't use any fancy macros
>> inside typedefs. So no '#define pgtype(x) _pg_##x' or anythin like that.
>
> Hmm ... we are fairly crawling with struct tags built that way:
>
> /* Introduces a catalog's structure definition */
> #define CATALOG(name,oid) typedef struct CppConcat(FormData_,name)
>
> but offhand I can't think of any actual typedef names built with ##.
> Does indent need a preset list of struct tags? Seems that would be
> stupid ...

It's not just ## that's a problem. Any macro used to build the typedef would
be a problem. There's not a whole lot of other reasons you would want to use
macros in a typedef but...

>> 3) How would this work with typedefs which come from system or library
>> includes?
>
> Ouch, that's a real good point. Maybe a certain amount of platform
> dependence is inevitable.

The reason I was asking these questions was because I was thinking about how
hard it would be to generate the list from a textual analysis instead of using
object files.

Such a tool *cannot* use cpp to preprocess the file because it would defeat
much of the purpose. The point is that we want to find all the typedefs in all
the #ifdef branches.

But if we don't preprocess the files with CPP then macros like the one I
included before wouldn't be interpreted. Nor would we be pulling in system or
library headers, so no typedefs from them.

But if we're just interested in the names I suppose a hybrid approach would
work. 1) The build farm builds a list of typedefs found in all the various
builds and we check that into CVS. 2) a textual tool run as part of your
normal build builds a list of typedefs found in your tree.

But if we're still doing object file analysis on the build farm and it's easy
to add typedefs manually then perhaps there's not much effort saved by having
such a tool. I think it would be possible to write but it wouldn't really be
easy. You have to parse just enough C to find the typedef but not so much you
get confused by invalid C syntax caused by looking at both sides of #ifdef
branches.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's 24x7 Postgres support!

Gregory Stark wrote:
> But if we're still doing object file analysis on the build farm and it's easy
> to add typedefs manually then perhaps there's not much effort saved by having
> such a tool. I think it would be possible to write but it wouldn't really be
> easy. You have to parse just enough C to find the typedef but not so much you
> get confused by invalid C syntax caused by looking at both sides of #ifdef
> branches.
>
>

I am pretty dead sure that a textual analysis tool is going to be far
too much work to write and maintain, for the benefit we might get from it.

cheers

andrew

On Fri, 18 Apr 2008, Gregory Stark wrote:

> The reason I was asking these questions was because I was thinking about
> how hard it would be to generate the list from a textual analysis
> instead of using object files.

Is there some reason I don't understand why the listing doyxgen creates
isn't good enough here? http://doxygen.postgresql.org/globals_type.html

Scraping that HTML seems like it would be pretty straightforward.

--
* Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD

Greg Smith wrote:
> On Fri, 18 Apr 2008, Gregory Stark wrote:
>
>> The reason I was asking these questions was because I was thinking
>> about how hard it would be to generate the list from a textual analysis
>> instead of using object files.
>
> Is there some reason I don't understand why the listing doyxgen creates
> isn't good enough here? http://doxygen.postgresql.org/globals_type.html
>
> Scraping that HTML seems like it would be pretty straightforward.

It's awfully incomplete. Bruce said to me the other day on IM that the
list he was getting with the Linux version of find_typedef was something
like 2800 symbols. I checked the doxygen list and I only see about a
dozen for each letter, so there's a whole lot missing here.

--
Alvaro Herrera http://www.amazon.com/gp/registry/CTMLCN8V17R4
"The Gord often wonders why people threaten never to come back after they've
been told never to return" (www.actsofgord.com)

Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> Greg Smith wrote:
>> Scraping that HTML seems like it would be pretty straightforward.

> It's awfully incomplete. Bruce said to me the other day on IM that the
> list he was getting with the Linux version of find_typedef was something
> like 2800 symbols. I checked the doxygen list and I only see about a
> dozen for each letter, so there's a whole lot missing here.

[ click click... ] A quick grep counts 2154 occurrences of the word
'typedef' in our tree. Some of them are no doubt false hits
(documentation etc), but on the other hand you need to add typedefs
coming from system headers.

doxygen's 200-some is clearly an order of magnitude too low, but I
wonder whether Bruce's list hasn't got some false hits ...

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> > Greg Smith wrote:
> >> Scraping that HTML seems like it would be pretty straightforward.
>
> > It's awfully incomplete. Bruce said to me the other day on IM that the
> > list he was getting with the Linux version of find_typedef was something
> > like 2800 symbols. I checked the doxygen list and I only see about a
> > dozen for each letter, so there's a whole lot missing here.
>
> [ click click... ] A quick grep counts 2154 occurrences of the word
> 'typedef' in our tree. Some of them are no doubt false hits
> (documentation etc), but on the other hand you need to add typedefs
> coming from system headers.
>
> doxygen's 200-some is clearly an order of magnitude too low, but I
> wonder whether Bruce's list hasn't got some false hits ...

My list is at:

http://momjian.us/tmp/pgtypedefs

pgindent is probably 97% optimal. Getting a better typedef list will
change that to perhaps 97.2% optimal. There is a lot of discussion
happening to try to get that 0.2%. :-O

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

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

Tom Lane wrote:

> [ click click... ] A quick grep counts 2154 occurrences of the word
> 'typedef' in our tree. Some of them are no doubt false hits
> (documentation etc), but on the other hand you need to add typedefs
> coming from system headers.
>
> doxygen's 200-some is clearly an order of magnitude too low, but I
> wonder whether Bruce's list hasn't got some false hits ...

You also have to account for typedefs in OpenSSL headers, Kerberos,
readline, etc. Perhaps you can count them as "system headers", but then
it starts to be clear that you can't just process our own source files,
or any system's headers alone.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Bruce Momjian wrote:

> pgindent is probably 97% optimal. Getting a better typedef list will
> change that to perhaps 97.2% optimal. There is a lot of discussion
> happening to try to get that 0.2%. :-O

If I'm allowed to make my own guesses I'd say pgindent is at about 90%
currently and we could get it to 99% or more by leveraging the
buildfarm.

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

Alvaro Herrera wrote:
> Bruce Momjian wrote:
>
> > pgindent is probably 97% optimal. Getting a better typedef list will
> > change that to perhaps 97.2% optimal. There is a lot of discussion
> > happening to try to get that 0.2%. :-O
>
> If I'm allowed to make my own guesses I'd say pgindent is at about 90%
> currently and we could get it to 99% or more by leveraging the
> buildfarm.

My point is that typedefs are only a small part of what makes pgindent
less than perfect. I should have said a "perfect typedef list" would
make it 97.2%.

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

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

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
>
>> Greg Smith wrote:
>>
>>> Scraping that HTML seems like it would be pretty straightforward.
>>>
>
>
>> It's awfully incomplete. Bruce said to me the other day on IM that the
>> list he was getting with the Linux version of find_typedef was something
>> like 2800 symbols. I checked the doxygen list and I only see about a
>> dozen for each letter, so there's a whole lot missing here.
>>
>
> [ click click... ] A quick grep counts 2154 occurrences of the word
> 'typedef' in our tree. Some of them are no doubt false hits
> (documentation etc), but on the other hand you need to add typedefs
> coming from system headers.
>
> doxygen's 200-some is clearly an order of magnitude too low, but I
> wonder whether Bruce's list hasn't got some false hits ...
>
>
>

2800 does seem a bit high. My buildfarm member dungbeetle just found
2482 on a build that is only missing the optional pam, bonjour and
gssapi config options.

Here's the list already loaded to the server:
http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=dungbeetle&dt=2008-04-18%20160103&stg=typedefs

I had to change the logic some - the stuff in the find_typedefs script
seemed to be quite broken on my Linux box (fairly vanilla fc6).

The relevant perl code is below.

I'll see about running this with Windows and Cygwin and I can even try OSX.

cheers

andrew

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

my @goodsyms = sort keys %syms;
map { s/$/\n/; } @goodsyms;

writelog('typedefs',\(at)goodsyms);

}

"Andrew Dunstan" <andrew(at)dunslane(dot)net> writes:

> Tom Lane wrote:
>> doxygen's 200-some is clearly an order of magnitude too low, but I
>> wonder whether Bruce's list hasn't got some false hits ...

Skimming the output it does have things like "int" and "float" but presumably
we would know if that caused any problem, they wouldn't inflate the numbers
much.

> 2800 does seem a bit high. My buildfarm member dungbeetle just found 2482 on a
> build that is only missing the optional pam, bonjour and gssapi config options.

The numbers going to vary heavily from OS to OS so it seems to me that these
are a basically the same order of magnitude.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's PostGIS support!

Gregory Stark wrote:
> "Andrew Dunstan" <andrew(at)dunslane(dot)net> writes:
>
>
>> Tom Lane wrote:
>>
>>> doxygen's 200-some is clearly an order of magnitude too low, but I
>>> wonder whether Bruce's list hasn't got some false hits ...
>>>
>
> Skimming the output it does have things like "int" and "float" but presumably
> we would know if that caused any problem, they wouldn't inflate the numbers
> much.
>
>
>> 2800 does seem a bit high. My buildfarm member dungbeetle just found 2482 on a
>> build that is only missing the optional pam, bonjour and gssapi config options.
>>
>
> The numbers going to vary heavily from OS to OS so it seems to me that these
> are a basically the same order of magnitude.
>

It looks like Windows will blow all our existing numbers out of the
water. Here's a list generated from Cygwin with 6088 symbols. I'm
working on getting a similar list from MinGW.

http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=brown_bat&dt=2008-04-18%20230054&stg=typedefs

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> It looks like Windows will blow all our existing numbers out of the
> water. Here's a list generated from Cygwin with 6088 symbols. I'm
> working on getting a similar list from MinGW.

Hmm, your toolset must be listing all typedefs present in the header
files, not just those actually used?

regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>> It looks like Windows will blow all our existing numbers out of the
>> water. Here's a list generated from Cygwin with 6088 symbols. I'm
>> working on getting a similar list from MinGW.
>>
>
> Hmm, your toolset must be listing all typedefs present in the header
> files, not just those actually used?
>
>
>

No, this is from objdump --stabs, just as find_typedefs does.

cheers

andrew

Andrew Dunstan wrote:
>
>
> Gregory Stark wrote:
>> "Andrew Dunstan" <andrew(at)dunslane(dot)net> writes:
>>
>>
>>> Tom Lane wrote:
>>>
>>>> doxygen's 200-some is clearly an order of magnitude too low, but I
>>>> wonder whether Bruce's list hasn't got some false hits ...
>>>>
>>
>> Skimming the output it does have things like "int" and "float" but
>> presumably
>> we would know if that caused any problem, they wouldn't inflate the
>> numbers
>> much.
>>
>>
>>> 2800 does seem a bit high. My buildfarm member dungbeetle just found
>>> 2482 on a
>>> build that is only missing the optional pam, bonjour and gssapi
>>> config options.
>>>
>>
>> The numbers going to vary heavily from OS to OS so it seems to me
>> that these
>> are a basically the same order of magnitude.
>>
>
> It looks like Windows will blow all our existing numbers out of the
> water. Here's a list generated from Cygwin with 6088 symbols. I'm
> working on getting a similar list from MinGW.
>
> http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=brown_bat&dt=2008-04-18%20230054&stg=typedefs
>

And here are the 7625 from MinGW.

http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=dawn_bat&dt=2008-04-19%20004514&stg=typedefs

It looks like we'll need some sort of extra filter.

cheers

andrew

Andrew Dunstan wrote:

> And here are the 7625 from MinGW.
>
> http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=dawn_bat&dt=2008-04-19%20004514&stg=typedefs
>
> It looks like we'll need some sort of extra filter.

Hmm. Wow. For example I see

FINDREPLACE
FINDREPLACEA
FINDREPLACEW

We use neither ... My guess is that they are used in the system DLLs or
something like that.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Andrew Dunstan wrote:
>> It looks like we'll need some sort of extra filter.

> Hmm. Wow. For example I see

> FINDREPLACE
> FINDREPLACEA
> FINDREPLACEW

> We use neither ... My guess is that they are used in the system DLLs or
> something like that.

Presumably we could grep our own sources for each proposed typedef list
entry --- no hits, you don't get in.

regards, tom lane

Andrew Dunstan wrote:
> > Skimming the output it does have things like "int" and "float" but presumably
> > we would know if that caused any problem, they wouldn't inflate the numbers
> > much.
> >
> >
> >> 2800 does seem a bit high. My buildfarm member dungbeetle just found 2482 on a
> >> build that is only missing the optional pam, bonjour and gssapi config options.
> >>
> >
> > The numbers going to vary heavily from OS to OS so it seems to me that these
> > are a basically the same order of magnitude.
> >
>
> It looks like Windows will blow all our existing numbers out of the
> water. Here's a list generated from Cygwin with 6088 symbols. I'm
> working on getting a similar list from MinGW.
>
> http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=brown_bat&dt=2008-04-18%20230054&stg=typedefs

I have created a proper typedef file that I would normally use for a
pgindent run of the entire tree (it has /contrib, 2628 entries). It is
at:

http://momjian.us/expire/pgtypedefs.bsdos

Andrew, feel free to replace the typedef script in /tools with yours and
we can all test it. I know mine worked on Ubuntu 7.10 and Alvaro got it
working too. We can test your once you are ready.

As soon as you have a stable typedef file we can all use please update
the pgindent README to point to that URL. Keep the instructions of how
to create it in our tree so we have it for future reference.

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

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

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> As soon as you have a stable typedef file we can all use please update
> the pgindent README to point to that URL. Keep the instructions of how
> to create it in our tree so we have it for future reference.

If we're going to go down this path, why would we not put the
"reference" typedef list into CVS?

regards, tom lane

Bruce Momjian wrote:

> I have created a proper typedef file that I would normally use for a
> pgindent run of the entire tree (it has /contrib, 2628 entries). It is
> at:
>
> http://momjian.us/expire/pgtypedefs.bsdos

Well, there are typedefs in there not used anywhere in our code, for
example ASN1_GENERALIZEDTIME ...

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > As soon as you have a stable typedef file we can all use please update
> > the pgindent README to point to that URL. Keep the instructions of how
> > to create it in our tree so we have it for future reference.
>
> If we're going to go down this path, why would we not put the
> "reference" typedef list into CVS?

Uh, I assume we don't want an automated system updating the file in CVS.
I can' think of any CVS file that is updated in an automated manner like
that.

If a buildfarm member can't compile one day does it update with those
entries missing, then re-add them the next day?

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

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

Alvaro Herrera wrote:
> Bruce Momjian wrote:
>
> > I have created a proper typedef file that I would normally use for a
> > pgindent run of the entire tree (it has /contrib, 2628 entries). It is
> > at:
> >
> > http://momjian.us/expire/pgtypedefs.bsdos
>
> Well, there are typedefs in there not used anywhere in our code, for
> example ASN1_GENERALIZEDTIME ...

Yep. I assume the Win32 list is longer only because the Win32 API is
larger.

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

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

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > Andrew Dunstan wrote:
> >> It looks like we'll need some sort of extra filter.
>
> > Hmm. Wow. For example I see
>
> > FINDREPLACE
> > FINDREPLACEA
> > FINDREPLACEW
>
> > We use neither ... My guess is that they are used in the system DLLs or
> > something like that.
>
> Presumably we could grep our own sources for each proposed typedef list
> entry --- no hits, you don't get in.

Just came up with this:

> found
> not-found
while read line ; do
echo "looking for $line"
rgrep -q --exclude cscope.out --exclude pgtypedefs.bsdos --exclude tags "\<$line\>" .
if [ $? == 0 ]; then
echo $line >> found
else
echo $line >> not-found
fi
done < pgtypedefs.bsdos

It's simple enough that there are some false matches, for example for
"AV" (which is a symbol we do use, but it also appears in strings etc).
But I'd say it's more than enough.

It does take a while to run though ... it's not something we'll want to
do routinely.

Okay, it finished:

$ wc -l found not-found
2035 found
592 not-found
2627 total

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Tom Lane wrote:
>> If we're going to go down this path, why would we not put the
>> "reference" typedef list into CVS?

> Uh, I assume we don't want an automated system updating the file in CVS.

Nowhere did I suggest that. What I suggested is that the "considered
good" reference list should be in CVS, not on some random wiki page.
In particular there's something pretty broken about the idea of a file
in CVS telling people to go to a wiki page for important data.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Tom Lane wrote:
> >> If we're going to go down this path, why would we not put the
> >> "reference" typedef list into CVS?
>
> > Uh, I assume we don't want an automated system updating the file in CVS.
>
> Nowhere did I suggest that. What I suggested is that the "considered
> good" reference list should be in CVS, not on some random wiki page.
> In particular there's something pretty broken about the idea of a file
> in CVS telling people to go to a wiki page for important data.

I have no problem using a URL to pull down the typedef list via wget.

How is that CVS file going to be updated? Is someone manually going to
update it in CVS, and how often?

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

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

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> It does take a while to run though ... it's not something we'll want to
> do routinely.

Well, we're not going to want to change the reference typedef list very
often anyway, because it'd just result in whitespace-thrashing in the
repository. I'm thinking an update once or twice per major release
cycle would be enough. So a basically manual process that combines the
results from various buildfarm members, and then filters them like this,
should be workable.

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > It does take a while to run though ... it's not something we'll want to
> > do routinely.
>
> Well, we're not going to want to change the reference typedef list very
> often anyway, because it'd just result in whitespace-thrashing in the
> repository. I'm thinking an update once or twice per major release
> cycle would be enough. So a basically manual process that combines the
> results from various buildfarm members, and then filters them like this,
> should be workable.

OK, fine, but when I do the full source tree pgindent, I will need a URL
to get the list, so let's document its location in the README.

Also, this improved typedef list is only making pgindent 0.2% better.
Does someone want to look at improving the pgindent script itself? Might
be a good time now that we have an updated typedef list for 8.4.

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

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

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> I have no problem using a URL to pull down the typedef list via wget.

> How is that CVS file going to be updated?

I do not follow your thought process. You would rather depend on a URL
that has no visible commit history?

As I already noted elsewhere in the thread, the last thing we want is a
typedef list that changes every five minutes. It *should* have adult
supervision, and the effort of checking a vetted update into CVS seems
entirely appropriate to me.

regards, tom lane

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Does someone want to look at improving the pgindent script itself?

I notice that you've carefully ignored the suggestion of re-testing
GNU indent.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
>> I have no problem using a URL to pull down the typedef list via wget.
>
>> How is that CVS file going to be updated?
>
> I do not follow your thought process. You would rather depend on a URL
> that has no visible commit history?

This does seem odd. Bruce if you want to use wget, point the wget at the
cvsweb repo checkout.

Joshua D. Drake

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Does someone want to look at improving the pgindent script itself?
>
> I notice that you've carefully ignored the suggestion of re-testing
> GNU indent.

No. Why would I carefully ignore testing GNU indent? Because I am
afraid pgindent will improve?

Please, folks, start taking over the things I do: FAQs, TODO, pgindent,
patch queue, whatever. I am tired of veiled complaints about how I
handle things. I do my best. Let someone else handle them and then I
can complain too.

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

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

What is our plan for pgindent for 8.4? I would rather not have to bug
someone to create a list of symbols manually. I would like it to be
built on a regular basis and I can pull it from there and add it to CVS
when I run pgindent.

---------------------------------------------------------------------------

Andrew Dunstan wrote:
>
>
> Gregory Stark wrote:
> > "Andrew Dunstan" <andrew(at)dunslane(dot)net> writes:
> >
> >
> >> Tom Lane wrote:
> >>
> >>> doxygen's 200-some is clearly an order of magnitude too low, but I
> >>> wonder whether Bruce's list hasn't got some false hits ...
> >>>
> >
> > Skimming the output it does have things like "int" and "float" but presumably
> > we would know if that caused any problem, they wouldn't inflate the numbers
> > much.
> >
> >
> >> 2800 does seem a bit high. My buildfarm member dungbeetle just found 2482 on a
> >> build that is only missing the optional pam, bonjour and gssapi config options.
> >>
> >
> > The numbers going to vary heavily from OS to OS so it seems to me that these
> > are a basically the same order of magnitude.
> >
>
> It looks like Windows will blow all our existing numbers out of the
> water. Here's a list generated from Cygwin with 6088 symbols. I'm
> working on getting a similar list from MinGW.
>
> http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=brown_bat&dt=2008-04-18%20230054&stg=typedefs
>
> cheers
>
> andrew
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

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

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

I will try to look at it again in a few days.

cheers

andrew

Bruce Momjian wrote:
> What is our plan for pgindent for 8.4? I would rather not have to bug
> someone to create a list of symbols manually. I would like it to be
> built on a regular basis and I can pull it from there and add it to CVS
> when I run pgindent.
>
> ---------------------------------------------------------------------------
>
> Andrew Dunstan wrote:
>
>> Gregory Stark wrote:
>>
>>> "Andrew Dunstan" <andrew(at)dunslane(dot)net> writes:
>>>
>>>
>>>
>>>> Tom Lane wrote:
>>>>
>>>>
>>>>> doxygen's 200-some is clearly an order of magnitude too low, but I
>>>>> wonder whether Bruce's list hasn't got some false hits ...
>>>>>
>>>>>
>>> Skimming the output it does have things like "int" and "float" but presumably
>>> we would know if that caused any problem, they wouldn't inflate the numbers
>>> much.
>>>
>>>
>>>
>>>> 2800 does seem a bit high. My buildfarm member dungbeetle just found 2482 on a
>>>> build that is only missing the optional pam, bonjour and gssapi config options.
>>>>
>>>>
>>> The numbers going to vary heavily from OS to OS so it seems to me that these
>>> are a basically the same order of magnitude.
>>>
>>>
>> It looks like Windows will blow all our existing numbers out of the
>> water. Here's a list generated from Cygwin with 6088 symbols. I'm
>> working on getting a similar list from MinGW.
>>
>> http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=brown_bat&dt=2008-04-18%20230054&stg=typedefs
>>
>> cheers
>>
>> andrew
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>
>