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
FROMpg_class WHERE oid = 'test'::regclass);ALTER TABLE test ALTER name TYPE varchar(65535);SELECT oldnode <>
relfilenodeAS rewrittenFROM 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.
