At Sun, 01 Nov 2020 21:05:29 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in > Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes: > > We cannot reach there with ev_action == NULL since it comes from a > > non-nullable column. Since most of the other columns has an assertion > > that !isnull, I think we should do the same thing for ev_action (and > > ev_qual). SPI_getvalue() returns C-NULL for SQL-NULL (or for some > > other unexpected situations.). > > Isn't the comment just above there wrong? > > /* these could be nulls */ > > I wonder just when that became outdated.
Mmm. I investigated that.
At the very beginning of CREATE RULE (d31084e9d1, 1996), InsertRule() did the following.
Doesn't seem that ev_qual and ev_action can be NULL. The same function in the current converts action list to string using nodeToSTring so NIL is converted into '<>', which is not NULL.
So I think ev_action cannot be null from the beginning of the history unless the columns is modified manually. ev_qual and ev_action are marked as non-nullable (9b39b799db, in 2018). They could be null if we modified that columns nullable then set NULL, but that could happen on all other columns in pg_rewite catalog, which are Assert(!null)ed.
Although ev_action cannot be a empty list using SQL interface. So we can get rid of the case list_length(action) == 0, but I'm not sure it's worth doing (but the attaches does..).
I think that Assert is not the right solution here.
For a function that returns NULL twice (SPI_getvalue), it is worth testing the result against NULL. In the future, any modification may cause further dereference. In addition, the static analysis tools would continue to note this snippet either as a bug or as a suspect.
Checking "actions" pointer against NULL, and acting appropriately would do.
