On Thu, Jul 30, 2015 at 11:28 PM, Andres Freund <and...@anarazel.de> wrote:
>
> > @@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =
>
> If we go through this list, I'd rather make informed decisions about
> each reloption. Otherwise we're going to get patches for each of them
> separately over the next versions.
>
I have no problem to do this change now instead of wait for next versions...
> > @@ -73,7 +75,8 @@ static relopt_bool boolRelOpts[] =
> > {
> > "fastupdate",
> > "Enables \"fast update\" feature for this GIN
index",
> > - RELOPT_KIND_GIN
> > + RELOPT_KIND_GIN,
> > + AccessExclusiveLock
> > },
> > true
> > },
>
>
> > @@ -95,7 +99,8 @@ static relopt_int intRelOpts[] =
> > {
> > "fillfactor",
> > "Packs table pages only to this percentage",
> > - RELOPT_KIND_HEAP
> > + RELOPT_KIND_HEAP,
> > + AccessExclusiveLock
> > },
> > HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100
>
> > [some other fillfactor settings]
>
> > {
> > {
> > "gin_pending_list_limit",
> > "Maximum size of the pending list for this GIN
index, in kilobytes.",
> > - RELOPT_KIND_GIN
> > + RELOPT_KIND_GIN,
> > + AccessExclusiveLock
> > },
> > -1, 64, MAX_KILOBYTES
> > },
> > @@ -297,7 +325,8 @@ static relopt_string stringRelOpts[] =
> > {
> > "buffering",
> > "Enables buffering build for this GiST index",
> > - RELOPT_KIND_GIST
> > + RELOPT_KIND_GIST,
> > + AccessExclusiveLock
> > },
> > 4,
> > false,
>
> Why? These options just change things for the future and don't influence
> past decisions. It seems unproblematic to change them.
>
+1
> > @@ -259,7 +283,8 @@ static relopt_real realRelOpts[] =
> > {
> > "seq_page_cost",
> > "Sets the planner's estimate of the cost of a
sequentially fetched disk page.",
> > - RELOPT_KIND_TABLESPACE
> > + RELOPT_KIND_TABLESPACE,
> > + AccessExclusiveLock
> > },
> > -1, 0.0, DBL_MAX
> > },
> > @@ -267,7 +292,8 @@ static relopt_real realRelOpts[] =
> > {
> > "random_page_cost",
> > "Sets the planner's estimate of the cost of a
nonsequentially fetched disk page.",
> > - RELOPT_KIND_TABLESPACE
> > + RELOPT_KIND_TABLESPACE,
> > + AccessExclusiveLock
> > },
> > -1, 0.0, DBL_MAX
> > },
> > @@ -275,7 +301,8 @@ static relopt_real realRelOpts[] =
> > {
> > "n_distinct",
> > "Sets the planner's estimate of the number of
distinct values appearing in a column (excluding child relations).",
> > - RELOPT_KIND_ATTRIBUTE
> > + RELOPT_KIND_ATTRIBUTE,
> > + AccessExclusiveLock
> > },
> > 0, -1.0, DBL_MAX
> > },
> > @@ -283,7 +310,8 @@ static relopt_real realRelOpts[] =
> > {
> > "n_distinct_inherited",
> > "Sets the planner's estimate of the number of
distinct values appearing in a column (including child relations).",
> > - RELOPT_KIND_ATTRIBUTE
> > + RELOPT_KIND_ATTRIBUTE,
> > + AccessExclusiveLock
> > },
> > 0, -1.0, DBL_MAX
> > },
>
> These probably are the settings that are most interesting to change
> without access exlusive locks.
>
+1
> > j = 0;
> > for (i = 0; boolRelOpts[i].gen.name; i++)
> > + {
> > + Assert(DoLockModesConflict(boolRelOpts[i].gen.lockmode,
> > +
boolRelOpts[i].gen.lockmode));
> > j++;
> > + }
> > for (i = 0; intRelOpts[i].gen.name; i++)
> > + {
> > + Assert(DoLockModesConflict(intRelOpts[i].gen.lockmode,
> > +
intRelOpts[i].gen.lockmode));
> > j++;
> > + }
> > for (i = 0; realRelOpts[i].gen.name; i++)
> > + {
> > + Assert(DoLockModesConflict(realRelOpts[i].gen.lockmode,
> > +
realRelOpts[i].gen.lockmode));
> > j++;
> > + }
> > for (i = 0; stringRelOpts[i].gen.name; i++)
> > + {
> > + Assert(DoLockModesConflict(stringRelOpts[i].gen.lockmode,
> > +
stringRelOpts[i].gen.lockmode));
> > j++;
> > + }
> > j += num_custom_options;
>
> Doesn't really seem worth it to assert individually in each case here to
> me.
>
What do you suggest then?
> > +/*
> > + * Determine the required LOCKMODE from an option list
> > + */
> > +LOCKMODE
> > +GetRelOptionsLockLevel(List *defList)
> > +{
> > + LOCKMODE lockmode = NoLock;
> > + ListCell *cell;
> > +
> > + if (defList == NIL)
> > + return AccessExclusiveLock;
> > +
> > + if (need_initialization)
> > + initialize_reloptions();
> > +
> > + foreach(cell, defList)
> > + {
> > + DefElem *def = (DefElem *) lfirst(cell);
> > + int i;
> > +
> > + for (i = 0; relOpts[i]; i++)
> > + {
> > + if (pg_strncasecmp(relOpts[i]->name,
def->defname, relOpts[i]->namelen + 1) == 0)
> > + {
> > + if (lockmode < relOpts[i]->lockmode)
> > + lockmode = relOpts[i]->lockmode;
> > + }
> > + }
> > + }
> > +
> > + return lockmode;
> > +}
>
> We usually don't compare lock values that way, i.e. there's not
> guaranteed to be a strict monotonicity between lock levels. I don't
> really agree with that policy, but it's nonetheless there.
>
And how is the better way to compare lock values to get the highest lock
level? Perhaps creating a function to compare lock levels?
Regards,
*** This work is funded by Zenvia Mobile Results (http://www.zenvia.com.br)
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
