Re: let ALTER TABLE DROP COLUMN drop whole-row referenced object

> 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/