Re: generic reloptions improvement

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;
case RELKIND_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(at)ak(dot)jp(dot)nec(dot)com>