FWD » Core Development » Database

This feature will require dynamic (runtime) conversion of the CREATE BUFFER statement.

After cleanup of source I observed that the code is much simple that I first implemented.

I choose to create a separate walk rule instead of altering explicit_buffer_def() function and then to check back the create buffer case. Also, the create buffer is suppose to be dynamic so any additional needed date should be extracted at runtime from statement's parameters.

Code review 20130117b:

I don't think all the extra overloading of the RecordBuffer.createBufferForTable method to support String arguments is necessary. The conversion generally will not produce arguments of type String unless you go out of your way to do so. By default, only character objects are generated. Did you find otherwise in your testing? If not, just the overloaded forms of that method with character parameters are needed. Let's also shorten the name to RecordBuffer.createBuffer, unless there's an important reason to keep the "ForTable" part. Otherwise, looks good.

Right now I'm trying to figure out how a dynamic buffer should be handled in the scopes outer to the point where the buffer was defined up to table definition scope (global for permanent tables). Normally when a buffer is defined we have openScope calls for new inner scopes, so I'm thinking about specificities on how we should emulate these calls.

In terms of scoping, it seems to me that there are only these possibilities:

1. From CREATE BUFFER to the end of the external procedure in which it is executed.
2. From CREATE BUFFER to the end of the top-level block (external procedure, internal procedure, user-defined function or trigger) in which it is executed.
3. From CREATE BUFFER to when the external procedure is unloaded when that procedure was RUN PERSISTENT.
4. From CREATE BUFFER to the end of the session.

I have made Constantin a watcher because he is our expert in persistent procedures. He can inform us about how we support/manage resources that are loaded in an external procedure that is RUN PERSISTENT. Normally, we can register a Finalizable instance at the external procedure scope and will get a call back to clean up. I don't know how we support the 4GL idea of deferring the cleanup of resources created in an external procedure RUN PERSISTENT until that procedure is unloaded.

Stas, while you're in this code, please move all the public static variants of RecordBuffer.create where they belong, further up in the file. I'm making changes in this file as well, but I'm going to leave those methods alone so as not to make more of a merge mess for you.

1. a program which defines some shared handle vars, runs another program and saves the resources created in it, res_test.p:
   

   def new shared var h as handle.
   def new shared var h2 as handle.
   def new shared var h3 as handle.
   
   run create_res.p persistent.
   
   delete object h. /* this will be a no-op, as h's resource is not dynamic */
   
   message h valid-handle(h).
   message h2 valid-handle(h2).
   message h3 valid-handle(h3).
   
   delete object session:last-procedure.
   
   /* all resources are deleted at this point */
   message h valid-handle(h).
   message h2 valid-handle(h2).
   message h3 valid-handle(h3).
   

2. the program which creates the resources, create_res.p:
   

   def temp-table tt1 field f1 as int.
   def shared var h as handle.
   def shared var h2 as handle.
   def shared var h3 as handle.
   
   h2 = buffer tt1:handle.
   h = temp-table tt1:handle.
   
   create buffer h3 for table "tt1".
   

