default attstattarget

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

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