Postgres Pro
Downloads
Home   >   mailing lists

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

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

From
jian he
Date:
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?

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

From
Tom Lane
Date:
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

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

From
Ashutosh Bapat
Date:
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