Обсуждение: disallow alter individual column if partition key contains wholerow reference

hi.

while reviewing disallow generated column as partition key in [1],
I found out this bug.

demo:
drop table if exists t4;
CREATE TABLE t4(f1 int, f2 bigint) PARTITION BY list ((t4));
create table t4_1 partition of t4 for values in ((1,2));
alter table t4 alter column f2 set data type text using f2;

insert into t4 select 1, '2';
ERROR: invalid memory alloc request size 18446744073709551615

turns out the fix seems pretty simple, mainly on has_partition_attrs.
has_partition_attrs is used to
Checks if any of the 'attnums' is a partition key attribute for rel.
if partition keys have column references, then has_partition_attrs
should return true.

[1]: https://postgr.es/m/CACJufxF=WDGthXSAQr9thYUsfx_1_t9E6N8tE3B8EqXcVoVfQw@mail.gmail.com

On Tue, Apr 22, 2025 at 7:39 PM jian he <jian.universality@gmail.com> wrote:
>
> demo:
> drop table if exists t4;
> CREATE TABLE t4(f1 int, f2 bigint) PARTITION BY list ((t4));
> create table t4_1 partition of t4 for values in ((1,2));
> alter table t4 alter column f2 set data type text using f2;
>
> insert into t4 select 1, '2';
> ERROR: invalid memory alloc request size 18446744073709551615
>
> turns out the fix seems pretty simple, mainly on has_partition_attrs.
> has_partition_attrs is used to
> Checks if any of the 'attnums' is a partition key attribute for rel.
> if partition keys have column references, then has_partition_attrs
> should return true.
>

