Re: generic reloptions improvement

Alvaro Herrera wrote:
> 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 the caller allocates a surplus area to store string on tail of the
StdRdOptions (or others), the string option can be represented as an
offset value and should be accessed via macros, like:

struct StdRdOptions
{
int32 vl_len_;
int fillfactor;
int test_option_a; /* indicate offset of the surplus area from head */
int test_option_b; /* of this structure. */
/* in actually surplus area is allocated here */
};

#define GetOptionString(ptr, ofs) (ofs==0 ? NULL : ((char *)(ptr) + (ptr)->(ofs)))

We can access string options as follows:

elog(NOTICE, "test_option_a is %s", GetOptionString(RdOpts, test_option_a));
elog(NOTICE, "test_option_b is %s", GetOptionString(RdOpts, test_option_b));

It enables to store multiple string options, and represent NULL string.
If possible, HANDLE_STRING_RELOPTION() should be able to manage the head of unused
surplus area and put the given val its offset automatically.

I think using a macro to access string option is more proper restriction than
mutually exclusive string options.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>