Re: Updates of SE-PostgreSQL 8.4devel patches (r1389)

Alvaro Herrera wrote:
> KaiGai Kohei wrote:
> 
>> Alvaro, could you check the patched code on reloptions.h, reloptions.c
>> and rel.h? It is a working example of string reloptions, and I could
>> found a few strange codes.
> 
> I'm intending to revisit the string code ... I was thinking yesterday
> night that I shouldn't have committed it at all, and left it for a
> subsequent patch that I had more chance to test properly :-(
> 
>> 1. HANDLE_STRING_RELOPTION() always put an empty string when
>>    optstring->default_isnull is true, even if user gives a
>>    valid string reloption.
> 
> This is a plain bug, sorry.
> 
>> 2. HANDLE_STRING_RELOPTION() cannot handle an offset style.
>>    The patched one enables to put reloption string on the
>>    tail of StdRdOptions structure, and adjust offset value.
> 
> I'll look at it, thanks.
> 
>> 3. Why the "StdRdOptions lopts;" is necessary?
> 
> It is like this because the autovacuum patch adds a few more options and
> I want to have the chance to not allocate the part belonging to
> autovacuum when none of the options are present.

We can return NULL immediately without any allocation, when numoptions=0.
Does it give us any pains?
http://code.google.com/p/sepgsql/source/browse/trunk/sepgsql/src/backend/access/common/reloptions.c#765

>> And, I have a request.
>> 4. Is it possible to support a call-back to validate a given
>>    string reloption? I want to check whether the given default
>>    Row-level ACLs has a valid format, or not.
> 
> Hmm, why a callback and not just call the validation function in
> heap_reloptions?

I thought you intend to apply validation checks in parse_one_reloption()
invoked from parseRelOptions(), but now we have no checks in string
reloptions.
In my personal preference, it is more simple design parse_one_reloption()
invoke a function pointer for validation checks.

Please decide a guideline to be followed when we add a new string reloption.
If it requires to invoke the function from heap_reloptions(), I'll follow it.

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>