1. P2J converts the h = BUFFER tt1:HANDLE and h = TEMP-tABLE tt1:HANDLE to the same construct, h.assign(tt1). It doesn't differentiate between the TEMP-TABLE and BUFFER resources.
2. If ran non-persistent, the resources (static or dynamic) must be deleted when the program ends.
3. If ran persistent, then (for any resource which supports the DYNAMIC attribute):
• the dynamic resources survive until are explicitly deleted or the persistent procedure is deleted
• the static resources survive until the persistent procedure is deleted
1. resources which do not support the DYNAMIC attribute can be explicitly deleted (and I think only the resources which don't have a static counterpart do not support the DYNAMIC attribute).

1. the DYNAMIC attribute (when supported and set to true) will stop a non-dynamic resource to be explicitly deleted
2. the INSTANTIATING-PROCEDURE is used to link resources with the procedure which created it, and automatically delete them when the procedure is deleted. This will mean that all code which automatically drops a i.e. temp-table when the associated Hibernate session is closed needs to be changed so that it will be aware of the instantiating-procedure's state.

If possible, the implementation of these two attributes and the integration with a DELETE OBJECT res or DELETE OBJECT procedure statement should be made generic, to be able to treat any kind of resource which supports them.

2. From CREATE BUFFER to the end of the top-level block (external procedure, internal procedure, user-defined function or trigger) in which it is executed.

The scope is restrained within a top-level block only when a named or explicit unnamed widget-pool is used (following the rules described in the javadoc for the WidgetPool.java class).

Let's forget for a second about persistent procedures and widget pools. For normal processing, as I said earlier, a dynamic buffer for a persistent table or dynamic temp table is scoped to the session, and a dynamic buffer for a static temp table is scoped to the external procedure in which it was declared. E.g.

---PROC1-------
   def shared handle h.

   ---PROC2-------
   def shared temp-table tt.

      ---PROC3-------
       create buffer h for tt

      ---PROC4-------
      valid-handle(h) -> yes

   valid-handle(h) -> yes

valid-handle(h) -> no

A dynamic buffer can contain either permanent or temp record. For permanent tables we may promote a block to full transaction during conversion if it modifies a permanent record and there is no outer transaction. Luckily no changes are required, dynamic buffers are treated as temp buffers on this part.

In its outermost scope the buffer should be
1. registered as finalizable
2. added to openBuffers and undoList (BTW undoList is a partitioned array which is not friendly to insertions at arbitrary scope) - that is supposed to ensure that commitables and undoables will be properly registered for newly opened scopes.

We have the set of scopes staring from the outermost scope for this buffer and ending with the current scope (all these scopes are opened). Obviously these scopes do not contain information for transaction processing for a new buffer. I don't think we need to expilictly add undoables for these scopes (but we must ensure that a newly created buffer is deleted if its creation is undone), while I think we need to add commitables to all of the scopes.

What do you think about the plan?

Stanislav Lomany wrote:

Determination of the outermost scope for a temp-table is the task for MetaSchema, it cannot be implemented using existing means.

I am a bit confused by this comment. MetaSchema will have schema information, basically the equivalent for temp-tables as what we have for persistent tables. I have not been planning on tracking information specific to temp-tables, such as when a temp-table comes into existence and when it goes out of scope with this mechanism, though I was planning on providing the same information about dynamic temp-tables as for static temp-tables. What did you have in mind here?

I am a bit confused by this comment. MetaSchema will have schema information, basically the equivalent for temp-tables as what we have for persistent tables. I have not been planning on tracking information specific to temp-tables, such as when a temp-table comes into existence and when it goes out of scope with this mechanism, though I was planning on providing the same information about dynamic temp-tables as for static temp-tables. What did you have in mind here?

Is this function supposed to work?

static Class DynamicTablesHelper.getTableDMOInterface(String tableName4GL)
{
   /// TODO implement using MetaSchema. It is assumed that for temp tables in the case of name
   // conflict (there are multiple tables with the same name in different procedures) the table
   // which is reachable from the current scope will be returned.

Sorry I missed that detail in my review. This is a good question, and after speaking with Greg, I am shifting my opinion a bit to put less emphasis on MetaSchema for dynamic runtime state. We would like to integrate the dynamic-temp table state information as much as possible with the SchemaDictionary, since that class and the Progress parser today already do all the work we need with statically-defined temp-tables (just not at runtime, yet). We will need this information to be available in the SchemaDictionary anyway in order for the dynamic conversion of CREATE QUERY to work, so I would rather use that mechanism as a base and improve it as necessary.

OK, please let me know if you see some obvious flaws in the rest of the plan.

• DatabaseInfo
• ADMData
• UniqueID
• NamespaceURI
• Errorable

We would like to integrate the dynamic-temp table state information as much as possible with the SchemaDictionary, since that class and the Progress parser today already do all the work we need with statically-defined temp-tables (just not at runtime, yet).

1. createTempTable(), which will be called when a new temp-table is being created (statically or dynamically). Note sure what data structure to pass to this API: should it be a TempTableBuilder instance? This is not enough currently, because for dynamic queries the temp and permanent tables need to be registered with the P2OLookup too (to be able to resolve the java name of a i.e. legacy field). The P2JField class provides us only the legacy field name; I might be missing something, but we need a mechanism of resolving these (in both ways, a legacy name for a Java name and a Java name for a legacy name). As I understand, the temp-tables will not be mapped by MetaSchema; thus, how do we obtain the legacy structure of a static temp-table? The buffer:TABLE-HANDLE can be used to obtain a TEMP-TABLE handle with the definition for a static temp-table too, not just dynamic; this will be a starting point of accessing the legacy table definition, but the mapping to converted names is missing.
   The same applies for the permanent tables - we need to determine the java name for a legacy name... will this be provided by MetaSchema APIs? And for a P2J BUFFER instance, we will need to provide an access to the definition of a permanent table; we can use the definitions provided by the Hibernate Configuration classes, if we have a java-to-progress mapping for the names.
2. removeTempTable(String legacyName), when an existing temp-table is being removed (implicitly for static temp-tables or when a dynamic temp-table goes out of scope, or explicitly when a dynamic temp-table is being deleted by the user).

Beside this, SchemaDictionary must implement Scopeable too, to proper track the temp-tables active for a certain scope.

I'll let Eric respond on the MetaSchema questions.

Beside this, SchemaDictionary must implement Scopeable too, to proper track the temp-tables active for a certain scope.

I prefer if another class implements Scopeable and then calls the proper interfaces in SchemaDictionary to maintain the state. This is effectively how we do it during parsing (the parser calls SymbolResolver interfaces when certain statements/blocks are entered/exited and SymbolResolver maintains the state in the SchemaDictionary.

If this is reasonable, then it will help reduce the number of changes to SchemaDictionary.

Constantin Asofiei wrote:

As I understand, the temp-tables will not be mapped by MetaSchema; thus, how do we obtain the legacy structure of a static temp-table?
[...]
The same applies for the permanent tables - we need to determine the java name for a legacy name... will this be provided by MetaSchema APIs?

The idea behind the MetaSchema interface is to provide a convenient, internal API to the data in the _file and _field metadata tables (or, more accurately the subset of that information as summarized in the first few entries in #2115). We had to support these tables for direct use by converted code, but it turns out that many of the database-related attributes need these values as well, hence the idea of the MetaSchema interface as an internal-use API. In addition, I have added a _new-name field to our implementation of both _file and _field (and played some conversion games so legacy code does not see them), so that we have a two-way mapping of the converted DMO and property names. MetaSchema will give you access to this lookup in both directions.

Now, Progress 4GL does not store temp-table data in these metadata tables, but we have the flexibility to do so, if it is useful. I would prefer not to change the schema of the metadata database, since it will be much easier to add this support and maintain it, if it is identical to the schema behind the permanent version.

MetaSchema will give you access to this lookup in both directions.

Thanks, this is good news.

Now, Progress 4GL does not store temp-table data in these metadata tables, but we have the flexibility to do so, if it is useful. I would prefer not to change the schema of the metadata database, since it will be much easier to add this support and maintain it, if it is identical to the schema behind the permanent version.

I don't think _file can be easily used for temp-tables, as I won't be able to do a "select all active temp-tables from _file", without changing the schema: at a certain scope some temp-tables might be active and others hidden. More, the same temp-table name may be used by different scopes, but point to two different temp-tables (this will conflict with the primary index on _file:_file-Name). I'm thinking if it would not be easier using annotations at the DMO interface definition; from these, we could build some scopeable maps for the progress-to-java and java-to-progress cases and maintain them as the temp-tables are created/removed.

Constantin Asofiei wrote:

I don't think _file can be easily used for temp-tables, as I won't be able to do a "select all active temp-tables from _file", without changing the schema: at a certain scope some temp-tables might be active and others hidden. More, the same temp-table name may be used by different scopes, but point to two different temp-tables (this will conflict with the primary index on _file:_file-Name).

Yes, this makes sense. I think we would have the same problem using this API in support of attributes, so I will need to rethink how to deal with attributes for temp-table buffers.

I'm thinking if it would not be easier using annotations at the DMO interface definition; from these, we could build some scopeable maps for the progress-to-java and java-to-progress cases and maintain them as the temp-tables are created/removed.

Please elaborate on this plan, specifically, how the mechanics of the annotations would work for this purpose. We would of course need to add special conversion support to emit annotations in DMO interfaces.

I'm thinking if it would not be easier using annotations at the DMO interface definition; from these, we could build some scopeable maps for the progress-to-java and java-to-progress cases and maintain them as the temp-tables are created/removed.

Please elaborate on this plan, specifically, how the mechanics of the annotations would work for this purpose. We would of course need to add special conversion support to emit annotations in DMO interfaces.

On a second thought, the best place would be the DMO implementation class, not interface, as we will annotate the actual fields (at the DMO interface, we would have to annotate the getters/setters, and this is a bit ugly IMO). The LegacyTable annotation would look like this:

/**
 * Annotation definition to mark a DMO implementation class with its legacy table name.
 */
@Target(ElementType.TYPE)
@Retention(RetentionPolicy.RUNTIME)
public @interface LegacyTable
{
   /** The name of the legacy table associated with the DMO implementation class. */
   String name();
}

/**
 * Annotation definition to mark a DMO property with its legacy field name.
 */
@Target(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
public @interface LegacyTable
{
   /** The name of the legacy field associated with a DMO property. */
   String name();

   /** Mark legacy fields which are converted to more than one DMO property. */
   boolean compound() default false;
}

@LegacyTable(name = "tt1")
public class TempRecord1Impl
implements Serializable,
           Persistable,
           TempRecord1
{
   ...

   @LegacyField(name = "f1")
   private final integer f1;

   ...

   static class Composite2
   implements Serializable
   {
      @LegacyField(name = "f2")
      private final integer f2;
      ...
   }
}

Interesting idea. This would obviate the need for the _new-name field in _file and _field. What would the code look like which would use these annotations at runtime? I assume this would be in a helper class in persist (or a sub-package)?

BTW, I have been thinking about annotating the DMOs anyway, to get rid of the separate Hibernate mapping documents, but there are other implications to that effort, since we massage these at server startup. Point is, a mechanism to use annotations in converted code is interesting on more than one level.

What would the code look like which would use these annotations at runtime?

• the mappings for the permanent DMOs are initiated at server startup; they are read-only and will never change once built
• the mappings for temp-tables are scope-aware
• the mappings for temp-tables are added/removed as the temp-tables are created/deleted (and this will work with both static and dynamic temp-tables)

• TableMapper.legacyName(boolean temporary, {Class dmoIface | String dmoIfcName}) to obtain the legacy table name for a DMO interface class or interface name.
• TableMapper.legacyFieldName(boolean temporary, {Class dmoIface | String dmoIfcName}, String property) to obtain the legacy field name for a DMO property.
• TableMapper.javaName(boolean temporary, String table) to obtain the DMO interface name for a legacy table name
• TableMapper.javaFieldName(boolean temporary, String table, String field) to obtain the DMO property name for a legacy field name.

Also (for dynamic query purpose), we will need the legacy name for the buffer names (both implicit, explicit buffers via DEFINE BUFFER and dynamic buffers). This should be kept at the BufferImpl instance I think, because RecordBuffer.variable holds something Java-specific, and not the legacy buffer name. BUFFER buf:HANDLE can be used to obtain the BUFFER resource for an existing buffer, and for this we need some conversion changes to emit the legacy buffer name at the RecordBuffer.define calls.

I agree with this proposed approach. One minor change is that I think we need to store the legacy buffer name in RecordBuffer instead of BufferImpl, because ultimately we will need to correct a long-standing inconsistency where we use the converted buffer name in error messages instead of the legacy name. BufferImpl can obtain this information from its backing RecordBuffer and just act as a pass-through.

One minor change is that I think we need to store the legacy buffer name in RecordBuffer instead of BufferImpl.

Eric, I can let Stanislav implement this, as I see that NAME attribute can be used to retreive the legacy buffer name. For my usage, I can just assume that res.getName() will return the legacy buffer name, if res is a BufferImpl instance.

OK.

One more request: for the API, let's use getDMOName instead of javaName and getPropertyName instead of javaFieldName. This nomenclature is more precise and is more consistent with the existing code in the persistence framework.

• Program 1 defines a temp-table and exposes its default buffer and the temp-table via shared handle vars.
  

  def temp-table tt1 field f1 as int.
  
  def new shared var ht as handle.
  def new shared var hb as handle.
  
  ht = temp-table tt1:handle.
  hb = buffer tt1:handle.
  
  run p2.p.
  

• when calling program 2, there are two visible temp-tables named tt1: one from program 1 and the other from program 2:
  

  def temp-table tt1 field f2 as int.
  
  def shared var ht as handle.
  def shared var hb as handle.
  
  /* following two messages show same name, but different unique-id's */
  message "ht:" ht:name ht:unique-id.
  message "tt1:" temp-table tt1:handle:name temp-table tt1:handle:unique-id.
  
  def var hq as handle.
  
  create query hq.
  hq:add-buffer(buffer tt1:handle).
  hq:add-buffer(hb).
  hq:query-prepare("for each tt1, each tt1").
  

  
  The buffers can end up being used in a dynamic query, even if the name collides. This is absurd, as the add-buffer should have not added a buffer with the same name; the failure in 4GL is at the query-prepare - this wants each of the two buffers to be referenced, but if two clauses are added, their name will conflict and 4GL sees only one of them.

All this translates into: the mappings can't use the legacy temp-table name as the key; instead, the UNIQUE-ID value must be used. And to be able to use the UNIQUE-ID for static temp-tables, P2J needs the TempTable resource to be implemented for static temp-tables too.

Stanislav Lomany wrote:

In its outermost scope the buffer should be
1. registered as finalizable
2. added to openBuffers and undoList (BTW undoList is a partitioned array which is not friendly to insertions at arbitrary scope) - that is supposed to ensure that commitables and undoables will be properly registered for newly opened scopes.

We have the set of scopes staring from the outermost scope for this buffer and ending with the current scope (all these scopes are opened). Obviously these scopes do not contain information for transaction processing for a new buffer. I don't think we need to expilictly add undoables for these scopes (but we must ensure that a newly created buffer is deleted if its creation is undone), while I think we need to add commitables to all of the scopes.

If a dynamic buffer is created in an inner scope, and then used to change data in an arbitrary, "middle" scope (i.e., between the outermost scope determined by the buffer's lifespan and the innermost scope in which it is created), don't we need undo information for that buffer in all the middle scopes, too? Is it valid for a buffer to be created in an inner scope and then used to change data in an older/outer scope? Doesn't that mean you need to make inserts to the undoList at all the scopes valid for that buffer's life?

Is it just the outermost scope at which the buffer needs to be registered, or also all the scopes between the outermost and the innermost one where the buffer was created, since we don't know at which scope a change will happen? [LE: just realized after lots of editing, that this question was redundant]

What is the plan to deal with the partitioned array problem?

Stanislav, while you are working on dynamic buffers, I think is a good opportunity to rename get and set methods accessible via the Buffer interface, to not collide with a potential DMO getter/setter. These interfaces have APIs which need to be renamed:

• DatabaseInfo
• ADMData
• UniqueID
• NamespaceURI
• Errorable

Constantin, how the new names should look like?

Some feedback on ca_upd20131015b.zip:
1. Temp tables make my test server fail:

aused by: java.lang.NullPointerException
        at com.goldencode.p2j.persist.TemporaryBuffer.tempTableResource(TemporaryBuffer.java:2637)
        at com.goldencode.p2j.persist.TemporaryBuffer.openScope(TemporaryBuffer.java:1983)
        at com.goldencode.p2j.persist.RecordBuffer.openScope(RecordBuffer.java:2010)
        at com.goldencode.testcases.Go$1.body(Go.java:28)
        at com.goldencode.p2j.util.BlockManager.processBody(BlockManager.java:6939)
        at com.goldencode.p2j.util.BlockManager.topLevelBlock(BlockManager.java:6846)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:213)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:195)
        at com.goldencode.testcases.Go.execute(Go.java:23)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:601)
        at com.goldencode.p2j.util.Utils.invoke(Utils.java:1154)
        at com.goldencode.p2j.main.StandardServer$MainInvoker.execute(StandardServer.java:1555)
        at com.goldencode.p2j.main.StandardServer.invoke(StandardServer.java:1061)
        at com.goldencode.p2j.main.StandardServer.standardEntry(StandardServer.java:342)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:601)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:687)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:316)
        at com.goldencode.p2j.net.Conversation.run(Conversation.java:158)
        at java.lang.Thread.run(Thread.java:722)

