Skip site navigation (1) Skip section navigation (2)

Re: generic reloptions improvement

From: KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
To: Alvaro Herrera <alvherre(at)commandprompt(dot)com>
Cc: KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Peter Eisentraut <peter_e(at)gmx(dot)net>, pgsql-hackers(at)postgresql(dot)org, ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>
Subject: Re: generic reloptions improvement
Date: 2009-01-05 00:44:49
Message-ID: 49615801.60201@ak.jp.nec.com (view raw or flat)
Thread:
Lists: 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;
        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>

In response to

Responses

pgsql-hackers by date

Next:From: Hitoshi HaradaDate: 2009-01-05 00:52:49
Subject: Re: WIP patch for basic window frame support
Previous:From: Tom LaneDate: 2009-01-05 00:34:53
Subject: Re: WIP patch for basic window frame support

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group