change "attnum <=0" to "attnum <0" for better reflect system attribute

From: jian he <jian(dot)universality(at)gmail(dot)com>
To: PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: change "attnum <=0" to "attnum <0" for better reflect system attribute
Date: 2024-09-10 03:16:34
Message-ID: CACJufxEAN_jgCdGQWyA0LZGBCrnm9iKs6sMKTeVHswovcAv4oQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

hi,
one minor issue. not that minor,
since many DDLs need to consider the system attribute.

looking at these functions:
SearchSysCacheCopyAttName
SearchSysCacheAttName
get_attnum

get_attnum says:
Returns InvalidAttrNumber if the attr doesn't exist (or is dropped).

So I conclude that "attnum == 0" is not related to the idea of a system column.

for example, ATExecColumnDefault, following code snippet,
the second ereport should be "if (attnum < 0)"
==========
attnum = get_attnum(RelationGetRelid(rel), colName);
if (attnum == InvalidAttrNumber)
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" of relation \"%s\" does not exist",
colName, RelationGetRelationName(rel))));

/* Prevent them from altering a system attribute */
if (attnum <= 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot alter system column \"%s\"",
colName)));
==========
but there are many occurrences of "attnum <= 0".
I am sure tablecmds.c, we can change to "attnum < 0".
not that sure with other places.

In some places in tablecmd.c,
we already use "attnum < 0" to represent the system attribute.
so it's kind of inconsistent already.

Should we do the change?

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2024-09-10 03:30:43 Re: change "attnum <=0" to "attnum <0" for better reflect system attribute
Previous Message Peter Smith 2024-09-10 03:09:45 Re: Disallow altering invalidated replication slots