moving some code out of explain.c

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

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

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

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/

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

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

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

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

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

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

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

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