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.