Postgres: Reloptions string -> enum

[nataraj]

While rewriting reloption.c code, I came to an idea, that all string reloptions that are used in the code are actually enum reloptions: just a fixed list of string constants, nothing more.


One string option is gist buffering, with the following validate function:

gistValidateBufferingOption(char *value)
{
     if (value == NULL ||
         (strcmp(value, "on") != 0 &&
          strcmp(value, "off") != 0 &&
          strcmp(value, "auto") != 0))
     {
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
                  errmsg("invalid value for \"buffering\" option"),
               errdetail("Valid values are \"on\", \"off\", and \"auto\".")));
     }
}

Another one is check_option for views. That also a list of two options

void
validateWithCheckOption(char *value)
{
     if (value == NULL ||
         (pg_strcasecmp(value, "local") != 0 &&
          pg_strcasecmp(value, "cascaded") != 0))
     {
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
                  errmsg("invalid value for \"check_option\" option"),
                  errdetail("Valid values are \"local\" and \"cascaded\".")));
     }
}

So the first idea, that came to me was to add a enum type, and switch the code to use it instead of string.

The second idea was, to get rid of string type at all. Because when parsing of reloptions is done, all the data is stored as a bytea binary chunk, that is copied from one memory context to another. So if there is no string data there, you can just work with it as with C structure. But if you have a string data, you should somehow put it into that data chunk, because otherwise it would not be copied from one memory context to another. This all require hacks that are not beautiful at all. And as I think should be avoided if possible. For example, this function takes a base size of reloption structure and adds some more space for each string option. I do not like this code:

void *
allocateReloptStruct(Size base, relopt_value *options, int numoptions)
{
	Size		size = base;
	int			i;

	for (i = 0; i < numoptions; i++)
		if (options[i].gen->type == RELOPT_TYPE_STRING)
			size += GET_STRING_RELOPTION_LEN(options[i]) + 1;

	return palloc0(size);
}

So I would ask the community to consider removing string option type, if nobody really using it right now. 'cause it is kind of evil for me.


PS. If you would like to comment this post, please login from any social network that is possible to login here, or at least write your name, so I would be able to answer you...

Comments

[david fetter (from livejournal.com)]

I think that the best idea would be to make this identical proposal on -hackers along with a proof of concept patch. You are guaranteed to get some kind of attention on it, especially if you put it in the next commitfest. Any performance or other testing that you do would also be very welcome.

[(Anonymous)]

Would You thin about this hack?

void
validateWithCheckOption(char *value)
{
if (value == NULL || (value[0] != 'c' && value[0] != 'l' ||) ||
(pg_strcasecmp(value, "cascaded") != 0 &&
pg_strcasecmp(value, "local") != 0))
{
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid value for \"check_option\" option"),
errdetail("Valid values are \"local\" and \"cascaded\".")));
}
}

[shaplov.livejournal.com]

I will surely offer this to the -hackers a little bit later, with the whole reloption patch I am working at now.

I do not think any performance test is really needed. Because most of the reloptions staff is done while opening tables and then it is cached using relcache. And all of this is nothing compared to disk I/O. So I think there is no need for performance test here.

[shaplov.livejournal.com]

I do not like hacks.

I would prefer to write good framework for easily creating enum reloptions.