Обсуждение: Remove extra includes of "access/xloginsert.h" when "access/xlog.h" is included

Hi,

It seems like there are some instances where xloginsert.h is included
right after xlog.h but xlog.h has already included xloginsert.h.
Unless I'm missing something badly, we can safely remove including
xloginsert.h after xlog.h. Attempting to post a patch to remove the
extra xloginsert.h includes.

Thoughts?

Regards,
Bharath Rupireddy.

On 2022-Jan-28, Bharath Rupireddy wrote:

> Hi,
> 
> It seems like there are some instances where xloginsert.h is included
> right after xlog.h but xlog.h has already included xloginsert.h.
> Unless I'm missing something badly, we can safely remove including
> xloginsert.h after xlog.h. Attempting to post a patch to remove the
> extra xloginsert.h includes.

Why isn't it better to remove the line that includes xloginsert.h in
xlog.h instead?  When xloginsert.h was introduced (commit 2076db2aea76),
XLogRecData was put there so xloginsert.h was necessary for xlog.h; but
now we have a forward declaration (per commit 2c03216d8311) so it
doesn't seem needed anymore.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/

On Fri, Jan 28, 2022 at 9:25 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2022-Jan-28, Bharath Rupireddy wrote:
>
> > Hi,
> >
> > It seems like there are some instances where xloginsert.h is included
> > right after xlog.h but xlog.h has already included xloginsert.h.
> > Unless I'm missing something badly, we can safely remove including
> > xloginsert.h after xlog.h. Attempting to post a patch to remove the
> > extra xloginsert.h includes.
>
> Why isn't it better to remove the line that includes xloginsert.h in
> xlog.h instead?  When xloginsert.h was introduced (commit 2076db2aea76),
> XLogRecData was put there so xloginsert.h was necessary for xlog.h; but
> now we have a forward declaration (per commit 2c03216d8311) so it
> doesn't seem needed anymore.

Removing the xloginsert.h in xlog.h would need us to add xloginsert.h
in more areas. And also, it might break any non-core extensions that
includes just xlog.h and gets xloginsert.h. Instead I prefer removing
xloginsert.h if there's xlog.h included.

Attaching v2 patch removing xloginsert.h in a few more places.

Regards,
Bharath Rupireddy.

On 2022-Jan-29, Bharath Rupireddy wrote:

> Removing the xloginsert.h in xlog.h would need us to add xloginsert.h
> in more areas.

Sure.

> And also, it might break any non-core extensions that
> includes just xlog.h and gets xloginsert.h.

That's a pretty easy fix anyway -- it's not even version-specific, since
the fix would work with the older versions.  It's not something that
would break on a minor version, either.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"The problem with the facetime model is not just that it's demoralizing, but
that the people pretending to work interrupt the ones actually working."
                                                           (Paul Graham)

On Sun, Jan 30, 2022 at 1:07 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2022-Jan-29, Bharath Rupireddy wrote:
>
> > Removing the xloginsert.h in xlog.h would need us to add xloginsert.h
> > in more areas.
>
> Sure.
>
> > And also, it might break any non-core extensions that
> > includes just xlog.h and gets xloginsert.h.
>
> That's a pretty easy fix anyway -- it's not even version-specific, since
> the fix would work with the older versions.  It's not something that
> would break on a minor version, either.

Here's the v3 patch removing xloginsert.h from xlog.h and adding
xloginsert.h in the required files.

Regards,
Bharath Rupireddy.

Hi,

On Sun, Jan 30, 2022 at 06:52:48PM +0530, Bharath Rupireddy wrote:
> 
> Here's the v3 patch removing xloginsert.h from xlog.h and adding
> xloginsert.h in the required files.

+1, this approach is better.  In general it's better to increase the number of
include lines rather than having a few that includes everything, for
compilation performance.

On 2022-Jan-30, Bharath Rupireddy wrote:

> Here's the v3 patch removing xloginsert.h from xlog.h and adding
> xloginsert.h in the required files.

Pushed, but I added xloginsert.h to 9 additional files that needed it to
avoid compiler warnings.  Thanks!

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/