[PATCH v1] eliminate duplicate code in table.c
Started by Junwang Zhaoover 3 years ago18 messages
There are some duplicate code in table.c, add a static inline function
to eliminate the duplicates.
--
Regards
Junwang Zhao
Attachments:
0001-eliminate-duplicate-code-in-table.c.patchapplication/octet-stream; name=0001-eliminate-duplicate-code-in-table.c.patchDownload
From 700ed59039a2a3fb8b6c4c4325a038d1929a42ad Mon Sep 17 00:00:00 2001
From: Junwang Zhao <zhjwpku@gmail.com>
Date: Thu, 21 Jul 2022 16:22:17 +0800
Subject: [PATCH v1] eliminate duplicate code in table.c
Signed-off-by: Junwang Zhao <zhjwpku@gmail.com>
---
src/backend/access/table/table.c | 74 ++++++++++++--------------------
1 file changed, 28 insertions(+), 46 deletions(-)
diff --git a/src/backend/access/table/table.c b/src/backend/access/table/table.c
index 744d3b550b..9f096a1e95 100644
--- a/src/backend/access/table/table.c
+++ b/src/backend/access/table/table.c
@@ -25,6 +25,7 @@
#include "access/table.h"
#include "storage/lmgr.h"
+static inline void check_wrong_object_type(Relation r);
/* ----------------
* table_open - open a table relation by relation OID
@@ -42,22 +43,11 @@ table_open(Oid relationId, LOCKMODE lockmode)
r = relation_open(relationId, lockmode);
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ check_wrong_object_type(r);
return r;
}
-
/* ----------------
* try_table_open - open a table relation by relation OID
*
@@ -76,17 +66,7 @@ try_table_open(Oid relationId, LOCKMODE lockmode)
if (!r)
return NULL;
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ check_wrong_object_type(r);
return r;
}
@@ -105,17 +85,7 @@ table_openrv(const RangeVar *relation, LOCKMODE lockmode)
r = relation_openrv(relation, lockmode);
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ check_wrong_object_type(r);
return r;
}
@@ -138,17 +108,7 @@ table_openrv_extended(const RangeVar *relation, LOCKMODE lockmode,
if (r)
{
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ check_wrong_object_type(r);
}
return r;
@@ -161,10 +121,32 @@ table_openrv_extended(const RangeVar *relation, LOCKMODE lockmode,
*
* Note that it is often sensible to hold a lock beyond relation_close;
* in that case, the lock is released automatically at xact end.
- * ----------------
+ * ----------------
*/
void
table_close(Relation relation, LOCKMODE lockmode)
{
relation_close(relation, lockmode);
}
+
+/* ----------------
+ * check_wrong_object_type - check the relation's kind
+ *
+ * Make sure relkind is not index or composite type
+ * ----------------
+ */
+static inline void
+check_wrong_object_type(Relation r)
+{
+ if (r->rd_rel->relkind == RELKIND_INDEX ||
+ r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("\"%s\" is an index",
+ RelationGetRelationName(r))));
+ else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("\"%s\" is a composite type",
+ RelationGetRelationName(r))));
+}
--
2.33.0
Re: [PATCH v1] eliminate duplicate code in table.c
On Thu, Jul 21, 2022 at 1:56 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
There are some duplicate code in table.c, add a static inline function
to eliminate the duplicates.
Can we name function as validate_object_type, or check_object_type?
Otherwise, the patch looks fine to me. Let's see if others have
something to say.
--
With Regards,
Amit Kapila.
Re: [PATCH v1] eliminate duplicate code in table.c
Hi hackers,
There are some duplicate code in table.c, add a static inline function
to eliminate the duplicates.Can we name function as validate_object_type, or check_object_type?
Otherwise, the patch looks fine to me. Let's see if others have
something to say.
LGTM
--
Best regards,
Aleksander Alekseev
Attachments:
v2-0001-Eliminate-duplicate-code-in-table.c.patchapplication/octet-stream; name=v2-0001-Eliminate-duplicate-code-in-table.c.patchDownload
From 148740118b24096dd18e606a34ff2895e4c1ac82 Mon Sep 17 00:00:00 2001
From: Junwang Zhao <zhjwpku@gmail.com>
Date: Thu, 21 Jul 2022 16:22:17 +0800
Subject: [PATCH v2] Eliminate duplicate code in table.c
Author: Junwang Zhao <zhjwpku@gmail.com>
---
src/backend/access/table/table.c | 74 ++++++++++++--------------------
1 file changed, 28 insertions(+), 46 deletions(-)
diff --git a/src/backend/access/table/table.c b/src/backend/access/table/table.c
index 744d3b550b..9140a824d7 100644
--- a/src/backend/access/table/table.c
+++ b/src/backend/access/table/table.c
@@ -25,6 +25,7 @@
#include "access/table.h"
#include "storage/lmgr.h"
+static inline void validate_object_type(Relation r);
/* ----------------
* table_open - open a table relation by relation OID
@@ -42,22 +43,11 @@ table_open(Oid relationId, LOCKMODE lockmode)
r = relation_open(relationId, lockmode);
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ validate_object_type(r);
return r;
}
-
/* ----------------
* try_table_open - open a table relation by relation OID
*
@@ -76,17 +66,7 @@ try_table_open(Oid relationId, LOCKMODE lockmode)
if (!r)
return NULL;
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ validate_object_type(r);
return r;
}
@@ -105,17 +85,7 @@ table_openrv(const RangeVar *relation, LOCKMODE lockmode)
r = relation_openrv(relation, lockmode);
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ validate_object_type(r);
return r;
}
@@ -138,17 +108,7 @@ table_openrv_extended(const RangeVar *relation, LOCKMODE lockmode,
if (r)
{
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ validate_object_type(r);
}
return r;
@@ -161,10 +121,32 @@ table_openrv_extended(const RangeVar *relation, LOCKMODE lockmode,
*
* Note that it is often sensible to hold a lock beyond relation_close;
* in that case, the lock is released automatically at xact end.
- * ----------------
+ * ----------------
*/
void
table_close(Relation relation, LOCKMODE lockmode)
{
relation_close(relation, lockmode);
}
+
+/* ----------------
+ * validate_object_type - check the relation's kind
+ *
+ * Make sure relkind is not index or composite type
+ * ----------------
+ */
+static inline void
+validate_object_type(Relation r)
+{
+ if (r->rd_rel->relkind == RELKIND_INDEX ||
+ r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("\"%s\" is an index",
+ RelationGetRelationName(r))));
+ else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("\"%s\" is a composite type",
+ RelationGetRelationName(r))));
+}
--
2.36.1
Re: [PATCH v1] eliminate duplicate code in table.c
On Thu, Jul 21, 2022 at 5:09 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
There are some duplicate code in table.c, add a static inline function
to eliminate the duplicates.Can we name function as validate_object_type, or check_object_type?
Otherwise, the patch looks fine to me. Let's see if others have
something to say.LGTM
@@ -161,10 +121,32 @@ table_openrv_extended(const RangeVar *relation,
LOCKMODE lockmode,
*
* Note that it is often sensible to hold a lock beyond relation_close;
* in that case, the lock is released automatically at xact end.
- * ----------------
+ * ----------------
*/
void
table_close(Relation relation, LOCKMODE lockmode)
I don't think this change should be part of this patch. Do you see a
reason for doing this?
--
With Regards,
Amit Kapila.
Re: [PATCH v1] eliminate duplicate code in table.c
Hi Amit,
I don't think this change should be part of this patch. Do you see a
reason for doing this?
My bad. I thought this was done by pgindent.
--
Best regards,
Aleksander Alekseev
Attachments:
v3-0001-Eliminate-duplicate-code-in-table.c.patchapplication/octet-stream; name=v3-0001-Eliminate-duplicate-code-in-table.c.patchDownload
From 94fc3e4ec4fb62189b5763e3a84e37f84b578733 Mon Sep 17 00:00:00 2001
From: Junwang Zhao <zhjwpku@gmail.com>
Date: Thu, 21 Jul 2022 16:22:17 +0800
Subject: [PATCH v3] Eliminate duplicate code in table.c
Author: Junwang Zhao <zhjwpku@gmail.com>
---
src/backend/access/table/table.c | 71 ++++++++++++--------------------
1 file changed, 27 insertions(+), 44 deletions(-)
diff --git a/src/backend/access/table/table.c b/src/backend/access/table/table.c
index 744d3b550b..290792ef7b 100644
--- a/src/backend/access/table/table.c
+++ b/src/backend/access/table/table.c
@@ -25,6 +25,7 @@
#include "access/table.h"
#include "storage/lmgr.h"
+static inline void validate_object_type(Relation r);
/* ----------------
* table_open - open a table relation by relation OID
@@ -42,17 +43,7 @@ table_open(Oid relationId, LOCKMODE lockmode)
r = relation_open(relationId, lockmode);
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ validate_object_type(r);
return r;
}
@@ -76,17 +67,7 @@ try_table_open(Oid relationId, LOCKMODE lockmode)
if (!r)
return NULL;
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ validate_object_type(r);
return r;
}
@@ -105,17 +86,7 @@ table_openrv(const RangeVar *relation, LOCKMODE lockmode)
r = relation_openrv(relation, lockmode);
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ validate_object_type(r);
return r;
}
@@ -138,17 +109,7 @@ table_openrv_extended(const RangeVar *relation, LOCKMODE lockmode,
if (r)
{
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ validate_object_type(r);
}
return r;
@@ -168,3 +129,25 @@ table_close(Relation relation, LOCKMODE lockmode)
{
relation_close(relation, lockmode);
}
+
+/* ----------------
+ * validate_object_type - check the relation's kind
+ *
+ * Make sure relkind is not index or composite type
+ * ----------------
+ */
+static inline void
+validate_object_type(Relation r)
+{
+ if (r->rd_rel->relkind == RELKIND_INDEX ||
+ r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("\"%s\" is an index",
+ RelationGetRelationName(r))));
+ else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("\"%s\" is a composite type",
+ RelationGetRelationName(r))));
+}
--
2.36.1
Re: [PATCH v1] eliminate duplicate code in table.c
On 2022-Jul-21, Junwang Zhao wrote:
There are some duplicate code in table.c, add a static inline function
to eliminate the duplicates.
Hmm, but see commit 2ed532ee8c47 about this kind of check. Perhaps we
should change these error messages to conform to the same message style.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"I love the Postgres community. It's all about doing things _properly_. :-)"
(David Garamond)
Re: [PATCH v1] eliminate duplicate code in table.c
Hi Alvaro,
Hmm, but see commit 2ed532ee8c47 about this kind of check. Perhaps we
should change these error messages to conform to the same message style.
Good point! Done.
--
Best regards,
Aleksander Alekseev
Attachments:
v4-0001-Eliminate-duplicate-code-in-table.c.patchapplication/octet-stream; name=v4-0001-Eliminate-duplicate-code-in-table.c.patchDownload
From 4f2f8ccbd4d8028cee54aac706725ed47413d44d Mon Sep 17 00:00:00 2001
From: Junwang Zhao <zhjwpku@gmail.com>
Date: Thu, 21 Jul 2022 16:22:17 +0800
Subject: [PATCH v4] Eliminate duplicate code in table.c
Additionally improve error messages similarly to how it was done in 2ed532ee.
Author: Junwang Zhao <zhjwpku@gmail.com>
Author: Aleksander Alekseev <aleksander@timescale.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Alvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/CAEG8a3KbVtBm_BYf5tGsKHvmMieQVsq_jBPOg75VViQB7ACL8Q%40mail.gmail.com
---
src/backend/access/table/table.c | 70 +++++++++++--------------------
src/test/regress/expected/tid.out | 3 +-
2 files changed, 26 insertions(+), 47 deletions(-)
diff --git a/src/backend/access/table/table.c b/src/backend/access/table/table.c
index 744d3b550b..93c98b4511 100644
--- a/src/backend/access/table/table.c
+++ b/src/backend/access/table/table.c
@@ -25,6 +25,7 @@
#include "access/table.h"
#include "storage/lmgr.h"
+static inline void validate_object_type(Relation r);
/* ----------------
* table_open - open a table relation by relation OID
@@ -42,17 +43,7 @@ table_open(Oid relationId, LOCKMODE lockmode)
r = relation_open(relationId, lockmode);
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ validate_object_type(r);
return r;
}
@@ -76,17 +67,7 @@ try_table_open(Oid relationId, LOCKMODE lockmode)
if (!r)
return NULL;
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ validate_object_type(r);
return r;
}
@@ -105,17 +86,7 @@ table_openrv(const RangeVar *relation, LOCKMODE lockmode)
r = relation_openrv(relation, lockmode);
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ validate_object_type(r);
return r;
}
@@ -137,19 +108,7 @@ table_openrv_extended(const RangeVar *relation, LOCKMODE lockmode,
r = relation_openrv_extended(relation, lockmode, missing_ok);
if (r)
- {
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
- }
+ validate_object_type(r);
return r;
}
@@ -168,3 +127,22 @@ table_close(Relation relation, LOCKMODE lockmode)
{
relation_close(relation, lockmode);
}
+
+/* ----------------
+ * validate_object_type - check the relation's kind
+ *
+ * Make sure relkind is not index or composite type
+ * ----------------
+ */
+static inline void
+validate_object_type(Relation r)
+{
+ if (r->rd_rel->relkind == RELKIND_INDEX ||
+ r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX ||
+ r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot operate on ralation \"%s\"",
+ RelationGetRelationName(r)),
+ errdetail_relkind_not_supported(r->rd_rel->relkind)));
+}
diff --git a/src/test/regress/expected/tid.out b/src/test/regress/expected/tid.out
index 7d8957bd6f..c11fb4f545 100644
--- a/src/test/regress/expected/tid.out
+++ b/src/test/regress/expected/tid.out
@@ -61,7 +61,8 @@ DROP SEQUENCE tid_seq;
-- Index, fails with incorrect relation type
CREATE INDEX tid_ind ON tid_tab(a);
SELECT currtid2('tid_ind'::text, '(0,1)'::tid); -- fails
-ERROR: "tid_ind" is an index
+ERROR: cannot operate on ralation "tid_ind"
+DETAIL: This operation is not supported for indexes.
DROP INDEX tid_ind;
-- Partitioned table, no storage
CREATE TABLE tid_part (a int) PARTITION BY RANGE (a);
--
2.36.1
Re: [PATCH v1] eliminate duplicate code in table.c
On Thu, Jul 21, 2022 at 6:12 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
Hi Alvaro,
Hmm, but see commit 2ed532ee8c47 about this kind of check. Perhaps we
should change these error messages to conform to the same message style.Good point! Done.
Yeah, that's better. On again thinking about the function name, I
wonder if validate_relation_type() suits here as there is no generic
object being passed?
--
With Regards,
Amit Kapila.
Re: [PATCH v1] eliminate duplicate code in table.c
Hi Amit,
Yeah, that's better. On again thinking about the function name, I
wonder if validate_relation_type() suits here as there is no generic
object being passed?
Yep, validate_relation_type() sounds better.
--
Best regards,
Aleksander Alekseev
Attachments:
v5-0001-Eliminate-duplicate-code-in-table.c.patchapplication/octet-stream; name=v5-0001-Eliminate-duplicate-code-in-table.c.patchDownload
From 97adb1066e0cc10501c7dafab250394f4a4c438f Mon Sep 17 00:00:00 2001
From: Junwang Zhao <zhjwpku@gmail.com>
Date: Thu, 21 Jul 2022 16:22:17 +0800
Subject: [PATCH v5] Eliminate duplicate code in table.c
Additionally improve error messages similarly to how it was done in 2ed532ee.
Author: Junwang Zhao <zhjwpku@gmail.com>
Author: Aleksander Alekseev <aleksander@timescale.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Alvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/CAEG8a3KbVtBm_BYf5tGsKHvmMieQVsq_jBPOg75VViQB7ACL8Q%40mail.gmail.com
---
src/backend/access/table/table.c | 70 +++++++++++--------------------
src/test/regress/expected/tid.out | 3 +-
2 files changed, 26 insertions(+), 47 deletions(-)
diff --git a/src/backend/access/table/table.c b/src/backend/access/table/table.c
index 744d3b550b..b33c73e806 100644
--- a/src/backend/access/table/table.c
+++ b/src/backend/access/table/table.c
@@ -25,6 +25,7 @@
#include "access/table.h"
#include "storage/lmgr.h"
+static inline void validate_relation_type(Relation r);
/* ----------------
* table_open - open a table relation by relation OID
@@ -42,17 +43,7 @@ table_open(Oid relationId, LOCKMODE lockmode)
r = relation_open(relationId, lockmode);
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ validate_relation_type(r);
return r;
}
@@ -76,17 +67,7 @@ try_table_open(Oid relationId, LOCKMODE lockmode)
if (!r)
return NULL;
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ validate_relation_type(r);
return r;
}
@@ -105,17 +86,7 @@ table_openrv(const RangeVar *relation, LOCKMODE lockmode)
r = relation_openrv(relation, lockmode);
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ validate_relation_type(r);
return r;
}
@@ -137,19 +108,7 @@ table_openrv_extended(const RangeVar *relation, LOCKMODE lockmode,
r = relation_openrv_extended(relation, lockmode, missing_ok);
if (r)
- {
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
- }
+ validate_relation_type(r);
return r;
}
@@ -168,3 +127,22 @@ table_close(Relation relation, LOCKMODE lockmode)
{
relation_close(relation, lockmode);
}
+
+/* ----------------
+ * validate_relation_type - check the relation's kind
+ *
+ * Make sure relkind is not index or composite type
+ * ----------------
+ */
+static inline void
+validate_relation_type(Relation r)
+{
+ if (r->rd_rel->relkind == RELKIND_INDEX ||
+ r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX ||
+ r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot operate on ralation \"%s\"",
+ RelationGetRelationName(r)),
+ errdetail_relkind_not_supported(r->rd_rel->relkind)));
+}
diff --git a/src/test/regress/expected/tid.out b/src/test/regress/expected/tid.out
index 7d8957bd6f..c11fb4f545 100644
--- a/src/test/regress/expected/tid.out
+++ b/src/test/regress/expected/tid.out
@@ -61,7 +61,8 @@ DROP SEQUENCE tid_seq;
-- Index, fails with incorrect relation type
CREATE INDEX tid_ind ON tid_tab(a);
SELECT currtid2('tid_ind'::text, '(0,1)'::tid); -- fails
-ERROR: "tid_ind" is an index
+ERROR: cannot operate on ralation "tid_ind"
+DETAIL: This operation is not supported for indexes.
DROP INDEX tid_ind;
-- Partitioned table, no storage
CREATE TABLE tid_part (a int) PARTITION BY RANGE (a);
--
2.36.1
Re: [PATCH v1] eliminate duplicate code in table.c
Hi Amit,
Yep, validate_relation_type() sounds better.
Or maybe validate_relation_kind() after all?
--
Best regards,
Aleksander Alekseev
Re: [PATCH v1] eliminate duplicate code in table.c
yeah, IMHO validate_relation_kind() is better ;)
On Thu, Jul 21, 2022 at 10:21 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
Hi Amit,
Yep, validate_relation_type() sounds better.
Or maybe validate_relation_kind() after all?
--
Best regards,
Aleksander Alekseev
--
Regards
Junwang Zhao
Re: [PATCH v1] eliminate duplicate code in table.c
btw, there are some typos in Patch v5, %s/ralation/relation/g
On Thu, Jul 21, 2022 at 10:05 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
Hi Amit,
Yeah, that's better. On again thinking about the function name, I
wonder if validate_relation_type() suits here as there is no generic
object being passed?Yep, validate_relation_type() sounds better.
--
Best regards,
Aleksander Alekseev
--
Regards
Junwang Zhao
Re: [PATCH v1] eliminate duplicate code in table.c
Hi Junwang,
btw, there are some typos in Patch v5, %s/ralation/relation/g
D'oh!
yeah, IMHO validate_relation_kind() is better ;)
Cool. Here is the corrected patch. Thanks!
--
Best regards,
Aleksander Alekseev
Attachments:
v6-0001-Eliminate-duplicate-code-in-table.c.patchapplication/octet-stream; name=v6-0001-Eliminate-duplicate-code-in-table.c.patchDownload
From ee9da6245d8f6894e811ec75a18c43078d01c233 Mon Sep 17 00:00:00 2001
From: Junwang Zhao <zhjwpku@gmail.com>
Date: Thu, 21 Jul 2022 16:22:17 +0800
Subject: [PATCH v6] Eliminate duplicate code in table.c
Additionally improve error messages similarly to how it was done in 2ed532ee.
Author: Junwang Zhao <zhjwpku@gmail.com>
Author: Aleksander Alekseev <aleksander@timescale.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Alvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/CAEG8a3KbVtBm_BYf5tGsKHvmMieQVsq_jBPOg75VViQB7ACL8Q%40mail.gmail.com
---
src/backend/access/table/table.c | 70 +++++++++++--------------------
src/test/regress/expected/tid.out | 3 +-
2 files changed, 26 insertions(+), 47 deletions(-)
diff --git a/src/backend/access/table/table.c b/src/backend/access/table/table.c
index 744d3b550b..af21944bf1 100644
--- a/src/backend/access/table/table.c
+++ b/src/backend/access/table/table.c
@@ -25,6 +25,7 @@
#include "access/table.h"
#include "storage/lmgr.h"
+static inline void validate_relation_kind(Relation r);
/* ----------------
* table_open - open a table relation by relation OID
@@ -42,17 +43,7 @@ table_open(Oid relationId, LOCKMODE lockmode)
r = relation_open(relationId, lockmode);
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ validate_relation_kind(r);
return r;
}
@@ -76,17 +67,7 @@ try_table_open(Oid relationId, LOCKMODE lockmode)
if (!r)
return NULL;
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ validate_relation_kind(r);
return r;
}
@@ -105,17 +86,7 @@ table_openrv(const RangeVar *relation, LOCKMODE lockmode)
r = relation_openrv(relation, lockmode);
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ validate_relation_kind(r);
return r;
}
@@ -137,19 +108,7 @@ table_openrv_extended(const RangeVar *relation, LOCKMODE lockmode,
r = relation_openrv_extended(relation, lockmode, missing_ok);
if (r)
- {
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
- }
+ validate_relation_kind(r);
return r;
}
@@ -168,3 +127,22 @@ table_close(Relation relation, LOCKMODE lockmode)
{
relation_close(relation, lockmode);
}
+
+/* ----------------
+ * validate_relation_kind - check the relation's kind
+ *
+ * Make sure relkind is not index or composite type
+ * ----------------
+ */
+static inline void
+validate_relation_kind(Relation r)
+{
+ if (r->rd_rel->relkind == RELKIND_INDEX ||
+ r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX ||
+ r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot operate on relation \"%s\"",
+ RelationGetRelationName(r)),
+ errdetail_relkind_not_supported(r->rd_rel->relkind)));
+}
diff --git a/src/test/regress/expected/tid.out b/src/test/regress/expected/tid.out
index 7d8957bd6f..b77c6cf0ed 100644
--- a/src/test/regress/expected/tid.out
+++ b/src/test/regress/expected/tid.out
@@ -61,7 +61,8 @@ DROP SEQUENCE tid_seq;
-- Index, fails with incorrect relation type
CREATE INDEX tid_ind ON tid_tab(a);
SELECT currtid2('tid_ind'::text, '(0,1)'::tid); -- fails
-ERROR: "tid_ind" is an index
+ERROR: cannot operate on relation "tid_ind"
+DETAIL: This operation is not supported for indexes.
DROP INDEX tid_ind;
-- Partitioned table, no storage
CREATE TABLE tid_part (a int) PARTITION BY RANGE (a);
--
2.36.1
Re: [PATCH v1] eliminate duplicate code in table.c
LGTM
On Thu, Jul 21, 2022 at 11:52 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
Hi Junwang,
btw, there are some typos in Patch v5, %s/ralation/relation/g
D'oh!
yeah, IMHO validate_relation_kind() is better ;)
Cool. Here is the corrected patch. Thanks!
--
Best regards,
Aleksander Alekseev
--
Regards
Junwang Zhao
Re: [PATCH v1] eliminate duplicate code in table.c
Hi.
+ errmsg("cannot operate on relation \"%s\"",
Other callers of errdetail_relkind_not_supported() describing
operations concretely. In that sense we I think should say "cannot
open relation \"%s\"" here.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: [PATCH v1] eliminate duplicate code in table.c
On Fri, Jul 22, 2022 at 7:39 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
+ errmsg("cannot operate on relation \"%s\"",
Other callers of errdetail_relkind_not_supported() describing
operations concretely. In that sense we I think should say "cannot
open relation \"%s\"" here.
Sounds reasonable to me. This will give more precise information to the user.
--
With Regards,
Amit Kapila.
Re: [PATCH v1] eliminate duplicate code in table.c
Here is the patch v7. Thanks!
On Fri, Jul 22, 2022 at 1:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Fri, Jul 22, 2022 at 7:39 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:+ errmsg("cannot operate on relation \"%s\"",
Other callers of errdetail_relkind_not_supported() describing
operations concretely. In that sense we I think should say "cannot
open relation \"%s\"" here.Sounds reasonable to me. This will give more precise information to the user.
--
With Regards,
Amit Kapila.
--
Regards
Junwang Zhao
Attachments:
v7-0001-Eliminate-duplicate-code-in-table.c.patchapplication/octet-stream; name=v7-0001-Eliminate-duplicate-code-in-table.c.patchDownload
From c4f1cec182c8b4ccfc728dac86c3962277b9611c Mon Sep 17 00:00:00 2001
From: Junwang Zhao <zhjwpku@gmail.com>
Date: Thu, 21 Jul 2022 16:22:17 +0800
Subject: [PATCH v7] Eliminate duplicate code in table.c
Additionally improve error messages similarly to how it was done in 2ed532ee.
Author: Junwang Zhao <zhjwpku@gmail.com>
Author: Aleksander Alekseev <aleksander@timescale.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Alvaro Herrera <alvherre@alvh.no-ip.org>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://postgr.es/m/CAEG8a3KbVtBm_BYf5tGsKHvmMieQVsq_jBPOg75VViQB7ACL8Q%40mail.gmail.com
---
src/backend/access/table/table.c | 70 +++++++++++--------------------
src/test/regress/expected/tid.out | 3 +-
2 files changed, 26 insertions(+), 47 deletions(-)
diff --git a/src/backend/access/table/table.c b/src/backend/access/table/table.c
index 744d3b550b..7e94232f01 100644
--- a/src/backend/access/table/table.c
+++ b/src/backend/access/table/table.c
@@ -25,6 +25,7 @@
#include "access/table.h"
#include "storage/lmgr.h"
+static inline void validate_relation_kind(Relation r);
/* ----------------
* table_open - open a table relation by relation OID
@@ -42,17 +43,7 @@ table_open(Oid relationId, LOCKMODE lockmode)
r = relation_open(relationId, lockmode);
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ validate_relation_kind(r);
return r;
}
@@ -76,17 +67,7 @@ try_table_open(Oid relationId, LOCKMODE lockmode)
if (!r)
return NULL;
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ validate_relation_kind(r);
return r;
}
@@ -105,17 +86,7 @@ table_openrv(const RangeVar *relation, LOCKMODE lockmode)
r = relation_openrv(relation, lockmode);
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
+ validate_relation_kind(r);
return r;
}
@@ -137,19 +108,7 @@ table_openrv_extended(const RangeVar *relation, LOCKMODE lockmode,
r = relation_openrv_extended(relation, lockmode, missing_ok);
if (r)
- {
- if (r->rd_rel->relkind == RELKIND_INDEX ||
- r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
- RelationGetRelationName(r))));
- else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
- RelationGetRelationName(r))));
- }
+ validate_relation_kind(r);
return r;
}
@@ -168,3 +127,22 @@ table_close(Relation relation, LOCKMODE lockmode)
{
relation_close(relation, lockmode);
}
+
+/* ----------------
+ * validate_relation_kind - check the relation's kind
+ *
+ * Make sure relkind is not index or composite type
+ * ----------------
+ */
+static inline void
+validate_relation_kind(Relation r)
+{
+ if (r->rd_rel->relkind == RELKIND_INDEX ||
+ r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX ||
+ r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot open relation \"%s\"",
+ RelationGetRelationName(r)),
+ errdetail_relkind_not_supported(r->rd_rel->relkind)));
+}
diff --git a/src/test/regress/expected/tid.out b/src/test/regress/expected/tid.out
index 7d8957bd6f..8cd6d60595 100644
--- a/src/test/regress/expected/tid.out
+++ b/src/test/regress/expected/tid.out
@@ -61,7 +61,8 @@ DROP SEQUENCE tid_seq;
-- Index, fails with incorrect relation type
CREATE INDEX tid_ind ON tid_tab(a);
SELECT currtid2('tid_ind'::text, '(0,1)'::tid); -- fails
-ERROR: "tid_ind" is an index
+ERROR: cannot open relation "tid_ind"
+DETAIL: This operation is not supported for indexes.
DROP INDEX tid_ind;
-- Partitioned table, no storage
CREATE TABLE tid_part (a int) PARTITION BY RANGE (a);
--
2.33.0