Re: [PATCH] Store Extension Options

On Fri, Feb 28, 2014 at 5:08 AM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:
>
> Hi Fabrízio.
>
> Here are a few comments based on a quick look at your updated patch.
>
> At 2014-02-13 22:44:56 -0200, fabriziomello@gmail.com wrote:
> >
> > diff --git a/doc/src/sgml/ref/alter_index.sgml b/doc/src/sgml/ref/alter_index.sgml
> > index d210077..5e9ee9d 100644
> > --- a/doc/src/sgml/ref/alter_index.sgml
> > +++ b/doc/src/sgml/ref/alter_index.sgml
> > @@ -82,6 +82,14 @@ ALTER INDEX [ IF EXISTS ] <replaceable class="PARAMETER">name</replaceable> RESE
> >        <xref linkend="SQL-REINDEX">
> >        to get the desired effects.
> >       </para>
> > +     <note>
> > +       <para>
> > +         A custom name can be used as namespace to define a storage parameter.
> > +         Storage option pattern: namespace.option=value
> > +         (namespace=custom name, option=option name and value=option value).
> > +         See example bellow.
> > +       </para>
> > +     </note>
> >      </listitem>
> >     </varlistentry>
>
> I was slightly confused by the wording here. I think it would be better
> to say something like "Custom storage parameters are of the form
> namespace.option" and leave it at that.
>
> (Aside: s/bellow/below/)
>

> > @@ -202,6 +210,17 @@ ALTER INDEX distributors SET (fillfactor = 75);
> >  REINDEX INDEX distributors;
> >  </programlisting></para>
> >
> > +  <para>
> > +   To set a custom storage parameter:
> > +<programlisting>
> > +ALTER INDEX distributors
> > +  SET (bdr.do_replicate=true);
> > +</programlisting>
> > +   (bdr=custom name, do_replicate=option name and
> > +   true=option value)
> > +</para>
> > +
> > +
> >   </refsect1>
>
> It might be best to avoid using bdr.do_replicate in the example, since
> it's still a moving target. It might be better to use a generic example
> like somenamespace.optionname=true, in which case the explanation isn't
> needed either.
>

Fixed.

> The patch applies and builds fine, the tests pass, and the code looks
> OK to me. I don't have a strong opinion on validating custom reloption
> values through hooks as discussed earlier in the thread, but the simple
> version (i.e. your latest patch) seems at least a useful starting point.
>

Thanks for your review.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello