FWD » Core Development » Database

Could the same prefix CC_PREFIX = "__" be used for any database supporting computed columns? As far as I understood it is not H2 specific but P2J feature allowing distinct computed columns from usual ones especially in indexes. Is it correct?

Your understanding is correct (assuming that prefix generates a legal name in SQL Server).

I have checked in SQL Server 2005 (I think the same for SQL Server 2008 and 2012): the following column names are correct:
"__", "___", "__a", "__aa"

I have noticed in import_util.rules that PostgreSQL connection string is configured only for default port (5432). Is it OK for PostgreSQL?
I think port is required for MSSQL server url because several sql server instances could be started on different ports.

One more question concerning mapping varchar to TEXT in PostgreSQL.

Vadim Nebogatov wrote:

I have noticed in import_util.rules that PostgreSQL connection string is configured only for default port (5432). Is it OK for PostgreSQL?
I think port is required for MSSQL server url because several sql server instances could be started on different ports.

I have found answer in P2J Conversion Handbook - port should be added to url for PostgreSQL if non-default port is used.

Vadim Nebogatov wrote:

One more question concerning mapping varchar to TEXT in PostgreSQL.

Progress character fields are limited internally to a combined maximum value per table, but each field is not limited explicitly like a varchar(n) declaration in SQL DDL. We want to use an unbounded data type, if possible. Otherwise, the type declarations for each field become problematic, because we don't know what the actual limits are. The text type was chosen for PostgreSQL because it is unbounded, and there is evidently no performance penalty for using it.

XXXXP2JDialect implementation (will be extended)

1. extractColumnName(String expr) , needsComputedColumns() and isCaseInsensitiveColumn(String name)

Implementation depends if database has
1) computed columns
2) expressions could be used in database indexes
3) quoted columns

MS SQL Server 2005 uses all these options.

2. getLowDate()/getHighDate()

lowDate/highDate depends on database and even on database version. For example,

