relrewrite not documented at the top of heap_create_with_catalog()

Hi, Michael,

The change looks good to me.

I made more change based on your patch to combine the old comment.

Regards,
Steven

Michael Paquier <michael@paquier.xyz> 于2025年6月16日周一 14:49写道：

On Mon, 16 Jun 2025 16:51:46 +0800
Steven Niu <niushiji@gmail.com> wrote:

Hi, Michael,

The change looks good to me.

I made more change based on your patch to combine the old comment.

I think it's a good idea to move the description of heap_create_with_catalog
directly above the function itself, as it seems better to keep the explanation
close to the function definition rather than placing it before related functions.
A similar change was made to heap_drop_with_catalog in commit 49ce6fff1d34.

I'm not sure whether this should be merged into the original patch, though.

I have a very minor comment on the initial patch.

+ * relrewrite: link to original relation during a table rewrite.

I wonder if the period at the end is unnecessary, since this is not a full
sentence, and for consistency with the other lines.

Best regards,
Yugo Nagata

Regards,
Steven

Michael Paquier <michael@paquier.xyz> 于2025年6月16日周一 14:49写道：

Hi all,

While looking at the heap code, I've found that relrewrite, parameter
used a trick to keep a link to the original relation rewritten, is not
documented at the top of heap_create_with_catalog() contrary to all
its other parameters.

A simple patch is attached to document that.

Thanks,
--
Michael

--
Yugo Nagata <nagata@sraoss.co.jp>

On Mon, Jun 16, 2025 at 10:37:43PM +0900, Yugo Nagata wrote:

I think it's a good idea to move the description of heap_create_with_catalog
directly above the function itself, as it seems better to keep the explanation
close to the function definition rather than placing it before related functions.
A similar change was made to heap_drop_with_catalog in commit 49ce6fff1d34.

I'm not sure whether this should be merged into the original patch, though.

More to the point: the whole comment block feels incomplete. It is
missing for example a couple of steps, like between 2) and 3) due to
the introduction of binary upgrade logic. The logic of the function
is much better guessed while reading through the code, IMO, so I would
suggest to just remove the whole comment block, with the part about
the "see comments above." at the top of the routine definition as we
are proving to not be good in maintaining it. There is perhaps a bit
more that could be cleaned up; that's some legacy code back from the
original Postgres95.

+ * relrewrite: link to original relation during a table rewrite.

I wonder if the period at the end is unnecessary, since this is not a full
sentence, and for consistency with the other lines.

On consistency grounds, that makes sense. Removed it and applied this
comment fix.
--
Michael

On Wed, 18 Jun 2025 11:12:48 +0900
Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Jun 16, 2025 at 10:37:43PM +0900, Yugo Nagata wrote:

I think it's a good idea to move the description of heap_create_with_catalog
directly above the function itself, as it seems better to keep the explanation
close to the function definition rather than placing it before related functions.
A similar change was made to heap_drop_with_catalog in commit 49ce6fff1d34.

I'm not sure whether this should be merged into the original patch, though.

More to the point: the whole comment block feels incomplete. It is
missing for example a couple of steps, like between 2) and 3) due to
the introduction of binary upgrade logic. The logic of the function
is much better guessed while reading through the code, IMO, so I would
suggest to just remove the whole comment block, with the part about
the "see comments above." at the top of the routine definition as we
are proving to not be good in maintaining it. There is perhaps a bit
more that could be cleaned up; that's some legacy code back from the
original Postgres95.

I agree that the comment block should be removed, as it doesn't seem
so useful. I previously mentioned commit 49ce6fff1d34, which moved the comments
on heap_drop_with_catalog, but I also noticed that the step-by-step overview had
already been removed in commit 448eb0837f7a8, due to changes in the function’s logic.

Best regards,
Yugo Nagata

--
Yugo Nagata <nagata@sraoss.co.jp>