Project

General

Profile

Feature #3145

implement _File._Template metadata support

Added by Eric Faulhaber almost 8 years ago. Updated almost 8 years ago.

Status:
Closed
Priority:
Normal
Target version:
-
Start date:
Due date:
% Done:

100%

billable:
No
vendor_id:
GCD
version_reported:
version_resolved:

History

#1 Updated by Eric Faulhaber almost 8 years ago

The MetaFile DMO is converted from the _File metadata table. It is populated at server startup with read-only data which was read from the application's .df file during schema conversion.

Every Progress database table has a template record. Apparently, the template record contains all the default information needed to create a new, empty record for a given table, including the initial values of all fields. The RECID of a table's template record is stored in the _Template field of the _File metadata table. Once the template record's RECID is retrieved from the _File table, the template record can be retrieved from a target table x using the idiom:

FIND x WHERE RECID(x) = <template_recid>

See also http://knowledgebase.progress.com/articles/Article/P105275.

Oddly, the _File._Template field is not marked MANDATORY, which suggests there are certain table types in Progress for which this feature does not apply. However, I think this field must not be unknown value for table type "T".

It seems the template record can only be found directly by its RECID, as described above. I've never seen such a record through normal use of a table, so I guess it is hidden from normal searches. Attempts to update the template record at runtime result in the message:

Cannot update a template record without a schema lock

#2 Updated by Eric Faulhaber almost 8 years ago

In terms of implementing this feature in P2J, it seems the _File._Template field could always contain -1 for table type "T" (which is the table type currently set for all _File records, though this probably is not a good long term plan).

We don't want to actually store such a template record in the database for every table, as this would have far-reaching, unwanted consequences with just about any normal use of that table. However, it seems a template record is essentially just a newly created DMO with all its default values. We could reserve the record ID -1 for this record and treat any FIND by RECID for that value differently, such that it loads a "blank" record into the current buffer. This determination would have to be made at runtime, since we won't know the RECID being checked until then, and we don't want to break the normal FIND by RECID idiom.

