Issues for named/mixed function notation patch
I've now read most of this patch, and I think there are some things that
need rework, and perhaps debate about what we want.
1. As I already mentioned, I think the mixed-notation business is a bad
idea. It's unintuitive, it's not especially useful, and it substantially
increases our risk of being semantically incompatible with whatever the
SQL committee might someday do in this area. I think we should disallow
it until we see what they do. I gather that this might be an unpopular
position though.
2. It doesn't appear that any attention has been given to what happens
if CREATE OR REPLACE FUNCTION is used to change the parameter names of
an existing function. Since the post-analysis representation of parameter
lists is still entirely positional, the actual effect on a function call
in a stored view or rule is nil --- it will still call the same function
it did before, passing the parameters that were originally identified to
be passed. That might be considered good, but it's quite unlike what
will happen to function calls that are stored textually (eg, in plpgsql
functions). Is that what we want? Or should we consider that parameter
names are now part of the API of a function, and forbid CREATE OR REPLACE
FUNCTION from changing them? Or perhaps we should recheck parameter name
matching someplace after analysis, perhaps at default-expansion time?
3. In the same vein, CREATE FUNCTION doesn't disallow duplicate parameter
names, nor functions that have names for some but not all parameters.
The patch appears to cope with the latter case but not the former.
Should we disallow these things in CREATE FUNCTION, or make the patch
never match such functions, or what?
4. No attention has been given to making ruleutils.c list out named or
mixed function calls correctly.
5. I don't like anything about the "leaky list" representation of
analyzed function arguments. Having null subexpressions in unexpected
places isn't a good idea --- it's likely to cause crashes in code that
isn't being really cautious. Moreover, if we did ultimately support
mixed notation, there's no way to list it out correctly on the basis
of this representation, because you can't tell which arguments were
named and which weren't. I think it would be better to keep the
ArgExprs in the transformed parameter list and have the planner
remove them (and reorder the arguments if needed) when it does
default-argument expansion. Depending on what you think about point
#2, the transformed ArgExprs might need to carry the argument number
instead of the argument name, but in any case they'd just be there
for named arguments. This approach probably will also reduce the
amount of change in the parser, since you won't have to separate
the names from the argument list and pass those all over the place
separately.
Some minor style issues:
* ArgExpr is confusingly named and incorrectly documented, since it's
only used for named arguments. Perhaps NamedArgExpr would be better.
Also, it'd probably be a good idea to include a location field in it
(pointing at the parameter name not the argument expression).
* Most of the PG source code just writes "short" or "long",
not "short int". Actually I wonder whether "int2" wouldn't
be preferred anyway, since that's how the relevant pg_proc
columns are declared.
* The error messages aren't even approximately conformant to style guide.
* Please avoid useless whitespace changes, especially ones as
ill-considered as this:
***************
*** 10028,10034 ****
;
! name: ColId { $$ = $1; };
database_name:
ColId { $$ = $1; };
--- 10056,10062 ----
;! name: ColId { $$ = $1; };
database_name:
ColId { $$ = $1; };
I'm going to mark the patch Waiting on Author, since it's not close
to being committable until these issues are resolved.
regards, tom lane
Oh, another thing: the present restriction that all function parameters
after the first one with a default must also have defaults is based on
limitations of positional call notation. Does it make sense to relax
that restriction once we allow named call notation, and if so what
should we do exactly? (This could be addressed in a followup patch,
it doesn't necessarily have to be dealt with immediately.)
regards, tom lane
On Sun, Aug 9, 2009 at 7:30 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
1. As I already mentioned, I think the mixed-notation business is a bad
idea. It's unintuitive, it's not especially useful, and it substantially
increases our risk of being semantically incompatible with whatever the
SQL committee might someday do in this area. I think we should disallow
it until we see what they do. I gather that this might be an unpopular
position though.
It seems like we could safely allow the cases which are unambiguous.
Namely where the call has a sequence of unnamed parameters followed by
some named parameters all of which refer to parameters which come
later.
So foo(1,2,3 as x,4 as y) would be legal as long as x and y were not
one of the first three arguments.
That's a pretty common idiom when you have a function which does
something normal to the first few arguments and then has a bunch of
non-standard modes which can be optionally invoked. Take for example
the perl DBI's connect method which takes a data source, username,
authentication token, then a list of parameters which can be any of
dozens of parameters (actually it takes a fifth argument which is a
hashref -- but the point here is just that it's a common style, not
that we should be copying perl's solution).
The reason I'm saying this is safe is because there's just no other
possible interpretation. As long as it only covers the unambiguous
cases then there's no other meaning other implementations can define
which this would conflict with.
On Sun, Aug 9, 2009 at 2:30 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
I've now read most of this patch, and I think there are some things that
need rework, and perhaps debate about what we want.1. As I already mentioned, I think the mixed-notation business is a bad
idea. It's unintuitive, it's not especially useful, and it substantially
increases our risk of being semantically incompatible with whatever the
SQL committee might someday do in this area. I think we should disallow
it until we see what they do. I gather that this might be an unpopular
position though.
LOL. I already did my yelling and screaming on this point... though
it's all good-natured, in case that doesn't come through in the email.
2. It doesn't appear that any attention has been given to what happens
if CREATE OR REPLACE FUNCTION is used to change the parameter names of
an existing function. Since the post-analysis representation of parameter
lists is still entirely positional, the actual effect on a function call
in a stored view or rule is nil --- it will still call the same function
it did before, passing the parameters that were originally identified to
be passed. That might be considered good, but it's quite unlike what
will happen to function calls that are stored textually (eg, in plpgsql
functions). Is that what we want?
That sounds pretty dangerous. What if someone renames a parameter so
as to invert its sense, or something? (automatically_delete_all_files
becomes confirm_before_deleting, or somesuch)
Or should we consider that parameter
names are now part of the API of a function, and forbid CREATE OR REPLACE
FUNCTION from changing them? Or perhaps we should recheck parameter name
matching someplace after analysis, perhaps at default-expansion time?
I'm not sure what the right way to go with this is, but we have to
think about how it plays with function overloading - can I define two
identically-named functions with different sets of positional
parameters, and then resolve the function call based on which
parameters are specified?
3. In the same vein, CREATE FUNCTION doesn't disallow duplicate parameter
names, nor functions that have names for some but not all parameters.
The patch appears to cope with the latter case but not the former.
Should we disallow these things in CREATE FUNCTION, or make the patch
never match such functions, or what?
I think duplicate parameter names shouldn't be allowed.
4. No attention has been given to making ruleutils.c list out named or
mixed function calls correctly.5. I don't like anything about the "leaky list" representation of
analyzed function arguments. Having null subexpressions in unexpected
places isn't a good idea --- it's likely to cause crashes in code that
isn't being really cautious. Moreover, if we did ultimately support
mixed notation, there's no way to list it out correctly on the basis
of this representation, because you can't tell which arguments were
named and which weren't. I think it would be better to keep the
ArgExprs in the transformed parameter list and have the planner
remove them (and reorder the arguments if needed) when it does
default-argument expansion. Depending on what you think about point
#2, the transformed ArgExprs might need to carry the argument number
instead of the argument name, but in any case they'd just be there
for named arguments. This approach probably will also reduce the
amount of change in the parser, since you won't have to separate
the names from the argument list and pass those all over the place
separately.Some minor style issues:
* ArgExpr is confusingly named and incorrectly documented, since it's
only used for named arguments. Perhaps NamedArgExpr would be better.
Also, it'd probably be a good idea to include a location field in it
(pointing at the parameter name not the argument expression).* Most of the PG source code just writes "short" or "long",
not "short int". Actually I wonder whether "int2" wouldn't
be preferred anyway, since that's how the relevant pg_proc
columns are declared.* The error messages aren't even approximately conformant to style guide.
* Please avoid useless whitespace changes, especially ones as
ill-considered as this:***************
*** 10028,10034 ****
;! name: ColId { $$ = $1; };
database_name: ColId { $$ = $1; }; --- 10056,10062 ---- ;! name: ColId { $$ = $1; };
database_name:
ColId { $$ = $1; };I'm going to mark the patch Waiting on Author, since it's not close
to being committable until these issues are resolved.
Is it realistic to think that this will be finished and committed this week?
...Robert
Robert Haas <robertmhaas@gmail.com> writes:
On Sun, Aug 9, 2009 at 2:30 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
I'm going to mark the patch Waiting on Author, since it's not close
to being committable until these issues are resolved.
Is it realistic to think that this will be finished and committed this week?
I didn't want to prejudge that question. We still have the rest of the
week, and there's not that much else left to do, at least from my
standpoint (some of the other committers still have stuff on their
plates).
regards, tom lane
On Sun, Aug 9, 2009 at 9:36 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Sun, Aug 9, 2009 at 2:30 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
I'm going to mark the patch Waiting on Author, since it's not close
to being committable until these issues are resolved.Is it realistic to think that this will be finished and committed this week?
I didn't want to prejudge that question. We still have the rest of the
week, and there's not that much else left to do, at least from my
standpoint (some of the other committers still have stuff on their
plates).
Fair point, my impatience is showing. Sorry.
...Robert
On Mon, Aug 10, 2009 at 2:23 AM, Robert Haas<robertmhaas@gmail.com> wrote:
2. It doesn't appear that any attention has been given to what happens
if CREATE OR REPLACE FUNCTION is used to change the parameter names of
an existing function. Since the post-analysis representation of parameter
lists is still entirely positional, the actual effect on a function call
in a stored view or rule is nil --- it will still call the same function
it did before, passing the parameters that were originally identified to
be passed. That might be considered good, but it's quite unlike what
will happen to function calls that are stored textually (eg, in plpgsql
functions). Is that what we want?That sounds pretty dangerous. What if someone renames a parameter so
as to invert its sense, or something? (automatically_delete_all_files
becomes confirm_before_deleting, or somesuch)
There's also the existing users using positional notation to consider.
If all my callers are using positional notation then I might be kind
of annoyed if I can't fix the parameter names of my functions because
it would change the function signature. That would be a functionality
regression for me.
But on balance I don't see a better solution. If we allow people to
change the parameter names and they're used for named arguments then
it seems like the behaviour is not very clear and predictable no
matter what resolution we pick.
2009/8/9 Tom Lane <tgl@sss.pgh.pa.us>:
I've now read most of this patch, and I think there are some things that
need rework, and perhaps debate about what we want.1. As I already mentioned, I think the mixed-notation business is a bad
idea. It's unintuitive, it's not especially useful, and it substantially
increases our risk of being semantically incompatible with whatever the
SQL committee might someday do in this area. I think we should disallow
it until we see what they do. I gather that this might be an unpopular
position though.
I disagree. I thing so people expect mainly mixed notation.
2. It doesn't appear that any attention has been given to what happens
if CREATE OR REPLACE FUNCTION is used to change the parameter names of
an existing function. Since the post-analysis representation of parameter
lists is still entirely positional, the actual effect on a function call
in a stored view or rule is nil --- it will still call the same function
it did before, passing the parameters that were originally identified to
be passed. That might be considered good, but it's quite unlike what
will happen to function calls that are stored textually (eg, in plpgsql
functions). Is that what we want? Or should we consider that parameter
names are now part of the API of a function, and forbid CREATE OR REPLACE
FUNCTION from changing them? Or perhaps we should recheck parameter name
matching someplace after analysis, perhaps at default-expansion time?
I can't to imagine some recheck, so I prefer forbid CREATE OR REPLACE
FUNCTION for name change. We should to find some better solution
later. When we immutable names, then we have to have well RENAME
statement in plpgsql.
3. In the same vein, CREATE FUNCTION doesn't disallow duplicate parameter
names, nor functions that have names for some but not all parameters.
The patch appears to cope with the latter case but not the former.
Should we disallow these things in CREATE FUNCTION, or make the patch
never match such functions, or what?
I thing, so duplicate parameter names is clean bug - minimally for
language like plpgsql. I can to imagine some use case in C or plperlu,
but now we have variadic params or arrays, so duplicate names should
be deprecated.
4. No attention has been given to making ruleutils.c list out named or
mixed function calls correctly.
5. I don't like anything about the "leaky list" representation of
analyzed function arguments. Having null subexpressions in unexpected
places isn't a good idea --- it's likely to cause crashes in code that
isn't being really cautious. Moreover, if we did ultimately support
mixed notation, there's no way to list it out correctly on the basis
of this representation, because you can't tell which arguments were
named and which weren't. I think it would be better to keep the
ArgExprs in the transformed parameter list and have the planner
remove them (and reorder the arguments if needed) when it does
default-argument expansion. Depending on what you think about point
#2, the transformed ArgExprs might need to carry the argument number
instead of the argument name, but in any case they'd just be there
for named arguments. This approach probably will also reduce the
amount of change in the parser, since you won't have to separate
the names from the argument list and pass those all over the place
separately.
I have to look on this - I newer did some changes in planner, so I
know nothing about it now.
Some minor style issues:
* ArgExpr is confusingly named and incorrectly documented, since it's
only used for named arguments. Perhaps NamedArgExpr would be better.
Also, it'd probably be a good idea to include a location field in it
(pointing at the parameter name not the argument expression).
ook
* Most of the PG source code just writes "short" or "long",
not "short int". Actually I wonder whether "int2" wouldn't
be preferred anyway, since that's how the relevant pg_proc
columns are declared.
ok
* The error messages aren't even approximately conformant to style guide.
* Please avoid useless whitespace changes, especially ones as
ill-considered as this:***************
*** 10028,10034 ****
;! name: ColId { $$ = $1; };
database_name: ColId { $$ = $1; }; --- 10056,10062 ---- ;! name: ColId { $$ = $1; };
database_name:
ColId { $$ = $1; };
I am sorry, I'll be more careful
I'm going to mark the patch Waiting on Author, since it's not close
to being committable until these issues are resolved.
I spend week out of office, and actually I working on house, but I
hope so tomorrow will have time for fixing these issues.
regards, tom lane
thank you
Pavel Stehule
2009/8/9 Tom Lane <tgl@sss.pgh.pa.us>:
Oh, another thing: the present restriction that all function parameters
after the first one with a default must also have defaults is based on
limitations of positional call notation. Does it make sense to relax
that restriction once we allow named call notation, and if so what
should we do exactly? (This could be addressed in a followup patch,
it doesn't necessarily have to be dealt with immediately.)
Yes, this rule should be useless. But with the remove of this rule, we
have to modify algorithm for positional notation. It depends on this
rule.
regards
Pavel Stehule
Show quoted text
regards, tom lane
Hello,
I reworked patch to respect mentioned issues. - this patch still
implement mixed notation - I am thing so this notation is really
important. All others I respect. The behave is without change, fixed
some bugs, enhanced regress tests.
Sorry for delay.
Regards
Pavel Stehule
p.s. Bernard, please, can you look on this version?
2009/8/9 Tom Lane <tgl@sss.pgh.pa.us>:
Show quoted text
I've now read most of this patch, and I think there are some things that
need rework, and perhaps debate about what we want.1. As I already mentioned, I think the mixed-notation business is a bad
idea. It's unintuitive, it's not especially useful, and it substantially
increases our risk of being semantically incompatible with whatever the
SQL committee might someday do in this area. I think we should disallow
it until we see what they do. I gather that this might be an unpopular
position though.2. It doesn't appear that any attention has been given to what happens
if CREATE OR REPLACE FUNCTION is used to change the parameter names of
an existing function. Since the post-analysis representation of parameter
lists is still entirely positional, the actual effect on a function call
in a stored view or rule is nil --- it will still call the same function
it did before, passing the parameters that were originally identified to
be passed. That might be considered good, but it's quite unlike what
will happen to function calls that are stored textually (eg, in plpgsql
functions). Is that what we want? Or should we consider that parameter
names are now part of the API of a function, and forbid CREATE OR REPLACE
FUNCTION from changing them? Or perhaps we should recheck parameter name
matching someplace after analysis, perhaps at default-expansion time?3. In the same vein, CREATE FUNCTION doesn't disallow duplicate parameter
names, nor functions that have names for some but not all parameters.
The patch appears to cope with the latter case but not the former.
Should we disallow these things in CREATE FUNCTION, or make the patch
never match such functions, or what?4. No attention has been given to making ruleutils.c list out named or
mixed function calls correctly.5. I don't like anything about the "leaky list" representation of
analyzed function arguments. Having null subexpressions in unexpected
places isn't a good idea --- it's likely to cause crashes in code that
isn't being really cautious. Moreover, if we did ultimately support
mixed notation, there's no way to list it out correctly on the basis
of this representation, because you can't tell which arguments were
named and which weren't. I think it would be better to keep the
ArgExprs in the transformed parameter list and have the planner
remove them (and reorder the arguments if needed) when it does
default-argument expansion. Depending on what you think about point
#2, the transformed ArgExprs might need to carry the argument number
instead of the argument name, but in any case they'd just be there
for named arguments. This approach probably will also reduce the
amount of change in the parser, since you won't have to separate
the names from the argument list and pass those all over the place
separately.Some minor style issues:
* ArgExpr is confusingly named and incorrectly documented, since it's
only used for named arguments. Perhaps NamedArgExpr would be better.
Also, it'd probably be a good idea to include a location field in it
(pointing at the parameter name not the argument expression).* Most of the PG source code just writes "short" or "long",
not "short int". Actually I wonder whether "int2" wouldn't
be preferred anyway, since that's how the relevant pg_proc
columns are declared.* The error messages aren't even approximately conformant to style guide.
* Please avoid useless whitespace changes, especially ones as
ill-considered as this:***************
*** 10028,10034 ****
;! name: ColId { $$ = $1; };
database_name: ColId { $$ = $1; }; --- 10056,10062 ---- ;! name: ColId { $$ = $1; };
database_name:
ColId { $$ = $1; };I'm going to mark the patch Waiting on Author, since it's not close
to being committable until these issues are resolved.regards, tom lane
Attachments:
On Mon, Aug 24, 2009 at 3:19 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
I reworked patch to respect mentioned issues. - this patch still
implement mixed notation - I am thing so this notation is really
important. All others I respect. The behave is without change, fixed
some bugs, enhanced regress tests.
This does not compile.
...Robert
2009/9/14 Robert Haas <robertmhaas@gmail.com>:
On Mon, Aug 24, 2009 at 3:19 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
I reworked patch to respect mentioned issues. - this patch still
implement mixed notation - I am thing so this notation is really
important. All others I respect. The behave is without change, fixed
some bugs, enhanced regress tests.This does not compile.
I'll recheck it today
Pavel
Show quoted text
...Robert
Hello Robert,
2009/9/14 Robert Haas <robertmhaas@gmail.com>:
On Mon, Aug 24, 2009 at 3:19 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
I reworked patch to respect mentioned issues. - this patch still
implement mixed notation - I am thing so this notation is really
important. All others I respect. The behave is without change, fixed
some bugs, enhanced regress tests.This does not compile.
please, can you try this version? I hope so this in commitfest form
too. I didn't do any changes, but it can be broken. I compiled
attached patch today without problems. I have Federa 11. If you will
have a problems still, please, send me log.
Thank You
Pavel
Show quoted text
...Robert
Attachments:
On Mon, Sep 14, 2009 at 6:09 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
Hello Robert,
2009/9/14 Robert Haas <robertmhaas@gmail.com>:
On Mon, Aug 24, 2009 at 3:19 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
I reworked patch to respect mentioned issues. - this patch still
implement mixed notation - I am thing so this notation is really
important. All others I respect. The behave is without change, fixed
some bugs, enhanced regress tests.This does not compile.
please, can you try this version? I hope so this in commitfest form
too. I didn't do any changes, but it can be broken. I compiled
attached patch today without problems. I have Federa 11. If you will
have a problems still, please, send me log.
Same problem. Build log attached.
...Robert
Attachments:
Same problem. Build log attached.
...Robert
My renonc, please, try new patch. I forgot mark regproc.c file.
regards
Pavel Stehule
Attachments:
On Tue, Sep 15, 2009 at 4:51 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
Same problem. Build log attached.
...Robert
My renonc, please, try new patch. I forgot mark regproc.c file.
regards
Pavel Stehule
This one compiles for me.
...Robert
On Tue, 2009-09-15 at 10:51 +0200, Pavel Stehule wrote:
My renonc, please, try new patch. I forgot mark regproc.c file.
I think the documentation around calling functions is disorganized:
Variadic functions, functions with defaults, SRFs, out parameters, and
polymorphism are all explained in 34.4, which is about SQL functions
specifically.
Overloading is in chapter 34 also, but not specifically in the SQL
function section like the rest.
Function calls themselves are only given 5 lines of explanation in
4.2.6, with no mention of things like the VARIADIC keyword.
These complaints aren't about the patch, but we might want to consider
some reorganization of those sections (probably a separate doc patch).
The interaction with variadic functions appears to be misdocumented.
From the code and tests, the VARIADIC keyword appears to be optional
when using named notation, but required when using positional notation.
But the documentation says:
"However, a named variadic argument can only be called the way shown in
the example above. The VARIADIC keyword must not be specified and a
variadic notation of all arguments is not supported. To use variadic
argument lists you must use positional notation instead."
What is the intended behavior? I think we should always require VARIADIC
to be specified regardless of using named notation.
I'm still reviewing the code.
Regards,
Jeff Davis
"However, a named variadic argument can only be called the way shown in
the example above. The VARIADIC keyword must not be specified and a
variadic notation of all arguments is not supported. To use variadic
argument lists you must use positional notation instead."What is the intended behavior? I think we should always require VARIADIC
to be specified regardless of using named notation.
maybe we could to support variadic named parameters in future - then
using VARIADIC keyword should be necessary - like
foo(10 AS p1, 20 AS p1, 30 AS p3) is equalent of
foo(VARIADIC ARRAY[10,20] AS p1, 30 AS p3)
if we plan this feature, the VARIADIC keyword have to be mandatory.
Regards
Pavel Stehule
Show quoted text
I'm still reviewing the code.
Regards,
Jeff Davis
On Sun, Sep 27, 2009 at 12:37 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
"However, a named variadic argument can only be called the way shown in
the example above. The VARIADIC keyword must not be specified and a
variadic notation of all arguments is not supported. To use variadic
argument lists you must use positional notation instead."What is the intended behavior? I think we should always require VARIADIC
to be specified regardless of using named notation.maybe we could to support variadic named parameters in future - then
using VARIADIC keyword should be necessary - likefoo(10 AS p1, 20 AS p1, 30 AS p3) is equalent of
foo(VARIADIC ARRAY[10,20] AS p1, 30 AS p3)
Pavel,
This doesn't make sense to me, FWIW. I don't think we should allow
parameters to be specified more than once. It's hard for me to
imagine how that could be useful.
I'm still reviewing the code.
Jeff,
When will you be able to post this review?
Thanks,
...Robert
2009/9/27 Robert Haas <robertmhaas@gmail.com>:
On Sun, Sep 27, 2009 at 12:37 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
"However, a named variadic argument can only be called the way shown in
the example above. The VARIADIC keyword must not be specified and a
variadic notation of all arguments is not supported. To use variadic
argument lists you must use positional notation instead."What is the intended behavior? I think we should always require VARIADIC
to be specified regardless of using named notation.maybe we could to support variadic named parameters in future - then
using VARIADIC keyword should be necessary - likefoo(10 AS p1, 20 AS p1, 30 AS p3) is equalent of
foo(VARIADIC ARRAY[10,20] AS p1, 30 AS p3)Pavel,
This doesn't make sense to me, FWIW. I don't think we should allow
parameters to be specified more than once. It's hard for me to
imagine how that could be useful.
ook I thing, so this should be little bit unclean too. I though why we
need VARIADIC keyword mandatory for named notation. When we could
specify only unique names, then we could use only one "packed"
variadic parameter - and then VARIADIC keyword isn't necessary.
Is this idea correct? I thing, so there are not problem ensure an
using VARIADIC keyword in this context - but, personally I don't feel,
so there it have to be. But I don't thing, so this is important
(minimally for me) - I'll accept others opinions.
Regards
Pavel
Show quoted text
I'm still reviewing the code.
Jeff,
When will you be able to post this review?
Thanks,
...Robert
