Re: ALTER TABLE ... ALTER COLUMN ... SET DISTINCT - Mailing list pgsql-hackers

On Fri, Jul 17, 2009 at 11:10 AM, Dimitri Fontaine<dfontaine@hi-> I
couldn't apply it to current head because of bitrot. It applies fine
to
> git commit bcaef8b5a0e2d5c143dabd8516090a09e39b27b8 [1] but

This is one of the things that I hate about the requirement to post
context diffs: filterdiff, at least for me, strips out the git tags
that indicate the base rev of the patch.  In fact that rev is BEFORE
the one I based the patch on, which was
64b2da3f7778bc6fd3d213ceb9e73ff3b6e03890, and it actually applies OK
up until 0e3abe7ec78a3d51032d684598f188b0b0304fe9, the commit
immediately preceding the 8.4 pgindent run.

> 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

Ah, good catch.

>  - 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.

Hmm, I didn't know that (my man page doesn't contain that language).
It looks like strtod() is more widely used in the existing source code
than atof(), so I'll change it (atof() is however used in the
floatVal() macro).

> 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));

Not a good plan.  In my experience, gcc doesn't like switch ()
statements over enums that don't list all the values, unless they have
a default clause; it masterminds by giving you a warning that you've
"inadvertently" left out some values.

> 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).

OK.  My one concern about updating this is that Peter is currently
reviewing my patch to autogenerate headers & bki stuff.  If that gets
committed it's going to break this all again, and I'd rather just fix
it once (especially since that patch would reduce the amount of fixing
needed here by 50%).  Also if this gets applied after being fixed it
will break that one.

...Robert