2. Letter case should be preserved in annotated table and field names. E.g. testcase:

def temp-table someTable field someField as integer.
def var h as handle.
create temp-table h.
h:add-like-index("", "fail", "sometable").

Error message:

ADD-LIKE-INDEX could not find index fail in table someTable. (9059)

And if we want to be completely perfect, we should convert temp tables which have the names in different cases to different classes.

PS: the NPE is fixed, but I need to finish the annotations for the hand-written DMOs in MAJIC. Will post this later today.

Remove the get, set or is prefixes (so there is no way to conflict with a DMO getter or setter).

And if we have both get and set for the same attribute?

there is a great chance that lots of temp-tables would no longer match, once we add case-sensitivity.

I don't think there is much playing with case, I suppose that the most of variety will be "tt" and "TT".

Stanislav Lomany wrote:

Remove the get, set or is prefixes (so there is no way to conflict with a DMO getter or setter).

And if we have both get and set for the same attribute?

The setter will have parameters, so there will be no conflict. We will have i.e. logical error() as getter and void error(logical) as setter. This is a downside of the fact that the i.e. TempRecord1.Buf interface extends both Buffer and the DMO interface.

Now I even can't start server:

Caused by: java.lang.NullPointerException: Property id for DMO com.goldencode.testcases.dmo.p2j_test.impl.AddressImpl must have a LegacyField annotation!
    at com.goldencode.p2j.persist.TableMapper$LegacyTableInfo.loadFields(TableMapper.java:669)
    at com.goldencode.p2j.persist.TableMapper$LegacyTableInfo.<init>(TableMapper.java:641)
    at com.goldencode.p2j.persist.TableMapper.mapClass(TableMapper.java:460)
    at com.goldencode.p2j.persist.TableMapper$PermanentTableMapper.mapClass(TableMapper.java:565)
    at com.goldencode.p2j.persist.TableMapper.mapPermanentDMO(TableMapper.java:326)
    at com.goldencode.p2j.persist.ORMHandler.mapClass(ORMHandler.java:223)
    at com.goldencode.p2j.persist.DatabaseManager.registerDatabase(DatabaseManager.java:2100)
    at com.goldencode.p2j.persist.DatabaseManager.initialize(DatabaseManager.java:1194)
    at com.goldencode.p2j.persist.Persistence.initialize(Persistence.java:1069)
    at com.goldencode.p2j.main.StandardServer$5.initialize(StandardServer.java:862)
    at com.goldencode.p2j.main.StandardServer.hookInitialize(StandardServer.java:1438)
    ... 5 more

