Re: generic reloptions improvement - Mailing list pgsql-hackers

Alvaro Herrera wrote:
> Alvaro Herrera wrote:
> 
>> Okay, it was basically fine except for the attached minor correction.
>> Warning: I intend to commit this patch fairly soon!
> 
> This is the patch in its final form.  I have included a few macros to
> simplify the writing of amoptions routines.

Thanks for your efforts!

However, I could find a few issues about string reloptions.

(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;                                                 \
}
 

(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?

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

It invokes heap_reloptions() under CurrentMemoryContext, and its result
is copied in CacheMemoryContext later, if the result is not NULL.
But it makes a matter when StdRdOptions contains a pointer.
If a string allocated under CurrentMemoryContext, target of the copied
pointer is not valid on the next query execution, even if StdRdOptions
is valid because it is copied to another memoery context.

I think RelationParseRelOptions() should be patched as follows:
   /* Parse into appropriate format; don't error out here */
+   oldctx = MemoryContextSwitchTo(CacheMemoryContext);   switch (relation->rd_rel->relkind)   {       case
RELKIND_RELATION:      case RELKIND_TOASTVALUE:       case RELKIND_UNCATALOGED:           options =
heap_reloptions(relation->rd_rel->relkind,datum,                                     false);           break;
caseRELKIND_INDEX:           options = index_reloptions(relation->rd_am->amoptions, datum,
       false);           break;       default:           Assert(false);      /* can't get here */           options =
NULL;    /* keep compiler quiet */           break;   }
 
+   MemoryContextSwitchTo(oldctx);

-   /* Copy parsed data into CacheMemoryContext */
-   if (options)
-   {
-       relation->rd_options = MemoryContextAlloc(CacheMemoryContext,
-                                                 VARSIZE(options));
-       memcpy(relation->rd_options, options, VARSIZE(options));
-   }

Thanks,
-- 
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>