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

On Tue, Sep 10, 2024 at 8:46 AM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> 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.

What it really means is "Prevent them from altering any attribute not
defined by user" - a whole row reference is not defined explicitly by
user; it's collection of user defined attributes and it's not
cataloged.

I think we generally confuse between system attribute and !(user
attribute); the grey being attnum = 0. It might be better to create
macros for these cases and use them to make their usage clear.

e.g. #define ATTNUM_IS_SYSTEM(attnum) ((attnum) < 0)
#define ATTNUM_IS_USER_DEFINED(attnum) ((attnum) > 0)
#define WholeRowAttrNumber 0
add good comments about usage near their definitions and use
appropriately in the code.

Example above would then turn into (notice ! in the condition)
/* Prevent them from altering an attribute not defined by user */
if (!ATTNUM_IS_USER_DEFINED(attnum) )
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("attribute \"%s\" is not a user-defined attribute",
colName)));

--
Best Wishes,
Ashutosh Bapat