Thread: default attstattarget
Hi all, The attached patch implements the pg_attribute.attstattarget scheme discussed on -hackers today. A value of "-1" in this column indicates that ANALYZE should use the compiled-in default value. This allows pg_dump to record and restore any changes made by the DBA to this value using ALTER TABLE ALTER COLUMN SET STATISTICS. I made another small behavioral change: IMHO, silently changing stat target values we deem inappropriate (< 0, > 1000) without warning the user is a bad idea. I changed the < 0 case to be a elog(ERROR) and added an elog(WARNING) for the > 1000, and documented the upper bound. Unless anyone sees any problems, please apply. Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
Attachment
nconway@klamath.dyndns.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
On Sat, Jul 20, 2002 at 01:18:22AM -0400, Tom Lane wrote: > 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. I'm indifferent about SET DEFAULT, so I removed it. > Otherwise it looks reasonable ... Thanks for the code review. A revised patch incorporating Tom's suggestions is attached. Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
Attachment
Your patch has been added to the PostgreSQL unapplied patches list at:
http://candle.pha.pa.us/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
---------------------------------------------------------------------------
Neil Conway wrote:
> On Sat, Jul 20, 2002 at 01:18:22AM -0400, Tom Lane wrote:
> > 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.
>
> I'm indifferent about SET DEFAULT, so I removed it.
>
> > Otherwise it looks reasonable ...
>
> Thanks for the code review.
>
> A revised patch incorporating Tom's suggestions is attached.
>
> Cheers,
>
> Neil
>
> --
> Neil Conway <neilconway@rogers.com>
> PGP Key ID: DB3C29FC
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Tom Lane will apply this as part of the DROP COLUMN merging. --------------------------------------------------------------------------- Neil Conway wrote: > Hi all, > > The attached patch implements the pg_attribute.attstattarget scheme > discussed on -hackers today. A value of "-1" in this column indicates > that ANALYZE should use the compiled-in default value. This allows > pg_dump to record and restore any changes made by the DBA to this > value using ALTER TABLE ALTER COLUMN SET STATISTICS. > > I made another small behavioral change: IMHO, silently changing > stat target values we deem inappropriate (< 0, > 1000) without > warning the user is a bad idea. I changed the < 0 case to be a > elog(ERROR) and added an elog(WARNING) for the > 1000, and > documented the upper bound. > > Unless anyone sees any problems, please apply. > > Cheers, > > Neil > > -- > Neil Conway <neilconway@rogers.com> > PGP Key ID: DB3C29FC [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 2: you can get off all lists at once with the unregister command > (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Actually, Tom will be applying this version of the patch. --------------------------------------------------------------------------- Neil Conway wrote: > On Sat, Jul 20, 2002 at 01:18:22AM -0400, Tom Lane wrote: > > 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. > > I'm indifferent about SET DEFAULT, so I removed it. > > > Otherwise it looks reasonable ... > > Thanks for the code review. > > A revised patch incorporating Tom's suggestions is attached. > > Cheers, > > Neil > > -- > Neil Conway <neilconway@rogers.com> > PGP Key ID: DB3C29FC [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
nconway@klamath.dyndns.org (Neil Conway) writes:
> A revised patch incorporating Tom's suggestions is attached.
Patch applied. It occurred to me that since DEFAULT_ATTSTATTARGET
wasn't being embedded into initdb data anymore, there wasn't any reason
why it had to be frozen at configure time. So it's history and there's
a GUC variable instead. Otherwise I took the patch pretty much as-is.
(I needed to get this in now because it would have failed to merge after
Chris' DROP COLUMN patch ... which is next on the queue.)
regards, tom lane