hi.
minor comments changes,
also add it on commitfest (https://commitfest.postgresql.org/patch/5988)

On Mon, Aug 25, 2025 at 11:58 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
> I tested this patch with “partition by range”, it works for me.
>
> Just have a few small comments:
>
> +                       if (bms_is_member(0 - FirstLowInvalidHeapAttributeNumber, expr_attrs))
>
> Can we simply check “if (Var *)expr->varno == 1 && (Var *) expr->varattno == 0”, which seems more direct?
>
hi.

in has_partition_attrs, we have:
if (partattno != 0)
{
}
else
{
            /* Arbitrary expression */
            Node       *expr = (Node *) lfirst(partexprs_item);
            Bitmapset  *expr_attrs = NULL;
            /* Find all attributes referenced */
            pull_varattnos(expr, 1, &expr_attrs);
}

see comments "   /* Arbitrary expression */"
after pull_varattnos, we can not assume "expr" is a Var node?


> +                       /*
> +                        * If partition expression contains wholerow reference, then any
> +                        * column is indirect part of the expression now.  unconditionally
> +                        * set used_in_expr to true.
> +                       */
>
> For the comment, a tiny enhancement:
>
> /*
>  * If the partition expression contains a whole-row reference, then every
>  * column is implicitly part of the expression. Set used_in_expr to true
>  * unconditionally.
>  */
>
Thanks, your comments are better than mine.

Hi Jian,

I built postgres on commit 879c49 and verified that these commands
produce the error as described:

=# drop table if exists t4;
NOTICE:  table "t4" does not exist, skipping
DROP TABLE
=# CREATE TABLE t4(f1 int, f2 bigint) PARTITION BY list ((t4));
CREATE TABLE
=# create table t4_1 partition of t4 for values in ((1,2));
CREATE TABLE
=# alter table t4 alter column f2 set data type text using f2;
ALTER TABLE
=# insert into t4 select 1, '2';
ERROR:  invalid memory alloc request size 18446744073709551615

I also verified that after applying
v2-0001-fix-wholerow-as-partition-key-reference.patch, the behavior
changed:

=# drop table if exists t4;
NOTICE:  table "t4" does not exist, skipping
DROP TABLE
=# CREATE TABLE t4(f1 int, f2 bigint) PARTITION BY list ((t4));
CREATE TABLE
=# create table t4_1 partition of t4 for values in ((1,2));
CREATE TABLE
=# alter table t4 alter column f2 set data type text using f2;
ERROR:  cannot alter column "f2" because it is part of the partition
key of relation "t4"
LINE 1: alter table t4 alter column f2 set data type text using f2;

I agree that this patch matches the spirit of has_partition_attrs: the
answer to the question "is column x part of the wholerow reference"
should always be "yes".

To try to understand the history of this issue, I did a git bisect and
found that the behavior of these commands changed at these commits:

- After commit f0e447 - "Implement table partitioning", wholerow
variables are forbidden in partitioning expressions.

- After commit bb4114 - "Allow whole-row Vars to be used in
partitioning expressions", wholerow variables are permitted in
partitioning expresisions, but the commands above trigger a segfault.

- After commit d87d54 - "Refactor to add pg_strcoll(), pg_strxfrm(),
and variants", the commands above no longer trigger a segfault, and
instead they present the "invalid memory alloc request size" error
described earlier in this thread.

I suspect that the specific error presented may depend on what happens
when the bits representing values of the old type are interpreted as
values of the new type. I tried with a few different types and got a few
different errors, and in some cases no error at all.

> > +if (bms_is_member(0 - FirstLowInvalidHeapAttributeNumber, expr_attrs))
> >
> > Can we simply check “if (Var *)expr->varno == 1 && (Var *) expr->varattno == 0”, which seems more direct?
> ...
> after pull_varattnos, we can not assume "expr" is a Var node?

This argument sounds convincing to me, but I'm unfamiliar with the
details. I found `bms_is_member(0 - FirstLowInvalidHeapAttributeNumber, _)`
a little confusing, but this pattern is used in a few other places
(deparse.c, allpaths.c). The comment helps make it clear that we're
working with a wholerow reference.

Best,
Matt Dailis

Hi all

On Fri, 26 Sept 2025 at 23:46, Matt Dailis <dwehttam@gmail.com> wrote:
>
> Hi Jian,
>
> I built postgres on commit 879c49 and verified that these commands
> produce the error as described:
>
> =# drop table if exists t4;
> NOTICE:  table "t4" does not exist, skipping
> DROP TABLE
> =# CREATE TABLE t4(f1 int, f2 bigint) PARTITION BY list ((t4));
> CREATE TABLE
> =# create table t4_1 partition of t4 for values in ((1,2));
> CREATE TABLE
> =# alter table t4 alter column f2 set data type text using f2;
> ALTER TABLE
> =# insert into t4 select 1, '2';
> ERROR:  invalid memory alloc request size 18446744073709551615
>
> I also verified that after applying
> v2-0001-fix-wholerow-as-partition-key-reference.patch, the behavior
> changed:
>
> =# drop table if exists t4;
> NOTICE:  table "t4" does not exist, skipping
> DROP TABLE
> =# CREATE TABLE t4(f1 int, f2 bigint) PARTITION BY list ((t4));
> CREATE TABLE
> =# create table t4_1 partition of t4 for values in ((1,2));
> CREATE TABLE
> =# alter table t4 alter column f2 set data type text using f2;
> ERROR:  cannot alter column "f2" because it is part of the partition
> key of relation "t4"
> LINE 1: alter table t4 alter column f2 set data type text using f2;
>
> I agree that this patch matches the spirit of has_partition_attrs: the
> answer to the question "is column x part of the wholerow reference"
> should always be "yes".
>
> To try to understand the history of this issue, I did a git bisect and
> found that the behavior of these commands changed at these commits:
>
> - After commit f0e447 - "Implement table partitioning", wholerow
> variables are forbidden in partitioning expressions.
>
> - After commit bb4114 - "Allow whole-row Vars to be used in
> partitioning expressions", wholerow variables are permitted in
> partitioning expresisions, but the commands above trigger a segfault.
>
> - After commit d87d54 - "Refactor to add pg_strcoll(), pg_strxfrm(),
> and variants", the commands above no longer trigger a segfault, and
> instead they present the "invalid memory alloc request size" error
> described earlier in this thread.
>
> I suspect that the specific error presented may depend on what happens
> when the bits representing values of the old type are interpreted as
> values of the new type. I tried with a few different types and got a few
> different errors, and in some cases no error at all.

Thanks for archaeology here! Looks like this is broken in HEAD for a while.

> > > +if (bms_is_member(0 - FirstLowInvalidHeapAttributeNumber, expr_attrs))
> > >
> > > Can we simply check “if (Var *)expr->varno == 1 && (Var *) expr->varattno == 0”, which seems more direct?
> > ...
> > after pull_varattnos, we can not assume "expr" is a Var node?
>
> This argument sounds convincing to me, but I'm unfamiliar with the
> details. I found `bms_is_member(0 - FirstLowInvalidHeapAttributeNumber, _)`
> a little confusing, but this pattern is used in a few other places
> (deparse.c, allpaths.c). The comment helps make it clear that we're
> working with a wholerow reference.
>

I can see that 0 - FirstLowInvalidHeapAttributeNumber is an existing
coding practise in sources, but so is
InvalidAttrNumber - FirstLowInvalidHeapAttributeNumber (sepgsql
contrib extension). I'm voting for the second one, even though it is
less used.

--
Best regards,
Kirill Reshke

On Tue, Oct 28, 2025 at 11:24 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
>
> I can see that 0 - FirstLowInvalidHeapAttributeNumber is an existing
> coding practise in sources, but so is
> InvalidAttrNumber - FirstLowInvalidHeapAttributeNumber (sepgsql
> contrib extension). I'm voting for the second one, even though it is
> less used.
>

hi.

now it's:
+            /*
+             * If the partition expression contains a whole-row reference,
+             * then all columns are indirectly associated with that
+             * expression.
+             */
+            if (bms_is_member(InvalidAttrNumber -
FirstLowInvalidHeapAttributeNumber,
+                              expr_attrs))
+            {
+                if (used_in_expr)
+                    *used_in_expr = true;
+                return true;
+            }

also polished the commit message. below is the commit message:
--------------------------
For partition key expressions containing whole-row reference, we cannot rely on
pg_depend lookups to determine whether an individual column can be altered
(drop, set data type).
As noted in the comments for find_expr_references_walker, no dependency entries
are recorded for whole-row expressions. Therefore whole-row reference check is
needed in has_partition_attrs.

Partition key expressions contain whole-row reference, it is effectively
equivalent to all user columns being indirectly associated with that expression.
Since columns that in a partition key cannot be altered in any way, the same
restriction should be applied to whole-row reference expressions in the
partition key.
--------------------------


--
jian
https://www.enterprisedb.com/