ANALYZE patch for review

"Mark Cave-Ayland" <m.cave-ayland@webbased.co.uk> writes:

Here is a first attempt at a patch to allow a customised ANALYZE
function to be specified for user-defined types, relating to the
following two threads:
http://archives.postgresql.org/pgsql-hackers/2003-10/msg00113.php and
http://archives.postgresql.org/pgsql-hackers/2004-01/msg00236.php

This seems to be going in mostly the right direction, but I think you've
made a few errors.

One is that I think that the VacAttrStats structure should be palloc'd
by the type-specific typanalyze function, and not by general code. The
reason for this is that a type-specific function could allocate a larger
struct of which VacAttrStats is just the first field, leaving room for
additional fields that can communicate data to the later type-specific
analysis routine (which would know that it is receiving this type and
not just generic VacAttrStats). This is already useful for the standard
analysis code --- it would want to keep eqopr, eqfunc, and ltopr there,
so as to avoid the extra lookups required in your patch. Other
datatypes would likely need comparable functionality.

This would mean that pretty much the whole of examine_attribute is
treated as type-specific code. In consequence there would be a certain
amount of duplication of code across different type-specific setup
routines, but that does not bother me.

Also, I'd suggest that it would be better to define a zero in
pg_type.typanalyze as selecting the default analysis routine.
This would put the special case in just one place rather than
having that knowledge all through pg_type.h plus CreateType.
(If you do this, I think the default analysis routine wouldn't
even need a pg_proc entry.)

The user-defined function uses a new STATISTICS_KIND_CUSTOM
constant to hold the data as I am not 100% sure how the planner
interprets the
STATISTIC_KIND_* values in the pg_statistic table. My aim is to
be able to store
a 2D histogram so that spatial row count estimates can be used
by the planner
in PostGIS (http://postgis.refractions.net) without having to
maintain the
statistics by manually running a stored procedure. The
pg_statistic.h file doesn't
seem clear to me as to what the values of the various columns
should be when not
dealing with single one-dimensional histograms.....

Obviously we'd need to define more STATISTIC_KIND_xxx values to
represent any additional kinds of statistics that are getting stored.
"STATISTICS_KIND_CUSTOM" is exactly the way *not* to do that, as it
would lead people to use the same ID code for different things.
What we probably need to do is think of a suitable scheme for reserving
STATISTIC_KIND codes for different uses. A really off-the-cuff
suggestion is:

1-100: reserved for assignment by the core Postgres project
100-199: reserved for assignment by PostGIS
200-9999: reserved for other globally-known stats kinds
10000-32767: reserved for private site-local use

People writing private datatypes would select random values above 10000
and have little chance of conflict. Anybody who wanted to write code
for public distribution would want to come to us and get an assignment
of space below 10000.

4) The VacAttrStats structure may need to be moved out to a
different .h file
for access by the programmer (currently it is in
commands/vacuum.h) - maybe a
new analyze.h would be better?

vacuum.h seems fine.

regards, tom lane

Hi Tom,

-----Original Message-----
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
Sent: 25 January 2004 23:08
To: Mark Cave-Ayland
Cc: pgsql-patches@postgresql.org
Subject: Re: [PATCHES] ANALYZE patch for review

"Mark Cave-Ayland" <m.cave-ayland@webbased.co.uk> writes:

Here is a first attempt at a patch to allow a customised ANALYZE
function to be specified for user-defined types, relating to the
following two threads:

http://archives.postgresql.org/pgsql-hackers/2003-10/msg00113.php and

http://archives.postgresql.org/pgsql-hackers/2004-01/msg00236.php

This seems to be going in mostly the right direction, but I
think you've made a few errors.

One is that I think that the VacAttrStats structure should be
palloc'd by the type-specific typanalyze function, and not by
general code. The reason for this is that a type-specific
function could allocate a larger struct of which VacAttrStats
is just the first field, leaving room for additional fields
that can communicate data to the later type-specific analysis
routine (which would know that it is receiving this type and
not just generic VacAttrStats). This is already useful for
the standard analysis code --- it would want to keep eqopr,
eqfunc, and ltopr there, so as to avoid the extra lookups
required in your patch. Other datatypes would likely need
comparable functionality.

