Обсуждение: 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 <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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
> <alvherre@commandprompt.com>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 <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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
> <alvherre@commandprompt.com>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 <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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

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

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 <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> 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