FWD » Core Development » Database

I wrote:

optimization of conversion resources into a queued service (i.e., to prevent one instance of SchemaDictionary in memory per session)

...as an idea for the production version of this facility.

However, after some discussion with Greg, it seems that making the SchemaDictionary threadsafe, and just having one instance in memory is a preferred approach.

It already is safe to run separate instances of ProgressParser on different threads. However, some work will be necessary to make SchemaDictionary threadsafe. The idea is to share the static, permanent table data it holds and to make any session-specific state (e.g., dynamic temp-tables and buffers, or which static temp-tables are in scope when a particular program is running) context local.

However, I haven't been in this code for a long time, so this approach needs a reality check.

1. it follows the syntax of the OPEN QUERY clause, without the OPEN QUERY prefix
2. reference to non-buffer data (i.e. widgets, variables) is not allowed
3. reference to other buffers (not registered with the query) is not allowed
4. for all registered buffers, a {FOR | PRESELECT} EACH (for the first buffer) or FOR {EACH | FIRST | LAST} (for joined buffers) must be specified

1. the query equivalent in P2J (AdaptiveQuery, PreselectQuery, etc)
2. where clause
3. index

1. temp-table definition (when a buffer references the temp-table)
2. maybe define an explicit buffer for each buffer used by the query. But this will need to be mapped with the buffers used in the WHERE clause; I think this is will not be too difficult, as if we make the resulted HQL query generic, will help optimization. We can cache the instantiated query (to some extent) for a certain (table list, prepare-query string) pair (we can't use the buffer names, as a buffer might reference a certain table now and different one later). If the resulted WHERE clause uses static buffer names, we can easily replace them with the runtime buffer names.
3. the OPEN QUERY command, like OPEN QUERY <query-prepare>..

Ideally, we will be able to dynamically add in the ProgressAst the BUFFER_SCOPE nodes for each buffer and also dynamically build the associated .dict definition for a temp table, to let only the OPEN QUERY command to be parsed, but I'm not yet convinced this will not be very complex (or even possible), as the query parsing requires the SymbolResolver to be initialized with the temp-table data, before the OPEN QUERY is parsed.

Interesting results. These are good limitations as they greatly reduce the possible functionality.

temp-table definition (when a buffer references the temp-table)

I hope you can avoid this.

If the TT is made using CREATE TEMP-TABLE, then during our runtime processing of the definition, we should be calling methods in the SchemaDictionary to add TT configuration that represents this table. This is what we do for DEFINE TEMP-TABLE in the parser. Perhaps you need slightly different methods, but the pieces should already be there.

If the TT is static (via DEFINE TEMP-TABLE/WORK-TABLE), then we should force that the matching configuration to be loaded into the SchemaDictionary. This is information we should save at conversion-time. I think we can include it in the application jar file and add a process to load it. OR we can leverage the XML structure Eric is building for permanent table metadata.

But if the SchemaDictionary is properly configured, it seems that we shouldn't need to fake out the definitions as 4GL code.

Don't know why I forgot the SymbolResolver is backed by the SchemaDictionary for temp-table stuff; with this in mind, I should be able to manage to avoid faking out the definitions as 4GL code.

If the TT is made using CREATE TEMP-TABLE, then during our runtime processing of the definition, we should be calling methods in the SchemaDictionary to add TT configuration that represents this table.

We will need to sync it with the resource's scoping.

If the TT is static (via DEFINE TEMP-TABLE/WORK-TABLE), then we should force that the matching configuration to be loaded into the SchemaDictionary. This is information we should save at conversion-time. I think we can include it in the application jar file and add a process to load it. OR we can leverage the XML structure Eric is building for permanent table metadata.

The temp-table name is not unique throught the application. More, the temp-table definition might come from a completely different file... when we define a temp table in P2J, we will need to resolve its associated legacy configuration (.dict file or anything else), based on the program name where the definition takes place. We can register it with the SchemaDictionary upfront, but we will a scoping mechanism along with it, as at a certain point only certain temp-tables are visible.

Related to note 8 - see the discussion in notes 20-36 in #1654. Summary: TableMapper was added to map legacy names for temp-tables.

Next on the table is how the dynamic queries work. In 4GL, a static query can have its predicate changed, via QUERY-PREPARE; beside this, some state can not be changed (i.e. buffers can't be added for a static query), but other state can be interrogated (i.e. INDEX-INFORMATION works for static queries too). With this in mind, the implementation of QUERY-PREPARE method (which changes the underlying P2J query) must be at the QueryWrapper class. Some of the dynamic query-related APIs need to be implemented at QueryWrapper, and not at AbstractQuery. AbstractQuery should throw a RuntimeException if some code finds its way there.

The conclusion is that for us, QueryWrapper is the most appropriate to mark as the legacy resource. A handle should never reference an AbstractQuery sub-class, only a QueryWrapper can be referenced.

1. walk the JavaAst and instantiate the query, add components, etc (this will be pretty complex, as there are lots of cases which will need to be treated). Consider that the following code is a valid prepare query:
   

   def temp-table tt1 field f1 as int field f2 as int extent 2.
   
   create tt1.
   tt1.f1 = 5.
   tt1.f2[1] = 1.
   tt1.f2[2] = 2.
   
   def var hq as handle.
   create query hq.
   hq:add-buffer(buffer tt1:handle).
   hq:add-buffer(buffer book:handle).
   hq:query-prepare("for each tt1, each book where book.book-id = tt1.f1 and tt1.f2[1] > 0").
   hq:query-open().
   
   hq:get-first().
   
   message tt1.f1 book.book-id book.book-title.
   

   
   During conversion of a open query for each tt1, each book where book.book-id = tt1.f1 and tt1.f2[1] > 0 this will result in client-side where clauses, multiple components, etc.
2. The second approach would be to alter the generated JavaAST so that this code is produced (an example for the for each tt1, each book where book.book-id = tt1.f1 and tt1.f2[1] > 0 predicate:
   

   public class InMemQuery
   {
      /* all buffer fields are uninitialized and made public */
      public Book.Buf book = null;
   
      public TempRecord1.Buf tt1 = null;
   
      /* the query field is moved to the top-level class */
      CompoundQuery query0 = null;
   
      WhereExpression whereExpr0 = new WhereExpression()
      {
         public logical evaluate(final BaseDataType[] args)
         {
            return isGreaterThan(tt1.getF2(0), 0);
         }
      };
   
      /* a getter for the generated query is added */
      public P2JQuery getQuery()
      {
         return query0;
      }
   
      public void execute()
      {
         // the RecordBuffer.openScope and the query0.open() calls are dropped, and all the code between these two calls are moved to the execute() method
         query0 = new CompoundQuery(true, false);
         query0.addComponent(new AdaptiveQuery(tt1, (String) null, null, "tt1.id asc"));
         RecordBuffer.prepare(book);
         query0.addComponent(new AdaptiveQuery(book, "book.bookId = ?", whereExpr0, "book.bookId asc", new Object[]
         {
            new FieldReference(tt1, "f1")
         }));
      }
   }
   

   
   Thus, we can use the same approach as with the in-memory DMOs: the source code is compiled in memory, added to the InMemoryClassLoader and this new class is used to instantiate the query using reflection, following these steps:
   - create a new instance of this class
   - as the names of the instance fields referencing the buffers have the same names as the passed buffers, set them accordingly
   - call execute() on this instance to initialize the query
   - call getQuery() on this instance to extract the created query.

The 2nd approach (in-memory compilation of real java code, like with the dynamic DMOs) is the one that I believe is the best option. It should be simpler (less work). It is also less risky because we don't have to create an interpreter which is essentially a parallel implementation of the some significant runtime functionality. I think the time to generate and compile the Java code will be a minimal cost to pay.

1. always use the fully-qualified converted class name as the key
2. use categories based on the AST class, to differentiate between i.e. JavaAST and ProgressAST
3. persist() and persistJavaFile() will check for in-memory conversion case and if so, it will store the AST in the required category, depending on the AST's class.

Stanislav, is there a specific reason why you chose to explicitly store objects rather than build something generic in persist() and persistJavaFile()?

At that point I tried to make a generic solution that works with abstract in-memory objects and require minimal changes in the existing conversion rules. Note that we can have xml files and may have additional steps for building file path in the case of persistent files.

1. following test does convert and produces valid output
   

   def temp-table tt1 field f1 as int.
   
   create tt1.
   tt1.f1 = 10.
   create tt1.
   tt1.f1 = 20.
   
   open query q for each tt1 where tt1.f1 < 20.
   get first q.
   message tt1.f1.
   
   release tt1.
   
   def var h as handle.
   
   create query h.
   h:add-buffer(buffer tt1:handle).
   h:query-prepare("for each tt1 where tt1.f1 < 20").
   h:query-open().
   h:get-first().
   
   message tt1.f1.
   

2. to some extent, static queries can be made dynamic (i.e.they can have their predicate changed at runtime). I will implement this too, if is not too much work.
3. currently, the entire annotations/base_structure/core_conversion rulesets are ran, but I will try to limit this to the query-related rulesets. Greg: is there a way to conditionally include a rule-set? I'm thinking of doing something like this, in i.e. annotations.xml:
   

      <!-- main processing (once per tree) -->
   
      <!-- must be executed first, to make sure ASTs are not removed, altered -->
      <rule-set condition="!isRuntimeQuery()" name="annotations/collect_published_events" />
   
      <rule-set condition="!isRuntimeQuery()" name="annotations/process_reachable_refs" />
   

   
   but I'm not sure how easy is to integrate it. After looking through the query-related conversion code, I'm a little reluctant in creating rulesets for the annotation/core-conversion of dyn queries. IMO it will be cleaner to conditionally include rulesets. Instead of using expressions for the condition attribute, we can define some static flags which can be checked from the Configuration object (I think this will be easier to do). We should allow more than one flag to be set for a rule-set, too.
4. had to build a InMemoryRegistryPlugin which is hooked up as the default AstManagerPlugin for dynamic queries. This acts like an in-memory storage for the created ASTs. The generic storage mechanism is still used to retrieve the java code from brew.xml, so that is still needed.
5. the support for the DYNAMIC attribute needs to be fixed, as this attribute is for more than one resource (currently is added in various interfaces). Stanislav: have you touched this? If not, I will work on it.
6. the dyn query implementation relies on the BUFFER:TABLE-HANDLE attribute to work for both static and dynamic temp-tables. Stanislav: is this going to be supported by the first version of the dynamic buffer implementation?
7. need to add index support at the .dict/.p2o integration for temp tables. But for this, I will need the legacy name/definition of the index. As Java 7 does not allow repeating annotations (Java 8 adds this), we can add something like this, at the DMO implementation class:
   

   @LegacyIndexes({@LegacyIndex(name = "idx1", primary = true,
                                fields = @LegacyIndexFields({@LegacyIndexField(name = "f1", asc = false),  
                                                             @LegacyIndexField(name = "f2", asc = false)})),
                   @LegacyIndex(name = "idx2", unique = true,
                                fields = @LegacyIndexFields({@LegacyIndexField(name = "f3", asc = true),  
                                                             @LegacyIndexField(name = "f4", asc = false)}))})
   public class TempRecord
   {
   ...
   }
   

   
   where the annotation classes look like:
   

   @Target(ElementType.TYPE)
   @Retention(RetentionPolicy.RUNTIME)
   public @interface LegacyIndexes
   {
      LegacyIndex[] value();
   }
   
   @Target(ElementType.TYPE)
   @Retention(RetentionPolicy.RUNTIME)
   public @interface LegacyIndex
   {
      String name();
   
      boolean unique() default false;
   
      boolean primary() default false;
   
      boolean word() default false;
   
      LegacyIndexFields fields();
   }
   
   @Target(ElementType.TYPE)
   @Retention(RetentionPolicy.RUNTIME)
   public @interface LegacyIndexFields
   {
      LegacyIndexField[] value();
   }
   
   @Target(ElementType.TYPE)
   @Retention(RetentionPolicy.RUNTIME)
   public @interface LegacyIndexField
   {
      String name();
   
      boolean asc() default true;
   }
   

   
   Eric: please let me know what you think.
8. need to check how DB aliases are used in prepare strings
9. need to check if my current SchemaDictionary can work easily with runtime-connected DBs
10. finish the others query attributes/methods

Constantin Asofiei wrote:

following test does convert and produces valid output
[...]

Very cool!

need to add index support at the .dict/.p2o integration for temp tables. But for this, I will need the legacy name/definition of the index. As Java 7 does not allow repeating annotations (Java 8 adds this), we can add something like this, at the DMO implementation class:
[...]
where the annotation classes look like:
[...]
Eric: please let me know what you think.

This looks like it will do the job, but note that we already have a lot of knowledge of index information in the runtime via the IndexHelper class. It may not be served exactly the way you need it, but I'm OK with changes to the API if it's close enough. You would need to use the field-level annotations you have added to the DMOs to replace the property names with their original, legacy counterparts at a minimum. Have a look and see if it meets your needs. If not, we can go with your proposed approach, but I'm concerned about maintaining this information in multiple places.

5. the support for the DYNAMIC attribute needs to be fixed, as this attribute is for more than one resource (currently is added in various interfaces). Stanislav: have you touched this? If not, I will work on it.

No, I haven't touched this.

6. the dyn query implementation relies on the BUFFER:TABLE-HANDLE attribute to work for both static and dynamic temp-tables. Stanislav: is this going to be supported by the first version of the dynamic buffer implementation?

AFAIK, we do not support temp-table handles, so I guess it will not come with the very first version.

AFAIK, we do not support temp-table handles, so I guess it will not come with the very first version.

The dynamic temp-table stuff you've added implements a dynamic TEMP-TABLE resource. But I need the equivalent for static temp-tables, as I don't want to keep the legacy temp-table mappings by temp-table name (as in 4GL they seem to be referenced internally by the UNIQUE-ID attribute). I don't need full support, I need just the TEMP-TABLE:UNIQUE-ID attribute and the TempTableBuilder.persistentClass to be implemented for static temp-tables.

Eric Faulhaber wrote:

... we already have a lot of knowledge of index information in the runtime via the IndexHelper class. ... Have a look and see if it meets your needs.

Actually, I think the DMOIndex.databaseIndexes is the right API for me; this will give me a list of P2jIndex instances with the full index definition. The only thing missing is the legacy index name. For this, I will need annotations like:

@LegacyTable(name = "tt1")
@LegacyIndexes({@LegacyIndex(name = "idx-1", converted = "idx1"),
                @LegacyIndex(name = "idx-2", converted = "idx2")})
public class TempRecord1
{
...
}

@Target(ElementType.TYPE)
@Retention(RetentionPolicy.RUNTIME)
public @interface LegacyIndexes
{
   LegacyIndex[] value();
}

package com.goldencode.p2j.persist;

import java.lang.annotation.*;

@Target(ElementType.TYPE)
@Retention(RetentionPolicy.RUNTIME)
public @interface LegacyIndex
{
   String converted();

   String name();
}

I should have thought of this earlier: I am regression testing metadata support (#2115), which includes the _Index table and _Index-Field table. I am currently regression testing the base support; next I will add a MetaSchema interface, which is an internal API to support attributes and other cases where we need legacy information. I can add an API to the MetaSchema interface to give you exactly what you need, without having to maintain those annotations separately. What would these API signatures look like for your needs?

Eric, the same problem with the legacy temp-table name: temp-table information is not in the legacy metadata tables; only the permanent tables are supported there. Unless there is something I'm missing, I don't think we can get away from using DMO impl class annotations with the legacy index names.

Yes, good point. Sorry, haven't had my morning coffee yet. <g>

currently, the entire annotations/base_structure/core_conversion rulesets are ran, but I will try to limit this to the query-related rulesets. Greg: is there a way to conditionally include a rule-set? I'm thinking of doing something like this, in i.e. annotations.xml:

   <!-- main processing (once per tree) -->

   <!-- must be executed first, to make sure ASTs are not removed, altered -->
   <rule-set condition="!isRuntimeQuery()" name="annotations/collect_published_events" />

   <rule-set condition="!isRuntimeQuery()" name="annotations/process_reachable_refs" />

but I'm not sure how easy is to integrate it. After looking through the query-related conversion code, I'm a little reluctant in creating rulesets for the annotation/core-conversion of dyn queries. IMO it will be cleaner to conditionally include rulesets. Instead of using expressions for the condition attribute, we can define some static flags which can be checked from the Configuration object (I think this will be easier to do). We should allow more than one flag to be set for a rule-set, too.

Sure, you can add set of flags for conditional loading. I prefer not to use an embedded expression. But flags are OK. We already do have something similar in our attribute optional="true". See the ruleSet() method in ConfigLoader where we process ATTR_OPT. The idea here would be the following:

• Mark the rule-sets that you need to include with a flag like load-condition="runtime-query".
• This value can be a comma-separated list of flags, so that we can create different "sets" to be loaded.
• By default, we process all rule-sets without honoring the load-condition processing.
• When in runtime query mode, we update the Configuration object to turn on the load limit mode and explicitly enable some list of flags.
• ConfigLoader.ruleSet() is modified to process this flag and when the load limit mode is enabled, only those rule-sets that match one of the enabled flags will be added to the maps of rule-sets. All other rule-sets are silently ignored.

I need just the TEMP-TABLE:UNIQUE-ID attribute and the TempTableBuilder.persistentClass to be implemented for static temp-tables.

Constantin, in order to generate UNIQUE-ID for static temp-tables, their creation should be tracked at runtime. Do we already have a facility impelmented for that?

Stanislav Lomany wrote:

Constantin, in order to generate UNIQUE-ID for static temp-tables, their creation should be tracked at runtime. Do we already have a facility impelmented for that?

The TEMP-TABLE resource should be created/dropped when the temp-table is created/dropped in P2J; there are no notifiers, for mapping I've just added TableMapper.map/removeTemporaryTable(s) in TemporaryBuffer$Context.dropTable, TemporarBuffer$Context.createTable and TemporaryBuffer$Context.dropAllTables.

Btw, I see that I've added a TableMapper.removeTemporaryTable call in TemporaryBuffer.dropTable - but I think this is incorrect, as the mappings need to be removed when the temp-table is actually dropped. I'll fix this.

1021g.zip (legacy index annotations), 1021f.zip (conditional rule-sets) and 1021c.zip (DYNAMIC attribute support) have passed conversion and runtime testing.

Code Review 1021c

1. It seems like the methods_attributes.rules change should have hwrap as DynamicResource not Dynamicable. But then again, the method name is unwrapDynamicable(). This seems broken.

2. TempTableBuilder._dynamic() is hard coded to true. Why not just return the value of the dynamic data member?

Code Review 1021f

I am fine with the change to brew.xml. Eric will have to review the rest.

Code Review 1021g

1. I prefer the enable/disable of conditional rule-set processing to be done at the PatternEngine level. That would allow different instances of the pattern engine to be configured differently within the same JVM. If you are directly creating a PatternEngine instance, then this is not difficult. If you are reusing the entire ConversionDriver, then a bit of extra effort may be there, but I think it is worth it.

2. Instead of hard coding the RUNTIME-QUERY string and explicit getters/setters into the Configuration object, I prefer if you would make the facility generic. That way, future users of this won't have to change Configuration. They should just be able to call addConditionalFlag() or removeConditionalFlag() to configure it.

Code Review 1021c
1. It seems like the methods_attributes.rules change should have hwrap as DynamicResource not Dynamicable. But then again, the method name is unwrapDynamicable(). This seems broken.

The handle.unwrap[hwrap] API referenced by the hwrap var in methods_attributes.rules is not hard-linked with the interface name. What it matters is that the returned type defines the APIs which are accessed using the handle.unwrap[hwrap] call. There are no failures related to the name.

2. TempTableBuilder._dynamic() is hard coded to true. Why not just return the value of the dynamic data member?

OK, I'll change it.

Code Review 1021g
1. I prefer the enable/disable of conditional rule-set processing to be done at the PatternEngine level.

This will mean that all the rule-sets (even ignored ones) will be parsed. I was thinking to avoid this. If the entire rule container is loaded and the flags are checked when the rule-set is executed, then the flags can not be at the rule-set node in the rule container, they must be in the root rule-set node in the .rules file. And I think this should only prevent rule-set processing (init-/walk-/post-rules) and still allow it to be included (to expose any function libraries).

That would allow different instances of the pattern engine to be configured differently within the same JVM.

I don't think I understand this one. The flags and the conditional rule-set enable/disable at the Configuration object are context-local, thus flags set by a certain user will be seen only by that context. More, for dyn query purpose, I'm processing each rule-container in its own PatternEngine instance. Do you mean that a PatternEngine holds some JVM-wide data?

If you are reusing the entire ConversionDriver, then a bit of extra effort may be there, but I think it is worth it.

I didn't find any gain in reusing the ConversionDriver, as for the dyn query purpose, the AST returned by the ProgressParser had to be annotated to specify java-related names (i.e. class, package) and I even had to add bogus ASTs representing a DEFINE TEMP-TABLE tt. statement (with no fields), so that the temp-table name will be converted properly (there is some code in record_scoping_prep.rules which relies on prog.define_temp_table nodes).

2. Instead of hard coding the RUNTIME-QUERY string and explicit getters/setters into the Configuration object, I prefer if you would make the facility generic. That way, future users of this won't have to change Configuration. They should just be able to call addConditionalFlag() or removeConditionalFlag() to configure it.

I'm OK with allowing the user to add/remove any flag, but I still want to have a central point to define the constants for the flag names. I think a ConversionFlag enum will be OK.

The handle.unwrap[hwrap] API referenced by the hwrap var in methods_attributes.rules is not hard-linked with the interface name. What it matters is that the returned type defines the APIs which are accessed using the handle.unwrap[hwrap] call. There are no failures related to the name.

OK. But I do prefer it to be consistent (resource name the same as the unwrap method name).

This will mean that all the rule-sets (even ignored ones) will be parsed. I was thinking to avoid this. If the entire rule container is loaded and the flags are checked when the rule-set is executed, then the flags can not be at the rule-set node in the rule container, they must be in the root rule-set node in the .rules file. And I think this should only prevent rule-set processing (init-/walk-/post-rules) and still allow it to be included (to expose any function libraries).

What I mean here is to move the setConditionalRuleSets()/isConditionalRuleSets() methods and flag into the PatternEngine from the Configuration class. The implementation of conditional processing would still be in ConfigLoader.

The flags and the conditional rule-set enable/disable at the Configuration object are context-local, thus flags set by a certain user will be seen only by that context.

Yes, this is a good point.

More, for dyn query purpose, I'm processing each rule-container in its own PatternEngine instance.

Good. And there will be more of this kind of thing in the future (where multiple PE instances will run in the same JVM at the same time) when we implement a fully-multithreaded TRPL.

The key point I am going for here is that this is really best suited as an attribute/flag of the PE, not something that is configured externally.

• We are trying to eliminate/reduce dependence on Configuration.
• This makes the feature more generic which is a useful thing for future TRPL users.

but I still want to have a central point to define the constants for the flag names. I think a ConversionFlag enum will be OK.

I am OK with an enum, but let's keep it out of code in the pattern/ and cfg/ packages. Those packages are meant to be generic TRPL and should have few (if any) dependencies on P2J or even the conversion driver. Put it in convert/ where we have code that is specific to converting 4GL to Java.

Again, this is consistent with making this a generic improvement to TRPL that future users can take advantage of.

1. in the rule-container, at the rule-set node (which is a child of a cfg node)
2. in the rule definition .rules file, at the root rule-set node

To me, it looks best to set the flags at the rule-container, and not at the .rules file. I don't want to exclude function libraries included via include. For this, I think is best to use the flags only if a name attribute is given to the rule-set node (i.e. this is a child of the rule-container's root cfg node).

I am OK with this approach.

The database portions (1021c/g) look OK to me, as long as we address the TODO in TempTableBuilder.asHandle. I have the same question as Greg above: why does the TempTableBuilder._dynamic method always return true instead of the value of dynamic?

Greg: 1023a.zip needs some more testing, please let me know if the approach is correct.

Code Review 1023a/1023b

I am good with all these changes.

The only request I have is to replace the javadoc text in PatternEngine that refers to RUNTIME-QUERY. Please make it generic.

If it passes conversion testing, then do check it in and distribute it.

Code Review 1023d

1. I really like the changes. It is possible some of the file normalization could cause breakage during conversion processing or possibly during report processing. But if it does cause any problems, it will be because we have some improper processing currently and I would prefer you to fix those locations. The bottom line is that I do believe there may be a few places where we aren't handling the normalization well, but I'd rather have them normalize.

2. The InMemoryRegistryPlugin has this comment: implements an in-memory XML storage option for an AstManagerPlugin. Does it really have anything to do with XML?

3. Should the AstManager.initialize() call (in StandardServer) really be added so early in bootstrap()? It seems to me that unless it is needed very early, it is safer to leave it to later (after registerDefaultServices() seems good). If it doesn't cause a problem to do this, I prefer it because putting it before the directory init and so forth suggests to future readers that there is some important need for this to be done so early. I prefer to make it clear that there is no such need.

2. The InMemoryRegistryPlugin has this comment: implements an in-memory XML storage option for an AstManagerPlugin. Does it really have anything to do with XML?

No, just ASTs. That should been read "AST storage".

3. Should the AstManager.initialize() call (in StandardServer) really be added so early in bootstrap()? It seems to me that unless it is needed very early, it is safer to leave it to later (after registerDefaultServices() seems good). If it doesn't cause a problem to do this, I prefer it because putting it before the directory init and so forth suggests to future readers that there is some important need for this to be done so early. I prefer to make it clear that there is no such need.

I want to initialize the AstManager before anyone else gets a chance to initialize it, as this is a JVM-wide setting. Other classes (like PatterEngine and SchemaLoader) call AstManager.initialize() in their static c'tor. To me, actually it would look even better if the initialziation is done in the static c'tor for StandardServer.

I want to initialize the AstManager before anyone else gets a chance to initialize it, as this is a JVM-wide setting. Other classes (like PatterEngine and SchemaLoader) call AstManager.initialize() in their static c'tor. To me, actually it would look even better if the initialziation is done in the static c'tor for StandardServer.

I understand the concern, but there shouldn't be any issue with PatternEngine or SchemaLoader executing before the registerDefaultServices() right? Until that point we can't even have converted 4GL code properly run.

If there is ever a time when we need the PatternEngine, SchemaLoader... to be run earlier in the server startup, we can move the AstManager.initialize() accordingly. Until then, let's keep it later rather than sooner.

Code Review 1023d

This has passed conversion testing (as the other 2 updates), but I'll keep it a little more until releasing it, want to make sure the file normalization is stable.

1. query attributes:
   The following are almost done:
   

   NAME
   DYNAMIC
   ADM-DATA
   TYPE
   UNIQUE-ID
   PRIVATE-DATA 
   PREPARE-STRING
   NUM-BUFFERS 
   NUM-RESULTS
   CURRENT-RESULT-ROW
   QUERY-OFF-END
   IS-OPEN
   FORWARD-ONLY
   

   
   Following are low priority:
   

   INDEX-INFORMATION
   

   
   Following are not needed for the server project:
   

   INSTANTIATING-PROCEDURE
   BASIC-LOGGING
   CACHE
   SKIP-DELETED-RECORD
   

2. query methods:
   QUERY-PREPARE() needs more work/testing, but the following are almost done:
   

   GET-CURRENT( )
   GET-FIRST( ) 
   GET-LAST( )
   GET-NEXT( ) 
   GET-PREV( )
   QUERY-CLOSE( ) 
   QUERY-OPEN( )
   ADD-BUFFER( )
   SET-BUFFERS( )
   GET-BUFFER-HANDLE( ) 
   

   
   Following are low priority:
   

   REPOSITION-FORWARD( ) 
   REPOSITION-TO-ROW( )
   REPOSITION-TO-ROWID( ) 
   CREATE-RESULT-LIST-ENTRY( )
   DELETE-RESULT-LIST-ENTRY( ) 
   

   
   Following are not needed for the server project:
   

   DUMP-LOGGING-NOW( ) 
   APPLY-CALLBACK( ) 
   SET-CALLBACK( ) 
   SET-CALLBACK-PROCEDURE( )
   GET-CALLBACK-PROC-CONTEXT( )
   GET-CALLBACK-PROC-NAME( ) 
   REPOSITION-BACKWARD( )
   

Eric: I need the extent information for a field too; should I annotate the inner Composite class or I can assume everything after the "Composite" prefix is the legacy extent?

Stanislav: for dynamic temp-tables, I see that you clean all the dynamic DMOs in TempTableBuilder$Context.finished. I probably need something like this for my query builder too, but I don't understand the Context.finished code: why is it dropping all DMOs? Because TEMP-TABLEs can be created in a program executed persistent and AFAIK they need to survive until the persistent program is deleted. But you drop them when the block which called the CREATE TEMP-TABLE statement ends; is there something I'm missing?

Do we need a new "hook" to handle the class unloading "event" in a RUN PERSISTENT case? For a procedure run as non-persistent, our Finalizable.finished() which is called when the external procedure ends is the equivalent to closing resources when the procedure is unloaded, since these both happen at the same time. But when a procedure is run persistent, then the external procedure will end (and presumeably Finalizable.finished() will be called) long before the procedure is actually unloaded.

I'm worried this has implications for many resources that use Finalizable.

Greg Shah wrote:

Do we need a new "hook" to handle the class unloading "event" in a RUN PERSISTENT case?

Yes, something like that. In 4GL, the resource might be linked via the INSTANTIATING-PROCEDURE to its "owner": maybe when a persistent procedure is deleted they notify all resources and the ones which have an equal INSTANTIATING-PROCEDURE reference are deleted too? In any case, this is not something query or temp-table specific. I think a separate task is appropriate, to check that all resources (static or dynamic) are deleted properly. And to implement this, I think we will need to register all resources with the current THIS-PROCEDURE instance, at the time of the resource creation; this way, when the procedure gets deleted, the resources can be deleted before the procedure. We might have problems with resources instantiated by the default c'tor, because THIS-PROCEDURE might not be pushed this early (IIRC is pushed when the external procedure block is executed).

Constantin Asofiei wrote:

Stanislav: for dynamic temp-tables, I see that you clean all the dynamic DMOs in TempTableBuilder$Context.finished. I probably need something like this for my query builder too, but I don't understand the Context.finished code: why is it dropping all DMOs? Because TEMP-TABLEs can be created in a program executed persistent and AFAIK they need to survive until the persistent program is deleted. But you drop them when the block which called the CREATE TEMP-TABLE statement ends; is there something I'm missing?

Are you talking about DynamicTablesHelper$Context? If yes, then finished is executed when session ends.

Constantin Asofiei wrote:

Eric: I need the extent information for a field too; should I annotate the inner Composite class or I can assume everything after the "Composite" prefix is the legacy extent?

If you are just looking for the size of the array/extent, you can either get this by parsing the "Composite" class name and taking it from the end, or by invoking the "sizer" methods. Each extent field has a size<PropertyName> method in the DMO, which returns an integer.

Stanislav Lomany wrote:

Constantin Asofiei wrote:

Stanislav: for dynamic temp-tables, I see that you clean all the dynamic DMOs in TempTableBuilder$Context.finished. I probably need something like this for my query builder too, but I don't understand the Context.finished code: why is it dropping all DMOs? Because TEMP-TABLEs can be created in a program executed persistent and AFAIK they need to survive until the persistent program is deleted. But you drop them when the block which called the CREATE TEMP-TABLE statement ends; is there something I'm missing?

Are you talking about DynamicTablesHelper$Context? If yes, then finished is executed when session ends.

Yes, about that. I didn't notice that you were registering the Finalizable in the global block (which is associated with the user's session). Thanks, now your code makes sense.

Eric Faulhaber wrote:

Each extent field has a size<PropertyName> method in the DMO, which returns an integer.

Thanks, I'll use this.

I think a separate task is appropriate, to check that all resources (static or dynamic) are deleted properly.

Please do create such a task and mark it for milestone 7.

Greg Shah wrote:

I think a separate task is appropriate, to check that all resources (static or dynamic) are deleted properly.

Please do create such a task and mark it for milestone 7.

See #2196.

Eric, I'm finally going into the record scoping behavior for the dynamic queries, and I have a question: why don't we set the RecordBuffer.dmoClass (and RecordBuffer.table too) at the buffer definition, in RecordBuffer.define? This info isn't related to the held record or transaction awareness and IMO access to them should be allowed regardless if the buffer was initialized or not.

I didn't want to rely on the naming convention alone to determine the DMO class from the DMO interface. So, in order to determine the DMO class, I needed some other precursor information to set the dmoClass. This information is only realized in the initialize method. If you see a better way to do this, I am not opposed to it in principal.

Eric Faulhaber wrote:

I didn't want to rely on the naming convention alone to determine the DMO class from the DMO interface. So, in order to determine the DMO class, I needed some other precursor information to set the dmoClass. This information is only realized in the initialize method. If you see a better way to do this, I am not opposed to it in principal.

Thanks, after a deeper look is more complex, as it depends on whether the DB is connected or not and other stuff. I'll call initialize() for now before getting the DMO impl class.

On a side note, I think the record scoping for OPEN QUERY is broken in P2J, when a RETURN ERROR is encountered:

def var h as handle.

procedure proc0.
   create query h.
   h:add-buffer(buffer book:handle).
   h:query-prepare("for each book").
   h:query-open().
   h:get-first().

   message book.book-id book.book-title.
   return error.
end.

procedure proc0b.
   open query q for each book.
   get first q.
   message book.book-id book.book-title.
   return error.
end.

procedure proc1.
  find first book no-error.
  if not avail book
  then do transaction:
     create book.
     book.book-title = "bla".
     book.isbn = "aaa".
     book.book-id = 10.
  end.
  release book.
end.

/* ensure there is at least one record in the table. */
run proc1.

run proc0 no-error.
if avail(book) then message "Case 1: Book should not be available!".

run proc0b no-error.
if avail(book) then message "Case 2: Book should not be available!".

I found something interesting and pretty weird about the static QUERY behaviour. Although the query handle CAN be deleted and is deleted, it seems like the query gets "resurrected":

def var h1 as handle.
def var h2 as handle.
def temp-table tt1 field f1 as int.

def var i as int.

do i = 1 to 10 transaction:
  create tt1.
  tt1.f1 = i.
end.
release tt1.

def query q for tt1.
open query q for each tt1.
get first q.
h1 = query q:handle.
h2 = query q:handle.

delete object h1.

if valid-handle(h1) <> valid-handle(h2) or h1 <> h2
   then message "handles must be the same!".

h1 = query q:handle. /* here, q points to a different resource !*/

if h1 = h2
   then message "handles must be different!".

if not valid-handle(h1)
   then message "h1 must be valid!".

release tt1.
get next q.
if tt1.f1 <> 2
   then message "incorrect record fetched!" 

I guess this makes sense on some level. For a dynamically created query, the handle(s) to it are the only way to access the backing query. When they are invalidated with the delete object statement, you can no longer reach/use the backing query, and I guess it becomes eligible for resource/garbage collection. However, for a statically defined query, apparently you can still access it with the static reference q after any handle(s) referencing it are invalidated/reassigned.

I have less experience with handles in Progress than you. What behavior would you expect from get next q in the above example? Do you think delete object h1 should have invalidated the static query?

Eric Faulhaber wrote:

I guess this makes sense on some level. For a dynamically created query, the handle(s) to it are the only way to access the backing query. When they are invalidated with the delete object statement, you can no longer reach/use the backing query, and I guess it becomes eligible for resource/garbage collection. However, for a statically defined query, apparently you can still access it with the static reference q after any handle(s) referencing it are invalidated/reassigned.

I have less experience with handles in Progress than you. What behavior would you expect from get next q in the above example? Do you think delete object h1 should have invalidated the static query?

Hmm... this might mean 4GL keeps the static resources and the HANDLE's in distinct structures; because, after DELETE OBJECT is called for a handle referring a query, re-accessing the HANDLE attribute for the static query gives us a different handle ID. This is a little tricky to implement in P2J as, until now, the HANDLE is hard linked with the resource (static or not) and deleting a handle means the resource is deleted too. For now, I'll make the static queries undeletable (regardless how the query is accessed) and I'll document this in a note in #2196.

Code Review 1029a

This update is good to go. Nice!

Eric will handle the other update.

I missed something in the QueryWrapper c'tor; I think this code:

      // Register a cleaner.  QueryWrappers are created as instance variables,
      // so we register to the next external scope, which corresponds to a
      // business logic class' execute() method.
      Finalizable cleaner = new Finalizable()
      {
         public void finished() { cleanup(); }
         public void iterate() {}
         public void retry() {}
      };
      TransactionManager.registerNextExternal(cleaner);

Constantin Asofiei wrote:

the following files and folders must exist in the server/ folder (i.e. current folder where the server is started):
- in cfg folder, the matchlist.xml and p2j.cfg.xml files
- in data/namespace folder, the *.dict and *.p2o files for the standard and all the permanent DBs.

This is all static data -- either inputs to or outputs from -- conversion. This requirement is OK temporarily for testing, but I think it would be better from a deployment perspective to load this from the application jar file.

Code review 20131030a:

Wow, brilliant update! Looks really good, only one major concern: the approach of having a separate, full instance of SchemaDictionary in each context. This is OK for a prototype (especially using a small schema, like p2j_test), but I don't think this is feasible for production use with real world (i.e., potentially huge) schemas, due to the memory requirements of the pristineScope data in each SchemaDictionary instance. We need to think about a more granular breakdown of the class in terms of sharing the pristine data in the global scope and managing the (much smaller) dynamic data per context.

• SymbolResolver.locate needs javadoc.
• In newly context-localized classes, when using the same WorkArea instance multiple times in the same method, please cache it and only call locate once. Each call requires a ThreadLocal lookup and multiple hash map lookups in SecurityManager. Not super expensive, but why do it more often than necessary?
• No header entry in UniqueIdGenerator.

Please check it in and distribute, since it already has passed regression testing, but we'll need to figure out some answers for another pass...

Let's create new tasks for the open issues:

• Conversion artifacts moved into and read from the application jar.
• Multiple copies of the SchemaDictionary condensed to a single copy with diffs done on a context-local basis.
• Anything else?

These tasks can be worked in M11.

Eric Faulhaber wrote:

... due to the memory requirements of the pristineScope data in each SchemaDictionary instance.

I think the DynamicQueryHelper.loadPermanentSchemas (which copy the global .dict and .p2o ASTs to each context's AST storage) plus the SchemaDictionar.database map might pose memory problems on the long term (as it copies the same ASTs multiple times).

About the SchemaDictionary.scopes. This is actually a stack, so I'm thinking: why not create a custom stack where the bottom of the stack (scope 0) is shared by all contexts and the other stack levels are context-local? I think this will be OK, as during runtime conversion, scope level 0 should be read-only (no one should add data to it). If it happens, it means that the pristine scope was not initialized properly.

Some other issues, all minor:

I'll fix these tomorrow (as I need to double-check note 66 too).

Let's create new tasks for the open issues:

• Anything else?

There are the GET NEXT/PREV/etc problems for both static and dynamic queries; we will need to fix these at some point. I'll put some examples in a dedicated task.

Greg Shah wrote:

Anything else?

The issues Constantin described in notes 63 & 66 above.

With regard to the AbstractQuery issue in note 63, I'm wondering if it makes sense to refactor P2JQuery and AbstractQuery, since so many of their methods can't be used, depending on whether the query is dynamic or not. Don't have a good idea on this yet, but it seems we have outgrown their initial design...

Eric Faulhaber wrote:

I'm wondering if it makes sense to refactor P2JQuery and AbstractQuery, since so many of their methods can't be used, depending on whether the query is dynamic or not. Don't have a good idea on this yet, but it seems we have outgrown their initial design...

I think we need a QueryResource interface to be implemented only by the QueryWrapper class (in addition of P2jQuery). QueryResource will contain all the QUERY resource APIs (extracted from P2JQuery) and AbstractQuery will have all the APIs defined by the QueryResource removed.

Constantin Asofiei wrote:

Some other issues, all minor:

I'll fix these tomorrow (as I need to double-check note 66 too).

OK, but tomorrow, please check in the version that already has passed regression testing before you make these changes. I am dependent on your annotations work for some work I am doing, and I would like to avoid waiting for another round of regression testing before merging with your code. Also, I want Stanislav to integrate with your update as soon as possible. The proposed changes shouldn't affect either of us.

Constantin Asofiei wrote:

I think we need a QueryResource interface to be implemented only by the QueryWrapper class (in addition of P2jQuery). QueryResource will contain all the QUERY resource APIs (extracted from P2JQuery) and AbstractQuery will have all the APIs defined by the QueryResource removed.

That sounds good. While we're at it, we should change P2JQuery to Query (or to Queryable, if Query conflicts with Hibernate's version), to address a long-standing complaint of Greg's.

But this refactoring can wait for M11.

I've found a minor bug: get-next returns yes even if there are no more results. Testcase:

def temp-table tt no-undo field f1 as integer field f2 as integer.
def var q as handle.

create query q.
q:set-buffers(BUFFER tt:HANDLE).
q:query-prepare("for each tt").
q:query-open.
MESSAGE STRING(q:get-next).

P2J output: yes, 4GL output: no.
Note that this testcase is not directly converted since legacy temp table names are not supported yet.
I'm not sure about the cause, but it may be about lenientOffEnd.

Yet another minor bug. Testcase:

def temp-table tt field f1 as integer.

def var h1 as handle.
def var h2 as handle.
def var q as handle.

h1 = buffer book:handle.
h2 = buffer tt:handle.

create query q.
q:set-buffers(h1, h2).
q:query-prepare("for each book, each tt").
q:query-open.
q:get-next.

Warning:

[11/14/2013 22:37:35 TMT] (com.goldencode.p2j.persist.Persistence$Context:WARNING) [00000001:00000007:syman-->local/p2j_test/primary] rolling back open transaction during context cleanup

Variation of the bug above. Testcase:

def temp-table tt0 field f1 as integer.
def temp-table tt field f1 as integer.

def var h1 as handle.
def var h2 as handle.
def var q as handle.

h1 = buffer tt0:handle.
h2 = buffer tt:handle.

create query q.
q:set-buffers(h1, h2).
q:query-prepare("for each tt0, each tt").
q:query-open.
q:get-next.

Stacktrace:

[11/14/2013 23:04:29 TMT] (com.goldencode.p2j.persist.Persistence$Context:WARNING) [00000001:00000006:syman-->local/_temp/primary] rolling back open transaction during context cleanup
[11/14/2013 23:04:29 TMT] (com.goldencode.p2j.persist.Persistence$Context:WARNING) [00000001:00000006:syman-->local/_temp/primary] rolling back orphaned transaction
[11/14/2013 23:04:29 TMT] (com.goldencode.p2j.persist.Persistence$Context:SEVERE) Error closing Hibernate session during context cleanup
com.goldencode.p2j.persist.PersistenceException: [00000001:00000006:syman-->local/_temp/primary] unable to rollback current transaction
        at com.goldencode.p2j.persist.Persistence$Context.rollback(Persistence.java:4615)
        at com.goldencode.p2j.persist.Persistence$Context.closeSession(Persistence.java:5095)
        at com.goldencode.p2j.persist.Persistence$Context.rollback(Persistence.java:4613)
        at com.goldencode.p2j.persist.Persistence$Context.cleanup(Persistence.java:5341)
        at com.goldencode.p2j.persist.Persistence$1.cleanup(Persistence.java:618)
        at com.goldencode.p2j.persist.Persistence$1.cleanup(Persistence.java:616)
        at com.goldencode.p2j.security.ContextLocal$Wrapper.cleanup(ContextLocal.java:381)
        at com.goldencode.p2j.security.SecurityContext.cleanup(SecurityContext.java:498)
        at com.goldencode.p2j.security.SecurityManager.endContext(SecurityManager.java:6772)
        at com.goldencode.p2j.security.SecurityManager.popContextWorker(SecurityManager.java:6710)
        at com.goldencode.p2j.security.SecurityManager.popAndRestoreSecurityContext(SecurityManager.java:3933)
        at com.goldencode.p2j.net.RouterSessionManager.restoreContext(RouterSessionManager.java:1065)
        at com.goldencode.p2j.net.Conversation.run(Conversation.java:191)
        at java.lang.Thread.run(Thread.java:722)
Caused by: org.hibernate.TransactionException: rollback failed
        at org.hibernate.engine.transaction.spi.AbstractTransactionImpl.rollback(AbstractTransactionImpl.java:215)
        at com.goldencode.p2j.persist.Persistence$Context.rollback(Persistence.java:4602)
        at com.goldencode.p2j.persist.Persistence$Context.closeSession(Persistence.java:5095)
        at com.goldencode.p2j.persist.Persistence$Context.rollback(Persistence.java:4613)
        at com.goldencode.p2j.persist.Persistence$Context.cleanup(Persistence.java:5341)
        at com.goldencode.p2j.persist.Persistence$1.cleanup(Persistence.java:618)
        at com.goldencode.p2j.persist.Persistence$1.cleanup(Persistence.java:616)
        at com.goldencode.p2j.security.ContextLocal$Wrapper.cleanup(ContextLocal.java:381)
        at com.goldencode.p2j.security.SecurityContext.cleanup(SecurityContext.java:498)
        at com.goldencode.p2j.security.SecurityManager.endContext(SecurityManager.java:6772)
        at com.goldencode.p2j.security.SecurityManager.popContextWorker(SecurityManager.java:6710)
        at com.goldencode.p2j.security.SecurityManager.popAndRestoreSecurityContext(SecurityManager.java:3933)
        at com.goldencode.p2j.net.RouterSessionManager.restoreContext(RouterSessionManager.java:1065)
        at com.goldencode.p2j.net.Conversation.run(Conversation.java:191)
        at java.lang.Thread.run(Thread.java:722)
Caused by: org.hibernate.TransactionException: unable to rollback against JDBC connection
        at org.hibernate.engine.transaction.internal.jdbc.JdbcTransaction.doRollback(JdbcTransaction.java:167)
        at org.hibernate.engine.transaction.spi.AbstractTransactionImpl.rollback(AbstractTransactionImpl.java:209)
        at com.goldencode.p2j.persist.Persistence$Context.rollback(Persistence.java:4602)
        at com.goldencode.p2j.persist.Persistence$Context.closeSession(Persistence.java:5095)
        at com.goldencode.p2j.persist.Persistence$Context.rollback(Persistence.java:4613)
        at com.goldencode.p2j.persist.Persistence$Context.cleanup(Persistence.java:5341)
        at com.goldencode.p2j.persist.Persistence$1.cleanup(Persistence.java:618)
        at com.goldencode.p2j.persist.Persistence$1.cleanup(Persistence.java:616)
        at com.goldencode.p2j.security.ContextLocal$Wrapper.cleanup(ContextLocal.java:381)
        at com.goldencode.p2j.security.SecurityContext.cleanup(SecurityContext.java:498)
        at com.goldencode.p2j.security.SecurityManager.endContext(SecurityManager.java:6772)
        at com.goldencode.p2j.security.SecurityManager.popContextWorker(SecurityManager.java:6710)
        at com.goldencode.p2j.security.SecurityManager.popAndRestoreSecurityContext(SecurityManager.java:3933)
        at com.goldencode.p2j.net.RouterSessionManager.restoreContext(RouterSessionManager.java:1065)
        at com.goldencode.p2j.net.Conversation.run(Conversation.java:191)
        at java.lang.Thread.run(Thread.java:722)
Caused by: org.h2.jdbc.JdbcSQLException: The object is already closed [90007-169]
        at org.h2.message.DbException.getJdbcSQLException(DbException.java:329)
        at org.h2.message.DbException.get(DbException.java:169)
        at org.h2.message.DbException.get(DbException.java:146)
        at org.h2.message.DbException.get(DbException.java:135)
        at org.h2.jdbc.JdbcConnection.checkClosed(JdbcConnection.java:1386)
        at org.h2.jdbc.JdbcConnection.checkClosedForWrite(JdbcConnection.java:1374)
        at org.h2.jdbc.JdbcConnection.rollback(JdbcConnection.java:460)
        at org.hibernate.engine.transaction.internal.jdbc.JdbcTransaction.doRollback(JdbcTransaction.java:163)
        at org.hibernate.engine.transaction.spi.AbstractTransactionImpl.rollback(AbstractTransactionImpl.java:209)
        at com.goldencode.p2j.persist.Persistence$Context.rollback(Persistence.java:4602)
        at com.goldencode.p2j.persist.Persistence$Context.closeSession(Persistence.java:5095)
        at com.goldencode.p2j.persist.Persistence$Context.rollback(Persistence.java:4613)
        at com.goldencode.p2j.persist.Persistence$Context.cleanup(Persistence.java:5341)
        at com.goldencode.p2j.persist.Persistence$1.cleanup(Persistence.java:618)
        at com.goldencode.p2j.persist.Persistence$1.cleanup(Persistence.java:616)
        at com.goldencode.p2j.security.ContextLocal$Wrapper.cleanup(ContextLocal.java:381)
        at com.goldencode.p2j.security.SecurityContext.cleanup(SecurityContext.java:498)
        at com.goldencode.p2j.security.SecurityManager.endContext(SecurityManager.java:6772)
        at com.goldencode.p2j.security.SecurityManager.popContextWorker(SecurityManager.java:6710)
        at com.goldencode.p2j.security.SecurityManager.popAndRestoreSecurityContext(SecurityManager.java:3933)
        at com.goldencode.p2j.net.RouterSessionManager.restoreContext(RouterSessionManager.java:1065)
        at com.goldencode.p2j.net.Conversation.run(Conversation.java:191)
        at java.lang.Thread.run(Thread.java:722)