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>