One should not be able to update the fields of the template record (i.e., any record with RECID -1) directly, and this record should never be added to the Hibernate session (we don't want it dirty checked or saved). This restriction requires some more thought as to its implications for the implementation.

#3 Updated by Ovidiu Maxiniuc almost 8 years ago

  • Status changed from New to WIP

Probably the correct way to implement this to match P4GL behaviour is at import time. Or maybe an intermediary step in the import procedure, after the schema is built using the DDL. I am thinking of some kind of database 'formatting' where the database is populated with these true records. They need to have some additional data to distinct them from normal records (like an additional boolean). Well, this is difficult to maintain and the record is exposed to external changes. Also, the queries will get more complex since the where predicates will need to incorporate filters for these records.
Because of these issues, I think we will stick to '-1' approach. Or generally, negative numbers if different values are required for different tables.

Some additional pieces of information:
  • the _template seems to be not unknown, but it can be 0 for special tables (some of S and V types);
  • if _template = 0 you cannot create new records in respective table. This seems logical. There error message is the following: Creating records in a Virtual System Table is not allowed. (6248).
  • attempting to even 'touch' a template record will terminate the current procedure with Cannot update a template record without a schema lock. (12376)

#4 Updated by Eric Faulhaber almost 8 years ago

Ovidiu Maxiniuc wrote:

[...]
Because of these issues, I think we will stick to '-1' approach.

Agreed, this should be a runtime-only solution, with minimal impact on existing "normal" database access.

Or generally, negative numbers if different values are required for different tables.

This is a good point, in that every RECID/ROWID internal value is unique today. I'm not sure how important it is in this case, since these will not be real records in the database, but it's probably a good idea to preserve this convention.

Some additional pieces of information:
  • the _template seems to be not unknown, but it can be 0 for special tables (some of S and V types);
  • if _template = 0 you cannot create new records in respective table. This seems logical. There error message is the following: Creating records in a Virtual System Table is not allowed. (6248).

We don't need to implement these two items at this time, as the use case we need to support is limited to table type "T", but it is good to know.

  • attempting to even 'touch' a template record will terminate the current procedure with Cannot update a template record without a schema lock. (12376)

Is this a STOP condition or a QUIT condition? We will need to emulate this properly.

#5 Updated by Ovidiu Maxiniuc almost 8 years ago

Eric Faulhaber wrote:

Some additional pieces of information:
  • the _template seems to be not unknown, but it can be 0 for special tables (some of S and V types);
  • if _template = 0 you cannot create new records in respective table. This seems logical. There error message is the following: Creating records in a Virtual System Table is not allowed. (6248).

We don't need to implement these two items at this time, as the use case we need to support is limited to table type "T", but it is good to know.

I though to have this here in 'knowledge base' for future reference.

  • attempting to even 'touch' a template record will terminate the current procedure with Cannot update a template record without a schema lock. (12376)

Is this a STOP condition or a QUIT condition? We will need to emulate this properly.

Sorry, this is just a normal ERROR condition. Using NO-ERROR option the procedure continues. I was trying to emphasize the 'touch' aspect. The error is raised even if the record is not actually changed.

#6 Updated by Ovidiu Maxiniuc almost 8 years ago

I started the implementation. To correctly identify the searched by RECID/ROWID I adjusted the HQLPreprocessor.checkFindByRowid(). However, after the query have failed to be positioned on a record and loading the default/template into the buffer as R/O is a little tricky.

#7 Updated by Eric Faulhaber almost 8 years ago

Ovidiu Maxiniuc wrote:

I started the implementation. To correctly identify the searched by RECID/ROWID I adjusted the HQLPreprocessor.checkFindByRowid(). However, after the query have failed to be positioned on a record and loading the default/template into the buffer as R/O is a little tricky.

We should never execute a query if the target ROWID is negative. Once we know we have a find by ROWID and a negative ROWID query substitution parameter, the logic must avoid executing the query and go into this other mode.

#8 Updated by Ovidiu Maxiniuc almost 8 years ago

I have created the task branch 3145a. The last revision is 11072.

I discovered that any 'find by rowid/recid' first/next/prev/last all behave like 'unique' find. This seem somewhat logical. But it's strange because if you find a first record you can then find the previous (which is the same record) so the

find first T where recid(T) = K.
do while available T:
    find next T where recid(T) = K.
end.

idiom is broken. This (and the similar other) is a endless loop.

I have no idea if TEMP-TABLES have a template record. At least the _file does not have records about these tables because they are not part of the db schema.

#9 Updated by Eric Faulhaber almost 8 years ago

Ovidiu Maxiniuc wrote:

I have created the task branch 3145a. The last revision is 11072.

I discovered that any 'find by rowid/recid' first/next/prev/last all behave like 'unique' find. This seem somewhat logical. But it's strange because if you find a first record you can then find the previous (which is the same record) so the
[...]
idiom is broken. This (and the similar other) is a endless loop.

I think we just have to accept that a FIND by RECID/ROWID is a special case. Do we behave the same way as Progress in this regard currently? IIRC, we would not, but I haven't looked at the related code in a while.

I have no idea if TEMP-TABLES have a template record. At least the _file does not have records about these tables because they are not part of the db schema.

If there is no way you can determine to retrieve the RECID/ROWID of a template record for a temp-table (I do not know of one), we should not implement (or disable) this behavior for temp-tables.

#10 Updated by Ovidiu Maxiniuc almost 8 years ago

Eric Faulhaber wrote:

I think we just have to accept that a FIND by RECID/ROWID is a special case. Do we behave the same way as Progress in this regard currently? IIRC, we would not, but I haven't looked at the related code in a while.

With 3145a/11072 we match FIND by RECID/ROWID Progress searches for basic queries. At this moment we cover the normal usecase for template records:
  • locate template id (from _file meta table)
  • use id to FIND to load it (using RECID function)
  • 'read' the fields

This is the basic support for template records that should cover 90% of the usages.

I have no idea if TEMP-TABLES have a template record. At least the _file does not have records about these tables because they are not part of the db schema.

If there is no way you can determine to retrieve the RECID/ROWID of a template record for a temp-table (I do not know of one), we should not implement (or disable) this behavior for temp-tables.

In 3145a/11072 the temp-tables are eliminated before loading the template record in RandomAccessQuery. At this moment I don't think that these is a similar _file meta table for temp-tables. Having the template records makes sens for permanent database because this is the way to access an unknown database schema. Working with temp-tables means the programmer has access to temp-tables. They are contained in teh same source file. At least this is my logic.

For compound queries things are a little complex. It is logical to have constructs like this:

DEFINE BUFFER book2 FOR book.

FOR FIRST _file WHERE _file._file-name EQ "book",
   FIRST book WHERE RECID(book) = _file._template 
      EACH book2 WHERE book2.isbn NE book.isbn
...

to examine records that have changed their isbn.

The place where the 2nd query can be intercepted would be in PreselectQuery.executeQuery(). If the PreselectQuery has a single QueryComponent component, we can check its HQLPreprocessor and at this point we have the args that should eventually contain the template record id it is passed case of SUBST node (the likely way).

To load the template back in an AdaptiveQuery we should return a new TemplateResult set with a single component for working with the db Cursor. The really big problem here is to avoid the entity to reach Hibernate.

#11 Updated by Eric Faulhaber almost 8 years ago

Code review 3145a/11072:

Please help me understand the purpose of the variable_references.rules change. Aren't we already doing this resolution at runtime? If not, why is this change needed now, but wasn't needed before?

The RecordBuffer changes don't look quite right to me. Specifically, the use of setCurrentRecord from the new loadTemplateRecord has many side effects that may not be desirable. One at least is the DMO reference counting we do here. We do not want template records reference counted, because the reference counting is all about keeping track of records in a Hibernate session. A template record should never be in the Hibernate session, and so we should never try to evict it from the session when its DMO reference count reaches 0. Also, taking its snapshot should not be necessary, because this is about FIND query navigation (which as you've found is not a feature of template records) and about preserving the state of a deleted record (and template records cannot be deleted).

