Re: [PATCH] Store Extension Options

On Fri, Feb 28, 2014 at 5:08 AM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)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(at)gmail(dot)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/)
>

You are correct... my english isn't so good... sorry!

Fixed.

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