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

jian he <jian.universality@gmail.com> writes:

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.

attnum = 0 is also used for whole-row Vars. This is a pretty
unfortunate choice given the alternative meaning of "invalid",
but cleaning it up would be a daunting task (with not a whole
lot of payoff in the end, AFAICS). It's deeply embedded.

That being the case, you have to tread *very* carefully when
considering making changes like this.

for example, ATExecColumnDefault, following code snippet,
the second ereport should be "if (attnum < 0)"

/* Prevent them from altering a system attribute */
if (attnum <= 0)

I think that's just fine as-is. Sure, the == case is unreachable,
but it is very very common to consider whole-row Vars as being more
like system attributes than user attributes. In this particular
case, for sure we don't want to permit attaching a default to a
whole-row Var. So I'm content to allow the duplicative rejection.

regards, tom lane

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