OTOH, a lot of the processing in setCurrentRecord is about flushing, validating, releasing, lock management, and possibly firing a write trigger for the record being pushed out of the buffer by the template record. So, much of this method's behavior is still necessary. I guess we either need to refactor this method, or add some conditional processing.

HQLPreprocessor: why are we storing information about a substitution parameter in the instance variable findByRowid_RecNo (btw, please don't use underscores in Java names)? This will only be correct the first time the HQLPreprocessor instance is used, but it will most likely be wrong coming out of the cache. It seems the determination of whether a query is a "find-by-recid/rowid" type of query is best done by HQLPreprocessor, but the determination of whether such a query actually represents a template record lookup is better done elsewhere (or, if in HQLPreprocessor, at least in such a way that stale information is not stored in a cached HQLPreprocessor object).

Please fix typos in file headers: metaschema.xml: "Initialix"; Persistence.java: "Duble"

#12 Updated by Ovidiu Maxiniuc almost 8 years ago

Thanks for review, Eric!
Eric Faulhaber wrote:

Code review 3145a/11072:

Please help me understand the purpose of the variable_references.rules change. Aren't we already doing this resolution at runtime? If not, why is this change needed now, but wasn't needed before?

Indeed, that was an initial attempt to extract the value from the rowid at the moment I was checking if the query is looking for the template. Hibernate uses RowidUserType but I needed the long value. This changes simply creates an int64 to wrap the rowid value because they are not directly compatible in P4GL. Later, I added special handling in getFindByRowid() but I forgot to drop the conversion changes.

The RecordBuffer changes don't look quite right to me. Specifically, the use of setCurrentRecord from the new loadTemplateRecord has many side effects that may not be desirable. One at least is the DMO reference counting we do here. We do not want template records reference counted, because the reference counting is all about keeping track of records in a Hibernate session. A template record should never be in the Hibernate session, and so we should never try to evict it from the session when its DMO reference count reaches 0. Also, taking its snapshot should not be necessary, because this is about FIND query navigation (which as you've found is not a feature of template records) and about preserving the state of a deleted record (and template records cannot be deleted).
OTOH, a lot of the processing in setCurrentRecord is about flushing, validating, releasing, lock management, and possibly firing a write trigger for the record being pushed out of the buffer by the template record. So, much of this method's behavior is still necessary. I guess we either need to refactor this method, or add some conditional processing.

I will review the setCurrentRecord and add supplementary conditionals so that the template DMOs won't be counted or copied.

