From: | Michael Paquier <michael(at)paquier(dot)xyz> |
---|---|
To: | Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> |
Cc: | Steven Niu <niushiji(at)gmail(dot)com>, Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: relrewrite not documented at the top of heap_create_with_catalog() |
Date: | 2025-06-18 02:12:48 |
Message-ID: | aFIgoOVW_nwuG4q4@paquier.xyz |
Views: | Whole Thread | Raw Message | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Fujii Masao | 2025-06-18 02:21:35 | Re: pg_dump misses comments on NOT NULL constraints |
Previous Message | Matt Smith (matts3) | 2025-06-18 01:11:44 | Re: [PATCH] Add an ldflags_sl meson build option |