Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1uy0CU-002tAV-Kg for pgsql-hackers@arkaria.postgresql.org; Mon, 15 Sep 2025 03:48:22 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1uy0CR-009gV5-Hj for pgsql-hackers@arkaria.postgresql.org; Mon, 15 Sep 2025 03:48:20 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1uy0CR-009gUw-76 for pgsql-hackers@lists.postgresql.org; Mon, 15 Sep 2025 03:48:20 +0000 Received: from mail-pf1-x431.google.com ([2607:f8b0:4864:20::431]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1uy0CM-000sSR-0w for pgsql-hackers@postgresql.org; Mon, 15 Sep 2025 03:48:18 +0000 Received: by mail-pf1-x431.google.com with SMTP id d2e1a72fcca58-7725de6b57dso4562528b3a.0 for ; Sun, 14 Sep 2025 20:48:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1757908093; x=1758512893; darn=postgresql.org; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:from:to:cc:subject:date:message-id:reply-to; bh=qt5hDc+HzEqS/IdNE3PGScAD7KGWpnXVGk8y6gs+DAo=; b=MP7J/KQ7NxizXCHG+k5cHdl8a60lkqdkW2vXWVCkcdBrPr0NTYtNcAbR9O4a/doD3u hAkEVHdE6H8A/DZXBFYjEdncWWOuCwHFDnhVzbLSAMXtnb5jEcSrOAzsaywEMHyvOgZQ xntnnnz+2+PKmazRAZKSHqb81IN2bpxDj7MIadDuvKr0MmvKlStlyRqj+6i+BOML2ST1 nz087ME/nzWgOlo57NQo3kDEHHsRj53md0szrN/IbMHe0IGGe8cQm1Nk9fyyVJyJ0Hse qwOVQRaFVQm9vqMs3no5WHaCU+5eaFPP2GAjh5QRJ9PPNJxQeQ+Z0OWtbPCWyK012N3T SojA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1757908093; x=1758512893; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=qt5hDc+HzEqS/IdNE3PGScAD7KGWpnXVGk8y6gs+DAo=; b=GRlM8SC2Xq/L9ajGR7XORV//zL1ekCfi3EQxKdGPbfl7PPm7jnDH1VZ9BGHt+fZ3TF dLUmUiU5SS6YgUNAIFBXvKJkIwjOUmAq1Ce9Z1Rh3QuDGf9M6bs4F1PaOWEdKRPzvzjJ Ww5VuENCeyYPefaURNWhbXkrE/eebyj4EMkY+GTdJZvR93nFmAJW6bObYWybh7HRJ7lX +9dFgvYSvgM1PD6/9kjdqrav9QUWzCRFeDvb//DIxWXOMYmalrbIdympW9qQMekZ83Hr 61/8BPMeQBKHo3+hDtccP0+c7Y9hPrPuFOBhhFJ+Cp6s51Yf7IUrOvIkCFtAZdGysDrC voJg== X-Gm-Message-State: AOJu0YyRlckrRHXnL0amkasDi3tbmpZSU+7ZUSsmmt2mvM48CpjlrA6a 7VdRGnbJPaQ7P3RO1W9JBJbS9WIaUwDAsEhfL0TieafmZNb5Rh4HTZtE X-Gm-Gg: ASbGncu6KdfuPtZO6swUy76FqXNEeo+B16CxUeM6rRhwv6Dfk6oxpQhkckDAAcK8E4k aK0V4GuvLCeLDrSWw/4cTMJCdByfpzptM+xxudWEkTQoQ9Gf+gFbx6Osupn8LQqkpuGkF9AWIke 0aWLZjKIHUOhyXP5zLhIhI5RmTRH6HRNgMezTr9uKzvJwPPFSvMRuG1q6D4SAKf55JkbXXrJewb GfU8gKM7V1ZEEd5m7Gx/rhcaP1L1c2H38Hr3dNXAeI/VdsVB+WdlLAy4xrwfcxmfxsTFrY8v5yL +1h+svgZskUYU30jVYpr7Nv1o5PoZ+fMW0J/NR1HetSS72gWP0ROjKeBBuH3Eeteqj2IlEjjWA2 1+raKGPaTUBjH4f+XYsXIwElfgksEQ4/D X-Google-Smtp-Source: AGHT+IH9+cwtJ5+MrPCuUDa0MLbs+WsaQlnwCZ0HHKZWzmhLSGT8Zjsd727z/nOkIQ9Cj+VgBH7fqg== X-Received: by 2002:a05:6a00:ad03:b0:776:1bb6:2194 with SMTP id d2e1a72fcca58-7761bb62263mr9071612b3a.16.1757908092457; Sun, 14 Sep 2025 20:48:12 -0700 (PDT) Received: from smtpclient.apple ([142.171.105.12]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-777b2074bcbsm2729226b3a.18.2025.09.14.20.48.10 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sun, 14 Sep 2025 20:48:11 -0700 (PDT) From: Chao Li Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_B4E742C1-55D1-47DD-AA23-A4D032417280" Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\)) Subject: Re: let ALTER TABLE DROP COLUMN drop whole-row referenced object Date: Mon, 15 Sep 2025 11:47:57 +0800 In-Reply-To: Cc: PostgreSQL-development To: jian he References: <2B70C6CA-606B-438C-8EF2-84DCBA1C0813@gmail.com> X-Mailer: Apple Mail (2.3826.700.81) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --Apple-Mail=_B4E742C1-55D1-47DD-AA23-A4D032417280 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > 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. >=20 > row security policy can have subquery, for example: > CREATE POLICY p1 ON document AS PERMISSIVE > USING (dlevel <=3D (SELECT seclv FROM uaccount WHERE pguser =3D = current_user)); >=20 > so I am still working on whole-row referenced policies interacting = with ALTER > TABLE SET DATA TYPE/ALTER TABLE DROP COLUMN. > = 1 - 0001 ``` + * ALTER TABLE DROP COLUMN also need drop indexes or constraints = that ``` Nit: need -> needs to 2 - 0001 ``` + tmpobject.classId =3D RelationRelationId; + tmpobject.objectId =3D RelationGetRelid(rel); + tmpobject.objectSubId =3D attnum; ``` Originally =E2=80=9Cobject=E2=80=9D points to the column to delete, but = with is patch, you are using =E2=80=9Cobject=E2=80=9D for = index/constrain to delete and =E2=80=9Ctmpobject=E2=80=9D for the column = to delete, which could be misleading. I=E2=80=99d suggest keep the meaning of =E2=80=9Cobject=E2=80=9D = unchanged, you need to pull up of initialization of =E2=80=9Cobject=E2=80=9D= to the place now you initiate =E2=80=9Ctmpobject=E2=80=9D. And only = define =E2=80=9Ctmpobject=E2=80=9D in sections where it is needed. So = you can name it =E2=80=9CidxObject=E2=80=9D or =E2=80=9CconsObject=E2=80=9D= , which will be more clearly meaning. I think you may also rename = =E2=80=9Cobject=E2=80=9D to =E2=80=9CcolObject=E2=80=9D. 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 = =E2=80=9CrecordDependencyOn()=E2=80=9D needs CommandCounterIncrement(). = I searched over all places when =E2=80=9CrecordDependencyOn()=E2=80=9D = is called, I don=E2=80=99t see CommandCounterIncrement() is called. 4 - 0001 ``` + if (!heap_attisnull(indexTuple, Anum_pg_index_indpred, = NULL)) + { =E2=80=A6. + } + + if (!found_whole_row && !heap_attisnull(indexTuple, = Anum_pg_index_indexprs, NULL)) + { =E2=80=A6 + } ``` 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 =C2=B7=C2=B7=C2=B7 + conscan =3D systable_beginscan(conDesc, = ConstraintRelidTypidNameIndexId, true, + = NULL, 3, skey); + if (!HeapTupleIsValid(contuple =3D = systable_getnext(conscan))) + elog(ERROR, "constraint \"%s\" = of relation \"%s\" does not exist", + constr_name, = RelationGetRelationName(rel)); =C2=B7=C2=B7=C2=B7 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 = =E2=80=9Cbecause constraint =E2=80=9Cxx=E2=80=9D uses whole-row type".=20= Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ --Apple-Mail=_B4E742C1-55D1-47DD-AA23-A4D032417280 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
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 <=3D (SELECT seclv FROM uaccount WHERE = pguser =3D 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-whe= n-wholerow-referenced-cons.patch><v3-0001-ALTER-TABLE-DROP-COLUMN-drop-wholerow-r= eferenced-object.patch>

<= /div>

1 - 0001
```
+ * ALTER = TABLE DROP COLUMN also need drop indexes or constraints = that
```

Nit: need -> needs = to

2 - 0001
```
+ = tmpobject.classId =3D RelationRelationId;
+ = tmpobject.objectId =3D RelationGetRelid(rel);
+ = tmpobject.objectSubId =3D = attnum;
```

Originally = =E2=80=9Cobject=E2=80=9D points to the column to delete, but with is = patch, you are using =E2=80=9Cobject=E2=80=9D for index/constrain to = delete and =E2=80=9Ctmpobject=E2=80=9D for the column to delete, which = could be misleading.

I=E2=80=99d suggest keep = the meaning of =E2=80=9Cobject=E2=80=9D unchanged, you need to pull up = of initialization of =E2=80=9Cobject=E2=80=9D to the place now you = initiate =E2=80=9Ctmpobject=E2=80=9D. And only define =E2=80=9Ctmpobject=E2= =80=9D in sections where it is needed. So you can name it = =E2=80=9CidxObject=E2=80=9D or =E2=80=9CconsObject=E2=80=9D, which will = be more clearly meaning. I think you may also rename =E2=80=9Cobject=E2=80= =9D to =E2=80=9CcolObject=E2=80=9D.

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 = =E2=80=9CrecordDependencyOn()=E2=80=9D needs CommandCounterIncrement(). = I searched over all places when =E2=80=9CrecordDependencyOn()=E2=80=9D = is called, I don=E2=80=99t see CommandCounterIncrement() is = called.

4 - = 0001
```
+ if = (!heap_attisnull(indexTuple, Anum_pg_index_indpred, = NULL))
+ {
  =                 =  =E2=80=A6.
+ = }
+
+ if (!found_whole_row = && !heap_attisnull(indexTuple, Anum_pg_index_indexprs, = NULL))
+ {
  =                 =  =E2=80=A6
+ = }
```

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
=C2=B7=C2=B7=C2=B7
+ = conscan =3D systable_beginscan(conDesc, = ConstraintRelidTypidNameIndexId, true,
+ = NULL, 3, = skey);
+ if = (!HeapTupleIsValid(contuple =3D = systable_getnext(conscan)))
+ = elog(ERROR, "constraint \"%s\" of relation \"%s\" does not = exist",
+ = constr_name, = RelationGetRelationName(rel));
=C2=B7=C2=B7=C2=B7

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 = =E2=80=9Cbecause constraint =E2=80=9Cxx=E2=80=9D uses whole-row = type". 

Best regards,
--
Chao Li (Evan)
HighGo Software = Co., = Ltd.
https://www.highgo.com/

= --Apple-Mail=_B4E742C1-55D1-47DD-AA23-A4D032417280--