HQLPreprocessor: why are we storing information about a substitution parameter in the instance variable findByRowid_RecNo (btw, please don't use underscores in Java names)? This will only be correct the first time the HQLPreprocessor instance is used, but it will most likely be wrong coming out of the cache. It seems the determination of whether a query is a "find-by-recid/rowid" type of query is best done by HQLPreprocessor, but the determination of whether such a query actually represents a template record lookup is better done elsewhere (or, if in HQLPreprocessor, at least in such a way that stale information is not stored in a cached HQLPreprocessor object).

HQLPreprocessor only stores the numeric literals encountered when parsing then hql. This will be hardcoded in processed hql and so the preprocessor can safely to the check. OTOH, if the lookup value is not hardcoded, the findByRowid_RecNo remains null and getFindByRowid uses the current parameters to return the lookup value.

Please fix typos in file headers: metaschema.xml: "Initialix"; Persistence.java: "Duble"

OK.

#13 Updated by Eric Faulhaber almost 8 years ago

Ovidiu Maxiniuc wrote:

I will review the setCurrentRecord and add supplementary conditionals so that the template DMOs won't be counted or copied.

OK. Please consider the logic for both the case where the template record is the new record being stored and the case where the template record is the old record being released.

HQLPreprocessor only stores the numeric literals encountered when parsing then hql. This will be hardcoded in processed hql and so the preprocessor can safely to the check. OTOH, if the lookup value is not hardcoded, the findByRowid_RecNo remains null and getFindByRowid uses the current parameters to return the lookup value.

OK, I see that now, thanks for the clarification. But please still remove the underscore.

#14 Updated by Ovidiu Maxiniuc almost 8 years ago

I implemented support for loading template records in simple PreselectQuery. The trick was to intercept the template queries and return a special Results with a single row. Unfortunately, I could not find a solution for more complex queries like the one at the end of note 10. At least not without breaking back the work done by CompoundQuery.
Latest revision is 11074. (not yet rebased).

The same update contains an workaround for ContextLocalCleanupException thrown when client disconnects and the Peristence context is not accessible through get().

#15 Updated by Eric Faulhaber almost 8 years ago

Code review 3145a/11077:

Sorry I didn't get to this yesterday. From my understanding of the template record feature in the 4GL, this update looks like it should work for our current purposes.

The changes to PreselectQuery I think need some attention, but not at this time. Note that this query type is not only used for FOR EACH constructs. It is the base class for other query types as well. I'm not sure if it was intentional to apply the template record changes to all these query types. One thing that looked strange is that the template record is only ever loaded into the first (i.e., zero-th) component's buffer -- that doesn't seem right. Please put a TODO marker in regarding this for future reference.

The javadoc for the TemplateResults inner class is unfinished and the method formatting needs some work (some method bodies are inline with the method names). This should be done now instead of being deferred.

The access modifier for RandomAccessQuery.getHelper has changed; please move the method accordingly.

Please change the exception thrown from RecordBuffer.loadTemplateRecord to IllegalArgumentException and add a @throws tag to the javadoc for it. A throws statement on the method itself is not needed, since this is an unchecked exception, but I like the javadoc to tell which exceptions (including unchecked) a method might throw.

#16 Updated by Ovidiu Maxiniuc almost 8 years ago

Eric Faulhaber wrote:

The changes to PreselectQuery I think need some attention, but not at this time. Note that this query type is not only used for FOR EACH constructs. It is the base class for other query types as well. I'm not sure if it was intentional to apply the template record changes to all these query types.

Yes, that was my intention, so all derived queries to benefit from (simple) template record lookup.

One thing that looked strange is that the template record is only ever loaded into the first (i.e., zero-th) component's buffer -- that doesn't seem right. Please put a TODO marker in regarding this for future reference.

I tried to isolate only the simple queries that expressly seek the template record. These queries have a single component (see getTemplateQueryRowid()) and the pattern is provided by its HQLPreprocessor. When iterating such result it's logical that only the single component's buffer to be loaded with template record.

#17 Updated by Eric Faulhaber almost 8 years ago

Ovidiu Maxiniuc wrote:

I tried to isolate only the simple queries that expressly seek the template record. These queries have a single component (see getTemplateQueryRowid()) and the pattern is provided by its HQLPreprocessor. When iterating such result it's logical that only the single component's buffer to be loaded with template record.

I understood from your previous findings that more complex queries are possible in the 4GL, so please document this current limitation of our implementation in the code. Thanks.

#18 Updated by Ovidiu Maxiniuc almost 8 years ago

Copy/pasted from mail conversation. Please read in timestamp (reverse) order:

  1. To check meta support, I will add a status getter in MetadataManager initialized at startup. The projects have about 2K tables so I am thinking of caching the _template column in a map when _File metatable is populated. There is a little issue there because the MM is really not aware of the semantics of the tables it populates but I can force it based on _file._template legacy name.
  2. I've redone some tests on windev01. There is nothing reported in client when the negative rowid is used in a query. Normally the ** FIND FIRST/LAST failed for table T. (565), as the query failed for a normal field. They say the error is only printed in database log file - I did not checked this, but as you said, it is not important. I did not observe any other special condition here.

On 07/22/2016 10:36 PM, Eric Faulhaber wrote:

This approach makes sense.

The checks to determine meta support and to verify the template record ID must be very fast. The latter should be cached, either at server startup or lazily.

I'm not sure error 18 is a runtime error condition, or just something Progress logs to the database log. If the former, we're already doing it wrong and this should be resolved. If the latter, I think it can be ignored. I guess it is the latter; otherwise, we probably would have had some fallout from the code you hit regression testing Majic before now. Anyway, please investigate what the knowledgebase article is talking about with a test case.

On 07/22/2016 03:02 PM, Ovidiu Maxiniuc wrote:

I am thinking like this:
  • if the project is configured without meta support, ignore the new code (this will make Majic work). Since the project is not metadata aware, template records are not available.
  • if meta _file is available, we extend the template detection with a (fast - I hope) lookup in _file table. If the template id is correct, we act the current path from 3145 otherwise (recid is negative but does not match the _file.template):
    • either call release on buffer (to empty it)
    • raise condition 18 as you noted.

Is this OK? I know this is not perfect, but it should work.

On 07/22/2016 09:52 PM, Eric Faulhaber wrote:

It seems non-positive RECID and ROWID values are invalid in Progress, but querying them is legal:
http://knowledgebase.progress.com/articles/Article/P12127
http://knowledgebase.progress.com/articles/Article/21092

Apparently, this idiom should be generating an error 18 in the database log file. I'm not sure what this means in terms of runtime behavior, however. If it does not produce a runtime error condition, I guess the net effect is that it empties the buffer. This would match the behavior of our pre-3145 implementation in P2J (albeit without the database log error).

Unfortunately, this means that a query for a non-positive RECID value is a legal thing to do in Progress, and produces a different result than what we've designed with 3145.

Anyone have any thoughts on how to salvage this?

On 07/22/2016 02:31 PM, Ovidiu Maxiniuc wrote:

I have a good news and a bad one. The good news is that all ETF tests passed without errors. I was on the edge of asking to commit this branch.

The bad news is the Majic failed. The new, unexpected error message appeared in both screens and logs.
I did some investigations and I found this piece of code in customer's code:

find T where recid(T) = -1 EXCLUSIVE-LOCK no-error.

I am not aware how this behave or what workaround is here but, with the new code the problem is that we are finding the respective record as being the template record. A couple of lines below, the job record is updated so normally, the 12376 condition is raised.

I am trying now to understand why Majic was looking for -1 job record, but I would appreciate any input from you.

I expected some trouble in the future and I let an option to verify if really the ID the recid find is really the template, but AFAIK, Majic runs without _file meta table so this check is impossible.

#19 Updated by Eric Faulhaber almost 8 years ago

Code review 3145a/11081:

HQLHelper.isFindTemplate should be package private and be moved to the appropriate location in the file.

Please update the javadoc for PreselectQuery.executeQuery and fetch to reflect the new behavior of each.

I don't think the MetadataManager.inUse flag is needed. No queries are permitted until the server is completely initialized. Unless that is changed in the future, there should never be a situation where there is a conflict between business logic and metadata initialization.

Everything else looks fine. If you already have begun regression testing this revision, there is no need to re-test after making these relatively minor changes.

#20 Updated by Ovidiu Maxiniuc almost 8 years ago

Eric Faulhaber wrote:

Code review 3145a/11081:

Thanks for the review. I will do the required changes. I am in the middle of testing

I don't think the MetadataManager.inUse flag is needed. No queries are permitted until the server is completely initialized. Unless that is changed in the future, there should never be a situation where there is a conflict between business logic and metadata initialization.

The reason for this method is not to synchronize access to internal but rather to check if the project was configured with this feature (for example, Majic is not) or if metadata initialization failed, and quickly leave the calling method. The method returns true only if the initialization completed successfully. If the initialization fails then the _meta tables are not "in use". I will revise the javadoc.

#21 Updated by Eric Faulhaber almost 8 years ago

Ovidiu Maxiniuc wrote:

The reason for this method is not to synchronize access to internal but rather to check if the project was configured with this feature...

I see, please disregard my comment then.

#22 Updated by Ovidiu Maxiniuc almost 8 years ago

The revision 11082 has passed all test (devsrv01 and ETF). The only issue was gso231 because my test framework was not updated after the fix.
The update was rebased so the latest revision is 11083.

#23 Updated by Eric Faulhaber almost 8 years ago

Great! Please merge to trunk.

#24 Updated by Ovidiu Maxiniuc almost 8 years ago

The task branch was merged to trunk as revision 11074.
The test results and branch were archived and notification was sent to team.

#25 Updated by Eric Faulhaber almost 8 years ago

  • Status changed from WIP to Closed
  • % Done changed from 0 to 100

Also available in: Atom PDF