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

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

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

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

> +/*
> + * 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.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to