> index expression/predicate and check constraint expression can not contain
> subquery, that's why using pull_varattnos to test whole-row containment works
> fine. but pull_varattnos can not cope with subquery, see pull_varattnos
> comments.
>
> row security policy can have subquery, for example:
> CREATE POLICY p1 ON document AS PERMISSIVE
> USING (dlevel <= (SELECT seclv FROM uaccount WHERE pguser = current_user));
>
> so I am still working on whole-row referenced policies interacting with ALTER
> TABLE SET DATA TYPE/ALTER TABLE DROP COLUMN.
> <v3-0002-disallow-ALTER-COLUMN-SET-DATA-TYPE-when-wholerow-referenced-cons.patch><v3-0001-ALTER-TABLE-DROP-COLUMN-drop-wholerow-referenced-object.patch>
1 - 0001
```
+ * ALTER TABLE DROP COLUMN also need drop indexes or constraints that
```
Nit: need -> needs to
2 - 0001
```
+ tmpobject.classId = RelationRelationId;
+ tmpobject.objectId = RelationGetRelid(rel);
+ tmpobject.objectSubId = attnum;
```
Originally “object” points to the column to delete, but with is patch, you are
using “object” for index/constrain to delete and “tmpobject” for the column to
delete, which could be misleading.
I’d suggest keep the meaning of “object” unchanged, you need to pull up of
initialization of “object” to the place now you initiate “tmpobject”. And only
define “tmpobject” in sections where it is needed. So you can name it
“idxObject” or “consObject”, which will be more clearly meaning. I think you
may also rename “object” to “colObject”.
3 - 0001
```
+ }
+ }
+ ReleaseSysCache(indexTuple);
+ }
+ CommandCounterIncrement();
```
Why CommandCounterIncrement() is needed? In current code, there is a
CommandCounterIncrement() after CatalogTupleUpdate(), which is necessary. But
for your new code, maybe you considered “recordDependencyOn()” needs
CommandCounterIncrement(). I searched over all places when
“recordDependencyOn()” is called, I don’t see CommandCounterIncrement() is
called.
4 - 0001
```
+ if (!heap_attisnull(indexTuple, Anum_pg_index_indpred, NULL))
+ {
….
+ }
+
+ if (!found_whole_row && !heap_attisnull(indexTuple,
Anum_pg_index_indexprs, NULL))
+ {
…
+ }
```
These two pieces of code are exactly the same expect operating different
Anum_pg_index_indpred/indexprs. I think we can create a static function to
avoid duplicate code.
5 - 0001
···
+ conscan = systable_beginscan(conDesc,
ConstraintRelidTypidNameIndexId, true,
+
NULL, 3, skey);
+ if (!HeapTupleIsValid(contuple =
systable_getnext(conscan)))
+ elog(ERROR, "constraint \"%s\" of
relation \"%s\" does not exist",
+ constr_name,
RelationGetRelationName(rel));
···
Should we continue after elog()?
6 - 0002
```
+ ereport(ERROR,
+
errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("cannot alter
table column \"%s\".\"%s\" to type %s because constraint \"%s\" uses table
\"%s\" row type",
+
RelationGetRelationName(rel),
+ colName,
+
format_type_with_typemod(targettype, targettypmod),
+ constr_name,
+
RelationGetRelationName(rel)),
```
I think the second relation name is quite duplicate. We can just say “because
constraint “xx” uses whole-row type".
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
