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.