Re: [HACKERS] Compiler warnings with stringRelOpts (was WIP: Fast GiST index build)
Heikki Linnakangas writes: > On 09.08.2011 18:04, Alvaro Herrera wrote: >> I think I vaguely remember that the reason for doing it this way is that >> the copy into the relcache worked, i.e. if the originals went away, >> there was no dangling pointer. Did you test this? > These strings are never freed, so I don't think it's possible to end up > with a dangling pointer. Well, in that case the relevant question is whether we need to worry about memory leaks due to multiple copies. Personally I'm wondering why the function is strdup'ing the default value at all. In practice, wouldn't it practically always be a compile time constant string? Why create possible issues by designing the code for a different use-case? In particular, why not declare the default value as "const char *" and specify that it's caller's responsibility that it live forever if it's not just a constant string? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Compiler warnings with stringRelOpts (was WIP: Fast GiST index build)
On 09.08.2011 18:04, Alvaro Herrera wrote: Excerpts from Heikki Linnakangas's message of mar ago 09 08:32:43 -0400 2011: On 09.08.2011 13:25, Heikki Linnakangas wrote: On 08.08.2011 22:11, Alvaro Herrera wrote: Perhaps the easiest way to fix it is as you suggest, by declaring the struct to take a pointer rather than the value directly. Not sure how to make both cases work sanely; the add_string_reloption path will need updates. Agreed, I propose the attached patch to do that. Committed this. Thanks. I think I vaguely remember that the reason for doing it this way is that the copy into the relcache worked, i.e. if the originals went away, there was no dangling pointer. Did you test this? These strings are never freed, so I don't think it's possible to end up with a dangling pointer. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Compiler warnings with stringRelOpts (was WIP: Fast GiST index build)
Excerpts from Heikki Linnakangas's message of mar ago 09 08:32:43 -0400 2011: > On 09.08.2011 13:25, Heikki Linnakangas wrote: > > On 08.08.2011 22:11, Alvaro Herrera wrote: > >> Perhaps the easiest way to fix it is as you suggest, by declaring the > >> struct to take a pointer rather than the value directly. Not sure how > >> to make both cases work sanely; the add_string_reloption path will need > >> updates. > > > > Agreed, I propose the attached patch to do that. > > Committed this. Thanks. I think I vaguely remember that the reason for doing it this way is that the copy into the relcache worked, i.e. if the originals went away, there was no dangling pointer. Did you test this? -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Compiler warnings with stringRelOpts (was WIP: Fast GiST index build)
On 09.08.2011 13:25, Heikki Linnakangas wrote: On 08.08.2011 22:11, Alvaro Herrera wrote: Perhaps the easiest way to fix it is as you suggest, by declaring the struct to take a pointer rather than the value directly. Not sure how to make both cases work sanely; the add_string_reloption path will need updates. Agreed, I propose the attached patch to do that. Committed this. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Compiler warnings with stringRelOpts (was WIP: Fast GiST index build)
On 08.08.2011 22:11, Alvaro Herrera wrote: Perhaps the easiest way to fix it is as you suggest, by declaring the struct to take a pointer rather than the value directly. Not sure how to make both cases work sanely; the add_string_reloption path will need updates. Agreed, I propose the attached patch to do that. Are there any extensions out there that use add_string_relopt(), that I could use for testing? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c index 4657425..900b222 100644 --- a/src/backend/access/common/reloptions.c +++ b/src/backend/access/common/reloptions.c @@ -371,8 +371,6 @@ allocate_reloption(bits32 kinds, int type, char *name, char *desc) size_t size; relopt_gen *newoption; - Assert(type != RELOPT_TYPE_STRING); - oldcxt = MemoryContextSwitchTo(TopMemoryContext); switch (type) @@ -386,6 +384,9 @@ allocate_reloption(bits32 kinds, int type, char *name, char *desc) case RELOPT_TYPE_REAL: size = sizeof(relopt_real); break; + case RELOPT_TYPE_STRING: + size = sizeof(relopt_string); + break; default: elog(ERROR, "unsupported option type"); return NULL; /* keep compiler quiet */ @@ -474,45 +475,29 @@ void add_string_reloption(bits32 kinds, char *name, char *desc, char *default_val, validate_string_relopt validator) { - MemoryContext oldcxt; relopt_string *newoption; - int default_len = 0; - - oldcxt = MemoryContextSwitchTo(TopMemoryContext); - - if (default_val) - default_len = strlen(default_val); - newoption = palloc0(sizeof(relopt_string) + default_len); + /* make sure the validator/default combination is sane */ + if (validator) + (validator) (default_val); - newoption->gen.name = pstrdup(name); - if (desc) - newoption->gen.desc = pstrdup(desc); - else - newoption->gen.desc = NULL; - newoption->gen.kinds = kinds; - newoption->gen.namelen = strlen(name); - newoption->gen.type = RELOPT_TYPE_STRING; + newoption = (relopt_string *) allocate_reloption(kinds, RELOPT_TYPE_STRING, + name, desc); newoption->validate_cb = validator; if (default_val) { - strcpy(newoption->default_val, default_val); - newoption->default_len = default_len; + newoption->default_val = MemoryContextStrdup(TopMemoryContext, + default_val); + newoption->default_len = strlen(default_val); newoption->default_isnull = false; } else { - newoption->default_val[0] = '\0'; + newoption->default_val = ""; newoption->default_len = 0; newoption->default_isnull = true; } - /* make sure the validator/default combination is sane */ - if (newoption->validate_cb) - (newoption->validate_cb) (newoption->default_val); - - MemoryContextSwitchTo(oldcxt); - add_reloption((relopt_gen *) newoption); } diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h index c7709cc..14f5034 100644 --- a/src/include/access/reloptions.h +++ b/src/include/access/reloptions.h @@ -108,7 +108,7 @@ typedef struct relopt_string int default_len; bool default_isnull; validate_string_relopt validate_cb; - char default_val[1]; /* variable length, zero-terminated */ + char *default_val; } relopt_string; /* This is the table datatype for fillRelOptions */ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Compiler warnings with stringRelOpts (was WIP: Fast GiST index build)
Excerpts from Alexander Korotkov's message of lun ago 08 13:21:17 -0400 2011: > On Mon, Aug 8, 2011 at 8:27 PM, Alvaro Herrera > wrote: > > > An array of relopt_string? Isn't that a bit strange? If I recall > > correctly, the point of this was to be able to allocate the > > relopt_string struct and the char array itself as a single palloc unit, > > in a single call somewhere in the reloptions API (which was convoluted > > in some points precisely to let the string case work). I don't have the > > details of this fresh in my mind though. It certainly worked with more > > than one string option when I committed it, IIRC. > > > Yes, it seems strange. But it also seems that both were added by your commit > of table-based parser: > http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=ba748f7a11ef884277b61d1708a17a44acfd1736 Oh, you're adding options directly to stringRelOpts? Hmm, I can't remember whether I tried to do that; I have memory of testing string options via add_string_reloption ... and in reflection, it seems obvious that it would fail. Perhaps the easiest way to fix it is as you suggest, by declaring the struct to take a pointer rather than the value directly. Not sure how to make both cases work sanely; the add_string_reloption path will need updates. I don't have time to work on it right now though. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Compiler warnings with stringRelOpts (was WIP: Fast GiST index build)
On Mon, Aug 8, 2011 at 8:27 PM, Alvaro Herrera wrote: > An array of relopt_string? Isn't that a bit strange? If I recall > correctly, the point of this was to be able to allocate the > relopt_string struct and the char array itself as a single palloc unit, > in a single call somewhere in the reloptions API (which was convoluted > in some points precisely to let the string case work). I don't have the > details of this fresh in my mind though. It certainly worked with more > than one string option when I committed it, IIRC. > Yes, it seems strange. But it also seems that both were added by your commit of table-based parser: http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=ba748f7a11ef884277b61d1708a17a44acfd1736 -- With best regards, Alexander Korotkov.
Re: [HACKERS] Compiler warnings with stringRelOpts (was WIP: Fast GiST index build)
Excerpts from Alexander Korotkov's message of lun ago 08 11:50:53 -0400 2011: > On Mon, Aug 8, 2011 at 7:43 PM, Alvaro Herrera > wrote: > > > Maybe this needs to use the new FLEXIBLE_ARRAY_MEMBER stuff. Can you try > > that please? > > > typedef struct relopt_string > { > relopt_gen gen; > int default_len; > bool default_isnull; > validate_string_relopt validate_cb; > char default_val[1]; /* variable length, zero-terminated */ > } relopt_string; > > static relopt_string stringRelOpts[] = > ... > > I doubt variable-length data structure is possible in this case, because we > don't have array of pointers to relopt_string, but just array > of relopt_string. May be just > char *default_val; > is possible? An array of relopt_string? Isn't that a bit strange? If I recall correctly, the point of this was to be able to allocate the relopt_string struct and the char array itself as a single palloc unit, in a single call somewhere in the reloptions API (which was convoluted in some points precisely to let the string case work). I don't have the details of this fresh in my mind though. It certainly worked with more than one string option when I committed it, IIRC. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Compiler warnings with stringRelOpts (was WIP: Fast GiST index build)
On Mon, Aug 8, 2011 at 7:43 PM, Alvaro Herrera wrote: > Maybe this needs to use the new FLEXIBLE_ARRAY_MEMBER stuff. Can you try > that please? typedef struct relopt_string { relopt_gen gen; int default_len; bool default_isnull; validate_string_relopt validate_cb; char default_val[1]; /* variable length, zero-terminated */ } relopt_string; static relopt_string stringRelOpts[] = ... I doubt variable-length data structure is possible in this case, because we don't have array of pointers to relopt_string, but just array of relopt_string. May be just char *default_val; is possible? -- With best regards, Alexander Korotkov.
Re: [HACKERS] Compiler warnings with stringRelOpts (was WIP: Fast GiST index build)
Excerpts from Alexander Korotkov's message of lun ago 08 06:27:33 -0400 2011: > String-formatted relopts was never used before, but I've used it in > buffering GiST index build patch and encountered with following compiler > warnings: > > reloptions.c:259: warning: initializer-string for array of chars is too long > reloptions.c:259: warning: (near initialization for > ‘stringRelOpts[0].default_val’**) > > It is caused by definition of default field of relopt_string structure as > 1-length character array. This seems to be a design flaw in the reloptions.c > code. Any thoughts? Maybe this needs to use the new FLEXIBLE_ARRAY_MEMBER stuff. Can you try that please? -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers