Re: [PATCH v1] [doc] polish the comments of reloptions

Patch applied.

---------------------------------------------------------------------------

On Tue, Aug 30, 2022 at 11:56:30AM +0800, Junwang Zhao wrote:
> When adding an option, we have 5 choices (bool, integer, real, enum, string),
> so the comments seem stale.
>
> There are some sentences missing *at ShareUpdateExclusiveLock*, this
> patch adds them to make the sentences complete.
>
> One thing I'm not sure is should we use *at ShareUpdateExclusiveLock* or
> *with ShareUpdateExclusiveLock*, pls take a look.
>
> src/backend/access/common/reloptions.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/src/backend/access/common/reloptions.c
> b/src/backend/access/common/reloptions.c
> index 609329bb21..9e99868faa 100644
> --- a/src/backend/access/common/reloptions.c
> +++ b/src/backend/access/common/reloptions.c
> @@ -42,9 +42,9 @@
> *
> * To add an option:
> *
> - * (i) decide on a type (integer, real, bool, string), name, default value,
> - * upper and lower bounds (if applicable); for strings, consider a validation
> - * routine.
> + * (i) decide on a type (bool, integer, real, enum, string), name, default
> + * value, upper and lower bounds (if applicable); for strings, consider a
> + * validation routine.
> * (ii) add a record below (or use add_<type>_reloption).
> * (iii) add it to the appropriate options struct (perhaps StdRdOptions)
> * (iv) add it to the appropriate handling routine (perhaps
> @@ -68,24 +68,24 @@
> * since they are only used by the AV procs and don't change anything
> * currently executing.
> *
> - * Fillfactor can be set because it applies only to subsequent changes made to
> - * data blocks, as documented in hio.c
> + * Fillfactor can be set at ShareUpdateExclusiveLock because it applies only to
> + * subsequent changes made to data blocks, as documented in hio.c
> *
> * n_distinct options can be set at ShareUpdateExclusiveLock because they
> * are only used during ANALYZE, which uses a ShareUpdateExclusiveLock,
> * so the ANALYZE will not be affected by in-flight changes. Changing those
> * values has no effect until the next ANALYZE, so no need for stronger lock.
> *
> - * Planner-related parameters can be set with ShareUpdateExclusiveLock because
> + * Planner-related parameters can be set at ShareUpdateExclusiveLock because
> * they only affect planning and not the correctness of the execution. Plans
> * cannot be changed in mid-flight, so changes here could not easily result in
> * new improved plans in any case. So we allow existing queries to continue
> * and existing plans to survive, a small price to pay for allowing better
> * plans to be introduced concurrently without interfering with users.
> *
> - * Setting parallel_workers is safe, since it acts the same as
> - * max_parallel_workers_per_gather which is a USERSET parameter that doesn't
> - * affect existing plans or queries.
> + * Setting parallel_workers at ShareUpdateExclusiveLock is safe, since it acts
> + * the same as max_parallel_workers_per_gather which is a USERSET parameter
> + * that doesn't affect existing plans or queries.
> *
> * vacuum_truncate can be set at ShareUpdateExclusiveLock because it
> * is only used during VACUUM, which uses a ShareUpdateExclusiveLock,
> --
> 2.33.0
>
>
> --
> Regards
> Junwang Zhao

> From 4cef73a6f7ef4b59a6cc1cd9720b4e545bc36861 Mon Sep 17 00:00:00 2001
> From: Junwang Zhao <zhjwpku(at)gmail(dot)com>
> Date: Tue, 30 Aug 2022 11:33:14 +0800
> Subject: [PATCH v1] [doc] polish the comments of reloptions
>
> 1. add the missing enum type and change the order to consistent
> with relopt_type
> 2. add some missing ShareUpdateExclusiveLock
> ---
> src/backend/access/common/reloptions.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
> index 609329bb21..9e99868faa 100644
> --- a/src/backend/access/common/reloptions.c
> +++ b/src/backend/access/common/reloptions.c
> @@ -42,9 +42,9 @@
> *
> * To add an option:
> *
> - * (i) decide on a type (integer, real, bool, string), name, default value,
> - * upper and lower bounds (if applicable); for strings, consider a validation
> - * routine.
> + * (i) decide on a type (bool, integer, real, enum, string), name, default
> + * value, upper and lower bounds (if applicable); for strings, consider a
> + * validation routine.
> * (ii) add a record below (or use add_<type>_reloption).
> * (iii) add it to the appropriate options struct (perhaps StdRdOptions)
> * (iv) add it to the appropriate handling routine (perhaps
> @@ -68,24 +68,24 @@
> * since they are only used by the AV procs and don't change anything
> * currently executing.
> *
> - * Fillfactor can be set because it applies only to subsequent changes made to
> - * data blocks, as documented in hio.c
> + * Fillfactor can be set at ShareUpdateExclusiveLock because it applies only to
> + * subsequent changes made to data blocks, as documented in hio.c
> *
> * n_distinct options can be set at ShareUpdateExclusiveLock because they
> * are only used during ANALYZE, which uses a ShareUpdateExclusiveLock,
> * so the ANALYZE will not be affected by in-flight changes. Changing those
> * values has no effect until the next ANALYZE, so no need for stronger lock.
> *
> - * Planner-related parameters can be set with ShareUpdateExclusiveLock because
> + * Planner-related parameters can be set at ShareUpdateExclusiveLock because
> * they only affect planning and not the correctness of the execution. Plans
> * cannot be changed in mid-flight, so changes here could not easily result in
> * new improved plans in any case. So we allow existing queries to continue
> * and existing plans to survive, a small price to pay for allowing better
> * plans to be introduced concurrently without interfering with users.
> *
> - * Setting parallel_workers is safe, since it acts the same as
> - * max_parallel_workers_per_gather which is a USERSET parameter that doesn't
> - * affect existing plans or queries.
> + * Setting parallel_workers at ShareUpdateExclusiveLock is safe, since it acts
> + * the same as max_parallel_workers_per_gather which is a USERSET parameter
> + * that doesn't affect existing plans or queries.
> *
> * vacuum_truncate can be set at ShareUpdateExclusiveLock because it
> * is only used during VACUUM, which uses a ShareUpdateExclusiveLock,
> --
> 2.33.0
>

--
Bruce Momjian <bruce(at)momjian(dot)us> https://momjian.us
EDB https://enterprisedb.com

Only you can decide what is important to you.