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

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > 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.
>
> Hmm ... I guess you could make the wrapping be "if (...) { ... } else {}"
> instead of do/while, but I'm pretty dubious of having a continue in the
> macros anyway --- that's an even stronger assumption about the context
> the macro is being used in than the original gripe.
>
> What you seem to be supposing is that the only possible use pattern
> for these macros is a for-loop containing nothing but calls to one
> or another of the macros.

You're right. I initially wrote these macros to reduce the amount of
code in heap_reloptions, but apparently went too far with what to
include in them. Perhaps it's better to define them this way:

! #define HANDLE_INT_RELOPTION(optname, var, option, wasset) \
! do { \
! if (HAVE_RELOPTION(optname, option)) \
! { \
! if (option.isset) \
! var = option.values.int_val; \
! else \
! var = ((relopt_int *) option.gen)->default_val; \
! (wasset) != NULL ? *(wasset) = option.isset : (dummyret)NULL; \
! } \
} while (0)
--- 116,148 ----
* 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 { \
! if (option.isset) \
! var = option.values.int_val; \
! else \
! var = ((relopt_int *) option.gen)->default_val; \
! (wasset) != NULL ? *(wasset) = option.isset : (dummyret)NULL; \
} while (0)

i.e. leave out the HAVE_RELOPTION() test; this allows callers to insert
the "continue" bit should they so desire. This makes the routines more
verbose, but not overly so, and seems more flexible. With these
definitions, default_reloptions looks like this (of course, the
"continue" makes no sense in this case, but it would if there were more
options):

/*
* Option parser for anything that uses StdRdOptions (i.e. fillfactor only)
*/
bytea *
default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
{
relopt_value *options;
StdRdOptions *rdopts;
int numoptions;
int len;
int i;

options = parseRelOptions(reloptions, validate, kind, &numoptions);

/* if none set, we're done */
if (numoptions == 0)
return NULL;

len = sizeof(StdRdOptions);
rdopts = palloc0(len);

for (i = 0; i < numoptions; i++)
{
if (HAVE_RELOPTION("fillfactor", options[i]))
{
HANDLE_INT_RELOPTION("fillfactor", rdopts->fillfactor, options[i],
(char *) NULL);
continue;
}
}

pfree(options);
SET_VARSIZE(rdopts, len);

return (bytea *) rdopts;
}

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