Re: Updates of SE-PostgreSQL 8.4devel patches (r1389) - Mailing list pgsql-hackers

Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.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