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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers