Обсуждение: Reduce the dependence on access/xlog_internal.h

Hackers,

Please find access/xlog_internal.h refactored in the attached patch series.  This header is included from many places,
includingexternal tools.  It is aesthetically displeasing to have something called "internal" used from so many places,
especiallywhen many of those places do not deal directly with the internal workings of xlog.  But it is even worse that
multiplefiles include this header for no reason. 

A small portion of access/xlog_internal.h defines the RmgrData struct, and in support of this struct the header
includesa number of other headers.  Files that include access/xlog_internal.h indirectly include these other headers,
whichmost do not need. (Only 3 out of 41 files involved actually need that portion of the header.)  For third-party
toolswhich deal with backup, restore, or replication matters, including xlog_internal.h is necessary to get macros for
calculatingxlog file names, but doing so also indirectly pulls in other headers, increasing the risk of unwanted symbol
collisions. Some colleagues and I ran into this exact problem in a C++ program that uses both xlog_internal.h and the
BoostC++ library. 


0001 - Removes gratuitous inclusion of access/xlog_internal.h from *.c files in core that are currently including it
withoutneed.  The following files are so modified: 

    src/backend/access/transam/rmgr.c
    src/backend/postmaster/bgwriter.c
    src/backend/replication/logical/decode.c
    src/backend/replication/logical/logical.c
    src/backend/replication/logical/logicalfuncs.c
    src/backend/replication/logical/worker.c
    src/bin/pg_basebackup/pg_recvlogical.c
    src/bin/pg_rewind/timeline.c
    src/bin/pg_waldump/rmgrdesc.c

0002 - Moves RmgrData from access/xlog_internal.h into a new file access/rmgr_internal.h.  I clearly did not waste time
thinkingof a clever file name.  Bikeshedding welcome.  Most files which currently include xlog_internal.h do not need
thedefinition of RmgrData.  As it stands now, inclusion of xlog_internal.h indirectly includes the following headers: 

    <fcntl.h>
    "access/rmgr.h"
    "access/rmgrlist.h"
    "access/transam.h"
    "access/xlogdefs.h"
    "access/xlogreader.h"
    "access/xlogrecord.h"
    "catalog/catversion.h"
    "common/relpath.h"
    "datatype/timestamp.h"
    "lib/stringinfo.h"
    "pgtime.h"
    "port/pg_bswap.h"
    "port/pg_crc32c.h"
    "storage/backendid.h"
    "storage/block.h"
    "storage/relfilenode.h"

After refactoring, the inclusion of xlog_internal.h includes indirectly only these headers:

    <fcntl.h>
    "access/xlogdefs.h"
    "datatype/timestamp.h"
    "pgtime.h"

and only these files need to be altered to include the new rmgr_internal.h header:

    src/backend/access/transam/rmgr.c
    src/backend/access/transam/xlog.c
    src/backend/utils/misc/guc.c

Thoughts?



—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

On 2020-10-19 18:29:27 -0700, Mark Dilger wrote:
> Please find access/xlog_internal.h refactored in the attached patch
> series.  This header is included from many places, including external
> tools.  It is aesthetically displeasing to have something called
> "internal" used from so many places, especially when many of those
> places do not deal directly with the internal workings of xlog.  But
> it is even worse that multiple files include this header for no
> reason.


> 0002 - Moves RmgrData from access/xlog_internal.h into a new file access/rmgr_internal.h.  I clearly did not waste
timethinking of a clever file name.  Bikeshedding welcome.  Most files which currently include xlog_internal.h do not
needthe definition of RmgrData.  As it stands now, inclusion of xlog_internal.h indirectly includes the following
headers:
> 
> After refactoring, the inclusion of xlog_internal.h includes indirectly only these headers:
> 
> and only these files need to be altered to include the new rmgr_internal.h header:
> 
>     src/backend/access/transam/rmgr.c
>     src/backend/access/transam/xlog.c
>     src/backend/utils/misc/guc.c
> 
> Thoughts?

It's not clear why the correct direction here is to make
xlog_internals.h less "low level" by moving things into headers like
rmgr_internal.h, rather than moving the widely used parts of
xlog_internal.h elsewhere.




> A small portion of access/xlog_internal.h defines the RmgrData struct,
> and in support of this struct the header includes a number of other
> headers.  Files that include access/xlog_internal.h indirectly include
> these other headers, which most do not need. (Only 3 out of 41 files
> involved actually need that portion of the header.)  For third-party
> tools which deal with backup, restore, or replication matters,
> including xlog_internal.h is necessary to get macros for calculating
> xlog file names, but doing so also indirectly pulls in other headers,
> increasing the risk of unwanted symbol collisions.  Some colleagues
> and I ran into this exact problem in a C++ program that uses both
> xlog_internal.h and the Boost C++ library.

It seems better to me to just use forward declarations for StringInfo
and XLogReaderState (and just generally use them mroe aggressively). We
don't need the functions for dealing with those datatypes here.


Greetings,

Andres Freund

> On Oct 19, 2020, at 7:05 PM, Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2020-10-19 18:29:27 -0700, Mark Dilger wrote:
>> Please find access/xlog_internal.h refactored in the attached patch
>> series.  This header is included from many places, including external
>> tools.  It is aesthetically displeasing to have something called
>> "internal" used from so many places, especially when many of those
>> places do not deal directly with the internal workings of xlog.  But
>> it is even worse that multiple files include this header for no
>> reason.
>
>
>> 0002 - Moves RmgrData from access/xlog_internal.h into a new file access/rmgr_internal.h.  I clearly did not waste
timethinking of a clever file name.  Bikeshedding welcome.  Most files which currently include xlog_internal.h do not
needthe definition of RmgrData.  As it stands now, inclusion of xlog_internal.h indirectly includes the following
headers:
>>
>> After refactoring, the inclusion of xlog_internal.h includes indirectly only these headers:
>>
>> and only these files need to be altered to include the new rmgr_internal.h header:
>>
>>    src/backend/access/transam/rmgr.c
>>    src/backend/access/transam/xlog.c
>>    src/backend/utils/misc/guc.c
>>
>> Thoughts?
>
> It's not clear why the correct direction here is to make
> xlog_internals.h less "low level" by moving things into headers like
> rmgr_internal.h, rather than moving the widely used parts of
> xlog_internal.h elsewhere.

Thanks for reviewing!

>> A small portion of access/xlog_internal.h defines the RmgrData struct,
>> and in support of this struct the header includes a number of other
>> headers.  Files that include access/xlog_internal.h indirectly include
>> these other headers, which most do not need. (Only 3 out of 41 files
>> involved actually need that portion of the header.)  For third-party
>> tools which deal with backup, restore, or replication matters,
>> including xlog_internal.h is necessary to get macros for calculating
>> xlog file names, but doing so also indirectly pulls in other headers,
>> increasing the risk of unwanted symbol collisions.  Some colleagues
>> and I ran into this exact problem in a C++ program that uses both
>> xlog_internal.h and the Boost C++ library.
>
> It seems better to me to just use forward declarations for StringInfo
> and XLogReaderState (and just generally use them mroe aggressively). We
> don't need the functions for dealing with those datatypes here.

Yeah, those are good points.  Please find attached version 2 of the patch set which preserves the cleanup of the first
version's0001 patch, and introduces two new patches, 0002 and 0003: 

0002 - Moves commonly used stuff from xlog_internal.h into other headers

0003 - Uses forward declarations for StringInfo and XLogReaderState so as to not need to include lib/stringinfo.h nor
access/xlogreader.hfrom access/xlog_internal.h 


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company