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
*** src/backend/access/common/reloptions.c 6 Jan 2009 14:47:37 -0000 1.16
--- src/backend/access/common/reloptions.c 7 Jan 2009 13:35:42 -0000
***************
*** 336,344 ****
/*
* add_string_reloption
* Add a new string reloption
*/
void
! add_string_reloption(int kind, char *name, char *desc, char *default_val)
{
MemoryContext oldcxt;
relopt_string *newoption;
--- 336,350 ----
/*
* add_string_reloption
* Add a new string reloption
+ *
+ * "validator" is an optional function pointer that can be used to test the
+ * validity of the values. It must elog(ERROR) when the argument string is
+ * not acceptable for the variable. Note that the default value must pass
+ * the validation.
*/
void
! add_string_reloption(int kind, char *name, char *desc, char *default_val,
! validate_string_relopt validator)
{
MemoryContext oldcxt;
relopt_string *newoption;
***************
*** 359,364 ****
--- 365,371 ----
newoption->gen.kind = kind;
newoption->gen.namelen = strlen(name);
newoption->gen.type = RELOPT_TYPE_STRING;
+ newoption->validate_cb = validator;
if (default_val)
{
strcpy(newoption->default_val, default_val);
***************
*** 372,377 ****
--- 379,388 ----
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);
***************
*** 729,738 ****
}
break;
case RELOPT_TYPE_STRING:
! option->values.string_val = value;
! nofree = true;
! parsed = true;
! /* no validation possible */
break;
default:
elog(ERROR, "unsupported reloption type %d", option->gen->type);
--- 740,754 ----
}
break;
case RELOPT_TYPE_STRING:
! {
! relopt_string *optstring = (relopt_string *) option->gen;
!
! option->values.string_val = value;
! nofree = true;
! if (optstring->validate_cb)
! (optstring->validate_cb) (value);
! parsed = true;
! }
break;
default:
elog(ERROR, "unsupported reloption type %d", option->gen->type);
*** src/include/access/reloptions.h 6 Jan 2009 14:47:37 -0000 1.8
--- src/include/access/reloptions.h 7 Jan 2009 13:31:25 -0000
***************
*** 90,100 ****
--- 90,103 ----
double max;
} relopt_real;
+ typedef void (*validate_string_relopt) (char *);
+
typedef struct relopt_string
{
relopt_gen gen;
int default_len;
bool default_isnull;
+ validate_string_relopt validate_cb;
char default_val[1]; /* variable length */
} relopt_string;
***************
*** 113,119 ****
* need this information.
*/
#define HAVE_RELOPTION(optname, option) \
! (pg_strncasecmp(option.gen->name, optname, option.gen->namelen) == 0)
#define HANDLE_INT_RELOPTION(optname, var, option, wasset) \
do { \
--- 116,122 ----
* need this information.
*/
#define HAVE_RELOPTION(optname, option) \
! (pg_strncasecmp(option.gen->name, optname, option.gen->namelen + 1) == 0)
#define HANDLE_INT_RELOPTION(optname, var, option, wasset) \
do { \
***************
*** 124,130 ****
else \
var = ((relopt_int *) option.gen)->default_val; \
(wasset) != NULL ? *(wasset) = option.isset : (dummyret)NULL; \
- continue; \
} \
} while (0)
--- 127,132 ----
***************
*** 137,143 ****
else \
var = ((relopt_bool *) option.gen)->default_val; \
(wasset) != NULL ? *(wasset) = option.isset : (dummyret) NULL; \
- continue; \
} \
} while (0)
--- 139,144 ----
***************
*** 150,156 ****
else \
var = ((relopt_real *) option.gen)->default_val; \
(wasset) != NULL ? *(wasset) = option.isset : (dummyret) NULL; \
- continue; \
} \
} while (0)
--- 151,156 ----
***************
*** 170,195 ****
if (HAVE_RELOPTION(optname, option)) \
{ \
relopt_string *optstring = (relopt_string *) option.gen;\
! char *string_val = NULL; \
\
if (option.isset) \
string_val = option.values.string_val; \
else if (!optstring->default_isnull) \
string_val = optstring->default_val; \
(wasset) != NULL ? *(wasset) = option.isset : (dummyret) NULL; \
\
! if (!string_val) \
var = 0; \
else \
{ \
! strcpy((char *)(base) + (offset), string_val); \
var = (offset); \
(offset) += strlen(string_val) + 1; \
} \
- continue; \
} \
} while (0)
extern int add_reloption_kind(void);
extern void add_bool_reloption(int kind, char *name, char *desc,
bool default_val);
--- 170,214 ----
if (HAVE_RELOPTION(optname, option)) \
{ \
relopt_string *optstring = (relopt_string *) option.gen;\
! char *string_val; \
\
if (option.isset) \
string_val = option.values.string_val; \
else if (!optstring->default_isnull) \
string_val = optstring->default_val; \
+ else \
+ string_val = NULL; \
(wasset) != NULL ? *(wasset) = option.isset : (dummyret) NULL; \
\
! if (string_val == NULL) \
var = 0; \
else \
{ \
! strcpy(((char *)(base)) + (offset), string_val); \
var = (offset); \
(offset) += strlen(string_val) + 1; \
} \
} \
} while (0)
+ /*
+ * For use during amoptions: get the strlen of a string option
+ * (either default or the user defined value)
+ */
+ #define GET_STRING_RELOPTION_LEN(option) \
+ ((option).isset ? strlen((option).values.string_val) : \
+ ((relopt_string *) (option).gen)->default_len)
+
+ /*
+ * For use by code reading options already parsed: get a pointer to the string
+ * value itself. "optstruct" is the StdRdOption struct or equivalent, "member"
+ * is the struct member corresponding to the string option
+ */
+ #define GET_STRING_RELOPTION(optstruct, member) \
+ ((optstruct)->member == 0 ? NULL : \
+ (char *)(optstruct) + (optstruct)->member)
+
+
extern int add_reloption_kind(void);
extern void add_bool_reloption(int kind, char *name, char *desc,
bool default_val);
***************
*** 198,204 ****
extern void add_real_reloption(int kind, char *name, char *desc,
double default_val, double min_val, double max_val);
extern void add_string_reloption(int kind, char *name, char *desc,
! char *default_val);
extern Datum transformRelOptions(Datum oldOptions, List *defList,
bool ignoreOids, bool isReset);
--- 217,223 ----
extern void add_real_reloption(int kind, char *name, char *desc,
double default_val, double min_val, double max_val);
extern void add_string_reloption(int kind, char *name, char *desc,
! char *default_val, validate_string_relopt validator);
extern Datum transformRelOptions(Datum oldOptions, List *defList,
bool ignoreOids, bool isReset);
Index: src/backend/access/common/reloptions.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/common/reloptions.c,v
retrieving revision 1.16
diff -c -p -r1.16 reloptions.c
Index: src/backend/access/nbtree/nbtinsert.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/nbtree/nbtinsert.c,v
retrieving revision 1.169
diff -c -p -r1.169 nbtinsert.c
*** src/backend/access/nbtree/nbtinsert.c 1 Jan 2009 17:23:35 -0000 1.169
--- src/backend/access/nbtree/nbtinsert.c 7 Jan 2009 03:21:31 -0000
***************
*** 17,22 ****
--- 17,23 ----
#include "access/heapam.h"
#include "access/nbtree.h"
+ #include "access/reloptions.h"
#include "access/transam.h"
#include "miscadmin.h"
#include "storage/bufmgr.h"
***************
*** 565,570 ****
--- 566,590 ----
OffsetNumber firstright = InvalidOffsetNumber;
Size itemsz;
+ {
+ BtOptions *options = (BtOptions *) rel->rd_options;
+
+ if (options)
+ {
+ char *str;
+
+ str = GET_STRING_RELOPTION(options, teststring);
+ elog(LOG, "the teststring option is %s",
+ str ? str : "empty");
+ str = GET_STRING_RELOPTION(options, teststring2);
+ elog(LOG, "the teststring2 option is %s",
+ str ? str : "empty");
+ str = GET_STRING_RELOPTION(options, teststring3);
+ elog(LOG, "the teststring3 option is %s",
+ str ? str : "empty");
+ }
+ }
+
page = BufferGetPage(buf);
lpageop = (BTPageOpaque) PageGetSpecialPointer(page);
Index: src/backend/access/nbtree/nbtutils.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/nbtree/nbtutils.c,v
retrieving revision 1.93
diff -c -p -r1.93 nbtutils.c
*** src/backend/access/nbtree/nbtutils.c 5 Jan 2009 17:14:28 -0000 1.93
--- src/backend/access/nbtree/nbtutils.c 7 Jan 2009 13:31:23 -0000
***************
*** 1395,1408 ****
Assert(found);
}
Datum
btoptions(PG_FUNCTION_ARGS)
{
Datum reloptions = PG_GETARG_DATUM(0);
bool validate = PG_GETARG_BOOL(1);
bytea *result;
- result = default_reloptions(reloptions, validate, RELOPT_KIND_BTREE);
if (result)
PG_RETURN_BYTEA_P(result);
PG_RETURN_NULL();
--- 1395,1490 ----
Assert(found);
}
+ void
+ validate_teststring(char *string);
+
+ void
+ validate_teststring(char *string)
+ {
+ if (string == NULL)
+ elog(ERROR, "teststring must not be null!");
+ if (strlen(string) != 3)
+ elog(ERROR, "teststring must be of length 3, but it's \"%s\"", string);
+ }
+
Datum
btoptions(PG_FUNCTION_ARGS)
{
Datum reloptions = PG_GETARG_DATUM(0);
bool validate = PG_GETARG_BOOL(1);
bytea *result;
+ relopt_value *options;
+ int numoptions;
+ int i;
+ static bool initialized = false;
+
+ if (!initialized)
+ {
+ add_string_reloption(RELOPT_KIND_BTREE, "teststring", NULL,
+ "foo", validate_teststring);
+ add_string_reloption(RELOPT_KIND_BTREE, "teststring2", NULL,
+ "", NULL);
+ add_string_reloption(RELOPT_KIND_BTREE, "teststring3", NULL,
+ NULL, NULL);
+ initialized = true;
+ }
+
+ options = parseRelOptions(reloptions, validate, RELOPT_KIND_BTREE,
+ &numoptions);
+
+ /* if none set, we're done */
+ if (numoptions == 0)
+ result = NULL;
+ else
+ {
+ BtOptions *rdopts;
+ int tstrlen;
+ int t2strlen;
+ int t3strlen;
+ int offset;
+
+ for (i = 0; i < numoptions; i++)
+ {
+ if (HAVE_RELOPTION("teststring", options[i]))
+ {
+ tstrlen = GET_STRING_RELOPTION_LEN(options[i]);
+ continue;
+ }
+ if (HAVE_RELOPTION("teststring2", options[i]))
+ {
+ t2strlen = GET_STRING_RELOPTION_LEN(options[i]);
+ continue;
+ }
+ if (HAVE_RELOPTION("teststring3", options[i]))
+ {
+ t3strlen = GET_STRING_RELOPTION_LEN(options[i]);
+ continue;
+ }
+ }
+
+ rdopts = palloc0(sizeof(BtOptions) + tstrlen + t2strlen + t3strlen + 4);
+ offset = sizeof(BtOptions);
+
+ for (i = 0; i < numoptions; i++)
+ {
+ HANDLE_INT_RELOPTION("fillfactor", rdopts->fillfactor, options[i],
+ (char *) NULL);
+ HANDLE_STRING_RELOPTION("teststring", rdopts->teststring,
+ options[i], rdopts, offset,
+ (char *) NULL);
+ HANDLE_STRING_RELOPTION("teststring2", rdopts->teststring2,
+ options[i], rdopts, offset,
+ (char *) NULL);
+ HANDLE_STRING_RELOPTION("teststring3", rdopts->teststring3,
+ options[i], rdopts, offset,
+ (char *) NULL);
+ }
+
+ pfree(options);
+ SET_VARSIZE(rdopts, offset);
+ result = (bytea *) rdopts;
+ }
if (result)
PG_RETURN_BYTEA_P(result);
PG_RETURN_NULL();
Index: src/include/access/nbtree.h
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/include/access/nbtree.h,v
retrieving revision 1.123
diff -c -p -r1.123 nbtree.h
*** src/include/access/nbtree.h 1 Jan 2009 17:23:56 -0000 1.123
--- src/include/access/nbtree.h 6 Jan 2009 20:35:32 -0000
***************
*** 595,598 ****
--- 595,609 ----
extern void btree_xlog_cleanup(void);
extern bool btree_safe_restartpoint(void);
+ /* must follow StdRdOptions conventions */
+ typedef struct BtOptions
+ {
+ int32 vl_len_;
+ int fillfactor;
+ int teststring;
+ int teststring2;
+ int teststring3;
+ /* variable length struct -- string is allocated here */
+ } BtOptions;
+
#endif /* NBTREE_H */
Index: src/include/access/reloptions.h
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/include/access/reloptions.h,v
retrieving revision 1.8
diff -c -p -r1.8 reloptions.h
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
