> 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