Horton

Thread

Commits

  1. Avoid including explain.h in explain_format.h and explain_dr.h

  2. Create explain_dr.c and move DestReceiver-related code there.

  3. Create explain_format.c and move relevant code there.

  1. moving some code out of explain.c

    Robert Haas <robertmhaas@gmail.com> — 2025-02-18T18:04:03Z

    While contemplating some additions to explain.c, I noticed that this
file has become rather large -- 5924 lines on master. I'm generally
not a fan of very large source files, and I think it would be a good
idea to start splitting this one up before things get completely out
of control. Here are a few patches to move some chunks of code to
separate files (explain_format.c and explain_dr.c).

I think in cases like this there is a tendency to want to achieve an
even split of the original file, but in practice I think that tends
not to be difficult to achieve. These two patches combined move about
1000 lines of code into separate files, which I think is actually
quite a good result for a refactoring of this sort. We might want to
split it up even more, but I don't feel like that has to be done in
the first set of patches or that all such patches have to come from
me. So I propose to do just this much for now.

Comments?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  2. Re: moving some code out of explain.c

    Peter Geoghegan <pg@bowt.ie> — 2025-02-18T18:11:23Z

    On Tue, Feb 18, 2025 at 1:04 PM Robert Haas <robertmhaas@gmail.com> wrote:
> I think in cases like this there is a tendency to want to achieve an
> even split of the original file, but in practice I think that tends
> not to be difficult to achieve. These two patches combined move about
> 1000 lines of code into separate files, which I think is actually
> quite a good result for a refactoring of this sort. We might want to
> split it up even more, but I don't feel like that has to be done in
> the first set of patches or that all such patches have to come from
> me. So I propose to do just this much for now.
>
> Comments?

Seems like a good idea to me.

-- 
Peter Geoghegan

  3. Re: moving some code out of explain.c

    Robert Haas <robertmhaas@gmail.com> — 2025-02-27T18:24:20Z

    On Tue, Feb 18, 2025 at 1:11 PM Peter Geoghegan <pg@bowt.ie> wrote:
> On Tue, Feb 18, 2025 at 1:04 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > I think in cases like this there is a tendency to want to achieve an
> > even split of the original file, but in practice I think that tends
> > not to be difficult to achieve. These two patches combined move about
> > 1000 lines of code into separate files, which I think is actually
> > quite a good result for a refactoring of this sort. We might want to
> > split it up even more, but I don't feel like that has to be done in
> > the first set of patches or that all such patches have to come from
> > me. So I propose to do just this much for now.
> >
> > Comments?
>
> Seems like a good idea to me.

Thanks for the quick response! I have committed these patches.

I see that the Redis-FDW test is failing; it will now need to include
"commands/explain_format.h" -- unless we  something, of course.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  4. Re: moving some code out of explain.c

    Peter Geoghegan <pg@bowt.ie> — 2025-02-27T18:31:00Z

    On Thu, Feb 27, 2025 at 1:24 PM Robert Haas <robertmhaas@gmail.com> wrote:
> Thanks for the quick response! I have committed these patches.

I recently did something similar myself when I moved all of the nbtree
preprocessing code into its own file.

The obvious downside is that it tends to make "git blame" much less
useful. It was definitely worth it, though.

-- 
Peter Geoghegan

  5. Re: moving some code out of explain.c

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-27T19:21:52Z

    On 2025-Feb-27, Robert Haas wrote:

> I see that the Redis-FDW test is failing; it will now need to include
> "commands/explain_format.h" -- unless we  something, of course.

I wonder if it was really a useful idea to move the declarations of
those functions from explain.h to the new explain_format.h file.  It
seems that this new file now has a bunch of declarations that have
always been something of a public interface, together with others that
are only of internal explain.c interest, such as
ExplainOpenSetAsideGroup() and friends.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

  6. Re: moving some code out of explain.c

    Robert Haas <robertmhaas@gmail.com> — 2025-02-27T19:41:05Z

    On Thu, Feb 27, 2025 at 2:21 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2025-Feb-27, Robert Haas wrote:
> > I see that the Redis-FDW test is failing; it will now need to include
> > "commands/explain_format.h" -- unless we  something, of course.
>
> I wonder if it was really a useful idea to move the declarations of
> those functions from explain.h to the new explain_format.h file.  It
> seems that this new file now has a bunch of declarations that have
> always been something of a public interface, together with others that
> are only of internal explain.c interest, such as
> ExplainOpenSetAsideGroup() and friends.

I'm not completely certain about what I did with the header files, but
for a somewhat different reason than what you mention here.

I don't see ExplainOpenSetAsideGroup() as being any different from
anything else that I put in explain_format.h -- it's something that
code might want to call if it's generating EXPLAIN output, and
otherwise not. But maybe I'm missing something -- what makes you see
it as different from the other stuff?

