public inbox for [email protected]  

help / color / mirror / Atom feed

From: Chao Li <[email protected]>
To: jian he <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: let ALTER TABLE DROP COLUMN drop whole-row referenced object
Date: Mon, 15 Sep 2025 11:47:57 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <CACJufxHmmsg7GTD_LpGGam09+Aq4KkYHe2dfPgtGjrUELqbK=w@mail.gmail.com>
References: <CACJufxGA6KVQy7DbHGLVw9s9KKmpGyZt5ME6C7kEfjDpr2wZCw@mail.gmail.com>
	<CACJufxFMqpraCreGKTWWB7cyPAYP+yehwTMFm-CSx+rzWdHf+A@mail.gmail.com>
	<[email protected]>
	<CACJufxHmmsg7GTD_LpGGam09+Aq4KkYHe2dfPgtGjrUELqbK=w@mail.gmail.com>


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

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected]
  Subject: Re: let ALTER TABLE DROP COLUMN drop whole-row referenced object
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox