removing unnecessary get_att*() lsyscache functions
I noticed that get_attidentity() isn't really necessary because the
information can be obtained from an existing tuple descriptor in each case.
Also, get_atttypmod() hasn't been used since 2004.
I propose the attached patches to remove these two functions.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Remove-get_attidentity.patchtext/plain; charset=UTF-8; name=0001-Remove-get_attidentity.patch; x-mac-creator=0; x-mac-type=0Download
From 2003c5b4fdad1cd444ddb1fad2bdc9ab09db2fd2 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Thu, 18 Oct 2018 19:27:18 +0200
Subject: [PATCH 1/2] Remove get_attidentity()
All existing uses can get this information more easily from the
relation descriptor, so the detour through the syscache is not
necessary.
---
src/backend/commands/tablecmds.c | 4 ++--
src/backend/parser/parse_utilcmd.c | 3 ++-
src/backend/utils/cache/lsyscache.c | 32 -----------------------------
src/include/utils/lsyscache.h | 1 -
4 files changed, 4 insertions(+), 36 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3e112b4ef4..7ff25a9eff 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5945,7 +5945,7 @@ ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
errmsg("cannot alter system column \"%s\"",
colName)));
- if (get_attidentity(RelationGetRelid(rel), attnum))
+ if (TupleDescAttr(RelationGetDescr(rel), attnum - 1)->attidentity)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("column \"%s\" of relation \"%s\" is an identity column",
@@ -6148,7 +6148,7 @@ ATExecColumnDefault(Relation rel, const char *colName,
errmsg("cannot alter system column \"%s\"",
colName)));
- if (get_attidentity(RelationGetRelid(rel), attnum))
+ if (TupleDescAttr(RelationGetDescr(rel), attnum - 1)->attidentity)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("column \"%s\" of relation \"%s\" is an identity column",
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index d8387d4356..212eedfa87 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -3067,7 +3067,8 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
* if attribute not found, something will error about it
* later
*/
- if (attnum != InvalidAttrNumber && get_attidentity(relid, attnum))
+ if (attnum != InvalidAttrNumber &&
+ TupleDescAttr(RelationGetDescr(rel), attnum - 1)->attidentity)
{
Oid seq_relid = getOwnedSequence(relid, attnum);
Oid typeOid = typenameTypeId(pstate, def->typeName);
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 12b2532d95..c5fcaa9542 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -821,38 +821,6 @@ get_attnum(Oid relid, const char *attname)
return InvalidAttrNumber;
}
-/*
- * get_attidentity
- *
- * Given the relation id and the attribute name,
- * return the "attidentity" field from the attribute relation.
- *
- * Returns '\0' if not found.
- *
- * Since no identity is represented by '\0', this can also be used as a
- * Boolean test.
- */
-char
-get_attidentity(Oid relid, AttrNumber attnum)
-{
- HeapTuple tp;
-
- tp = SearchSysCache2(ATTNUM,
- ObjectIdGetDatum(relid),
- Int16GetDatum(attnum));
- if (HeapTupleIsValid(tp))
- {
- Form_pg_attribute att_tup = (Form_pg_attribute) GETSTRUCT(tp);
- char result;
-
- result = att_tup->attidentity;
- ReleaseSysCache(tp);
- return result;
- }
- else
- return '\0';
-}
-
/*
* get_atttype
*
diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index 8c57de77c0..151081c693 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -85,7 +85,6 @@ extern Oid get_opfamily_proc(Oid opfamily, Oid lefttype, Oid righttype,
int16 procnum);
extern char *get_attname(Oid relid, AttrNumber attnum, bool missing_ok);
extern AttrNumber get_attnum(Oid relid, const char *attname);
-extern char get_attidentity(Oid relid, AttrNumber attnum);
extern Oid get_atttype(Oid relid, AttrNumber attnum);
extern int32 get_atttypmod(Oid relid, AttrNumber attnum);
extern void get_atttypetypmodcoll(Oid relid, AttrNumber attnum,
base-commit: a7a1b44516e7db89104c06440f759c2831fcb0ff
--
2.19.1
0002-Remove-get_atttypmod.patchtext/plain; charset=UTF-8; name=0002-Remove-get_atttypmod.patch; x-mac-creator=0; x-mac-type=0Download
From 013205793925abd336744890c49e30bfca06db01 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Thu, 18 Oct 2018 19:28:28 +0200
Subject: [PATCH 2/2] Remove get_atttypmod()
This has been unused since 2004. get_atttypetypmodcoll() is often a
better alternative.
---
src/backend/utils/cache/lsyscache.c | 29 +----------------------------
src/include/utils/lsyscache.h | 1 -
2 files changed, 1 insertion(+), 29 deletions(-)
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index c5fcaa9542..892ddc0d48 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -848,40 +848,13 @@ get_atttype(Oid relid, AttrNumber attnum)
return InvalidOid;
}
-/*
- * get_atttypmod
- *
- * Given the relation id and the attribute number,
- * return the "atttypmod" field from the attribute relation.
- */
-int32
-get_atttypmod(Oid relid, AttrNumber attnum)
-{
- HeapTuple tp;
-
- tp = SearchSysCache2(ATTNUM,
- ObjectIdGetDatum(relid),
- Int16GetDatum(attnum));
- if (HeapTupleIsValid(tp))
- {
- Form_pg_attribute att_tup = (Form_pg_attribute) GETSTRUCT(tp);
- int32 result;
-
- result = att_tup->atttypmod;
- ReleaseSysCache(tp);
- return result;
- }
- else
- return -1;
-}
-
/*
* get_atttypetypmodcoll
*
* A three-fer: given the relation id and the attribute number,
* fetch atttypid, atttypmod, and attcollation in a single cache lookup.
*
- * Unlike the otherwise-similar get_atttype/get_atttypmod, this routine
+ * Unlike the otherwise-similar get_atttype, this routine
* raises an error if it can't obtain the information.
*/
void
diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index 151081c693..ff1705ad2b 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -86,7 +86,6 @@ extern Oid get_opfamily_proc(Oid opfamily, Oid lefttype, Oid righttype,
extern char *get_attname(Oid relid, AttrNumber attnum, bool missing_ok);
extern AttrNumber get_attnum(Oid relid, const char *attname);
extern Oid get_atttype(Oid relid, AttrNumber attnum);
-extern int32 get_atttypmod(Oid relid, AttrNumber attnum);
extern void get_atttypetypmodcoll(Oid relid, AttrNumber attnum,
Oid *typid, int32 *typmod, Oid *collid);
extern char *get_collation_name(Oid colloid);
--
2.19.1
On Thu, Oct 18, 2018 at 09:57:00PM +0200, Peter Eisentraut wrote:
I noticed that get_attidentity() isn't really necessary because the
information can be obtained from an existing tuple descriptor in each
case.
This one is also recent, so it looks fine to remove it.
Also, get_atttypmod() hasn't been used since 2004.
github is not actually reporting areas where this is used.
I propose the attached patches to remove these two functions.
- if (get_attidentity(RelationGetRelid(rel), attnum)) + if (TupleDescAttr(RelationGetDescr(rel), attnum - 1)->attidentity)
I find this style heavy, saving Form_pg_attribute into a different
variable would be more readable in my opinion..
--
Michael
On 19/10/2018 16:00, Michael Paquier wrote:
- if (get_attidentity(RelationGetRelid(rel), attnum)) + if (TupleDescAttr(RelationGetDescr(rel), attnum - 1)->attidentity)I find this style heavy, saving Form_pg_attribute into a different
variable would be more readable in my opinion..
OK, slightly reworked version attached.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
v2-0001-Remove-get_atttypmod.patchtext/plain; charset=UTF-8; name=v2-0001-Remove-get_atttypmod.patch; x-mac-creator=0; x-mac-type=0Download
From d9a63e636cb5ad41d3105d62dce497141685ac22 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Thu, 18 Oct 2018 19:28:28 +0200
Subject: [PATCH v2 1/2] Remove get_atttypmod()
This has been unused since 2004. get_atttypetypmodcoll() is often a
better alternative.
---
src/backend/utils/cache/lsyscache.c | 29 +----------------------------
src/include/utils/lsyscache.h | 1 -
2 files changed, 1 insertion(+), 29 deletions(-)
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 12b2532d95..c53bda9867 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -880,40 +880,13 @@ get_atttype(Oid relid, AttrNumber attnum)
return InvalidOid;
}
-/*
- * get_atttypmod
- *
- * Given the relation id and the attribute number,
- * return the "atttypmod" field from the attribute relation.
- */
-int32
-get_atttypmod(Oid relid, AttrNumber attnum)
-{
- HeapTuple tp;
-
- tp = SearchSysCache2(ATTNUM,
- ObjectIdGetDatum(relid),
- Int16GetDatum(attnum));
- if (HeapTupleIsValid(tp))
- {
- Form_pg_attribute att_tup = (Form_pg_attribute) GETSTRUCT(tp);
- int32 result;
-
- result = att_tup->atttypmod;
- ReleaseSysCache(tp);
- return result;
- }
- else
- return -1;
-}
-
/*
* get_atttypetypmodcoll
*
* A three-fer: given the relation id and the attribute number,
* fetch atttypid, atttypmod, and attcollation in a single cache lookup.
*
- * Unlike the otherwise-similar get_atttype/get_atttypmod, this routine
+ * Unlike the otherwise-similar get_atttype, this routine
* raises an error if it can't obtain the information.
*/
void
diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index 8c57de77c0..23ed5324b5 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -87,7 +87,6 @@ extern char *get_attname(Oid relid, AttrNumber attnum, bool missing_ok);
extern AttrNumber get_attnum(Oid relid, const char *attname);
extern char get_attidentity(Oid relid, AttrNumber attnum);
extern Oid get_atttype(Oid relid, AttrNumber attnum);
-extern int32 get_atttypmod(Oid relid, AttrNumber attnum);
extern void get_atttypetypmodcoll(Oid relid, AttrNumber attnum,
Oid *typid, int32 *typmod, Oid *collid);
extern char *get_collation_name(Oid colloid);
base-commit: 55853d666ce5d0024c50dc3d223346df28abfa70
--
2.19.1
v2-0002-Remove-get_attidentity.patchtext/plain; charset=UTF-8; name=v2-0002-Remove-get_attidentity.patch; x-mac-creator=0; x-mac-type=0Download
From 932a53b5012a7df695fc8f26586c17ca697d50f4 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Thu, 18 Oct 2018 19:27:18 +0200
Subject: [PATCH v2 2/2] Remove get_attidentity()
All existing uses can get this information more easily from the
relation descriptor, so the detour through the syscache is not
necessary.
---
src/backend/commands/tablecmds.c | 13 ++++++------
src/backend/parser/parse_utilcmd.c | 5 ++++-
src/backend/utils/cache/lsyscache.c | 32 -----------------------------
src/include/utils/lsyscache.h | 1 -
4 files changed, 11 insertions(+), 40 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3e112b4ef4..1a1c80ef8b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5917,6 +5917,7 @@ static ObjectAddress
ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
{
HeapTuple tuple;
+ Form_pg_attribute attTup;
AttrNumber attnum;
Relation attr_rel;
List *indexoidlist;
@@ -5929,13 +5930,12 @@ ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
attr_rel = heap_open(AttributeRelationId, RowExclusiveLock);
tuple = SearchSysCacheCopyAttName(RelationGetRelid(rel), colName);
-
if (!HeapTupleIsValid(tuple))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" of relation \"%s\" does not exist",
colName, RelationGetRelationName(rel))));
-
+ attTup = (Form_pg_attribute) GETSTRUCT(tuple);
attnum = ((Form_pg_attribute) GETSTRUCT(tuple))->attnum;
/* Prevent them from altering a system attribute */
@@ -5945,7 +5945,7 @@ ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
errmsg("cannot alter system column \"%s\"",
colName)));
- if (get_attidentity(RelationGetRelid(rel), attnum))
+ if (attTup->attidentity)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("column \"%s\" of relation \"%s\" is an identity column",
@@ -6014,9 +6014,9 @@ ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
/*
* Okay, actually perform the catalog change ... if needed
*/
- if (((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull)
+ if (attTup->attnotnull)
{
- ((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull = false;
+ attTup->attnotnull = false;
CatalogTupleUpdate(attr_rel, &tuple->t_self, tuple);
@@ -6128,6 +6128,7 @@ static ObjectAddress
ATExecColumnDefault(Relation rel, const char *colName,
Node *newDefault, LOCKMODE lockmode)
{
+ TupleDesc tupdesc = RelationGetDescr(rel);
AttrNumber attnum;
ObjectAddress address;
@@ -6148,7 +6149,7 @@ ATExecColumnDefault(Relation rel, const char *colName,
errmsg("cannot alter system column \"%s\"",
colName)));
- if (get_attidentity(RelationGetRelid(rel), attnum))
+ if (TupleDescAttr(tupdesc, attnum - 1)->attidentity)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("column \"%s\" of relation \"%s\" is an identity column",
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index d8387d4356..a6a2de94ea 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -2919,6 +2919,7 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
const char *queryString)
{
Relation rel;
+ TupleDesc tupdesc;
ParseState *pstate;
CreateStmtContext cxt;
List *result;
@@ -2938,6 +2939,7 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
/* Caller is responsible for locking the relation */
rel = relation_open(relid, NoLock);
+ tupdesc = RelationGetDescr(rel);
/* Set up pstate */
pstate = make_parsestate(NULL);
@@ -3067,7 +3069,8 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
* if attribute not found, something will error about it
* later
*/
- if (attnum != InvalidAttrNumber && get_attidentity(relid, attnum))
+ if (attnum != InvalidAttrNumber &&
+ TupleDescAttr(tupdesc, attnum - 1)->attidentity)
{
Oid seq_relid = getOwnedSequence(relid, attnum);
Oid typeOid = typenameTypeId(pstate, def->typeName);
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index c53bda9867..892ddc0d48 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -821,38 +821,6 @@ get_attnum(Oid relid, const char *attname)
return InvalidAttrNumber;
}
-/*
- * get_attidentity
- *
- * Given the relation id and the attribute name,
- * return the "attidentity" field from the attribute relation.
- *
- * Returns '\0' if not found.
- *
- * Since no identity is represented by '\0', this can also be used as a
- * Boolean test.
- */
-char
-get_attidentity(Oid relid, AttrNumber attnum)
-{
- HeapTuple tp;
-
- tp = SearchSysCache2(ATTNUM,
- ObjectIdGetDatum(relid),
- Int16GetDatum(attnum));
- if (HeapTupleIsValid(tp))
- {
- Form_pg_attribute att_tup = (Form_pg_attribute) GETSTRUCT(tp);
- char result;
-
- result = att_tup->attidentity;
- ReleaseSysCache(tp);
- return result;
- }
- else
- return '\0';
-}
-
/*
* get_atttype
*
diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index 23ed5324b5..ff1705ad2b 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -85,7 +85,6 @@ extern Oid get_opfamily_proc(Oid opfamily, Oid lefttype, Oid righttype,
int16 procnum);
extern char *get_attname(Oid relid, AttrNumber attnum, bool missing_ok);
extern AttrNumber get_attnum(Oid relid, const char *attname);
-extern char get_attidentity(Oid relid, AttrNumber attnum);
extern Oid get_atttype(Oid relid, AttrNumber attnum);
extern void get_atttypetypmodcoll(Oid relid, AttrNumber attnum,
Oid *typid, int32 *typmod, Oid *collid);
--
2.19.1
On Mon, Oct 22, 2018 at 07:12:28PM +0200, Peter Eisentraut wrote:
OK, slightly reworked version attached.
+ attTup = (Form_pg_attribute) GETSTRUCT(tuple);
attnum = ((Form_pg_attribute) GETSTRUCT(tuple))->attnum;
No need to call twice GETSTRUCT here.. The rest looks fine.
--
Michael
Hi,
On 2018-10-23 09:11:17 +0900, Michael Paquier wrote:
On Mon, Oct 22, 2018 at 07:12:28PM +0200, Peter Eisentraut wrote:
OK, slightly reworked version attached.
+ attTup = (Form_pg_attribute) GETSTRUCT(tuple);
attnum = ((Form_pg_attribute) GETSTRUCT(tuple))->attnum;No need to call twice GETSTRUCT here.. The rest looks fine.
Just about every optimize compiler can optimize that away, it's just a
bit of pointer magic. Obviously we don't want to repeat longer lines
superfluously, but twice isn't that bad...
Greetings,
Andres Freund
On 23/10/2018 02:11, Michael Paquier wrote:
On Mon, Oct 22, 2018 at 07:12:28PM +0200, Peter Eisentraut wrote:
OK, slightly reworked version attached.
+ attTup = (Form_pg_attribute) GETSTRUCT(tuple);
attnum = ((Form_pg_attribute) GETSTRUCT(tuple))->attnum;No need to call twice GETSTRUCT here.. The rest looks fine.
Fixed that, and committed, thanks.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services