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

KaiGai Kohei wrote:
> Alvaro Herrera wrote:

>>> 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

That's not what I mean -- the problem is that some tables can have only
fillfactor allocated, and I didn't want to allocate the whole struct
just for fillfactor.  The technique I was using (which was to check the
length of the struct) is not going to work now with string reloptions
though, so it's not much of an issue.

> 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.

Agreed, it seems better.  The attached patch adds that, a macro you
originally requested and another one, and it also fixes an off-by-one
bug I discovered while testing all of this.  I also attach the testing
patch I play with to check that this all works nicely.

Oh, the patch also removes a bunch of "continue" statements that, as far
as I can tell, no longer work after the macros were wrapped in
do { ... } while (0) :-(  I don't see any nice way to put the facility
back.

Thanks for all the input.

--
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support