SEO

Hi,

I noticed heapam.h is included in way too many places. This is bad IMHO
because heapam.h itself includes a lot of other headers.

A lot of these are easy to fix; the source files just need to include
some other headers. Standard cleanup, I don't think anybody would
object to that. For example,

Index: src/backend/access/gin/ginvacuum.c
================================================== =================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/gin/ginvacuum.c,v
retrieving revision 1.19
diff -c -p -r1.19 ginvacuum.c
*** src/backend/access/gin/ginvacuum.c 1 Jan 2008 19:45:46 -0000 1.19
--- src/backend/access/gin/ginvacuum.c 9 May 2008 18:44:31 -0000
***************
*** 15,24 ****
#include "postgres.h"
#include "access/genam.h"
#include "access/gin.h"
- #include "access/heapam.h"
#include "miscadmin.h"
#include "storage/freespace.h"
! #include "storage/freespace.h"
#include "commands/vacuum.h"

typedef struct
--- 15,23 ----
#include "postgres.h"
#include "access/genam.h"
#include "access/gin.h"
#include "miscadmin.h"
#include "storage/freespace.h"
! #include "storage/lmgr.h"
#include "commands/vacuum.h"

typedef struct


Others are more conflictive. For example syncscan.c is keeping the
prototypes for its own functions on heapam.h. Also pruneheap.c and
rewriteheap.c. As a result, not only themselves need to include
heapam.h (without any other need for it), but they force some other
files into including heapam.h to get their prototypes. I think this is
a mistake; I propose splitting those prototypes to their own files, and
#including those as appropriate.

Objections?

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

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

Alvaro Herrera napsal(a):

> Others are more conflictive. For example syncscan.c is keeping the
> prototypes for its own functions on heapam.h. Also pruneheap.c and
> rewriteheap.c. As a result, not only themselves need to include
> heapam.h (without any other need for it), but they force some other
> files into including heapam.h to get their prototypes. I think this is
> a mistake; I propose splitting those prototypes to their own files, and
> #including those as appropriate.
>
> Objections?

I have similar thing in my TODO list. See my patch from March commit fest and
discussion. I need solve two main issues - remove postgres.h from binaries and
keep history of structures (for pg_upgrade project).

My idea is split structures and functions in separate header files.

Zdenek

http://archives.postgresql.org/pgsql...0/msg00197.php
http://archives.postgresql.org/pgsql...4/msg00149.php




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

Zdenek Kotala wrote:
> Alvaro Herrera napsal(a):
>
>> Others are more conflictive. For example syncscan.c is keeping the
>> prototypes for its own functions on heapam.h. Also pruneheap.c and
>> rewriteheap.c. As a result, not only themselves need to include
>> heapam.h (without any other need for it), but they force some other
>> files into including heapam.h to get their prototypes. I think this is
>> a mistake; I propose splitting those prototypes to their own files, and
>> #including those as appropriate.
>>
>> Objections?
>
> I have similar thing in my TODO list. See my patch from March commit fest
> and discussion. I need solve two main issues - remove postgres.h from
> binaries and keep history of structures (for pg_upgrade project).

Yeah, I remember that. Is there any progress on that front?

BTW I noticed that I was a bit careless in the description. rewriteheap
already has its own rewriteheap.h file; and there's no point at all in
separating pruneheap.c declarations into another file.

The one that makes a bit more sense is a new syncscan.h. And there are
a lot of things in heapam.h that actually correspond to tuple
manipulation (heap_form_tuple and so on), so perhaps a new header file
would be appropriate, but there's already htup.h which contains
tuple-related stuff.

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

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

Alvaro Herrera <alvherre@commandprompt.com> writes:
> The one that makes a bit more sense is a new syncscan.h. And there are
> a lot of things in heapam.h that actually correspond to tuple
> manipulation (heap_form_tuple and so on), so perhaps a new header file
> would be appropriate, but there's already htup.h which contains
> tuple-related stuff.

After actually looking at the header a bit ...

+1 for moving fastgetattr, heap_getattr, and the heaptuple.c functions
to htup.h. I don't see any big gain from relocating the other stuff;
it seems to largely all use about the same set of typedefs.

It looks to me actually that a large part of your complaint is that
heapam.h #includes more than it has to. Have you tried just cutting its
#include list to the minimum needed to compile its function declarations?
Likely this would force more #includes in .c files but I don't object
to that. (I might be wrong, but I believe that Bruce's script for
removing "unnecessary" #includes is not bright enough to make such
tradeoffs, so it'd let bloated #include lists in headers survive.)

regards, tom lane

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

Tom Lane wrote:

> +1 for moving fastgetattr, heap_getattr, and the heaptuple.c functions
> to htup.h. I don't see any big gain from relocating the other stuff;
> it seems to largely all use about the same set of typedefs.

Ultimately that was my conclusion too. I tried moving the heaptuple.c
functions to htup.h, but hit the problem that pg_dump.c wants to include
that file. It then fails to compile because it doesn't know Datum
(defined in postgres.h, so unavailable to frontend programs). I worked
around that by enclosing the prototypes in #ifndef FRONTEND, but that
seems ugly.

Apparently the reason for pg_dump.c to need htup.h is getAttrName which
needs system columns' attribute numbers. Of course, the first thing
that comes to mind is that we should fix pg_dump to not require that
header in the first place. Perhaps we can get the names by querying the
server, at the same time we get the user column names. (Apparently this
is only used to dump unique indexes on system columns.)

The easiest actual solution, however, seems to be to move those
attribute numbers to a separate header, say access/sysattrs.h.

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

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

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Apparently the reason for pg_dump.c to need htup.h is getAttrName which
> needs system columns' attribute numbers. Of course, the first thing
> that comes to mind is that we should fix pg_dump to not require that
> header in the first place. Perhaps we can get the names by querying the
> server, at the same time we get the user column names. (Apparently this
> is only used to dump unique indexes on system columns.)

> The easiest actual solution, however, seems to be to move those
> attribute numbers to a separate header, say access/sysattrs.h.

Yeah, probably. There is certainly no reason for a frontend program
to be dealing in Datum; but it isn't unreasonable for it to know
the system column numbers, since it can see those in the catalogs.

I'm sure we have work in front of us to get these things separated
out a bit better.

regards, tom lane

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

Alvaro Herrera <alvherre@commandprompt.com> writes:
> So here's a patch (includes the #ifndef FRONTEND hack in htup.h.)

I like this except for the #ifndef FRONTEND hack --- you're going to
need to fix that before applying. I'm good with doing that by pushing
the system attribute numbers into a separate header.

BTW, you didn't compress the patch, which likely explains why it didn't
show up on -hackers. If you want to post a shorter form, I think that
the diffs in the header files are the only interesting part; the ensuing
additions in .c files are just mechanical.

regards, tom lane

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

Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > So here's a patch (includes the #ifndef FRONTEND hack in htup.h.)
>
> I like this except for the #ifndef FRONTEND hack --- you're going to
> need to fix that before applying. I'm good with doing that by pushing
> the system attribute numbers into a separate header.

Committed with the attribute numbers moved to access/sysattr.h, which is
what pg_dump now uses.

Thanks.

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

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

Alvaro Herrera wrote:
> Tom Lane wrote:
> > Alvaro Herrera <alvherre@commandprompt.com> writes:
> > > So here's a patch (includes the #ifndef FRONTEND hack in htup.h.)
> >
> > I like this except for the #ifndef FRONTEND hack --- you're going to
> > need to fix that before applying. I'm good with doing that by pushing
> > the system attribute numbers into a separate header.
>
> Committed with the attribute numbers moved to access/sysattr.h, which is
> what pg_dump now uses.

Oops :-( I just noticed that I removed bufmgr.h from bufpage.h, which
is a change you had objected to previously :-(

http://archives.postgresql.org/pgsql...4/msg00149.php

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

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

Alvaro Herrera wrote:

> Oops :-( I just noticed that I removed bufmgr.h from bufpage.h, which
> is a change you had objected to previously :-(

However, it seems the right fix is to move BufferGetPageSize and
BufferGetPage from bufpage.h to bufmgr.h.

(Digging further, it seems like bufpage.h should also include transam.h
in order to get TransactionIdIsNormal ... I start to wonder how many
problems of this nature we have on our headers. Without having a way to
detect whether the defined macros are valid, it seems hard to check
programatically, however.)

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

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