Re: generic reloptions improvement

KaiGai Kohei wrote:

> (1) Who/Where should allocate a string area?
>
> + /* Note that this assumes that the variable is already allocated! */
> + #define HANDLE_STRING_RELOPTION(optname, var, option)                 \
> +       if (HAVE_RELOPTION(optname, option))                        \
> +       {                                                           \
> +           strcpy(var,                                             \
> +                  option.isset ? option.values.string_val :        \
> +                  ((relopt_string *) option.gen)->default_val);    \
> +           continue;                                               \
> +       }
>
> I think HANDLE_STRING_RELOPTION() should allocate a string area via
> pstrdup(). If we have to put individual pstrdup() for each reloptions,
> it will make noisy listing of codes.
>
> How do you think:
>
> #define HANDLE_STRING_RELOPTION(optname, var, option)             \
>     if (HAVE_RELOPTION(optname, option))                          \
>     {                                                             \
>         char *tmp = (option.isset ? option.values.string_val :    \
>                     ((relopt_string *) option.gen)->default_val); \
>         var = pstrdup(tmp);                                       \
>         continue;                                                 \
>     }

Well, that's precisely the problem with string options.  If we want
memory to be freed properly, we can only allocate a single chunk which
is what's going to be stored under the rd_options bytea pointer.
Allocating separately doesn't work because we need to rebuild the
relcache entry (freeing it and allocating a new one) when it is
invalidated for whatever reason.  Since the relcache code cannot follow
a pointer stored in the bytea area, this would result in a permanent
memory leak.

So the rule I came up with is that the caller is responsible for
allocating it -- but it must be inside the bytea area to be returned.
Below is a sample amoptions routine to show how it works.  Note that
this is exactly why I said that only a single string option can be
supported.

If you have a better idea, I'm all ears.

> (2) How does it represent NULL in string_option?
>
> It seems to me we cannot represent a NULL string in the default.
> Is it possible to add a mark to indicate NULL, like "bool default_null"
> within struct relopt_string?

Ah, good point.  I'll have a look at this.

> (3) heap_reloptions() from RelationParseRelOptions() makes a trouble.

This is the same as (1) actually.

--
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.