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

thoughts?

On Tue, Aug 30, 2022 at 11:56 AM Junwang Zhao <zhjwpku@gmail.com> 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

--
Regards
Junwang Zhao

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@gmail.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@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Only you can decide what is important to you.