Re: [HACKERS] src/tools/pginclude considered harmful (was Re:

2006-07-18 Thread Bruce Momjian

Good, added to pginclude/README:

Also, tests should be done with configure settings of --enable-cassert
and EXEC_BACKEND on and off.

I think we had more problems this time just because our code is more
complex.

---

Tom Lane wrote:
 Bruce Momjian [EMAIL PROTECTED] writes:
  FYI, 527 include were removed from non-header C files in this run.  That
  is not something that can be easily done manually.
 
 It's not so easily done automatically, either :-(.  I'm not sure why
 this go-round was so much more painful than the last, but it very
 clearly exposed the deficiencies in your testing process.  Mainly,
 that you tested only one set of configure options on one platform.
 
 I would say that minimum requirements for doing this again in future
 are (1) test with and without --enable-cassert, and (2) test with and
 without EXEC_BACKEND.  We *know* that both those things change the
 set of headers required.  It might be necessary to actually test the
 WIN32 port separately --- EXEC_BACKEND is close but not the same.
 
   regards, tom lane

-- 
  Bruce Momjian   [EMAIL PROTECTED]
  EnterpriseDBhttp://www.enterprisedb.com

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

---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings


Re: [HACKERS] src/tools/pginclude considered harmful (was Re:

2006-07-17 Thread Bruce Momjian

FYI, I updated pginclude/README to explain the complexity of
removing includes from include files:

pgfixinclude
sort include references
run multiple times:
pgcompinclude
pgrminclude /src/include
pgrminclude /
pgcheckdefines

There is a complexity when modifying /src/include.  If include file 1
includes file 2, and file 2 includes file 3, then when file 1 is
processed, it needs only file 2, not file 3.  However, if later, include
file 2 is processed, and file 3 is not needed by file 2 and is removed,
file 1 might then need to include file 3.  For this reason, the
pgcompinclude and pgrminclude /src/include steps must be run several
times until all includes compile cleanly.

I followed this process, but didn't full understand why multiple include
runs were necessary until I thought about it.

FYI, 527 include were removed from non-header C files in this run.  That
is not something that can be easily done manually.

---

Tom Lane wrote:
 Andrew Dunstan [EMAIL PROTECTED] writes:
  I agree with reverting. The tool looks pretty broken anyway, with 
  hardcoded paths and all sorts of stuff quite apart from logic problems.
 
 Well, it's only intended to work on Bruce's system, so until someone
 else takes over the position of chief gruntwork-doer I don't think the
 portability issues are much of a factor.  What I'm worried about at the
 moment is the correctness issue.
 
 After some reflection it seems that there is only one case where removal
 of a needed include file would not lead to a compiler error or warning,
 assuming gcc with ordinary -W settings (notably -Wmissing-prototypes).
 That case is exactly what Kris found: removal of a #define that is
 tested via #ifdef or #if defined().  (Can anyone think of other cases?)
 
 It's not hard to imagine getting burnt by this through ordinary manual
 rearrangement or cleanup of inclusions, so I'm thinking that blaming
 pgrminclude may be too hasty.  It'd be worth our time to make a tool to
 check for it.  I'm going to work on a Perl script that sucks up all the
 #defines in all our .h files and looks for #ifdef foo or defined foo
 where foo is defined in a file not included by the referencing file
 (gcc -H looks like the right tool for checking that).  Another thing
 such a script could easily do is check for inclusion of system headers
 before c.h.
 
   regards, tom lane

-- 
  Bruce Momjian   [EMAIL PROTECTED]
  EnterpriseDBhttp://www.enterprisedb.com

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

---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster


Re: [HACKERS] src/tools/pginclude considered harmful (was Re:

2006-07-17 Thread Tom Lane
Bruce Momjian [EMAIL PROTECTED] writes:
 FYI, 527 include were removed from non-header C files in this run.  That
 is not something that can be easily done manually.

It's not so easily done automatically, either :-(.  I'm not sure why
this go-round was so much more painful than the last, but it very
clearly exposed the deficiencies in your testing process.  Mainly,
that you tested only one set of configure options on one platform.

I would say that minimum requirements for doing this again in future
are (1) test with and without --enable-cassert, and (2) test with and
without EXEC_BACKEND.  We *know* that both those things change the
set of headers required.  It might be necessary to actually test the
WIN32 port separately --- EXEC_BACKEND is close but not the same.

regards, tom lane

---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?

   http://www.postgresql.org/docs/faq


Re: [HACKERS] src/tools/pginclude considered harmful (was Re: [PATCHES]

2006-07-14 Thread Andrew Dunstan

Tom Lane wrote:


In combination with the amount of time wasted over the past two days,
it is now perfectly clear that the existing pginclude tools are not
NEARLY good enough to detect what they are breaking.  I would like to
propose that we revert all the include-related changes of the past two
days, and that src/tools/pginclude be removed from the CVS tree, until
such time as it is rewritten to be much smarter about what it is doing.

 



I agree with reverting. The tool looks pretty broken anyway, with 
hardcoded paths and all sorts of stuff quite apart from logic problems.


cheers

andrew

---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?

  http://www.postgresql.org/docs/faq


Re: [HACKERS] src/tools/pginclude considered harmful (was Re: [PATCHES]

2006-07-14 Thread Tom Lane
Andrew Dunstan [EMAIL PROTECTED] writes:
 I agree with reverting. The tool looks pretty broken anyway, with 
 hardcoded paths and all sorts of stuff quite apart from logic problems.

Well, it's only intended to work on Bruce's system, so until someone
else takes over the position of chief gruntwork-doer I don't think the
portability issues are much of a factor.  What I'm worried about at the
moment is the correctness issue.

After some reflection it seems that there is only one case where removal
of a needed include file would not lead to a compiler error or warning,
assuming gcc with ordinary -W settings (notably -Wmissing-prototypes).
That case is exactly what Kris found: removal of a #define that is
tested via #ifdef or #if defined().  (Can anyone think of other cases?)

It's not hard to imagine getting burnt by this through ordinary manual
rearrangement or cleanup of inclusions, so I'm thinking that blaming
pgrminclude may be too hasty.  It'd be worth our time to make a tool to
check for it.  I'm going to work on a Perl script that sucks up all the
#defines in all our .h files and looks for #ifdef foo or defined foo
where foo is defined in a file not included by the referencing file
(gcc -H looks like the right tool for checking that).  Another thing
such a script could easily do is check for inclusion of system headers
before c.h.

regards, tom lane

---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster


Re: [HACKERS] src/tools/pginclude considered harmful (was Re: [PATCHES]

2006-07-14 Thread Andrew Dunstan

Tom Lane wrote:


Andrew Dunstan [EMAIL PROTECTED] writes:
 

I agree with reverting. The tool looks pretty broken anyway, with 
hardcoded paths and all sorts of stuff quite apart from logic problems.
   



Well, it's only intended to work on Bruce's system, so until someone
else takes over the position of chief gruntwork-doer I don't think the
portability issues are much of a factor.  




Shouldn't the README reflect that, then?

cheers

andrew

---(end of broadcast)---
TIP 4: Have you searched our list archives?

  http://archives.postgresql.org


Re: [HACKERS] src/tools/pginclude considered harmful (was Re:

2006-07-14 Thread Bruce Momjian
Tom Lane wrote:
 Martijn van Oosterhout kleptog@svana.org writes:
  On Fri, Jul 14, 2006 at 04:24:59PM -0400, Tom Lane wrote:
  After some reflection it seems that there is only one case where removal
  of a needed include file would not lead to a compiler error or warning,
  assuming gcc with ordinary -W settings (notably -Wmissing-prototypes).
  That case is exactly what Kris found: removal of a #define that is
  tested via #ifdef or #if defined().  (Can anyone think of other cases?)
 
  My off-the-top-of-my-head solution would be a script that would pass
  each file through gcc -E (the preprocessor), and compare before and
  after rearrangement. You'd have to ignore the effects of included
  header files, but it would pick up the cases where a block of code that
  was previously included no longer is. Or if a macro is expanded
  differently...
 
 You'd still have to try to compile the code though; AFAICS the above
 doesn't catch whether you've removed a typedef or function declaration
 that's referenced in the file.
 
 BTW, one of the remaining holes in pgrminclude is that it compiles with
 -fsyntax-only, which apparently causes it to fail to detect some errors
 of significance --- I assume that's how it managed to foul up lmgr.c,
 inet_net_ntop.c, etc.

But I do run a full compile and regression before commit, so that should
have caught it even if pgrminclude didn't.

-- 
  Bruce Momjian   [EMAIL PROTECTED]
  EnterpriseDBhttp://www.enterprisedb.com

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

---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings


Re: [HACKERS] src/tools/pginclude considered harmful (was Re:

2006-07-14 Thread Bruce Momjian
Andrew Dunstan wrote:
 Tom Lane wrote:
 
 Andrew Dunstan [EMAIL PROTECTED] writes:
   
 
 I agree with reverting. The tool looks pretty broken anyway, with 
 hardcoded paths and all sorts of stuff quite apart from logic problems.
 
 
 
 Well, it's only intended to work on Bruce's system, so until someone
 else takes over the position of chief gruntwork-doer I don't think the
 portability issues are much of a factor.  
 
 
 
 Shouldn't the README reflect that, then?

The script should work on any computer.  I have just never tried it. 
Feel free to send in a patch to make it work on yours.

-- 
  Bruce Momjian   [EMAIL PROTECTED]
  EnterpriseDBhttp://www.enterprisedb.com

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

---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
   subscribe-nomail command to [EMAIL PROTECTED] so that your
   message can get through to the mailing list cleanly


Re: [HACKERS] src/tools/pginclude considered harmful (was Re:

2006-07-14 Thread Bruce Momjian
Tom Lane wrote:
 Bruce Momjian [EMAIL PROTECTED] writes:
  Tom Lane wrote:
  BTW, one of the remaining holes in pgrminclude is that it compiles with
  -fsyntax-only, which apparently causes it to fail to detect some errors
  of significance --- I assume that's how it managed to foul up lmgr.c,
  inet_net_ntop.c, etc.
 
  But I do run a full compile and regression before commit, so that should
  have caught it even if pgrminclude didn't.
 
 Doesn't catch warnings (unless you add -Werror during the build, which
 will have problems with the remaining expected warnings...)

/tools/pgtest always greps out the warnings and displays them at the end
of the regression tests.

-- 
  Bruce Momjian   [EMAIL PROTECTED]
  EnterpriseDBhttp://www.enterprisedb.com

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

---(end of broadcast)---
TIP 6: explain analyze is your friend


Re: [HACKERS] src/tools/pginclude considered harmful (was Re: [PATCHES]

2006-07-14 Thread Martijn van Oosterhout
On Fri, Jul 14, 2006 at 04:24:59PM -0400, Tom Lane wrote:
 After some reflection it seems that there is only one case where removal
 of a needed include file would not lead to a compiler error or warning,
 assuming gcc with ordinary -W settings (notably -Wmissing-prototypes).
 That case is exactly what Kris found: removal of a #define that is
 tested via #ifdef or #if defined().  (Can anyone think of other cases?)

My off-the-top-of-my-head solution would be a script that would pass
each file through gcc -E (the preprocessor), and compare before and
after rearrangement. You'd have to ignore the effects of included
header files, but it would pick up the cases where a block of code that
was previously included no longer is. Or if a macro is expanded
differently...

Have a nice day,
-- 
Martijn van Oosterhout   kleptog@svana.org   http://svana.org/kleptog/
 From each according to his ability. To each according to his ability to 
 litigate.


signature.asc
Description: Digital signature


Re: [HACKERS] src/tools/pginclude considered harmful (was Re:

2006-07-14 Thread Neil Conway
On Fri, 2006-07-14 at 14:20 -0400, Tom Lane wrote:
 I would like to propose that we revert all the include-related changes
 of the past two days, and that src/tools/pginclude be removed from the
 CVS tree, until such time as it is rewritten to be much smarter about
 what it is doing.

Rather than reverting the changes in CVS and then redoing them
correctly, perhaps we could make the necessary improvements to the
tools, apply the improved tools to the pre-cleaned-up version of the
tree, get a diff against HEAD, and then apply any fixes the improved
tools have made as a patch. That would avoid cluttering CVS with two
redundant changes to almost every single source file in the tree.

In any case, I agree that the cleanup was not very well done. Widespread
cleanup of this sort should be done with a lot more care. One useful
improvement would be to make the changes on a local repository first,
and then post the suggested changes as a patch. Making the entire set of
required changes as a single CVS commit would also be good: it makes it
easier for folks to back out the patch locally if it causes problems, as
well as making the CVS history more comprehensible.

-Neil



---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings


Re: [HACKERS] src/tools/pginclude considered harmful (was Re: [PATCHES]

2006-07-14 Thread Tom Lane
Bruce Momjian [EMAIL PROTECTED] writes:
 Tom Lane wrote:
 BTW, one of the remaining holes in pgrminclude is that it compiles with
 -fsyntax-only, which apparently causes it to fail to detect some errors
 of significance --- I assume that's how it managed to foul up lmgr.c,
 inet_net_ntop.c, etc.

 But I do run a full compile and regression before commit, so that should
 have caught it even if pgrminclude didn't.

Doesn't catch warnings (unless you add -Werror during the build, which
will have problems with the remaining expected warnings...)

regards, tom lane

---(end of broadcast)---
TIP 6: explain analyze is your friend


Re: [HACKERS] src/tools/pginclude considered harmful (was Re: [PATCHES]

2006-07-14 Thread Tom Lane
Martijn van Oosterhout kleptog@svana.org writes:
 On Fri, Jul 14, 2006 at 04:24:59PM -0400, Tom Lane wrote:
 After some reflection it seems that there is only one case where removal
 of a needed include file would not lead to a compiler error or warning,
 assuming gcc with ordinary -W settings (notably -Wmissing-prototypes).
 That case is exactly what Kris found: removal of a #define that is
 tested via #ifdef or #if defined().  (Can anyone think of other cases?)

 My off-the-top-of-my-head solution would be a script that would pass
 each file through gcc -E (the preprocessor), and compare before and
 after rearrangement. You'd have to ignore the effects of included
 header files, but it would pick up the cases where a block of code that
 was previously included no longer is. Or if a macro is expanded
 differently...

You'd still have to try to compile the code though; AFAICS the above
doesn't catch whether you've removed a typedef or function declaration
that's referenced in the file.

BTW, one of the remaining holes in pgrminclude is that it compiles with
-fsyntax-only, which apparently causes it to fail to detect some errors
of significance --- I assume that's how it managed to foul up lmgr.c,
inet_net_ntop.c, etc.

regards, tom lane

---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings


Re: [HACKERS] src/tools/pginclude considered harmful (was Re: [PATCHES] toast index entries again)

2006-07-14 Thread Tom Lane
Neil Conway [EMAIL PROTECTED] writes:
 On Fri, 2006-07-14 at 14:20 -0400, Tom Lane wrote:
 I would like to propose that we revert all the include-related changes
 of the past two days, and that src/tools/pginclude be removed from the
 CVS tree, until such time as it is rewritten to be much smarter about
 what it is doing.

 Rather than reverting the changes in CVS and then redoing them
 correctly, perhaps we could make the necessary improvements to the
 tools, apply the improved tools to the pre-cleaned-up version of the
 tree, get a diff against HEAD, and then apply any fixes the improved
 tools have made as a patch. That would avoid cluttering CVS with two
 redundant changes to almost every single source file in the tree.

I've calmed down a little and am no longer wanting to insist on
reversion.  My little Perl script is turning over and has found a number
of issues besides the original TOAST_INDEX_HACK one; some of them may
have been there before, so I'm thinking I'm going to add it to src/tools
rather than just treat it as a one-shot.

We still have the issue of how Bruce managed to miss undeclared-function
warnings in some files.  I'm concerned that that problem may remain for
some platforms or option combinations.  Is there a way to get the
buildfarm to highlight compiler warnings?

regards, tom lane

---(end of broadcast)---
TIP 4: Have you searched our list archives?

   http://archives.postgresql.org