Re: Identifying no-op length coercions

On Tue, Jun 21, 2011 at 06:31:44PM +0300, Alexey Klyukin wrote:
> Here is my review of this patch.

Thanks!

> The patch applies cleanly to the HEAD, produces no additional warnings. It
> doesn't include additional regression tests. One can include a test, using the
> commands like the ones included below.
>
> Changes to the documentation are limited to the a description of a new
> pg_class attribute. It would probably make sense to document all the
> exceptions to the table's rewrite on ALTER TABLE documentation page, although
> it could wait for more such exceptions.

I like the current level of detail in the ALTER TABLE page. Having EXPLAIN
ALTER TABLE would help here, but that's a bigger project than this one.

> postgres=# alter table test alter column name type varchar(10);
> ALTER TABLE
> postgres=# select relfilenode from pg_class where oid='test'::regclass;
> relfilenode
> -------------
> 66308
> (1 row)
> postgres=# alter table test alter column name type varchar(65535);
> ALTER TABLE
> postgres=# select relfilenode from pg_class where oid='test'::regclass;
> relfilenode
> -------------
> 66308
> (1 row)

A pg_regress test needs stable output, so we would do it roughly like this:

CREATE TEMP TABLE relstorage AS SELECT 0::regclass AS oldnode;
...
UPDATE relstorage SET oldnode =
(SELECT relfilenode FROM pg_class WHERE oid = 'test'::regclass);
ALTER TABLE test ALTER name TYPE varchar(65535);
SELECT oldnode <> relfilenode AS rewritten
FROM pg_class, relstorage WHERE oid = 'test'::regclass;

I originally rejected that as too ugly to read. Perhaps not.

> The only nitpick code-wise is these lines in varchar_transform:
>
> + int32 old_max = exprTypmod(source) - VARHDRSZ;
> + int32 new_max = new_typmod - VARHDRSZ;
>
> I have a hard time understanding why VARHDRSZ is subtracted here, so I'd assume that's a bug.

We track the varchar typmod internally as (max length) + VARHDRSZ.