diff options
author | Tom Lane <tgl@sss.pgh.pa.us> | 2006-07-03 22:45:41 +0000 |
---|---|---|
committer | Tom Lane <tgl@sss.pgh.pa.us> | 2006-07-03 22:45:41 +0000 |
commit | b7b78d24f7fc8d621af40b2e404b6a3f3420e89e (patch) | |
tree | da6b05ca5779ad812557b5d4cd38be79bf524825 /src/backend/commands | |
parent | feed07350b63e32ba2fbe50181df7d40ca2ee33e (diff) | |
download | postgresql-b7b78d24f7fc8d621af40b2e404b6a3f3420e89e.tar.gz postgresql-b7b78d24f7fc8d621af40b2e404b6a3f3420e89e.zip |
Code review for FILLFACTOR patch. Change WITH grammar as per earlier
discussion (including making def_arg allow reserved words), add missed
opt_definition for UNIQUE case. Put the reloptions support code in a less
random place (I chose to make a new file access/common/reloptions.c).
Eliminate header inclusion creep. Make the index options functions safely
user-callable (seems like client apps might like to be able to test validity
of options before trying to make an index). Reduce overhead for normal case
with no options by allowing rd_options to be NULL. Fix some unmaintainably
klugy code, including getting rid of Natts_pg_class_fixed at long last.
Some stylistic cleanup too, and pay attention to keeping comments in sync
with code.
Documentation still needs work, though I did fix the omissions in
catalogs.sgml and indexam.sgml.
Diffstat (limited to 'src/backend/commands')
-rw-r--r-- | src/backend/commands/cluster.c | 28 | ||||
-rw-r--r-- | src/backend/commands/define.c | 37 | ||||
-rw-r--r-- | src/backend/commands/indexcmds.c | 18 | ||||
-rw-r--r-- | src/backend/commands/prepare.c | 3 | ||||
-rw-r--r-- | src/backend/commands/sequence.c | 14 | ||||
-rw-r--r-- | src/backend/commands/tablecmds.c | 121 | ||||
-rw-r--r-- | src/backend/commands/vacuum.c | 12 | ||||
-rw-r--r-- | src/backend/commands/vacuumlazy.c | 4 |
8 files changed, 129 insertions, 108 deletions
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 08b76e96d5f..fa41de3a2dc 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -11,7 +11,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/commands/cluster.c,v 1.148 2006/07/02 02:23:19 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/commands/cluster.c,v 1.149 2006/07/03 22:45:38 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -567,7 +567,8 @@ make_new_heap(Oid OIDOldHeap, const char *NewName, Oid NewTableSpace) Oid OIDNewHeap; Relation OldHeap; HeapTuple tuple; - ArrayType *options; + Datum reloptions; + bool isNull; OldHeap = heap_open(OIDOldHeap, AccessExclusiveLock); OldHeapDesc = RelationGetDescr(OldHeap); @@ -584,19 +585,12 @@ make_new_heap(Oid OIDOldHeap, const char *NewName, Oid NewTableSpace) tuple = SearchSysCache(RELOID, ObjectIdGetDatum(OIDOldHeap), 0, 0, 0); - if (tuple) - { - Datum datum; - bool isNull; - datum = SysCacheGetAttr(RELOID, tuple, - Anum_pg_class_reloptions, &isNull); - options = isNull ? NULL : DatumGetArrayTypeP(datum); - } - else - { - /* should not happen */ - options = NULL; - } + if (!HeapTupleIsValid(tuple)) + elog(ERROR, "cache lookup failed for relation %u", OIDOldHeap); + reloptions = SysCacheGetAttr(RELOID, tuple, Anum_pg_class_reloptions, + &isNull); + if (isNull) + reloptions = (Datum) 0; OIDNewHeap = heap_create_with_catalog(NewName, RelationGetNamespace(OldHeap), @@ -609,8 +603,8 @@ make_new_heap(Oid OIDOldHeap, const char *NewName, Oid NewTableSpace) true, 0, ONCOMMIT_NOOP, - allowSystemTableMods, - options); + reloptions, + allowSystemTableMods); ReleaseSysCache(tuple); diff --git a/src/backend/commands/define.c b/src/backend/commands/define.c index ebde8f10950..149e1b6dae0 100644 --- a/src/backend/commands/define.c +++ b/src/backend/commands/define.c @@ -9,7 +9,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/commands/define.c,v 1.96 2006/07/02 02:23:19 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/commands/define.c,v 1.97 2006/07/03 22:45:38 tgl Exp $ * * DESCRIPTION * The "DefineFoo" routines take the parse tree and pick out the @@ -110,7 +110,6 @@ defGetNumeric(DefElem *def) case T_Integer: return (double) intVal(def->arg); case T_Float: - case T_String: /* XXX: needs strict check? */ return floatVal(def->arg); default: ereport(ERROR, @@ -128,27 +127,39 @@ bool defGetBoolean(DefElem *def) { /* - * Presently, boolean flags must simply be present/absent or - * integer 0/1. Later we could allow 'flag = t', 'flag = f', etc. + * If no parameter given, assume "true" is meant. */ if (def->arg == NULL) return true; + /* + * Allow 0, 1, "true", "false" + */ switch (nodeTag(def->arg)) { case T_Integer: switch (intVal(def->arg)) { - case 0: - return false; - case 1: - return true; + case 0: + return false; + case 1: + return true; + default: + /* otherwise, error out below */ + break; } break; default: + { + char *sval = defGetString(def); + + if (pg_strcasecmp(sval, "true") == 0) + return true; + if (pg_strcasecmp(sval, "false") == 0) + return false; + + } break; } - - /* on error */ ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("%s requires a boolean value", @@ -172,7 +183,7 @@ defGetInt64(DefElem *def) case T_Integer: return (int64) intVal(def->arg); case T_Float: - case T_String: /* XXX: needs strict check? */ + /* * Values too large for int4 will be represented as Float * constants by the lexer. Accept these if they are valid int8 @@ -293,10 +304,14 @@ defGetTypeLength(DefElem *def) return 0; /* keep compiler quiet */ } +/* + * Create a DefElem setting "oids" to the specified value. + */ DefElem * defWithOids(bool value) { DefElem *f = makeNode(DefElem); + f->defname = "oids"; f->arg = (Node *)makeInteger(value); return f; diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 7e35258ac09..d2c4bf89059 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/commands/indexcmds.c,v 1.142 2006/07/02 02:23:19 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/commands/indexcmds.c,v 1.143 2006/07/03 22:45:38 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -17,6 +17,7 @@ #include "access/genam.h" #include "access/heapam.h" +#include "access/reloptions.h" #include "catalog/catalog.h" #include "catalog/dependency.h" #include "catalog/heap.h" @@ -72,12 +73,12 @@ static bool relationHasPrimaryKey(Relation rel); * to index on. * 'predicate': the partial-index condition, or NULL if none. * 'rangetable': needed to interpret the predicate. + * 'options': reloptions from WITH (in list-of-DefElem form). * 'unique': make the index enforce uniqueness. * 'primary': mark the index as a primary key in the catalogs. * 'isconstraint': index is for a PRIMARY KEY or UNIQUE constraint, * so build a pg_constraint entry for it. * 'is_alter_table': this is due to an ALTER rather than a CREATE operation. - * 'options': options passed by WITH. * 'check_rights': check for CREATE rights in the namespace. (This should * be true except when ALTER is deleting/recreating an index.) * 'skip_build': make the catalog entries but leave the index file empty; @@ -110,6 +111,8 @@ DefineIndex(RangeVar *heapRelation, Relation rel; HeapTuple tuple; Form_pg_am accessMethodForm; + RegProcedure amoptions; + Datum reloptions; IndexInfo *indexInfo; int numberOfAttributes; @@ -261,6 +264,8 @@ DefineIndex(RangeVar *heapRelation, errmsg("access method \"%s\" does not support multicolumn indexes", accessMethodName))); + amoptions = accessMethodForm->amoptions; + ReleaseSysCache(tuple); /* @@ -368,6 +373,13 @@ DefineIndex(RangeVar *heapRelation, } /* + * Parse AM-specific options, convert to text array form, validate. + */ + reloptions = transformRelOptions((Datum) 0, options, false, false); + + (void) index_reloptions(amoptions, reloptions, true); + + /* * Prepare arguments for index_create, primarily an IndexInfo structure. * Note that ii_Predicate must be in implicit-AND format. */ @@ -399,7 +411,7 @@ DefineIndex(RangeVar *heapRelation, index_create(relationId, indexRelationName, indexRelationId, indexInfo, accessMethodId, tablespaceId, classObjectId, - options, primary, false, isconstraint, + reloptions, primary, false, isconstraint, allowSystemTableMods, skip_build); } diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c index 27ccb3fb879..78ec950f235 100644 --- a/src/backend/commands/prepare.c +++ b/src/backend/commands/prepare.c @@ -10,7 +10,7 @@ * Copyright (c) 2002-2006, PostgreSQL Global Development Group * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/commands/prepare.c,v 1.54 2006/07/02 02:23:19 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/commands/prepare.c,v 1.55 2006/07/03 22:45:38 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -197,7 +197,6 @@ ExecuteQuery(ExecuteStmt *stmt, ParamListInfo params, errmsg("prepared statement is not a SELECT"))); query->into = copyObject(stmt->into); query->intoOptions = copyObject(stmt->intoOptions); - query->intoHasOids = stmt->into_has_oids; query->intoOnCommit = stmt->into_on_commit; if (stmt->into_tbl_space) query->intoTableSpaceName = pstrdup(stmt->into_tbl_space); diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c index 65712c2092c..cae956c22d9 100644 --- a/src/backend/commands/sequence.c +++ b/src/backend/commands/sequence.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/commands/sequence.c,v 1.133 2006/07/02 02:23:19 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/commands/sequence.c,v 1.134 2006/07/03 22:45:38 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -205,17 +205,7 @@ DefineSequence(CreateSeqStmt *seq) /* Now form & insert sequence tuple */ tuple = heap_formtuple(tupDesc, value, null); - - { - /* - * HACK: Sequences insert only one tuple during initialize. - * We treat sequences as heaps then. - */ - HeapOption opaque = { sizeof(HeapOption), 100 }; - rel->rd_options = (bytea *) &opaque; - simple_heap_insert(rel, tuple); - rel->rd_options = NULL; - } + simple_heap_insert(rel, tuple); Assert(ItemPointerGetOffsetNumber(&(tuple->t_self)) == FirstOffsetNumber); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 7b2a0508feb..f67f1b62ec4 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -8,13 +8,14 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.191 2006/07/02 05:17:26 neilc Exp $ + * $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.192 2006/07/03 22:45:38 tgl Exp $ * *------------------------------------------------------------------------- */ #include "postgres.h" #include "access/genam.h" +#include "access/reloptions.h" #include "access/tuptoaster.h" #include "catalog/catalog.h" #include "catalog/dependency.h" @@ -62,6 +63,7 @@ #include "utils/relcache.h" #include "utils/syscache.h" + /* * ON COMMIT action list */ @@ -248,7 +250,7 @@ static void ATExecDropCluster(Relation rel); static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel, char *tablespacename); static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace); -static void ATExecSetOptions(Relation rel, List *newOptions); +static void ATExecSetRelOptions(Relation rel, List *defList, bool isReset); static void ATExecEnableDisableTrigger(Relation rel, char *trigname, bool enable, bool skip_system); static void ATExecAddInherits(Relation rel, RangeVar *parent); @@ -283,10 +285,10 @@ DefineRelation(CreateStmt *stmt, char relkind) bool localHasOids; int parentOidCount; List *rawDefaults; + Datum reloptions; ListCell *listptr; int i; AttrNumber attnum; - ArrayType *options; /* * Truncate relname to appropriate length (probably a waste of time, as @@ -339,6 +341,13 @@ DefineRelation(CreateStmt *stmt, char relkind) /* note InvalidOid is OK in this case */ } + /* + * Parse and validate reloptions, if any. + */ + reloptions = transformRelOptions((Datum) 0, stmt->options, true, false); + + (void) heap_reloptions(relkind, reloptions, true); + /* Check permissions except when using database's default */ if (OidIsValid(tablespaceId)) { @@ -428,7 +437,6 @@ DefineRelation(CreateStmt *stmt, char relkind) } } - options = OptionBuild(NULL, stmt->options); relationId = heap_create_with_catalog(relname, namespaceId, tablespaceId, @@ -440,10 +448,8 @@ DefineRelation(CreateStmt *stmt, char relkind) localHasOids, parentOidCount, stmt->oncommit, - allowSystemTableMods, - options); - if (options) - pfree(options); + reloptions, + allowSystemTableMods); StoreCatalogInheritance(relationId, inheritOids); @@ -2103,7 +2109,8 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, ATPrepSetTableSpace(tab, rel, cmd->name); pass = AT_PASS_MISC; /* doesn't actually matter */ break; - case AT_SetOptions: /* SET (...) */ + case AT_SetRelOptions: /* SET (...) */ + case AT_ResetRelOptions: /* RESET (...) */ ATSimplePermissionsRelationOrIndex(rel); /* This command never recurses */ /* No command-specific prep needed */ @@ -2279,8 +2286,11 @@ ATExecCmd(AlteredTableInfo *tab, Relation rel, AlterTableCmd *cmd) * Nothing to do here; Phase 3 does the work */ break; - case AT_SetOptions: /* SET (...) */ - ATExecSetOptions(rel, (List *) cmd->def); + case AT_SetRelOptions: /* SET (...) */ + ATExecSetRelOptions(rel, (List *) cmd->def, false); + break; + case AT_ResetRelOptions: /* RESET (...) */ + ATExecSetRelOptions(rel, (List *) cmd->def, true); break; case AT_EnableTrig: /* ENABLE TRIGGER name */ ATExecEnableDisableTrigger(rel, cmd->name, true, false); @@ -5757,24 +5767,29 @@ ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel, char *tablespacename) } /* - * ALTER TABLE/INDEX SET (...) + * ALTER TABLE/INDEX SET (...) or RESET (...) */ static void -ATExecSetOptions(Relation rel, List *newOptions) +ATExecSetRelOptions(Relation rel, List *defList, bool isReset) { Oid relid; Relation pgclass; HeapTuple tuple; + HeapTuple newtuple; Datum datum; bool isnull; - ArrayType *mergedOptions; - bytea *options; + Datum newOptions; + Datum repl_val[Natts_pg_class]; + char repl_null[Natts_pg_class]; + char repl_repl[Natts_pg_class]; - if (list_length(newOptions) == 0) - return; /* do nothing */ + if (defList == NIL) + return; /* nothing to do */ - relid = RelationGetRelid(rel); pgclass = heap_open(RelationRelationId, RowExclusiveLock); + + /* Get the old reloptions */ + relid = RelationGetRelid(rel); tuple = SearchSysCache(RELOID, ObjectIdGetDatum(relid), 0, 0, 0); @@ -5783,59 +5798,54 @@ ATExecSetOptions(Relation rel, List *newOptions) datum = SysCacheGetAttr(RELOID, tuple, Anum_pg_class_reloptions, &isnull); - mergedOptions = OptionBuild( - isnull ? NULL : DatumGetArrayTypeP(datum), newOptions); + /* Generate new proposed reloptions (text array) */ + newOptions = transformRelOptions(isnull ? (Datum) 0 : datum, + defList, false, isReset); + /* Validate */ switch (rel->rd_rel->relkind) { case RELKIND_RELATION: case RELKIND_TOASTVALUE: - options = heap_option(rel->rd_rel->relkind, mergedOptions); + (void) heap_reloptions(rel->rd_rel->relkind, newOptions, true); break; case RELKIND_INDEX: - options = index_option(rel->rd_am->amoption, mergedOptions); + (void) index_reloptions(rel->rd_am->amoptions, newOptions, true); break; default: - elog(ERROR, "unexpected RELKIND=%c", rel->rd_rel->relkind); - options = NULL; /* keep compiler quiet */ + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("\"%s\" is not a table, index, or TOAST table", + RelationGetRelationName(rel)))); break; } - if (rel->rd_options != options) - { - HeapTuple newtuple; - Datum repl_val[Natts_pg_class]; - char repl_null[Natts_pg_class]; - char repl_repl[Natts_pg_class]; - - /* XXX: This is not necessarily required. */ - if (rel->rd_options) - pfree(rel->rd_options); - rel->rd_options = options; + /* + * All we need do here is update the pg_class row; the new options will be + * propagated into relcaches during post-commit cache inval. + */ + memset(repl_val, 0, sizeof(repl_val)); + memset(repl_null, ' ', sizeof(repl_null)); + memset(repl_repl, ' ', sizeof(repl_repl)); - memset(repl_repl, ' ', sizeof(repl_repl)); - memset(repl_null, ' ', sizeof(repl_null)); - repl_repl[Anum_pg_class_reloptions - 1] = 'r'; + if (newOptions != (Datum) 0) + repl_val[Anum_pg_class_reloptions - 1] = newOptions; + else + repl_null[Anum_pg_class_reloptions - 1] = 'n'; - if (mergedOptions) - repl_val[Anum_pg_class_reloptions - 1] = - PointerGetDatum(mergedOptions); - else - repl_null[Anum_pg_class_reloptions - 1] = 'n'; + repl_repl[Anum_pg_class_reloptions - 1] = 'r'; - newtuple = heap_modifytuple(tuple, RelationGetDescr(pgclass), - repl_val, repl_null, repl_repl); + newtuple = heap_modifytuple(tuple, RelationGetDescr(pgclass), + repl_val, repl_null, repl_repl); - simple_heap_update(pgclass, &newtuple->t_self, newtuple); - CatalogUpdateIndexes(pgclass, newtuple); + simple_heap_update(pgclass, &newtuple->t_self, newtuple); - heap_freetuple(newtuple); - } + CatalogUpdateIndexes(pgclass, newtuple); - if (mergedOptions) - pfree(mergedOptions); + heap_freetuple(newtuple); ReleaseSysCache(tuple); + heap_close(pgclass, RowExclusiveLock); } @@ -6642,6 +6652,9 @@ AlterTableCreateToastTable(Oid relOid, bool silent) * even if its master relation is a temp table. There cannot be any * naming collision, and the toast rel will be destroyed when its master * is, so there's no need to handle the toast rel as temp. + * + * XXX would it make sense to apply the master's reloptions to the toast + * table? */ toast_relid = heap_create_with_catalog(toast_relname, PG_TOAST_NAMESPACE, @@ -6654,8 +6667,8 @@ AlterTableCreateToastTable(Oid relOid, bool silent) true, 0, ONCOMMIT_NOOP, - true, - NULL); + (Datum) 0, + true); /* make the toast relation visible, else index creation will fail */ CommandCounterIncrement(); @@ -6689,7 +6702,7 @@ AlterTableCreateToastTable(Oid relOid, bool silent) indexInfo, BTREE_AM_OID, rel->rd_rel->reltablespace, - classObjectId, NIL, + classObjectId, (Datum) 0, true, true, false, true, false); /* diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 24a93e998de..17a802dc30e 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -13,7 +13,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/commands/vacuum.c,v 1.331 2006/07/02 02:23:19 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/commands/vacuum.c,v 1.332 2006/07/03 22:45:38 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -3119,8 +3119,6 @@ vac_update_fsm(Relation onerel, VacPageList fraged_pages, * vacuumlazy.c does, we'd be skewing that statistic. */ threshold = GetAvgFSMRequestSize(&onerel->rd_node); - if (threshold < HeapGetPageFreeSpace(onerel)) - threshold = HeapGetPageFreeSpace(onerel); pageSpaces = (PageFreeSpaceInfo *) palloc(nPages * sizeof(PageFreeSpaceInfo)); @@ -3391,11 +3389,13 @@ static Size PageGetFreeSpaceWithFillFactor(Relation relation, Page page) { PageHeader pd = (PageHeader) page; - Size pagefree = HeapGetPageFreeSpace(relation); Size freespace = pd->pd_upper - pd->pd_lower; + Size targetfree; - if (freespace > pagefree) - return freespace - pagefree; + targetfree = RelationGetTargetPageFreeSpace(relation, + HEAP_DEFAULT_FILLFACTOR); + if (freespace > targetfree) + return freespace - targetfree; else return 0; } diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c index 8e97fa47563..264cb437786 100644 --- a/src/backend/commands/vacuumlazy.c +++ b/src/backend/commands/vacuumlazy.c @@ -31,7 +31,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/commands/vacuumlazy.c,v 1.71 2006/07/02 02:23:20 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/commands/vacuumlazy.c,v 1.72 2006/07/03 22:45:38 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -149,8 +149,6 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt) /* Set threshold for interesting free space = average request size */ /* XXX should we scale it up or down? Adjust vacuum.c too, if so */ vacrelstats->threshold = GetAvgFSMRequestSize(&onerel->rd_node); - if (vacrelstats->threshold < HeapGetPageFreeSpace(onerel)) - vacrelstats->threshold = HeapGetPageFreeSpace(onerel); /* Open all indexes of the relation */ vac_open_indexes(onerel, ShareUpdateExclusiveLock, &nindexes, &Irel); |