The thing that was bugging me a bit is that explain_format.h includes
explain.h. It would be nice if those were more independent of each
other, but I'm not sure if there's a reasonably clean way of
accomplishing that. When deciding what to do there, it's also worth
keeping in mind that there may be more opportunities to move stuff out
of explain.c, so we might not want to get too dogmatic about the
header file organization just yet. But, I'm certainly open to ideas.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  7. Re: moving some code out of explain.c

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-27T20:05:54Z

    Robert Haas <robertmhaas@gmail.com> writes:
> The thing that was bugging me a bit is that explain_format.h includes
> explain.h.

Yeah, I did not like that at all either -- it makes things a bit
circular, and I fear IWYU is going to make stupid recommendations
like not including explain.h in explain.c.

Did you look at avoiding that with our standard trick of writing
detail-free struct declarations?  That is, explain_format.h
would need

struct ExplainState;		/* avoid including explain.h here */

and then s/ExplainState/struct ExplainState/ in all the function
declarations.  You'd still need to get List from someplace, but
it could be gotten by including primnodes.h or even pg_list.h.

			regards, tom lane

  8. Re: moving some code out of explain.c

    Robert Haas <robertmhaas@gmail.com> — 2025-02-27T21:07:05Z

    On Thu, Feb 27, 2025 at 3:05 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > The thing that was bugging me a bit is that explain_format.h includes
> > explain.h.
>
> Yeah, I did not like that at all either -- it makes things a bit
> circular, and I fear IWYU is going to make stupid recommendations
> like not including explain.h in explain.c.
>
> Did you look at avoiding that with our standard trick of writing
> detail-free struct declarations?  That is, explain_format.h
> would need
>
> struct ExplainState;            /* avoid including explain.h here */
>
> and then s/ExplainState/struct ExplainState/ in all the function
> declarations.  You'd still need to get List from someplace, but
> it could be gotten by including primnodes.h or even pg_list.h.

OK, here's a patch that does that. Thoughts?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  9. Re: moving some code out of explain.c

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-27T21:12:05Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Feb 27, 2025 at 3:05 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Did you look at avoiding that with our standard trick of writing
>> detail-free struct declarations?

> OK, here's a patch that does that. Thoughts?

+1, but how about explain_dr.h too?  It doesn't seem though that
we can avoid #include "executor/instrument.h" there, so it'd be
a little asymmetrical.  But the executor inclusion doesn't seem
nearly as much almost-circular.

			regards, tom lane

  10. Re: moving some code out of explain.c

    Robert Haas <robertmhaas@gmail.com> — 2025-02-27T21:25:39Z

    On Thu, Feb 27, 2025 at 4:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> +1, but how about explain_dr.h too?  It doesn't seem though that
> we can avoid #include "executor/instrument.h" there, so it'd be
> a little asymmetrical.  But the executor inclusion doesn't seem
> nearly as much almost-circular.

OK, here is v2. One slightly unfortunate thing about this is that we
end up with a line that is over 80 characters:

extern DestReceiver *CreateExplainSerializeDestReceiver(struct
ExplainState *es);

Aside from perhaps shortening the function name, I don't see how to avoid that.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  11. Re: moving some code out of explain.c

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-27T21:48:01Z

    Robert Haas <robertmhaas@gmail.com> writes:
> OK, here is v2. One slightly unfortunate thing about this is that we
> end up with a line that is over 80 characters:

> extern DestReceiver *CreateExplainSerializeDestReceiver(struct
> ExplainState *es);

> Aside from perhaps shortening the function name, I don't see how to avoid that.

Can't get terribly excited about that.  But speaking of
CreateExplainSerializeDestReceiver, I see that a declaration
of it is still there at the bottom of explain.h:

    extern DestReceiver *CreateExplainSerializeDestReceiver(ExplainState *es);

    #endif							/* EXPLAIN_H */

Oversight I assume?

			regards, tom lane

  12. Re: moving some code out of explain.c

    Robert Haas <robertmhaas@gmail.com> — 2025-02-28T14:52:24Z

    On Thu, Feb 27, 2025 at 4:48 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Oversight I assume?

Yeah, that was dumb. Thanks for catching it. Here's v3.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  13. Re: moving some code out of explain.c

    Robert Haas <robertmhaas@gmail.com> — 2025-02-28T18:28:22Z

    On Fri, Feb 28, 2025 at 9:52 AM Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Feb 27, 2025 at 4:48 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Oversight I assume?
>
> Yeah, that was dumb. Thanks for catching it. Here's v3.

Committed so I can see what the buildfarm thinks before it gets too
late in the day.

-- 
Robert Haas
EDB: http://www.enterprisedb.com