Re: generic reloptions improvement - Mailing list pgsql-hackers

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
fromhead */   int         test_option_b;  /* of this structure.                            */   /* in actually surplus
areais 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@ak.jp.nec.com>