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

Kris Jurka <books@ejurka.com> writes:
> The inclusion of access/tuptoaster.h in access/common/indextuple.c brought 
> in the define of TOAST_INDEX_HACK which compresses large index entries. 
> When this was removed the entries were no longer compressed which caused 
> btree_gist to fail.

This is seriously scary, as it seems quite possible that there are other
similar cases in which we have simply silently lost functionality or
performance, and none of the regression tests will expose it.

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.
        regards, tom lane

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

Andrew Dunstan <andrew@dunslane.net> 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

Tom Lane wrote:

>Andrew Dunstan <andrew@dunslane.net> 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

Andrew Dunstan wrote:
> Tom Lane wrote:
> 
> >Andrew Dunstan <andrew@dunslane.net> 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   bruce@momjian.us EnterpriseDB    http://www.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

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.

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

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

Neil Conway <neilc@samurai.com> 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

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 /    pgcheckdefinesThere is a complexity when modifying
/src/include. If include file 1includes file 2, and file 2 includes file 3, then when file 1 isprocessed, it needs only
file2, not file 3.  However, if later, includefile 2 is processed, and file 3 is not needed by file 2 and is
removed,file1 might then need to include file 3.  For this reason, thepgcompinclude and pgrminclude /src/include steps
mustbe run severaltimes 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 <andrew@dunslane.net> 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   bruce@momjian.us EnterpriseDB    http://www.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> 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

Good, added to pginclude/README:
Also, tests should be done with configure settings of --enable-cassertand 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 <bruce@momjian.us> 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   bruce@momjian.us EnterpriseDB    http://www.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +