> On Oct 19, 2020, at 7:05 PM, Andres Freund 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 time thinking of a clever
>> file name. Bikeshedding welcome. Most files which currently include
>> xlog_internal.h do not need the 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's 0001 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.h from
access/xlog_internal.h
v2-0001-Removing-gratuitous-inclusion-of-xlog_internal.h.patch
Description: Binary data
v2-0002-Refactoring-access-xlog_internal.h.patch
Description: Binary data
v2-0003-Using-forward-declarations-to-reduce-including-he.patch
Description: Binary data
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
