Re: disallow alter individual column if partition key contains wholerow reference

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