Re: ANALYZE patch for review

Hi Tom,

> -----Original Message-----
> From: Tom Lane [mailto:tgl(at)sss(dot)pgh(dot)pa(dot)us]
> Sent: 02 February 2004 16:54
> To: Mark Cave-Ayland
> Cc: pgsql-patches(at)postgresql(dot)org
> Subject: Re: [PATCHES] ANALYZE patch for review
>
>
> "Mark Cave-Ayland" <m(dot)cave-ayland(at)webbased(dot)co(dot)uk> writes:
> > So I'd like to propose a slightly different solution. I think that
> > examine_attribute() should return a pointer to a custom structure
> > containing any information that needs to be passed to the datatype
> > specific routine (not the entire VacAttrStats structure),
> or NULL if
> > the column should not be analyzed.
>
> Just a void* you mean? Sure, we could do that, although it
> might result in some duplicated effort. Another possibility
> is that analyze.c goes ahead and creates a VacAttrStats
> struct (including a void* extension pointer that it
> initializes to NULL) and then passes the struct to
> examine_attribute, which returns a bool and optionally
> modifies fields in the VacAttrStats struct --- in particular
> the requested-row-count and extension pointer. If false is
> returned then analyze.c just throws away the VacAttrStats
> struct instead of including it in its to-do list.

Yup indeed. Please find enclosed the latest version of the analyze patch
taking into account all the things we have discussed in the thread. It
feels a lot cleaner than the other - the advantage of having
analyze_rel() create the VacAttrStats structure is that you can put some
vaguely sensible defaults in the user-defined function so it shouldn't
bomb too badly if someone gets it wrong, plus the attribute and type
information can be fed in automagically.

This rewrite has the benefit that it gets rid of the duplication of
routines having to get their own copies of the attribute and type rows
based on attnum, and also the (void *) pointer gives a clear abstraction
of the data required for the pg_statistic table entry and those that are
used by the current default routine. I also altered the
examine_attribute() to allow the user-defined function to check
attstattarget for the column manually (in case they want to do something
special with minus or zero values), and also as discussed the internal
stats routine is called directly without requiring an entry in the
catalog.

> >> If you suppose that the "major" field is the upper bits of
> >> the statistics ID value, then this is just a slightly
> >> different way of thinking about the range-based allocation
> >> method I suggested before.
>
> > I was thinking perhaps in terms of an extra staowner int2 field in
> > pg_statistic where the IDs are allocated by the PGDG.
>
> I do not really want to add a field to pg_statistic. That
> complicates matters more than we have a justification to do.
> Nor do we have any reason at this point to think that we need
> a 2^32 namespace for statistics kinds. (If 2^16 ever starts
> to look cramped, we can just widen the fields to int4 without
> introducing any backwards compatibility issues --- existing
> code assignments will remain valid. But I find it hard to
> foresee that happening.)

OK, I've left this as it is at the moment.

Cheers,

Mark.

---

Mark Cave-Ayland
Webbased Ltd.
Tamar Science Park
Derriford
Plymouth
PL6 8BX
England

Tel: +44 (0)1752 764445
Fax: +44 (0)1752 764446

This email and any attachments are confidential to the intended
recipient and may also be privileged. If you are not the intended
recipient please delete it from your system and notify the sender. You
should not copy it or use it for any purpose nor disclose or distribute
its contents to any other person.