There are massive changes in TableMapper. Is that right table mapper?

Now I even can't start server:

Please comment line 223 in ORMHandler (to disable mappings for permanent DBs) until I fix this and add annotations to the hand-written DMOs too.

There are massive changes in TableMapper. Is that right table mapper?

Yes, but the update is not yet ready to be officially released.

Stanislav, some notes on TempTableBuilder getters for various attributes; i.e. TempTableBuilder.uniqueId is defined as an integer, but getUniqueId() does not return a copy of the uniqueId field, it returns the actual reference. This can pose problems, because if the caller saves the reference returned by getUniqueId, it will change the TempTableBuilder.uniqueId too. Is best to make sure the actual reference remains private, else it might hurt us in the future.

And if we want to be completely perfect, we should convert temp tables which have the names in different cases to different classes.

Is there any way we can annotate this in the code that creates a temp-table buffer? That is where the difference matters and it is where the difference is known at conversion time.

I prefer to keep a common/shared temp-table class where it is structurally possible, as this is a much better result. But I do want to be absolutely compatible in messages, so this is a worthy goal.

If a dynamic buffer is created in an inner scope, and then used to change data in an arbitrary, "middle" scope (i.e., between the outermost scope determined by the buffer's lifespan and the innermost scope in which it is created), don't we need undo information for that buffer in all the middle scopes, too? Is it valid for a buffer to be created in an inner scope and then used to change data in an older/outer scope? Doesn't that mean you need to make inserts to the undoList at all the scopes valid for that buffer's life?

As far as I understand, a buffer is registered single time at its outermost scope in the undoList, so when we discussing undo process in the middle scopes, we are talking about undoables (which are generated for a new scope basing on the undoList).
How undoables work? When a scope is rolled back, the old value of the buffer is copied over the new one. For all middle scopes we have no previous value for the buffer, so our job is to 1. ensure that the handle value is correct after rollback 2. deregister just created buffer. I think I don't need a buffer's undoable for that job and, eventually, practical tests will put everything in its place.

What is the plan to deal with the partitioned array problem?

Sorry, I messed up, PartitionedArray is an array of DynamicArray s, so no problem here.

Greg,

I prefer to keep a common/shared temp-table class where it is structurally possible, as this is a much better result. But I do want to be absolutely compatible in messages, so this is a worthy goal.

I don't think there is another way other than making the temp-table names/fields case-sensitive; note that the default buffer and the backing temp-table are two different resources in 4GL, so a temp-table can be the backend for more than one buffer. I don't see any other place where we could put these annotations, other than the DMO implementation class for the temp-table.

There must be a place in the generated business logic where we access the DMO implementation class for the temp-table. That is the place to store the name. The issue is that we get major benefits from sharing the DMO implementation class for the temp-table across many different procedures. In fact, it may even be necessary that the same class is used in the case where it is a shared temp-table. I don't want the name to cause us to have multiple DMO implementation classes where we used to have one.

Greg Shah wrote:

There must be a place in the generated business logic where we access the DMO implementation class for the temp-table. That is the place to store the name.

/* program 1:*/
def new shared temp-table TT1 field f1 as int.

create tt1.
tt1.f1 = 10.

run p2.p

/* program 2*/
def shared temp-table tt1 field f2 as int.

for each tt1:
display tt1.f2. /* "f2" is used as the label */
end.

/* still in p2.p */
def var hq as handle.
create query hq.
hq:add-buffer(buffer tt1:handle).
hq:query-prepare("for each tt1 where tt1.f1 > 10"). /* tt1.f2 is not allowed here */
hq:query-open().
hq:get-next().
message tt1.f2.

/* still in p2 */
message temp-table tt1:name. /* shows "TT1", not the name at the shared temp-table definition. */

<?xml version="1.0"?>
<xsd:schema xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns="" 
   xmlns:prodata="urn:schemas-progress-com:xml-prodata:0001">
   <xsd:element name="TT1" prodata:proTempTable="true" prodata:undo="true">
      <xsd:complexType>
         <xsd:sequence>
            <xsd:element name="TT1Row" minOccurs="0" maxOccurs="unbounded">
               <xsd:complexType>
                  <xsd:sequence>
                     <xsd:element name="f1" type="xsd:int" nillable="true" />
                  </xsd:sequence>
               </xsd:complexType>
            </xsd:element>
         </xsd:sequence>
      </xsd:complexType>
   </xsd:element>
</xsd:schema>

• the same table name (from the master definition) is used throughout the error messages, even for the shared temp-table.
• for validation errors, the temp-tables do no display the field name. Only errors related to permanent tables show field info (couldn't find a way to show an error message with a field name for a temp-table).
• the most important: when validation errors are encountered, my testing showed that a message like ** Book already exists with Book ID 10. (132) is shown for permanent tables. Thus, the legacy field name is not used, what it uses is the field's label. And, if a field doesn't have a label, then nothing is shown related to the field.

1. case-sensitivity problem is no longer an issue for fields, as legacy field names are not used in error messages
2. for temp-tables, we can emit the legacy table name at the TemporaryBuffer.define calls

Please let me know if you see something wrong in my reasoning above (and an initial review for ca_upd20131016b.zip would be great).

LE: actually, for parsing the prepare query string, I need the legacy buffer name, not the legacy temp-table name. And in the error messages, I think the buffer name is used, not the temp-table name (the fact that the default buffer has the same name as the temp-table is just a "coincidence"). Thus, the TemporaryBuffer.define for a DEFINE TEMP-TABLE statement needs the legacy temp-table name (as this is the the default buffer name too) and the explicit buffer name is emitted only when a DEFINE BUFFER is used.

Constantin Asofiei wrote:

Please let me know if you see something wrong in my reasoning above (and an initial review for ca_upd20131016b.zip would be great).

This makes sense to me (given also your later entry). I will review this today.

Code review 20131016b:

This update looks quite good, but I have a few questions:

• It's been a long time since I was in this code, but shouldn't the call to TableMapper.removeTemporaryTables in TemporaryBuffer.Context.doDropTable be in TemporaryBuffer.Context.dropAllTables instead?
• It was hard to tell what the converted DMO code will look like just from a review of brew.xml. Could you please post a sample here?

• It's been a long time since I was in this code, but shouldn't the call to TableMapper.removeTemporaryTables in TemporaryBuffer.Context.doDropTable be in TemporaryBuffer.Context.dropAllTables instead?

Thanks, you are correct; I meant to place it in dropAllTables but somehow ended up in doDropTable.

• It was hard to tell what the converted DMO code will look like just from a review of brew.xml. Could you please post a sample here?

This is from a converted DEFINE TEMP-TABLE tt1 FIELD f1 AS INT FIELD f2 AS INT EXTENT 2:

/** Data Model Object corresponding with a temporary table. */
@LegacyTable(name = "tt1")
public class TempRecord1Impl
implements Serializable,
           Persistable,
           TempRecord1
{
   /** Multiplex ID */
   private Integer _multiplex = null;

   /** Surrogate primary key */
   private Long id = null;

   /** Data member */
   @LegacyField(name = "f1")
   private final integer f1;

   /**
    * List of composite properties; 2 elements of type {@link
    * TempRecord1Impl.Composite2 Composite2}
    */
   private List composite2 = new ArrayList(2);

   /**
    * Inner class which composites properties of like extent. All properties
    * migrated from the legacy table _temp.tt1 of extent 2 are
    * data members of this class.
    */
   static class Composite2
   implements Serializable
   {
      /** Data member */
      @LegacyField(name = "f2")
      private final integer f2;
   }
}

The code in brew.xml is generic, so multiple annotation elements would be emitted like:

      @LegacyField(name = "f2", label = "f2")
      private final integer f2;

      @LegacyField
      private final integer f2;

Second run of CTRL-C testing for 1018a.zip was OK.

This update looks fine to me. Please go ahead and commit/distribute it.

Stanislav, if you've managed to implement BUFFER's NAME and TABLE-HANDLE attributes, please extract these changes (in a separate update (if it's possible) and post them here.

Stanislav, if you've managed to implement BUFFER's NAME and TABLE-HANDLE attributes, please extract these changes (in a separate update (if it's possible) and post them here.

I haven't done them yet, but I'll manage them in the first place.

Constantin, I didn't think of this earlier when we were discussing all the new annotations for the DMOs: are we going to have trouble with com.goldencode.proxy? We don't support annotations in that library. Do we need to? AFAICT, you don't need to do any annotation processing directly with the proxies, just the DMO interfaces and implementation classes, correct?

Eric Faulhaber wrote:

Constantin, I didn't think of this earlier when we were discussing all the new annotations for the DMOs: are we going to have trouble with com.goldencode.proxy? We don't support annotations in that library. Do we need to? AFAICT, you don't need to do any annotation processing directly with the proxies, just the DMO interfaces and implementation classes, correct?

Annotations (legacy table/field name, legacy index names) are done ONLY at the DMO implementation class. So proxies are not involved at all.

Constantin, as you have noticed, TableMapper impelementation depends on getUniqueId() and getPersistentClass().getMappedClass() methods of a TempTable object. However I think it is more appropriate to call TempTable interface DynamicTempTable as it contains add*, create* and other methods specific for dynamic temp tables. So I suggest to rename TempTable to DynamicTempTable and create new interface TempTable which will contain methods common to static and dynamic tables, for a start it will be getUniqueId and getDMOClass. And I'll be able to create objects associated with static temp tables which will keep its unique ids and implement TempTable interface.

I don't really like this, because a TEMP-TABLE resource in 4GL exists for both static and dynamic temp-tables. Although a static temp-table can't have its definition altered, attributes/methods can be called on its TEMP-TABLE resource, other than UNIQUE-ID; not allowing a static temp-table resource to call a method which would show an error (i.e. add*, create*) will not allow us to properly implement the error messages.

Instead, why not extract the attrs/methods available for both static and dynamic temp-tables in an abstract class and let the TempTableBuilder specialize in dynamic temp-tables and another class specialize in static temp-tables.

Constantin,

Can you post (at the task) the rules you've found relating to how scoping works for dynamic buffers

Dynamic buffers for permanent tables and dynamic tables have global scope (unless they or parent dynamic table has been explicitly deleted). Dynamic buffers for temp tables has the same scope as the parent temp table.

and what changes you expect to make in RecordBuffer.finished?

I haven't planed any changes.

Stanislav (from email):

Heh, BUFFER:NAME is an attribute of a static or dynamic buffer. I don't mind to implement it, but I haven't supposed that it is included in #1654. It first appears when Constantin notes that NAME and TABLE-HANDLE should be implemented in order to make dynamic queries work and I missed the point that NAME is actually a legacy name. I guess we can add legacy name parameter to RecordBuffer.define unless we want to use annotations (we can discuss that in Redmine).

The solution is to emit the legacy buffer name at i.e. the Record/TemporaryBuffer.define calls. Annotations will be of no use, as a certain DMO may have multiple buffers associated with it (one implicit and zero or more explicit buffers), each with their unique name.

FYI, the original version of dynamic queries do not work with dynamic buffers because of the following: consider the buffer named "some-buffer" and the dynamic query predicate "for each some-buffer where some-buffer.f1 = 2". Static buffer named "some-buffer" will be converted to "someBuffer", and the dynamic queries code (which uses the same conversion code) will convert the predicate to "for each someBuffer where someBuffer.f1 = 2".
But dynamic buffers use unique buffer naming, so the Java buffer name will be "someBuffer__1" and we have buffer name mismatch with the converted predicate. Unique naming for dynamic buffers is a valid trick since Java names do not match legacy names anyway and P2J persistence is heavily based on referencing buffers by their unique names, so I didn't want to switch to unique IDs.
As the solution I will have to modify conversion for dynamic queries. Modifying AST after conversion or in some middle conversion stage is tricky since buffer name can be referenced in various forms. So I'm going to put "forceBufferName" annotation to dynamic buffer nodes on the initial AST and add handling of this annotation to conversion rules.

As the solution I will have to modify conversion for dynamic queries. Modifying AST after conversion or in some middle conversion stage is tricky since buffer name can be referenced in various forms. So I'm going to put "forceBufferName" annotation to dynamic buffer nodes on the initial AST and add handling of this annotation to conversion rules.

OK, I think I understand your problem for dynamic buffers: is at the DMO variable name (aka DMO alias) in the HQL, right? And your problem is that the HQL alias identifying a dynamic buffer follows some special internal P2J rules (thus generated HQL code ends up referencing an incorrect DMO alias). In this case, I think there is a better name for the annotation: forceDMOAlias suggets that the DMO alias will be forced, not the buffer name (which might lead you to think of a legacy buffer name).

todo list for this task:
1. fix dynamic tables + dynamic queries (WIP)
2. fix scoping for dynamic buffers for static temp tables
3. check undo of just created buffer
4. check scoping for buffer parameters (if they actually work in P2J)
5. check undo in constraints violations case

not directly related:
6. fix memory leak in dynamic buffers for dynamic tables map
7. implement todos for dynamic tables where meta schema should be used
8. re-run all testcases for dynamic tables.

Guys, in order to make Java compiler work for dynamic queries + dynamic tables, I've done the following:
1. InMemoryClassLoader stores InMemoryClass objects along with Class objects.
2. InMemoryFileManager implements

public Iterable<JavaFileObject> list(Location location, String packageName, Set<JavaFileObject.Kind> kinds, boolean recurse)

which in our case returns InMemoryClass instances for a specific package. recurse parameter is false in our case.
If I implement a generic solution I need to keep InMemoryClass objects in a tree-like package-based structure. However for this task we can live with one or two packages and I don't know if we will need this facility for some compilation later, so we can live with a simple map at this point.
The question is - can I implement a simple map for now or should I go for a general solution and a package tree?

Do what you need to do to get this issue finished for M7 (so...simple map for now). Please open a separate issue for the more general solution. Unless we know of a future task that requires the more complicated approach for the current project, do not assign a milestone to the new issue.

Is there any significant technical debt you are adding by going with the simple map approach now, which will complicate the more generic approach later? If so, please note this in the new issue.

Is there any significant technical debt you are adding by going with the simple map approach now, which will complicate the more generic approach later?

Not, it's not a problem to change the underlying structure.

The problem: dynamic queries do not recognize fields of dynamic tables specified in 4GL where clause.
Cause:
1. Names of dynamic table fields can be arbitrary with almost no limitations, so these names are converted to hibernate field names "field<number>".
2. At the time when dynamic tables facility was created there were no legacy annotations, so "field<number>" are passed as the legacy names to the conversion pipeline.
3. Table mapping facility uses legacy annotations to get legacy field names.

Solution options:
Option 1. Probably the most correct solution is to add "force_hibernate_name" field conversion annotations.
Option 2. Specify proper "historical" conversion attribute for fields.
Option 3. The fastest one which I've implemented: use DynamicTablesHelper.get4GLFieldName to get the legacy name.

So if for some reason we need 100% correct generated DMO implementation class we should go with #1 (or #2), otherwise #3 is OK.

So, when you use a dynamic table tt (with fields f-1 and f-2) in a dynamic query, and in 4GL you have this predicate: each tt1 where tt.f-1 = 0 or tt.f-2 = 0, how will the converted HQL look like, with solution 3?

With any solution each tt1 where tt.f-1 = 0 or tt.f-2 = 0 will be converted to select tt__1.id from DynamicRecord1Impl as tt__1 where ((tt__1._multiplex = ?) and (tt__1.field1 = 0 or tt__1.field2 = 0)) order by tt__1._multiplex asc, tt__1.id asc

Code review 20131129a:

Looks good overall, but I have some questions/comments:

• In DMOValidator.uniqueConstraintViolation, you made a change to use the legacy table name. Have you confirmed that it is the legacy table name in use in these error messages, rather than the legacy buffer name? Also, there is a TODO for the legacy field name. We have the annotations in the DMO implementation class and the TableMapper.getLegacyFieldName method. Is there still some missing functionality that prevents retrieving the legacy field name?
• In StaticTempTable, many of the methods have TODOs to be implemented. In the header, you have the comment, "Most of TODOs is just about throwing a proper error". How does this affect runtime behavior? Is this just about errors that only come into play in improperly written 4GL code?
• In entry 207 in the RecordBuffer header, there seems to be a cut-and-paste error that left behind the text "Added metadata support."
• Why are createDynamicBufferForTempTable and getParentTable in the RecordBuffer class instead of TemporaryBuffer? Both methods need javadoc.
• Minor typo in header entry for DynamicQueryHelper.

Frankly, the amount of change in some sensitive areas of the persistence framework makes me a bit nervous. I will feel better when this update has passed regression testing. Do you have a set of standalone tests we can use going forward as a rough regression suite for the new, dynamic functionality (albeit not automated)?

Please also note that a large update for #2116 was just committed, and may require merges.

• In DMOValidator.uniqueConstraintViolation, you made a change to use the legacy table name. Have you confirmed that it is the legacy table name in use in these error messages, rather than the legacy buffer name?

Yes

Also, there is a TODO for the legacy field name. We have the annotations in the DMO implementation class and the TableMapper.getLegacyFieldName method. Is there still some missing functionality that prevents retrieving the legacy field name?

It's no problem to make that change. At first I wasn't going to modify validation code, but dynamic table names like "table__1" attracted my attention. I was in a hurry to make the update, so I'll finish that now.

• In StaticTempTable, many of the methods have TODOs to be implemented. In the header, you have the comment, "Most of TODOs is just about throwing a proper error". How does this affect runtime behavior? Is this just about errors that only come into play in improperly written 4GL code?

The errors appear if you try to modify a static table definitions, so yes, it will appear in improperly written 4GL code.

• Why are createDynamicBufferForTempTable and getParentTable in the RecordBuffer class instead of TemporaryBuffer?

createDynamicBufferForTempTable - yes that is a mistake. getParentTable is a convenience function for "RecordBuffer.tableHandle() + unwrap handle" so that is correct.

Frankly, the amount of change in some sensitive areas of the persistence framework makes me a bit nervous. I will feel better when this update has passed regression testing.

I'll run regression testing on this weekend.

Do you have a set of standalone tests we can use going forward as a rough regression suite for the new, dynamic functionality (albeit not automated)?

Yes, I have testcases, I'll review and post them.

Is this just about errors that only come into play in improperly written 4GL code?

The errors appear if you try to modify a static table definitions, so yes, it will appear in improperly written 4GL code.

Are these compile errors? The only errors we don't duplicate are compile errors. If the errors can occur in working 4GL code, even if that code reacts badly, we still duplicate the behavior. It is possible that applications can depend upon the error behavior, depending on how the block structure is written. The programmers may not even know that the code fails, because the app appears to work and the failing code is basically just "dead".

OK, I'll implement error handling in StaticTempTable. I don't think this should take much time.

Guy, what do you think is the best way to resolve naming conflict between buf.name and buffer buf:name? There are also attributes TABLE, DB-NAME etc. Maybe we should use _getName() instead?

Testcase:

def temp-table tt field name as char.
create tt.
tt.name = "val".

message string(tt.name).
message string(buffer tt:name).

Converted code:

tt.create();
tt.setName(new character("val"));
message(valueOf((character) new FieldReference(tt, "name").getValue()));
message(valueOf(tt.getName()));

Correct output:

val
tt

P2J output:

val
val

My update fails regression testing because this issue.

Stanislav Lomany wrote:

Guy, what do you think is the best way to resolve naming conflict between buf.name and buffer buf:name? There are also attributes TABLE, DB-NAME etc. Maybe we should use _getName() instead?

To fully fix this we need to look up the BufferImpl hierarchy and rename all the java-style getters/setters for legacy attributes to something else, so they will not collide with DMO property getters/setters (temporary or permanent). To fully fix this deficiency will touch lots of files, as almost all resources will be affected by this renaming.

As an approach, for some attributes we chose to use i.e. character name() as getter and i.e. name(character something) as setter (i.e. just remove the get/set prefix).

Also, we could fix only the conflicts for the server project now and leave the rest for later.

As an approach, for some attributes we chose to use i.e. character name() as getter and i.e. name(character something) as setter (i.e. just remove the get/set prefix).

Also, we could fix only the conflicts for the server project now and leave the rest for later.

You are right about the amount of changes, so I've changed get/setName to name only.

As far as I can tell, there is the single regression left:

public static String TableMapper.getLegacyFieldName(Class<?> dmoClass, String property)
{
   String schema = DatabaseManager.getSchemaByClass(dmoClass);
   if (DatabaseManager.TEMP_TABLE_SCHEMA.equals(schema))
   {
      final String msg =
         "Legacy field names for a property of a temporary DMO can not be resolved using " +
        "this API!";
      throw new IllegalArgumentException(msg);
   }

   synchronized (permanentDBs)
   {
      PermanentTableMapper mapper = locatePerm(schema);

      return mapper.legacyFieldName(dmoClass, property);  // NPE HERE!
   }
}

1. It is reproducible on a server after regression testing.
2. It is NOT reproducible on a fresh server.
3. The issue is about a permanent DMO of the Majic database.

In case you have any ideas about the origin of this bug let me know.

Some possible problems:
1. is it possible that getLegacyFieldName is called with a DMO interface parameter, not a DMO class?
2. the permanent mappings are kept by schema. Looking back, I don't think this was a fortunate choice, as a transient DB might use the same schema as a permanent DB. And when the transient DB is disconnected, the entire schema might get removed. Thus, try this scenario with a fresh server: connect to RFQ, disconnect RFQ and after this check the NPE scenario.

If this is the problem, then add a test in DatabaseManage.deregisterDatabase:1883:

            // deregister the legacy name mappings for all tables in this permanent DB
            String schema = DatabaseManager.getSchema(database);
            if (<no DB in the permanentConnections set uses this schema>)
            {
               TableMapper.removePermanentSchema(schema);
            }

Great! You are correct about the schema issue.

1204a looks good. Please commit and distribute the update.

Committed to bzr revision 10418.

All unfinished issues were moved to #2207.

Stanislav/Eric: is there a task for implementing the attributes/methods for the BUFFER-field resource? I ask this because for M7 we will need at least support for the DATA-TYPE attribute for the BUFFER-field resource. The server project uses at least one construct like h:BUFFER-FIELD(ch):DATA-TYPE.

Use #1655 for this. We can change the assignee.

OK, I've added the info there.