> On Oct 19, 2020, at 7:05 PM, Andres Freund <and...@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 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
