Обсуждение: Fix wrong reference in pg_overexplain's doc
Hi, pg_overexplain's documentation mentions that the definition of RangeTblEntry is in nodes/plannodes.h, which seems to be wrong as it is defined in nodes/parsenodes.h. Please find a small patch fixing this. Regards, -- Julien Tachoires
Вложения
LGTM The RangeTblEntry struct is indeed defined in parsenodes.h, not plannodes.h. This patch corrects the incorrect reference. All tests pass with make check-world. Regards, Shixin Wang
On Thu, Dec 18, 2025 at 6:23 PM Julien Tachoires <julien@tachoires.me> wrote: > > Hi, > > pg_overexplain's documentation mentions that the definition of > RangeTblEntry is in nodes/plannodes.h, which seems to be wrong as it > is defined in nodes/parsenodes.h. Please find a small patch fixing this. Thanks for the patch! It looks good to me. Barring any objections, I'll commit it and backpatch to v18, where pg_overexplain was introduced. Regards, -- Fujii Masao
On Fri, Dec 19, 2025 at 12:16 PM Fujii Masao <masao.fujii@gmail.com> wrote: > > On Thu, Dec 18, 2025 at 6:23 PM Julien Tachoires <julien@tachoires.me> wrote: > > > > Hi, > > > > pg_overexplain's documentation mentions that the definition of > > RangeTblEntry is in nodes/plannodes.h, which seems to be wrong as it > > is defined in nodes/parsenodes.h. Please find a small patch fixing this. > > Thanks for the patch! It looks good to me. > > Barring any objections, I'll commit it and backpatch to v18, > where pg_overexplain was introduced. I've pushed the patch. Thanks! After that, I noticed that the pg_overexplain docs uses the <literal> tag for file names, struct names, and commands. It would be more appropriate to use <filename>, <structname>, and <command> instead. I've attached a patch that updates the documentation accordingly. Thoughts? Regards, -- Fujii Masao
Вложения
Hi Fujii-san
> It would be more appropriate to use
> <filename>, <structname>, and <command> instead.
```
- following fields. See <literal>PlannedStmt</literal> in
- <literal>nodes/plannodes.h</literal> for additional detail.
+ following fields. See <structname>PlannedStmt</structname> in
+ <filename>nodes/plannodes.h</filename> for additional detail.
```
Switching to <filename> here makes sense. While looking through the SGML files,
I noticed that the way header file paths are written is a bit inconsistent across the documentation.
In many places we use full paths including src/include, for example in fdw_handler.sgml and create_type.sgml:
```
in <filename>src/include/nodes/plannodes.h</filename>, and the comments for
<type>ExecRowMark</type> in <filename>src/include/nodes/execnodes.h</filename> for
```
But in a few files, such as pgoverexplain.sgml (this patch), spi.sgml,
and xfunc.sgml, the paths are written without the src/include/ prefix.
I’m fine with the change as-is; just wanted to ask whether you’d like to use
this patch to address that inconsistency, or keep the existing style in this file.
Regards,
Shixin Wang.
On Tue, Dec 23, 2025 at 10:25 AM Shixin Wang <wang-shi-xin@outlook.com> wrote: > > Hi Fujii-san > > > It would be more appropriate to use > > <filename>, <structname>, and <command> instead. > > ``` > - following fields. See <literal>PlannedStmt</literal> in > - <literal>nodes/plannodes.h</literal> for additional detail. > + following fields. See <structname>PlannedStmt</structname> in > + <filename>nodes/plannodes.h</filename> for additional detail. > ``` > > Switching to <filename> here makes sense. While looking through the SGML files, Thanks for the review! > I noticed that the way header file paths are written is a bit inconsistent across the documentation. > > In many places we use full paths including src/include, for example in fdw_handler.sgml and create_type.sgml: > > ``` > in <filename>src/include/nodes/plannodes.h</filename>, and the comments for > <type>ExecRowMark</type> in <filename>src/include/nodes/execnodes.h</filename> for > ``` > > But in a few files, such as pgoverexplain.sgml (this patch), spi.sgml, > and xfunc.sgml, the paths are written without the src/include/ prefix. Yes, and you can see similar inconsistencies elsewhere as well. For example, paths to C source files are written inconsistently: they usually start with src/backend or contrib, but some entries don't follow that convention. > I’m fine with the change as-is; just wanted to ask whether you’d like to use > this patch to address that inconsistency, or keep the existing style in this file. At the moment, I don't plan to update the patch to address those inconsistencies... Regards, -- Fujii Masao
> On Dec 23, 2025, at 16:41, Fujii Masao <masao.fujii@gmail.com> wrote: > > On Tue, Dec 23, 2025 at 10:25 AM Shixin Wang <wang-shi-xin@outlook.com> wrote: >> >> Hi Fujii-san >> >>> It would be more appropriate to use >>> <filename>, <structname>, and <command> instead. >> >> ``` >> - following fields. See <literal>PlannedStmt</literal> in >> - <literal>nodes/plannodes.h</literal> for additional detail. >> + following fields. See <structname>PlannedStmt</structname> in >> + <filename>nodes/plannodes.h</filename> for additional detail. >> ``` >> >> Switching to <filename> here makes sense. While looking through the SGML files, > > Thanks for the review! > > >> I noticed that the way header file paths are written is a bit inconsistent across the documentation. >> >> In many places we use full paths including src/include, for example in fdw_handler.sgml and create_type.sgml: >> >> ``` >> in <filename>src/include/nodes/plannodes.h</filename>, and the comments for >> <type>ExecRowMark</type> in <filename>src/include/nodes/execnodes.h</filename> for >> ``` >> >> But in a few files, such as pgoverexplain.sgml (this patch), spi.sgml, >> and xfunc.sgml, the paths are written without the src/include/ prefix. > > Yes, and you can see similar inconsistencies elsewhere as well. For example, > paths to C source files are written inconsistently: they usually start with > src/backend or contrib, but some entries don't follow that convention. > > >> I’m fine with the change as-is; just wanted to ask whether you’d like to use >> this patch to address that inconsistency, or keep the existing style in this file. > > At the moment, I don't plan to update the patch to address those > inconsistencies... > The patch itself looks good to me. Following Shixin’s comment, I do see the inconsistencies of full path and relative pathof .h files are referenced, if you don’t plan to address the inconvenience, I may file a patch for that. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
On Wed, Dec 24, 2025 at 8:54 AM Chao Li <li.evan.chao@gmail.com> wrote: > The patch itself looks good to me. Thanks for the review! I've pushed the patch. > Following Shixin’s comment, I do see the inconsistencies of full path and relative path of .h files are referenced, ifyou don’t plan to address the inconvenience, I may file a patch for that. Please feel free to work on that! Regards, -- Fujii Masao