relrewrite not documented at the top of heap_create_with_catalog()
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
Attachments:
heap-relrewrite.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index fbaed5359ad7..77b6f02ca95d 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1100,6 +1100,7 @@ AddNewRelationType(const char *typeName,
* if false, relacl is always set NULL
* allow_system_table_mods: true to allow creation in system namespaces
* is_internal: is this a system-generated catalog?
+ * relrewrite: link to original relation during a table rewrite.
*
* Output parameters:
* typaddress: if not null, gets the object address of the new pg_type entry
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写道：
Show quoted text
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
Attachments:
0001-Improve-the-comment-of-heap_create_with_catalog.patchapplication/octet-stream; name=0001-Improve-the-comment-of-heap_create_with_catalog.patchDownload
From 6a0c84a7360ae94af84dd744f18cbd703f5a7e47 Mon Sep 17 00:00:00 2001
From: Steven Niu <niushiji@highgo.com>
Date: Mon, 16 Jun 2025 16:47:02 +0800
Subject: Improve the comment of heap_create_with_catalog()
Combine the old comment and new comment.
Add decription for relrewrite parameter.
---
src/backend/catalog/heap.c | 63 ++++++++++++++++++--------------------
1 file changed, 30 insertions(+), 33 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index fbaed5359ad..4b90f4aef35 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -406,36 +406,6 @@ heap_create(const char *relname,
return rel;
}
-/* ----------------------------------------------------------------
- * heap_create_with_catalog - Create a cataloged relation
- *
- * this is done in multiple steps:
- *
- * 1) CheckAttributeNamesTypes() is used to make certain the tuple
- * descriptor contains a valid set of attribute names and types
- *
- * 2) pg_class is opened and get_relname_relid()
- * performs a scan to ensure that no relation with the
- * same name already exists.
- *
- * 3) heap_create() is called to create the new relation on disk.
- *
- * 4) TypeCreate() is called to define a new type corresponding
- * to the new relation.
- *
- * 5) AddNewRelationTuple() is called to register the
- * relation in pg_class.
- *
- * 6) AddNewAttributeTuples() is called to register the
- * new relation's schema in pg_attribute.
- *
- * 7) StoreConstraints() is called - vadim 08/22/97
- *
- * 8) the relations are closed and the new relation's oid
- * is returned.
- *
- * ----------------------------------------------------------------
- */
/* --------------------------------
* CheckAttributeNamesTypes
@@ -1074,10 +1044,36 @@ AddNewRelationType(const char *typeName,
InvalidOid); /* rowtypes never have a collation */
}
-/* --------------------------------
- * heap_create_with_catalog
+/* ----------------------------------------------------------------
+ * heap_create_with_catalog
+ *
+ * Description:
+ * Create a cataloged relation.
+ *
+ * this is done in multiple steps:
+ *
+ * 1) CheckAttributeNamesTypes() is used to make certain the tuple
+ * descriptor contains a valid set of attribute names and types
+ *
+ * 2) pg_class is opened and get_relname_relid()
+ * performs a scan to ensure that no relation with the
+ * same name already exists.
+ *
+ * 3) heap_create() is called to create the new relation on disk.
+ *
+ * 4) TypeCreate() is called to define a new type corresponding
+ * to the new relation.
+ *
+ * 5) AddNewRelationTuple() is called to register the
+ * relation in pg_class.
+ *
+ * 6) AddNewAttributeTuples() is called to register the
+ * new relation's schema in pg_attribute.
+ *
+ * 7) StoreConstraints() is called - vadim 08/22/97
*
- * creates a new cataloged relation. see comments above.
+ * 8) the relations are closed and the new relation's oid
+ * is returned.
*
* Arguments:
* relname: name to give to new rel
@@ -1100,6 +1096,7 @@ AddNewRelationType(const char *typeName,
* if false, relacl is always set NULL
* allow_system_table_mods: true to allow creation in system namespaces
* is_internal: is this a system-generated catalog?
+ * relrewrite: link to original relation during a table rewrite.
*
* Output parameters:
* typaddress: if not null, gets the object address of the new pg_type entry
--
2.43.0
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,
StevenMichael 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>