This would mean that pretty much the whole of
examine_attribute is treated as type-specific code. In
consequence there would be a certain amount of duplication of
code across different type-specific setup routines, but that
does not bother me.

My thinking behind this was that examine_attribute decides whether a
column of a given type is analyzable so it made sense to provide the
user with the type and column information. The one thing I do want to
ensure is that the user function doesn't have to do things like check if
the column is dropped or if the stats target is zero - that should be
done by Postgres IMHO. I do like the idea of having a custom structure
passed between functions as like you say it makes things a lot more
flexible.... Will see what I can do about it.

Also, I'd suggest that it would be better to define a zero in
pg_type.typanalyze as selecting the default analysis routine.
This would put the special case in just one place rather than
having that knowledge all through pg_type.h plus CreateType.
(If you do this, I think the default analysis routine
wouldn't even need a pg_proc entry.)

I'll have a look at this too. The thing I really want to avoid is having
to create lots of 'special case' code depending upon whether the default
analysis routine is used or an external function. At the moment, the
logic just calls the typanalyze entry using OidFunctionCall1 regardless
which seems quite a neat solution.

The user-defined function uses a new STATISTICS_KIND_CUSTOM
constant to hold the data as I am not 100% sure how the planner
interprets the
STATISTIC_KIND_* values in the pg_statistic table. My

aim is to be

able to store
a 2D histogram so that spatial row count estimates can

be used by the

planner
in PostGIS (http://postgis.refractions.net) without having to
maintain the
statistics by manually running a stored procedure. The

pg_statistic.h

file doesn't
seem clear to me as to what the values of the various

columns should

be when not
dealing with single one-dimensional histograms.....

Obviously we'd need to define more STATISTIC_KIND_xxx values
to represent any additional kinds of statistics that are
getting stored. "STATISTICS_KIND_CUSTOM" is exactly the way
*not* to do that, as it would lead people to use the same ID
code for different things. What we probably need to do is
think of a suitable scheme for reserving STATISTIC_KIND codes
for different uses. A really off-the-cuff suggestion is:

1-100: reserved for assignment by the core Postgres project
100-199: reserved for assignment by PostGIS
200-9999: reserved for other globally-known stats kinds
10000-32767: reserved for private site-local use

People writing private datatypes would select random values
above 10000 and have little chance of conflict. Anybody who
wanted to write code for public distribution would want to
come to us and get an assignment of space below 10000.

Would it really be necessary to need to do this? My understanding would
be that using STATISTICS_KIND_CUSTOM merely marks the pg_statistic entry
as being valid, i.e. it is valid data but the row sta* contents can only
be interpreted by the specified type and operator combination, and
should not attempt to be interpreted by any of the standard estimation
functions.

Also in a selectivity estimator function would it be reasonable to call
get_attstatsslot(i.e. is it a relatively stable API for external as well
as internal use) since this function does a lot of the work that would
be required by an estimation function?

4) The VacAttrStats structure may need to be moved out

to a different

.h file
for access by the programmer (currently it is in
commands/vacuum.h) - maybe a
new analyze.h would be better?

vacuum.h seems fine.

OK, I'll leave it where it is.

Many thanks,

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.

"Mark Cave-Ayland" <m.cave-ayland@webbased.co.uk> writes:

This would mean that pretty much the whole of
examine_attribute is treated as type-specific code. In
consequence there would be a certain amount of duplication of
code across different type-specific setup routines, but that
does not bother me.

My thinking behind this was that examine_attribute decides whether a
column of a given type is analyzable so it made sense to provide the
user with the type and column information. The one thing I do want to
ensure is that the user function doesn't have to do things like check if
the column is dropped or if the stats target is zero - that should be
done by Postgres IMHO.

[ shrug ... ] It's only a couple lines either way, and you could argue
that the present behavior for attstattarget = 0 should be subject to
customization anyway. But if you prefer to pass in the pg_attribute and
pg_type rows already fetched, I won't object.

I'll have a look at this too. The thing I really want to avoid is having
to create lots of 'special case' code depending upon whether the default
analysis routine is used or an external function. At the moment, the
logic just calls the typanalyze entry using OidFunctionCall1 regardless
which seems quite a neat solution.

My point was that there is going to be a 'special case' to provide the
default somewhere --- if not here, then in the CREATE TYPE code. It
seems to me that localizing knowledge of what the default behavior is
wouldn't be a bad idea.

What we probably need to do is
think of a suitable scheme for reserving STATISTIC_KIND codes
for different uses.

Would it really be necessary to need to do this? My understanding would
be that using STATISTICS_KIND_CUSTOM merely marks the pg_statistic entry
as being valid, i.e. it is valid data but the row sta* contents can only
be interpreted by the specified type and operator combination, and
should not attempt to be interpreted by any of the standard estimation
functions.

I don't much like that. It amounts to assuming that you will always be
able to guess from context what a STATISTICS_KIND_CUSTOM entry actually
is. I think that's shortsighted. As soon as there are a few different
custom estimation functions out there, they will be tripping over each
others' data. You may be thinking that only one kind of custom data
would actually exist for any one datatype, but I'd turn that around:
what of custom analysis functions that are supposed to work for a range
of datatypes? It'd be very unsafe for such functions to all use the
same STATISTICS_KIND_CUSTOM label. The operator selectivity functions
that need to use the data couldn't be sure what they were looking at.

Also in a selectivity estimator function would it be reasonable to call
get_attstatsslot(i.e. is it a relatively stable API for external as well
as internal use)

Yes, it's as stable as any other part of Postgres ... I don't see any
reason that an external selectivity estimator should be coded any
differently from the built-in ones.

regards, tom lane

Hi Tom,

-----Original Message-----
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
Sent: 27 January 2004 17:16
To: Mark Cave-Ayland
Cc: pgsql-patches@postgresql.org
Subject: Re: [PATCHES] ANALYZE patch for review

My thinking behind this was that examine_attribute decides

whether a

column of a given type is analyzable so it made sense to

provide the

user with the type and column information. The one thing I

do want to

ensure is that the user function doesn't have to do things

like check

if the column is dropped or if the stats target is zero -

that should

be done by Postgres IMHO.

[ shrug ... ] It's only a couple lines either way, and you
could argue that the present behavior for attstattarget = 0
should be subject to customization anyway. But if you prefer
to pass in the pg_attribute and pg_type rows already fetched,
I won't object.

OK you've won me over on the stats target. I can see where someone could
have a case where an operation could fail if no statistics are available
and hence they would want to intercept the case where the target is zero
to place in some initial data.

I have a question about something you wrote in your previous email (this
is more a C question than anything else):

One is that I think that the VacAttrStats structure should be
palloc'd by the type-specific typanalyze function, and not by
general code. The reason for this is that a type-specific
function could allocate a larger struct of which VacAttrStats
is just the first field, leaving room for additional fields
that can communicate data to the later type-specific analysis
routine (which would know that it is receiving this type and
not just generic VacAttrStats). This is already useful for
the standard analysis code --- it would want to keep eqopr,
eqfunc, and ltopr there, so as to avoid the extra lookups
required in your patch. Other datatypes would likely need
comparable functionality.

Is what you're saying that a custom function would do something like
this:

typedef struct
{
VacAttrStats *v;
int mydata1;
int mydata2;
int mydata3;
} mystruct;

myanalyzeinfo = palloc0(sizeof(mystruct));
..
..
return myanalyzeinfo;

This means that a structure of kind mystruct is returned back to
analyze_rel(), but analyze_rel() still wants to access some information
from VacAttrStats. Does that mean that the reference to VacAttrStats in
analyze_rel() would be derived something like this?

VacAttrStats *vacattrptr;
vacattrptr = (VacAttrStats *)myanalyzeinfo;
vacattrptr->minrows = 2;

Firstly, is this right? And secondly, if so, is there a way of arranging
the structures to ensure that the compiler throws a warning if
VacAttrStats is not the first element of the structure?

I'll have a look at this too. The thing I really want to avoid is
having to create lots of 'special case' code depending upon whether
the default analysis routine is used or an external

function. At the

moment, the logic just calls the typanalyze entry using
OidFunctionCall1 regardless which seems quite a neat solution.

My point was that there is going to be a 'special case' to
provide the default somewhere --- if not here, then in the
CREATE TYPE code. It seems to me that localizing knowledge
of what the default behavior is wouldn't be a bad idea.

I suppose the question here would be does having a default case in this
way make things harder for developers to understand/follow the code? If
it makes sense that a value of 0 would indicate the default statistics
routines (which would be called directly) and the localization still
maintains readability then I'm happy to go with this.

What we probably need to do is
think of a suitable scheme for reserving STATISTIC_KIND codes
for different uses.

Would it really be necessary to need to do this? My understanding
would be that using STATISTICS_KIND_CUSTOM merely marks the
pg_statistic entry as being valid, i.e. it is valid data

but the row

sta* contents can only be interpreted by the specified type and
operator combination, and should not attempt to be

interpreted by any

of the standard estimation functions.

I don't much like that. It amounts to assuming that you will
always be able to guess from context what a
STATISTICS_KIND_CUSTOM entry actually is. I think that's
shortsighted. As soon as there are a few different custom
estimation functions out there, they will be tripping over
each others' data. You may be thinking that only one kind of
custom data would actually exist for any one datatype, but
I'd turn that around: what of custom analysis functions that
are supposed to work for a range of datatypes? It'd be very
unsafe for such functions to all use the same
STATISTICS_KIND_CUSTOM label. The operator selectivity
functions that need to use the data couldn't be sure what
they were looking at.

I'm beginning to think that perhaps we're looking at this in the wrong
way, and that a more elegant version of what you're suggesting could be
implemented using a major/minor method of identifying a statistics type.

As an example, for existing types, the major would identify the owner of
the data type as being STATSOWNER_PGDG, while the minor would indicate
the algorithm, e.g. ALG_INDEX_SCALAR or ALG_INDEX_MCV. In the case of
PostGIS this would mean that the PGDG would only have to assign them a
STATSOWNER_POSTGIS and then they would be free to divide the minor to
indicate whatever type of algorithm they need, without having to
allocate chunks of potentially unused number space.

This also then brings up the idea that different types of statistics can
be kept on a given relation; for example index selectivity stats could
be one, where as another could be the number of times the letter 'e'
appears. Your example where a custom analysis function works on a range
of datatypes can be handled by the stats owners themselves, by
allocating a block of minor numbers for an algorithm where each entry
can correspond to a different data type as they require.

I suppose this also means that there might need to be a mechanism where
a custom analysis function would want to add more than one row to the
pg_statistic table (ie it collects more than one set of statistics per
column); I can see this being used to implement statistics keeping on
functional indexes for example, where for a column X, a copy is made and
f(X) calculated for each index and added to the pg_statistic table.

Also in a selectivity estimator function would it be reasonable to
call get_attstatsslot(i.e. is it a relatively stable API

for external

as well as internal use)

Yes, it's as stable as any other part of Postgres ... I don't
see any reason that an external selectivity estimator should
be coded any differently from the built-in ones.

OK, great. That makes life slightly easier.

Many thanks,

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.

"Mark Cave-Ayland" <m.cave-ayland@webbased.co.uk> writes:

Is what you're saying that a custom function would do something like
this:

typedef struct
{
VacAttrStats *v;
int mydata1;
int mydata2;
int mydata3;
} mystruct;

myanalyzeinfo = palloc0(sizeof(mystruct));
..
..
return myanalyzeinfo;

Exactly, except that the return would look like

return (VacAttrStruct *) myanalyzeinfo;

(or at least it would if you weren't converting to Datum anyway). The
main analyze code would think it had a plain pointer to VacAttrStats,
but the data analysis subroutine associated with this setup routine
would know it could cast the pointer back to (mystruct *).

VacAttrStats *vacattrptr;
vacattrptr = (VacAttrStats *)myanalyzeinfo;
vacattrptr->minrows = 2;

Seems like the hard way; you can just access myanalyzeinfo->v.minrows
when you're working with a pointer declared as mystruct *.

is there a way of arranging
the structures to ensure that the compiler throws a warning if
VacAttrStats is not the first element of the structure?

Not that I know of, but we use techniques like this in very many places
in the PG code and have not had troubles of that sort. I'm not too
concerned about that.

I'm beginning to think that perhaps we're looking at this in the wrong
way, and that a more elegant version of what you're suggesting could be
implemented using a major/minor method of identifying a statistics type.

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.
However, the range-based method can adapt to allocating different
amounts of identifier space to different owners, whereas a major/minor
approach can't easily do that since you've defined it to be 2^N minor
IDs for each major code.

I suppose this also means that there might need to be a mechanism where
a custom analysis function would want to add more than one row to the
pg_statistic table (ie it collects more than one set of statistics per
column);

As long as the custom analysis function is tied to a datatype, I don't
see the need for that. It already has the ability to define up to four
different "kinds" of stats for its column, and if someone makes a case
for wanting more, I'd be inclined to widen pg_statistic rather than get
into multiple rows per column. (To point out just one problem, such an
arrangement would break the unique index scheme presently used.) I'm
not sure yet about whether we need special hooks for keeping stats on
functional indexes, but if we do they'd surely not be looked up by
datatype, or at least not datatype alone.

regards, tom lane

Hi Tom,

-----Original Message-----
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
Sent: 29 January 2004 15:31
To: Mark Cave-Ayland
Cc: pgsql-patches@postgresql.org
Subject: Re: [PATCHES] ANALYZE patch for review

<lots cut about pointers>

OK, I've had another attempt at writing the code as you suggested but
the more I work on it the less I like it :(. What I would like to do is
make the VacAttrStats structure so that it just contains the information
that is updated in the pg_statistic table, however this fell apart when
I realised that update_attstats() suddenly requires the attr and
attrtype fields to be present. Doh.

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. I'm also considering changing the
examine_attribute() input parameters to be Relation, Attribute, Type for
the current column along with a pointer to a bool to indicate whether or
not the column should be analyzed or not.

If examine_attribute() sets the bool to false then the column is
ignored. If the bool is set to true then a VacAttrStats structure is
created in memory, and then the Attribute and Type tuple information is
copied into the VacAttrStats structure. A new field for VacAttrStats
will contain the pointer to the custom structure returned by
examine_attribute() which can then be passed into the compute_*_stats()
functions as an extra parameter.

This seems to achieve the aims of abstracting the statistics data from
the intermediate information required by the statistics routines,
allowing extra/custom data to be passed between the typanalyze function
and the statistics algorithm, and allowing the user to have the attr and
attrtype structures given to them. The only thing I don't really like
about this is providing a pointer to a bool in examine_attribute() -
however this is needed to distinguish from a NULL meaning 'I have no
custom data but the analyze function should still be called' and 'This
column should not be analyzed'. I can't think of a better solution at
the moment.

I'm beginning to think that perhaps we're looking at this

in the wrong

way, and that a more elegant version of what you're

suggesting could

be implemented using a major/minor method of identifying a

statistics

type.

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. However, the range-based method
can adapt to allocating different amounts of identifier space
to different owners, whereas a major/minor approach can't
easily do that since you've defined it to be 2^N minor IDs
for each major code.

I was thinking perhaps in terms of an extra staowner int2 field in
pg_statistic where the IDs are allocated by the PGDG. Then each
group/project would only require one owner id to be allocated to them
and then have the existing 2^16 stakind space to organise themselves.
The advantage of this is that projects can allocate their own stakind
fields, implementing new or improved statistic algorithms without having
to wait on the new allocation from the PGDG.

Many thanks,

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.

"Mark Cave-Ayland" <m.cave-ayland@webbased.co.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.

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

regards, tom lane

Hi Tom,

-----Original Message-----
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
Sent: 02 February 2004 16:54
To: Mark Cave-Ayland
Cc: pgsql-patches@postgresql.org
Subject: Re: [PATCHES] ANALYZE patch for review

"Mark Cave-Ayland" <m.cave-ayland@webbased.co.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.

Hi Tom,

Did you manage to find a spare moment over the past week to check the
revised ANALYZE patch I posted to the list? Is there any more work that
needs to be done to it before it can applied to CVS?

TIA,

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.

"Mark Cave-Ayland" <m.cave-ayland@webbased.co.uk> writes:

Yup indeed. Please find enclosed the latest version of the analyze patch
taking into account all the things we have discussed in the thread.

I've reviewed and applied this with some small changes. You did a good
job --- the only things you missed AFAICS were pg_dump support and
documentation.

I changed the API slightly for the typanalyze function and the
compute_stats subroutine. I don't think it's necessary or appropriate
to pass in the Relation pointer to typanalyze; that simplifies its
signature to a single INTERNAL parameter. (If we did want to pass the
Relation, I'd be inclined to make it a field in VacAttrStats.) Also
I tweaked compute_stats so that the attribute number of the column is
passed explicitly. I foresee needing this for functional-index stats
gathering --- it's likely that the accumulated sample rows will be built
on the fly and will have a tupdesc that includes both the main table
rows and the functional index columns. So compute_stats shouldn't
assume that the nominal column number is the right thing to use to
extract the column it wants.

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

As I've committed it, the convention that zero means "no stats" is
enforced by examine_attribute(), but the typanalyze function is
responsible for deciding what a negative value means. Seem reasonable?

Also, I put the following documentation about "kind" values into
pg_statistic.h.

/*
* The present allocation of "kind" codes is:
*
* 1-99: reserved for assignment by the core PostgreSQL project
* (values in this range will be documented in this file)
* 100-199: reserved for assignment by the PostGIS project
* (values to be documented in PostGIS documentation)
* 200-9999: reserved for future public assignments
*
* For private use you may choose a "kind" code at random in the range
* 10000-30000. However, for code that is to be widely disseminated it is
* better to obtain a publicly defined "kind" code by request from the
* PostgreSQL Global Development Group.
*/

regards, tom lane

Applied by Tom. Thanks.

---------------------------------------------------------------------------

Mark Cave-Ayland wrote:

Hi Tom,

-----Original Message-----
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
Sent: 02 February 2004 16:54
To: Mark Cave-Ayland
Cc: pgsql-patches@postgresql.org
Subject: Re: [PATCHES] ANALYZE patch for review

"Mark Cave-Ayland" <m.cave-ayland@webbased.co.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.

[ Attachment, skipping... ]

[ Attachment, skipping... ]

[ Attachment, skipping... ]

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Hi Tom,

-----Original Message-----
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
Sent: 12 February 2004 23:52
To: Mark Cave-Ayland
Cc: pgsql-patches@postgresql.org
Subject: Re: [PATCHES] ANALYZE patch for review

"Mark Cave-Ayland" <m.cave-ayland@webbased.co.uk> writes:

Yup indeed. Please find enclosed the latest version of the analyze
patch taking into account all the things we have discussed in the
thread.

I've reviewed and applied this with some small changes. You
did a good job --- the only things you missed AFAICS were
pg_dump support and documentation.

Ahhh pg_dump. Knew I forgot something :) I was going to ask about
patches for the documentation but it looks like you beat me to it.

I changed the API slightly for the typanalyze function and
the compute_stats subroutine. I don't think it's necessary
or appropriate to pass in the Relation pointer to typanalyze;
that simplifies its signature to a single INTERNAL parameter.
(If we did want to pass the Relation, I'd be inclined to
make it a field in VacAttrStats.) Also I tweaked
compute_stats so that the attribute number of the column is
passed explicitly. I foresee needing this for
functional-index stats gathering --- it's likely that the
accumulated sample rows will be built on the fly and will
have a tupdesc that includes both the main table rows and the
functional index columns. So compute_stats shouldn't assume
that the nominal column number is the right thing to use to
extract the column it wants.

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

As I've committed it, the convention that zero means "no
stats" is enforced by examine_attribute(), but the typanalyze
function is responsible for deciding what a negative value
means. Seem reasonable?

I've had a quick browse of CVS on developer.postgresql.org and the work
you've done looks great. The only reason I kept the Relation parameter
was because I wasn't sure if there was a historical reason why someone
would need the relation information as well as the attribute
information. No problem with the zero value enforcing no stats. I also
noticed your later commit with the added fetch_func parameter which
brings stats on functional indexes even closer :)

Also, I put the following documentation about "kind" values
into pg_statistic.h.

/*
* The present allocation of "kind" codes is:
*
* 1-99: reserved for assignment by the core
PostgreSQL project
* (values in this range will be documented
in this file)
* 100-199: reserved for assignment by the PostGIS project
* (values to be documented in PostGIS documentation)
* 200-9999: reserved for future public assignments
*
* For private use you may choose a "kind" code at random in the range
* 10000-30000. However, for code that is to be widely
disseminated it is
* better to obtain a publicly defined "kind" code by request from the
* PostgreSQL Global Development Group.
*/

Fantastic stuff. I'll email Dave and Paul at Refractions and let them
know the changes for analyzing custom types have been made - not sure
whether it will be them or myself that get to implement the changes in
PostGIS but it will be a good test that everything works as it should :)

Thanks again for all your work on this,

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.

"Mark Cave-Ayland" <m.cave-ayland@webbased.co.uk> writes:

The only reason I kept the Relation parameter
was because I wasn't sure if there was a historical reason why someone
would need the relation information as well as the attribute
information.

I can't think of one, but if someone did, they could extract the
relation OID from the pg_attribute row and re-open it for themselves.
So AFAICS this API does not omit any critical info.

I forgot to email you about the fetch_function revision, but I trust
it meets with your approval. Right now it's just a wrapper around
heap_fetch, but I thought we might conceivably want something different
when we do functional-index stats. The fetch function will give us
wiggle room if we need it.

regards, tom lane

-----Original Message-----
From: pgsql-patches-owner@postgresql.org
[mailto:pgsql-patches-owner@postgresql.org] On Behalf Of Tom Lane
Sent: 13 February 2004 14:41
To: Mark Cave-Ayland
Cc: pgsql-patches@postgresql.org
Subject: Re: [PATCHES] ANALYZE patch for review

"Mark Cave-Ayland" <m.cave-ayland@webbased.co.uk> writes:

The only reason I kept the Relation parameter
was because I wasn't sure if there was a historical reason

why someone

would need the relation information as well as the attribute
information.

I can't think of one, but if someone did, they could extract
the relation OID from the pg_attribute row and re-open it for
themselves. So AFAICS this API does not omit any critical info.

Great.

I forgot to email you about the fetch_function revision, but
I trust it meets with your approval. Right now it's just a
wrapper around heap_fetch, but I thought we might conceivably
want something different when we do functional-index stats.
The fetch function will give us wiggle room if we need it.

Yep no problems as long as the functionality is there. I think I can
also see where you're going with functional indexes - during an analyze
phase, for a column X, a pseudo-column f(X) is generated from the sample
data before the stats calculation is performed and the results dropped
into pg_statistic. Hopefully this now shouldn't be too difficult for
someone to pick it up and run with it.

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.