Internal catalog tables cannot be updated with SQL

[jkosh44]

Bug Report

Summary

If you try and update an internal catalog table through SQL then the indexes won't be properly updated. The issue is in how we store internal catalog index schemas and how update plans are generated.

Looking at pg_proc as an example, if we look at how the schema is created we see the following: https://github.com/cmu-db/noisepage/blob/d60c55a979e200500ad11b50661ad338be2007d6/src/catalog/postgres/builder.cpp#L796-L807 Notice that the ColumnValueExpression in the IndexSchema::Column does not contain the column name. All other internal catalog tables follow this same pattern.

If we look at the PlanGenerator for Update, we differentiate between indexed updates and non-indexed updates with the following logic: https://github.com/cmu-db/noisepage/blob/d60c55a979e200500ad11b50661ad338be2007d6/src/optimizer/plan_generator.cpp#L888-L930 To summarize this, we collect the column names of all columns from all indexes from the table we are updating. If any of the column names that are being updated are found in this list of column names from the indexes, then it is an indexed update. The difference being that for indexed updates we must update the index and the underlying table, but for non-indexed updates we can just update the underlying table.

The issue here is that the internal catalog tables do not store the column names in the schema, as pointed out above. Therefore all updates on internal catalog tables will incorrectly be categorized as a non-indexed update. This leads to all sorts of issues, specifically that the indexes aren't updated when they should be.

As far as I know there isn't any current use cases that require the internal catalog to be updated through SQL. Still it would probably be best to handle this properly because we may have use cases for this in the future, it prevents people from accidentally corrupting their internal catalogs, and it prevents people from wasting a couple hours trying to figure out why they can't update the internal catalog tables (cough me cough).

Below are two possible approaches to fixing this

1. In the binder, for UpdateStatements collect the column oids of all the columns that are being updated. Then in the PlanGenerator for Update instead of comparing column names you can compare column oids. Credit to @tanujnay112's comment the PlanGenerator.
2. When creating the indexes for the internal catalog tables, add the column name to the ColumnValueExpressions in the IndexSchema::Columns.

I personally prefer the first option because comparing col_oid_t is more efficient that comparing strings and it's probably more consistent too.

This would be a good first issue for someone because it's pretty straightforward, not critical, and would expose you to various different parts of the database.

Environment

OS: Ubuntu (LTS) 20.04

Compiler: GCC 7.0+

CMake Profile: Debug

Jenkins/CI: N/A

Note: This issue is environment independent though the above is the environment I used.

Steps to Reproduce

1. Compile with the following args: -DCMAKE_BUILD_TYPE=Debug -DNOISEPAGE_USE_ASAN=ON
2. Run NoisePage with parallel execution turned off noisepage -parallel_execution=false
3. Try and update an internal catalog table

Expected Behavior

noisepage=# SELECT * FROM pg_language;
 lanoid | lanname  | lanispl | lanpltrusted | lanplcallfoid | laninline | lanvalidator 
--------+----------+---------+--------------+---------------+-----------+--------------
     75 | plpgsql  | f       | t            |               |           |             
     74 | internal | f       | t            |               |           |             
(2 rows)

noisepage=# UPDATE pg_language SET lanoid = 75 WHERE lanoid = 74;
ERROR:  Query failed.
noisepage=# SELECT * FROM pg_language;
 lanoid | lanname  | lanispl | lanpltrusted | lanplcallfoid | laninline | lanvalidator 
--------+----------+---------+--------------+---------------+-----------+--------------
     75 | plpgsql  | f       | t            |               |           |             
     74 | internal | f       | t            |               |           |             
(2 rows)

noisepage=# UPDATE pg_language SET lanoid = 74 WHERE lanoid = 75;
ERROR:  Query failed.
noisepage=# SELECT * FROM pg_language;
 lanoid | lanname  | lanispl | lanpltrusted | lanplcallfoid | laninline | lanvalidator 
--------+----------+---------+--------------+---------------+-----------+--------------
     75 | plpgsql  | f       | t            |               |           |             
     74 | internal | f       | t            |               |           |             
(2 rows)

Actual Behavior

noisepage=# SELECT * FROM pg_language;
 lanoid | lanname  | lanispl | lanpltrusted | lanplcallfoid | laninline | lanvalidator 
--------+----------+---------+--------------+---------------+-----------+--------------
     75 | plpgsql  | f       | t            |               |           |             
     74 | internal | f       | t            |               |           |             
(2 rows)

noisepage=# UPDATE pg_language SET lanoid = 75 WHERE lanoid = 74;
UPDATE 1
noisepage=# SELECT * FROM pg_language;
 lanoid | lanname  | lanispl | lanpltrusted | lanplcallfoid | laninline | lanvalidator 
--------+----------+---------+--------------+---------------+-----------+--------------
     75 | plpgsql  | f       | t            |               |           |             
     75 | internal | f       | t            |               |           |             
(2 rows)

noisepage=# UPDATE pg_language SET lanoid = 74 WHERE lanoid = 75;
UPDATE 1
noisepage=# SELECT * FROM pg_language;
 lanoid | lanname  | lanispl | lanpltrusted | lanplcallfoid | laninline | lanvalidator 
--------+----------+---------+--------------+---------------+-----------+--------------
     74 | plpgsql  | f       | t            |               |           |             
     75 | internal | f       | t            |               |           |             
(2 rows)

A lot of weird stuff going on with this query. lanoid is supposed to be unique but we're breaking that constraint. Our second update should update both rows but it only updates one. The underlying issue is that the table is being updated but not the index.

[jkosh44]

The other option is that we don't ever want to update a catalog table with SQL, in which case we could just return an error or something.