Thread: Re: 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
Attachment
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
Attachment
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
Attachment
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