Re: Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. ); - Mailing list pgsql-hackers

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