Date: Sun, 5 Jan 2020 22:13:14 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Simon Riggs <simon@2ndquadrant.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: doc: alter table references bogus table-specific planner
 parameters
Message-ID: <20200106041314.GB12066@telsasoft.com>
References: <20200103141427.GK12066@telsasoft.com>
 <CAMkU=1xcf-igvHSrta+gnTgE+K+Cnc-F=7z8G6YJTHwNSCyP1Q@mail.gmail.com>
 <20200104165047.GW12066@telsasoft.com>
 <20200106025623.GA12066@telsasoft.com>
 <CANP8+jJLEBva0Yq-qHz0r0SFyrFDG_O7GTF1bcqEmFXrtP81-A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CANP8+jJLEBva0Yq-qHz0r0SFyrFDG_O7GTF1bcqEmFXrtP81-A@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

On Mon, Jan 06, 2020 at 03:48:52AM +0000, Simon Riggs wrote:
> On Mon, 6 Jan 2020 at 02:56, Justin Pryzby <pryzby@telsasoft.com> wrote:
> 
> > commit 6f3a13ff058f15d565a30c16c0c2cb14cc994e42 Enhance docs for ALTER TABLE lock levels of storage parms
> > Author: Simon Riggs <simon@2ndQuadrant.com>
> > Date:   Mon Mar 6 16:48:12 2017 +0530
> >
> >     <varlistentry>
> >      <term><literal>SET ( <replaceable
> > class="PARAMETER">storage_parameter</replaceable> = <replaceable
> > class="PARAMETER">value</replaceable> [, ... ] )</literal></term>
> > ...
> > -      Changing fillfactor and autovacuum storage parameters acquires a
> > <literal>SHARE UPDATE EXCLUSIVE</literal> lock.
> > +      <literal>SHARE UPDATE EXCLUSIVE</literal> lock will be taken for
> > +      fillfactor and autovacuum storage parameters, as well as the
> > +      following planner related parameters:
> > +      effective_io_concurrency, parallel_workers, seq_page_cost
> > +      random_page_cost, n_distinct and n_distinct_inherited.
> >
> > effective_io_concurrency, seq_page_cost and random_page_cost cannot be set
> > for
> > a table - reloptions.c shows that they've always been
> > RELOPT_KIND_TABLESPACE.
> 
> I agree with the sentiment of the third doc change, but your patch removes
> the mention of n_distinct, which isn't appropriate.

I think it's correct to remove n_distinct there, as it's documented previously,
since e5550d5f.  That's a per-attribute option (not storage) and can't be
specified there.

    <varlistentry>
     <term><literal>SET ( <replaceable class="PARAMETER">attribute_option</replaceable> = <replaceable class="PARAMETER">value</replaceable> [, ... ] )</literal></term>
     <term><literal>RESET ( <replaceable class="PARAMETER">attribute_option</replaceable> [, ... ] )</literal></term>
     <listitem>
      <para>
       This form sets or resets per-attribute options.  Currently, the only
...
+     <para>
+      Changing per-attribute options acquires a
+      <literal>SHARE UPDATE EXCLUSIVE</literal> lock.
+     </para>

> The second change in your patch alters the meaning of the sentence in a way
> that is counter to the first change. The name of these parameters is
> "Storage Parameters" (in various places); I might agree with describing
> them in text as "storage or planner parameters", but if you do that you
> can't then just refer to "storage parameters" later, because if you do it
> implies that planner parameters operate differently to storage parameters,
> which they don't.

The 2nd change is:

       for details on the available parameters.  Note that the table contents
-      will not be modified immediately by this command; depending on the
+      will not be modified immediately by setting its storage parameters; depending on the
       parameter you might need to rewrite the table to get the desired effects.

I deliberately qualified that as referring only to "storage params" rather than
"this command", since planner params never "modify the table contents".
Possibly other instances in the document (and createtable) should be changed
for consistency.

Justin