From: | Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com> |
---|---|
To: | Andres Freund <andres(at)anarazel(dot)de> |
Cc: | PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Reduce the dependence on access/xlog_internal.h |
Date: | 2020-10-21 02:25:11 |
Message-ID: | 1CFC971A-6240-4292-B9D9-BFC134D0B277@enterprisedb.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
> On Oct 19, 2020, at 7:05 PM, Andres Freund <andres(at)anarazel(dot)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
Attachment | Content-Type | Size |
---|---|---|
v2-0001-Removing-gratuitous-inclusion-of-xlog_internal.h.patch | application/octet-stream | 4.0 KB |
v2-0002-Refactoring-access-xlog_internal.h.patch | application/octet-stream | 30.4 KB |
v2-0003-Using-forward-declarations-to-reduce-including-he.patch | application/octet-stream | 1.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Masahiko Sawada | 2020-10-21 02:44:47 | Re: Track statistics for streaming of in-progress transactions |
Previous Message | Robert Haas | 2020-10-21 01:54:31 | Re: [PATCH] SET search_path += octopus |