Hi, On 2018-10-15 16:45:03 -0700, Andres Freund wrote: > I've a patch making .data variables constant, fwiw. Will post in a > bit. Just so we don't duplicate work too much.
I pushed a few very obvious ones. An additional fix, for the system attributes in heap.c, is attached. But that's a bit more invasive, so I wanted to get some input. I think all of the changes are fairly obvious and the new const attributes just document what already had to be the case. But to correctly propagate the const through typedef-the-pointer-away typedefs, I had to use the underlying type (e.g. use const NameData *n2 instad of Name n2). To me that's the correct fix, but then I hate these typedefs with a passion anyway. An alternative fix is to add additional typedefs like ConstName, but I don't find that particularly advantageous. Comments? Arguably we should use consts more aggresively in signatures anyway, but I think that's a separate effort. Greetings, Andres Freund
>From 6db74bb80e13415467b1be6c340b0a06713f891b Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Mon, 15 Oct 2018 21:05:05 -0700 Subject: [PATCH] Correct constness of system attributes in heap.c & prerequisites. This allows the compiler / linker to mark affected pages as read-only. There's a fair number of pre-requisite changes, so the const can properly be propagated. Most of these were already required for correctness anyway. Arguably we could be more aggressive in using consts in related code, but.. Discussion: https://postgr.es/m/20181015200754.7y7zfuzsoux2c...@alap3.anarazel.de --- src/backend/catalog/heap.c | 22 +++++++++++----------- src/backend/catalog/index.c | 2 +- src/backend/executor/spi.c | 4 ++-- src/backend/optimizer/util/plancat.c | 2 +- src/backend/parser/parse_relation.c | 8 ++++---- src/backend/parser/parse_utilcmd.c | 2 +- src/backend/utils/adt/expandedrecord.c | 13 +++++++------ src/backend/utils/adt/name.c | 2 +- src/include/catalog/heap.h | 4 ++-- src/include/parser/parse_relation.h | 2 +- src/include/utils/builtins.h | 2 +- 11 files changed, 32 insertions(+), 31 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 4ad58689fc3..ea69aa7f12a 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -144,7 +144,7 @@ static List *insert_ordered_unique_oid(List *list, Oid datum); * fixed-size portion of the structure anyway. */ -static FormData_pg_attribute a1 = { +static const FormData_pg_attribute a1 = { .attname = {"ctid"}, .atttypid = TIDOID, .attlen = sizeof(ItemPointerData), @@ -158,7 +158,7 @@ static FormData_pg_attribute a1 = { .attislocal = true, }; -static FormData_pg_attribute a2 = { +static const FormData_pg_attribute a2 = { .attname = {"oid"}, .atttypid = OIDOID, .attlen = sizeof(Oid), @@ -172,7 +172,7 @@ static FormData_pg_attribute a2 = { .attislocal = true, }; -static FormData_pg_attribute a3 = { +static const FormData_pg_attribute a3 = { .attname = {"xmin"}, .atttypid = XIDOID, .attlen = sizeof(TransactionId), @@ -186,7 +186,7 @@ static FormData_pg_attribute a3 = { .attislocal = true, }; -static FormData_pg_attribute a4 = { +static const FormData_pg_attribute a4 = { .attname = {"cmin"}, .atttypid = CIDOID, .attlen = sizeof(CommandId), @@ -200,7 +200,7 @@ static FormData_pg_attribute a4 = { .attislocal = true, }; -static FormData_pg_attribute a5 = { +static const FormData_pg_attribute a5 = { .attname = {"xmax"}, .atttypid = XIDOID, .attlen = sizeof(TransactionId), @@ -214,7 +214,7 @@ static FormData_pg_attribute a5 = { .attislocal = true, }; -static FormData_pg_attribute a6 = { +static const FormData_pg_attribute a6 = { .attname = {"cmax"}, .atttypid = CIDOID, .attlen = sizeof(CommandId), @@ -234,7 +234,7 @@ static FormData_pg_attribute a6 = { * table of a particular class/type. In any case table is still the word * used in SQL. */ -static FormData_pg_attribute a7 = { +static const FormData_pg_attribute a7 = { .attname = {"tableoid"}, .atttypid = OIDOID, .attlen = sizeof(Oid), @@ -248,14 +248,14 @@ static FormData_pg_attribute a7 = { .attislocal = true, }; -static const Form_pg_attribute SysAtt[] = {&a1, &a2, &a3, &a4, &a5, &a6, &a7}; +static const FormData_pg_attribute * SysAtt[] = {&a1, &a2, &a3, &a4, &a5, &a6, &a7}; /* * This function returns a Form_pg_attribute pointer for a system attribute. * Note that we elog if the presented attno is invalid, which would only * happen if there's a problem upstream. */ -Form_pg_attribute +const FormData_pg_attribute * SystemAttributeDefinition(AttrNumber attno, bool relhasoids) { if (attno >= 0 || attno < -(int) lengthof(SysAtt)) @@ -269,14 +269,14 @@ SystemAttributeDefinition(AttrNumber attno, bool relhasoids) * If the given name is a system attribute name, return a Form_pg_attribute * pointer for a prototype definition. If not, return NULL. */ -Form_pg_attribute +const FormData_pg_attribute * SystemAttributeByName(const char *attname, bool relhasoids) { int j; for (j = 0; j < (int) lengthof(SysAtt); j++) { - Form_pg_attribute att = SysAtt[j]; + const FormData_pg_attribute *att = SysAtt[j]; if (relhasoids || att->attnum != ObjectIdAttributeNumber) { diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 339bb35f9bc..f1ef4c319a0 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -352,7 +352,7 @@ ConstructTupleDescriptor(Relation heapRelation, if (atnum != 0) { /* Simple index column */ - Form_pg_attribute from; + const FormData_pg_attribute *from; if (atnum < 0) { diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c index fb36e762f28..19212738568 100644 --- a/src/backend/executor/spi.c +++ b/src/backend/executor/spi.c @@ -899,7 +899,7 @@ int SPI_fnumber(TupleDesc tupdesc, const char *fname) { int res; - Form_pg_attribute sysatt; + const FormData_pg_attribute *sysatt; for (res = 0; res < tupdesc->natts; res++) { @@ -921,7 +921,7 @@ SPI_fnumber(TupleDesc tupdesc, const char *fname) char * SPI_fname(TupleDesc tupdesc, int fnumber) { - Form_pg_attribute att; + const FormData_pg_attribute *att; SPI_result = 0; diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c index 8369e3ad62d..46de00460d9 100644 --- a/src/backend/optimizer/util/plancat.c +++ b/src/backend/optimizer/util/plancat.c @@ -1692,7 +1692,7 @@ build_index_tlist(PlannerInfo *root, IndexOptInfo *index, if (indexkey != 0) { /* simple column */ - Form_pg_attribute att_tup; + const FormData_pg_attribute *att_tup; if (indexkey < 0) att_tup = SystemAttributeDefinition(indexkey, diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c index f115ed863af..ac374d1d894 100644 --- a/src/backend/parser/parse_relation.c +++ b/src/backend/parser/parse_relation.c @@ -3135,7 +3135,7 @@ attnameAttNum(Relation rd, const char *attname, bool sysColOK) static int specialAttNum(const char *attname) { - Form_pg_attribute sysatt; + const FormData_pg_attribute *sysatt; sysatt = SystemAttributeByName(attname, true /* "oid" will be accepted */ ); @@ -3152,12 +3152,12 @@ specialAttNum(const char *attname) * heap_open()'ed. Use the cache version get_atttype() * for access to non-opened relations. */ -Name +const NameData* attnumAttName(Relation rd, int attid) { if (attid <= 0) { - Form_pg_attribute sysatt; + const FormData_pg_attribute *sysatt; sysatt = SystemAttributeDefinition(attid, rd->rd_rel->relhasoids); return &sysatt->attname; @@ -3179,7 +3179,7 @@ attnumTypeId(Relation rd, int attid) { if (attid <= 0) { - Form_pg_attribute sysatt; + const FormData_pg_attribute *sysatt; sysatt = SystemAttributeDefinition(attid, rd->rd_rel->relhasoids); return sysatt->atttypid; diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index 42bf9bfec62..d8387d43569 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -2065,7 +2065,7 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt) for (i = 0; i < index_form->indnatts; i++) { int16 attnum = index_form->indkey.values[i]; - Form_pg_attribute attform; + const FormData_pg_attribute *attform; char *attname; Oid defopclass; diff --git a/src/backend/utils/adt/expandedrecord.c b/src/backend/utils/adt/expandedrecord.c index b1b6883c19f..9aa6f3aac51 100644 --- a/src/backend/utils/adt/expandedrecord.c +++ b/src/backend/utils/adt/expandedrecord.c @@ -1025,6 +1025,7 @@ expanded_record_lookup_field(ExpandedRecordHeader *erh, const char *fieldname, TupleDesc tupdesc; int fno; Form_pg_attribute attr; + const FormData_pg_attribute *sysattr; tupdesc = expanded_record_get_tupdesc(erh); @@ -1044,13 +1045,13 @@ expanded_record_lookup_field(ExpandedRecordHeader *erh, const char *fieldname, } /* How about system attributes? */ - attr = SystemAttributeByName(fieldname, tupdesc->tdhasoid); - if (attr != NULL) + sysattr = SystemAttributeByName(fieldname, tupdesc->tdhasoid); + if (sysattr != NULL) { - finfo->fnumber = attr->attnum; - finfo->ftypeid = attr->atttypid; - finfo->ftypmod = attr->atttypmod; - finfo->fcollation = attr->attcollation; + finfo->fnumber = sysattr->attnum; + finfo->ftypeid = sysattr->atttypid; + finfo->ftypmod = sysattr->atttypmod; + finfo->fcollation = sysattr->attcollation; return true; } diff --git a/src/backend/utils/adt/name.c b/src/backend/utils/adt/name.c index fe20448ac57..c266da2de1c 100644 --- a/src/backend/utils/adt/name.c +++ b/src/backend/utils/adt/name.c @@ -188,7 +188,7 @@ namege(PG_FUNCTION_ARGS) /* (see char.c for comparison/operation routines) */ int -namecpy(Name n1, Name n2) +namecpy(Name n1, const NameData *n2) { if (!n1 || !n2) return -1; diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h index b3e8fdd9c60..39f04b06eef 100644 --- a/src/include/catalog/heap.h +++ b/src/include/catalog/heap.h @@ -127,10 +127,10 @@ extern void RemoveAttrDefault(Oid relid, AttrNumber attnum, extern void RemoveAttrDefaultById(Oid attrdefId); extern void RemoveStatistics(Oid relid, AttrNumber attnum); -extern Form_pg_attribute SystemAttributeDefinition(AttrNumber attno, +extern const FormData_pg_attribute *SystemAttributeDefinition(AttrNumber attno, bool relhasoids); -extern Form_pg_attribute SystemAttributeByName(const char *attname, +extern const FormData_pg_attribute *SystemAttributeByName(const char *attname, bool relhasoids); extern void CheckAttributeNamesTypes(TupleDesc tupdesc, char relkind, diff --git a/src/include/parser/parse_relation.h b/src/include/parser/parse_relation.h index 687a7d7b1b9..03160fd71da 100644 --- a/src/include/parser/parse_relation.h +++ b/src/include/parser/parse_relation.h @@ -125,7 +125,7 @@ extern void expandRTE(RangeTblEntry *rte, int rtindex, int sublevels_up, extern List *expandRelAttrs(ParseState *pstate, RangeTblEntry *rte, int rtindex, int sublevels_up, int location); extern int attnameAttNum(Relation rd, const char *attname, bool sysColOK); -extern Name attnumAttName(Relation rd, int attid); +extern const NameData* attnumAttName(Relation rd, int attid); extern Oid attnumTypeId(Relation rd, int attid); extern Oid attnumCollationId(Relation rd, int attid); extern bool isQueryUsingTempRelation(Query *query); diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h index 6f55699912c..61785a24335 100644 --- a/src/include/utils/builtins.h +++ b/src/include/utils/builtins.h @@ -37,7 +37,7 @@ extern unsigned hex_decode(const char *src, unsigned len, char *dst); extern int2vector *buildint2vector(const int16 *int2s, int n); /* name.c */ -extern int namecpy(Name n1, Name n2); +extern int namecpy(Name n1, const NameData *n2); extern int namestrcpy(Name name, const char *str); extern int namestrcmp(Name name, const char *str); -- 2.18.0.rc2.dirty
