Обсуждение: autovacuum and reloptions
So I gave up waiting for someone else to do the reloptions patch for autovacuum and started work on it myself. What I soon discovered is that on first blush it seems a lot easier than I had expected. On second look, however, the problem is that I seem to be forced to declare all the autovacuum-related options and their parsing properties in reloptions.c. This seems very ugly. I'd very much prefer to be able to declare the options in autovacuum.c and let the rest of the code just ignore them. However, parseRelOptions seems inclined to barf about options it doesn't know about. Maybe that's fine with the current usage, but I think it would be better to leave the options in StdRdOptions alone, and have the autovacuum options defined elsewhere, which seems to require an API change to parseRelOptions -- though I'm not sure what's appropriate. AFAICS this is completely uncharted territory -- the current code understands only fillfactor as a valid option. If we were down the route of just adding the new options just like fillfactor is currently dealt with, the API would get really ugly very quickly. It seems to me we should provide a way to "register" valid options, so that autovacuum.c could inform reloptions.c what are the valid keys that a normal option parsing should just ignore (and, conversely, what options should it ignore when parsing for autovacuum). Thinking more about it, it seems to me that the treatment that fillfactor currently gets should be ripped out in favor of being registered too, somehow ... Before we waste too much time thinking how this registering is to be done, does anybody think that the current approach is OK and thus I should just add the autovacuum options directly into StdRdOptions and default_reloptions? -- Alvaro Herrera Developer, http://www.PostgreSQL.org/ "El Maquinismo fue proscrito so pena de cosquilleo hasta la muerte" (Ijon Tichy en Viajes, Stanislaw Lem)
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On second look, however, the problem is that I seem to be
> forced to declare all the autovacuum-related options and their parsing
> properties in reloptions.c. This seems very ugly.
That was in fact the intended design, and is why the functions exist in
a separate file reloptions.c rather than having been dumped into some
existing place like heapam.c. I don't see anything very wrong with
having autovacuum options in StdRdOptions and default_reloptions().
There might at some point be a good case for inventing a plugin approach
here, but since autovacuum is a pretty much core component now, I don't
see the need to do so for it.
regards, tom lane
Tom Lane wrote: > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > On second look, however, the problem is that I seem to be > > forced to declare all the autovacuum-related options and their parsing > > properties in reloptions.c. This seems very ugly. > > That was in fact the intended design, and is why the functions exist in > a separate file reloptions.c rather than having been dumped into some > existing place like heapam.c. I don't see anything very wrong with > having autovacuum options in StdRdOptions and default_reloptions(). Hmm, OK. However, given that the various AMs amoptions also use default_reloptions, they are going to accept the autovacuum options too. Is that OK? -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Hmm, OK. However, given that the various AMs amoptions also use
> default_reloptions, they are going to accept the autovacuum options too.
> Is that OK?
Well, we might want to split default_reloptions into two versions,
one for heaps and the other for indexes. But it doesn't bother me a
whole lot if we don't.
regards, tom lane
Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > Before we waste too much time thinking how this registering is to be > done, does anybody think that the current approach is OK and thus I > should just add the autovacuum options directly into StdRdOptions and > default_reloptions? Given Simon's suggestion that i/o parameters should be per-tablespace I think we might need to refactor this further. I wonder if we could piggy-back on guc parameters. So you would register a guc variable with a flag saying it's sensible to be set per-tablespace or per-table. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's PostGIS support!
Gregory Stark <stark@enterprisedb.com> writes:
> I wonder if we could piggy-back on guc parameters.
God, no. GUC is hopelessly complex already, we should *not* try to make
it track different values of a parameter for different tables.
Attaching a reloptions-like column to pg_tablespace might not be
unreasonable ... but I think that has little to do with Alvaro's
immediate problem.
regards, tom lane
Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > So I gave up waiting for someone else to do the reloptions patch for > autovacuum and started work on it myself. Is it needed to keep backward compatibility? I'd like to suggest to keep pg_catalog.pg_autovacuum as a system view even after the options is put into reloptons, and the view to be updatable using RULEs if possible. Current pg_autovacuum-table approach has a benefit that we can configure options by rule, for example: INSERT INTO pg_autovacuu SELECT ... FROM pg_class WHERE ...; But we will not able to do that if the settings will be in reloptions because ALTER TABLE SET cannot be used with JOINs. Regards, --- ITAGAKI Takahiro NTT Open Source Software Center
ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
> Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>> So I gave up waiting for someone else to do the reloptions patch for
>> autovacuum and started work on it myself.
> Is it needed to keep backward compatibility?
> I'd like to suggest to keep pg_catalog.pg_autovacuum as a system view
> even after the options is put into reloptons, and the view to be
> updatable using RULEs if possible.
Ugh. No. It has been explicitly stated all along that pg_autovacuum
was a temporary API and that anyone depending on it could expect future
trouble.
> But we will not able to do that if the settings will be in reloptions
> because ALTER TABLE SET cannot be used with JOINs.
Any mechanism that a rule might use to set reloptions would be just
as usable in a join as the rule itself ...
regards, tom lane
Tom Lane wrote: > Gregory Stark <stark@enterprisedb.com> writes: >> I wonder if we could piggy-back on guc parameters. > > God, no. GUC is hopelessly complex already, we should *not* try to make > it track different values of a parameter for different tables. Are there any more specific reasons than "it's very complex"? After all, all the autovacuum options already exist as GUC parameters, so you don't have to repeat all the validation code, for example.
Peter Eisentraut <peter_e@gmx.net> writes:
> Tom Lane wrote:
>> God, no. GUC is hopelessly complex already, we should *not* try to make
>> it track different values of a parameter for different tables.
> Are there any more specific reasons than "it's very complex"?
That one seems quite sufficient to me; but consider dump/restore if you
need more.
regards, tom lane
Alvaro Herrera escreveu: > So I gave up waiting for someone else to do the reloptions patch for > autovacuum and started work on it myself. What I soon discovered is > that on first blush it seems a lot easier than I had expected. > Sorry about that. :( I was swamped with PGCon Brasil and then I took some days to rest. I'm expecting to finish it before next CF. What did I already do? I refactored reloptions.c to support multiple options. I tried to follow up the same way GUC do (of course, it is much simpler). I'm thinking about removing (replacing?) StdRdOptions 'cause we need a different struct to store reloptions. Suggestions? I'm attaching the WIP patch so you can comment on it. I want to continue working on it but I'm afraid you already did more than I do (in this case, let me know for not duplicating efforts). -- Euler Taveira de Oliveira http://www.timbira.com/ Index: src/backend/access/common/reloptions.c =================================================================== RCS file: /a/pgsql/dev/anoncvs/pgsql/src/backend/access/common/reloptions.c,v retrieving revision 1.11 diff -c -r1.11 reloptions.c *** src/backend/access/common/reloptions.c 23 Jul 2008 17:29:53 -0000 1.11 --- src/backend/access/common/reloptions.c 10 Oct 2008 13:55:15 -0000 *************** *** 24,29 **** --- 24,182 ---- #include "utils/guc.h" #include "utils/rel.h" + /* + * Contents of pg_class.reloptions + * + * To add an option: + * + * (i) decide on a class (integer, double, bool), name, default value, upper + * and lower bounds (if applicable). + * + * (ii) add a record below. + * + * (iii) don't forget to document the option + */ + + static struct relopt_bool relOptBools[] = + { + { + { + "autovacuum_enabled", + "Enables autovacuum in this relation", + RO_BOOL + }, + false + }, + /* End-of-list marker */ + { + { + NULL, + NULL, + RO_BOOL + }, + false + } + }; + + static struct relopt_int relOptInts[] = + { + { + { + "fillfactor", + "Packs table pages only to this percentage", + RO_INT + }, + 100, + 10, + 100 + }, + { + { + "autovacuum_vac_base_thresh", + "Minimum number of tuple updates or deletes prior to vacuum", + RO_INT + }, + 50, + 0, + INT_MAX + }, + { + { + "autovacuum_anl_base_thresh", + "Minimum number of tuple inserts, updates or deletes prior to analyze", + RO_INT + }, + 50, + 0, + INT_MAX + }, + { + { + "autovacuum_vac_cost_delay", + "Vacuum cost delay in milliseconds, for autovacuum", + RO_INT + }, + 20, + -1, + 1000 + }, + { + { + "autovacuum_vac_cost_limit", + "Vacuum cost ammount available before napping, for autovacuum", + RO_INT + }, + -1, + -1, + 10000 + }, + { + { + "autovacuum_freeze_min_age", + "Minimum age at which VACUUM should freeze a table row, for autovacuum", + RO_INT + }, + 100000000, + 0, + 1000000000 + }, + { + { + "autovacuum_freeze_max_age", + "Age at which to autovacuum a table to prevent transaction ID wraparound", + RO_INT + }, + 200000000, + 100000000, + 2000000000 + }, + /* End-of-list marker */ + { + { + NULL, + NULL, + RO_INT + }, + 0, + 0, + 0 + } + }; + + struct relopt_real relOptReals[] = + { + { + { + "autovacuum_vac_scale_factor", + "Number of tuples inserts, updates or deletes prior to vacuum as a fraction of reltuples", + RO_REAL + }, + 0.2, + 0.0, + 100.0 + }, + { + { + "autovacuum_anl_scale_factor", + "Number of tuples inserts, updates or deletes prior to analyze as a fraction of reltuples", + RO_REAL + }, + 0.1, + 0.0, + 100.0 + }, + /* End-of-list marker */ + { + { + NULL, + NULL, + RO_REAL + }, + 0.0, + 0.0, + 0.0 + } + }; /* * Transform a relation options list (list of DefElem) into the text array *************** *** 51,56 **** --- 204,212 ---- ArrayBuildState *astate; ListCell *cell; + ereport(DEBUG2, + (errmsg("starting transformRelOptions() ..."))); + /* no change if empty list */ if (defList == NIL) return oldOptions; *************** *** 77,82 **** --- 233,243 ---- char *text_str = VARDATA(oldoption); int text_len = VARSIZE(oldoption) - VARHDRSZ; + char *tmp = text_str; + tmp[text_len] = '\0'; + ereport(DEBUG1, + (errmsg("old reloption: %s", text_str))); + /* Search for a match in defList */ foreach(cell, defList) { *************** *** 93,98 **** --- 254,261 ---- astate = accumArrayResult(astate, oldoptions[i], false, TEXTOID, CurrentMemoryContext); + ereport(DEBUG1, + (errmsg("added old reloption: %s", tmp))); } } } *************** *** 136,141 **** --- 299,307 ---- SET_VARSIZE(t, len); sprintf(VARDATA(t), "%s=%s", def->defname, value); + ereport(DEBUG1, + (errmsg("added new reloption: %s=%s", def->defname, value))); + astate = accumArrayResult(astate, PointerGetDatum(t), false, TEXTOID, CurrentMemoryContext); *************** *** 147,152 **** --- 313,321 ---- else result = (Datum) 0; + ereport(DEBUG2, + (errmsg("ending transformRelOptions() ..."))); + return result; } *************** *** 164,169 **** --- 333,341 ---- int noptions; int i; + ereport(DEBUG2, + (errmsg("starting untransformRelOptions() ..."))); + /* Nothing to do if no options */ if (!PointerIsValid(DatumGetPointer(options))) return result; *************** *** 188,196 **** --- 360,375 ---- *p++ = '\0'; val = (Node *) makeString(pstrdup(p)); } + + ereport(DEBUG1, + (errmsg("added reloption: %s=%s", s, p))); + result = lappend(result, makeDefElem(pstrdup(s), val)); } + ereport(DEBUG2, + (errmsg("ending untransformRelOptions() ..."))); + return result; } *************** *** 199,206 **** * Interpret reloptions that are given in text-array format. * * options: array of "keyword=value" strings, as built by transformRelOptions - * numkeywords: number of legal keywords - * keywords: the allowed keywords * values: output area * validate: if true, throw error for unrecognized keywords. * --- 378,383 ---- *************** *** 209,221 **** * containing the corresponding value, or NULL if the keyword does not appear. */ void ! parseRelOptions(Datum options, int numkeywords, const char *const * keywords, ! char **values, bool validate) { ArrayType *array; Datum *optiondatums; int noptions; int i; /* Initialize to "all defaulted" */ MemSet(values, 0, numkeywords * sizeof(char *)); --- 386,410 ---- * containing the corresponding value, or NULL if the keyword does not appear. */ void ! parseRelOptions(Datum options, char **values, bool validate, int minFillfactor) { ArrayType *array; Datum *optiondatums; int noptions; int i; + int numkeywords = 0; + + ereport(DEBUG2, + (errmsg("starting parseRelOptions() ..."))); + + for (i = 0; relOptBools[i].gen.name; i++) + numkeywords++; + for (i = 0; relOptInts[i].gen.name; i++) + numkeywords++; + for (i = 0; relOptReals[i].gen.name; i++) + numkeywords++; + ereport(DEBUG1, + (errmsg("number of keywords: %d", numkeywords))); /* Initialize to "all defaulted" */ MemSet(values, 0, numkeywords * sizeof(char *)); *************** *** 236,267 **** text *optiontext = DatumGetTextP(optiondatums[i]); char *text_str = VARDATA(optiontext); int text_len = VARSIZE(optiontext) - VARHDRSZ; ! int j; /* Search for a match in keywords */ ! for (j = 0; j < numkeywords; j++) { ! int kw_len = strlen(keywords[j]); if (text_len > kw_len && text_str[kw_len] == '=' && ! pg_strncasecmp(text_str, keywords[j], kw_len) == 0) { ! char *value; ! int value_len; if (values[j] && validate) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), ! errmsg("parameter \"%s\" specified more than once", ! keywords[j]))); value_len = text_len - kw_len - 1; value = (char *) palloc(value_len + 1); memcpy(value, text_str + kw_len + 1, value_len); value[value_len] = '\0'; values[j] = value; break; } } if (j >= numkeywords && validate) { char *s; --- 425,574 ---- text *optiontext = DatumGetTextP(optiondatums[i]); char *text_str = VARDATA(optiontext); int text_len = VARSIZE(optiontext) - VARHDRSZ; ! int j = 0; ! bool found = false; ! ! char *tmp = text_str; ! tmp[text_len] = '\0'; ! ereport(DEBUG1, ! (errmsg("parsing reloption %s ...", tmp))); /* Search for a match in keywords */ ! while (relOptBools[j].gen.name) { ! int kw_len = strlen(relOptBools[j].gen.name); if (text_len > kw_len && text_str[kw_len] == '=' && ! pg_strcasecmp(text_str, relOptBools[j].gen.name, kw_len) == 0) { ! char *value; ! int value_len; if (values[j] && validate) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), ! errmsg("parameter \"%s\" specified more than once", ! relOptBools[j].gen.name))); ! value_len = text_len - kw_len - 1; value = (char *) palloc(value_len + 1); memcpy(value, text_str + kw_len + 1, value_len); value[value_len] = '\0'; + + if (parse_bool(value, NULL) == false && validate) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("invalid input value for parameter %s: \"%s\"", + relOptBools[j].gen.name, value), + errhint("value must be a boolean"))); + values[j] = value; + + found = true; break; } + + j++; } + + while (relOptInts[j].gen.name || !found) + { + int kw_len = strlen(relOptInts[j].gen.name); + + if (text_len > kw_len && text_str[kw_len] == '=' && + pg_strcasecmp(text_str, relOptInts[j].gen.name, kw_len) == 0) + { + char *value; + int value_len; + + if (values[j] && validate) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("parameter \"%s\" specified more than once", + relOptInts[j].gen.name))); + + value_len = text_len - kw_len - 1; + value = (char *) palloc(value_len + 1); + memcpy(value, text_str + kw_len + 1, value_len); + value[value_len] = '\0'; + + if (parse_int(value, NULL, 0, NULL) == false && validate) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("invalid input value for parameter %s: \"%s\"", + relOptBools[j].gen.name, value), + errhint("value must be an integer"))); + + if ((value < relOptInts[j].min || value > relOptInts[j].max) && validate) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("%s=%s is out of range (should be between %d and %d)", + relOptInts[j].gen.name, value, relOptInts[j].min, relOptInts[j].max))); + + /* + * Especial check for fillfactor because minimum fillfactor values + * are AMs and/or heap dependant. We don't need to check upper + * limit because we already did it above. + */ + if (pg_strcasecmp(relOptInts[j].gen.name, "fillfactor") == 0 && + value < minFillfactor && validate) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("fillfactor=%s is out of range (should be between %d and 100)", + value, minFillfactor))); + + values[j] = value; + + found = true; + break; + } + + j++; + } + + while (relOptReals[j].gen.name || !found) + { + int kw_len = strlen(relOptReals[j].gen.name); + + if (text_len > kw_len && text_str[kw_len] == '=' && + pg_strcasecmp(text_str, relOptReals[j].gen.name, kw_len) == 0) + { + char *value; + int value_len; + + if (values[j] && validate) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("parameter \"%s\" specified more than once", + relOptReals[j].gen.name))); + + value_len = text_len - kw_len - 1; + value = (char *) palloc(value_len + 1); + memcpy(value, text_str + kw_len + 1, value_len); + value[value_len] = '\0'; + + if (parse_real(value, NULL) == false && validate) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("invalid input value for parameter %s: \"%s\"", + relOptBools[j].gen.name, value), + errhint("value must be a float"))); + + if ((value < relOptReals[j].min || value > relOptReals[j].max) && validate) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("%s=%s is out of range (should be between %d and %d)", + relOptReals[j].gen.name, value, relOptReals[j].min, relOptReals[j].max))); + + values[j] = value; + + found = true; + break; + } + + j++; + } + if (j >= numkeywords && validate) { char *s; *************** *** 276,281 **** --- 583,591 ---- errmsg("unrecognized parameter \"%s\"", s))); } } + + ereport(DEBUG2, + (errmsg("ending parseRelOptions() ..."))); } *************** *** 286,297 **** default_reloptions(Datum reloptions, bool validate, int minFillfactor, int defaultFillfactor) { - static const char *const default_keywords[1] = {"fillfactor"}; char *values[1]; int fillfactor; StdRdOptions *result; ! parseRelOptions(reloptions, 1, default_keywords, values, validate); /* * If no options, we can just return NULL rather than doing anything. --- 596,609 ---- default_reloptions(Datum reloptions, bool validate, int minFillfactor, int defaultFillfactor) { char *values[1]; int fillfactor; StdRdOptions *result; ! ereport(DEBUG2, ! (errmsg("starting default_options() ..."))); ! ! parseRelOptions(reloptions, values, validate, minFillfactor); /* * If no options, we can just return NULL rather than doing anything. *************** *** 301,331 **** if (values[0] == NULL) return NULL; - if (!parse_int(values[0], &fillfactor, 0, NULL)) - { - if (validate) - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("fillfactor must be an integer: \"%s\"", - values[0]))); - return NULL; - } - - if (fillfactor < minFillfactor || fillfactor > 100) - { - if (validate) - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("fillfactor=%d is out of range (should be between %d and 100)", - fillfactor, minFillfactor))); - return NULL; - } - result = (StdRdOptions *) palloc(sizeof(StdRdOptions)); SET_VARSIZE(result, sizeof(StdRdOptions)); result->fillfactor = fillfactor; return (bytea *) result; } --- 613,626 ---- if (values[0] == NULL) return NULL; result = (StdRdOptions *) palloc(sizeof(StdRdOptions)); SET_VARSIZE(result, sizeof(StdRdOptions)); result->fillfactor = fillfactor; + ereport(DEBUG2, + (errmsg("ending default_reloptions() ..."))); + return (bytea *) result; } *************** *** 336,344 **** bytea * heap_reloptions(char relkind, Datum reloptions, bool validate) { ! return default_reloptions(reloptions, validate, ! HEAP_MIN_FILLFACTOR, ! HEAP_DEFAULT_FILLFACTOR); } --- 631,642 ---- bytea * heap_reloptions(char relkind, Datum reloptions, bool validate) { ! ereport(DEBUG2, ! (errmsg("starting heap_reloptions() ..."))); ! ! return default_reloptions(reloptions, validate, ! HEAP_MIN_FILLFACTOR, ! HEAP_DEFAULT_FILLFACTOR); } *************** *** 356,361 **** --- 654,662 ---- FunctionCallInfoData fcinfo; Datum result; + ereport(DEBUG2, + (errmsg("starting index_reloptions() ..."))); + Assert(RegProcedureIsValid(amoptions)); /* Assume function is strict */ *************** *** 377,381 **** --- 678,685 ---- if (fcinfo.isnull || DatumGetPointer(result) == NULL) return NULL; + ereport(DEBUG2, + (errmsg("ending index_reloptions() ..."))); + return DatumGetByteaP(result); } Index: src/backend/access/gin/ginutil.c =================================================================== RCS file: /a/pgsql/dev/anoncvs/pgsql/src/backend/access/gin/ginutil.c,v retrieving revision 1.17 diff -c -r1.17 ginutil.c *** src/backend/access/gin/ginutil.c 30 Sep 2008 10:52:10 -0000 1.17 --- src/backend/access/gin/ginutil.c 2 Oct 2008 20:33:13 -0000 *************** *** 339,347 **** #define GIN_MIN_FILLFACTOR 10 #define GIN_DEFAULT_FILLFACTOR 100 ! result = default_reloptions(reloptions, validate, ! GIN_MIN_FILLFACTOR, GIN_DEFAULT_FILLFACTOR); if (result) PG_RETURN_BYTEA_P(result); PG_RETURN_NULL(); --- 339,350 ---- #define GIN_MIN_FILLFACTOR 10 #define GIN_DEFAULT_FILLFACTOR 100 ! /* TODO how can we pass the min|default fillfactor to reloptions? */ ! /* XXX different AM has different values! */ ! result = default_reloptions(reloptions, validate, ! GIN_MIN_FILLFACTOR, GIN_DEFAULT_FILLFACTOR); + if (result) PG_RETURN_BYTEA_P(result); PG_RETURN_NULL(); Index: src/backend/access/gist/gistutil.c =================================================================== RCS file: /a/pgsql/dev/anoncvs/pgsql/src/backend/access/gist/gistutil.c,v retrieving revision 1.31 diff -c -r1.31 gistutil.c *** src/backend/access/gist/gistutil.c 30 Sep 2008 10:52:10 -0000 1.31 --- src/backend/access/gist/gistutil.c 2 Oct 2008 20:33:28 -0000 *************** *** 670,678 **** bool validate = PG_GETARG_BOOL(1); bytea *result; ! result = default_reloptions(reloptions, validate, ! GIST_MIN_FILLFACTOR, GIST_DEFAULT_FILLFACTOR); if (result) PG_RETURN_BYTEA_P(result); PG_RETURN_NULL(); --- 670,681 ---- bool validate = PG_GETARG_BOOL(1); bytea *result; ! /* TODO how can we pass the min|default fillfactor to reloptions? */ ! /* XXX different AM has different values! */ ! result = default_reloptions(reloptions, validate, ! GIST_MIN_FILLFACTOR, GIST_DEFAULT_FILLFACTOR); + if (result) PG_RETURN_BYTEA_P(result); PG_RETURN_NULL(); Index: src/backend/access/hash/hashutil.c =================================================================== RCS file: /a/pgsql/dev/anoncvs/pgsql/src/backend/access/hash/hashutil.c,v retrieving revision 1.57 diff -c -r1.57 hashutil.c *** src/backend/access/hash/hashutil.c 15 Sep 2008 18:43:41 -0000 1.57 --- src/backend/access/hash/hashutil.c 2 Oct 2008 20:32:48 -0000 *************** *** 224,232 **** bool validate = PG_GETARG_BOOL(1); bytea *result; ! result = default_reloptions(reloptions, validate, ! HASH_MIN_FILLFACTOR, HASH_DEFAULT_FILLFACTOR); if (result) PG_RETURN_BYTEA_P(result); PG_RETURN_NULL(); --- 224,235 ---- bool validate = PG_GETARG_BOOL(1); bytea *result; ! /* TODO how can we pass the min|default fillfactor to reloptions? */ ! /* XXX different AM has different values! */ ! result = default_reloptions(reloptions, validate, ! HASH_MIN_FILLFACTOR, HASH_DEFAULT_FILLFACTOR); + if (result) PG_RETURN_BYTEA_P(result); PG_RETURN_NULL(); Index: src/backend/access/nbtree/nbtutils.c =================================================================== RCS file: /a/pgsql/dev/anoncvs/pgsql/src/backend/access/nbtree/nbtutils.c,v retrieving revision 1.91 diff -c -r1.91 nbtutils.c *** src/backend/access/nbtree/nbtutils.c 19 Jun 2008 00:46:03 -0000 1.91 --- src/backend/access/nbtree/nbtutils.c 2 Oct 2008 20:32:35 -0000 *************** *** 1402,1410 **** bool validate = PG_GETARG_BOOL(1); bytea *result; ! result = default_reloptions(reloptions, validate, ! BTREE_MIN_FILLFACTOR, BTREE_DEFAULT_FILLFACTOR); if (result) PG_RETURN_BYTEA_P(result); PG_RETURN_NULL(); --- 1402,1413 ---- bool validate = PG_GETARG_BOOL(1); bytea *result; ! /* TODO how can we pass the min|default fillfactor to reloptions? */ ! /* XXX different AM has different values! */ ! result = default_reloptions(reloptions, validate, ! BTREE_MIN_FILLFACTOR, BTREE_DEFAULT_FILLFACTOR); + if (result) PG_RETURN_BYTEA_P(result); PG_RETURN_NULL(); Index: src/include/access/reloptions.h =================================================================== RCS file: /a/pgsql/dev/anoncvs/pgsql/src/include/access/reloptions.h,v retrieving revision 1.5 diff -c -r1.5 reloptions.h *** src/include/access/reloptions.h 1 Jan 2008 19:45:56 -0000 1.5 --- src/include/access/reloptions.h 7 Oct 2008 04:57:59 -0000 *************** *** 20,36 **** #include "nodes/pg_list.h" extern Datum transformRelOptions(Datum oldOptions, List *defList, bool ignoreOids, bool isReset); extern List *untransformRelOptions(Datum options); ! extern void parseRelOptions(Datum options, int numkeywords, ! const char *const * keywords, ! char **values, bool validate); ! extern bytea *default_reloptions(Datum reloptions, bool validate, ! int minFillfactor, int defaultFillfactor); extern bytea *heap_reloptions(char relkind, Datum reloptions, bool validate); --- 20,85 ---- #include "nodes/pg_list.h" + /* types supported by reloptions */ + enum ro_type + { + RO_BOOL, + RO_INT, + RO_REAL + }; + + /* kind supported by reloptions */ + enum ro_kind + { + RO_INDEX, + RO_HEAP + }; + + /* generic struct to hold shared data */ + struct relopt_gen + { + const char *name; + const char *desc; + enum ro_type type; /* type of variable */ + enum ro_kind kind; /* index or heap? */ + }; + + /* reloptions records for specific variable types */ + struct relopt_bool + { + struct relopt_gen gen; + bool value; + bool reset_value; /* XXX useful? */ + }; + + struct relopt_int + { + struct relopt_gen gen; + int value; + int min; + int max; + int reset_value; /* XXX useful? */ + }; + + struct relopt_real + { + struct relopt_gen gen; + double value; + double min; + double max; + double reset_value; /* XXX useful? */ + }; + extern Datum transformRelOptions(Datum oldOptions, List *defList, bool ignoreOids, bool isReset); extern List *untransformRelOptions(Datum options); ! extern void parseRelOptions(Datum options, char **values, bool validate, ! int minFillfactor); ! extern bytea *default_reloptions(Datum reloptions, bool validate, ! int minFillfactor, int defaultFillfactor); extern bytea *heap_reloptions(char relkind, Datum reloptions, bool validate);
Euler Taveira de Oliveira wrote: > What did I already do? I refactored reloptions.c to support multiple > options. I tried to follow up the same way GUC do (of course, it is much > simpler). I'm thinking about removing (replacing?) StdRdOptions 'cause > we need a different struct to store reloptions. Suggestions? > > I'm attaching the WIP patch so you can comment on it. I want to continue > working on it but I'm afraid you already did more than I do (in this > case, let me know for not duplicating efforts). Interesting. The main problem with this patch is that it loses the ability to pass to parseRelOptions the set of options that are valid for each context. Right now all callers use the same list comprising only fillfactor, but my guess is that once we add new options it will make sense to start differentiating. It makes no sense for indexes to have autovacuum options, for example. This is why I suggested in the email that started this thread we needed some sort of registering capability (which was thrown down). I think the main idea in your patch is fine, and better than what I was doing which was just adding every option in default_reloptions. However it needs to be adjusted somehow so that it doesn't mix all the options. Maybe we could add a "category" bitmask for which each option would be valid. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera escreveu: > Maybe we could add a "category" bitmask for which each option would be > valid. > The category tests are fine, that's why I introduced relopt_gen.kind but I'm not using it yet. I'll try to refactor it to use bitmask (some options could be used to both -- fillfactor) and to add it in the validation code. -- Euler Taveira de Oliveira http://www.timbira.com/
Euler Taveira de Oliveira wrote: > Alvaro Herrera escreveu: > > > Maybe we could add a "category" bitmask for which each option would be > > valid. > > > The category tests are fine, that's why I introduced relopt_gen.kind but > I'm not using it yet. Ah, right, I hadn't noticed the kind stuff, maybe because it's unused ;-) > I'll try to refactor it to use bitmask (some options could be used to > both -- fillfactor) and to add it in the validation code. Right, that's why I suggested using a bitmask. Okay, so I'll let you deal with this for a while yet. A minor suggestion: label the enum members distinctively, i.e. instead of RO_BOOL perhaps use RELOPT_TYPE_BOOL, and RO_HEAP should be RELOPT_KIND_HEAP (note this cannot be a plain enum, each category needs a separate bit). -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Okay, so I'll let you deal with this for a while yet. A minor
> suggestion: label the enum members distinctively, i.e. instead of
> RO_BOOL perhaps use RELOPT_TYPE_BOOL, and RO_HEAP should be
> RELOPT_KIND_HEAP (note this cannot be a plain enum, each category needs
> a separate bit).
My first reaction was that the categories should just be the different
possible values of relkind. However, it also seems possible that there
could be index-AM-specific reloptions. So maybe what we need is a
categorization that is like relkind but breaks down RELKIND_INDEX into a
category per AM.
regards, tom lane
Euler Taveira de Oliveira wrote: > Alvaro Herrera escreveu: > > So I gave up waiting for someone else to do the reloptions patch for > > autovacuum and started work on it myself. What I soon discovered is > > that on first blush it seems a lot easier than I had expected. > > > Sorry about that. :( I was swamped with PGCon Brasil and then I took > some days to rest. I'm expecting to finish it before next CF. So did this go anywhere? -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
I have a comment about reloptions of autovacuum parameters: I'd like to have an easier way to extract individual parameters. Alvaro Herrera <alvherre@commandprompt.com> wrote: > Euler Taveira de Oliveira wrote: > > What did I already do? I refactored reloptions.c to support multiple > > options. I tried to follow up the same way GUC do (of course, it is much > > simpler). I'm thinking about removing (replacing?) StdRdOptions 'cause > > we need a different struct to store reloptions. Suggestions? > Interesting. We store reloptions as an array of 'key=value' text, but there is no official way to read each parameter. I always create an user defined function to extract fillfactors, but it would be better if we have a standard method to do that. ---- [brute force way] CREATE FUNCTION pg_fillfactor(reloptions text[], relam OID) RETURNS integer AS $$ SELECT (regexp_matches(array_to_string($1, '/'), 'fillfactor=([0-9]+)'))[1]::integer AS fillfactor UNION ALL SELECT CASE $2 WHEN 0 THEN 100 -- heap WHEN 403 THEN 90 -- btree WHEN 405 THEN 70 -- hash WHEN 783 THEN 90 -- gist WHEN 2742 THEN 100 -- gin END LIMIT 1; $$ LANGUAGE sql STABLE; ---- Regards, --- ITAGAKI Takahiro NTT Open Source Software Center
ITAGAKI Takahiro escreveu: [Sorry for the delay. I'm preparing the final patch and in a day or so I'll post it.] > I have a comment about reloptions of autovacuum parameters: > I'd like to have an easier way to extract individual parameters. > > Alvaro Herrera <alvherre@commandprompt.com> wrote: > >> Euler Taveira de Oliveira wrote: >>> What did I already do? I refactored reloptions.c to support multiple >>> options. I tried to follow up the same way GUC do (of course, it is much >>> simpler). I'm thinking about removing (replacing?) StdRdOptions 'cause >>> we need a different struct to store reloptions. Suggestions? >> Interesting. > > We store reloptions as an array of 'key=value' text, but there is > no official way to read each parameter. I always create an user > defined function to extract fillfactors, but it would be better > if we have a standard method to do that. > We will have an official function (getRelOptions()?) to extract the reloption (autovacuum) parameters because we need to transform pg_autovacuum catalog table in a view to maintain backward compatibility. -- Euler Taveira de Oliveira http://www.timbira.com/
Euler Taveira de Oliveira wrote: > We will have an official function (getRelOptions()?) to extract the > reloption (autovacuum) parameters because we need to transform > pg_autovacuum catalog table in a view to maintain backward compatibility. Why? I'd say don't waste your time. If anything, it can be done after the initial patch has been committed. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Euler Taveira de Oliveira <euler@timbira.com> wrote: > We will have an official function (getRelOptions()?) to extract the > reloption (autovacuum) parameters Yeah, I want it, but... > because we need to transform > pg_autovacuum catalog table in a view to maintain backward compatibility. I don't think we need pg_autovacuum in 8.4 because it is a deprecated API. However, we should provide some easier methods to create an user-defined pg_autovacuum view in case that users really need it. Regards, --- ITAGAKI Takahiro NTT Open Source Software Center
Euler Taveira de Oliveira escreveu: > [Sorry for the delay. I'm preparing the final patch and in a day or so > I'll post it.] > Here is the patch that replace pg_autovaccum catalog with reloptions. I refactored the reloptions.c to support multiple parameters and made the action of adding a new option an easy task. I'm testing it yet, so don't expect it to work properly. I'll prepare docs as soon as I finish the tests. Do i have to prepare some regression tests? I don't provide a pg_autovacuum view as suggested by Itagari-san [1] but if others agree that we need it, I will work on it. I don't if we need a function (wrapper around getRelOption()) to get an option from reloptions array. I add an ugly-hack to \d+ foo. IMHO, it'll be good to know what options are used by table/index foo (we already do it for oids) but I'm not happy with my suggestion. I move RelationGet*() functions from rel.h. That's because we need some knowledge that's only in reloptions.c (getRelOptions). But I want to avoid including reloptions.h at some files. Comments? PS> don't forget to remove include/catalog/pg_autovacuum.h [1] http://archives.postgresql.org/pgsql-hackers/2008-11/msg00830.php -- Euler Taveira de Oliveira http://www.timbira.com/
Вложения
> Here is the patch that replace pg_autovaccum catalog with reloptions. I > refactored the reloptions.c to support multiple parameters and made the > action of adding a new option an easy task. I'm testing it yet, so don't > expect it to work properly. I'll prepare docs as soon as I finish the > tests. Do i have to prepare some regression tests? > > I don't provide a pg_autovacuum view as suggested by Itagari-san [1] but > if others agree that we need it, I will work on it. I don't if we need a > function (wrapper around getRelOption()) to get an option from > reloptions array. > > I add an ugly-hack to \d+ foo. IMHO, it'll be good to know what options > are used by table/index foo (we already do it for oids) but I'm not > happy with my suggestion. > > I move RelationGet*() functions from rel.h. That's because we need some > knowledge that's only in reloptions.c (getRelOptions). But I want to > avoid including reloptions.h at some files. > > Comments? > > PS> don't forget to remove include/catalog/pg_autovacuum.h Several things related to this patch have been committed: http://archives.postgresql.org/message-id/20081219143958.6F2DD7563FE@cvs.postgresql.org http://archives.postgresql.org/message-id/20090105171428.77B29754A17@cvs.postgresql.org (and several follow-on commits) What still remains to be done for 8.4? ...Robert
Robert Haas escreveu: > Several things related to this patch have been committed: > > http://archives.postgresql.org/message-id/20081219143958.6F2DD7563FE@cvs.postgresql.org > http://archives.postgresql.org/message-id/20090105171428.77B29754A17@cvs.postgresql.org > (and several follow-on commits) > > What still remains to be done for 8.4? > autovacuum part? -- Euler Taveira de Oliveira http://www.timbira.com/
On Sun, Jan 11, 2009 at 6:47 PM, Euler Taveira de Oliveira <euler@timbira.com> wrote: > Robert Haas escreveu: >> Several things related to this patch have been committed: >> >> http://archives.postgresql.org/message-id/20081219143958.6F2DD7563FE@cvs.postgresql.org >> http://archives.postgresql.org/message-id/20090105171428.77B29754A17@cvs.postgresql.org >> (and several follow-on commits) >> >> What still remains to be done for 8.4? >> > autovacuum part? I guess Alvaro is working on that? Or are you? ...Robert
Robert Haas escribió: > On Sun, Jan 11, 2009 at 6:47 PM, Euler Taveira de Oliveira > <euler@timbira.com> wrote: > > Robert Haas escreveu: > >> Several things related to this patch have been committed: > >> > >> http://archives.postgresql.org/message-id/20081219143958.6F2DD7563FE@cvs.postgresql.org > >> http://archives.postgresql.org/message-id/20090105171428.77B29754A17@cvs.postgresql.org > >> (and several follow-on commits) > >> > >> What still remains to be done for 8.4? > >> > > autovacuum part? > > I guess Alvaro is working on that? Or are you? I have a followup patch that allows one to use a namespace in ALTER TABLE SET commands, like this: alter table foo set (toast.fillfactor = 50) This is still WIP because it has some minor annoyances. I will finish and commit the table-based amoptions infrastructure and then be back on the namespace patch. This namespace patch is a prerequisite of the autovacuum work, because without it, it is impossible to change autovacuum settings for toast tables, which would be a regression. I have a separate branch on which I keep the old patch from Euler updated to the current reloptions code; so it is probably very similar to what Euler just sent. (I'll have a look at that soon anyway.) -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera escribió: > I have a separate branch on which I keep the old patch from Euler > updated to the current reloptions code; so it is probably very similar > to what Euler just sent. (I'll have a look at that soon anyway.) Huh, nevermind -- I thought that Euler had just sent an updated version of his patch, but only now I noticed that the message I was looking at is dated Nov. 21st. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera escreveu: > Alvaro Herrera escribió: > >> I have a separate branch on which I keep the old patch from Euler >> updated to the current reloptions code; so it is probably very similar >> to what Euler just sent. (I'll have a look at that soon anyway.) > > Huh, nevermind -- I thought that Euler had just sent an updated version > of his patch, but only now I noticed that the message I was looking at > is dated Nov. 21st. > And I did it (It is even listed in the wiki). Last version was [1]. I fixed some problems and added some docs. [1] http://archives.postgresql.org/pgsql-hackers/2008-12/msg00292.php -- Euler Taveira de Oliveira http://www.timbira.com/
Euler Taveira de Oliveira escribió: > Alvaro Herrera escreveu: > > Alvaro Herrera escribió: > > > >> I have a separate branch on which I keep the old patch from Euler > >> updated to the current reloptions code; so it is probably very similar > >> to what Euler just sent. (I'll have a look at that soon anyway.) > > > > Huh, nevermind -- I thought that Euler had just sent an updated version > > of his patch, but only now I noticed that the message I was looking at > > is dated Nov. 21st. > > > And I did it (It is even listed in the wiki). Last version was [1]. I fixed > some problems and added some docs. Yes, the autovacuum patch I have is loosely based on that one IIRC. (I modified it heavily though.) -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera wrote: > Euler Taveira de Oliveira escribió: >> Alvaro Herrera escreveu: >>> Alvaro Herrera escribió: >>> >>>> I have a separate branch on which I keep the old patch from Euler >>>> updated to the current reloptions code; so it is probably very similar >>>> to what Euler just sent. (I'll have a look at that soon anyway.) >>> Huh, nevermind -- I thought that Euler had just sent an updated version >>> of his patch, but only now I noticed that the message I was looking at >>> is dated Nov. 21st. >>> >> And I did it (It is even listed in the wiki). Last version was [1]. I fixed >> some problems and added some docs. > > Yes, the autovacuum patch I have is loosely based on that one IIRC. (I > modified it heavily though.) Could someone udpate the status of this item in the commitfest wiki?
Alvaro Herrera escribió: > Euler Taveira de Oliveira escribió: > > And I did it (It is even listed in the wiki). Last version was [1]. I fixed > > some problems and added some docs. > > Yes, the autovacuum patch I have is loosely based on that one IIRC. (I > modified it heavily though.) Here's my proposed patch. There is a bug in the handling of TOAST tables; I'm sending this as a WIP to add it to the commitfest status page for this patch. This also needs to be merged with my other patch to handle reloptions namespaces, which will allow us to set reloptions for TOAST tables. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Вложения
Alvaro Herrera escribió: > Here's my proposed patch. There is a bug in the handling of TOAST > tables; I'm sending this as a WIP to add it to the commitfest status > page for this patch. Sorry, that was a really stupid bug. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Вложения
So here's what looks like a committable patch. Note to self: remember to remove src/include/catalog/pg_autovacuum.h and to bump catversion. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Вложения
Alvaro Herrera escreveu: > So here's what looks like a committable patch. > > Note to self: remember to remove src/include/catalog/pg_autovacuum.h and > to bump catversion. > > Works for me. Just a few comments. (i) I don't like this construction "by entries by changing storage parameters". I prefer "by changing storage parameters" or "by entries in pg_class.reloptions"; (ii) I think we should change the expression "storage parameters" for something else because autovacuum is related to maintenance. My suggestion is a general expression like "relation parameters"; (iii) I noticed that GUC defaults and relopt defaults are different (autovacuum_cost_delay and autovacuum_cost_limit). Is there any reason for not using -1? (iv) Maybe we should document that pg_dump will only dump reloptions like toast.foo iff the relation has an associated TOAST table. This seems obvious but ... -- Euler Taveira de Oliveira http://www.timbira.com/
Euler Taveira de Oliveira escribió: > (i) I don't like this construction "by entries by changing storage > parameters". I prefer "by changing storage parameters" or "by entries in > pg_class.reloptions"; Heh, obvious "by entries by" is a cut'n'pasto; fixed. Maybe an xref would be better there. I attach the doc patch again; I fiddled a bit with it this morning. Comments? > (ii) I think we should change the expression "storage parameters" for > something else because autovacuum is related to maintenance. My suggestion is > a general expression like "relation parameters"; I'm not sure I agree with this idea, because the term "storage parameter" has been used for several releases already. This would be a relatively major terminology change. > (iii) I noticed that GUC defaults and relopt defaults are different > (autovacuum_cost_delay and autovacuum_cost_limit). Is there any reason for not > using -1? Yeah, -1 does not make sense. It made sense in pg_autovacuum because otherwise you didn't have to specify "skip this setting and use the default". In reloptions, if you don't want to specify the setting, just don't specify it. > (iv) Maybe we should document that pg_dump will only dump reloptions like > toast.foo iff the relation has an associated TOAST table. This seems obvious > but ... Well, if it doesn't have a toast table, then there's no need for toast settings, is there? I mean, you could construe it as a gotcha: if you CREATE TABLE with only fixed-length columns and specify some reloptions, and later add a column that requires a toast table, it won't have the options you set at CREATE time. However, it seems to me to be very low in the importance scale. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Вложения
Alvaro Herrera escreveu: >> (ii) I think we should change the expression "storage parameters" for >> something else because autovacuum is related to maintenance. My suggestion is >> a general expression like "relation parameters"; > > I'm not sure I agree with this idea, because the term "storage > parameter" has been used for several releases already. This would be a > relatively major terminology change. > I don't buy your argument. 'fillfactor' is a _storage_ parameter but 'autovacuum_enabled' is not. I don't like terminology changes too but in this case it sounds strange calling autovacuum_* as storage parameters. -- Euler Taveira de Oliveira http://www.timbira.com/
On Fri, Feb 6, 2009 at 5:45 PM, Euler Taveira de Oliveira <euler@timbira.com> wrote: > Alvaro Herrera escreveu: >>> (ii) I think we should change the expression "storage parameters" for >>> something else because autovacuum is related to maintenance. My suggestion is >>> a general expression like "relation parameters"; >> >> I'm not sure I agree with this idea, because the term "storage >> parameter" has been used for several releases already. This would be a >> relatively major terminology change. >> > I don't buy your argument. 'fillfactor' is a _storage_ parameter but > 'autovacuum_enabled' is not. I don't like terminology changes too but in this > case it sounds strange calling autovacuum_* as storage parameters. > +1 -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157