Re: ALTER TABLE ... ALTER COLUMN ... SET DISTINCT

Hi,

Le 3 mai 09 à 22:13, Robert Haas a écrit :
> OK, new version of patch, this time with the weird scaling removed and
> the datatype changed to float4.

You've been assigning me this patch review, so here it goes :)

> I have not changed the minimum value for remoteVersion in pg_dump.c,
> as that would make the patch not able to be tested now. So that line
> and the comment two lines following will need to be updated prior to
> application. Also requires catversion bump.

I guess now would be a good time to fix this part of the patch?

I couldn't apply it to current head because of bitrot. It applies fine
to git commit bcaef8b5a0e2d5c143dabd8516090a09e39b27b8 [1] but from
there the automatic forward merge of git isn't able to merge
pg_attribute.h. As I don't have as much time to give to the review as
I'd like, I'd very much welcome if you could fix this part of your
patch and I'll resume my work thereafter.
I'll change the patch status to "Waiting on Author" as soon as I'll
have this mail id.

Now I've had time to read the code, here are my raw notes:

pg_dump.c:
tbinfo->attstattarget[j] = atoi(PQgetvalue(res, j,
i_attstattarget));
+ tbinfo->attdistinct[j] = strdup(PQgetvalue(res, j,
i_attdistinct));
...
+ if (atof(tbinfo->attdistinct[j]) != 0 &&
+ !tbinfo->attisdropped[j])

- style issue, convert at PQgetvalue() time

- prefer strtod() over atof? Here's what my local man page has to
say about the case:

The atof() function has been deprecated by strtod() and should
not be used in
new code.

tablecmds.c:
+ switch (nodeTag(newValue))
+ {
+ case T_Integer:
...
+ case T_Float:
...
+ default:
+ elog(ERROR, "unrecognized node type: %d",
+ (int) nodeTag(newValue));

What about adding the following before the switch, to do like
surrounding code?
Assert(IsA(newValue, Integer) || IsA(newValue, Float));

Given your revised version I'll try to play around with ndistinct
behavior and exercise dump and restore, etc, but for now I'll pause my
work.

I guess I'll have a second look at the code to check that it's all in
the spirit of surrounding code, which I didn't complete yet (wanted to
exercise my abilities to apply the patch from a past commit and
forward-merge from there).

Regards,
--
dim

[1]: http://git.postgresql.org/gitweb?p=postgresql.git;a=commit;h=bcaef8b5a0e2d5c143dabd8516090a09e39b27b8