Re: default attstattarget

nconway(at)klamath(dot)dyndns(dot)org (Neil Conway) writes:
> /* Don't analyze column if user has specified not to */
> ! if (attr->attstattarget <= 0)
> return NULL;

> /* If column has no "=" operator, we can't do much of anything */
> --- 384,390 ----
> VacAttrStats *stats;

> /* Don't analyze column if user has specified not to */
> ! if (attr->attstattarget == 0)
> return NULL;

followed by

> + /* If the attstattarget column is set to "-1", use the default value */
> + if (stats->attr->attstattarget == -1)
> + stats->attr->attstattarget = DEFAULT_ATTSTATTARGET;
> +
> /* Is there a "<" operator with suitable semantics? */

As written, this code will crash (or at least behave very undesirably)
if attstattarget < -1 --- a situation easily created with manual update
of attstattarget, regardless of what you may try to enforce in ALTER
TABLE. I suggest making the second test be

+ /* If the attstattarget column is negative, use the default value */
+ if (stats->attr->attstattarget < 0)
+ stats->attr->attstattarget = DEFAULT_ATTSTATTARGET;

which is bulletproof at the same or less cost as not being bulletproof.
The same sort of change should be made in pg_dump (ignore any negative
attstattarget, not only -1).

The modified pg_dump will fail on pre-7.2 databases, because you
neglected to add a dummy attstattarget result for the pg_attribute
select commands used in pre-7.2 cases. You need a "-1 as attstattarget"
in there.

I'm unexcited about the proposed gram.y changes, especially since you
didn't document them. I do not think we need a SET DEFAULT variant;
anyone bright enough to be toying with attstattarget at all can figure
out how to revert it to -1 if they want.

Otherwise it looks reasonable ...

regards, tom lane