Re: Identifying no-op length coercions - Mailing list pgsql-hackers

Hi,

On Jun 19, 2011, at 2:10 PM, Noah Misch wrote:

> On Sat, Jun 18, 2011 at 11:32:20PM -0400, Robert Haas wrote:
>> On Sat, Jun 18, 2011 at 11:12 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> On Sat, Jun 18, 2011 at 11:06 PM, Noah Misch <noah@leadboat.com> wrote:
>>>> On Sat, Jun 18, 2011 at 10:57:13PM -0400, Robert Haas wrote:
>>>>> On Sat, Jun 11, 2011 at 5:13 PM, Noah Misch <noah@leadboat.com> wrote:
>>>>>> Sounds good. ?Updated patch attached, incorporating your ideas. ?Before applying
>>>>>> it, run this command to update the uninvolved pg_proc.h DATA entries:
>>>>>> ?perl -pi -e 's/PGUID(\s+\d+){4}/$& 0/' src/include/catalog/pg_proc.h
>>>>>
>>>>> This doesn't quite apply any more. ?I think the pgindent run broke it slightly.
>>>>
>>>> Hmm, I just get two one-line offsets when applying it to current master. ?Note
>>>> that you need to run the perl invocation before applying the patch. ?Could you
>>>> provide full output of your `patch' invocation, along with any reject files?
>>>
>>> Ah, crap. ?You're right. ?I didn't follow your directions for how to
>>> apply the patch. ?Sorry.
>>
>> I think you need to update the comment in simplify_function() to say
>> that we have three strategies, rather than two.  I think it would also
>> be appropriate to add a longish comment just before the test that
>> calls protransform, explaining what the charter of that function is
>> and why the mechanism exists.
>
> Good idea.  See attached.
>
>> Documentation issues aside, I see very little not to like about this.
>
> Great!  Thanks for reviewing.
> <noop-length-coercion-v3.patch>


Here is my review of this patch.

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.

The feature works as intended and I haven't noticed any crashes or assertion failures, i.e.

postgres=# create table test(id integer, name varchar);
CREATE TABLE
postgres=# insert into test values(1, 'test');
INSERT 0 1
postgres=# select relfilenode from pg_class where oid='test'::regclass;relfilenode
-------------      66302
(1 row)
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)

The code looks good and takes into account all the previous suggestions.

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.

Other than that, I haven't noticed any issues w/ this patch.

Sincerely,
Alexey.

--
Command Prompt, Inc.                              http://www.CommandPrompt.com
PostgreSQL Replication, Consulting, Custom Development, 24x7 support