Thread: relscan.h split

Hi,

relscan.h is very widely used -- in particular it is included by some
headers that want the IndexScanDesc and HeapScanDesc definitions in
prototypes.  However, most of the time they are just passing the struct
through; they don't need to see the actual Heap/IndexScanDescData
definitions.

I propose the following patch which moves the struct definitions to a
separate new header relscan_internal.h.  Files that actually need the
definitions can include the new header.  They are not that many -- I
count 22 inclusions, all of them in .c files.  Headers only include the
.h file, which has the benefit that since it is a lean file, it needn't
include all the other headers needed by the struct declaration.

Zdenek says that this patch changes the number of times certain headers
are opened (data gathered using dtrace):

                                new     old     diff
src/include/access/skey.h       347     465     -118
src/include/utils/rel.h         851     921     -70
src/include/access/relscan.h    340     360     -20

So it doesn't have a tremendous impact, but it does have some.

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

Alvaro Herrera <alvherre@commandprompt.com> writes:
> I propose the following patch which moves the struct definitions to a
> separate new header relscan_internal.h.

This seems a little bizarre, seeing that there is almost nothing in
relscan.h except those structs.

Perhaps a better idea would be to put the opaque-pointer typedefs into
heapam.h and genam.h respectively, and then see where you could remove
inclusions of relscan.h.

Also, it seemed like some of those .c files had no business poking into
the scan structs anyway; particularly contrib.  Did you check whether
the inclusions could be avoided?

            regards, tom lane

Tom Lane wrote:

> Perhaps a better idea would be to put the opaque-pointer typedefs into
> heapam.h and genam.h respectively, and then see where you could remove
> inclusions of relscan.h.

Hmm, this seems to be closely equivalent.  Patch attached.  I also moved
SysScanDescData from genam.h to relscan.h.

> Also, it seemed like some of those .c files had no business poking into
> the scan structs anyway; particularly contrib.  Did you check whether
> the inclusions could be avoided?

Not really, unless we were to provide something a routine that returns
the current block of a scan.  There are a few occurrences of this:

        /* must hold a buffer lock to call HeapTupleSatisfiesUpdate */
        LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);

which of course need the definition.  Maybe providing it is not a bad
idea, because that kind of coding is used in the backend too.

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

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Tom Lane wrote:
>> Also, it seemed like some of those .c files had no business poking into
>> the scan structs anyway; particularly contrib.  Did you check whether
>> the inclusions could be avoided?

> Not really, unless we were to provide something a routine that returns
> the current block of a scan.

Current buffer you mean.  That wouldn't be a bad idea --- the wart I
added to genam.c the other day to recheck the current tuple could be
replaced with that, I think, though it wouldn't really be much less
warty.

BTW I think your change in pgstattuple.c is probably dangerous: if the
relation gets extended between where heap_beginscan_strat sets
rs_nblocks and where you put RelationGetNumberOfBlocks, I think the
block counting will get messed up.  That one really does need access
to the internals.

Other than that, this looks pretty sane to me.

            regards, tom lane