MS SQL Server 2005 (http://msdn.microsoft.com/en-US/library/ms187819%28v=sql.90%29.aspx):

datetime - January 1, 1753, through December 31, 9999

MS SQL Server 2008 and 2012 (http://msdn.microsoft.com/en-us/library/bb630289%28v=sql.100%29.aspx):

datetime - January 1, 1753, through December 31, 9999supportsFunctionOverloading()
datetimeoffset- 0001-01-01 through 9999-12-31 (new date type is appeared)

3. formatDate(date d)

H2 and PostgreSQL format is yyyy-mm-dd.
MS SQL Server supports converting date to string and back based on styles (http://msdn.microsoft.com/en-us/library/ms187928.aspx) not on flexible m..y..d combinations.
MS SQL Server format used in SQL statements is yyyy-mm-dd HH:mm:ss (ODBC canonical style, 120). I am not sure if yyyy-mm-dd will also work and whether current P2J implementation supports using expression convert(varchar, d, 120) instead of date string (d is date to be converted).

4. supportsFunctionOverloading()

MS SQL Server should return false. isCaseInsensitiveColumn(String name)
http://technet.microsoft.com/en-us/library/ms186755%28v=sql.100%29.aspx needsComputedColumns(): “Function names ... must be unique within the database and to its schema.”

5. isConnectionError(Exception exc)

Sometimes SQL states give not enough information about disconnect and do not allow to distinct disconnect from cancel. In some cases we need at first to analyze vendor error codes (exc.getErrorCode()). In some databases we need to check vendor code and parse error message to find some additional sql states.

MS SQL Server has SQL state 08S01 – common state for disconnect and cancel.

Oracle at all has no reliable disconnect sql states, only vendor sql codes 17002, 17008, 17410 and 28, which could be considered as disconnect. Cancel sql state for Oracle is 1013.

6. getDropIndexString(P2JIndex index)

Why “drop index if exists” and not usual “drop index” is used for PostgreSQL?

For MS SQL Server we also can check if index exists with statement like

IF EXISTS (SELECT name FROM sysindexes WHERE name = 'index_name') DROP INDEX index_name

MS SQL Server requires fully qualified index name for index removing.

7. getCreateSequenceString/getDropSequenceString/getSequenceSetValString/getSequenceCurrValString/hasFullSequenceSupport/createSequenceHandler

SQL Server 2008 does not have the concept of a SEQUENCE. It is appeared since SQL Server 2012 (http://technet.microsoft.com/en-us/library/ff878091.aspx). Will we create separate P2JSQLServer2008Dialect and P2JSQLServer2012Dialect?

8. explicitSetCollation()

Should return false. MS SQL Server collation is a property of a database or a column, not of a connection. But we can override the collation on the stetement level using the COLLATE statement like SELECT ... FROM ... COLLATE ...

9. rtrim()

MS SQL Server function rtrim() allows only space trimming and have one argument.

10. getAlterComputedColumnString()

http://technet.microsoft.com/ru-ru/library/ms190273.aspx

MS SQL Server ALTER TABLE does not allow altering usual column to computed one.
For now, I suggest to implement this method with using two ALTER TABLE statements

"alter table %s drop column %s;\n\t alter table %s add %s as %s;"

(drop prefixed column and add computed column with the same name)
It will allow avoid changes in generate_ddl.xml.

Anotgher option is to declare computed columns on CREATE TABLE. BTW, computed columns have no datatype declared.

11. Table creation

Maximum MS SQL Server NUMERIC column precision is 38.
Current hardcoded value for NUMERIC column is set in hibernate.xml on the line

execLib('putAttribute', nextNode, 'precision', '50')

Not fully clear so far how to parameterize this constant depending on database type.

12. "if exists"

For MS SQL Server both supportsIfExistsBeforeTableName() and supportsIfExistsAfterTableName() methods of org.hibernate.dialect.Dialect return false. So org.hibernate.mapping.Table.sqlDropString will never contain "if exists" in contrast to H2 or PostgreSQL. Solution for MSSQL is if exists (select 1 from tablename) drop table tablename

I have added item 7 about sequences to note 9 and question.

I think all usages of dialect.getComputedColumnPrefix() should be corrected for the case when prefix could be meet inside double quoted column name. What do you think?

I'm not sure what use case would make this necessary. We control the conversion of all names, and we don't allow double quoted names. How much effort is this, and under what circumstances do you see it to be needed?

MS SQL Server allows double quoted column names and I meant we need support this too... But it seems I understood now: it is not required for database conversion from 4GL. We don't need double quoted columns at all and shoud use combination of H2/PostgreSQL approaches: prefix and upper/rtrim.

Eric, what do you think about my last question in note 9 ("Will we create separate P2JSQLServer2008Dialect and P2JSQLServer2012Dialect?")?

Sorry, I missed this earlier. Yes, we need to support both (and only those) versions (2008 and 2012). We do not need to support 2005. Where practical, please try to abstract all common code, to avoid duplication.

Please bear in mind that the scope of this task is to generate a SQL Server schema, not to implement all functionality needed at runtime. We will need to add the runtime support later of course, but the customer needs a schema as soon as possible, for evaluation and planning purposes. Where you have identified support that will be needed at runtime, but not necessary to generate a schema, mark those places with method stubs and TODOs in the code, and push forward with the minimum needed to generate a syntactically valid schema. Start with the 2012 support, if that's easier, given the availability of sequences.

I have added item 8 about collation to note 9.

Items 9, 10 about rtrim and getAlterComputedColumnString added to note 9.

Vadim Nebogatov wrote:

10. getAlterComputedColumnString()
...
MS SQL Server ALTER TABLE does not allow altering usual column to computed one.
For now, I suggest to implement this method with using two ALTER TABLE statements

"alter table %s drop column %s;\n\t alter table %s add %s as %s;"

(drop prefixed column and add computed column with the same name)
It will allow avoid changes in generate_ddl.xml.

Anotgher option is to declare computed columns on CREATE TABLE. BTW, computed columns have no datatype declared.

Both solutions seem reasonable. Your choice, just document the reasoning.

I have added item 11 about table creation to note 11.

Vadim Nebogatov wrote:

11. Table creation

Maximum MS SQL Server NUMERIC column precision is 38.

The more you research SQL Server, the less impressed I am by this database!

I also saw a note in the SQL Server doc that this limit applies to 2012, and that the limit for earlier versions is 28 (however I looked at the doc for 2008 R2, 2008, and 2005, and they all said the same thing, so I'm not sure which earlier versions they are talking about)!

Current hardcoded value for NUMERIC column is set in hibernate.xml on the line

execLib('putAttribute', nextNode, 'precision', '50')

Not fully clear so far how to parameterize this constant depending on database type.

I don't think this should be parameterized. These mapping files are independent of database dialect.

Please look into the Hibernate 4.1.8 Final source to see how the mapping code deals with this. I suspect it is used when creating instances of BigDecimal, but we need to know what Hibernate does when the data in Java doesn't match the capabilities of the data type in the backing database. The answer may be in the JDBC driver code. I doubt MS makes that available.

Yes, I also already understood that it does not depend on dialect.

Another problem I have found out that for MS SQL Server both supportsIfExistsBeforeTableName() and supportsIfExistsAfterTableName() methods of org.hibernate.dialect.Dialect return false. So org.hibernate.mapping.Table.sqlDropString will never contain "if exists" in contrast to H2 or PostgreSQL. Solution for MSSQL is if exists (select 1 from tablename) drop table tablename

Does this come into play anywhere other than in generate_ddl.xml? The if exists prefix is a nice safety feature, but in practice we rarely will run the DDL over an existing database. The recommended approach is to run it against a newly created, empty database. How much effort is it to implement your workaround?

Eric Faulhaber wrote:

Does this come into play anywhere other than in generate_ddl.xml?

Hibernate has only two usages of Table.sqlDropString() method:

1) Configuration.generateDropSchemaScript()

Namely this method is invoked now from ConversionDriver before creating new schema.

2) PersistentTableBulkIdStrategy.exportTableDefinitions

Filling tableCleanUpDdl list. Used in MultiTableDeleteExecutor for MSSQL dialect because dialect.supportsTemporaryTables()==false in contrast to H2 and PostgreSQL. Used for ddl generation of clean up concrete internal Hibernate temporary tables (idTableDefinitions ) - it seems not important for P2J because these ddls are executed only on release the strategy, called as the SessionFactory is being shut down.

Eric Faulhaber wrote:

The if exists prefix is a nice safety feature, but in practice we rarely will run the DDL over an existing database. The recommended approach is to run it against a newly created, empty database. How much effort is it to implement your workaround?

If "if exists" is not important we could avoid it on schema export using in generate_ddl.xml:

<rule>exporter.execute(org.hibernate.tool.hbm2ddl.Target.NONE, org.hibernate.tool.hbm2ddl.SchemaExport.Type.CREATE)</rule>

instead of

<rule>exporter.create(false, false)</rule>.

What do you think?

As I understood Hibernate doesn't worry about real column precision in table and takes column
precision from htm file

       <property name="distance">
            <column name="distance" scale="15" precision="3" sql-type="number(15,3)"/>
        </property>

After that this column precision is substituted to database type name.

P2J sets precision, scale in hibernate.xml. Workaround for MSSQL is using in P2JSQLServer2008Dialect

registerColumnType(Types.NUMERIC, "numeric(38,$s)");

in contrast to P2JPostgreSQLDialect:

registerColumnType(Types.NUMERIC, "numeric($p,$s)");

$s will be substituted like now from 'decimals' attribute. Value 50 for precision from hibernate.xml will be ignored in such case. We can do with such way for PostgresSQL too and remove setting precision from hibernate.xml.

Vadim Nebogatov wrote:

If "if exists" is not important we could avoid it on schema export using in generate_ddl.xml:

[...]

instead of

[...]

What do you think?

The Hibernate SchemaExport code is complex enough that it is not obvious to me what your proposed change would do without testing it. Have you tried it? If dropping the "if exists" prefix is the only difference, with no other side effects, then I would say, yes, let's do it this way.

Vadim Nebogatov wrote:

[...]
P2J sets precision, scale in hibernate.xml. Workaround for MSSQL is using in P2JSQLServer2008Dialect

registerColumnType(Types.NUMERIC, "numeric(38,$s)");

in contrast to P2JPostgreSQLDialect:

registerColumnType(Types.NUMERIC, "numeric($p,$s)");

$s will be substituted like now from 'decimals' attribute. Value 50 for precision from hibernate.xml will be ignored in such case. We can do with such way for PostgresSQL too and remove setting precision from hibernate.xml.

Yes, makes sense, there's no need to duplicate the precision for every field, especially since it is subject to a dialect-specific maximum, which I had not considered originally. Good idea, let's do it.

Eric Faulhaber wrote:

Vadim Nebogatov wrote:

If "if exists" is not important we could avoid it on schema export using in generate_ddl.xml:

[...]

instead of

[...]

What do you think?

The Hibernate SchemaExport code is complex enough that it is not obvious to me what your proposed change would do without testing it. Have you tried it? If dropping the "if exists" prefix is the only difference, with no other side effects, then I would say, yes, let's do it this way.

In Hibernate SchemaExport:

public void create(boolean script, boolean export) {
   create( Target.interpret( script, export ) );
}

public void create(Target output) {
   // need to drop tables before creating so need to specify Type.BOTH
   execute( Target.NONE, Type.BOTH );
}

public void execute(Target output, Type type) {
...
}

execute(Target.NONE, Type.CREATE)

create(false, false). 

I tried to do it, but it is strange, generate_ddl.xml rule could not find execute(...) method even I specify classes with packages. I also tried to create wrapping method in P2J code – again does not see method. It works prefect if I insert execute invocation to Java code and I think it is resolvable issue but I wanted to show you before continue next tries.

Ah, TRPL cannot deal with static references. What error messages do you get?

Try:

exporter.execute(false, false, false, true)

Eric Faulhaber wrote:

Ah, TRPL cannot deal with static references. What error messages do you get?

Try:
[...]

It works! No drop statements are generated in ddl files.

Eric Faulhaber wrote:

Vadim Nebogatov wrote:

[...]
P2J sets precision, scale in hibernate.xml. Workaround for MSSQL is using in P2JSQLServer2008Dialect

registerColumnType(Types.NUMERIC, "numeric(38,$s)");

in contrast to P2JPostgreSQLDialect:

registerColumnType(Types.NUMERIC, "numeric($p,$s)");

$s will be substituted like now from 'decimals' attribute. Value 50 for precision from hibernate.xml will be ignored in such case. We can do with such way for PostgresSQL too and remove setting precision from hibernate.xml.

Yes, makes sense, there's no need to duplicate the precision for every field, especially since it is subject to a dialect-specific maximum, which I had not considered originally. Good idea, let's do it.

One more notice: max precision for PostgreSQL is 1000 (http://www.postgresql.org/docs/8.4/static/datatype-numeric.html), for H2 precision is practically unlimited (BigDecimal).

So, precision could be set as dialect && conversion specific in p2j.cfg.xml:

       <parameter name="ddl-dialects" value="h2,postgresql,mssql" />
         <dialect-specific name="h2">
            <parameter name="precision" value="100" />
         </dialect-specific>
         <dialect-specific name="postgresql">
            <parameter name="precision" value="200" />
         </dialect-specific>
         <dialect-specific name="mssql">
            <parameter name="precision" value="38" />
         </dialect-specific>

No, there is no need to set the precision differently by conversion project. The value of 50 was chosen to match Progress. The only reason to deviate from that value is if a specific dialect cannot support it. So, we should go with your earlier approach of setting it in the dialect class.

Please prioritize finishing support for 2012 over 2008. Hopefully, getting sequences to emit in the DDL is easier for that target, since they are actually supported.

Is there anything else left to do besides sequences before you are able to generate a SQL Server 2012 schema that is as feature complete as we have today for PostgreSQL?

TODO after sequences:

isQueryRangeParameterInlined(),
inlineLimit(),
requiresExplicitCastInsideTernary,
usesSingleBackslash(),
prepareMetadataParameter(),
processMetadataResult(),
isMetadataSortDesc(),
isMetadataSortAsc()

It seems like these methods would be necessary at runtime. Are any of them necessary for the generation of a feature-complete schema? If not, this work should be deferred.

BTW, are you creating an org.hibernate.dialect.SQLServer2012Dialect class that we could submit back to the Hibernate project (I'm surprised there isn't one already, considering the differences between the 2008 and 2012 versions)? Or are you implementing the non-P2J-specific dialect methods in P2JSQLServer2012Dialect?

Eric Faulhaber wrote:

It seems like these methods would be necessary at runtime. Are any of them necessary for the generation of a feature-complete schema? If not, this work should be deferred.

They are not used in schema generation. Only sequences remain, I plan complete it in 2-3 hours. Main problem was in implementation of analogue of "if exists" like

   public String getDropSequenceString(String name)
   {
      StringBuilder buf = new StringBuilder(
         "if exists (select * from sys.sequences where name = '");
      buf.append(name);
      buf.append("' and type='SO') drop sequence ");
      buf.append(name);

      return buf.toString();
   }

The same I implemented for indexes

   public String getDropIndexString(P2JIndex index)
   {
      // MS SQL Server requires fully qualified index name for drop index.
      StringBuilder buf = new StringBuilder(
         "if exists (select * from sys.indexes where name = '");
      buf.append(index.getName());
      buf.append("') drop index ");
      buf.append(index.getTable());
      buf.append('.');
      buf.append(index.getName());

      return buf.toString();
   }

If we still need drop tables "if exists" for schema generation, I would suggest to add one more method to P2JDialect specially for drop tables if exists based on select from system views (not use Hibernate's drop table using substituted internally “if exists”).

BTW, are you creating an org.hibernate.dialect.SQLServer2012Dialect class that we could submit back to the Hibernate project (I'm surprised there isn't one already, considering the differences between the 2008 and 2012 versions)? Or are you implementing the non-P2J-specific dialect methods in P2JSQLServer2012Dialect?

There are no SQLServer2012Dialect in Hibernate version we use, so I have created P2jSQLServer2012Dialect extends P2jSQLServer2008Dialect with differences only in 6 sequence methods.

Is it necessary to create the placeholder computed columns in the CREATE TABLE statements, if we are just going to drop them and recreate them in the ALTER TABLE statements later? Why not skip the create and drop steps for these columns?

The varchar(255) columns are going to be problematic. Many converted Progress fields will not fit in this data type. Is there no variable length data type with indeterminate length, like text in PostgreSQL, or varchar (with no length specified) in H2?

Eric Faulhaber wrote:

Is it necessary to create the placeholder computed columns in the CREATE TABLE statements, if we are just going to drop them and recreate them in the ALTER TABLE statements later? Why not skip the create and drop steps for these columns?

I did not find so far where these placeholders are generated? (please igore question, it is found)

The varchar(255) columns are going to be problematic. Many converted Progress fields will not fit in this data type. Is there no variable length data type with indeterminate length, like text in PostgreSQL, or varchar (with no length specified) in H2?

I will use varchar and varbinary.

The output looks good. I would like to review the code. Please upload an update when ready.

Where did the choice of 8,000 as a limit come from?

My only other concern is the rtrim function. The SQL Server version simply doesn't do what we need, since it only trims the space character and not tabs, line feeds, and carriage returns. We can easily write test cases in the 4GL which will break under P2J, due to this limitation. What other alternatives do we have to get the needed functionality?

Eric Faulhaber wrote:

Where did the choice of 8,000 as a limit come from?

It is private constant from SQLServer2005Dialect used in sql types mapping with registerColumnType(). We need to overwrite mapping to varchar/varbinary exactly with this constant.

But I think 4GL character fields can be larger than 8,000.

Greg: what was the rule again? AFAIR, the whole record can be no larger than 32K, but any individual field can claim up to that total. Is that right?

Eric Faulhaber wrote:

But I think 4GL character fields can be larger than 8,000.

Greg: what was the rule again? AFAIR, the whole record can be no larger than 32K, but any individual field can claim up to that total. Is that right?

It seems we need to use special varchar(max) type.

http://technet.microsoft.com/en-us/library/ms176089.aspx:

"max indicates that the maximum storage size is 2^31-1 bytes (2 GB). The storage size is the actual length of the data entered + 2 bytes."

"Use varchar(max) when the sizes of the column data entries vary considerably, and the size might exceed 8,000 bytes."

and to varbinary(MAX). BTW SQLServer2005Dialect also maps to these types when size is not known.

        registerColumnType( Types.BLOB, "varbinary(MAX)" );
        registerColumnType( Types.VARBINARY, "varbinary(MAX)" );
        registerColumnType( Types.VARBINARY, MAX_LENGTH, "varbinary($l)" );
        registerColumnType( Types.LONGVARBINARY, "varbinary(MAX)" );

        registerColumnType( Types.CLOB, "varchar(MAX)" );
        registerColumnType( Types.LONGVARCHAR, "varchar(MAX)" );
        registerColumnType( Types.VARCHAR, "varchar(MAX)" );
        registerColumnType( Types.VARCHAR, MAX_LENGTH, "varchar($l)" );

Greg: what was the rule again? AFAIR, the whole record can be no larger than 32K, but any individual field can claim up to that total. Is that right?

I'm not sure about the "whole record" limitation, but individual text fields can certainly be up to 32000 bytes in size (possibly more if Progress implements any kind of hidden extra byte support.

It seems it is a problem in MSSQL: varchar(MAX) cannot be used in indexes. The maximum amount of bytes on a index is 900. If the column is bigger than 900 bytes, you can create the index but any insert with more then 900 bytes will fail.

http://technet.microsoft.com/en-us/library/ms188783.aspx
Up to 16 columns can be combined into a single composite index key.
The maximum allowable size of the combined index values is 900 bytes.
Columns that are of the large object (LOB) data types ntext, text, varchar(max), nvarchar(max), varbinary(max), xml, or image cannot be specified as key columns for an index.

And N=8000 is restriction for varchar(N) . BTW varchar(MAX) in MSSQL - special type name.

varchar is equivalent to varchar(1)

Eric Faulhaber wrote:

My only other concern is the rtrim function. The SQL Server version simply doesn't do what we need, since it only trims the space character and not tabs, line feeds, and carriage returns. We can easily write test cases in the 4GL which will break under P2J, due to this limitation. What other alternatives do we have to get the needed functionality?

I think we can use expression like

substring(expr, 0, len(replace(replace(replace(expr, CHAR(9), ' '), CHAR(10), ' '), CHAR(13), ' ')))

Or even simpler:

substring(expr, 0, len(replace(expr, CHAR(9)+CHAR(10)+CHAR(13), ' ')))

because + is concatenation operator in sql server.

Vadim Nebogatov wrote:

It seems it is a problem in MSSQL: varchar(MAX) cannot be used in indexes.

[...]

OK, let's go with your original approach with the 8,000 max.

This is good work you are doing, but the limitations you are discovering in this database are quite disappointing. We will have to educate our customers that there are fundamental incompatibilities between the new and old environments when they choose SQL Server as a back end. Hopefully, these will only manifest themselves in some unusual corner cases.

Vadim Nebogatov wrote:

[substring(expr, 0, len(replace(expr, CHAR(9)+CHAR(10)+CHAR(13), ' ')))]

I appreciate your creative solution, looks like that will do what we need. I wish there was a more compact way to represent it, though. Is there any facility to create a function alias for this? Any indication in your research as to whether something this complex within a computed column will have a noticeable impact on performance, especially as tables get large?

Eric Faulhaber wrote:

Vadim Nebogatov wrote:

[substring(expr, 0, len(replace(expr, CHAR(9)+CHAR(10)+CHAR(13), ' ')))]

I appreciate your creative solution, looks like that will do what we need. I wish there was a more compact way to represent it, though. Is there any facility to create a function alias for this?

Yes, I will create sql function for this.

Any indication in your research as to whether something this complex within a computed column will have a noticeable impact on performance, especially as tables get large?

I think performance will depend on concrete tables content. It is possible, in some cases computed columns could decrease performance. Maybe make using computed columns as option for conversion process?

We can also execute UPDATE STATISTICS statement for explicit optimization of repeatable statements. BTW PostgreSQL has an analogue: ANALYZE statement for collect statistics about the content of tables in the database.

Vadim Nebogatov wrote:

Eric Faulhaber wrote:

Vadim Nebogatov wrote:

[substring(expr, 0, len(replace(expr, CHAR(9)+CHAR(10)+CHAR(13), ' ')))]

I appreciate your creative solution, looks like that will do what we need. I wish there was a more compact way to represent it, though. Is there any facility to create a function alias for this?

Yes, I will create sql function for this.

Any indication in your research as to whether something this complex within a computed column will have a noticeable impact on performance, especially as tables get large?

I think performance will depend on concrete tables content. It is possible, in some cases computed columns could decrease performance. Maybe make using computed columns as option for conversion process?

Unfortunately, the work done by the computed columns is required for the behavior to be correct. We can't know whether any given string will have trailing whitespace, so it's not really an option to forego the appropriate trimming. We discovered this early in our first project.

My concern is that the alias will add a significant amount of overhead. With PostgreSQL, we tried to replace the ugliness of upper(rtrim(xyz, ' \t\n\r')) with a PL/SQL function wrapper. Unfortunately, basic operations within the application were up to 3 times slower, so we had to back it out again. I'm worried we will face something similar here, where we have an even more complicated operation.

We can also execute UPDATE STATISTICS statement for explicit optimization of repeatable statements. BTW PostgreSQL has an analogue: ANALYZE statement for collect statistics about the content of tables in the database.

I'm familiar. It's an absolute necessity to have updated statistics to get the query planner to use the correct indexes, but that is a different problem than the basic efficiency of the trimming operation at a low level.

Eric Faulhaber wrote:

I'm familiar. It's an absolute necessity to have updated statistics to get the query planner to use the correct indexes, but that is a different problem than the basic efficiency of the trimming operation at a low level.

Yes it is true.

About using Hibernate SQL Functions and stored procedures (functions) for new rtrim.

It is possible Hibernate SQL Function for new rtrim would be useful in runtime, but for DDL generation more useful would be stored sql function (procedure). I have created and tested such procedure rtrim4gl:

CREATE FUNCTION rtrim4gl (@expr varchar)
RETURNS varchar
WITH EXECUTE AS CALLER
AS
BEGIN
     DECLARE @result varchar;
  SET @result = substring(@expr, 0, len(replace(@expr, CHAR(9) + CHAR(10) + CHAR(13), ' ')));
     RETURN(@result);
END;

alter table address add __city as upper(dbo.rtrim4gl(city));

But I received error message with using such columns in indexes: column __city became “non-deterministic”:

Column '__city' in table 'address' cannot be used in an index or statistics or as a partition key because it is non-deterministic.

If I generate ALTER TABLE as earlier,

alter table address add __city as upper(rtrim(city));

substring(expr, 0, len(replace(expr, CHAR(9) + CHAR(10) + CHAR(13), ' ')))

Not developer-friendly database)

Series of warnings if map to varchar(8000)

Warning! The maximum key length is 900 bytes. The index 'idx__address_city_state' has maximum length of 16008 bytes. For some combination of large values, the insert/update operation will fail.
Warning! The maximum key length is 900 bytes. The index 'idx__address_zip' has maximum length of 8008 bytes. For some combination of large values, the insert/update operation will fail.
Warning! The maximum key length is 900 bytes. The index 'idx__book_isbn' has maximum length of 8000 bytes. For some combination of large values, the insert/update operation will fail.
Warning! The maximum key length is 900 bytes. The index 'idx__book_publisher' has maximum length of 8008 bytes. For some combination of large values, the insert/update operation will fail.
Warning! The maximum key length is 900 bytes. The index 'idx__pers_addr_link_type' has maximum length of 8008 bytes. For some combination of large values, the insert/update operation will fail.
Warning! The maximum key length is 900 bytes. The index 'idx__person_exten' has maximum length of 8000 bytes. For some combination of large values, the insert/update operation will fail.
Warning! The maximum key length is 900 bytes. The index 'idx__person_emp_ssn' has maximum length of 8000 bytes. For some combination of large values, the insert/update operation will fail.
Warning! The maximum key length is 900 bytes. The index 'idx__person_name' has maximum length of 24008 bytes. For some combination of large values, the insert/update operation will fail.
Warning! The maximum key length is 900 bytes. The index 'idx__person_pay_type' has maximum length of 8008 bytes. For some combination of large values, the insert/update operation will fail.
Warning! The maximum key length is 900 bytes. The index 'idx__person_phone' has maximum length of 24000 bytes. For some combination of large values, the insert/update operation will fail.
Warning! The maximum key length is 900 bytes. The index 'idx__vehicle_si_vin' has maximum length of 8000 bytes. For some combination of large values, the insert/update operation will fail.
Warning! The maximum key length is 900 bytes. The index 'idx__vehicle_pi_make_model' has maximum length of 16000 bytes. For some combination of large values, the insert/update operation will fail.
Warning! The maximum key length is 900 bytes. The index 'idx__vehicle_driver_pi_name' has maximum length of 8000 bytes. For some combination of large values, the insert/update operation will fail.
Warning! The maximum key length is 900 bytes. The index 'idx__vehicle_driver_si_vin' has maximum length of 8008 bytes. For some combination of large values, the insert/update operation will fail.
Warning! The maximum key length is 900 bytes. The index 'idx__vehicle_owner_pi_name' has maximum length of 8000 bytes. For some combination of large values, the insert/update operation will fail.
Warning! The maximum key length is 900 bytes. The index 'idx__vehicle_owner_si_vin' has maximum length of 8000 bytes. For some combination of large values, the insert/update operation will fail.

Does this mean a row will not be inserted into the table if it exceeds these limitations of the index, or that the index won't include such an entry? The message suggests the former, though I suppose not having it in the index is just as bad as not having it in the table, if the query planner selects that index for a particular query. Anyway, please confirm the actual behavior.

It may be necessary that we have to put real limits into these data types declarations, which means manual review of every schema generated, a much more time-consuming and error-prone approach. The 4GL schema doesn't give us hard limits to work with, but we can use certain information as clues. We've also created a statistical analysis to estimate actual size in exported data, but we abandoned that approach long ago when we discovered size limits on text data weren't necessary in PostgreSQL and H2. Trouble is, you never know for sure whether a particular data set is truly representative of production data.

Eric Faulhaber wrote:

Does this mean a row will not be inserted into the table if it exceeds these limitations of the index, or that the index won't include such an entry? The message suggests the former, though I suppose not having it in the index is just as bad as not having it in the table, if the query planner selects that index for a particular query. Anyway, please confirm the actual behavior.

Operation failed in this case and row is not inserted into the table.
I have checked with table having one varchar(8000) field and index based on this field. Error happens already when I try to insert varchar having 901 chars. It does not depend whether index is unique or not.

Please clarify something for me. I should have asked this earlier, but I just noticed it now:

substring(city, 0, len(replace(city, CHAR(9) + CHAR(10) + CHAR(13), ' ')))

Also, isn't replace going to replace all specified search strings in the target, wherever they appear (as opposed to just at the end/right)?

We want to right-trim ANY whitespace character (i.e., '\t', '\n', '\r') which appears after the last non-whitespace character in the text data, not the specific sequence of "\t\n\r". So, I think the concatenation is not correct here, if replace looks for the combined string in the target.

I think you are right about this.

Also, isn't replace going to replace all specified search strings in the target, wherever they appear (as opposed to just at the end/right)?

Yes, but the result is OK since len() returns back the length of the string excluding trailing space chars. The actual modified string is only used as input to len(), so the overall result is correct.

Eric, Greg,

Yes, this expression is wrong because second parameter in replace() is the substring to be found, not list of chars to be replaced as I understood initially (they called parameter as string_pattern - it confused me). http://technet.microsoft.com/en-us/library/ms186862.aspx

But first expression I sent seems correct nevertheless

substring(expr, 0, len(replace(replace(replace(expr, CHAR(9), ' '), CHAR(10), ' '), CHAR(13), ' ')))

We replace all chars 9, 10, 13 to spaces, calculate len (without trailing spaces) and return substring of initial expression.

It's very possible that I'm misunderstanding the issue, but isn't the MS SQL server limit less restrictive than the corresponding progress limit?

I was quite worried about this until I discussed it with Guy who pointed out the following:
http://knowledgebase.progress.com/articles/Article/P113284
which indicates that Progress has a 188char limit on index key lengths.

If I understand correctly, this is good news. I was not aware of this limit in Progress.

Vadim, please put together a test case in Progress which would exceed this limit and report the behavior here.

Test:

def temp-table person
    field lname   as char
    field fname  as char
    index piname is primary lname fname
    index iname lname.
def var x as char.
x = … (With different values of variable x )
create person.
person.lname = x.
find person where lname = x.

The total length of the fields in an index exceeds max key size. Index piname of table 
(TEMP-TABLE)person (129) (11353)

The total length of the fields in an index exceeds max key size. Index piname of table 
(TEMP-TABLE)person (129) (11353)
The total length of the fields in an index exceeds max key size. Index iname of table 
(TEMP-TABLE)person (129) (11353)

I guess the 2-character difference in the "usable" portion of the key is attributable to extra overhead caused by the additional field in the piname index, compared to the single field in the iname index.

The knowledgebase article conflicts with the documentation to which it refers (dmadm.pdf for v10.2b, page 2-10). The documentation states a 2000 character limit, with an effective limit of 1970, for version 10.2b. The article states 200 characters (188 effective) for v 10.x and 11.x. The latter seems wrong. However, the documentation implies that there was a 200 limit for versions prior to v10.1c, which is carried forward with databases migrated to 10.1c, "unless explicitly enabled to accept larger index entries" (not sure to what that refers -- maybe a startup option?). [Later update: I needed to read one sentence further: "Enable your migrated database with PROUTIL ENABLELARGEKEYS".]

Anyway, your findings more or less agree with the documentation. Unfortunately, since 1970 > 900, we still have the potential for a runtime issue with a SQL Server schema converted from Progress.

The customer currently is conducting an audit of its customers' installations, in part to determine the actual maximum sizes of the various schema fields. In order to develop the final schema, we may need to use the results of that audit and engineer a mechanism to apply those lengths to the P2J schema conversion. However, we have to hold off on that effort until we have a better idea of what their audit will produce. It may be that such an audit simply becomes a requirement for any customer wishing to use SQL Server with a converted Progress app.

Code review 20131006:

Nice work.

I'm just curious: what is the SelectedMethod=Cursor parameter in the database URL for (generate_ddl.xml)?

Other than this, just some minor formatting/code style requests:

Please remove blank lines between a javadoc comment and its associated method body (see c'tor in P2JSQLServer2008Dialect).

Please javadoc every field. For instance, in P2JSQLServer2008Dialect, there is one javadoc comment for multiple rtrim-related constants. This will generate javadoc for only the first.

For consistency with other code in the project, please use the following form for multi-line field javadoc comments:

/**
 * Comment text...
 */

/** Comment text...
 */

Be consistent with @param tags; the tag and the parameter name should be on the same line, the description on the following line, indented to be even with the parameter name on the previous line. There's a weird @param tag typo or cut-and-paste error for P2JSQLServer2008Dialect.addRtrim4gl.

implements clauses should be on the line after the class declaration.

Don't include the JPRM column in the file header of new files. This refers to an old system we have deprecated.

Use the wildcard in import statements instead of listing every imported class (except to resolve conflicts).

After this bit of cleanup, please run regression testing and commit/distribute.

Eric Faulhaber wrote:

I'm just curious: what is the SelectedMethod=Cursor parameter in the database URL for (generate_ddl.xml)?

List of possible connection properties http://technet.microsoft.com/en-us/library/ms378988.

Connection setting selectMethod = cursor requires less client memory, in our case less memory for P2J server, and more memory for sql server. We can use selectMethod = direct (default setting) in opposite side.

If use recommended for JDBC driver version 2 connection property responseBuffering = adaptive, it is not recommended to set selectMethod = cursor at all http://technet.microsoft.com/us-us/library/bb879937.aspx

I have executed regression two times.

Regression 20131013_090710, 3 FAILED tests:

1. tc_job_002 FAILED failure in step 40: 'Unexpected EOF in actual at page # 1.'
2. gso_292 FAILED failure in step 20: 'timeout before the specific screen buffer became available (Mismatched data at line 2, column 39. Expected 'R' (0x0052 at relative Y 2, relative X 39) and found '' (0x0000 at relative Y 2, relative X 39).)'
3. tc_job_clock_002 FAILED failure in step 20: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 18. Expected '' (0x0000 at relative Y 5, relative X 18) and found '┌' (0x250C at relative Y 5, relative X 18).)'

Regression 20131014_075907, 4 FAILED tests:

1. tc_job_002 FAILED failure in step 40: 'Unexpected EOF in actual at page # 1.'
2. gso_237 FAILED failure in step 8: 'timeout before the specific screen buffer became available (Mismatched data at line 1, column 73. Expected '(' (0x0028 at relative Y 1, relative X 73) and found ' ' (0x0020 at relative Y 1, relative X 73).)'
3. tc_item_master_031 FAILED failure in step 24: 'timeout before the specific screen buffer became available (Mismatched data at line 6, column 72. Expected 'Y' (0x0059 at relative Y 6, relative X 72) and found 'R' (0x0052 at relative Y 6, relative X 72).)'
4. tc_job_clock_002 FAILED failure in step 20: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 18. Expected '' (0x0000 at relative Y 5, relative X 18) and found '┌' (0x250C at relative Y 5, relative X 18).)'

Results:

tc_job_clock_002 failed both times, so I intend to repeat regression 3rd time.
One more detail: timeout descriptions are more descriptive now than earlier.

What time of day (i.e., devsrv01 time -- EDT) did you run the harness?

Eric Faulhaber wrote:

What time of day (i.e., devsrv01 time -- EDT) did you run the harness?

Regression 20131013_090710

Start Date/Time
10/13/2013 09:07:43 -0400
End Date/Time
10/13/2013 12:24:20 -0400

Regression 20131014_075907

Start Date/Time
10/14/2013 07:59:39 -0400
End Date/Time
10/14/2013 11:12:33 -0400

Sorry, silly question, I should have looked at the timestamps. Times seem OK.

Perhaps your last run will resolve this, these particular tests seem very sensitive. I really can't see anything in your changes that would trigger a problem.

Regression 20131015_061357

A lot (20+) failed ts_tests, all related with timeout. I think something happened and I need to repeat test one more time

Regression 20131017_034322, 9 FAILED and 3 FAILED_DEPENDENCY tests:

1. gso_491 FAILED failure in step 15: 'timeout before the specific screen buffer became available (Mismatched data at line 2, column 9. Expected '┌' (0x250C at relative Y 2, relative X 9) and found '' (0x0000 at relative Y 2, relative X 9).)'
2. tc_job_002 FAILED failure in step 40: 'Unexpected EOF in actual at page # 1.'
3. tc_dc_slot_024 FAILED failure in step 8: 'timeout before the specific screen buffer became available (Mismatched data at line 4, column 9. Expected '' (0x0000 at relative Y 4, relative X 9) and found '0' (0x0030 at relative Y 4, relative X 9).)'
4.tc_dc_slot_029 FAILED failure in step 20: 'timeout before the specific screen buffer became available (Mismatched data at line 4, column 0. Expected '' (0x0000 at relative Y 4, relative X 0) and found 'L' (0x004C at relative Y 4, relative X 0).)'
5. tc_job_clock_001 FAILED failure in step 9: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 18. Expected '' (0x0000 at relative Y 5, relative X 18) and found '┌' (0x250C at relative Y 5, relative X 18).)'
6. tc_job_clock_002 FAILED failure in step 20: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 18. Expected '' (0x0000 at relative Y 5, relative X 18) and found '┌' (0x250C at relative Y 5, relative X 18).)'
7. tc_job_clock_003 FAILED_DEPENDENCY dependency chain: tc_job_clock_001
8. tc_job_clock_004 FAILED failure in step 3: 'Timeout while waiting for event semaphore to be posted.'
9. tc_job_clock_005 FAILED failure in step 23: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 18. Expected '' (0x0000 at relative Y 5, relative X 18) and found '┌' (0x250C at relative Y 5, relative X 18).)'
10. tc_job_clock_006 FAILED_DEPENDENCY dependency chain: tc_job_clock_001 --> tc_job_clock_003
11. tc_dc_slot_023 FAILED_DEPENDENCY dependency chain: tc_job_clock_001 --> tc_job_clock_003 --> tc_job_clock_006
12. tc_pay_no_emp_clk_001 FAILED failure in step 18: 'timeout before the specific screen buffer became available (Mismatched data at line 2, column 3. Expected 'E' (0x0045 at relative Y 2, relative X 3) and found ' ' (0x0020 at relative Y 2, relative X 3).)'

Resume:

tc_job_clock_002 failed 3rd time...

Start regression one more time?

Vadim Nebogatov wrote:

Start regression one more time?

Might as well, but please also work with Eugenie in the meantime to interpret the results to understand whether the first 3 failures are legitimate.

Regression 20131018_102350, 3 FAILED tests:

1. tc_job_002 FAILED failure in step 40: 'Unexpected EOF in actual at page # 1.'
2. tc_job_clock_002 FAILED failure in step 20: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 18. Expected '' (0x0000 at relative Y 5, relative X 18) and found '┌' (0x250C at relative Y 5, relative X 18).)'4.tc_dc_slot_029 FAILED failure in step 20: 'timeout before the specific screen buffer became available (Mismatched data at line 4,
3. tc_item_master_104 FAILED failure in step 45: 'Mismatched data at absolute row 89473, relative row 37, page # 1543, page size 58, last page false, column index 20. Expected 'C' (0x43) and found 'E' (0x45).'

But, again, tc_job_clock_002 failed.

Regression 20131018_172945, 5 FAILED tests:

1. tc_job_002 FAILED failure in step 40: 'Unexpected EOF in actual at page # 1.'
2. tc_dc_slot_026 FAILED failure in step 8: 'timeout before the specific screen buffer became available (Mismatched data at line 4, column 9. Expected '' (0x0000 at relative Y 4, relative X 9) and found '1' (0x0031 at relative Y 4, relative X 9).)'
3. tc_job_clock_004 FAILED failure in step 35: 'Timeout while waiting for event semaphore to be posted.'
4. tc_pay_emp_abs_001 FAILED failure in step 12: 'timeout before the specific screen buffer became available (Mismatched data at line 23, column 73. Expected '' (0x0000 at relative Y 23, relative X 73) and found 'I' (0x0049 at relative Y 23, relative X 73).)'
5. gso_144 FAILED Failure in step 69: 'java.lang.RuntimeException: java.lang.RuntimeException: 2: No such file
at com.goldencode.harness.test.FileComparison.execute(FileComparison.java:144)
at com.goldencode.harness.test.Test.execute(Test.java:278)
at com.goldencode.harness.Driver.run(Driver.java:121)
Caused by: java.lang.RuntimeException: 2: No such file
at com.goldencode.harness.transport.SFTPSession.get(SFTPSession.java:284)
at com.goldencode.harness.test.FileComparison.download(FileComparison.java:190)
at com.goldencode.harness.test.FileComparison.execute(FileComparison.java:129)
at com.goldencode.harness.test.Test.execute(Test.java:278)
at com.goldencode.harness.Driver.run(Driver.java:121)
Caused by: com.jcraft.jsch.SftpException: No such file
at com.jcraft.jsch.ChannelSftp.throwStatusError(ChannelSftp.java:2793)
at com.jcraft.jsch.ChannelSftp._stat(ChannelSftp.java:2149)
at com.jcraft.jsch.ChannelSftp._stat(ChannelSftp.java:2166)
at com.jcraft.jsch.ChannelSftp.get(ChannelSftp.java:878)
at com.jcraft.jsch.ChannelSftp.get(ChannelSftp.java:838)
at com.goldencode.harness.transport.SFTPSession.get(SFTPSession.java:278)
at com.goldencode.harness.test.FileComparison.download(FileComparison.java:190)
at com.goldencode.harness.test.FileComparison.execute(FileComparison.java:129)
at com.goldencode.harness.test.Test.execute(Test.java:278)
at com.goldencode.harness.Driver.run(Driver.java:121)

Finally, tc_job_clock_002 passed and I think issue passes regression.

Finally, tc_job_clock_002 passed and I think issue passes regression.

Yes, exactly.

Vadim Nebogatov wrote:

Update vmn_upd20131104a.zip is attached containing P2JH2Dialect fix (method hasFullSequenceSupport() returns true).

Please update the javadoc for the hasFullSequenceSupport() too.

Regresion 20131105_131845 executed with 4 FAILED tests:

I expect these to be false negatives, as there is nothing in MAJIC which uses legacy sequences (plus H2 dialect is used only for temp-tables in MAJIC). Have you checked the DDL differences for H2 dialect? Is the identity generated for H2?

Constantin Asofiei wrote:

I expect these to be false negatives, as there is nothing in MAJIC which uses legacy sequences (plus H2 dialect is used only for temp-tables in MAJIC).

Should I run regression one more time nevertheless?

Have you checked the DDL differences for H2 dialect? Is the identity generated for H2?

I checked DDL differences, of course. Sequences related lines are generated after fix in schema_index_p2j_test_h2.sql:

drop sequence if exists p2j_id_generator_sequence;
create sequence p2j_id_generator_sequence start with 10000;

Please update the javadoc for the hasFullSequenceSupport() too.

Actually I am not sure about FULL sequences support meantioned in javadoc (... bounded values, cycle). Possible support is really not full, but enough for schema conversion.

Vadim Nebogatov wrote:

Should I run regression one more time nevertheless?

No, is not needed.

I checked DDL differences, of course. Sequences related lines are generated after fix in schema_index_p2j_test_h2.sql:

OK.

Actually I am not sure about FULL sequences support meantioned in javadoc (... bounded values, cycle). Possible support is really not full, but enough for schema conversion.

I think I understand what you mean. This might mean that in our H2 DDL case, we need a P2JDialect.hasSequenceSupport API to be used when generating identity sequence DDL. This way we can leave the P2JDialect.hasFullSequenceSupport to be for 4GL-style sequences.

Eric: looking at how DB-level sequences are currently used, P2J will not work with dialects which do not support sequences at all (as our SequenceIdentityManager implementation relies on the DB-level sequences); thus, testing if the dialect has sequences before generating the p2j_id_generator_sequence DDL, to me looks redundant. So: are we planning to implement an IdentityManager which does not rely on the DB-level sequence support? If there is no such need, then we can assume that all dialects (which will be added to P2J) do support DB-level sequences and the dialect.hasFullSequenceSupport() test from ImportWorker.generateIndexDDL can be backed out completely; else, the P2JDialect.hasSequenceSupport API needs to be used.

There is no plan currently to implement an IdentityManager which does not rely on sequences. So far, SQL Server 2008 is the only "modern" database we have found that does not support sequences at all, and our current customer has confirmed they will only require support for SQL Server 2012 and later (which does support sequences). For the foreseeable future, I do not plan to add support for any database which does not support sequences at least at a basic level. Let's then assume for now we will always create the p2j_id_generator_sequence, and if we at some point come across a database without sequence support that a customer absolutely must have, we can revisit this decision.

Is it ok if I change comment for P2JH2Dielect.hasFullSequenceSupport()

@return false as H2 dialect does not fully support 4GL sequences

to

@return true as H2 dialect supports 4GL sequences at least for index DDL generation.

No, the point of the previous entry was to indicate that P2JH2Dialect.hasFullSequenceSupport should NOT be used to determine whether or not to generate the p2j_id_generator_sequence during DDL generation; we will assume all supported databases at least can create that sequence and always include its creation in the DDL. The JavaDoc is thus correct in its current form, as the method is about the "full" support of sequences as compared to the baseline defined by the 4GL sequence feature set.

The update looks good.

Eric: the other change is in the schema/import_util.rules file (the sqlserver2008 dialect is no longer supported). The update can be committed without any more regression testing, as the changes are pretty straightforward and do not affect runtime/conversion (except the H2 DDL).