Re: [PATCH] Store Extension Options

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/)

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

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.

-- Abhijit