Even if we used JDBC, it would only be completely portable if none of the Oracle-specific features were used. But then we'd better go with just the AOLserver API.

On sql separation - I agree it does benefit the programmer to have the full freedom of sql in tcl pages (or whatever scripting language). But it would be nice (and maybe achievable) if this were restricted as much as possible to standard SQL. Instead of writing

db_multirow surveys survey_select {
              select survey_id, name
              from survsimp_surveys, acs_objects
              where object_id = survey_id
              and acs_permission.permission_p(object_id, $user_id,'survsimp_take_survey') = 't'
              and enabled_p = 't'
          }

db_permitted_multirow $user_id surveys survey_select {
             select survey_id, name
              from survsimp_surveys, acs_objects
              where object_id = survey_id
             and enabled_p = 't'
     }

Incidentally, please consider all these postings as an offer of resources to the OpenACS port :)

db_permitted_multirow doesn't currently exist, but I was suggesting it should. In general, Oracle specific sql should be hidden behind functions like these that take 'standard' SQL (in this case the select without the pl/sql call where clause). The aD versions of these functions still call the Oracle specific stuff - in this case they just tag the extra where clause on the end and send it off. But OpenACS can do its own thing. This is kind of like what you say about the utility of db_sysdate etc. - that sort of thing could be enforced across the whole codebase. calls to ns_ora and use of bind variables could be hidden behind this layer too.

For each function/procedure contained within a PL/SQL package we write pltcl or plpgsql function named as <package_name>__<function_name>. Sometimes function_name can be abbreviated when it's obvious from package_name, e.g. acs_object_type__create instead of acs_object_type__create_type.

However, we must take care of the following:

• not to pass NULL as one of the arguments, because function will simply return NULL
• whenever dynamic sql is required, we are forced to use pltcl (is this correct?)
• whenever we want to _return_ NULL, we must resort to plpgsql (correct?)
So, here are the simple ones:

create function acs_attribute__add_description(text, text, text, text) returns text as '
  declare
    v_object_type       alias for $1;
    v_attribute_name    alias for $2;
    v_description_key   alias for $3;
    v_description       alias for $4;
  begin
    insert into acs_attribute_descriptions
     (object_type, attribute_name, description_key, description)
    values
     (v_object_type, v_attribute_name,
      v_description_key, v_description);
  end;
  return v_description;
' language 'plpgsql';



create function acs_attribute__drop_description(text, text, text) returns text as '
  declare
    v_object_type       alias for $1;
    v_attribute_name    alias for $2;
    v_description_key   alias for $3;
  begin
    delete from acs_attribute_descriptions
      where object_type = v_object_type
      and attribute_name = v_attribute_name
      and description_key = v_description_key;
  end;
  return v_description_key;
' language 'plpgsql';

Now, here's the complicated one. Once again, I'll try to find a workaround to Oracle's default value handling. Needless to say, it's ugly, again. I've created a simple pltcl function:

create function arg_pack(text, text) returns text as '
  return "[list $1 $2] "
' language 'pltcl' ;

So that Oracle's PL/SQL is tranformed in following way:

  Oracle:

   attr_id := acs_attribute.create_attribute(
        object_type => 'acs_object',
     attribute_name => 'object_type',
           datatype => 'string',
        pretty_name => 'Object Type',
      pretty_plural => 'Object Types'
   );

  Postgres:

   select acs_attribute__create(
     arg_pack('object_type', 'acs_object') ||
     arg_pack('attribute_name', 'object_type') ||
     arg_pack('datatype', 'string') ||
     arg_pack('pretty_name', 'Object Type') ||
     arg_pack('pretty_plural', 'Object Types')
   );

arg_pack is used to construct key/value pairs into array-like lists which could be then parsed from within plpgsql function, using arg_unpack:

create function arg_unpack(text, text, text) returns text as '
  declare
    packed_args           alias for $1;
    argument_name         alias for $2;
    default_value         alias for $3;
  begin
    if ... NULL indicator is set ... then
      return NULL;
    else
      if ... the argument is supplied ...  then
        return argument_value;
      else
        return default_value;
      end if;
    end if;
  end;
' language 'plpgsql' ;

arg_unpack accepts 3 arguments: the first one is packed argument list ready to be fed into array set, the second is the argument name (key) and the third is default value which is returned if key does not appear in packed argument list. The function is written in plpgsql so we can return genuine NULL from it.

Yes, but how can we explicitly set one named argument to NULL? Since we cannot pass NULL value to arg_pack (because complete packed argument list would be NULL-ified), we denote NULL argument by setting its "NULL indicator", i.e. another argument named <argument_name>.null to arbitrary value:

   select acs_object_type__create(
     arg_pack('object_type', 'acs_object') ||
     arg_pack('supertype.null', 'yes') ||
     arg_pack('pretty_name' , 'Object') ||
     arg_pack('pretty_plural' , 'Objects') ||
     arg_pack('table_name' , 'acs_objects') ||
     arg_pack('id_column' , 'object_id') ||
     arg_pack('package_name' , 'acs_object') ||
     arg_pack('name_method' , 'acs_object__default_name')
   );

Let's see how acs_object_type__create handles this:

create function acs_object_type__create(text) returns text as '

declare
    packed_args         alias for $1;
    v_object_type       acs_object_types.object_type%TYPE;
    v_pretty_name       acs_object_types.pretty_name%TYPE;
    v_pretty_plural     acs_object_types.pretty_plural%TYPE;
    v_supertype         acs_object_types.supertype%TYPE;
    v_table_name        acs_object_types.table_name%TYPE;
    v_id_column         acs_object_types.id_column%TYPE;
    v_package_name      acs_object_types.package_name%TYPE;
    v_abstract_p        acs_object_types.abstract_p%TYPE;
    v_type_extension_table acs_object_types.type_extension_table%TYPE;
    v_name_method       acs_object_types.name_method%TYPE;
begin
  select
    arg_unpack(packed_args, ''object_type''),
    arg_unpack(packed_args, ''pretty_name''),
    arg_unpack(packed_args, ''pretty_plural''),
    arg_unpack(packed_args, ''supertype'', ''acs_object''),
    arg_unpack(packed_args, ''table_name''),
    arg_unpack(packed_args, ''id_column''),
    arg_unpack(packed_args, ''package_name''),
    arg_unpack(packed_args, ''abstract_p'', ''f''),
    arg_unpack(packed_args, ''type_extension_table''),
    arg_unpack(packed_args, ''name_method'')
  into
    v_object_type,
    v_pretty_name,
    v_pretty_plural,
    v_supertype,
    v_table_name,
    v_id_column,
    v_package_name,
    v_abstract_p,
    v_type_extension_table,
    v_name_method;

    if v_package_name is null then
      v_package_name := v_object_type;
    end if;

    insert into acs_object_types
      (object_type, pretty_name, pretty_plural, supertype, table_name,
       id_column, abstract_p, type_extension_table, package_name,
       name_method)
    values
      (v_object_type, v_pretty_name, v_pretty_plural, v_supertype, v_table_name,
       v_id_column, v_abstract_p, v_type_extension_table, v_package_name,
       v_name_method);

    return v_object_type;
end;
' language 'plpgsql';

With respect to default values: I think the excessive use of PL/Tcl could be a bad idea from a performance standpoint. I'm thinking, like Don (maybe I should just let him speak :), that we can deal without default values. I will look over the source code some more to double check this.

Most importantly, I think we should try a simple implementation first. If we find that this might be incomplete and a real pain in the butt, we should by all means go with something more complete However, my current understanding of the source code is that this won't be too bad.

Opinions? Dissents?