FWD » Core Development » Database

The following schema metadata features are used in an application and are either unsupported or are partially implemented. Where the purpose is very tightly dependent upon the internal implementation in Progress (e.g., _area), we may need to explore alternatives, or these just might not make sense to support and could require application changes.

_area._area-number
_area._area-name

_connect._connect-transid
_connect._connect-disconnect
_connect._connect-name
_connect._connect-time
_connect._connect-type
_connect._connect-transid
_connect._connect-device
_connect._connect-pid
_connect._connect-server
_connect._connect-batch

_myconnection._myconn-userid

_database-feature.dbfeature-id
_database-feature.dbfeature_enabled

_file._can-create
_file._can-write
_file._can-read
_file._can-delete
_file._crc
_file._file-label-sa
_file._fil-misc2
_file._ianum
_file._valmsg-sa

_field._can-read
_field._can-write
_field._charset
_field._collation
_field._col-label-sa
_field._decimals
_field._desc
_field._dtype
_field._fetch-type
_field._field-physpos
_field._fld-stoff
_field._fld-case
_field._fld-misc1
_field._fld-misc2
_field._fld-res1
_field._fld-res2
_field._fld-stdtype
_field._fld-stlen
_field._fld-stoff
_field._format-sa
_field._for-allocated
_field._for-id
_field._for-itype
_field._for-maxsize
_field._for-name
_field._for-primary
_field._for-retrieve
_field._for-scale
_field._for-separator
_field._for-spacing
_field._for-type
_field._for-xpos
_field._help
_field._help-sa
_field._initial
_field._initial-sa
_field._label-sa
_field._sys-field
_field._user-misc
_field._valmsg-sa
_field._view-as
_field._width

_filelist._filelist-name

_file-trig._event
_file-trig._override
_file-trig._proc-name
_file-trig._trig-crc
_file-trig._file-recid

_field-trig._override
_field-trig._event
_field-trig._file-recid
_field-trig._field-recid
_field-trig._proc-name
_field-trig._trig-crc

_index._active
_index._idx-num
_index._wordidx
_index._ianum
_index._desc

_index-field._ascending
_index-field._unsorted
_index-field._abbreviate
_index-field._index-seq

_lock._lock-chain
_lock._lock-flags

_sequence._cycle-ok
_sequence._db-recid
_sequence._seq-incr
_sequence._seq-min
_sequence._seq-max
_sequence._seq-name
_sequence._seq-init

_startup._startup-locktable

_usertablestat._usertablestat-conn
_usertablestat._usertablestat-num
_usertablestat._usertablestat-read
_usertablestat._usertablestat-update
_usertablestat._usertablestat-create
_usertablestat._usertablestat-delete

Which FWD classes are working with metadata? I've never worked with this and need some starting point.
Thank you.

Igor Skornyakov wrote:

Which FWD classes are working with metadata? I've never worked with this and need some starting point.
Thank you.

The metadata is scattered around persistence support. Please take a look at MetadataManager. This is the starting point. Then there are some specialized classes like: LockTableUpdater, ConnectTableUpdater, ConnectTableUpdater, TransactionTableUpdater, ConnectionManager and DatabaseManager.
Note that there are three kind of meta data: persistent (like _Users table, these can be altered), static system info (with information on tables, fields and indexes), and statistic (volatile information that is relative to current server lifetime).
There are some important 'entry-points' for these, like ConnectionManager.initializeMeta(), TransactionManager.initializeMeta() and MetadataManager.populateDatabase() mehods.
Then, the metadata works hand-in-hand with SVT (System Virtual Tables - these tables are more like statistics collected by the system and behave a bit different). Take a look over the current VST support in RecordBuffer.

Thank you very much, Ovidiu!

Additional metadata requirements:

Need conversion and runtime support:

_connect._connect-clienttype
_connect._connect-ipaddress
_connect._connect-tenantid
_connect._connect-transid
_connect._connect-type

_field._decimals
_field._help

_file._creator
_file._file-attributes

_file-trig._event
_file-trig._file-recid

_filelist._filelist-name

_index-field._ascending
_index-field._abbreviate
_index-field._index-seq
_index-field._unsorted

_index._active
_index._wordidx

_sequence._seq-attributes
_sequence._seq-name
_sequence._seq-num

_tenant._tenant-attributes
_tenant._tenant-name
_tenant._tenant-type

Need runtime support:

_connect._connect-server
_connect._connect-batch

Need runtime testing:

_connect._connect-device
_connect._connect-name
_connect._connect-pid
_connect._connect-time

_lock._lock-flags

_myconnection._myconn-userid

Need runtime review:

_file._owner
_file._tbl-type

Where can I find the description of the schema metadata? So far I've found only this article: https://knowledgebase.progress.com/articles/Article/P14266 which says that it was never documented (and there where no intent to do this).
Thank you.

Igor, you can query the meta tables from a 4GL environment as a standard stable. For example:

   FOR EACH _connect:
       DISPLAY _connect.
   END.

Optionally you can get the dump of these tables (as .d files), but some of them (the VSTs) do not make sense offline.

I hope the will help you somehow. I am not sure if you can get detailed information about all these tables. Most likely you'll have to do additional investigations if the .df description are very explicit.

Ovidiu Maxiniuc wrote:

Igor, you can query the meta tables from a 4GL environment as a standard stable. For example:
[...]
You can export their definition also as for standard table (in the GUI Data Dictionary, enable View/Show Hidden Tables). We already have a full set of them, see the standard.df file in any project. Pay attention that these are dumped from different installations and they differ a bit (incrementally) with each P4GL version.

Optionally you can get the dump of these tables (as .d files), but some of them (the VSTs) do not make sense offline.

I hope the will help you somehow. I am not sure if you can get detailed information about all these tables. Most likely you'll have to do additional investigations if the .df description are very explicit.

Thank you, Ovidiu!

I understand that most of the data mentioned in this task are related to data security. I'm not sure that most of them have direct counterparts in the relational databases supported by FWD (PostgreSQL and MS SQL Server?). In any case, using of such counterparts require user impersonation. Do we support such impersonation now?
Support of these security primitives purely at the FWD level will most likely need a lot of development and may result in a substantial runtime overhead.

Maybe, at the first stage, it makes sense just to add support for the operations with these data but without the support of the semantics? Please note that the exact semantics is not documented and trying to figure it using "black box testing" will be very time-consuming and unreliable simply due to a large number of moving parts.

Please clarify.
Thank you.

I understand that most of the data mentioned in this task are related to data security.

I don't think so. The _file._can* and _field._can* are the only ones that seem to be security related.

Anything in _connect and _my-connection will be supported by application server state, not database.

Can you be more specific with what items you think are security-related?

I'm not sure that most of them have direct counterparts in the relational databases supported by FWD (PostgreSQL and MS SQL Server?).

No one is saying that all of this is implemented at the database. Much of it would be expected to be in the FWD application server.

You've been analyzing this for a few days. Please provide more details so we can discuss.

In any case, using of such counterparts require user impersonation. Do we support such impersonation now?

We don't support it now, but it is guaranteed that we will need to do so at some point. I'm not sure if that point is now or not. It will depend on the use cases in the specific applications being worked on.

Greg Shah wrote:

You've been analyzing this for a few days. Please provide more details so we can discuss.

Greg,
I'm working on a detailed report right now. It will be ready by the end of the weekend.

In this note, I summarise my current understanding of the scope of this task and things which are not clear for me.

The task is about the FWD support of the 4GL metadata tables which are are of two different types:

1. Virtual System Tables (VSTs) - exist at runtime only:

   _file-name	_file-number
   _connect	-16385
   _myconnection	-16422
   _filelist	-16393
   _startup	-16392
   _usertablestat	-16430
   _lock	-16412
   _database-feature	-16427

1. Scheme tables - a "real" tables in the database schema:

   _file-name	_file-number	_category
   _area	-71	PHY_ST
   _tenant	-340	AUTH
   _file	-1	SCHEMA
   _field	-2	SCHEMA
   _file-trig	-65	SCHEMA
   _field_trig	-66	SCHEMA
   _index	-3	SCHEMA
   _index-field	-4	SCHEMA
   _sequence	-67	SCHEMA

The Progress documentation contains a section with a very brief description of VST fields (see https://documentation.progress.com/output/ua/OpenEdge_latest/index.html#page/dmadm%2Fvirtual-system-table-summaries.html%2). I understand these tables contain different runtime statistics and are read-only for the application code. The support for these VSTs is mostly already implemented in FWD and what is required is to add support of missing fields and test support for some existing ones. I do not understand what the "conversion support" (#3814-7) means as it seems that it is sufficient just to add the names of these tables to the standard metadata section of the p2j.cfg.xml file. However, the lack of a detailed description complicates both adding support for new fields and testing the support of the existing ones. Maybe we have something more informative than mentioned Progress documentation?

The situation with schema tables is more complicated. I've not found and information about most of them in the standard Progress documentation but there is some useful information published at Progress User Forums: https://pugchallenge.org/downloads2015/448_schema_tables_v04.pdf, https://pugchallenge.org/downloads2019/364_SystemTables.pptx. There is also some useful info on Russian forums: https://rupug.pro/openedge-security/ and https://www.progress-tech.ru/blog/openedge-security (this a translated paper but I was unable to find the English version).

The _area table is related to a physical layout of the 4GL database files and cannot have any reasonable counterpart in FWD.

I was unable to find any information about the _tenant table. It seems also that FWD does not support multi-tenant databases so far.

For the rest of the schema tables in the scope of this task, there are CAN* fields in the _file and _field tables that define user access rights on the table/field level. I understand that these fields can be changed by the application code. Although these access rights have more or less equivalent counterparts in the relational databases supported by FWD I think that it will be more practical to enforce them in the FWD code (in the RecordBuffer and BufferFieldImpl). Indeed, the fields' getters/setters are invoked reflectively and the corresponding logic can be implemented in the corresponding proxy objects. The metadata can be kept in the cache so the additional runtime overhead should be reasonable.

• Can these fields be updated by the application code?
• How the values of these fields affect the implicit runtime behavior (I mean - the behavior which is not a result of the conditional logic in the application code)? To provide compatibility of the application logic based on the values of these fields it may be sufficient just to migrate the data as we do with "normal" tables.

Again, the most serious problem is the lack of documentation. Maybe it makes sense to start with use cases based on how these fields are used in customer applications?

Well, it seems that we need some changes in the conversion process. The SCHEMA meta tables should be included in the databases, not just standard. Like we do now with _User table.

Well, maybe Multi-tenancy and New VST Tables can be applicable.

I do not understand what the "conversion support" (#3814-7) means as it seems that it is sufficient just to add the names of these tables to the standard metadata section of the p2j.cfg.xml file.

Actually, this is something we want to get rid of. It seems a bit silly to have to write the table names in a configuration file instead of just converting everything found. I've discussed this previously with Eric. I understand that it would require TRPL rules to gather the list of all the referenced metadata tables (both VST and schema) at some point in the conversion process. I don't recall why this is needed, Eric can say.

However, the lack of a detailed description complicates both adding support for new fields and testing the support of the existing ones. Maybe we have something more informative than mentioned Progress documentation?

No, we don't have any better documentation.

The _area table is related to a physical layout of the 4GL database files and cannot have any reasonable counterpart in FWD.

I understand the concern here. Even in such a case, it almost always makes sense to provide some kind of "mocked" support for such things. The reason is simple: every time we add a requirement that customer code must be rewritten by hand before it can convert, we are adding a great deal of friction to the process. This make it more effort in the conversion project and it makes it harder to succeed by adding tasks that could be avoided.

Of course, some usage may still need to be rewritten. But perhaps we can handle a large number of the cases with a mocked version. Often, this kind of usage is to report on the status of the environment for diagnosis purposes. If the reports are less meaningful, that is OK. The customer can always rewrite those later or replace them using some pre-built tools. Avoiding the hard requirement for manual rewriting is critical.

Please plan for at least a mocked version of all of this, even if we don't have a meaningful backing that makes sense.

I was unable to find any information about the _tenant table. It seems also that FWD does not support multi-tenant databases so far.

We are not planning to support it right now. Our customers which use this feature in OpenEdge have found it to be completely unstable. When a multi-tenant database crashes, which may be once a week, all tenants are down until the system is recovered. The OE support lacks even basic tools for managing tenants. For example, there is no built-in way to import/export the data for just one tenant. It is a complete mess and these customers are all trying to leave this behind in moving to FWD.

Perhaps someday we will implement such support, but not now. Anyway, we will need some mocked support here.

Maybe it makes sense to start with use cases based on how these fields are used in customer applications?

Yes, agreed.

Looking at the content of the _Database-Feature VST from the sample customer environment I do not think that it can have a meaningful counterpart in FWD:
...
Well, maybe Multi-tenancy and New VST Tables can be applicable.

I think there is no reason we can't offer the full table here. It is completely correct to report 0 for OpenEdge replication. And I imagine that we do in fact support things like 64-bit sequences etc... As with the other things in our list, we must map things as possible to real concepts in our FWD application server or our database support and anything else that does not fit gets mocked with our best possible understanding of the "correct" answer.

Greg Shah wrote:

I do not understand what the "conversion support" (#3814-7) means as it seems that it is sufficient just to add the names of these tables to the standard metadata section of the p2j.cfg.xml file.

Actually, this is something we want to get rid of. It seems a bit silly to have to write the table names in a configuration file instead of just converting everything found. I've discussed this previously with Eric. I understand that it would require TRPL rules to gather the list of all the referenced metadata tables (both VST and schema) at some point in the conversion process. I don't recall why this is needed, Eric can say.

However, the lack of a detailed description complicates both adding support for new fields and testing the support of the existing ones. Maybe we have something more informative than mentioned Progress documentation?

No, we don't have any better documentation.

The _area table is related to a physical layout of the 4GL database files and cannot have any reasonable counterpart in FWD.

I understand the concern here. Even in such a case, it almost always makes sense to provide some kind of "mocked" support for such things. The reason is simple: every time we add a requirement that customer code must be rewritten by hand before it can convert, we are adding a great deal of friction to the process. This make it more effort in the conversion project and it makes it harder to succeed by adding tasks that could be avoided.

Of course, some usage may still need to be rewritten. But perhaps we can handle a large number of the cases with a mocked version. Often, this kind of usage is to report on the status of the environment for diagnosis purposes. If the reports are less meaningful, that is OK. The customer can always rewrite those later or replace them using some pre-built tools. Avoiding the hard requirement for manual rewriting is critical.

Please plan for at least a mocked version of all of this, even if we don't have a meaningful backing that makes sense.

I was unable to find any information about the _tenant table. It seems also that FWD does not support multi-tenant databases so far.

We are not planning to support it right now. Our customers which use this feature in OpenEdge have found it to be completely unstable. When a multi-tenant database crashes, which may be once a week, all tenants are down until the system is recovered. The OE support lacks even basic tools for managing tenants. For example, there is no built-in way to import/export the data for just one tenant. It is a complete mess and these customers are all trying to leave this behind in moving to FWD.

Perhaps someday we will implement such support, but not now. Anyway, we will need some mocked support here.

Maybe it makes sense to start with use cases based on how these fields are used in customer applications?

Yes, agreed.

Looking at the content of the _Database-Feature VST from the sample customer environment I do not think that it can have a meaningful counterpart in FWD:
...
Well, maybe Multi-tenancy and New VST Tables can be applicable.

I think there is no reason we can't offer the full table here. It is completely correct to report 0 for OpenEdge replication. And I imagine that we do in fact support things like 64-bit sequences etc... As with the other things in our list, we must map things as possible to real concepts in our FWD application server or our database support and anything else that does not fit gets mocked with our best possible understanding of the "correct" answer.

Got it. Thank you. I will start with the mock implementation of the VSTs mentioned in this task. I think it makes sense to do it data-driven (at least partially) to avoid coding when a new VST support will be needed.

Greg/Igor: a VST can be used from dynamic queries, too. And this will leave nothing for us to determine via the standard TRPL rules.

So we still need a way to let the conversion know what the VSTs are.

We can leave behind a conversion artifact for this, since the dynamic queries are only processed at runtime this should work.

At this moment I'm using the current approach based on metadata in p2j.cfg.xml. I believe that it will be easy to change when a decision will be made regarding another way.

Igor Skornyakov wrote:

Well, it seems that we need some changes in the conversion process. The SCHEMA meta tables should be included in the databases, not just standard. Like we do now with _User table.

Please provide the reasoning behind this conclusion.

Eric Faulhaber wrote:

Igor Skornyakov wrote:

Well, it seems that we need some changes in the conversion process. The SCHEMA meta tables should be included in the databases, not just standard. Like we do now with _User table.

Please provide the reasoning behind this conclusion.

Well, for example, _file and _field contain information about database tables and fields. Obviously in the situation of multiple databases is it impossible to avoid name conflicts. This is the reason for me to keep these tables in the database they describe. Please note also that databases can be connected and disconnected dynamically. The same is applicable to e.g. _Index-Field table, the tables of the AUTH category (_sec-authentication-domain, _sec-authentication-system ... ) and of the PHY_ST category (such as Area) for which we have to provide at least mock support if I understood Greg correctly.

Igor, AFAIK the meta database in FWD is a separate H2 in-memory database, for each physical database. So even if the schema is the same, the actual data is different.

See MetadataManager.prepareDatabase as it is called from DatabaseManager.initialize.

Constantin Asofiei wrote:

Igor, AFAIK the meta database in FWD is a separate H2 in-memory database, for each physical database. So even if the schema is the same, the actual data is different.

See MetadataManager.prepareDatabase as it is called from DatabaseManager.initialize.

Really? I've not realized this. In any case, the content of the non-VST system tables should be really persistent and survive the application restart. What is the problem to keep these tables in the database as we do with the _User table? This will at least reduce the startup time.
Thank you.

I've not realized this. In any case, the content of the non-VST system tables should be really persistent and survive the application restart. What is the problem to keep these tables in the database as we do with the _User table?

At least for tables such as file_ or field_, these must always represent the latest structure of the database. If we persist them at some point, then we would need to drop/refresh the table every time DDL is executed. Since the admin can (and does) execute DDL whenever they want, we would certainly get out of sync with reality.

Instead, we calculate this at server startup so that it is always accurate. We also live with the concept that one may not randomly apply DDL while the server is running. Note that this will change shortly because of #3912.

Anyway, it seems a bad idea to persist such data.

Greg Shah wrote:

I've not realized this. In any case, the content of the non-VST system tables should be really persistent and survive the application restart. What is the problem to keep these tables in the database as we do with the _User table?

At least for tables such as file_ or field_, these must always represent the latest structure of the database. If we persist them at some point, then we would need to drop/refresh the table every time DDL is executed. Since the admin can (and does) execute DDL whenever they want, we would certainly get out of sync with reality.

Instead, we calculate this at server startup so that it is always accurate. We also live with the concept that one may not randomly apply DDL while the server is running. Note that this will change shortly because of #3912.

Anyway, it seems a bad idea to persist such data.

I understand this point. Please note however that there are many undocumented fields even in _file and _field tables. As we discussed before, it makes sense to analyze how the customer application uses these (and other) tables. I can easily imagine the situation when we'll be able to provide compatibility with 4GL for the application if we will just import the content of (most) non-VST tables from the 4GL database. I've written many database applications and can hardly imagine the situation when the application logic depends on the dynamically changed schema changes in any significant way. (I mean - by analyzing the database metadata changes). Of course I understand that my personal experience is limited.

I can easily imagine the situation when we'll be able to provide compatibility with 4GL for the application if we will just import the content of (most) non-VST tables from the 4GL database. I've written many database applications and can hardly imagine the situation when the application logic depends on the dynamically changed schema changes in any significant way. (I mean - by analyzing the database metadata changes). Of course I understand that my personal experience is limited.

In #3912 we are seeing this exact requirement. They have 4GL code which manipulates the database schema to create tables/indexes from 4GL code by editing the metadata itself. It must be "live" data. Although I'm not asking you to implement this approach (yet :)), the idea to import such hard coded data from the 4GL is taking us further away from that requirement.

We also have existing applications which use these tables for things such as data export. It will not work to keep reporting the old 4GL data. We can't go down this path.

We already have quite a bit of these tables implemented in a somewhat compatible form. Please focus on incrementally improving our support in this area. Yes, it will require doing some testing to determine the content of some fields. And it will require looking at how the 4GL code uses those fields.

Greg Shah wrote:

I can easily imagine the situation when we'll be able to provide compatibility with 4GL for the application if we will just import the content of (most) non-VST tables from the 4GL database. I've written many database applications and can hardly imagine the situation when the application logic depends on the dynamically changed schema changes in any significant way. (I mean - by analyzing the database metadata changes). Of course I understand that my personal experience is limited.

In #3912 we are seeing this exact requirement. They have 4GL code which manipulates the database schema to create tables/indexes from 4GL code by editing the metadata itself. It must be "live" data. Although I'm not asking you to implement this approach (yet :)), the idea to import such hard coded data from the 4GL is taking us further away from that requirement.

We also have existing applications which use these tables for things such as data export. It will not work to keep reporting the old 4GL data. We can't go down this path.

We already have quite a bit of these tables implemented in a somewhat compatible form. Please focus on incrementally improving our support in this area. Yes, it will require doing some testing to determine the content of some fields. And it will require looking at how the 4GL code uses those fields.

I see. Thank you.

Greg Shah wrote:

At least for tables such as file_ or field_, these must always represent the latest structure of the database. If we persist them at some point, then we would need to drop/refresh the table every time DDL is executed. Since the admin can (and does) execute DDL whenever they want, we would certainly get out of sync with reality.

Instead, we calculate this at server startup so that it is always accurate.

Unless I'm misunderstanding your statement or someone has implemented something completely different than the original implementation since trunk rev 11384, no, this is not what happens. This information is captured at conversion only. It is NOT calculated at server startup. It is only read at server startup from an XML file created during schema conversion.

What would be calculated from the migrated database? This is legacy metadata... the legacy names of fields and tables, column names, validation expressions and messages, etc. Some things could be gleaned from RDBMS system tables in a dialect-specific way (e.g., mandatory fields), but most of this data is static from conversion of the 4GL schema.

When you add a column or an index post-conversion, it must be represented in the metadata. Relying upon static definitions from 4GL is insufficient.

Greg Shah wrote:

When you add a column or an index post-conversion, it must be represented in the metadata. Relying upon static definitions from 4GL is insufficient.

But I believe that DDL operations will be rare and affect only a tiny part of the schema. Why not keep the metadata in the database as 4GL does? It is possible of course that the application will crash in the middle of the metadata update after DDL operation. But this can be solved by persisting the information about the executed DDL operation in a separate table before executing the DLL. In this case, at the startup, we'll need to update only a few metadata records at most. The customer databases I've seen contain a lot of objects and this can substantially reduce both the startup time and memory overhead.

Why not keep the metadata in the database as 4GL does?

Just because the 4GL represents these as tables, doesn't mean that there are actual tables in the database which store this stuff. Who knows what their implementation is under the covers.

This information can be inspected using SQL. Why would we want to have a potentially stale set of data when it can be inspected at any time from the database itself?

And we do not want to force the customer to write extra SQL to maintain these unnecessary tables. It adds effort. It adds points of failure. It is error prone.

And we MUST make this dynamic ANYWAY as noted above, for #3912. This isn't optional.

The customer databases I've seen contain a lot of objects and this can substantially reduce both the startup time and memory overhead.

This is not worth the costs of management and the potential problems that come with mistakes. Every time we add extra steps in having to manage the installation, we make it harder and more expensive for our customers to succeed. And we make it more likely there is downtime and extra effort in debugging.

Anyway, as noted above we cannot live with a static representation.

Greg Shah wrote:

Why not keep the metadata in the database as 4GL does?

Just because the 4GL represents these as tables, doesn't mean that there are actual tables in the database which store this stuff. Who knows what their implementation is under the covers.

If I understand this document https://pugchallenge.org/downloads2015/448_schema_tables_v04.pdf correctly the schema tables are indeed "real" tables.

Igor,

On a project, we had to include _connect in the metadata. At that point, we received java.lang.ClassNotFoundException: ..._meta.impl.MetaMyconnectionImpl.

We added _myconnection, and now we are receiving: java.lang.NoSuchMethodException: ...dmo._meta.impl.MetaMyconnectionImpl.setMyconnTenantid(com.goldencode.p2j.util.NumberType)

Is this in your scope, such that we would get full support of this class? We are working with 4231b branch.

Roger Borrello wrote:

Igor,
On a project, we had to include _connect in the metadata. At that point, we received java.lang.ClassNotFoundException: ..._meta.impl.MetaMyconnectionImpl.
We added _myconnection, and now we are receiving: java.lang.NoSuchMethodException: ...dmo._meta.impl.MetaMyconnectionImpl.setMyconnTenantid(com.goldencode.p2j.util.NumberType)
Is this in your scope, such that we would get full support of this class? We are working with 4231b branch.

I will answer this:
there are a few versions of standard.df in use. Depending on the version of the OE they were exported, the tenant-related fields may or not may be present. You need to reconvert using the newer standard.df otherwise disable tenant-related fields from the MinimalMyconnection class. There is already some redmine task open for generally fixing this kind of issue by replacing the standard.df.

Ovidiu Maxiniuc wrote:

I will answer this:
there are a few versions of standard.df in use. Depending on the version of the OE they were exported, the tenant-related fields may or not may be present. You need to reconvert using the newer standard.df otherwise disable tenant-related fields from the MinimalMyconnection class. There is already some redmine task open for generally fixing this kind of issue by replacing the standard.df.

Thank you, Ovidiu! Is that #3757?

It's #4155.
I was looking for #3757 several days ago and for some reasons I could not reach it. Thanks for finding it. It contains some changes and discussions which may be useful for Igor.

• The most notable thing is that my code uses LegacyField annotation which seems to be replaced Property annotation in 4011a. I will isolate the corresponding logic in a separate method so it can be easily replaced.
• I've introduced a SystemTable enum which describes the 4GL system tables (as of Progress 11.6.3). In particular, it has a flag, which specifies that the corresponding table is a part of the database (not a separate h2 metadatabase). Instead of a check for a specific table name in the MetadataManager.initialize() I check the value of this flag. At least _sec_authentication_domain and _sec_authentication_system tables should be a part of the database (at least my changes to the security are based on this assumption).
• I do not use xxx.meta.xml. At this moment there is an additional flag of the SystemTable enum which enables the table population from this file for a specific table, but I think that this file should be obsolete. Indeed, the tables which are not VSTs and supposed to have static content, such as ones from the AUTH category is natural to keep in the main database (as _user, _sec_authentication_domain, and _sec_authentication_system tables). The tables with "live" content (such as _file and _field tables) should be populated at startup. BTW: the current implementation on the xxx.meta.xml - based population of the e.g. _file table is incorrect as it doesn't include metatables.
• I suggest getting rid of the metadata section of the p2j.cf.xml. At this moment if we remove e.g. _index from this section it results in a TypeNotPresentException at the server startup. It seems more logical to include all system tables which are currently supported by FWD and leave metadata section only to specify the name of the .df file if required. Another solution is to enforce the presence of mandatory system tables even if they are not specified in the p2j.cf.xml.

I've made MetadataManager.initialize() implementation as close to one in 4011a as possible. Both implementations are now essentially the same but I use DMOIndex (with small add-ones) instead of DmoMetadataManager which is used in 4011a

Igor Skornyakov wrote:

I do not see any potential conflicts between what I see in 4011a and my changes in 4231b. Some changes will be required of course but it seems that they will be local and minor.

• The most notable thing is that my code uses LegacyField annotation which seems to be replaced Property annotation in 4011a. I will isolate the corresponding logic in a separate method so it can be easily replaced.

Good.

• I've introduced a SystemTable enum which describes the 4GL system tables (as of Progress 11.6.3). In particular, it has a flag, which specifies that the corresponding table is a part of the database (not a separate h2 metadatabase). Instead of a check for a specific table name in the MetadataManager.initialize() I check the value of this flag. At least _sec_authentication_domain and _sec_authentication_system tables should be a part of the database (at least my changes to the security are based on this assumption).

I agree that a check for a specific table name (I think _file is used) is not the best. It is too arbitrary. I don't know anything about the security related tables, so I can't comment on whether these should be in the primary database or not.

• I do not use xxx.meta.xml. At this moment there is an additional flag of the SystemTable enum which enables the table population from this file for a specific table, but I think that this file should be obsolete. Indeed, the tables which are not VSTs and supposed to have static content, such as ones from the AUTH category is natural to keep in the main database (as _user, _sec_authentication_domain, and _sec_authentication_system tables). The tables with "live" content (such as _file and _field tables) should be populated at startup. BTW: the current implementation on the xxx.meta.xml - based population of the e.g. _file table is incorrect as it doesn't include metatables.

• I suggest getting rid of the metadata section of the p2j.cf.xml. At this moment if we remove e.g. _index from this section it results in a TypeNotPresentException at the server startup. It seems more logical to include all system tables which are currently supported by FWD and leave metadata section only to specify the name of the .df file if required. Another solution is to enforce the presence of mandatory system tables even if they are not specified in the p2j.cf.xml.

Greg and I have been discussing getting rid of the metadata section from p2j.cfg.xml, primarily because it is error prone, in that people are likely to either put too little or too much in that section, compared to what an application actually uses. The idea is to let the conversion identify and enable those actually used by an application. Greg, do we already have an issue for this? I cannot find one ATM.

I do not want to include all system tables supported by FWD, if an application does not use them. This can result in a performance drag on the system, in that the maintenance of the state of these tables consumes resources, which we only want to do if they are needed.

do we already have an issue for this?

Not that I recall.

I do not want to include all system tables supported by FWD, if an application does not use them.

This is a problem that will hinder the wide-spread use of FWD. Not only does this idea add friction to the system, but there are ways of using these tables that cannot be planned for in advance. For example, dynamic database can use these at any time. Long term, these all need to be well supported, even if the result for a given column is more static and less useful than in the 4GL.

The sooner we solve this (rather than putting it off for later), the better. Otherwise we (or the users of FWD) always have to put extra work in for metadata support in every project...

This can result in a performance drag on the system, in that the maintenance of the state of these tables consumes resources, which we only want to do if they are needed.

While I understand the concern, we would be better off leaving the support there but not initializing it or tracking any non-essential state until it is requested. I'm sure we can find a way to mitigate this.

For example, I would much prefer if the _file and _field tables were constructed "on the fly" from the RDBMS system catalog tables + annotations from the DMOs (where available). Most applications use metadata rarely, so tracking everything all the time makes no sense.

Greg Shah wrote:

This can result in a performance drag on the system, in that the maintenance of the state of these tables consumes resources, which we only want to do if they are needed.

While I understand the concern, we would be better off leaving the support there but not initializing it or tracking any non-essential state until it is requested. I'm sure we can find a way to mitigate this.

For example, I would much prefer if the _file and _field tables were constructed "on the fly" from the RDBMS system catalog tables + annotations from the DMOs (where available). Most applications use metadata rarely, so tracking everything all the time makes no sense.

I've implemented (optional) timing of the population of system tables, so we can have a better idea of the actual overhead.

Populating _File system table based only on database metadata (provided by Connection.getMetaData()) looks to be too limited. Indeed, it provides only the table names and we need to use FWD metadata to populate at least the fields which are currently populated based on xxx.meta.xml file. The situation is even worse for the _Field system table. Just to retrieve a list of table columns we have to get the ResultSet metadata for something like SELECT * FROM <table>
WHERE 0 = 1 which can be very expensive. And, of course, we can get very little information from this metadata (for example we cannot populate _format field in _Field table)

I've implemented the logic, based on the FWD metadata (DMO object annotations). I'm not sure if this is the right approach if our objective is to have "live" data, but at least I was able to add system tables' metadata to the _File table. I've noticed that the current data for the _User table in the xxx.meta.xml file is not correct: the value of the _Db-recid field is always 4032 in 4GL for all system tables while FWD assigns something looking like a generated unique id; _Tbl-Type is always S (not T) for all system tables and _Template is always zero (not a negative number).

After testing my changes with a large customer application and adding Javadocs and headers I think it will be safe to commit it to 4231b (the branch I'm working with). I think I will do it tomorrow.

At this moment I'm thinking about the efficient algorithm for populating fields which are actually foreign keys (such as _Prime-Index in _File or __File-recid in _Field). I do not like the idea to solve this problem by populating _File and all is logical "child" such as _Field, _Index, and _Index-Field in one iteration over tables' set as it results in a too complicated code. From the other side if e.g. DatabaseManager.getDatabaseTables method doesn't produce an unacceptable memory overhead, maybe using HashMap and a proper ordering of tables to be populated can be acceptable.

Igor Skornyakov wrote:

[...] and _Template is always zero (not a negative number).

I do not remember exactly, but I remember this might have been done intentionally. Please read #3145.

Ovidiu Maxiniuc wrote:

Igor Skornyakov wrote:

[...] and _Template is always zero (not a negative number).

I do not remember exactly, but I remember this might have been done intentionally. Please read #3145.

Thank you, Ovidiu. I will check.

Igor Skornyakov wrote:

[...]
I've noticed that the current data for the _User table in the xxx.meta.xml file is not correct: the value of the _Db-recid field is always 4032 in 4GL for all system tables while FWD assigns something looking like a generated unique id; _Tbl-Type is always S (not T) for all system tables and _Template is always zero (not a negative number).

As Ovidiu noted, the use of negative numbers for template keys is intentional and was an implementation choice. We do not attempt to match the internal RECID (or ROWID) representation of legacy records (not only metadata, but with all database records). Our primary key is always a Long (positive for regular records, negative for template records), whereas this does not hold for Progress RECID and ROWID values. This is an intentional deviation. So long as a foreign key reference points to the correct record in the associated primary table, that is what we care about.

Eric Faulhaber wrote:

Igor Skornyakov wrote:

[...]
I've noticed that the current data for the _User table in the xxx.meta.xml file is not correct: the value of the _Db-recid field is always 4032 in 4GL for all system tables while FWD assigns something looking like a generated unique id; _Tbl-Type is always S (not T) for all system tables and _Template is always zero (not a negative number).

As Ovidiu noted, the use of negative numbers for template keys is intentional and was an implementation choice. We do not attempt to match the internal RECID (or ROWID) representation of legacy records (not only metadata, but with all database records). Our primary key is always a Long (positive for regular records, negative for template records), whereas this does not hold for Progress RECID and ROWID values. This is an intentional deviation. So long as a foreign key reference points to the correct record in the associated primary table, that is what we care about.

Got it. Thank you!

I'm a little bit confused with the discussion in #3145. At the bottom line - is it OK to set the value of the _Template field in the _File system table to -1 for all types of tables?
Thank you.

Igor Skornyakov wrote:

I'm a little bit confused with the discussion in #3145. At the bottom line - is it OK to set the value of the _Template field in the _File system table to -1 for all types of tables?

In this case how would you make the difference between the template records belonging to different tables?

Ovidiu Maxiniuc wrote:

Igor Skornyakov wrote:

I'm a little bit confused with the discussion in #3145. At the bottom line - is it OK to set the value of the _Template field in the _File system table to -1 for all types of tables?

In this case how would you make the difference between the template records belonging to different tables?

I've seen somewhere that in 4GL the semantics of the _Template field is a (fictitious) offset of the template record which is used when a new uninitialized record is added. In #3145-3 you've suggested a "-1" value, but in a sample xxx.meta.xml file it seems to be a negated value of the id field (while the id is long and the _Template is int). In 4GL _Template is always 0 for system tables and some positive number (in many cases it is pretty large) for normal tables.
This is why I'm confused. I do not understand your question as the _Template field is a part of the _File table record and it is always clear what table it is about.
Thank you.

The _File table population for a large customer application (both system and user tables) takes ~100 ms in my environment.
To resolve cross-references (foreign keys) in the most efficient way I'm working now on population _File, _Field, _Index, and _Index-Field tables in a single iteration over the tables' set.

I've finished the population of the _File, _Field, _Index, and _Index-Field system tables base on DMO reflection. The large customer application starts OK with my changes and the population of the tables takes less than a second on my machine.
The only remaining thing is a _File._Template field correct value, see #3814-(52-54). At this moment I'm analyzing how this value is used in FWD. Afer appropriate fixes will be made (if one will be required, it should be really minor changes) I can commit my changes to 4321b (a branch I'm working with now).

Igor Skornyakov wrote:

I've finished the population of the _File, _Field, _Index, and _Index-Field system tables base on DMO reflection. The large customer application starts OK with my changes and the population of the tables takes less than a second on my machine.
The only remaining thing is a _File._Template field correct value, see #3814-(52-54). At this moment I'm analyzing how this value is used in FWD. Afer appropriate fixes will be made (if one will be required, it should be really minor changes) I can commit my changes to 4321b (a branch I'm working with now).

I've fixed the _File._Template field value to be compatible with the current FWD approach and added population of the MetadataManager.templateRecords map not from ?meta.xml file.
Is it OK to commit my changes to 4321b or it is safer to use another branch?
Thank you.

I think 4231b is fine.

Greg Shah wrote:

I think 4231b is fine.

Committed to 4231b revision 11549.

From an email discussion:

Constantin:

BTW, leaving an empty metadata in p2j.cfg.xml works in FWD, so the meta tables are still optional. The idea is: at least I want to enable these only if I work with them. Otherwise, there will be additional overhead for conversion/runtime which I don't need.

Igor:

This is strange - I've experienced errors on server start w/o some tables. See #3814-42 and Eric's comment in #3814-44. May be I've missed something. In any case my changes to not introduce any new mandatory tables (I hope).

Constantin:

That is because some meta tables are dependent upon each other. We should document this and maybe hard-code (or throw an error, log or something else) when these meta-tables are being read/used by the conversion process, to avoid runtime problems, plus a new long-running conversion for the project after we fixed the metadata section in p2j.cfg.xml.

Igor:

BTW: I'm planning to enforce adding "minimal" set of meta tables regardless of the p2j.cfg.xml settings.

I'm not OK with this. Meta-tables should be optional. It adds overhead to other projects which may not need them.

Constantin Asofiei wrote:

I'm not OK with this. Meta-tables should be optional. It adds overhead to other projects which may not need them.

The overhead with the creation/population of the "minimal" set of meta-tables is negligible. And I still do not understand how to avoid the issue with missed ones described in #3414-42.

Igor Skornyakov wrote:

The overhead with the creation/population of the "minimal" set of meta-tables is negligible.

What are these tables exactly? Are they read-only, once populated?

And I still do not understand how to avoid the issue with missed ones described in #3414-42.

You mean the _index table question? _connect and _myconnection looks like need to be both specified if one is specified. For _index I assume, that too is part of a larger set of meta tables which always need to be specified.

My idea was for the conversion rules to do some sanity checks: if you specified one or more table which is part of a set of dependent tables (which can't work unless all the dependent tables are specified), then abend and log that all tables in that set need to be specified. So, _connect and _myconnection is a set of dependent tables, _index, _file and others may be another set, _sec-authentication-domain and _sec-authentication-system another set, etc. Does this make sense?

We really only have one project where we made the effort to eliminate all metadata usage. That may not ever happen again. So we can expect that all but that one project will need metadata.

On the other hand, the usage of metadata for the average user's processing is typically very, very small. I agree that overhead should be minimized, but I think this is a core design requirement rather than an optional thing for some projects. In other words, we should eliminate any unnecessary metadata processing and defer it to the time it is needed even if it is more work at that time. The bias in the solution should be to make the server run with as close to zero metadata overhead as is possible for all applications.

In the short term, we need to eliminate the manual configuration of metadata at conversion time. As noted before this is very error prone and has caused real problems MANY times.

Long term, metadata itself needs to just be part of the FWD server, available fully for all applications. It should NOT be part of the application-level conversion. Converting it with the application is an unnecessary complexity and set of failure conditions. We have always seen that the 4GL itself only ever adds new metadata in later versions, it does not take away metadata (at least not yet). This means that later metadata has always been able to work with applications written to older versions.

In addition, consider #4155. We are moving to eliminate even using the input of a .df. Although that task is written to just change the very early part of the process, it starts us moving in the right long term direction.

My idea was for the conversion rules to do some sanity checks: if you specified one or more table which is part of a set of dependent tables (which can't work unless all the dependent tables are specified), then abend and log that all tables in that set need to be specified. So, _connect and _myconnection is a set of dependent tables, _index, _file and others may be another set, _sec-authentication-domain and _sec-authentication-system another set, etc. Does this make sense?

At this point I see no reason to continue with the configuration at all. Why are we not just converting all of them? This attempt to make each converted application unique is the wrong direction in my opinion.

Greg Shah wrote:

At this point I see no reason to continue with the configuration at all. Why are we not just converting all of them? This attempt to make each converted application unique is the wrong direction in my opinion.

I totally agree with the idea to convert all system tables. After all, in 4GL they exist in every database. There are only 81 system tables (including VSTs) as of Progress 11.6.3. Both conversion and initialization overhead will be minimal.

Igor Skornyakov wrote:

I totally agree with the idea to convert all system tables. After all, in 4GL they exist in every database. There are only 81 system tables (including VSTs) as of Progress 11.6.3.

Yes, this can work.

What about the _lock meta-table? Currently FWD enables this only if _lock is explicitly specified.

Also, can we have a flag in p2j.cfg.xml to avoid metadata conversions? it can default to true, to always convert metadata. Or a mode in the ConversionDriver, to exclude the metadata.

Constantin Asofiei wrote:

What about the _lock meta-table? Currently FWD enables this only if _lock is explicitly specified.

I've not analyzed the logic related to the _lock meta-table yet.

Also, can we have a flag in p2j.cfg.xml to avoid metadata conversions? it can default to true, to always convert metadata. Or a mode in the ConversionDriver, to exclude the metadata.

This can be done of course if you believe that it makes sense to complicate the conversion/initialization logic to get a minor performance improvement.

Igor Skornyakov wrote:

This can be done of course if you believe that it makes sense to complicate the conversion/initialization logic to get a minor performance improvement.

It adds up when doing development work, both at conversion and runtime.

Constantin Asofiei wrote:

Igor Skornyakov wrote:

This can be done of course if you believe that it makes sense to complicate the conversion/initialization logic to get a minor performance improvement.

It adds up when doing development work, both at conversion and runtime.

OK. It should be easy to add a configuration option for disabling metadata processing.

Please post the detailed status of this task. I would like to know:

• In regard to the list of work in #3814-1 and #3814-7:
  • Which items are completely finished?
  • Which items are being worked on now?
• Have you checked in your testcases which are being used to figure out the behavior?
• How much time is needed to complete this task? I understand this is just an estimate.

Greg Shah wrote:

Please post the detailed status of this task. I would like to know:

• In regard to the list of work in #3814-1 and #3814-7:
  • Which items are completely finished?

I've implemented a population of the _File, _Field, _Index, and _Index-Field tables, based on the reflection-based analysis of tables DMO. Comparing with existing FWD support data for system tables are added, but the fields are the same.

• Which items are being worked on now?

I'm adding additional fields to the tables mentioned above. At this moment I consider only the fields which can be populated based on the analysis of the .df files. It requires changes to the conversion. I understand that not all fields can be populated based on the .df files, an analysis of the tables' data can be required.

• Have you checked in your testcases which are being used to figure out the behavior?

I've used small ad hoc tests. For example, created tables using Data Dictionary and Data Administractor and dumped it. Or executed simple script which displays the values of the fields of the system tables. I plan to create a more usable test with corresponding .df and .d files and commit it.

• How much time is needed to complete this task? I understand this is just an estimate.

It is difficult to say. The system tables are very poorly documented and contain a lot of fields. I think it can take a week or two.

Regarding enabling all metadata in all applications, I have no problem doing this eventually, but we do not yet have the optimized implementations of the "active" metadata tables (_lock, _connect, etc.). Until we have those, it WILL add a lot of overhead (especially _lock, which is extremely active). We should NOT enable them all before we have those optimized implementations.

Eric Faulhaber wrote:

Regarding enabling all metadata in all applications, I have no problem doing this eventually, but we do not yet have the optimized implementations of the "active" metadata tables (_lock, _connect, etc.). Until we have those, it WILL add a lot of overhead (especially _lock, which is extremely active). We should NOT enable them all before we have those optimized implementations.

Sorry Eric, I do not understand your point. How additional metadata tables can add overhead if they will not be used by the application? At this moment we are talking only about creating them.

Igor Skornyakov wrote:

Eric Faulhaber wrote:

Regarding enabling all metadata in all applications, I have no problem doing this eventually, but we do not yet have the optimized implementations of the "active" metadata tables (_lock, _connect, etc.). Until we have those, it WILL add a lot of overhead (especially _lock, which is extremely active). We should NOT enable them all before we have those optimized implementations.

Sorry Eric, I do not understand your point. How additional metadata tables can add overhead if they will not be used by the application? At this moment we are talking only about creating them.

Consider the _lock table as an example. With the current implementation, the meta_lock table is constantly being updated with the latest lock states, whether or not an application ever queries the table. Every lock state change causes a database update. This effort consumes a lot of resources on a server, which is entirely wasted if application logic never accesses the table.

We have plans to implement a more passive version of this and other "live" metadata tables which do not consume as much resource, but until those are available, we should not enable those tables, if we know an application does not use them.

This is just a statement of the timing of enabling all metadata, not about whether we should.

Eric Faulhaber wrote:

Consider the _lock table as an example. With the current implementation, the meta_lock table is constantly being updated with the latest lock states, whether or not an application ever queries the table. Every lock state change causes a database update. This effort consumes a lot of resources on a server, which is entirely wasted if application logic never accesses the table.

We have plans to implement a more passive version of this and other "live" metadata tables which do not consume as much resource, but until those are available, we should not enable those tables, if we know an application does not use them.

This is just a statement of the timing of enabling all metadata, not about whether we should.

Thank you for the clarification. I have to look at the LockTableUpdater more thoroughly.

I'm trying to add conversion support for string attributes for tables and fields (*-SA properties).
I've added the definitions of the keywords for these properties tp progress.g, e.g.

         new Keyword("valmsg-sa"         ,   0, KW_VALMSG_SA   , false)

table_props   
   :
      { astFactory.makeASTRoot(currentAST, #[PROPERTIES]); }
      (
           area
         | label
         | label_sa
         | description
         | valExp
         | valMsg
         | valMsg_sa
         | frozen
         | hidden
         | dumpName
         | category
         | permissions
         | table_trigger
         | unknownOption!

valMsg_sa!
   :
      m:KW_VALMSG_SA v:STRING
      {
         ## = #([VALMSG_SA, "valmsg-sa"], ([EXPRESSION, "expression"], v));
         forceLineColumnNums(#m, ##);
      }
   ;

             <!-- graft "valmsg-sa" annotation -->
             <rule>copy.isAnnotation("valmsg-sa")
                <action>
                   tpl.graft('annotation_assign_string', null, tableAst,
                             'key', 'valmsg_sa',
                             'value', getNoteString("valmsg-sa"))
                </action>
             </rule>

Please post your changed versions of both schema.g and progress.g.

Some feedback:

1. Keyword token lengths are limited to KW_ + 8 characters. KW_VALMSG_SA would be too long. This is not a technical limitation, but a coding standard.

2. All the new artifical tokens for *_SA should be inside the BEGIN_SCHEMAKW/END_SCHEMAKW section. These are specific to schema processing (they are about "storage areas", I think).

3. Some problems are caused by incorrect definition of your keywords. This seems wrong in 3 ways:

new Keyword("column-label_sa" , 10, KW_COL_LAB_SA, true ),

• The text of the keyword us is "column-label-sa" not "column-label_sa".
• I don't think it is correct to set the abbreviation to 10 characters. As far as I know, there should be no abbreviation possible (set it to 0). By setting it to 10, you are conflicting with the abbreviation for "column-label" and this will cause problems.
• I don't know if this keyword is reserved or not. The keyword index doesn't list this keyword. So you would have to test it in some simple 4GL (e.g. def var column-label-sa as char.). That will tell you if the reserved keyword flag should be set to true or false. Making it reserved when it should not be reserved will cause issues.

For label-sa and help-sa, only the last item is an issue.

4. column-label-sa", @label-sa and help-sa keyword definitions should be moved into initializeSchemaKeywords() since these are specific to the schema processing.

5. Did you update rules/schema/fixups.xml to include new annotations copying? Search for kw_col_lab in there to see an example. Without that your code that does copy.isAnnotation("valmsg-sa") will not work.

Greg Shah wrote:

Some feedback:

1. Keyword token lengths are limited to KW_ + 8 characters. KW_VALMSG_SA would be too long. This is not a technical limitation, but a coding standard.

2. All the new artifical tokens for *_SA should be inside the BEGIN_SCHEMAKW/END_SCHEMAKW section. These are specific to schema processing (they are about "storage areas", I think).

3. Some problems are caused by incorrect definition of your keywords. This seems wrong in 3 ways:

new Keyword("column-label_sa" , 10, KW_COL_LAB_SA, true ),

• The text of the keyword us is "column-label-sa" not "column-label_sa".
• I don't think it is correct to set the abbreviation to 10 characters. As far as I know, there should be no abbreviation possible (set it to 0). By setting it to 10, you are conflicting with the abbreviation for "column-label" and this will cause problems.
• I don't know if this keyword is reserved or not. The keyword index doesn't list this keyword. So you would have to test it in some simple 4GL (e.g. def var column-label-sa as char.). That will tell you if the reserved keyword flag should be set to true or false. Making it reserved when it should not be reserved will cause issues.

For label-sa and help-sa, only the last item is an issue.

4. column-label-sa", @label-sa and help-sa keyword definitions should be moved into initializeSchemaKeywords() since these are specific to the schema processing.

5. Did you update rules/schema/fixups.xml to include new annotations copying? Search for kw_col_lab in there to see an example. Without that your code that does copy.isAnnotation("valmsg-sa") will not work.

Got it. Thanks a lot!

Feedback:

1. The artificial token definitions for KW_COL_LAB, KW_HELP and KW_LABEL should not be moved into the schema section. These are also real 4GL code keywords and must be put back where they were originally defined.

2. You left the new Keyword(... for column-label-sa, help-sa and label-sa in the regular code section. You copied them into the initializeSchemaKeywords() section, but they should have been moved instead of copied.

3. There are tabs used in the new token definitions which cause the indent to be incorrect.

4. schema.g is missing rules for KW_FMT_SA.

Please note, that the TYPE for the LABEL property is KW_LABEL, but is VALEXP for VALEXP (not KW_VALEXP)

Yes, that is how it was written before your changes and it is OK.

The validation message and expression can also appear in 4GL code. When it appears in code (instead of the schema), it appears as an expression which is not just a string. In order to make the structure of the AST the same as how it is done for code, the schema.g re-writes the AST to have an extra EXPRESSION node in between the VALMSG and the STRING. When that re-write is done, we are intentionally switching the token from KW_VALMSG to VALMSG which is just an artificial token with no matching keyword. This code in schema.g does this:

valMsg! (the ! tells ANTLR not to create an AST by default)

and this:

## = #([VALMSG, "valmsg"], ([EXPRESSION, "expression"], v));

manually creates the new AST in a manner that is compatible with our 4GL code matching.

What I'm doing wrong?

This is all intentional. I don't think you are doing anything wrong in this regard. Why do you think it is a problem?

Greg Shah wrote:

Feedback:

1. The artificial token definitions for KW_COL_LAB, KW_HELP and KW_LABEL should not be moved into the schema section. These are also real 4GL code keywords and must be put back where they were originally defined.

2. You left the new Keyword(... for column-label-sa, help-sa and label-sa in the regular code section. You copied them into the initializeSchemaKeywords() section, but they should have been moved instead of copied.

3. There are tabs used in the new token definitions which cause the indent to be incorrect.

4. schema.g is missing rules for KW_FMT_SA.

Fixed

Greg Shah wrote:

Please note, that the TYPE for the LABEL property is KW_LABEL, but is VALEXP for VALEXP (not KW_VALEXP)

Yes, that is how it was written before your changes and it is OK.

The validation message and expression can also appear in 4GL code. When it appears in code (instead of the schema), it appears as an expression which is not just a string. In order to make the structure of the AST the same as how it is done for code, the schema.g re-writes the AST to have an extra EXPRESSION node in between the VALMSG and the STRING. When that re-write is done, we are intentionally switching the token from KW_VALMSG to VALMSG which is just an artificial token with no matching keyword. This code in schema.g does this:

valMsg! (the ! tells ANTLR not to create an AST by default)

and this:

## = #([VALMSG, "valmsg"], ([EXPRESSION, "expression"], v));

manually creates the new AST in a manner that is compatible with our 4GL code matching.

What I'm doing wrong?

This is all intentional. I don't think you are doing anything wrong in this regard. Why do you think it is a problem?

Thank you for the clarification. I was not sure that I did everything right because I do not the expected results.

I see some strange situation:
There are two similar annotations for the field AST in the .schema file:

        <annotation datatype="java.lang.String" key="col_lab" value="f1CL"/>
        <annotation datatype="java.lang.String" key="col_lab-byrule" value="schema/fixups.xml:216"/>
        <annotation datatype="java.lang.String" key="col_lab_sa" value="T20"/>
        <annotation datatype="java.lang.String" key="col_lab_sa-byrule" value="schema/fixups.xml:216"/>

         <!-- graft "column-label" annotation -->
         <rule>sourceAst.isAnnotation("col_lab")
            <action>tmpString = sourceAst.getAnnotation('col_lab')</action>
            <action>
               tpl.graft('annotation_assign_string', null, fieldAst,
                         'key', 'columnLabel',
                         'value', tmpString)
            </action>
         </rule>

         <!-- graft "column-label-sa" annotation -->
         <rule>sourceAst.isAnnotation("col_lab_sa")
            <action>tmpString = sourceAst.getAnnotation('col_lab_sa')</action>
            <action>
               tpl.graft('annotation_assign_string', null, fieldAst,
                         'key', 'columnLabel_sa',
                         'value', tmpString)
            </action>
         </rule>

Post your dmo_common.rules.

Are the annotations correct (as you would expect) in the AST?

Please show the output jast for the example field.

Greg Shah wrote:

Please show the output jast for the example field.

      <ast col="5" id="21474837421" line="392" text="f1" type="FIELD_CHAR">
        <annotation datatype="java.lang.String" key="historical" value="f1"/>
        <annotation datatype="java.lang.String" key="description" value="f1 desc"/>
        <annotation datatype="java.lang.String" key="description-byrule" value="schema/fixups.xml:606"/>
        <annotation datatype="java.lang.String" key="format" value="x(8)"/>
        <annotation datatype="java.lang.String" key="format-byrule" value="schema/fixups.xml:626"/>
        <annotation datatype="java.lang.String" key="format_sa" value="T20"/>
        <annotation datatype="java.lang.String" key="format_sa-byrule" value="schema/fixups.xml:216"/>
        <annotation datatype="java.lang.String" key="init_sa" value="T20"/>
        <annotation datatype="java.lang.String" key="init_sa-byrule" value="schema/fixups.xml:216"/>
        <annotation datatype="java.lang.String" key="label" value="f1L"/>
        <annotation datatype="java.lang.String" key="label-byrule" value="schema/fixups.xml:216"/>
        <annotation datatype="java.lang.String" key="label_sa" value="L10"/>
        <annotation datatype="java.lang.String" key="label_sa-byrule" value="schema/fixups.xml:216"/>
        <annotation datatype="java.lang.Long" key="position" value="2"/>
        <annotation datatype="java.lang.String" key="position-byrule" value="schema/fixups.xml:685"/>
        <annotation datatype="java.lang.String" key="col_lab" value="f1CL"/>
        <annotation datatype="java.lang.String" key="col_lab-byrule" value="schema/fixups.xml:216"/>
        <annotation datatype="java.lang.String" key="col_lab_sa" value="T20"/>
        <annotation datatype="java.lang.String" key="col_lab_sa-byrule" value="schema/fixups.xml:216"/>
        <annotation datatype="java.lang.String" key="valexp" value="1 = 1"/>
        <annotation datatype="java.lang.String" key="valexp-byrule" value="schema/fixups.xml:591"/>
        <annotation datatype="java.lang.String" key="valmsg" value="Fld validation msg"/>
        <annotation datatype="java.lang.String" key="valmsg-byrule" value="schema/fixups.xml:567"/>
        <annotation datatype="java.lang.String" key="help" value="f1 help"/>
        <annotation datatype="java.lang.String" key="help-byrule" value="schema/fixups.xml:216"/>
        <annotation datatype="java.lang.String" key="help_sa" value="T20"/>
        <annotation datatype="java.lang.String" key="help_sa-byrule" value="schema/fixups.xml:216"/>
        <annotation datatype="java.lang.Long" key="order" value="10"/>
        <annotation datatype="java.lang.String" key="order-byrule" value="schema/fixups.xml:709"/>
        <annotation datatype="java.lang.Boolean" key="mandatory" value="true"/>
        <annotation datatype="java.lang.String" key="mandatory-byrule" value="schema/fixups.xml:714"/>
        <annotation datatype="java.lang.Long" key="peerid" value="34359738541"/>
        <annotation datatype="java.lang.String" key="peerid-byrule" value="schema/p2o.xml:1349"/>
        <ast col="3" id="21474837428" line="396" text="INITIAL" type="KW_INIT">
          <ast col="11" id="21474837429" line="396" text="&quot;initial&quot;" type="STRING"/>
        </ast>
        <ast col="3" id="21474837438" line="401" text="MAX-WIDTH" type="KW_MAX_WID">
          <ast col="13" id="21474837439" line="401" text="16" type="NUM_LITERAL"/>
        </ast>
        <ast col="3" id="21474837444" line="405" text="VALEXP" type="VALEXP">
          <annotation datatype="java.lang.String" key="original_text" value="1 = 1"/>
          <ast col="10" id="21474837445" line="405" text="1 = 1" type="STRING"/>
        </ast>
      </ast>

I think you have shown the 4GL AST. The code calling declareProperty is building a Java AST. Please show that Java AST which is the output.

Yes.

Greg Shah wrote:

Yes.

Well, as you can see the new properties were not added.

Put debug printf statements in the dmo_common.rules section for graft "column-label-sa" annotation. Either you are loading the wrong version of dmo_common.rules or the input AST is not what you think it is at that point in the code. You can use sourceAst.dumpTree() and copy.dumpTree() to get a rendering of the input ASTs.

Greg Shah wrote:

Put debug printf statements in the dmo_common.rules section for graft "column-label-sa" annotation. Either you are loading the wrong version of dmo_common.rules or the input AST is not what you think it is at that point in the code. You can use sourceAst.dumpTree() and copy.dumpTree() to get a rendering of the input ASTs.

Got it. Thank you. I'm quite sure that the version of the dmo_common.rules is correct as I've encountered errors that have gone after I've fixed my changes.

Debugging the conversion I've realized that I have to add support for the new table/field properties to the p2o.xml as well.

Added conversion support for the tables/fields string attributes (*-SA properties)
Committed to 4231b rev. 11577.

Added conversion support for the _Field.CAN-* fields and population of CAN_* and *-SA fields for _File/_Field tables.
Committed to 4231b rev. 11583.

1. restored _database-feature support previously implemented by Ovidiu
2. added the following _file fields:
   

   _file._can-create
   _file._can-write
   _file._can-read
   _file._can-delete
   _file._can-dump
   _file._can-load
   _file._file-label-sa
   _file._valmsg-sa
   _file._valexp
   

   
   According to the https://knowledgebase.progress.com/articles/Article/P51432
   

   The _File._ianum field contains the Storage Area number that a table was first created in.  After a table is created the _ianum field is not used again.  When a table is moved using the tablemove qualifier, _file._ianum is not updated to reflect the new location per design.
   

   
   This means that the _File._ianum is almost useless and has not any meaningful counterpart in FWD. I understand the same is applicable to the _index._ianum. I was unable to figure the semantics of the _file._fil-misc2. The algorithm for the _file._crc value calculation is undocumented and in any case, we cannot implement it w/o full support of other fields).
3. added the following _field fields:
   

   _field._can-read
   _field._can-write
   _field._col-label-sa
   _field._field-physpos
   _field._format-sa
   _field._help
   _field._help-sa
   _field._initial
   _field._initial-sa
   _field._label-sa
   _field._valmsg-sa
   

   
   The semantics of the remaining fields is unclear.

This means that the _File._ianum is almost useless and has not any meaningful counterpart in FWD. I understand the same is applicable to the _index._ianum.

We expect that some of these fields from #3814-1 have no good mapping in FWD. Please recall what I said in #3814-18:

I understand the concern here. Even in such a case, it almost always makes sense to provide some kind of "mocked" support for such things. The reason is simple: every time we add a requirement that customer code must be rewritten by hand before it can convert, we are adding a great deal of friction to the process. This make it more effort in the conversion project and it makes it harder to succeed by adding tasks that could be avoided.

Of course, some usage may still need to be rewritten. But perhaps we can handle a large number of the cases with a mocked version. Often, this kind of usage is to report on the status of the environment for diagnosis purposes. If the reports are less meaningful, that is OK. The customer can always rewrite those later or replace them using some pre-built tools. Avoiding the hard requirement for manual rewriting is critical.

Please plan for at least a mocked version of all of this, even if we don't have a meaningful backing that makes sense.

For such things, it is best to make a list of the ones that have no good meaning and find a simple way to make them safe/mocked. Usually this is just putting a default safe value into the field. Implement these safe values and post the list and move on.

My concern here is that it is taking a very long time to implement the support here. At the current pace, it may take months to complete this task. It needs to be done much sooner.

What can be done to move this along faster?

Greg Shah wrote:

This means that the _File._ianum is almost useless and has not any meaningful counterpart in FWD. I understand the same is applicable to the _index._ianum.

We expect that some of these fields from #3814-1 have no good mapping in FWD. Please recall what I said in #3814-18:

I understand the concern here. Even in such a case, it almost always makes sense to provide some kind of "mocked" support for such things. The reason is simple: every time we add a requirement that customer code must be rewritten by hand before it can convert, we are adding a great deal of friction to the process. This make it more effort in the conversion project and it makes it harder to succeed by adding tasks that could be avoided.

Of course, some usage may still need to be rewritten. But perhaps we can handle a large number of the cases with a mocked version. Often, this kind of usage is to report on the status of the environment for diagnosis purposes. If the reports are less meaningful, that is OK. The customer can always rewrite those later or replace them using some pre-built tools. Avoiding the hard requirement for manual rewriting is critical.

Please plan for at least a mocked version of all of this, even if we don't have a meaningful backing that makes sense.

For such things, it is best to make a list of the ones that have no good meaning and find a simple way to make them safe/mocked. Usually this is just putting a default safe value into the field. Implement these safe values and post the list and move on.

This is exactly whatI'm going to do. In particular, the ianum field support will be added together with the mock for the _Area.

My concern here is that it is taking a very long time to implement the support here. At the current pace, it may take months to complete this task. It needs to be done much sooner.

What can be done to move this along faster?

Regarding the undocumented fields of the _Field table, I think that it can be sufficient to set them to a "typical" value. I've looked at the dump of this table from the customer database in the VM I'm using for 4GL tests and it looks that most of them are either UNDEFINED or contain some "standard" value. I hope that this is the fact for most other undocumented fields, at least for the table of the SCHEMA category.

Regarding the undocumented fields of the _Field table, I think that it can be sufficient to set them to a "typical" value. I've looked at the dump of this table from the customer database in the VM I'm using for 4GL tests and it looks that most of them are either UNDEFINED or contain some "standard" value. I hope that this is the fact for most other undocumented fields, at least for the table of the SCHEMA category.

Yes, good.

Do we generate any artifacts for SEQUENCE? I cannot find any. The corresponding AST exists. Please advise.
Thank you.

Of course, please see the last lines in ddl/schema_table_<db-name>_<sql-dialect>.sql. After a successuful conversion you should find something similar to:

drop sequence if exists seq_1;
create sequence seq_1 increment 7 minvalue 0 maxvalue 2000000000 start 25468 cache 1;
...
drop sequence if exists seq_n;
create sequence seq_n increment 1 minvalue 0 maxvalue 9223372036854775807 start 0 cycle cache 1;

• for trunk: see rules/schema/generate_seq_ddl.xml, around line 107, where the getCreateSequenceString() method of P2JDialect is used;
• for 4011a: see rules/schema/java_dmo.xml, around line 226 or search for generateSequenceDDLs() method of DDLGeneratorWorker.java.

Ovidiu Maxiniuc wrote:

Of course, please see the last lines in ddl/schema_table_<db-name>_<sql-dialect>.sql. After a successuful conversion you should find something similar to:
[...]

They are generated for each dialect (because the syntax is a bit different and also the SQL capabilities - not all database engines handle these the same):

• for trunk: see rules/schema/generate_seq_ddl.xml, around line 107, where the getCreateSequenceString() method of P2JDialect is used;
• for 4011a: see rules/schema/java_dmo.xml, around line 226 or search for generateSequenceDDLs() method of DDLGeneratorWorker.java.

Thank you, Ovidiu. Unfortunately, these artifacts do not help in populating metadata. However, I've found that I can use DMOIndex (with a minor add-on).

Ovidiu: Doesn't DMOIndex go away in 4011a?

Greg Shah wrote:

Ovidiu: Doesn't DMOIndex go away in 4011a?

I already use DMOIndex in the MetadataManager. See #3814-43

Greg Shah wrote:

Ovidiu: Doesn't DMOIndex go away in 4011a?

Yes, the DMOIndex class is already gone in 4011a. The problem we are facing now is that there is no metadata runtime support for sequences similar to the new Table/Property annotations in this branch. This is because the sequences are just too simple to have their dedicated abstract objects on FWD server tier. When a value from sequence is needed, FWD just uses the dialects to query (or set) the next value from SQL sequence.

• a dedicated xml (or similar) file - which will need parsing or,
• generate some class in dmo with sequences as member fields annotated with metadata information, like the tables. Since this looks the better solution, here is an hand-written enum class for the above example:

  package com.goldencode.testcases.dmo;
  
  /** Sequence list for {@code testcases} schema. */
  public enum Sequences
  {
     @Sequence(name = "seq1", legacy = "seq_1", increment = 7, maxValue = 2000000000L, start = 25468)
     seq1,
  
     [...]
  
     @Sequence(name = "seq1", legacy = "seq_n", minValue = 0, maxValue = 9223372036854775807L, start = 25468, cycle = true)
     seqn, 
  }

  with @Sequence annotation being defined as:
  

  @Target(ElementType.FIELD)
  @Retention(RetentionPolicy.RUNTIME)
  public @interface Sequence
  {
     /** The name of the sequence. Mandatory. */
     String name();
  
     /** The name of the sequence. Mandatory. */
     String legacy();
  
     /** The minimum value. Mandatory. */
     long start();
  
     /** The incrementation value. Default is 1. */
     long increment() default 1L;
  
     /** The minimum value. Default is 0. */
     long minValue() default 0L;
  
     /** The minimum value. Default is {@link Long#MAX_VALUE}. */
     long maxValue() default Long.MAX_VALUE;
  
     /** Is this sequence cycling? Default is {@code false}. */
     boolean cycle() default false;
  }
  

Ovidiu Maxiniuc wrote:

Greg Shah wrote:

Ovidiu: Doesn't DMOIndex go away in 4011a?

Yes, the DMOIndex class is already gone in 4011a.

• getBasePackage()
• getDMOImpl(String schema, Class dmoIface)
• getDMOInterface(String schema, Class dmoClass)
  I planned to add and use a method to get the list of Sequence for a schema. Is it OK for 4231b?
  Thank you.

My worry here is that you are creating new things that will be hard to merge with 4011a. We plan to do that merge very soon, so I want to avoid adding work later.

Please follow Ovidiu's annotations suggestion.

Greg Shah wrote:

My worry here is that you are creating new things that will be hard to merge with 4011a. We plan to do that merge very soon, so I want to avoid adding work later.

Please follow Ovidiu's annotations suggestion.

OK. Thank you.

As you check in updates for this task, the rules/gaps/database.rules (addMetaschema section) needs to be updated so that the gap marking matches your improvements.

Greg Shah wrote:

As you check in updates for this task, the rules/gaps/database.rules (addMetaschema section) needs to be updated so that the gap marking matches your improvements.

OK. Thanks for reminding me.

I'm using the ChUI regression test suite. I found that with 4231b, it is now required to have a cfg/p2j.cfg.xml built into the converted application jar. Since the file isn't available, the server startup fails. Although I have temporarily added it to the jar file so that I can run the servers, this is not a dependency which is useful.

Please remove this dependency. If the cfg/p2j.cfg.xml is not found, then safely default all settings as if the metadata was disabled for that application.

Greg Shah wrote:

I'm using the ChUI regression test suite. I found that with 4231b, it is now required to have a cfg/p2j.cfg.xml built into the converted application jar. Since the file isn't available, the server startup fails. Although I have temporarily added it to the jar file so that I can run the servers, this is not a dependency which is useful.

Please remove this dependency. If the cfg/p2j.cfg.xml is not found, then safely default all settings as if the metadata was disabled for that application.

Do you mean the situation when cfg/p2j.cfg.xml file is not found or a <metadata> section of this file is not present?
Thank you.

Please make both cases safe.

I've fixed the situation when there is no <metadata> section on the p2j.cfg.xml. However it seems that the application cannot run if this file is not present, regardless of the metadata.

com.goldencode.p2j.cfg.ConfigurationException:  Initialization failure
    at com.goldencode.p2j.main.StandardServer.hookInitialize(StandardServer.java:2083)
    at com.goldencode.p2j.main.StandardServer.bootstrap(StandardServer.java:999)
    at com.goldencode.p2j.main.ServerDriver.start(ServerDriver.java:483)
    at com.goldencode.p2j.main.CommonDriver.process(CommonDriver.java:444)
    at com.goldencode.p2j.main.ServerDriver.process(ServerDriver.java:207)
    at com.goldencode.p2j.main.ServerDriver.main(ServerDriver.java:860)
Caused by: java.lang.RuntimeException: com.goldencode.p2j.cfg.ConfigurationException: Error loading p2j.cfg.xml configuration
    at com.goldencode.p2j.cfg.Configuration.createConfiguration(Configuration.java:808)
    at com.goldencode.p2j.cfg.Configuration.getInstance(Configuration.java:743)
    at com.goldencode.p2j.cfg.Configuration.getSchemaConfig(Configuration.java:366)
    at com.goldencode.p2j.persist.meta.MetadataManager.initialize(MetadataManager.java:209)
    at com.goldencode.p2j.persist.DatabaseManager.initialize(DatabaseManager.java:1512)
    at com.goldencode.p2j.persist.Persistence.initialize(Persistence.java:881)
    at com.goldencode.p2j.main.StandardServer$11.initialize(StandardServer.java:1244)
    at com.goldencode.p2j.main.StandardServer.hookInitialize(StandardServer.java:2079)
    ... 5 more
Caused by: com.goldencode.p2j.cfg.ConfigurationException: Error loading p2j.cfg.xml configuration
    ... 13 more
Caused by: com.goldencode.p2j.cfg.ConfigurationException: Cannot find resource cfg/p2j.cfg.xml
    at com.goldencode.p2j.cfg.Configuration.createConfiguration(Configuration.java:776)
    ... 12 more

A safe default configuration (which has nothing enabled) should be present if the file is not available. No abends.

Greg Shah wrote:

A safe default configuration (which has nothing enabled) should be present if the file is not available. No abends.

Done. Also, if <metadata> section is present in p2j.cfg.xml all mandatory tables are added.
Not committed since 4231b is on freeze.

Please post the change here in patch format so I can review it. If OK, you will check it in to 4231b since this is meant to fix problems with this branch.

Code Review cfg.diff

1. At this time, it is intentional that we do not use logging in Configuration. It is used in many different contexts, including during conversion, reporting, call graph processing etc... Logging is not always something that is safe to enable. Please remove all the logging from your changes.

2. Remove your changes in Configuration.createConfiguration() for the isRuntimeConfig() false case. We are only trying to handle the runtime case here. I think you can also remove the change in the catch() block for Configuration.createConfiguration() since the isRuntimeConfig() true case will no longer raise an exception.

3. Please add history entries to MetadataManager and SchemaConfig.

Code Review cfg.diff

Please remove the new imports in Configuration and also remove the change for the catch() block in Configuration.createConfiguration().

You can also delete instead of commenting the throw line in Configuration.createConfiguration(). I don't see a need to keep that code there.

Why did you remove the throw in the catch() block of Configuration.createConfiguration()? This means that the conversion cases are changed, which we don't want to do. Since the runtime case no longer throws an exception above, I see no need for making a change in the catch() block.

Am I misunderstanding this?

The change looks good. Please test it with Hotel GUI and Hotel ChUI.

Greg Shah wrote:

The change looks good. Please test it with Hotel GUI and Hotel ChUI.

Both apps started OK. Please note however that in both cases the file p2j.cfg.xml was in place.

Igor Skornyakov wrote:

Greg Shah wrote:

The change looks good. Please test it with Hotel GUI and Hotel ChUI.

Both apps started OK. Please note however that in both cases the file p2j.cfg.xml was in place.

Please delete it from the jar and re-test the server startup to confim it is OK.

OK, please commit the change to 4231b.

Greg Shah wrote:

OK, please commit the change to 4231b.

Committed revision 11609.

1. Added _Sequence table population based on the new SEQUENCE support (backported from 4011a)
2. Adding of the mandatory meta-tables is enforced. Working now on handling some corner cases.
3. The populating of the additional fields in the _Filelist, File-trig, and _Field-trig meta-tables should take one day.
4. Another day is required to implement mocks for the meta-table of the PHY_ST category (such as _Area) and setting the remaining fields of the _Field table to the "typical" values (see #3814-103).
5. It is difficult to estimate the time which is required for implementing support for additional fields OF VSTs, as they are non-documented. It will be great if somebody who knows 4GL better than me will create tests to clarify the semantics of these fields.

Task branch 4231b has been merged to trunk as revision 11347.

Greg, which branch should I use for #3814 now?
Thank you.

Hynek will open 3821b shortly.

Greg Shah wrote:

Hynek will open 3821b shortly.

Got it. Thanks!

3821c not b

Greg Shah wrote:

3821c not b

OK, thank you.

Backport of the SEQUENCE support from 4011a and population of the _Sequence table committed to 3821c rev. 11351.

Do we really need to support p2j.cf.xml w/o a <metadata> section? It requires a lot of changes as in many places it is assumed that SchemaConfig.getMetadata() returns non-null value.
Maybe it is OK to assume that when this section is not present it is equivalent to <metadata name="standard"/>.
Thank you.

Yes, I think that is fine.

Finished re-working metadata configuration in p2j.cfg.xml.
Now, if there is no <metadata> section on the p2j.cfg.xml file no DMOs will be generated and no metadata table will be created at runtime.
If the <metadata> section is present then all mandatory tables will be added so it is sufficient to specify <metadata name="standard"/>.

Committed to 3821c rev. 11357.

The conversion of the .df files produces a number of warnings like this:

     [java] WARNING: ignoring invalid option KW_VIEW_AS [388:3] (error = 'null'):  VIEW-AS EDITOR SIZE 70 BY 8 SCROLLBAR-VERTICAL MAX-CHARS 500

     [java] java.lang.NullPointerException
     [java]     at com.goldencode.p2j.uast.SymbolResolver.getCurrentClassDef(SymbolResolver.java:3923)
     [java]     at com.goldencode.p2j.uast.SymbolResolver.addWrappedWorker(SymbolResolver.java:8567)
     [java]     at com.goldencode.p2j.uast.SymbolResolver.addBuiltinFunction(SymbolResolver.java:3494)
     [java]     at com.goldencode.p2j.uast.SymbolResolver.addBuiltinFunction(SymbolResolver.java:3517)
     [java]     at com.goldencode.p2j.uast.ProgressParser.initializeFunctionDictionary(ProgressParser.java:2675)
     [java]     at com.goldencode.p2j.uast.ProgressParser.<init>(ProgressParser.java:483)
     [java]     at com.goldencode.p2j.schema.SchemaLoader.replaceAst(SchemaLoader.java:946)
     [java]     at com.goldencode.p2j.schema.SchemaLoader.postProcessImport(SchemaLoader.java:510)
     [java]     at com.goldencode.p2j.schema.SchemaLoader.importSchema(SchemaLoader.java:418)
     [java]     at com.goldencode.p2j.schema.SchemaLoader.importAll(SchemaLoader.java:300)
     [java]     at com.goldencode.p2j.convert.TransformDriver.runSchemaLoader(TransformDriver.java:327)
     [java]     at com.goldencode.p2j.convert.TransformDriver.front(TransformDriver.java:229)
     [java]     at com.goldencode.p2j.convert.TransformDriver.executeJob(TransformDriver.java:944)
     [java]     at com.goldencode.p2j.convert.ConversionDriver.main(ConversionDriver.java:1025)

Igor Skornyakov wrote:

The conversion of the .df files produces a number of warnings like this:
[...]
This happens while parsing VIEW-AS field option. As a result, this option is not parsed. By adding stack trace dump SchemaLoader line 980 I've got the following:
[...]
What I can see is that SymbolResolver.currentCls is null.
Can anybody provide a clue on how to fix this?
Thank you.

I think is enough to add a NPE check there for currentCls. During schema parser, there is no legacy class parsing needed.

Constantin Asofiei wrote:

I think is enough to add a NPE check there for currentCls. During schema parser, there is no legacy class parsing needed.

I've tried this. It doesn't help, NPE happens in SymbolResolver.addWrappedWorker

Igor Skornyakov wrote:

Constantin Asofiei wrote:

I think is enough to add a NPE check there for currentCls. During schema parser, there is no legacy class parsing needed.

I've tried this. It doesn't help, NPE happens in SymbolResolver.addWrappedWorker

I mean in getCurrentClassDef, return null if currentCls is null. All callers should be aware of null, as this method can already return null.

Constantin Asofiei wrote:

I've tried this. It doesn't help, NPE happens in SymbolResolver.addWrappedWorker

I mean in getCurrentClassDef, return null if currentCls is null. All callers should be aware of null, as this method can already return null.

This is exactly what I did. It seems that not callers are ready for null. But I will try to add checks for null.
Thank you.

Igor Skornyakov wrote:

This is exactly what I did. It seems that not callers are ready for null. But I will try to add checks for null.

Where did you see other NPEs related to getCurrentClassDef returning null?

Constantin Asofiei wrote:

Igor Skornyakov wrote:

This is exactly what I did. It seems that not callers are ready for null. But I will try to add checks for null.

Where did you see other NPEs related to getCurrentClassDef returning null?

There are multiple NPEs in SymbolResolver. However, they are the result of the fact that multiple fields of SymbolResolver are null (e.g, attributes, attrTypes...). I understand that this because the instance of the resolver was created with dict == false.

Greg, do you think is safe to set dict=true at the SymbolResolver instance created by SchemaLoader.postProcessImport line 484?

Constantin Asofiei wrote:

Greg, do you think is safe to set dict=true at the SymbolResolver instance created by SchemaLoader.postProcessImport line 484?

With this change, there is no warning on conversion but the value of the VIEW-AS field is parsed while in the _Field._View-As we need original text.

Constantin Asofiei wrote:

Greg, do you think is safe to set dict=true at the SymbolResolver instance created by SchemaLoader.postProcessImport line 484?

This is a bad idea. That code is trying to finish parsing the .df file which is the source of data for the SchemaDictionary which would be referenced in the SymbolResolver when dict=true. We have never needed it and it is intentionally bypassed. Avoiding this also is much faster and reduces memory usage.

Instead, we need to make the code safe for this being null.

With this change, there is no warning on conversion but the value of the VIEW-AS field is parsed while in the _Field._View-As we need original text.

The schema.g generates an AST that has this as a STRING. Can't you save the string value as an annotation before the child tree is rewritten?

Greg Shah wrote:

Constantin Asofiei wrote:

Greg, do you think is safe to set dict=true at the SymbolResolver instance created by SchemaLoader.postProcessImport line 484?

This is a bad idea. That code is trying to finish parsing the .df file which is the source of data for the SchemaDictionary which would be referenced in the SymbolResolver when dict=true. We have never needed it and it is intentionally bypassed. Avoiding this also is much faster and reduces memory usage.

Instead, we need to make the code safe for this being null.

With this change, there is no warning on conversion but the value of the VIEW-AS field is parsed while in the _Field._View-As we need original text.

The schema.g generates an AST that has this as a STRING. Can't you save the string value as an annotation before the child tree is rewritten?

I see, thank you. At this moment, however, there is no KW_VIEW_AS node in the AST at all. Continue with NPE safety.

however, there is no KW_VIEW_AS node in the AST at all

The replaceAst() rewrites that KW_VIEW_AS subtree, though I still would expect the parent node to be KW_VIEW_AS.

Greg Shah wrote:

however, there is no KW_VIEW_AS node in the AST at all

The replaceAst() rewrites that KW_VIEW_AS subtree, though I still would expect the parent node to be KW_VIEW_AS.

I do not see KW_VIEW_AS node in the .schema or .dict files. Maybe this is a result of NPE.

I do not see KW_VIEW_AS node in the .schema or .dict files. Maybe this is a result of NPE.

Yes, possibly so.

After adding checks for null to the SymbolResolver I do not see warning anymore and there is a KW_VIEW_AS note in the .dict file. I've added the annotation with the text literal to this node.
Tomorrow I will add a population of the _Field._View-As.

Finished with VIEW-AS field property conversion and population of the _Field._View-As.
Tested with a standalone test and Hotel-GUI app.
Committed to 3821c rev. 11364.

Added _File-Trig and _Field-Trig population
Committing to 3821c revision 11366.

3821c revision 11377 fixes a regression DMO generation. The VIEW-AS text can have embedded quotes and escape sequences. When present, these were not encoded properly in the DMO, causing the DMOs to fail compilation. I found this in real customer projects.

I started receiving this during runtime of my testcase:

com.goldencode.p2j.cfg.ConfigurationException:  Initialization failure
        at com.goldencode.p2j.main.StandardServer.hookInitialize(StandardServer.java:2083)
        at com.goldencode.p2j.main.StandardServer.bootstrap(StandardServer.java:999)
        at com.goldencode.p2j.main.ServerDriver.start(ServerDriver.java:483)
        at com.goldencode.p2j.main.CommonDriver.process(CommonDriver.java:444)
        at com.goldencode.p2j.main.ServerDriver.process(ServerDriver.java:207)
        at com.goldencode.p2j.main.ServerDriver.main(ServerDriver.java:860)
Caused by: java.lang.RuntimeException: com.goldencode.p2j.persist.PersistenceException: Error executing DDL for local/testcases/meta
        at com.goldencode.p2j.main.StandardServer$11.initialize(StandardServer.java:1248)
        at com.goldencode.p2j.main.StandardServer.hookInitialize(StandardServer.java:2079)
        ... 5 more
Caused by: com.goldencode.p2j.persist.PersistenceException: Error executing DDL for local/testcases/meta
        at com.goldencode.p2j.persist.meta.MetadataManager.executeDDLScript(MetadataManager.java:979)
        at com.goldencode.p2j.persist.meta.MetadataManager.prepareDatabase(MetadataManager.java:344)
        at com.goldencode.p2j.persist.DatabaseManager.initialize(DatabaseManager.java:1547)
        at com.goldencode.p2j.persist.Persistence.initialize(Persistence.java:881)
        at com.goldencode.p2j.main.StandardServer$11.initialize(StandardServer.java:1244)
        ... 6 more
Caused by: com.goldencode.p2j.MissingDataException: Could not load DDL script com/goldencode/testcases/dmo/_meta/meta_table_ddl.sql
        at com.goldencode.p2j.persist.meta.MetadataManager.executeDDLScript(MetadataManager.java:971)
        ... 10 more

I made a few updates to p2j.cfg.xml to change the behavior, including just creating the <metadata name="standard"/> entry, but it keeps happening. I have the latest standard.df as well.

Roger Borrello wrote:

I started receiving this during runtime of my testcase:
[...]

I made a few updates to p2j.cfg.xml to change the behavior, including just creating the <metadata name="standard"/> entry, but it keeps happening. I have the latest standard.df as well.

Roger,
I'm busy now with some urgent assignment. Was the meta_table_ddl.sql file generated?
Thank you.

Igor Skornyakov wrote:

Roger,
I'm busy now with some urgent assignment. Was the meta_table_ddl.sql file generated?
Thank you.

Understood. No, it was not generated. No errors were reported during conversion.

Roger Borrello wrote:

Igor Skornyakov wrote:

Roger,
I'm busy now with some urgent assignment. Was the meta_table_ddl.sql file generated?
Thank you.

Understood. No, it was not generated. No errors were reported during conversion.

Very strange. Can you please send me your project? I will take a look tomorrow.
Thank you.

Roger, what version of 3821c do you have? Note that trunk had a metadata generation issue which was solved in a 3821c commit. If you don't have that commit, then you have this problem.

Constantin Asofiei wrote:

Roger, what version of 3821c do you have? Note that trunk had a metadata generation issue which was solved in a 3821c commit. If you don't have that commit, then you have this problem.

3821c-11377, which Greg just checked in.

Igor Skornyakov wrote:

Very strange. Can you please send me your project? I will take a look tomorrow.
Thank you.

I placed "failed_testcase.zip" on devsrv01:/tmp. You can remove it when you gather it.

I tried an even smaller testcase, and it still failed on runtime. Are there any directory.xml changes related to the metadata changes?

Thanks!

Roger Borrello wrote:

Igor Skornyakov wrote:

Very strange. Can you please send me your project? I will take a look tomorrow.
Thank you.

I placed "failed_testcase.zip" on devsrv01:/tmp. You can remove it when you gather it.

I tried an even smaller testcase, and it still failed on runtime. Are there any directory.xml changes related to the metadata changes?

Thanks!

Thank you, I will take a look later. No changes to directory.xml are required for metadata.

Roger Borrello wrote:

Igor Skornyakov wrote:

Very strange. Can you please send me your project? I will take a look tomorrow.
Thank you.

I placed "failed_testcase.zip" on devsrv01:/tmp. You can remove it when you gather it.

I tried an even smaller testcase, and it still failed on runtime. Are there any directory.xml changes related to the metadata changes?

Thanks!

Roger,
It seems that that reason is that you're converting the project w/o a database but with an option which expects such. When I've added your test program and file-cvt-list.txt to the project I've worked with when you had a similar problem, the DDL files where generated (this project does have a database)

Found a minor issue with _Field-Trig population.

Committing to 3821c revision 11381.

Fixed the issue with the null ConnectionListener found in automated regression testing by Roger
Committed to 3821c rev. 11388.
Testing restarted.

Igor Skornyakov wrote:

Fixed the issue with the null ConnectionListener found in automated regression testing by Roger
Committed to 3821c rev. 11388.
Testing restarted.

Some of the tests failed but it looks that the issue with NPE is gone.

Can we ask our customers (or at least some of them) to provide an export (.d files) of the meta-tables from their 4GL databases? Even having data from the _File and _Field table will be great.
Thank you.

I'm trying to create an H2 database containing all metatables from a large customer database for a detailed analysis.
I've created testcases project containing all metatables definitions and the exported data. The conversion runs OK, ant create.db.h2 creates the database with all tables, but ant import.db imports only a small subset of tables. It doesn't even attempt to load others. I do not see any error messages in the importdb_xxx.log
What I'm doing wrong?
Thank you.

Igor Skornyakov wrote:

I'm trying to create an H2 database containing all metatables from a large customer database for a detailed analysis.
I've created testcases project containing all metatables definitions and the exported data. The conversion runs OK, ant create.db.h2 creates the database with all tables, but ant import.db imports only a small subset of tables. It doesn't even attempt to load others. I do not see any error messages in the importdb_xxx.log
What I'm doing wrong?
Thank you.

The problem was caused by the fact that 4GL does not include DUMP-NAME property for most meta-tables. It is included only for _sec_* tables and _User. I've added a workaround by setting it to <table name>.d in p2o.xml.
After that ant import.db starts importing all meta-tables, however, I see multiple import errors like this:

SEVERE: Error processing import data from /media/ias/ssd480/_gcdc/testcases/uast/data/dump/meta/_field.d;  0 of ? record(s) successfully processed;  0 record(s) uncommitted due to this error;  0 record(s) dropped
com.goldencode.p2j.util.ErrorConditionException: ** Input value: x(1) should be yes/no. (87)
Caused by: com.goldencode.p2j.NumberedException: Input value: x(1) should be yes/no
        at com.goldencode.p2j.util.ErrorManager.recordOrThrowError(ErrorManager.java:1445)
        at com.goldencode.p2j.util.ErrorManager.recordOrThrowError(ErrorManager.java:1360)
        at com.goldencode.p2j.util.ErrorManager.recordOrThrowError(ErrorManager.java:1339)
        at com.goldencode.p2j.util.logical.<init>(logical.java:454)
        at com.goldencode.p2j.util.logical.<init>(logical.java:345)
        at com.goldencode.p2j.util.Stream.assignDatum(Stream.java:863)
        at com.goldencode.p2j.util.Stream.readField(Stream.java:1763)
        at com.goldencode.p2j.schema.RecordLoader.nextRecord(RecordLoader.java:227)
        at com.goldencode.p2j.schema.ImportWorker$SqlRecordLoader.nextRecord(ImportWorker.java:2225)
        at com.goldencode.p2j.schema.ImportWorker$Library.importTable(ImportWorker.java:1044)
        at com.goldencode.p2j.schema.ImportWorker$Library.lambda$importAsync$0(ImportWorker.java:1525)
        at java.lang.Thread.run(Thread.java:748)

The problems has multiple reasons.
1. The table dump from the Data Administrator skips some fields for the reason I do not understand. For example for the _Field table it skips not only _File-recid which has type recid (this is a documented behavior), but also at least the _dtype field which has type integer. The later results in the data shift and the RecordLoader.nextRecord method attempts to assign a value of the format field to the manadatory field which cases the exception described in 3814-179.
2. The missed value of the _File-recid field results in the the unique key violation for the table _Field for the for the multiple indexes as all of them include this field, e.g.:

ADD INDEX "_Field-Position" ON "_Field" 
  AREA "Schema Area" 
  UNIQUE
  INDEX-FIELD "_File-recid" ASCENDING 
  INDEX-FIELD "_Order" ASCENDING 

It is possible to export all fields using a custom script like this:

OUTPUT TO _field.d
for each _field:
EXPORT
<list of fields>
.
end. 
OUTPUT CLOSE.

The list of fields can be generated by a simple command line from the .df field:

grep "ADD FIELD" meta.df | grep '_Field\"' | sed 's/ADD FIELD "//; s/".* AS.*//' 

However the RecordLoader ignores fields of the recid type, so it is necessary to add some conditional logic to load it for metatables.

We don't want to import this data. Why spend time on this when we want the DMO annotations to hold everything needed for _file and _field?

Greg,
DMO annotations keeps the properties which are defined in the .df file. They all are already added to the DMO annotation and loaded to the meta-table on the server starup. However, there are a lot of other fields in the _Field table which are mentioned in the this task and I've suggested that they should be assigned to a "typical" values. I understand that you've agreed with this idea. I wanted to import the "real" data from the large customer database into the SQL database (H2) to simplify the analysis of the these typical values. This seemed to be a pretty simple task, but I've encountered some problems I've not expected.

If it is just for analysis for this task, it is fine. I just want to be clear that we should not expect to do an import of these tables to make a running system.

Greg Shah wrote:

If it is just for analysis for this task, it is fine. I just want to be clear that we should not expect to do an import of these tables to make a running system.

Of course it is not for runtime. I had such an idea at the very beginning, but you've explained me that it is not a good one.

1. Adding of the mandatory meta-tables is enforced. Working now on handling some corner cases.
2. The populating of the additional fields in the File-trig, and _Field-trig is done. Doing this for _Filelist, meta-tables should take half a day or so.
3. Another day is required to implement mocks for the meta-table of the PHY_ST category (such as _Area) and setting the remaining fields of the _Field table to the "typical" values (see #3814-103).
4. It is difficult to estimate the time which is required for implementing support for additional fields OF VSTs, as they are non-documented. It will be great if somebody who knows 4GL better than me will create tests to clarify the semantics of these fields.

Can you finish #3814-185 items 1-3 by Friday?

It is difficult to estimate the time which is required for implementing support for additional fields OF VSTs, as they are non-documented. It will be great if somebody who knows 4GL better than me will create tests to clarify the semantics of these fields.

Please make a most specific list of the VST fields which are in question.

Greg Shah wrote:

Can you finish #3814-185 items 1-3 by Friday?

Yes, I think that it possible. If not, I will finish at the weekend for sure.

It is difficult to estimate the time which is required for implementing support for additional fields OF VSTs, as they are non-documented. It will be great if somebody who knows 4GL better than me will create tests to clarify the semantics of these fields.

Please make a most specific list of the VST fields which are in question.

OK.

Based on the analisys of the content of the _Field table in the large customer database (12000+ records in this table) the following fields typically have non-null values:

_field._charset                 "iso8859-1" - for character or ?
_field._collation               "basic_I"   - for character ("basic_S" if CASE-SENSITIVE property is specified) or ?
_field._decimals                DECIMALS property
_field._desc                    DESCRIPTION -property

_field._dtype                   depends on the field datatype
                                character     - 1
                                date          - 2        
                                logical       - 3
                                integer       - 4
                                decimal       - 5
                                recid         - 7
                                raw           - 8
                                blob          - 18
                                clob          - 19
                                short         - 22
                                float         - 24
                                double        - 25
                                fixchar       - 31
                                bigint        - 32
                                time          - 33
                                timestamp     - 34
                                datetime      - 34
                                datetime-tz   - 40
                                int64         - 41

_field._fetch-type              depends on the field datatype
                                character     - "varchar" 
                                date          - "date"        
                                logical       - "bit" 
                                integer       - "integer" 
                                decimal       - "numeric" 
                                recid         - "integer" 
                                raw           - "varbinary" 
                                blob          - "varbinary" 
                                clob          - "varchar" 
                                short         - "smallint" 
                                float         - "float" 
                                double        - "real" 
                                fixchar       - "character" 
                                bigint        - "bigint" 
                                time          - "time" 
                                timestamp     - "timestamp" 
                                datetime      - "timestamp" 
                                datetime-tz   - "timestamp_timezone" 
                                int64         - "bigint" 

_field._fld-case                "yes" if CASE-SENSITIVE property is specified, otherwise "no" 
_field._sys-field               "no" for most of field, for some field which names start with underscore (such as @_Field._Fetch-Type@) - "yes". Seems to be an extra 
                                piece of data which cannot be figured from the table definition. I suggest do not populate this field in FWD for now or always set "no".
_field._width                   MAX-WIDTH property

Is _field._dtype the same as the data-type list I've described in #3549-3 and #3549-4?

Greg Shah wrote:

Is _field._dtype the same as the data-type list I've described in #3549-3 and #3549-4?

Greg, I understand that _dtype is not an additional datatype, it looks more like internal 4GL type, like _fetch_type. It can be used however for some serialization. At this moment I do not see direct relation of it to #3549-3 and #3549-4, but will think about this.

Igor, please see the TYPE_ constants in LegacyJavaAppserverApi - I think _dtype is the same. You can confirm this by creating a permanent table in OE, with fields of all possible types, and then dump the _field meta table.

Constantin Asofiei wrote:

Igor, please see the TYPE_ constants in LegacyJavaAppserverApi - I think _dtype is the same. You can confirm this by creating a permanent table in OE, with fields of all possible types, and then dump the _field meta table.

OK, thank you Constantin.
Please note that I do not see at least RAW and FIXCHAR in the LegacyJavaAppserverApi

It is interesting that for some system tables the _Field table contains records for fields which are not shown in the .df file.
For example such are _sec-granted-role._Alt, _sec-granted-role._Del and _sec-granted-role._Exe "fields".

Added population of the _Field table fields, described in #3814-188.
Committed to 3821c revision 11447.

3821c rev. 11449 does not compile

[ant:javac] /media/ias/ssd480/_gcdc/3821c/src/com/goldencode/p2j/ui/ControlEntityExt.java:293: error: method does not override or implement a method from a supertype
[ant:javac]    @Override
[ant:javac]    ^
[ant:javac] /media/ias/ssd480/_gcdc/3821c/src/com/goldencode/p2j/ui/ControlEntityExt.java:377: error: method does not override or implement a method from a supertype
[ant:javac]    @Override
[ant:javac]    ^
[ant:javac] /media/ias/ssd480/_gcdc/3821c/src/com/goldencode/p2j/ui/ControlEntityExt.java:447: error: method does not override or implement a method from a supertype
[ant:javac]      @Override
[ant:javac]      ^
[ant:javac] /media/ias/ssd480/_gcdc/3821c/src/com/goldencode/p2j/ui/ControlEntityExt.java:506: error: method does not override or implement a method from a supertype
[ant:javac]    @Override
[ant:javac]    ^
[ant:javac] /media/ias/ssd480/_gcdc/3821c/src/com/goldencode/p2j/ui/ControlEntityExt.java:565: error: method does not override or implement a method from a supertype
[ant:javac]    @Override
[ant:javac]    ^
[ant:javac] /media/ias/ssd480/_gcdc/3821c/src/com/goldencode/p2j/ui/ControlEntityExt.java:624: error: method does not override or implement a method from a supertype
[ant:javac]    @Override
[ant:javac]    ^
[ant:javac] /media/ias/ssd480/_gcdc/3821c/src/com/goldencode/p2j/ui/ControlEntityExt.java:674: error: method does not override or implement a method from a supertype
[ant:javac]    @Override
[ant:javac]    ^
[ant:javac] /media/ias/ssd480/_gcdc/3821c/src/com/goldencode/p2j/ui/ControlEntityExt.java:724: error: method does not override or implement a method from a supertype
[ant:javac]    @Override
[ant:javac]    ^
[ant:javac] /media/ias/ssd480/_gcdc/3821c/src/com/goldencode/p2j/ui/ControlEntityExt.java:784: error: method does not override or implement a method from a supertype
[ant:javac]    @Override
[ant:javac]    ^

Yes

Igor Skornyakov wrote:

3821c rev. 11449 does not compile
[...]

Fixed in rev 11450.

As I can see the following fields are already supported, at least the "Minimal*" interfaces in the ConnectTableUpdater and LockTableUpdater have the corresponding setters

_connect._connect-name
_connect._connect-time
_connect._connect-device
_connect._connect-pid
_connect._connect-batch

_myconnection._myconn-userid

_lock._lock-chain
_lock._lock-flags

I've added setters for the

_connect._connect-transid
_connect._connect-type
_connect._connect-disconnect
_connect._connect-server
_connect._connect-clienttype
_connect._connect-ipaddress
_connect._connect-tenantid

The remaining VSTs are:

_area
_filelist
_tenant
_usertablestat

I do not know if we already have any support for the notion of tenant.

The remaining _usertablestat VST contains data which is updated at runtime. Now I'm working on the UserTableStatUpdater based on the ConnectTableUpdater and LockTableUpdater code.

I do not know if we already have any support for the notion of tenant.

No, we do not. We are not currently planning to implement this. For the current implementation, just report the same thing that the 4GL would report for a single tenant system.

Greg Shah wrote:

I do not know if we already have any support for the notion of tenant.

No, we do not. We are not currently planning to implement this. For the current implementation, just report the same thing that the 4GL would report for a single tenant system.

Do you mean that the _Tenant table should not be populated (as I see in non-multi-tenant databases)?
Thank you.

Yes, if that is how it works in the 4GL for a non-multi-tenant case.

How quickly do you estimate that can you complete this work?

Greg Shah wrote:

How quickly do you estimate that can you complete this work?

I plan to finish with the updater today. I'm not sure how many places should be modified to use this updater for gathering statistics. Maybe it makes send to do it only for the new ORM.

Maybe it makes send to do it only for the new ORM.

Yes, probably so. Get it to the point where everything is ready except for the integration with the new ORM. We will defer this for a day or two until 4011b is merged to trunk and 3821c is rebased.

When will you get to a good stopping point?

Greg Shah wrote:

When will you get to a good stopping point?

Most likely today. Tomorrow at the latest.

I'm working now on the _usertablestat VST support. This VST holds information about the CRUD operations. The corresponding UserTableStatUpdater should contain method(s) which will be called to update the statistics. To implement the method(s) with "right" signature(s) I need to understand from which places these method(s) should called. Is my understanding correct that it is ChangeBorker.stateChanged()?
Thank you.

Is the _usertablestat contents specific to the current user's session? If so, then it is important to capture and store these on a per-session basis. And we will want to avoid any cross-session synchronization overhead.

Greg Shah wrote:

Is the _usertablestat contents specific to the current user's session? If so, then it is important to capture and store these on a per-session basis. And we will want to avoid any cross-session synchronization overhead.

The _usertablestat record contains _UserTableStat-Conn which holds the user number and @_UserTableStat-TenantId which holds its tenant Id. I do not think that any cross-session synchronization is required. I'm using BlockingLinkedQueue to hold the pended updates, It is a concurrent data structure which does not require aquiring monitor to access it from different threads.

Igor Skornyakov wrote:

I'm working now on the _usertablestat VST support. This VST holds information about the CRUD operations. The corresponding UserTableStatUpdater should contain method(s) which will be called to update the statistics. To implement the method(s) with "right" signature(s) I need to understand from which places these method(s) should called. Is my understanding correct that it is ChangeBorker.stateChanged()?

I don't know enough about the information you need to collect to answer this. This method gets notifications about insert, update, and delete operations, but nothing about read.

But to take a step back... whatever the solution is, it must add as little overhead as possible. I imagine this VST is used rarely by application logic, and I am concerned that the constant collection of CRUD stats (which generally is redundant with information collected by the database itself) will add an unacceptable amount of overhead.

It might make sense to leave this table unpopulated, such that it will return with "safe" (i.e., no) results from queries, with the understanding that any utility programs which might have used this are no longer relevant in the new environment, and would need to look to the vendor-specific system tables of the new database.

Eric Faulhaber wrote:

I don't know enough about the information you need to collect to answer this. This method gets notifications about insert, update, and delete operations, but nothing about read.

The _usertablestat collects information about reads as well. At this moment I think that a better place to track insert/update/delete is RecordBuffer.reportChange. We agreed with Greg that real statistics collection will be implemented for new ORM. I think that for testing my code it is OK do not track reads.

But to take a step back... whatever the solution is, it must add as little overhead as possible. I imagine this VST is used rarely by application logic, and I am concerned that the constant collection of CRUD stats (which generally is redundant with information collected by the database itself) will add an unacceptable amount of overhead.

I'm not sure that the information collected by the database is 100% replacement of the VST in question. To be honest, I've never seen what information the database collects, but as long as we do not use impersonation for the database content is simple cannot distinguish between different users.

It might make sense to leave this table unpopulated, such that it will return with "safe" (i.e., no) results from queries, with the understanding that any utility programs which might have used this are no longer relevant in the new environment, and would need to look to the vendor-specific system tables of the new database.

I'm trying to reduce the overhead as much as I can. Do you mean that I should stop working on the updater at this moment?
Thank you.

Even if the overhead is minimized, the current approach wastes CPU cycles for a feature that is almost never used.

In a normal end user's session, these VST tables are NEVER used from the 4GL. The usage is usually in some tools intended to help system admins to see what locks are being held or how many users are connected. The _usertablestat is probably used for some debugging or diagnosis purposes. On most FWD servers, I suspect these will never ever be used.

Today we apply changes to _lock, _trans, _connect as the state changes in each session. Even though we queue these and apply them on a separate thread, there is still constant overhead here. As can be seen in #3896, I want to reverse the concept of these VST tables. The idea: leave the VST table empty and fill it dynamically "just in time" if and only if a query to the VST occurs. For the _lock table, this means we use the data we already have in the lock manager to build the table when needed.

For _usertablestat, we probably need some per-session long counters that just get incremented during normal processing. Then we have the raw data in each session which can be stuck into the table just in time, if it is ever used.

Even in the rare case that the VST is accessed, there will be no need for this code to be fast. I don't care if it takes 60 seconds to build the table on the fly, that is far better than losing CPU cycles all along the way.

Greg Shah wrote:

Even if the overhead is minimized, the current approach wastes CPU cycles for a feature that is almost never used.

In a normal end user's session, these VST tables are NEVER used from the 4GL. The usage is usually in some tools intended to help system admins to see what locks are being held or how many users are connected. The _usertablestat is probably used for some debugging or diagnosis purposes. On most FWD servers, I suspect these will never ever be used.

Today we apply changes to _lock, _trans, _connect as the state changes in each session. Even though we queue these and apply them on a separate thread, there is still constant overhead here. As can be seen in #3896, I want to reverse the concept of these VST tables. The idea: leave the VST table empty and fill it dynamically "just in time" if and only if a query to the VST occurs. For the _lock table, this means we use the data we already have in the lock manager to build the table when needed.

For _usertablestat, we probably need some per-session long counters that just get incremented during normal processing. Then we have the raw data in each session which can be stuck into the table just in time, if it is ever used.

Even in the rare case that the VST is accessed, there will be no need for this code to be fast. I don't care if it takes 60 seconds to build the table on the fly, that is far better than losing CPU cycles all along the way.

So, should I stop working on the updater completely and start working on the per-session counters?
Thank you.

It is also possible to have the updater, but it will not try to persist statistics constantly but keep it in the in-memory map. Or even can do one of two things based on configuration.

Igor, to best answer you, please help me understand the purpose of these fields:

_usertablestat._usertablestat-conn
_usertablestat._usertablestat-num
_usertablestat._usertablestat-read
_usertablestat._usertablestat-update
_usertablestat._usertablestat-create
_usertablestat._usertablestat-delete

Are these just counters of the number of times certain operations take place (i.e., connections, reads, updates, creates, deletes, etc.)? What is the _usertablestat-num field about?

If they are just counters, are they per-session, or across all sessions?

Eric Faulhaber wrote:

What is the _usertablestat-num field about?

Is this a "foreign key" of sorts to the _user._user_number field?

Eric,
Yes, the table effectively holds counters, 4 for 4 different CRUD operations per user/table combination. At least this is my understanding.
See https://documentation.progress.com/output/ua/OpenEdge_latest/index.html#page/dmadm%2Fuser-table-activity-(-usertablestat).html%23

Eric Faulhaber wrote:

Eric Faulhaber wrote:

What is the _usertablestat-num field about?

Is this a "foreign key" of sorts to the _user._user_number field?

Yes, I think so.

That doc page suggests _usertablestat-num is the table number. Perhaps this is some kind of unique number associated with each table?

Isn't the _usertablestat-conn the one that maps to the user?

Greg Shah wrote:

That doc page suggests _usertablestat-num is the table number. Perhaps this is some kind of unique number associated with each table?

Isn't the _usertablestat-conn the one that maps to the user?

In the _File table each table has a unique number. Looking at the sample content of the _usertablestat I think that it is the value of the _usertablestat-conn

Greg Shah wrote:

That doc page suggests _usertablestat-num is the table number. Perhaps this is some kind of unique number associated with each table?

Isn't the _usertablestat-conn the one that maps to the user?

That would make sense, and would suggest that the stats map into our notion of context (as opposed to tracking by user across multiple sessions with the same user).

In that case, I think it makes sense to store the counters in a context-local object, one that is readily available without an additional context-local lookup. Perhaps we add a UserStats object to Persistence$Context. The latter is available through every RecordBuffer instance. The counters can be updated with very low overhead. Then, as Greg suggests, when a query against the _usertablestat table occurs, we populate a private/local H2 temporary table with the information requested in a just-in-time fashion.

I doubt it is critically important to synchronize access to the counters, and we probably want to avoid this for performance. As long as the current counter value is consistent across threads (volatile), we should be ok.

Eric Faulhaber wrote:

Greg Shah wrote:

That doc page suggests _usertablestat-num is the table number. Perhaps this is some kind of unique number associated with each table?

Isn't the _usertablestat-conn the one that maps to the user?

That would make sense, and would suggest that the stats map into our notion of context (as opposed to tracking by user across multiple sessions with the same user).

In that case, I think it makes sense to store the counters in a context-local object, one that is readily available without an additional context-local lookup. Perhaps we add a UserStats object to Persistence$Context. The latter is available through every RecordBuffer instance. The counters can be updated with very low overhead. Then, as Greg suggests, when a query against the _usertablestat table occurs, we populate a private/local H2 temporary table with the information requested in a just-in-time fashion.

I doubt it is critically important to synchronize access to the counters, and we probably want to avoid this for performance. As long as the current counter value is consistent across threads (volatile), we should be ok.

I understand that it should be a map of arrays of counters. If it will be ConcurrentMap with Atomic values no additional synchronization is required. Please note however that if session is closed the counters will be lost, if I understand correctly.

Igor Skornyakov wrote:

I understand that it should be a map of arrays of counters. If it will be ConcurrentMap with Atomic values no additional synchronization is required.

OK, I was still thinking of this in terms of being on the per-session level, but you are right...this table must reflect stats for all sessions, organized by user connection. So, ignore my previous suggestion about storing this information at the context-local level.

Please note however that if session is closed the counters will be lost, if I understand correctly.

I assume the entries for a particular session should be removed when that session ends. Otherwise, this table (map, in our case) would grow endlessly. But this warrants some testing. If so, we must be sure we remove a context's entries on abnormal end as well as normal end, to avoid a memory leak.

I think your earlier idea of enabling/disabling this tracking with runtime configuration makes sense as well.

Eric Faulhaber wrote:

I assume the entries for a particular session should be removed when that session ends. Otherwise, this table (map, in our case) would grow endlessly. But this warrants some testing. If so, we must be sure we remove a context's entries on abnormal end as well as normal end, to avoid a memory leak.

Do you mean that every session has it own set of VSTs? I have an impression that at least for _Lock this is not the case. But this can be a misunderstanding.

.

I understand that it should be a map of arrays of counters. If it will be ConcurrentMap with Atomic values no additional synchronization is required. Please note however that if session is closed the counters will be lost, if I understand correctly.

This still seems expensive. We don't want to take a map lookup hit in order to increment a counter.

The data is table-level by user, right? It seems OK to do something context local where each RecordBuffer would have direct access to the table-level counters. To build the table we will have an expensive operation of walking through all sessions and pulling the stats. That is OK. It will also naturally clean up when the session goes away.

Greg Shah wrote:

This still seems expensive. We don't want to take a map lookup hit in order to increment a counter.

The data is table-level by user, right? It seems OK to do something context local where each RecordBuffer would have direct access to the table-level counters. To build the table we will have an expensive operation of walking through all sessions and pulling the stats. That is OK. It will also naturally clean up when the session goes away.

Sorry, I do not understand. The counters' set is separate for different tables. The session can access multiple tables. How can we avoid using map?

public class TableStats
{
   public AtomicLong create = new AtomicLong();
   public AtomicLong read = new AtomicLong();
   public AtomicLong update = new AtomicLong();
   public AtomicLong delete = new AtomicLong();
}

There would be one of these for each table ever accessed. At the first time a table is accessed, it would be created and be added into a context-specific list. A reference to it would also be directly kept in every RecordBuffer instance that refers to that table. This allows direct access with the absolute minimum cost.

Greg Shah wrote:

[...]

There would be one of these for each table ever accessed. At the first time a table is accessed, it would be created and be added into a context-specific list. A reference to it would also be directly kept in every RecordBuffer instance that refers to that table. This allows direct access with the absolute minimum cost.

Ah, got it! To ensure that the TableStats should be one per table I think that it is better to hold them in a context-specific map. This will require lookup only at the RecordBuffer creation. After that it will be accessed via the reference from the RecordBuffer.
Is it OK?

BTW: I do not think that the overhead will be substantially increased if the map will be not context-local but a field in the database. This will simplify the VST population.

Igor Skornyakov wrote:

To ensure that the TableStats should be one per table I think that it is better to hold them in a context-specific map. This will require lookup only at the RecordBuffer creation. After that it will be accessed via the reference from the RecordBuffer.
Is it OK?

Yes, a map lookup at RecordBuffer creation (or initialization) seems an acceptable level of overhead.

Note that simple long types will do. There is no need to use AtomicLong, since these variables are counting operations per user session, per table. So, they would only ever be incremented in a single thread.

Greg, when it comes time to populate the table with the aggregate data for all contexts, how are all the active contexts "scanned" to retrieve the map of TableStats objects in each context? I am not aware of an API for this.

Eric Faulhaber wrote:
.> Yes, a map lookup at RecordBuffer creation (or initialization) seems an acceptable level of overhead.

Note that simple long types will do. There is no need to use AtomicLong, since these variables are counting operations per user session, per table. So, they would only ever be incremented in a single thread.

AtomicLong doesn't add significant overhead. After all it will be increased only after a database operations which are much more expensive. And keeping this map in the Database instance will greatly simplify the aggregation. It may be even possible to mimic the read operations on the VST w/o populating the meta-database.

.

Igor Skornyakov wrote:

And keeping this map in the Database instance will greatly simplify the aggregation.

Please help me understand what you intend to do here.

I do not want to add a database query to fetch this map whenever a RecordBuffer instance is constructed. There are many cases where RecordBuffer instances are constructed and never even used. Consider, for example, the common use of include files which define buffers or temp-tables. They are included in many programs as a developer convenience, but then the including program doesn't use most of the buffers. This would mean we would add the overhead of a database query to a buffer which would otherwise be doing nothing. Even in the case where a buffer is actually used for a database query, adding another query for the map retrieval potentially can double the cost. It is only amortized if the buffer is used for much more database activity. If I have misunderstood your intent, please clarify for me.

I think I understand what you are getting at with the easier table population, but this approach suggests the _usertablestats table is long-lived, not just-in-time. This further requires that some explicit cleanup of the table needs to occur when a context ends (gracefully or otherwise). If the map resides in a context-local object, this cleanup happens automatically. However, the part of the puzzle I am still missing is how we collect all this information across contexts; hence, my previous question to Greg.

Using a just-in-time approach is less than ideal if a program makes more than one query against the _usertablestats VST, assuming the table is populated from scratch every time a query occurs. However, this is probably an acceptable tradeoff to minimizing the runtime overhead for the more common case where this metadata is not used.

Eric Faulhaber wrote:

Igor Skornyakov wrote:

And keeping this map in the Database instance will greatly simplify the aggregation.

Please help me understand what you intend to do here.

I do not want to add a database query to fetch this map whenever a RecordBuffer instance is constructed. There are many cases where RecordBuffer instances are constructed and never even used. Consider, for example, the common use of include files which define buffers or temp-tables. They are included in many programs as a developer convenience, but then the including program doesn't use most of the buffers. This would mean we would add the overhead of a database query to a buffer which would otherwise be doing nothing. Even in the case where a buffer is actually used for a database query, adding another query for the map retrieval potentially can double the cost. It is only amortized if the buffer is used for much more database activity. If I have misunderstood your intent, please clarify for me.

I'm not talking about database query. Me suggestion is have the map as a field in the Database object instance.

I think I understand what you are getting at with the easier table population, but this approach suggests the _usertablestats table is long-lived, not just-in-time. This further requires that some explicit cleanup of the table needs to occur when a context ends (gracefully or otherwise). If the map resides in a context-local object, this cleanup happens automatically. However, the part of the puzzle I am still missing is how we collect all this information across contexts; hence, my previous question to Greg.

Sorry, I do not understand. What you call "just-in time"? If the VST instances are different for different sessions why do we need to collect the data from multiple contexts? If there is only one VST per database, how can we prevent the data loss on conetext end?

Using a just-in-time approach is less than ideal if a program makes more than one query against the _usertablestats VST, assuming the table is populated from scratch every time a query occurs. However, this is probably an acceptable tradeoff to minimizing the runtime overhead for the more common case where this metadata is not used.

This why I mentioned simulation database operation on the "table" backed by a map

Igor Skornyakov wrote:

I'm not talking about database query. Me suggestion is have the map as a field in the Database object instance.

I see. If you mean com.goldencode.p2j.persist.Database, that object's primary purpose is to be a hash map key and it may be serialized and sent as a remote parameter across JVMs. They are created often and are potentially short-lived. This does not seem to be a good location for keeping this metadata. Perhaps there is a better object to which to attach the map?

Sorry, I do not understand. What you call "just-in time"? If the VST instances are different for different sessions why do we need to collect the data from multiple contexts? If there is only one VST per database, how can we prevent the data loss on conetext end?

I think the VST instances should be implemented as local H2 temporary tables, populated just-in-time for a query which requests the metadata. However, I would expect that they must include the data for all user connections (i.e., multiple contexts). Otherwise, how would a 4GL program work that does something like this?

FIND FIRST _File WHERE _File._File-Name = "my-table".
FOR EACH _Usertablestat WHERE _Usertablestat._Usertablestat-num = _File._File-Number:
   ...
END.

This query does not filter on the user connection, so presumably it would return stats for ALL user connections active at that moment. Perhaps you have found otherwise and my concern is unfounded.

Using a just-in-time approach is less than ideal if a program makes more than one query against the _usertablestats VST, assuming the table is populated from scratch every time a query occurs. However, this is probably an acceptable tradeoff to minimizing the runtime overhead for the more common case where this metadata is not used.

This why I mentioned simulation database operation on the "table" backed by a map

I'm not clear on what you mean by this, but it sounds like it involves parsing SQL to simulate a query without being backed by a database. This can get very complicated and I think goes beyond the scope of this feature.

I am thinking we are not visualizing this solution the same way, because of the questions we are asking each other. My understanding so far was that there would be a map of TableStats objects, keyed by DMO UID or table number (i.e., _File._File-Number). The map would be added to an existing, context-local object, such as Persistence$Context. An instance variable of type TableStats would be added to the RecordBuffer class. Upon construction or initialization of each RecordBuffer instance, a map lookup would occur to assign that instance variable with the appropriate TableStats instance for that buffer's table. At that point, the stats counters can be updated efficiently from within RecordBuffer (or classes which use it), without any context-local lookup necessary.

At the time of a query against the _Usertablestat table, a local/private H2 temporary table is created, all the maps of TableStats objects are accessed across all contexts and their stats aggregated, the required inserts into the table are made, and the query is executed. The temporary table is then dropped. There are two questions in my mind on this part: how to collect the cross-context data, and do we need some thread safety or synchronization beyond the AtomicLong use to ensure the integrity of the aggregated statistics? My gut says that since these are statistics, the thread safety offered by AtomicLong is good enough.

Greg, Igor, if either of you is visualizing something substantially different than this, let's clear up any confusion.

Eric Faulhaber wrote:

Igor Skornyakov wrote:

I'm not talking about database query. Me suggestion is have the map as a field in the Database object instance.

I see. If you mean com.goldencode.p2j.persist.Database, that object's primary purpose is to be a hash map key and it may be serialized and sent as a remote parameter across JVMs. They are created often and are potentially short-lived. This does not seem to be a good location for keeping this metadata. Perhaps there is a better object to which to attach the map?

1. The aggregation looks pretty complicated.
2. The data from the contexts that are closed before the start of the aggregation will be lost.

Anyway, tomorrow I will start with attaching TableStats objects to RecordBuffer and updating the counters.

Note that simple long types will do. There is no need to use AtomicLong, since these variables are counting operations per user session, per table. So, they would only ever be incremented in a single thread.

After #3254 this won't be true. So let's stay with AtomicLong.

However, the part of the puzzle I am still missing is how we collect all this information across contexts; hence, my previous question to Greg.

The easiest thing would be to have a static map of maps. If each session has a context-local Map<_table_number_, TableStats> then the static map would be Map<_session_key_, Map<_table_number_, TableStats>>. We add our Map<_table_number_, TableStats> into this static map when the persistence layer initializes for a session and we remove it when the context-local cleanup occurs. Then at any time we can just iterate over these maps to populate the VST.

Using a just-in-time approach is less than ideal if a program makes more than one query against the _usertablestats VST, assuming the table is populated from scratch every time a query occurs. However, this is probably an acceptable tradeoff to minimizing the runtime overhead for the more common case where this metadata is not used.

I understand and agree this is a limitation we can live with. Actual usage of the _usertablestats may be terrible. That is OK.

I am thinking we are not visualizing this solution the same way, because of the questions we are asking each other. My understanding so far was that there would be a map of TableStats objects, keyed by DMO UID or table number (i.e., _File._File-Number). The map would be added to an existing, context-local object, such as Persistence$Context. An instance variable of type TableStats would be added to the RecordBuffer class. Upon construction or initialization of each RecordBuffer instance, a map lookup would occur to assign that instance variable with the appropriate TableStats instance for that buffer's table. At that point, the stats counters can be updated efficiently from within RecordBuffer (or classes which use it), without any context-local lookup necessary.

At the time of a query against the _Usertablestat table, a local/private H2 temporary table is created, all the maps of TableStats objects are accessed across all contexts and their stats aggregated, the required inserts into the table are made, and the query is executed. The temporary table is then dropped.

Yes, this is how I see it as well.

how to collect the cross-context data,

Hopefully may answer above handles this.

and do we need some thread safety or synchronization beyond the AtomicLong use to ensure the integrity of the aggregated statistics? My gut says that since these are statistics, the thread safety offered by AtomicLong is good enough.

The accuracy of the user stats is meaningless. Of course, the stats will be changing while we are copying the values out to populate the VST. This does not matter. AtomicLong should be fine.

The data from the contexts that are closed before the start of the aggregation will be lost.

I would have thought that this is the way it should work. Does the 4GL report values from sessions that are not active? That seems to be a terrible idea. When would those get cleared?

My understanding is that VSTs are created at the connect to the database and are not cleared until the connection is closed. But I may be wrong.

My understanding is that VSTs are created at the connect to the database and are not cleared until the connection is closed. But I may be wrong.

If they are created/deleted with the database connection, then you are saying that the VSTs are different for each user?

And they still provide data for multuple users? The design doesn't make much sense.

Greg Shah wrote:

My understanding is that VSTs are created at the connect to the database and are not cleared until the connection is closed. But I may be wrong.

If they are created/deleted with the database connection, then you are saying that the VSTs are different for each user?

And they still provide data for multuple users? The design doesn't make much sense.

Sorry, my saying was not accurate enough. According to the Progress documentation

Virtual system tables (VSTs) give ABL and SQL applications access to the same type of database information that you collect with the OpenEdge Monitor (PROMON) utility. Virtual system tables enable an application to examine the status of a database and monitor its performance. With the database broker running, ABL and SQL applications can query a VST and retrieve the specified information as run-time data.

In any case, I think that aggregating the data from all local contexts and tolerating missing data from closed contexts is not a consistent approach. I do not understand how to create a test to check if it matches the 4GL behavior.

Run multuple clients at different times and see if the data for earlier closed clients is seen by later clients.

Greg Shah wrote:

Run multuple clients at different times and see if the data for earlier closed clients is seen by later clients.

1. Started the PROMON utility for a local database
2. Started the Procedure Editor and connected to the same database
3. Requested the database activity and see a non zero value of the reads' counter
4. Closed the Procedure Editor
5. Requested the database activity again and see the same value of the reads' counter

you mean by multiple clients

You've been saying that it shows the stats for multuple users. Each user would be running an mpro process at the operating system level. Each mpro process is a 4GL client.

To test how the VST works, don't you need to run more than one mpro process? Make sure they run code that accesses real tables. Then look at the VST usage to see which of the clients appear. Does it show all clients from all time? If so, when does this get cleared? Surely it doesn't grow without bounds.

1. For every connection 4GL creates 50 slots with counters with values of the _UserTableStat-Num from 1 to 50.
2. If the connection is accessing table with the _File-Number in the [1,50] interval, the corresponding counter is updated. If the _File-Number is out of this interval, the VST is not updated.
3. The data for all connections are visible in the VST
4. When the connection is closed, the counters' values for it are reset to zero and the value of the _UserTableStat-TenantId column is set to -32767.
5. The value of the _UserTableStat-Conn field is actually a connection Id, not the user Id as doc says.
   I've found the following sample code in the Internet (https://rajsn2012.wixsite.com/techman/post/openedge-schema-tables) illustating how to find table currently in use:
   

   find first _MyConnection no-lock.
   for each _UserTableStat where _UserTableStat-Conn = _MyConnection._MyConn-UserId no-lock:
       find _file where _file-num = _UserTableStat-Num no-lock no-error. 
       if available _file then
           display _UserTableStat-Num  
                   _file-name format "x(20)" 
                   _UserTableStat-read format ">>>>>>>>>>" 
                   _UserTableStat-create format ">>>>>>>>>>" 
                   _UserTableStat-update format ">>>>>>>>>>" 
                   _UserTableStat-delete  format ">>>>>>>>>>".
   end.
   

Based on the following statement from the Progress doc:

Activity and status of ABL temp-tables is available in this virtual system table. The statistics gathered allow you to monitor the operation and performance of deployed client temp-tables. Specify the range of temp-tables to track with the -ttbasetable and -tttablerangesize startup parameters. Specify temp table indexes with -ttbaseindex and -ttindexrangesize startup parameters. For more information monitoring and debugging temp tables with VSTs, see OpenEdge Development: Debugging and Troubleshooting.

Unless there is a way to write application logic to have some dependency on this interval, I don't see any advantage to implementing that particular aspect of the feature. It would complicate the implementation unnecessarily, and I think we already have an approach that is quite low overhead, whether it's 50 tables enabled or all of them.

Eric Faulhaber wrote:

Unless there is a way to write application logic to have some dependency on this interval, I don't see any advantage to implementing that particular aspect of the feature. It would complicate the implementation unnecessarily, and I think we already have an approach that is quite low overhead, whether it's 50 tables enabled or all of them.

I agree. I think that may be it is better not pre-allocate the slots for all tables (to a given interval), but to add the slot dynamically. Please note however that so far I've not considered temporary tables at all.

UserTableStatUpdater core functionality implemented. The exposed public methods are:

• getTableStats(integer connectId, integer tableNo) - returns table statistics holder to a given connection and table
• persist() - incremetally updates _UserTableStat meta table (maybe not in the most efficient way)
• disconnected(int64 connectId) - cleanups the statisctics for a closed connection

Added field for a table statistics holder to the RecordBuffer and a setter for it.

Added map of UserTableStatUpdater by database to the ConnectionManager and invocation of UserTableStatUpdater.disconnected() on disconnect.

Committed to 3821c revision 11463.

1. When table statistics holder should be attached to a RecordBuffer?
2. When a RecordBuffer should update the attached statistics?
3. When a UserTableStatUpdater.persist() should be triggered?

After that some testing is required.

Good job. I think we should pause the work there. Is it safe to include in the branch pending the final changes?

Greg Shah wrote:

Good job. I think we should pause the work there. Is it safe to include in the branch pending the final changes?

I've commented the initialization of the ConnectionManager.tableStatUpdaters field, so now it is 100% safe.
See 3821c revision 11465.

Igor: Are _area, _filelist and _tenant all "complete" (stubbed out with a basic functional set of constant data)?

Am I understanding correctly that only _usertablestat remains to be finished in this task?

Greg Shah wrote:

Igor: Are _area, _filelist and _tenant all "complete" (stubbed out with a basic functional set of constant data)?

Am I understanding correctly that only _usertablestat remains to be finished in this task?

Greg,
For _usertablestat we have to add calls for updating data. The _area, _filelist and _tenant are not done. I was planning to do it after _usertablestat but got another assignment. In fact populating these tables should be very easy. This is why it was posponed "for a dessert"

Assuming _usertablestat is frequently updated, we do not want to take an approach like we have now with the _lock metadata table. See #3896 for the flaws with this implementation.

If _usertablestat is frequently updated, we have to implement an approach for this table similar to that outlined in #3896 for the _lock table. In fact, for any metadata table which is updated frequently due to system state changes, we have to consider that queries against this data will be very infrequent for most applications, relative to the state changes themselves. Thus, we must bias the performance toward the insert/update/delete cases, rather than for the read case. That may mean that the state changes are maintained efficiently in memory and only applied to a metadata table on demand, when a query needs them.

Eric Faulhaber wrote:

Assuming _usertablestat is frequently updated, we do not want to take an approach like we have now with the _lock metadata table. See #3896 for the flaws with this implementation.

If _usertablestat is frequently updated, we have to implement an approach for this table similar to that outlined in #3896 for the _lock table. In fact, for any metadata table which is updated frequently due to system state changes, we have to consider that queries against this data will be very infrequent for most applications, relative to the state changes themselves. Thus, we must bias the performance toward the insert/update/delete cases, rather than for the read case. That may mean that the state changes are maintained efficiently in memory and only applied to a metadata table on demand, when a query needs them.

Sorry. Do you mean something different from what we have now? It seems that most of what you describe is already implemented and we just need to add several calls to the correct places.
Thank you.

Sorry, please ignore my last post. I was going from memory, but upon rereading this issue's history, I see we already went through a lengthy discussion about the design, and I was misremembering. Please continue as we previously discussed.

Eric Faulhaber wrote:

Sorry, please ignore my last post. I was going from memory, but upon rereading this issue's history, I see we already went through a lengthy discussion about the design, and I was misremembering. Please continue as we previously discussed.

No problem, I understand.
It was decided that actual updates of the _usertablestat should be postponed. Do you think that it is time to do it right now?
Thank you.

Yes, now is the time to complete this task.

Greg Shah wrote:

Yes, now is the time to complete this task.

Thank you. Working in this.

Please post an example of the changes you are making (as a diff). I want Eric to have a chance to review before you get to the end.

Greg Shah wrote:

Please post an example of the changes you are making (as a diff). I want Eric to have a chance to review before you get to the end.

OK. I will do it before commit.

Please post it now, so that it is early enough to let Eric provide feedback. It may save time.

Greg Shah wrote:

Please post it now, so that it is early enough to let Eric provide feedback. It may save time.

Greg,
I'm still looking for the right places to call UserTableStatUpdater methods.

I'm looking for the right place for the UserTableStatUpdater.persist() call which flashes collected statistics from the in-memory data structures to the META database and I have two questions.
1. We have one instance of the UserTableStatUpdater per primary database in the ConnectionManager.tableStatUpdaters with primary database id as a key. How can I figure the primary database while processing the following statement:

find first _UserTableStat where _UserTableStat-Conn = _MyConnection._MyConn-UserId and _UserTableStat-Num = _File._file-num no-lock no-error.

2. I understand that there are at least two different types of queries where I should add the call in question: RandomAccessQuery and AdaptiveQuery since they are processed in a different way. Should I consider any other cases?

Thank you.

Igor Skornyakov wrote:

2. I understand that there are at least two different types of queries where I should add the call in question: RandomAccessQuery and AdaptiveQuery since they are processed in a different way. Should I consider any other cases?

PreselectQuery (the superclass of AdaptiveQuery) is the better place for this than AdaptiveQuery.

Still thinking about the other question...

Eric Faulhaber wrote:

PreselectQuery (the superclass of AdaptiveQuery) is the better place for this than AdaptiveQuery.

Thank you!

Igor Skornyakov wrote:

1. We have one instance of the UserTableStatUpdater per primary database in the ConnectionManager.tableStatUpdaters with primary database id as a key. How can I figure the primary database while processing the following statement:
[...]?

You have access to the RecordBuffer object(s) for every query. RecordBuffer.getDatabase returns the corresponding Database object (despite the incorrect javadoc).

Eric Faulhaber wrote:

You have access to the RecordBuffer object(s) for every query. RecordBuffer.getDatabase returns the corresponding Database object (despite the incorrect javadoc).

But in this case the RecordBuffer.getDatabase returns the META database. Or I miss something? I understand that what I need is the default primary database associated with the current session but I do not see how I can get it.
Thank you.

The fact that there is a "meta" database is only a FWD implementation detail. The metadata always should be about a particular, primary database.

For this reason, I think the IDs of the primary and meta Database objects for the same primary database are the same. If you request the ID of the database (i.e., Database.getId), I think it should give you the ID of the primary database, even if the Database object is of type META. But please confirm this.

Eric Faulhaber wrote:

The fact that there is a "meta" database is only a FWD implementation detail. The metadata always should be about a particular, primary database.

For this reason, I think the IDs of the primary and meta Database objects for the same primary database are the same. If you request the ID of the database (i.e., Database.getId), I think it should give you the ID of the primary database, even if the Database object is of type META. But please confirm this.

Thank you, checking...

Igor Skornyakov wrote:

Eric Faulhaber wrote:

PreselectQuery (the superclass of AdaptiveQuery) is the better place for this than AdaptiveQuery.

Thinking more on this, if there is commonality in the changes for RandomAccessQuery and PreselectQuery, then AbstractQuery (the common ancestor of both of these and CompoundQuery) would be an even better place.

In any case, include CompoundQuery in your consideration. I'm not sure if this class needs to be changed, because it delegates to both of the other query types.

Igor Skornyakov wrote:

Eric Faulhaber wrote:

The fact that there is a "meta" database is only a FWD implementation detail. The metadata always should be about a particular, primary database.

For this reason, I think the IDs of the primary and meta Database objects for the same primary database are the same. If you request the ID of the database (i.e., Database.getId), I think it should give you the ID of the primary database, even if the Database object is of type META. But please confirm this.

Yes, you're right. Thank you.

Eric Faulhaber wrote:

Thinking more on this, if there is commonality in the changes for RandomAccessQuery and PreselectQuery, then AbstractQuery (the common ancestor of both of these and CompoundQuery) would be an even better place.

In any case, include CompoundQuery in your consideration. I'm not sure if this class needs to be changed, because it delegates to both of the other query types.

I see, thank you.

There was a bug in the ConnetionManager which prevented the creation of the UserTableStatUpdater instances. After it was fixed I see the following error on the server startup.

com.goldencode.p2j.cfg.ConfigurationException:  Initialization failure
        at com.goldencode.p2j.main.StandardServer.hookInitialize(StandardServer.java:2088)
        at com.goldencode.p2j.main.StandardServer.bootstrap(StandardServer.java:1004)
        at com.goldencode.p2j.main.ServerDriver.start(ServerDriver.java:485)
        at com.goldencode.p2j.main.CommonDriver.process(CommonDriver.java:444)
        at com.goldencode.p2j.main.ServerDriver.process(ServerDriver.java:209)
        at com.goldencode.p2j.main.ServerDriver.main(ServerDriver.java:863)
Caused by: java.lang.RuntimeException: Error preparing MetaLock implementation class
        at com.goldencode.p2j.persist.meta.MetaTableWrapper.<init>(MetaTableWrapper.java:146)
        at com.goldencode.p2j.persist.meta.UserTableStatUpdater.<init>(UserTableStatUpdater.java:28)
        at com.goldencode.p2j.persist.ConnectionManager.initializeMeta(ConnectionManager.java:2339)
        at com.goldencode.p2j.persist.DatabaseManager.initialize(DatabaseManager.java:1137)
        at com.goldencode.p2j.persist.Persistence.initialize(Persistence.java:841)
        at com.goldencode.p2j.main.StandardServer$11.initialize(StandardServer.java:1249)
        at com.goldencode.p2j.main.StandardServer.hookInitialize(StandardServer.java:2084)
        ... 5 more
Caused by: java.lang.ClassNotFoundException: com.goldencode.testcases.dmo._meta.impl.MetaUsertablestatImpl
        at java.net.URLClassLoader.findClass(URLClassLoader.java:382)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
        at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:349)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
        at java.lang.Class.forName0(Native Method)
        at java.lang.Class.forName(Class.java:264)
        at com.goldencode.p2j.persist.meta.MetaTableWrapper.<init>(MetaTableWrapper.java:136)

Error preparing MetaLock implementation class

Something doesn't seem right. Why MetaLock?

Eric Faulhaber wrote:

Something doesn't seem right. Why MetaLock?

I do not know. The constructor is just

   public UserTableStatUpdater(Database metaDatabase)
   {
      this.metaDatabase = metaDatabase;
      this.persistence = PersistenceFactory.getInstance(metaDatabase);
   }

The exception was thrown by

         this.dmoClass = Class.forName(dmoEntity);

Igor Skornyakov wrote:

Eric Faulhaber wrote:

Something doesn't seem right. Why MetaLock?

I do not know. The constructor is just
[...]

The exception was thrown by
[...]
where dmoEntity = com.goldencode.testcases.dmo._meta.impl.MetaUsertablestatImpl.
The MetaLock in the message is just a result of the copy/paste. Fixed.

I think I understand how this should be fixed with the new ORM. Sorry for distirbing.

At some point before Class.forName is called, DmoMeta.registerDmo must have been called for the DMO implementation class to have been assembled and loaded. That must not be happening in your call path.

Eric Faulhaber wrote:

At some point before Class.forName is called, DmoMeta.registerDmo must have been called for the DMO implementation class to have been assembled and loaded. That must not be happening in your call path.

Yes, you're right. I've fixed it already.

I've noticed an interesting thing about _usertablestat.
Consider the following program:

find first _MyConnection no-lock.

for each _UserTableStat where _UserTableStat-Conn = _MyConnection._MyConn-UserId AND _UserTableStat-read > 0 no-lock:
    message 
        _UserTableStat-Conn
        _UserTableStat-Num
        _UserTableStat-create
        _UserTableStat-read
        _UserTableStat-delete
        _UserTableStat-update
   .
end.

PROCEDURE test:
    FIND FIRST words NO-LOCK.
END.

Please note that test is never invoked. However, the read counter for words table is 1.
My understanding of this is that 4GL increases this counter when a table is first seen during the code compilation before the run. I'm not sure if it is possible to implement a 100% compatible behavior in FWD.

This would never be seen in a production application which is compiled in advance. That means that the compilation is done in a separate step from running the application. We don't care about this difference.

Greg Shah wrote:

This would never be seen in a production application which is compiled in advance. That means that the compilation is done in a separate step from running the application. We don't care about this difference.

I see. Thank you.

It seems that we have problems with _MyConnection VST support.
With 4GL the statement find first _MyConnection no-lock. returns a record with a correct value of the _MyConn-UserId field.
With FWD the value of this field is always zero.

This prevents the testing of the persistence layer (operations with VST) of the _UserTableStat support.
Trying to fix it.

I recall that _myconnection was added during #3293 and that there is some kind of "singleton" problem with it. But unfortunately, there is no documentation of this at all. Eric was the one that should know about the singleton issue.

What I see in the H2 trace log is that we add two records to this table and never delete them.
inserts:

/*SQL l:166 #:1*/insert into meta_myconnection (myconn_id, myconn_userid, myconn_pid, myconn_numseqbuffers, myconn_usedseqbuffers, myconn_tenantid, recid) values (?, ?, ?, ?, ?, ?, ?) {1: 1, 2: 0, 3: -1, 4: 0, 5: 0, 6: 0, 7: 10001};
/*SQL l:166 #:1*/insert into meta_myconnection (myconn_id, myconn_userid, myconn_pid, myconn_numseqbuffers, myconn_usedseqbuffers, myconn_tenantid, recid) values (?, ?, ?, ?, ?, ?, ?) {1: 2, 2: 13, 3: 27455, 4: 0, 5: 0, 6: 0, 7: 10003};
/*SQL l:166 #:1*/insert into meta_myconnection (myconn_id, myconn_userid, myconn_pid, myconn_numseqbuffers, myconn_usedseqbuffers, myconn_tenantid, recid) values (?, ?, ?, ?, ?, ?, ?) {1: 3, 2: 14, 3: 27513, 4: 0, 5: 0, 6: 0, 7: 10005};

/*SQL l:68 #:1*/delete \n\t\nfrom\n\tmeta_myconnection metamyconn0_ \nwhere\n\tmyconn_id = ? {1: 3};

This is why we always retrieve the wrong record with FIND FIRST

The first and second inserted records from #3814-282 are a result of the ConnectManager.register() call at the server startup.
I'm not sure that it is correct to register such connections.

Igor Skornyakov wrote:

The first and second inserted records from #3814-282 are a result of the ConnectManager.register() call at the server startup.
I'm not sure that it is correct to register such connections.

The first record insertion can be suppressed by checking Persistence.isActive() flag.

I do not see how to deal with the second one. The stack trace looks pretty innocent:

Daemon Thread [Conversation [0000000D:appserver_process]] (Suspended (breakpoint at line 303 in ConnectTableUpdater))    
    owns: Object  (id=209)    
    ConnectTableUpdater.databaseConnected(int64) line: 303    
    ConnectionManager.putConnected(String, Database, String) line: 4069    
    ConnectionManager.register() line: 514    
    ConnectionManager.get() line: 2359    
    BufferManager.get() line: 713    
    1800022151.createScopeable() line: not available    
    TransactionManager$ContextContainer.obtain() line: 10548    
    TransactionManager.getTransactionHelper() line: 1725    
    ProcedureManager$WorkArea.init() line: 4436    
    ProcedureManager$ContextContainer(ContextLocal<T>).initializeValue(T) line: 655    
    ProcedureManager$ContextContainer(ContextLocal<T>).get(boolean) line: 494    
    ProcedureManager$ContextContainer(ContextLocal<T>).get() line: 430    
    ProcedureManager.getProcedureHelper() line: 1026    
    LogicalTerminal.init() line: 1365    
    LogicalTerminal$1(ContextLocal<T>).initializeValue(T) line: 655    
    LogicalTerminal$1(ContextLocal<T>).get(boolean) line: 494    
    LogicalTerminal$1(ContextLocal<T>).get() line: 430    
    LogicalTerminal.locate() line: 11680    
    LogicalTerminal.setClientMetrics(ClientMetrics) line: 12868    
    LogicalTerminalMethodAccess.invoke(Object, int, Object...) line: not available    
    MethodInvoker.invoke(Object[]) line: 156    
    Dispatcher.processInbound(InboundRequest, boolean, NetResource) line: 783    
    Conversation.block() line: 422    
    Conversation.run() line: 232    
    Thread.run() line: 748    

A 'normal' one is:

Daemon Thread [Conversation [0000000E:bogus]] (Suspended)    
    SQLQuery.list(Session, List<RowStructure>) line: 395    
    Query.list(Session) line: 280    
    Persistence.list(String[], String, Object[], int, int, boolean, boolean) line: 1451    
    ProgressiveResults.getResults(ProgressiveResults$Locator) line: 1114    
    ProgressiveResults.moveTo(int, boolean) line: 945    
    ProgressiveResults.moveTo(int) line: 795    
    ProgressiveResults.next() line: 409    
    ResultsAdapter.next() line: 161    
    AdaptiveQuery.next(LockType) line: 2127    
    AdaptiveQuery(PreselectQuery).next() line: 2536    
    Stat.lambda$null$11(AdaptiveQuery) line: 104    
    2115323143.body() line: not available    
    Block.body() line: 605    
    BlockManager.processForBody(BlockManager$WorkArea, Block, ToClause, LogicalOp, OnPhrase[]) line: 8661    
    BlockManager.forEachWorker(BlockManager$TransactionType, String[], String, ToClause, LogicalOp, OnPhrase[], Block) line: 10406    
    BlockManager.forEach(String, Block) line: 4369    
    Stat.lambda$stat$12() line: 95    
    1788208851.body() line: not available    
    Block.body() line: 605    
    BlockManager.processBody(BlockManager$WorkArea, Block, OnPhrase[]) line: 8559    
    BlockManager.topLevelBlock(Class<?>, Object, String, TransactionType, BlockType, Block) line: 8228    
    BlockManager.internalProcedure(BlockManager$TransactionType, Block) line: 583    
    BlockManager.internalProcedure(Block) line: 569    
    Stat.stat() line: 91    
    StatMethodAccess.invoke(Object, int, Object...) line: not available    
    ControlFlowOps$InternalEntryCaller.invokeImpl(String, Object...) line: 8328    
    ControlFlowOps$InternalEntryCaller.invoke(String, Object...) line: 8299    
    ControlFlowOps.lambda$invokeImpl$10(ControlFlowOps$InternalEntryCaller[], Object, boolean, handle, String, String, boolean, Object[], String, Object[], Exception[]) line: 6828    
    769595127.run() line: not available    
    ControlFlowOps.invokeImpl(List<Resolver>, handle, character, boolean, boolean, boolean, boolean, boolean, handle, String, ArgumentResolver) line: 6843    
    ControlFlowOps.invoke(handle, character, boolean, boolean, boolean, boolean, String, Object...) line: 4066    
    ControlFlowOps.invokeImpl(AsyncRequestImpl, handle, handle, character, boolean, boolean, boolean, boolean, String, Object...) line: 6422    
    ControlFlowOps.invokeImpl(handle, handle, character, boolean, boolean, boolean, boolean, String, Object...) line: 6325    
    ControlFlowOps.invoke(character, Object...) line: 1006    
    ControlFlowOps.invoke(String, Object...) line: 992    
    Stat.lambda$execute$6() line: 84    
    1674664044.body() line: not available    
    Block.body() line: 605    
    BlockManager.processBody(BlockManager$WorkArea, Block, OnPhrase[]) line: 8559    
    BlockManager.topLevelBlock(Class<?>, Object, String, TransactionType, BlockType, Block) line: 8228    
    BlockManager.externalProcedure(Object, BlockManager$TransactionType, Block) line: 496    
    BlockManager.externalProcedure(Object, Block) line: 467    
    Stat.execute() line: 40    
    NativeMethodAccessorImpl.invoke0(Method, Object, Object[]) line: not available [native method]    
    NativeMethodAccessorImpl.invoke(Object, Object[]) line: 62    
    DelegatingMethodAccessorImpl.invoke(Object, Object[]) line: 43    
    Method.invoke(Object, Object...) line: 498    
    Utils.invoke(Class<?>, Method, Object...) line: 1548    
    StandardServer$MainInvoker.execute() line: 2210    
    StandardServer.invoke(int, Isolatable) line: 1646    
    StandardServer.standardEntry() line: 581    
    StandardServerMethodAccess.invoke(Object, int, Object...) line: not available    
    MethodInvoker.invoke(Object[]) line: 156    
    Dispatcher.processInbound(InboundRequest, boolean, NetResource) line: 783    
    Conversation.block() line: 422    
    Conversation.run() line: 232    
    Thread.run() line: 748    

Greg, is right, we did not implement the _myconnection table correctly. For each context/session, there should only be one record in the table: the one with that session's connection ID.

Probably the best way to implement this is to not create a shared meta_myconnection table at server startup, but rather to create a separate, private/local meta_myconnection temp-table per session. It would be created when the session connection occurs and dropped when the session ends. Only one record is ever added to the table, that of the current session's connection ID. Thus, FIND statements or queries involving this table should convert and work naturally, and will only ever have one record to find.

Eric Faulhaber wrote:

Greg, is right, we did not implement the _myconnection table correctly. For each context/session, there should only be one record in the table: the one with that session's connection ID.

Probably the best way to implement this is to not create a shared meta_myconnection table at server startup, but rather to create a separate, private/local meta_myconnection temp-table per session. It would be created when the session connection occurs and dropped when the session ends. Only one record is ever added to the table, that of the current session's connection ID. Thus, FIND statements or queries involving this table should convert and work naturally, and will only ever have one record to find.

I'm not sure that I understand your idea. I was thinking about re-writing queries that retrieve data from the meta_myconnection by adding a condition that guarantees a result set with a single correct record.
Anyway, I understand that w/o the correct support of the _myconnection VST the support for _usertablestat is mostly useless since common use cases of it are based on _myconnection. Should I switch to fixing the issue?
Thank you.

I do think we need to work on correcting the _myconnection implementation sooner rather than later.

The implementation idea I proposed above is based on my understanding that the _myconnection VST is special, in that it only contains a single record from the perspective of the context querying it, but that record is specific to each context. This understanding should be verified, as it is based only on use cases I have seen in applications, but I have not confirmed that there is no more than one record. This should be easy enough to do with a simple FOR EACH loop. I am pretty sure a _myconnection VST exists for each connected, primary database (i.e., db1._myconnection, db2._myconnection, ...), as opposed to 1 _myconnection table across all connected, primary databases, but this should be verified as well.

If the above assumptions are correct, then one can think of this VST essentially as a view on the full set of active _myconnection records to a particular, primary database, across all contexts, which is constrained for any particular context by the implicit filter _myconnection._myconn-id = <current context's unique connection id>. So, my first thought was that we maintain a "normal" table meta_myconnection, which has a record added every time a new connection (in the 4GL sense) is established, and that record is deleted when the 4GL connection ends, using a private SQL view to constrain each context to see only the relevant record. However, AFAICT, there is no ability to create a private view (like a private temp-table) in H2. A view in H2 appears to be a global resource that can be accessed by all JDBC connections.

So, my next thought was that instead of maintaining a master meta_myconnection table containing records for all active 4GL connections, we instead create a private temp-table for each context, which only that context's JDBC connection can see. We create the record representing that context's (4GL) connection in the temp-table at the time the (4GL) connection is established, and we delete it (and drop the temp-table altogether) when the 4GL connection ends. That way, the meta_myconnection temp-table only contains the record representing that context's (4GL) connection, and it can be used in normal queries alongside other meta tables, without any special rewriting of each query.

Let me know if that makes sense.

Eric Faulhaber wrote:

I do think we need to work on correcting the _myconnection implementation sooner rather than later.

The implementation idea I proposed above is based on my understanding that the _myconnection VST is special, in that it only contains a single record from the perspective of the context querying it, but that record is specific to each context. This understanding should be verified, as it is based only on use cases I have seen in applications, but I have not confirmed that there is no more than one record. This should be easy enough to do with a simple FOR EACH loop. I am pretty sure a _myconnection VST exists for each connected, primary database (i.e., db1._myconnection, db2._myconnection, ...), as opposed to 1 _myconnection table across all connected, primary databases, but this should be verified as well.

Well, it seems that your assumption is correct. At least if I open two instances of the 4GL Procedure Editor connected to the same database and dump the content of the _MyConnection VST from both I get two files containing a single record but these records are different. So logically there exist a table with a single record for every connection.

If the above assumptions are correct, then one can think of this VST essentially as a view on the full set of active _myconnection records to a particular, primary database, across all contexts, which is constrained for any particular context by the implicit filter _myconnection._myconn-id = <current context's unique connection id>. So, my first thought was that we maintain a "normal" table meta_myconnection, which has a record added every time a new connection (in the 4GL sense) is established, and that record is deleted when the 4GL connection ends. However, AFAICT, there is no ability to create a private view (like a private temp-table) in H2. A view in H2 appears to be a global resource that can be accessed by all JDBC connections.

So, my next thought was that instead of maintaining a master meta_myconnection table containing records for all active 4GL connections, we instead create a private temp-table for each context, which only that context's JDBC connection can see. We create the record representing that context's (4GL) connection in the temp-table at the time the (4GL) connection is established, and we delete it (and drop the temp-table altogether) when the 4GL connection ends. That way, the meta_myconnection temp-table only contains the record representing that context's (4GL) connection, and it can be used in normal queries alongside other metatables, without any special rewriting of each query.

Let me know if that makes sense.

I was thinking about the global view but this view will need a parameter that depends on the session. Actually what I've suggested before (re-writing SQL query that retrieves data from meta_myconnection by adding a session-based condition can be considered as a variant of the view-based approach.

We already drop the meta_myconnection record when the connection is closed so it will not result in too large meta_myconnection.

Your suggestion with temp tables looks cleaner than mine but seems to be more complicated to implement (at least for me). It requires multiple changes in many places.

However, if you believe that my idea is not good enough, I will start working on your approach.

What do you think?

Thank you.

Igor Skornyakov wrote:

Your suggestion with temp tables looks cleaner than mine but seems to be more complicated to implement (at least for me). It requires multiple changes in many places.

It seems to me this can be driven primarily from the ConnectionManager. Where are the many places you have in mind?

Eric Faulhaber wrote:

It seems to me this can be driven primarily from the ConnectionManager. Where are the many places you have in mind?

I mean at least changes in the META database creation and population. And we will have to change all places where we work with this table if it will be really located in _temp.
Or you mean that we will create meta_myconnection in META but as a temporary table? In this case, we should change DDL generation or modify the DDL statement at runtime.

Sorry, I should have used the term temporary table, so as not to confuse with temp-table, which is the 4GL term.

This temporary table must be in the _meta database, not in the _temp database.

Eric Faulhaber wrote:

Sorry, I should have used the term temporary table, so as not to confuse with temp-table, which is the 4GL term.

This temporary table must be in the _meta database, not in the _temp database.

I see. Thank you. Will implement this approach.

Igor Skornyakov wrote:

Eric Faulhaber wrote:

Sorry, I should have used the term temporary table, so as not to confuse with temp-table, which is the 4GL term.

This temporary table must be in the _meta database, not in the _temp database.

I see. Thank you. Will implement this approach.

I understand that using a local temporary table for the _UserTableStat requires a separate connection to the META database for each connection to the primary database. It seems that this requires significant changes in the core logic since now FWD uses a single connection.
Is my understanding correct?
Thank you.

Igor Skornyakov wrote:

I understand that using a local temporary table for the _UserTableStat requires a separate connection to the META database for each connection to the primary database. It seems that this requires significant changes in the core logic since now FWD uses a single connection.
Is my understanding correct?

I am not suggesting any change to your planned support for the _UserTableStat table. But for the _Myconnection table to be a local temporary table, yes, it would require that a separate JDBC connection to the _meta database be maintained for the life of each session.

Alternatively, this temporary table and its single record would have to be created each time a JDBC connection is made for a query against the _Myconnection table is made.

Either way is a bit messy, admittedly. Perhaps we need to reconsider the query rewriting approach you suggested. Please describe the details of this approach, so we can discuss...

Eric Faulhaber wrote:

I am not suggesting any change to your planned support for the _UserTableStat table. But for the _Myconnection table to be a local temporary table, yes, it would require that a separate JDBC connection to the _meta database be maintained for the life of each session.

Sorry, it was a misprint. I mean _MeConnection.

Alternatively, this temporary table and its single record would have to be created each time a JDBC connection is made for a query against the _Myconnection table is made.

Either way is a bit messy, admittedly. Perhaps we need to reconsider the query rewriting approach you suggested. Please describe the details of this approach, so we can discuss...

I suggested adding an additional condition to all SELECTs from _MyConnection so that only the record for the current session will be retrieved (or none if other conditions were specified). Since no INSERT/UPDATE/DELETE operations are allowed for the _MyConnection this should be sufficient. It is like defining a read-only view that uses a connection-specific server-side value. I've used this trick in the past but it is not applicable in the situation in question.

Igor Skornyakov wrote:

I suggested adding an additional condition to all SELECTs from _MyConnection so that only the record for the current session will be retrieved (or none if other conditions were specified). Since no INSERT/UPDATE/DELETE operations are allowed for the _MyConnection this should be sufficient. It is like defining a read-only view that uses a connection-specific server-side value. I've used this trick in the past but it is not applicable in the situation in question.

One more suggestion. Create an auxiliary table and view:

create table meta_myconnection_ (
   recid bigint not null,
   myconn_id bigint,
   myconn_userid integer,
   myconn_pid integer,
   myconn_numseqbuffers integer,
   myconn_usedseqbuffers integer,
   myconn_tenantid integer,
   session_id integer default session_id(),
   primary key (recid)
);

CREATE VIEW meta_myconnection AS
SELECT RECID, MYCONN_ID, MYCONN_USERID, MYCONN_PID, MYCONN_NUMSEQBUFFERS, MYCONN_USEDSEQBUFFERS, MYCONN_TENANTID FROM META_MYCONNECTION_
WHERE session_id = session_id();
);

Since we insert into _MyConnection only in the ConnectTableUpdater it is not a problem to use meta_myconnection_ instead of meta_myconnection. In all other places only at most a single record will be retrieved automatically.

Igor Skornyakov wrote:

Igor Skornyakov wrote:

I suggested adding an additional condition to all SELECTs from _MyConnection so that only the record for the current session will be retrieved (or none if other conditions were specified). Since no INSERT/UPDATE/DELETE operations are allowed for the _MyConnection this should be sufficient. It is like defining a read-only view that uses a connection-specific server-side value. I've used this trick in the past but it is not applicable in the situation in question.

One more suggestion. Create an auxiliary table and view:
[...]

Since we insert into _MyConnection only in the ConnectTableUpdater it is not a problem to use meta_myconnection_ instead of meta_myconnection. In all other places only at most a single record will be retrieved automatically.

If we create a corresponding INSTEAD OF triggers for the view, then only changes in the DDL generation for the META will be required.

Igor Skornyakov wrote:

If we create a corresponding INSTEAD OF triggers for the view, then only changes in the DDL generation for the META will be required.

Please illustrate this idea further. I have not used INSTEAD OF triggers, but I understand something like this would be needed to execute DML using the view?

Eric Faulhaber wrote:

Please illustrate this idea further. I have not used INSTEAD OF triggers, but I understand something like this would be needed to execute DML using the view?

Exactly. H2 does not have updatable views, so INSTEAD OF triggers are used in the situations as we discuss here. See e.g.
https://github.com/h2database/h2database/blob/master/h2/src/test/org/h2/samples/UpdatableView.java

I'm not sure why the trigger would be needed in this case. The _Myconnection VST is read-only for general use. The only updates will be made by FWD runtime logic at well-known times. It seems like this logic can simply operate on the real table backing the view, and any business logic queries will just operate on the read-only view. Is there some other reason for the trigger I am overlooking?

If I understand correctly, each context will use the view to see only its "own" _Myconnection record. If that is the case, why do we need to do any SQL rewriting?

Eric Faulhaber wrote:

I'm not sure why the trigger would be needed in this case. The _Myconnection VST is read-only for general use. The only updates will be made by FWD runtime logic at well-known times. It seems like this logic can simply operate on the real table backing the view, and any business logic queries will just operate on the read-only view. Is there some other reason for the trigger I am overlooking?

The reason is to avoid any changes in the Java code. All we need is a new trigger class and change in the DDL generation.

If I understand correctly, each context will use the view to see only its "own" _Myconnection record. If that is the case, why do we need to do any SQL rewriting?

With the view-based approach, no SQL re-writing is required. At this moment I'm testing this with manually changed meta_table_ddl.sql.

Another question...isn't

   session_id integer default session_id(),

redundant with _MyConn-Id, which already is guaranteed to be unique, and is a value that we create and track per session/connection?

It seems like the view could be filtered on _MyConn-Id, rather than introducing another unique value to the table.

Igor Skornyakov wrote:

The reason is to avoid any changes in the Java code. All we need is a new trigger class and change in the DDL generation.

Sorry, I'm not following... do you mean the FWD runtime code, or the converted business logic? If the former, there is no such requirement; I would rather change the runtime code to make the overall design simpler. If the latter, I don't understand what changes would be necessary to the converted code, if we are using the view.

Eric Faulhaber wrote:

It seems like the view could be filtered on _MyConn-Id, rather than introducing another unique value to the table.

I think that it is not redundant since session_id holds the server-side value which can be different from the one we use in FWD. At least this is how I understand it. We can use only server-side values in the view definition.

Eric Faulhaber wrote:

Sorry, I'm not following... do you mean the FWD runtime code, or the converted business logic? If the former, there is no such requirement; I would rather change the runtime code to make the overall design simpler. If the latter, I don't understand what changes would be necessary to the converted code, if we are using the view.

I'm talking about FWD runtime. Without a trigger, we have to completely re-write the ConnectTableUpdater code to use plain SQL. These changes are more intrusive than ones required by the trigger-based approach.

As we see the SESSION_ID value is always 1 while MYCONN_ID is different for different records.

So we should either use different connections to the META database for different sections and use (temporary meta_myconnection)/(view+trigger) or use SQL-rewriting for SELECTs from the meta_myconnection.

I think that the SQL-rewriting is simpler to implement and it is less intrusive.

There is no physically separate "server-side" in this case. The _meta database is an embedded, in-memory database, running within the FWD server. Can we not take advantage of this co-existence and create a UDF myconn_id(), which returns a context-local _MyConn-Id, so that we can base the view on this instead of session_id()? This will allow us to keep the meta_myconnection_ table identical to its original structure, and create a view like this:

CREATE VIEW meta_myconnection AS
SELECT recid, myconn_id, myconn_userid, myconn_pid, myconn_numseqbuffers, myconn_usedseqbuffers, myconn_tenantid from meta_myconnection_
WHERE myconn_id = myconn_id();

The context-local _MyConn-Id will have to be the same value across connected, primary databases, but that should not be a problem. I am not aware of any requirement for this value to be unique across primary databases.

Eric Faulhaber wrote:

The context-local _MyConn-Id will have to be the same value across connected, primary databases, but that should not be a problem. I am not aware of any requirement for this value to be unique across primary databases.

Well, I can try. Please note however that this will make it impossible to use the remote META database. I use this for testing and, if I understand correctly, it is was possible to configure META to expose the server before.

I've created a function

   @HQLFunction
   public static int fwdSession()
   {
      return SecurityManager.getInstance().getSessionId().intValue();
   }

org.h2.jdbc.JdbcSQLSyntaxErrorException: Function "FWDSESSION" not found; SQL statement:

create table meta_myconnection_ (
   recid bigint not null,
   myconn_id bigint,
   myconn_userid integer,
   myconn_pid integer,
   myconn_numseqbuffers integer,
   myconn_usedseqbuffers integer,
   myconn_tenantid integer,
   session_id integer default fwdSession(),
   primary key (recid)
) [90022-200]

I think the UDF definition is correct, but please remove the session_id column and the use of the UDF in the create table statement. The backing table should not contain any additional columns, which are not in the original VST.

When inserting a record in your prototype, use the FWD session value for the myconn_id column. This will be unique to the context (though not to the combination of context and primary database, but I think that's ok).

What we need to know to determine whether the view approach is viable is whether the view can be created using the UDF in the view's WHERE filter expression. Does this work?

I think the UDF definition is correct, but please remove the session_id column and the use of the UDF in the create table statement. The backing table should not contain any additional columns, which are not in the original VST.

Sorry, I do not understand. It doesn't matter if we add a column or not. Anyway, we need to have a table containing all records and a view that contains a subset. The question is how to define the view. The session_id() UDF doesn't work, a UDF based on FWD data is not accepted by H2, at least in the table/view definition.

When inserting a record in your prototype, use the FWD session value for the myconn_id column. This will be unique to the context (though not to the combination of context and primary database, but I think that's ok).

The value of the myconn_id is already correct.

What we need to know to determine whether the view approach is viable is whether the view can be created using the UDF in the view's WHERE filter expression. Does this work?

As I wrote, H2 seems not to understand a UDF definition that I've provided. This means that it cannot be used in a view.

Igor Skornyakov wrote:

As I wrote, H2 seems not to understand a UDF definition that I've provided. This means that it cannot be used in a view.

This was my stupid fault, sorry.

With the following manual change the logic works for a first client session:

create table meta_myconnection_ (
   recid bigint not null,
   myconn_id bigint,
   myconn_userid integer,
   myconn_pid integer,
   myconn_numseqbuffers integer,
   myconn_usedseqbuffers integer,
   myconn_tenantid integer,
   primary key (recid)
);

create view meta_myconnection as
select * from meta_myconnection_
where myconn_userid = fwdSession();

create trigger meta_myconnection_trg instead of insert, update, delete on meta_myconnection for each row call "com.goldencode.p2j.persist.h2.UpdatableMyConnectionView";

However, with subsequent connections, I have some problems. Working on this.

The cache logic in the RandomAccessQuery.execute() seems to be not 100% correct. For _MyConnection it finds the RecordIdentifier which points to the ready deleted record. Moreover, even if the record was not deleted using the cached RecordIdentifier is incorrect.

I think that the caching logic for the case when whereExpr == null is seriously broken.

Igor Skornyakov wrote:

The cache logic in the RandomAccessQuery.execute() seems to be not 100% correct. For _MyConnection it finds the RecordIdentifier which points to the ready deleted record. Moreover, even if the record was not deleted using the cached RecordIdentifier is incorrect.

I think that the caching logic for the case when whereExpr == null is seriously broken.

I've disabled the RandomAccessQuery.ffCache usage for meta tables and now MyConnction support looks working as expected.

However, I still think that something is wrong with this cache.

Working on DDL generation for MyConnection (so far I was tested with manually changed meta_table_ddl.sql)

Igor Skornyakov wrote:

I've disabled the RandomAccessQuery.ffCache usage for meta tables and now MyConnction support looks working as expected.

However, I still think that something is wrong with this cache.

Yes, we probably don't have invalidation logic for places where we update/insert/delete metadata records. The cache requires invalidation whenever a record is inserted or updated or deleted. The validation is index-based and is scoped as narrowly as possible, depending on the type of change.

We have adjusted (hopefully all of) the code paths for "normal" tables, but I expect we have missed most if not all of the metadata paths.

Eric Faulhaber wrote:

Yes, we probably don't have invalidation logic for places where we update/insert/delete metadata records. The cache requires invalidation whenever a record is inserted or updated or deleted. The validation is index-based and is scoped as narrowly as possible, depending on the type of change.

We have adjusted (hopefully all of) the code paths for "normal" tables, but I expect we have missed most if not all of the metadata paths.

I do not think that it makes much sense to use cache with metatables, at least for requests w/o WHERE clause.

Igor Skornyakov wrote:

I do not think that it makes much sense to use cache with metatables, at least for requests w/o WHERE clause.

Please support that opinion with some further explanation. If we are going to make a blanket exclusion of metadata from the caching, there should be a good reason, and it should be documented in the code where the exclusion is made.

Eric Faulhaber wrote:

Igor Skornyakov wrote:

I do not think that it makes much sense to use cache with metatables, at least for requests w/o WHERE clause.

Please support that opinion with some further explanation. If we are going to make a blanket exclusion of metadata from the caching, there should be a good reason, and it should be documented in the code where the exclusion is made.

Well, I assume that metatables do not contain too many records (the most heavily populated ones are _File, _Field, maybe _Index, and _IndexField).
I cannot imagine a business logic which uses these table very heavily, especially w/o WHERE clause.

In any case caching is an error-prone technique and should be used only when it is really required and such a decision should be made based on profiling (in a broad sense), not on common sense or "just in case". I believe that the famous D.Knuth's statement about premature optimization is fully applicable here.

In a short, I believe that the right question is not why we do not use caching for metadata but why we should use it. After all, caching comes at a cost, both in terms of more complicated program logic and regarding memory consumption. Using cache for the data from the in-memory database looks very questionable for me.

In addition to a previous note.

In contrast with the situation with customer tables (both primary and temporary), we know everything about metatables, moreover, we understand (more or less) the typical queries against them. This means that if any performance issues will be encountered with some of them, we can implement more specific solutions, such as adding additional indices to the generated DDL scripts or a "smart" caching strategy using the fact that these tables are modified only (mostly) by FWD code.

Eric will respond more fully to your concerns. But one thing I can point out here is that the converted 4GL code is submitting arbitrary queries for these tables. The classes that implement these queries are primarily used for non-meta tables. This means that when we use these on meta tables we will find them to be more fragile to the degree that the meta tables work differently.

In other words, I think Eric is trying to keep as much to the legacy rules with the meta tables as possible, to minimize the places that could break. Even so, we still often find many bugs in this area. This is out of proportion with the absolute number of queries executed since meta tables are "rarely" used relative to non-meta tables. So meta tables cause far more than their "fair share" of problems. If the elimination of caching or other differences for meta tables can be cleanly isolated inside a layer that is specific to meta tables, then perhaps it is less of an issue.

I agree that the performance of the meta tables is secondary, but the minimizing the stability is important.

Greg Shah wrote:

Eric will respond more fully to your concerns. But one thing I can point out here is that the converted 4GL code is submitting arbitrary queries for these tables. The classes that implement these queries are primarily used for non-meta tables. This means that when we use these on meta tables we will find them to be more fragile to the degree that the meta tables work differently.

In other words, I think Eric is trying to keep as much to the legacy rules with the meta tables as possible, to minimize the places that could break. Even so, we still often find many bugs in this area. This is out of proportion with the absolute number of queries executed since meta tables are "rarely" used relative to non-meta tables. So meta tables cause far more than their "fair share" of problems. If the elimination of caching or other differences for meta tables can be cleanly isolated inside a layer that is specific to meta tables, then perhaps it is less of an issue.

I understand this. However, disabling cache for RandomAccessQuery against metatables is a very local change (a one-liner). My point is that it can make sense to accept it for now until we will encounter real-life issues with access to the metadata from the application code.

Adding cache invalidation logic for these tables can be much more time-consuming and error-prone.

Added additional configuration options for the 'remote-meta' mode to simplify the development/debugging of the metadata-related code (access to the _meta database from an external database client). The default behavior is not affected.

Committed to 3821c/12264.

I am ok with disabling the caching for metadata tables, for the reasons already discussed above. If we find some performance issue with the more frequently accessed tables, we can revisit this.

Still reviewing the remaining changes...

I've tested my (not yet committed) changes with a large customer app.
I've noticed a problem with _UserTableStat on the server startup:

Caused by: com.goldencode.p2j.util.StopConditionException: [00000011:00000029:GenerateInitialDataProcess-->local/mtc/meta] Error executing SQL statement:  SELECT ID FROM meta_usertablestat WHERE USERTABLESTAT_CONN = ?1 AND USERTABLESTAT_NUM = ?2
Caused by: com.goldencode.p2j.persist.PersistenceException: Failed to execute [SELECT ID FROM meta_usertablestat WHERE USERTABLESTAT_CONN = ?1 AND USERTABLESTAT_NUM = ?2]
Caused by: org.h2.jdbc.JdbcSQLSyntaxErrorException: Column "ID" not found; SQL statement:
SELECT ID FROM meta_usertablestat WHERE USERTABLESTAT_CONN = ?1 AND USERTABLESTAT_NUM = ?2 [42122-200]

Igor Skornyakov wrote:

I've tested my (not yet committed) changes with a large customer app.
I've noticed a problem with _UserTableStat on the server startup:
[...]
Indeed, there is no ID column in the meta_usertablestat. I understand that it should be USERTABLESTAT_ID or RECID. Not sure if this is a runtime or conversion error. The corresponding converted statement is FIND FIRST containing only _UserTableStat._UserTableStat-Num and _UserTableStat._UserTableStat-Conn. so it looks more like a runtime issue, but at this moment I cannot understand its nature.

See UserTableStatUpdater.updateRecord at line 225:

         Long id = (Long)persistence.getSingleSQLResult(
               "SELECT ID FROM " + USERTABLESTAT_TABLE + " WHERE " + 
                     "USERTABLESTAT_CONN = ?1 AND USERTABLESTAT_NUM = ?2",
                new Object[] {connId.longValue(), tblId.longValue()});

This file needs a file header, BTW.

Eric Faulhaber wrote:

See UserTableStatUpdater.updateRecord at line 225:

[...]

This file needs a file header, BTW.

I see. Thank you! Actually, I've added the header. See most recent .diff.
Thank you.

I'm running the following test program:

DEF VAR hBuffer AS HANDLE NO-UNDO.
DEF VAR hQuery AS HANDLE NO-UNDO.

CREATE BUFFER hBuffer FOR TABLE 'customer'.
CREATE QUERY hQuery.
hQuery:SET-BUFFERS(hBuffer).
hQuery:QUERY-PREPARE('FOR EACH customer NO-LOCK').
hQuery:QUERY-OPEN().

FOR EACH Customer:
  DELETE Customer.
END.

  DO TRANSACTION:        
    IF hBuffer:BUFFER-CREATE() THEN DO:      
        hBuffer:BUFFER-FIELD('customerNum'):BUFFER-VALUE= 1234.      
        hBuffer:BUFFER-FIELD('customerName'):BUFFER-VALUE='xxx'.       
    END.
    ELSE
        MESSAGE 'Error in create record'.    
    hBuffer:BUFFER-RELEASE().
  END. 

  hQuery:GET-FIRST().

  DO TRANSACTION:        
     hQuery:GET-CURRENT(EXCLUSIVE-LOCK).        
     hBuffer:BUFFER-FIELD('customerName'):BUFFER-VALUE='yyy'.       
     hBuffer:BUFFER-RELEASE().
  END.    

  hQuery:GET-FIRST().

  DO TRANSACTION:        
      hQuery:GET-CURRENT(EXCLUSIVE-LOCK).        
      hBuffer:BUFFER-DELETE().    
  END.

With 4GL I see 1 creates, 5 reads, 1 delete, and 2 updates.
However, with FWD I do not see any records created in the Customer table.
What I'm doing wrong?
Thank you.

Without debugging, I do not know. What does the program do in FWD? Are you seeing the error message after the BUFFER-CREATE()?

Igor Skornyakov wrote:

I'm running the following test program:
[...]

With 4GL I see 1 creates, 5 reads, 1 delete, and 2 updates.
However, with FWD I do not see any records created in the Customer table.
What I'm doing wrong?
Thank you.

In addition. If I attempt to add a record with a duplicate primary key by repeating the corresponding code fragment, FWD reports an error, but the table is still empty. However, 4GL displays a modal message box while FWD does not. In addition FWD generates an error 142 'Unable to update field; which I do not see in 4GL.

I did a quick conversion and run your testcase with show_sql=true for persistent database in my directory.xml.
Here are the filtered results for local/fwd/primary:

select customer__0_.recid as col_0_0_ from customer customer__0_ order by customer__0_.customernum asc

select nextval('p2j_id_generator_sequence') from generate_series(1, 1000)

select 0 from customer where recid != 780000 and customernum = 1234

insert into customer (customernum, customername, customeraddress, customeremail, active, turnover, employees, established, created, updated, updated_offset, logo, description, recid) values (1234, 'xxx', '', '', 'FALSE', '0E-10', 0, NULL, NULL, NULL, NULL, NULL, NULL, 780000)

select customer__0_.recid as id0_, customer__0_.customernum as customernum1_0_, customer__0_.customername as customername2_0_, customer__0_.customeraddress as customeraddress3_0_, customer__0_.customeremail as customeremail4_0_, customer__0_.active as active5_0_, customer__0_.turnover as turnover6_0_, customer__0_.employees as employees7_0_, customer__0_.established as established8_0_, customer__0_.created as created9_0_, customer__0_.updated as updated10_0_, customer__0_.updated_offset as column_11_0_, customer__0_.logo as logo12_0_, customer__0_.description as description13_0_ from customer customer__0_ order by customer__0_.customernum asc limit 1

update customer set customername='yyy' where recid=780000

select customernum, customername, customeraddress, customeremail, active, turnover, employees, established, created, updated, updated_offset, logo, description from customer where recid=780000

delete from customer where recid = 780000

It looks fine to me at this level.

Eric Faulhaber wrote:

Without debugging, I do not know. What does the program do in FWD? Are you seeing the error message after the BUFFER-CREATE()?

No, I do not see any errors or exceptions. The program just does nothing.

See testcases/meta/stat1.p in rev. 1037

Thank you.

Ovidiu Maxiniuc wrote:

I did a quick conversion and run your testcase with show_sql=true for persistent database in my directory.xml.
Here are the filtered results for local/fwd/primary:

[...]

It looks fine to me at this level.

Oh, sorry. I've set the breakpoint in the wrong place.
Thank you.

BTW, to see what exactly there is stored in a table at a given moment you can put a breakpoint in your IDE and execute:

    System.out.println(((BufferImpl) hBuffer.getResource()).buffer().sqlTableContent(0));

It will show you the content of the table at that table from the POV of the current context. If you execute

   select * from customer

in a SQL console, you may not get the same result, as the transactions might not be committed.

Ovidiu Maxiniuc wrote:

BTW, to see what exactly there is stored in a table at a given moment you can put a breakpoint in your IDE and execute:
[...]

It will show you the content of the table at that table from the POV of the current context. If you execute
[...]

in a SQL console, you may not get the same result, as the transactions might not be committed.

I see. Thank you!

Sometimes I get the following exception:

[04/14/2021 14:12:57 GMT+03:00] (com.mchange.v2.log.slf4j.Slf4jMLog$Slf4jMLogger$WarnLogger:WARNING) com.mchange.v2.resourcepool.BasicResourcePool@2dc319cf -- an attempt to checkout a resource was interrupted, and the pool is still live: some other thread must have interrupted the Thread attempting checkout!
java.lang.InterruptedException
        at java.lang.Object.wait(Native Method)
        at com.mchange.v2.resourcepool.BasicResourcePool.awaitAvailable(BasicResourcePool.java:1465)
        at com.mchange.v2.resourcepool.BasicResourcePool.prelimCheckoutResource(BasicResourcePool.java:644)
        at com.mchange.v2.resourcepool.BasicResourcePool.checkoutResource(BasicResourcePool.java:554)
        at com.mchange.v2.c3p0.impl.C3P0PooledConnectionPool.checkoutAndMarkConnectionInUse(C3P0PooledConnectionPool.java:758)
        at com.mchange.v2.c3p0.impl.C3P0PooledConnectionPool.checkoutPooledConnection(C3P0PooledConnectionPool.java:685)
        at com.mchange.v2.c3p0.impl.AbstractPoolBackedDataSource.getConnection(AbstractPoolBackedDataSource.java:140)
        at com.goldencode.p2j.persist.orm.JdbcDataSource.getConnection(JdbcDataSource.java:107)
        at com.goldencode.p2j.persist.orm.Session.<init>(Session.java:273)
        at com.goldencode.p2j.persist.orm.Session.<init>(Session.java:222)
        at com.goldencode.p2j.persist.Persistence$Context.getSession(Persistence.java:4476)
        at com.goldencode.p2j.persist.Persistence$Context.beginTransaction(Persistence.java:4274)
        at com.goldencode.p2j.persist.Persistence.beginTransaction(Persistence.java:3048)
        at com.goldencode.p2j.persist.Persistence.beginTransaction(Persistence.java:3029)
        at com.goldencode.p2j.persist.Persistence.beginTransaction(Persistence.java:2767)
        at com.goldencode.p2j.persist.meta.UserTableStatUpdater.disconnected(UserTableStatUpdater.java:308)
        at com.goldencode.p2j.persist.meta.UserTableStatUpdater.disconnected(UserTableStatUpdater.java:136)
        at com.goldencode.p2j.persist.meta.ConnectTableUpdater.databaseDisconnected(ConnectTableUpdater.java:426)

Igor Skornyakov wrote:

Sometimes I get the following exception:
[...]
I've never seen that before. Please note that it happens not always. What I'm doing wrong?
Thank you.

It seems that it happens only at the first client session end.

More about the issue with InterruptedExcception.
I have two tests in testcases/meta: stat.p and stat1.p. I see the issue only with stat1.p. The most obvious difference between these tests is that stat.p contains only SELECT queries and does not use explicit transactions.

I have a question that can be related to this difference (or can be not).
Does an explicit transaction block DO TRANSACTION: ... END. affect operations with the _meta database such as counters' updates/flash or it is only about a primary database?
Thank you.

Igor Skornyakov wrote:

More about the issue with InterruptedExcception.
I have two tests in testcases/meta: stat.p and stat1.p. I see the issue only with stat1.p. The most obvious difference between these tests is that stat.p contains only SELECT queries and does not use explicit transactions.

I have a question that can be related to this difference (or can be not).
Does an explicit transaction block DO TRANSACTION: ... END. affect operations with the _meta database such as counters' updates/flash or it is only about a primary database?
Thank you.

To the degree a metadata table is writable and is updated from converted application logic, the legacy support for block-level, application-level transactions applies. However, most state updates for metadata tables occurs within the runtime itself, where transactions are managed explicitly by runtime code. For writable tables, we need to be mindful in the runtime code of whether an application-level transaction is active when we make updates. The early metadata runtime code was not necessarily written with this in mind, because I don't think we were considering that application-level writes to metadata tables were possible when that was written.

Eric Faulhaber wrote:

To the degree a metadata table is writable and is updated from converted application logic, the legacy support for block-level, application-level transactions applies. However, most state updates for metadata tables occurs within the runtime itself, where transactions are managed explicitly by runtime code. For writable tables, we need to be mindful in the runtime code of whether an application-level transaction is active when we make updates. The early metadata runtime code was not necessarily written with this in mind, because I don't think we were considering that application-level writes to metadata tables were possible when that was written.

I see. Thank you. I'm specifically concerned about the situation when the application code reads _UserTableStat table within an application-level transaction which can cause the update of this table.

I've noticed a strange thing. As I wrote before, I have two tests stat.p and stat1.p. I convert both of them.
If I switch between them in the directory.xml then the client executes the test I expect.
However, the Swing client seems to "remember" the test which was executed previously and executes it regardless of the changes in the directory.xml and client.xml.
It seems that I've not experienced this problem before.
What I'm doing wrong?
Thank you.

Igor Skornyakov wrote:

I've noticed a strange thing. As I wrote before, I have two tests stat.p and stat1.p. I convert both of them.
If I switch between them in the directory.xml then the client executes the test I expect.
However, the Swing client seems to "remember" the test which was executed previously and executes it regardless of the changes in the directory.xml and client.xml.
It seems that I've not experienced this problem before.
What I'm doing wrong?
Thank you.

Sorry, It was my stupid mistake. Please disregard.

One more difference between the two tests.
With stat.p I see a normal decorated window from the very beginning. With the first run of the stat1.p after the server start (when I get InterruptedException at the client session close), I see just a blank rectangle for a couple of seconds. In the subsequent runs, I see the normal window from the very beginning and no InterruptedException.

Igor Skornyakov wrote:

One more difference between the two tests.
With stat.p I see a normal decorated window from the very beginning. With the first run of the stat1.p after the server start (when I get InterruptedException at the client session close), I see just a blank rectangle for a couple of seconds. In the subsequent runs, I see the normal window from the very beginning and no InterruptedException.

I suspect that this can be also an effect of using exclusive locks in stat1.p. I see that the LockTableUpdater uses old-style data structures and locking techniques which results in an excessive synchronization overhead. In particular, I suggest using LinkedBlockingQueue for the eventQueue. This will substantially reduce the need for synchronization and make the code more simple and maintainable.
What do you think about this?
Thank you.

Please see #3896. We need to make much more substantial changes to the implementation supporting _Lock metadata, though it is not the top priority at this time. Even if we were to replace the synchronization logic in LockTableUpdater, I believe the main pain comes from the constant H2 updates. I don't want to invest more effort into the current implementation, knowing it needs to be substantially redesigned.

That being said, please post any ideas you have for improvement to the _Lock implementation in that task, so we have all our thoughts on the subject in one place. Thank you.

Eric Faulhaber wrote:

Please see #3896. We need to make much more substantial changes to the implementation supporting _Lock metadata, though it is not the top priority at this time. Even if we were to replace the synchronization logic in LockTableUpdater, I believe the main pain comes from the constant H2 updates. I don't want to invest more effort into the current implementation, knowing it needs to be substantially redesigned.

That being said, please post any ideas you have for improvement to the _Lock implementation in that task, so we have all our thoughts on the subject in one place. Thank you.

I see. I have added my more radical suggestion to #3896.

However, my initial idea was about the problem with InterruptedException. Of course, I can just ignore this exception but since I do not understand what causes it, I'm not sure if this will not result in uncontrolled growth of the _UserTableStat table.

Igor Skornyakov wrote:

BTW: I already have a lot of uncommitted changes for the 3821c. Maybe it makes sense to create a separate task branch from 3821c for them? Just to avoid the risk of accidentally lost.
What do you think?

If you think (a) you are not close to finished with the changes; or (b) they are especially risky; then go ahead with a new branch based on 3821c.

Eric Faulhaber wrote:

If you think (a) you are not close to finished with the changes; or (b) they are especially risky; then go ahead with a new branch based on 3821c.

Thank you. Actually, I hope to finish tomorrow.

Good news.

Contrary to my initial assumptions based on an incorrect interpretation of the early test results, _UserTableStat counters do not care about transactions' rollback - once updated they retain their values.

See updated testcases/meta/stat1.p.

This greatly simplifies my job.

_UserTableStat support is finished. At least now testcases/meta/stat1.p produces the same results both in 4GL and FWD (apart from minor discrepancies described in #3814-277, #3814-332).

Committed to the task branch 3814a (based on 3821c) rev. 12300.

Please review.

Thank you.

A "smoke test" of the large customer app passed OK with 3814a and manually updated meta database DDLs (_MyConnect changes).

Is there a reason we need this in a separate branch?

meta database DLLs

Dynamic Link Libraries? What is DLL in this context?

The _area, _filelist and _tenant are not done. I was planning to do it after _usertablestat but got another assignment. In fact populating these tables should be very easy. This is why it was posponed "for a dessert"

Are these complete now?

Is there any work in this task that is not yet complete?

Greg Shah wrote:

Is there a reason we need this in a separate branch?

Eric agreed recently with the separate branch (#3814-350). I think that code review will take some time.

meta database DLLs

Dynamic Link Libraries? What is DLL in this context?

Sorry, it was a misprint. I meant DDL. Fixed now.

Greg Shah wrote:

The _area, _filelist and _tenant are not done. I was planning to do it after _usertablestat but got another assignment. In fact populating these tables should be very easy. This is why it was posponed "for a dessert"

Are these complete now?

No, but since it is just 'stubs' it should be easy.

Is there any work in this task that is not yet complete?

I understand that apart from _area, _filelist, and _tenant all is done.

Please finish _area, _filelist, and _tenant.

Greg Shah wrote:

Please finish _area, _filelist, and _tenant.

OK. Thank you.

I see no real usage of _Filelist and _Tenant in the real project I know about. There is a single use of the _Area in the customer code and a few more in the possenet code used by the same app.
If I understand correctly the customer code will run OK with just an empty _Area table.

Do we really need to create mocks for these VSTs now? Without real-life use cases, it is not likely that we can provide really useful mocks. When such use cases will be encountered, adding a mock will be trivial.
What do you think?
Thank you.

There are multiple applications in consideration here. All 3 tables are used. Our environment should report that it is single tenant. Whatever the 4GL reports for that should work.

Added population of the _Area, _Filelist, and _Tenant metatables. For the _Tenant only toplevel method and a reference to it were added because in all databases I've seen this table is empty.

Committed to 3814a/12301.

Eric: Can we close this task?

Greg Shah wrote:

Eric: Can we close this task?

Greg,
Please remember that the changes are not merged to 3821c so far.

Eric: Please review and get this into 3821c unless there is an issue.

Code review 3814a rev 12304-12307, part 1.

There are functional, format, and documentation issues I think we need to address in this update.

Functionally, I think there may be some problems with the placement of the calls into the UserTableStatUpdater counter methods. It seems I did not fully understand the context of what you were asking when we were discussing the query hierarchy earlier in this task, but now that I look at the code, I see what you were asking.

As I understand it, the _Usertablestat VST tracks occurrences of record creates, retrievals, updates, and deletes. Every such CRUD action flows through the RecordBuffer class. It seems like this probably is the right location to place these counter hooks, but we need to know the answer to some questions to be sure. If the hooks need to be closer to where the database actually is accessed, then maybe some classes deeper in the orm package (e.g., Loader, Persister) are more appropriate, but I'd like to avoid cluttering those low-level classes, if possible.

Create

Should the counter be incremented when the record first is created in the buffer, or when the new, fully validated record is flushed to the database from the buffer?

The statistics hook currently seems only to be implemented in BufferImpl.bufferCreate. This suggests the create statistic is incremented immediately when 4GL first creates a record in a buffer, regardless of whether the record is later validated and flushed to the database or not. Was this the intention, or should the statistic only be updated update successful validation and flush? Note that this location only manages the case of a record being created by the handle-based method BUFFER-CREATE, but not the case when the record is created with the CREATE language statement, or when a destination record implicitly is created by the BUFFER-COPY statement or method.

You determined that a transaction rollback does not roll back statistics counters, but what happens to the create counter if a create trigger fails and the record is not created?

Update

Does the VST count each individual field update, or the flush of some set of changes to the database? The current implementation suggests the latter. On a related note, is a BUFFER-COPY counted as the number of fields updated, or as a single update? Can an assign trigger interfere with the count? If the counter applies to individual field updates, is the counter incremented even if the value is updated, but the new value of the field is not actually different than the old?

Delete

The implementation only counts the delete statistic from BufferImpl.deleteRecord, which covers the case of the handle-based BUFFER-DELETE method, but not the DELETE language statement. RecordBuffer.delete probably is the better place for the hook.

Can a delete trigger have any impact on the statistic?

Retrieve

At what point is the retrieve statistic incremented? Is it one retrieval per record or one per record set (considering OPEN QUERY cases)? The current implementation it is one per record. It seems logical that the statistic would be updated when a record has been found and is set into a record buffer. If so, it seems RecordBuffer.setRecord (not setCurrentRecord) is a better location to place the statistic hook than across the various query implementations.

More to follow...

Eric Faulhaber wrote:

Code review 3814a rev 12304-12307, part 1.

There are functional, format, and documentation issues I think we need to address in this update.

Functionally, I think there may be some problems with the placement of the calls into the UserTableStatUpdater counter methods. It seems I did not fully understand the context of what you were asking when we were discussing the query hierarchy earlier in this task, but now that I look at the code, I see what you were asking.

As I understand it, the _Usertablestat VST tracks occurrences of record creates, retrievals, updates, and deletes. Every such CRUD action flows through the RecordBuffer class. It seems like this probably is the right location to place these counter hooks, but we need to know the answer to some questions to be sure. If the hooks need to be closer to where the database actually is accessed, then maybe some classes deeper in the orm package (e.g., Loader, Persister) are more appropriate, but I'd like to avoid cluttering those low-level classes, if possible.

Create

Should the counter be incremented when the record first is created in the buffer, or when the new, fully validated record is flushed to the database from the buffer?

The statistics hook currently seems only to be implemented in BufferImpl.bufferCreate. This suggests the create statistic is incremented immediately when 4GL first creates a record in a buffer, regardless of whether the record is later validated and flushed to the database or not. Was this the intention, or should the statistic only be updated update successful validation and flush? Note that this location only manages the case of a record being created by the handle-based method BUFFER-CREATE, but not the case when the record is created with the CREATE language statement, or when a destination record implicitly is created by the BUFFER-COPY statement or method.

You determined that a transaction rollback does not roll back statistics counters, but what happens to the create counter if a create trigger fails and the record is not created?

My implementation is based on the results of the testcases/meta/stat1.p run. Hopefully, my interpretation is correct. According to it, the create counter is increased right after BUFFER-CREATE, even if the update operation failed. I've not considered BUFFER-COPY. Will add it to my test. I know nothing about the create triggers but will add a corresponding test case.

Update

Does the VST count each individual field update, or the flush of some set of changes to the database? The current implementation suggests the latter. On a related note, is a BUFFER-COPY counted as the number of fields updated, or as a single update? Can an assign trigger interfere with the count? If the counter applies to individual field updates, is the counter incremented even if the value is updated, but the new value of the field is not actually different than the old?

According to the test results the update counter is increased once per record. Again, I've not considered triggers yet.

Delete

The implementation only counts the delete statistic from BufferImpl.deleteRecord, which covers the case of the handle-based BUFFER-DELETE method, but not the DELETE language statement. RecordBuffer.delete probably is the better place for the hook.

Can a delete trigger have any impact on the statistic?

I will check it.

Retrieve

At what point is the retrieve statistic incremented? Is it one retrieval per record or one per record set (considering OPEN QUERY cases)? The current implementation it is one per record. It seems logical that the statistic would be updated when a record has been found and is set into a record buffer. If so, it seems RecordBuffer.setRecord (not setCurrentRecord) is a better location to place the statistic hook than across the various query implementations.

This sounds logical. I will check it.

More to follow...

Thank you.

Igor Skornyakov wrote:

My implementation is based on the results of the testcases/meta/stat1.p run. Hopefully, my interpretation is correct. According to it, the create counter is increased right after BUFFER-CREATE, even if the update operation failed. I've not considered BUFFER-COPY. Will add it to my test.

Sorry if this is redundant, but I want to reiterate the following point, because you did not address it directly in your reply and I want to make sure it makes sense. I notice that in stat1.p you are using only handle-based methods to do persistence work. There is nothing wrong with this, but it only shows part of the picture. These methods are all implemented in BufferImpl, but that is only an entry point for converted, handle-based methods into RecordBuffer, where the real work is done. None of the counters should be in the BufferImpl access layer, as that will miss any persistence work that is not invoked via handle-based methods.

Eric Faulhaber wrote:

Igor Skornyakov wrote:

My implementation is based on the results of the testcases/meta/stat1.p run. Hopefully, my interpretation is correct. According to it, the create counter is increased right after BUFFER-CREATE, even if the update operation failed. I've not considered BUFFER-COPY. Will add it to my test.

Sorry if this is redundant, but I want to reiterate the following point, because you did not address it directly in your reply and I want to make sure it makes sense. I notice that in stat1.p you are using only handle-based methods to do persistence work. There is nothing wrong with this, but it only shows part of the picture. These methods are all implemented in BufferImpl, but that is only an entry point for converted, handle-based methods into RecordBuffer, where the real work is done. None of the counters should be in the BufferImpl access layer, as that will miss any persistence work that is not invoked via handle-based methods.

Thank you, Eric.
I understand this and working now on adding new use cases to my test, and looking into our persistence layer logic.

I've added a trigger to the test table:

ADD TABLE "customer" 
  AREA "Schema Area" 
  DUMP-NAME "customer" 
  TABLE-TRIGGER "CREATE" NO-OVERRIDE PROCEDURE "create-trg.p" CRC "30042" 

** "create-trg.p" was not found. (293)

It must be found in the propath.

Greg Shah wrote:

It must be found in the propath.

Greg.
In my environment the PROPATH is defined in p2j.cfg.xml like this:

      <parameter name="propath"          value="${P2J_HOME}:${P2J_HOME}/abl:${P2J_HOME}/appsrv/api:." />

I've tried to put the trigger function both to $P2_HOME and . but the result is still the same.
Or you mean some another place?
Thank you.

What is the PROPATH in directory.xml? That one is used at runtime.

Is create-trg.p in some folder? If so, add that to PROPATH.

Constantin Asofiei wrote:

What is the PROPATH in directory.xml? That one is used at runtime.

Is create-trg.p in some folder? If so, add that to PROPATH.

It is both in the current folder of the conversion (.) and in $P2j_HOME.

Those are both conversion-time values and they have no meaning at runtime. What project are you using?

Greg Shah wrote:

Those are both conversion-time values and they have no meaning at runtime. What project are you using?

I use sftp://xfer.goldencode.com/opt/testcases/

OK, so your code should not just be dropped in the root directory of the project. It should be in some structured path below there. Perhaps we already have other triggers in the project?

Greg Shah wrote:

OK, so your code should not just be dropped in the root directory of the project. It should be in some structured path below there. Perhaps we already have other triggers in the project?

I've moved the trigger program to the meta subfolder and adjusted PROPATH in the p2j.cfg.xml accordingly. However, the problem persists.
This is the first time I'm trying to add a trigger to my test project. Sorry for my ignorance.

Add the path inside your trigger definition in the .df otherwise it cannot be found in the propath since the propath knows nothing about meta/ (and we don't want to add ./meta/ into the propath.

Please post here the create-trg.p program and the program which tests this trigger.

I assume you are not running create-trg.p from the FWD client, right?

Here is the result of the table conversion:

@Table(name = "customer", legacy = "customer")
@Indices(
{
   @Index(name = "customer_pk", legacy = "pk", primary = true, unique = true, components = 
   {
      @IndexComponent(name = "customernum", legacy = "customerNum")
   })
})
@Triggers(
{
   @Trigger(event = "CREATE", procedure = "meta/create-trg.p")
})

• added the TABLE-TRIGGER "CREATE" NO-OVERRIDE PROCEDURE "create-trg.p" CRC "30042" line to fwd.df customer table
• copied the stat1.p and create-trg.p programs in the root of the xfer testcases project, and added these two files to file-cvt-list.txt
• ran ant deploy.all
• ran the stat1.p program with the terminal client and I see the create allow/rejected messages.

I haven't done any other edits to the xfer testcases project (except a fix in client.xml which is in rev 1047, but this is not related to DB triggers).

Please try the same with 3821c. If is still not working, then is a setup problem for you - and you should try with a clean checkout.

Otherwise, if is working with 3821c and not with your code, then some of your changes affect trigger invocation.

Constantin Asofiei wrote:

I've tested with 3821c:

• added the TABLE-TRIGGER "CREATE" NO-OVERRIDE PROCEDURE "create-trg.p" CRC "30042" line to fwd.df customer table
• copied the stat1.p and create-trg.p programs in the root of the xfer testcases project, and added these two files to file-cvt-list.txt
• ran ant deploy.all
• ran the stat1.p program with the terminal client and I see the create allow/rejected messages.

I haven't done any other edits to the xfer testcases project (except a fix in client.xml which is in rev 1047, but this is not related to DB triggers).

Please try the same with 3821c. If is still not working, then is a setup problem for you - and you should try with a clean checkout.

Otherwise, if is working with 3821c and not with your code, then some of your changes affect trigger invocation.

I see. Thanks a lot! Trying.

Constantin Asofiei wrote:

I've tested with 3821c:

• added the TABLE-TRIGGER "CREATE" NO-OVERRIDE PROCEDURE "create-trg.p" CRC "30042" line to fwd.df customer table
• copied the stat1.p and create-trg.p programs in the root of the xfer testcases project, and added these two files to file-cvt-list.txt
• ran ant deploy.all
• ran the stat1.p program with the terminal client and I see the create allow/rejected messages.

I haven't done any other edits to the xfer testcases project (except a fix in client.xml which is in rev 1047, but this is not related to DB triggers).

Please try the same with 3821c. If is still not working, then is a setup problem for you - and you should try with a clean checkout.

Otherwise, if is working with 3821c and not with your code, then some of your changes affect trigger invocation.

With 3821c the trigger is working. Trying to understand how I managed to break the triggers in 3814a.
Thanks a lot for the help!

After rebasing 3814a to 3821c/12334 the trigger is working but contrary to 4GL behavior the converted RETURN ERROR in the trigger (returnError() call) does not prevent the record creation (at least hBuffer.unwrapBuffer().bufferCreate() returns true)

Does the 4GL return false/no from this BUFFER-CREATE method call? If so, then that looks like a bug in our create trigger implementation.

Igor Skornyakov wrote:

After rebasing 3814a to 3821c/12334 the trigger is working but contrary to 4GL behavior the converted RETURN ERROR in the trigger (returnError() call) does not prevent the record creation (at least hBuffer.unwrapBuffer().bufferCreate() returns true)

In addition FIND trigger works, but not in the same way as in 4GL. It seems that in FWD it is called more often.
In 4GL for FIND FIRST; GET CURRENT it is called once while in FWD I see two invocations.

Eric Faulhaber wrote:

Does the 4GL return false/no from this BUFFER-CREATE method call? If so, then that looks like a bug in our create trigger implementation.

Yes, 4GL returns no if the create trigger rejects the operation.

In some situations, the FIND rejected by the trigger causes NPE in the PreselectQuery line 5630 (results field is null)

Igor Skornyakov wrote:

In some situations, the FIND rejected by the trigger causes NPE in the PreselectQuery line 5630 (results field is null)

As far as I can see, this NPE happens on REPOSITION-FORWARD in the presence of the FIND trigger, even if the trigger accepts all operations.

Code review 3814a rev 12304-12307, part 2.

Functional:

I understand your term for persisting the state from the in-memory counters to the metadata database is "flash", so I will use this term. Please help me understand why it is necessary to flash the metadata from both the various query classes and from FqlToSqlConverter. This seems redundant, but perhaps I missed something. If it is necessary to flash the data from the query classes, please add a common method to the base class AbstractQuery and call that where necessary.

Documentation:

A general comment: I would like a deeper level of source-code level documentation (javadoc as well as inline comments), particularly for newly introduced classes, tricky concepts, or when we expand an existing implementation or API in some significant way. These areas need sufficient documentation, so that someone coming along and reading the code later does not have to spend a lot of time figuring out what it is meant to do.

Take, for instance, UserTableStatUpdater. This is not only an unusual class conceptually to begin with, but it is one for which we have done a lot of design discussion outside the implementation (i.e., here in Redmine). One should not have to hunt back through Redmine issues to understand the purpose and design of a class. In its current form, UserTableStatUpdater needs a lot more documentation to meet this basic standard. Consider also that we intended this implementation to be the model for the "flash" technique, to be adapted for other VST implementations (I recall having this discussion, though I cannot find it now). As such, it needs detailed documentation about the design, including how it interacts with other classes.

Please review the other code in this update and enhance the documentation to provide this level of understanding, if the documentation currently is sparse.

Functions.fwdSession: This UDF can only work correctly from an embedded H2 database running within the FWD server process. It will not work correctly from a standalone database server, like PostgreSQL. I think it would return null in such a case. While it is not being installed via PL/Java, this restriction should be well documented in the javadoc for this UDF. Otherwise, someone would have to notice that it has not been added to pl/p2jpl.ddr to understand it is neither available nor appropriate in PostgreSQL.

Format/Coding Standards:

Please try to adhere to the coding standards, especially around format. As an open source project where we are judged in part by the readability of our code and the quality of source-level documentation, it is important for us to be consistent in this regard throughout the project. I just spent 10 minutes cleaning up the code format in ConnectionManager in 3821c and committed these changes to rev 12333 in that branch. Please diff this class with its previous revision to see what I mean. While it is not necessary to add indents for blank lines or remove every trailing white space, the rest of the cleanup I did in that revision is an example of what we would like to maintain in terms of format. Thanks.

It seems we have enough trigger-related items to warrant a separate issue. Please open a normal priority bug issue in the Database project and add these items there. Make me a watcher and Ovidiu the assignee. Thanks.

Eric Faulhaber wrote:

Code review 3814a rev 12304-12307, part 2.

Functional:

I understand your term for persisting the state from the in-memory counters to the metadata database is "flash", so I will use this term. Please help me understand why it is necessary to flash the metadata from both the various query classes and from FqlToSqlConverter. This seems redundant, but perhaps I missed something. If it is necessary to flash the data from the query classes, please add a common method to the base class AbstractQuery and call that where necessary.

Documentation:

A general comment: I would like a deeper level of source-code level documentation (javadoc as well as inline comments), particularly for newly introduced classes, tricky concepts, or when we expand an existing implementation or API in some significant way. These areas need sufficient documentation, so that someone coming along and reading the code later does not have to spend a lot of time figuring out what it is meant to do.

Take, for instance, UserTableStatUpdater. This is not only an unusual class conceptually to begin with, but it is one for which we have done a lot of design discussion outside the implementation (i.e., here in Redmine). One should not have to hunt back through Redmine issues to understand the purpose and design of a class. In its current form, UserTableStatUpdater needs a lot more documentation to meet this basic standard. Consider also that we intended this implementation to be the model for the "flash" technique, to be adapted for other VST implementations (I recall having this discussion, though I cannot find it now). As such, it needs detailed documentation about the design, including how it interacts with other classes.

Please review the other code in this update and enhance the documentation to provide this level of understanding, if the documentation currently is sparse.

Functions.fwdSession: This UDF can only work correctly from an embedded H2 database running within the FWD server process. It will not work correctly from a standalone database server, like PostgreSQL. I think it would return null in such a case. While it is not being installed via PL/Java, this restriction should be well documented in the javadoc for this UDF. Otherwise, someone would have to notice that it has not been added to pl/p2jpl.ddr to understand it is neither available nor appropriate in PostgreSQL.

Format/Coding Standards:

Please try to adhere to the coding standards, especially around format. As an open source project where we are judged in part by the readability of our code and the quality of source-level documentation, it is important for us to be consistent in this regard throughout the project. I just spent 10 minutes cleaning up the code format in ConnectionManager in 3821c and committed these changes to rev 12333 in that branch. Please diff this class with its previous revision to see what I mean. While it is not necessary to add indents for blank lines or remove every trailing white space, the rest of the cleanup I did in that revision is an example of what we would like to maintain in terms of format. Thanks.

OK. Thank you. Will be done tomorrow.

It looks that if a trigger rejects CREATE/DELETE/FIND/WRITE operation the corresponding _UserTableStat counter is not updated (see triggers' tests in the sftp://ias@xfer.goldencode.com/opt/testcases/meta).
As far as I understand the current implementation of the counters' updates in FWD should compatible with this, but I cannot check it because of the known issues with triggers support in FWD (#5829).

I've noticed what looks like incompatibility.

In 4GL if inside a transaction block two records are inserted where the second insert violates the unique constraint, the first record remains inserted while in FWD it doesn't.

As I've mentioned before, in this case, FWD reports ** Unable to update <table> Field. (142) error, while 4GL does not, only error ** <table> already exists with <field> <value>. (132) is reported.

Eric Faulhaber wrote:

Code review 3814a rev 12304-12307, part 1.

Update

Does the VST count each individual field update, or the flush of some set of changes to the database? The current implementation suggests the latter. On a related note, is a BUFFER-COPY counted as the number of fields updated, or as a single update? Can an assign trigger interfere with the count? If the counter applies to individual field updates, is the counter incremented even if the value is updated, but the new value of the field is not actually different than the old?

If the record data was not actually changed the counter is not updated. Fixed the code accordingly.

Delete

The implementation only counts the delete statistic from BufferImpl.deleteRecord, which covers the case of the handle-based BUFFER-DELETE method, but not the DELETE language statement. RecordBuffer.delete probably is the better place for the hook.

Updating the counter in the BufferImpl.deleteRecord works for the DELETE statement as well.

Can a delete trigger have any impact on the statistic?

See #3814-397.

Retrieve

At what point is the retrieve statistic incremented? Is it one retrieval per record or one per record set (considering OPEN QUERY cases)? The current implementation it is one per record. It seems logical that the statistic would be updated when a record has been found and is set into a record buffer. If so, it seems RecordBuffer.setRecord (not setCurrentRecord) is a better location to place the statistic hook than across the various query implementations.

Indeed, RecordBuffer.setRecord looks to be a better place. However I've encountered a problem that this method is called twice in many cases. For example for GET-FIRST it is first called at:

Daemon Thread [Conversation [0000000F:bogus]] (Suspended (breakpoint at line 191 in UserTableStatUpdater))    
    UserTableStatUpdater.retrieve(Database, RecordBuffer) line: 191    
    RecordBuffer.setRecord(Record, LockType, boolean, SortIndex, OffEnd, boolean, boolean, boolean) line: 9325    
    RandomAccessQuery.updateBuffer(Record, LockType, boolean, OffEnd) line: 3252    
    RandomAccessQuery.next(Object[], LockType) line: 1849    
    RandomAccessQuery.next() line: 1717    
    AdaptiveQuery$DynamicResults.next() line: 4239    
    ResultsAdapter.next() line: 161    
    AdaptiveQuery.next(LockType) line: 2136    
    AdaptiveQuery.first(LockType) line: 1680    
    AdaptiveQuery(PreselectQuery).first() line: 2395    
    AdaptiveQuery(AbstractQuery).getFirst() line: 2362    
    QueryWrapper.lambda$getFirst$4() line: 4889    
    167937990.get() line: not available    
    QueryWrapper.handleQueryOffEnd(Supplier<logical>) line: 6577    
    QueryWrapper.getFirst() line: 4889    
    Stat1.lambda$execute$14() line: 231    

and then at

Daemon Thread [Conversation [0000000F:bogus]] (Suspended (breakpoint at line 191 in UserTableStatUpdater))    
    UserTableStatUpdater.retrieve(Database, RecordBuffer) line: 191    
    RecordBuffer.setRecord(Record, LockType, boolean, SortIndex, OffEnd, boolean, boolean, boolean) line: 9325    
    AdaptiveQuery(PreselectQuery).coreFetch(Object[], LockType, boolean, boolean) line: 5866    
    AdaptiveQuery(PreselectQuery).fetch(boolean, LockType, boolean) line: 5631    
    AdaptiveQuery.fetch(boolean, LockType, boolean) line: 3762    
    AdaptiveQuery.next(LockType) line: 2158    
    AdaptiveQuery.first(LockType) line: 1680    
    AdaptiveQuery(PreselectQuery).first() line: 2395    
    AdaptiveQuery(AbstractQuery).getFirst() line: 2362    
    QueryWrapper.lambda$getFirst$4() line: 4889    
    167937990.get() line: not available    
    QueryWrapper.handleQueryOffEnd(Supplier<logical>) line: 6577    
    QueryWrapper.getFirst() line: 4889    
    Stat1.lambda$execute$14() line: 231    

What is the best way to deal with this?
Thank you.

GET-CURRENT has an interesting effect on the _UserTableStat_read counter.

• The combination GET-CURRENT/DELETE does not change the value.
• The combination GET-CURRENT/update increases the value by 1.
• If in the GET-CURRENT/update the update was rejected by the WRITE trigger, the value is increased by 2.

Igor Skornyakov wrote:

Indeed, RecordBuffer.setRecord looks to be a better place. However I've encountered a problem that this method is called twice in many cases. For example for GET-FIRST it is first called at:
[...]

and then at
[...]

What is the best way to deal with this?

Sorry I forgot to respond to this yesterday. Are you sure these code paths are not mutually exclusive for a given record retrieval? I believe the first stack is only active when an AdaptiveQuery is in dynamic mode, while the second is only active when it is in preselect mode. AdaptiveQuery starts in preselect mode and only switches to dynamic mode if we detect the index on which the preselected result set is based has been modified before we are finished iterating results, such that we have to discard the result set as potentially invalid.

Eric Faulhaber wrote:

Sorry I forgot to respond to this yesterday. Are you sure these code paths are not mutually exclusive for a given record retrieval? I believe the first stack is only active when an AdaptiveQuery is in dynamic mode, while the second is only active when it is in preselect mode. AdaptiveQuery starts in preselect mode and only switches to dynamic mode if we detect the index on which the preselected result set is based has been modified before we are finished iterating results, such that we have to discard the result set as potentially invalid.

Yes, I'm sure that these are not mutually exclusive paths. This can also be seen from the stack traces. Bothe where taken during the processing of the same converted 4GL statement.

Is the same record being set into the buffer in both calls to RecordBuffer.setRecord?

Eric Faulhaber wrote:

Is the same record being set into the buffer in both calls to RecordBuffer.setRecord?

In my test, it was the same record if I've not overlooked something. However, I'm not sure that it is always the case.

Please post the original 4GL code and the converted code (not the whole program, just the snippets relevant to this query).

Your test case seems to be putting this query into dynamic mode. This is not a problem per se, but I did not expect the record to be set into the buffer twice in dynamic mode. The DynamicResults object does it once via RandomAccessQuery.next (called from DynamicResults.next in AdaptiveQuery.next(LockType)). The second time, when we drop down to the fetch call in AdaptiveQuery.next(LockType), is redundant.

Ultimately, we want to avoid setting the record into the buffer twice. It seems there is an optimization to be had here, as there is a lot of work being done in RecordBuffer.setRecord. But removing this redundancy will require a good deal of analysis beyond the scope of this task.

In the meantime, I am not sure yet what the best approach is from the perspective of the statistics counter. We might avoid updating the counter in RecordBuffer.setRecord, if the incoming record is the same as the buffer's current record, but I suspect that might be naive and may break another valid use case. Can you think of another retrieval case which this approach would break? Any other ideas are welcome...

Eric Faulhaber wrote:

Please post the original 4GL code and the converted code (not the whole program, just the snippets relevant to this query).

The test is pretty simple:

DEF VAR hBuffer AS HANDLE NO-UNDO.
DEF VAR hQuery AS HANDLE NO-UNDO.
CREATE BUFFER hBuffer FOR TABLE 'customer'.
CREATE QUERY hQuery.
hQuery:SET-BUFFERS(hBuffer).
hQuery:QUERY-PREPARE('FOR EACH customer NO-LOCK').
hQuery:QUERY-OPEN().

hQuery:GET-FIRST().

The converted code:

         hQuery.unwrapQuery().getFirst();

In the meantime, I am not sure yet what the best approach is from the perspective of the statistics counter. We might avoid updating the counter in RecordBuffer.setRecord, if the incoming record is the same as the buffer's current record, but I suspect that might be naive and may break another valid use case. Can you think of another retrieval case which this approach would break? Any other ideas are welcome...

I was thinking about the approach based on record reference but also decided that it is "too straightforward". Thinking about something more elaborate, but have no solution so far.

This observation is not directly to the problem at hand, but it is curious that there isn't anything I see about your test case that should cause the query to be in dynamic mode. AdaptiveQuery should begin in preselect mode and only switch to dynamic mode if there is a change on the sorting index. We must either be starting in dynamic mode, or there is something about the GET implementation (or something even earlier) that switches us over.

Eric Faulhaber wrote:

This observation is not directly to the problem at hand, but it is curious that there isn't anything I see about your test case that should cause the query to be in dynamic mode. AdaptiveQuery should begin in preselect mode and only switch to dynamic mode if there is a change on the sorting index. We must either be starting in dynamic mode, or there is something about the GET implementation (or something even earlier) that switches us over.

Do you mean that this should be fixed and there is no need to think about a workaround just for _UserTableStat counters?
Thank you.

Igor Skornyakov wrote:

Eric Faulhaber wrote:

This observation is not directly to the problem at hand, but it is curious that there isn't anything I see about your test case that should cause the query to be in dynamic mode. AdaptiveQuery should begin in preselect mode and only switch to dynamic mode if there is a change on the sorting index. We must either be starting in dynamic mode, or there is something about the GET implementation (or something even earlier) that switches us over.

Do you mean that this should be fixed and there is no need to think about a workaround just for _UserTableStat counters?
Thank you.

Well, there are two problems.

The one quoted immediately above is not the one which affects the counters, other than incidentally. Dynamic mode must be supported, whether or not this particular case gets into dynamic mode when it probably shouldn't.

The root cause of the double-counting of the retrieve statistic is that we are setting the record into the buffer twice when we are in dynamic query mode (whether we are in that mode legitimately or not). Please put a TODO in the code about this double-counting issue and don't worry about a workaround just now. I want to think about how risky a fix for the root cause is, before we spend any time on a workaround. I'll try to get to this in the next day or so.

In the meantime, please continue addressing the other aspects of the code reviews instead. Thanks.

Eric Faulhaber wrote:

The root cause of the double-counting of the retrieve statistic is that we are setting the record into the buffer twice when we are in dynamic query mode (whether we are in that mode legitimately or not). Please put a TODO in the code about this double-counting issue and don't worry about a workaround just now. I want to think about how risky a fix for the root cause is, before we spend any time on a workaround. I'll try to get to this in the next day or so.

In the meantime, please continue addressing the other aspects of the code reviews instead. Thanks.

I see. Thank you.

I've noticed one more discrepancy between 4GL and FWD.

If a record is updated twice w/o commit like this:

DO TRANSACTION:   
  RUN update('xxx').    
END.    

DO TRANSACTION:   
  RUN update('xxx').    
END.    

Where

PROCEDURE update:
   DEF INPUT PARAM val AS CHAR.
   hQuery:GET-CURRENT(EXCLUSIVE-LOCK).        
   hBuffer:BUFFER-FIELD('customerName'):BUFFER-VALUE=val.       
   hBuffer:BUFFER-RELEASE().
END.

then 4GL complain is

Warning: A buffer in query  contains a different record than the current query result list entry. (4108)

while with FWD we get:

** Unable to update customer Field. (142)

I think that the 4GL message is more informative.

Please note that both in 4GL and FWD the warning is emitted even if the record was not actually changed.

Code review 3814a rev 12304-12307, part 2.

Functional:

I understand your term for persisting the state from the in-memory counters to the metadata database is "flash", so I will use this term. Please help me understand why it is necessary to flash the metadata from both the various query classes and from FqlToSqlConverter. This seems redundant, but perhaps I missed something. If it is necessary to flash the data from the query classes, please add a common method to the base class AbstractQuery and call that where necessary.

I was unable to find a right place in the AbstractQuery for flush. It is possible to use a common method defined in the AbstractQuery which is called in AdaptiveQyery.initialize() and RandomAccessQuery.initialize() but I do not think that it is possible to avoid flush in FqlToSqlConverter because this is the only place where we can figure tha the query accessses _UserTableStat.

Documentation:

A general comment: I would like a deeper level of source-code level documentation (javadoc as well as inline comments), particularly for newly introduced classes, tricky concepts, or when we expand an existing implementation or API in some significant way. These areas need sufficient documentation, so that someone coming along and reading the code later does not have to spend a lot of time figuring out what it is meant to do.

Take, for instance, UserTableStatUpdater. This is not only an unusual class conceptually to begin with, but it is one for which we have done a lot of design discussion outside the implementation (i.e., here in Redmine). One should not have to hunt back through Redmine issues to understand the purpose and design of a class. In its current form, UserTableStatUpdater needs a lot more documentation to meet this basic standard. Consider also that we intended this implementation to be the model for the "flash" technique, to be adapted for other VST implementations (I recall having this discussion, though I cannot find it now). As such, it needs detailed documentation about the design, including how it interacts with other classes.

Please review the other code in this update and enhance the documentation to provide this level of understanding, if the documentation currently is sparse.

Done.

Functions.fwdSession: This UDF can only work correctly from an embedded H2 database running within the FWD server process. It will not work correctly from a standalone database server, like PostgreSQL. I think it would return null in such a case. While it is not being installed via PL/Java, this restriction should be well documented in the javadoc for this UDF. Otherwise, someone would have to notice that it has not been added to pl/p2jpl.ddr to understand it is neither available nor appropriate in PostgreSQL.

Done.

See also #3814-399.

Committed to 3814a/12341.

Code review 3821c/12341:

Igor Skornyakov wrote:

Updating the counter in the BufferImpl.deleteRecord works for the DELETE statement as well.

RecordBuffer.delete is called from a few other locations as well, which do not go through BufferImpl.delete.

I was unable to find a right place in the AbstractQuery for flush. It is possible to use a common method defined in the AbstractQuery which is called in AdaptiveQyery.initialize() and RandomAccessQuery.initialize() but I do not think that it is possible to avoid flush in FqlToSqlConverter because this is the only place where we can figure tha the query accessses _UserTableStat.

I was not insisting it is necessary to flush the statistics from AbstractQuery (or from any query). The suggestion was to centralize the logic in AbstractQuery only IF it is necessary to flush from the queries. In fact, I prefer we only flush from FqlToSqlConverter, as you have done in rev 12341.

Please correct the file header entry in AdaptiveQuery.

One consideration I did not think of earlier: are statistics collected for temp-tables? I don't see how one could construct a query for that metadata, so I'm guessing not. If that is the case, we should bypass all work related to these statistics for temp-tables.

One last question (but don't let this hold up the rest of the work): what is your assessment of the amount of performance overhead introduced by the _Usertablestat feature overall? I am always concerned about adding work into constantly used code paths in support of features which will be infrequently used by some applications and not used at all by most others. I know the design is biased toward minimizing the statistics collection work, but do we need to consider disabling it altogether if we know the feature is not needed for a given application?

Eric Faulhaber wrote:

Updating the counter in the BufferImpl.deleteRecord works for the DELETE statement as well.

RecordBuffer.delete is called from a few other locations as well, which do not go through BufferImpl.delete.

OK, I will double-check/ Thank you.

I was unable to find a right place in the AbstractQuery for flush. It is possible to use a common method defined in the AbstractQuery which is called in AdaptiveQyery.initialize() and RandomAccessQuery.initialize() but I do not think that it is possible to avoid flush in FqlToSqlConverter because this is the only place where we can figure tha the query accessses _UserTableStat.

I was not insisting it is necessary to flush the statistics from AbstractQuery (or from any query). The suggestion was to centralize the logic in AbstractQuery only IF it is necessary to flush from the queries. In fact, I prefer we only flush from FqlToSqlConverter, as you have done in rev 12341.

Unfortunatelly, FqlToSQLConverter is not used in the AdaptiveQuery and RandomAccessQuery. This is why I call UserTableStatUpdater.persist() there as well.

Please correct the file header entry in AdaptiveQuery.

One consideration I did not think of earlier: are statistics collected for temp-tables? I don't see how one could construct a query for that metadata, so I'm guessing not. If that is the case, we should bypass all work related to these statistics for temp-tables.

According to 4GL docs _UserTableStats contains statistics for _temp tables as well, but I understand that support for this was postponed so I ignore temp-tables for now.

One last question (but don't let this hold up the rest of the work): what is your assessment of the amount of performance overhead introduced by the _Usertablestat feature overall? I am always concerned about adding work into constantly used code paths in support of features which will be infrequently used by some applications and not used at all by most others. I know the design is biased toward minimizing the statistics collection work, but do we need to consider disabling it altogether if we know the feature is not needed for a given application?

I understand your concern and tried to do my best to reduce the statistic gathering overhead to a minimum. I plan to profile a large customer app to figure the real impact on the performance.

Do you mean that we need a configuration parameter (in directory.xml?) to disable statistics?

Thank you.

Igor Skornyakov wrote:

Unfortunatelly, FqlToSQLConverter is not used in the AdaptiveQuery and RandomAccessQuery. This is why I call UserTableStatUpdater.persist() there as well.

That doesn't sound right. All queries must ultimately have their FQL statements translated to SQL.

Perhaps this is an issue of caching? If so, then we must ensure the flush happens even in the case where the SQL for a particular FQL statement is retrieved from the cache. Perhaps we save the information about whether a query is related to these statistics when we first generate the SQL, and perform the flush outside of the SQL generation, based on that saved state, so that it occurs whether or not the generated SQL was retrieved from the cache.

According to 4GL docs _UserTableStats contains statistics for _temp tables as well, but I understand that support for this was postponed so I ignore temp-tables for now.

I went back through the history and saw where we briefly discussed this 9 months ago (notes 243-245). I had completely forgotten about this.

What do you think the level of effort is to implement this? It seems like all the collection points and flushing points are the same as for regular tables, so what still would need to be done to support temp-tables?

Do you mean that we need a configuration parameter (in directory.xml?) to disable statistics?

Eventually, if this proves to be a measurable bottleneck, but not right now. Let's get the primary issues taken care of, close out this task, and then we can deal with any follow-up issues (like performance) as needed, separately.

Eric Faulhaber wrote:

Unfortunatelly, FqlToSQLConverter is not used in the AdaptiveQuery and RandomAccessQuery. This is why I call UserTableStatUpdater.persist() there as well.

That doesn't sound right. All queries must ultimately have their FQL statements translated to SQL.

Perhaps this is an issue of caching? If so, then we must ensure the flush happens even in the case where the SQL for a particular FQL statement is retrieved from the cache. Perhaps we save the information about whether a query is related to these statistics when we first generate the SQL, and perform the flush outside of the SQL generation, based on that saved state, so that it occurs whether or not the generated SQL was retrieved from the cache.

OK, I will double-check. Thank you, it seems that I've overlooked something.

According to 4GL docs _UserTableStats contains statistics for _temp tables as well, but I understand that support for this was postponed so I ignore temp-tables for now.

I went back through the history and saw where we briefly discussed this 9 months ago (notes 243-245). I had completely forgotten about this.

What do you think the level of effort is to implement this? It seems like all the collection points and flushing points are the same as for regular tables, so what still would need to be done to support temp-tables?

I do not think that it is a big deal. But I need to analyze how 4GL supports counters for the temp-tables first.

Do you mean that we need a configuration parameter (in directory.xml?) to disable statistics?

Eventually, if this proves to be a measurable bottleneck, but not right now. Let's get the primary issues taken care of, close out this task, and then we can deal with any follow-up issues (like performance) as needed, separately.

I see. Thank you.

I do not think that it is a big deal. But I need to analyze how 4GL supports counters for the temp-tables first.

If it is going to take more than 1 hour to do this analysis, then postpone it. We simply have no time for continuing this task.

Greg Shah wrote:

I do not think that it is a big deal. But I need to analyze how 4GL supports counters for the temp-tables first.

If it is going to take more than 1 hour to do this analysis, then postpone it. We simply have no time for continuing this task.

Sorry, but I do not think that I can decipher the 4GL logic in less than an hour. In addition, if I remember correctly, counters for temp-tables should be explicitly enabled at the app. startup.

AFAICT, we make no distinction between persistent and temp-tables in the current implementation. So, I am just wondering how temp-tables work from the query side. What is an example of a 4GL query against _Usertablestat for a persistent table vs. for a temp-table?

Eric Faulhaber wrote:

AFAICT, we make no distinction between persistent and temp-tables in the current implementation. So, I am just wondering how temp-tables work from the query side. What is an example of a 4GL query against _Usertablestat for a persistent table vs. for a temp-table?

Well, I planned to ignore the statistics updates for temp-tables but it looks that I forgot to add a corresponding check. This is not correct and should be fixed. Thanks for noticing.

Igor Skornyakov wrote:

Well, I planned to ignore the statistics updates for temp-tables but it looks that I forgot to add a corresponding check. This is not correct and should be fixed. Thanks for noticing.

My point is: isn't this already working for temp-tables, without further implementation? Is there something that needs to be different about the query side for temp-tables, or will this just work "as is", too?

Eric Faulhaber wrote:

My point is: isn't this already working for temp-tables, without further implementation? Is there something that needs to be different about the query side for temp-tables, or will this just work "as is", too?

It can be, but I'm not sure. As I mentioned in #3814-418 4GL at least limits the number of temp-tables which should be tracked by a startup argument but I do not know how it works exactly.

Igor Skornyakov wrote:

Eric Faulhaber wrote:

My point is: isn't this already working for temp-tables, without further implementation? Is there something that needs to be different about the query side for temp-tables, or will this just work "as is", too?

It can be, but I'm not sure. As I mentioned in #3814-418 4GL at least limits the number of temp-tables which should be tracked by a startup argument but I do not know how it works exactly.

They may do that for performance considerations, and we may need to do something similar if this is revealed as a bottleneck.

For now, let's get the flushing part of this settled and we'll consider this task complete.

Eric Faulhaber wrote:

They may do that for performance considerations, and we may need to do something similar if this is revealed as a bottleneck.

Agree.

For now, let's get the flushing part of this settled and we'll consider this task complete.

I do not see the problems with the flushing part. What is not done yet are counters' updates on COPY-BUFFER, discrepancies with the READ counter (related to #5306 and #3814-400), and the effect of triggers (which can not be done until #5289 is not resolved).

Igor Skornyakov wrote:

For now, let's get the flushing part of this settled and we'll consider this task complete.

I do not see the problems with the flushing part.

I was referring to this:

Unfortunatelly, FqlToSQLConverter is not used in the AdaptiveQuery and RandomAccessQuery. This is why I call UserTableStatUpdater.persist() there as well.

That doesn't sound right. All queries must ultimately have their FQL statements translated to SQL.

Perhaps this is an issue of caching? If so, then we must ensure the flush happens even in the case where the SQL for a particular FQL statement is retrieved from the cache. Perhaps we save the information about whether a query is related to these statistics when we first generate the SQL, and perform the flush outside of the SQL generation, based on that saved state, so that it occurs whether or not the generated SQL was retrieved from the cache.

OK, I will double-check. Thank you, it seems that I've overlooked something.

I don't think the current implementation is correct in this regard.

You are flushing the stats from FqlToSqlConverter.collectTableAliases. The problem is that this won't necessarily be called every time we execute a query, because Query instances are cached and reused. They only generate SQL statements the first time they are used, with a call to Query.createSqlQuery. The first time this method is invoked, it will use FqlToSqlConverter to generate a SQL statement string, which is then used to create a SQLQuery instance, which is then stored inside the Query object, which may itself be stored within the query cache.

Thus, if we get a Query instance from the cache, it won't need to generate the SQL. Accordingly, FqlToSqlConverter.collectTableAliases won't be invoked and we will miss the flush, if this is a _Usertablestat query. I think this happened to work ok with your test case because either it doesn't exercise the cache this way, OR it does, and somehow this caching behavior caused confusion about AdaptiveQuery and RandomAccessQuery not using FqlToSqlConverter.

I believe a good solution is to:

• remove all flushing logic from the various P2JQuery concrete implementations;
• add a boolean instance variable to FqlToSqlConverter (e.g., isUsertablestatQuery) and a getter method for it;
• in FqlToSqlConverter.collectTableAliases, set that variable if dmoInfo.isVST(SystemTable._UserTableStat);
• in Query.createSqlQuery, after the if (sqlQuery == null) block, call the getter to check the flag and flush the stats if it is true.

Let me know if you see a problem with this approach.

What is not done yet are counters' updates on COPY-BUFFER,

A TODO in the RecordBuffer.copy code will have to do for now.

discrepancies with the READ counter (related to #5306 and #3814-400),

This should work itself out when we address #5306. There should be nothing else left to do for this. We are not addressing this with a workaround.

and the effect of triggers (which can not be done until #5289 is not resolved).

Yes, this has to be deferred.

Eric Faulhaber wrote:

I believe a good solution is to:

• remove all flushing logic from the various P2JQuery concrete implementations;
• add a boolean instance variable to FqlToSqlConverter (e.g., isUsertablestatQuery) and a getter method for it;
• in FqlToSqlConverter.collectTableAliases, set that variable if dmoInfo.isVST(SystemTable._UserTableStat);
• in Query.createSqlQuery, after the if (sqlQuery == null) block, call the getter to check the flag and flush the stats if it is true.

Yes, this works. Fixed my code. Thanks for the advice.

A TODO in the RecordBuffer.copy code will have to do for now.

Done.

Committed to 3814a/12394.

The final version of the test (meta/*.p committed to xfer.goldencode.com/opt/testcases/ rev. 1059.

Eric.
I cannot find a better place for updating the 'delete' counter (see #3814-413). What use case where the current implementation will behave incorrectly you have in mind?
Thank you.

Apart from this I understand that the work on _UserTableStat support should be suspended until the known issues will be fixed.

Igor Skornyakov wrote:

I cannot find a better place for updating the 'delete' counter (see #3814-413).

The delete is invoked from RecordBuffer.delete(Supplier<logical> valexp, Supplier<character> valmsg). Why not here, close to the persistence.delete(currentRecord) call?

I don't want it in the Persistence.delete method itself, because that gets invoked for non-legacy purposes as well.

What use case where the current implementation will behave incorrectly you have in mind?

See the call hierarchy for RecordBuffer.delete(Supplier<logical> valexp, Supplier<character> valmsg). There are call paths (e.g., some looping bulk delete implementations) which do not go through BufferImpl.

BTW, I am concerned that non-looping bulk delete (e.g., those for temp-tables) will never be right with the table statistics, but we'll have to worry about those if/when we extend this support to temp-tables.

Eric Faulhaber wrote:

Igor Skornyakov wrote:

I cannot find a better place for updating the 'delete' counter (see #3814-413).

The delete is invoked from RecordBuffer.delete(Supplier<logical> valexp, Supplier<character> valmsg). Why not here, close to the persistence.delete(currentRecord) call?

I don't want it in the Persistence.delete method itself, because that gets invoked for non-legacy purposes as well.

What use case where the current implementation will behave incorrectly you have in mind?

See the call hierarchy for RecordBuffer.delete(Supplier<logical> valexp, Supplier<character> valmsg). There are call paths (e.g., some looping bulk delete implementations) which do not go through BufferImpl.

Thank you. Looking.

BTW, I am concerned that non-looping bulk delete (e.g., those for temp-tables) will never be right with the table statistics, but we'll have to worry about those if/when we extend this support to temp-tables.

Agree.

Eric Faulhaber wrote:

See the call hierarchy for RecordBuffer.delete(Supplier<logical> valexp, Supplier<character> valmsg). There are call paths (e.g., some looping bulk delete implementations) which do not go through BufferImpl.

Moved the _UserTableStat-delete counter update to the RecordBuffer.delete().

Committed to 3214a/12395.

Code review 3814a/12395:

I don't understand why you renamed the original RecordBuffer method delete to _delete, just to add one line of code, but then you don't actually invoke _delete in any other place. AFAICT, there is no need for the extra method. Why not just add the single line of code to the existing delete method, if _delete is not going be called from anywhere else?

In Query, I don't think we need the isUserTableStatRead() getter method; it is not called by anything. The userTableStatRead flag is only needed by Query.createSqlQuery(), so the instance variable on its own seems sufficient.

Other than that, the changes look good to me. Please address these points (or help me understand what I may have missed), then rebase and merge 3814a back to 3821c. If these are the only changes you make, I don't need to review again before you merge. Thank you.

Eric Faulhaber wrote:

Code review 3814a/12395:

I don't understand why you renamed the original RecordBuffer method delete to _delete, just to add one line of code, but then you don't actually invoke _delete in any other place. AFAICT, there is no need for the extra method. Why not just add the single line of code to the existing delete method, if _delete is not going be called from anywhere else?

Indeed, it was a stupid decision. Fixed.

In Query, I don't think we need the isUserTableStatRead() getter method; it is not called by anything. The userTableStatRead flag is only needed by Query.createSqlQuery(), so the instance variable on its own seems sufficient.

The getter is removed.

Other than that, the changes look good to me. Please address these points (or help me understand what I may have missed), then rebase and merge 3814a back to 3821c. If these are the only changes you make, I don't need to review again before you merge. Thank you.

Re-tested and merged to 3821c/12389.

Igor, I'm re-familiarizing myself with MetadataManager for changes we need for #3912. Can you please help me understand why we have MetadataManager$SystemTable.isIgnoredInMetaXML(), instead of just eliminating the use of the XML file (or at least most of its contents)? Most (probably >99.9% for most projects) of the content of the XML file is about the _File, _Field, _Index, Index-Field, _File-Trig, _Field-Trig, and _Filelist metadata tables, but these are all ignored during the metadata database's population, in this code:

         // walk the DOM and prepare metadata records
         Element root = dom.getDocumentElement();
         NodeList recNodes = root.getElementsByTagName("record");
         int numRecs = recNodes.getLength();
         for (int i = 0; i < numRecs; i++)
         {
            Element record = (Element) recNodes.item(i);
            Record dmo = prepareRecord(record, schema);
            DmoMeta dmoInfo = DmoMetadataManager.getDmoInfo(dmo.getClass());
            SystemTable systemTable = SystemTable.forIface(dmoInfo.getAnnotatedInterface().getSimpleName());
            if (systemTable != null && systemTable.isIgnoredInMetaXML())
            {
               continue;
            }

            log.log(Level.INFO, "Persisting record for the metatable " + dmo.getClass().getSimpleName());
            p.save(dmo, dmo.primaryKey());
         }

Originally, the only purpose in generating the XML file during conversion was to populate the metadata database at server startup. I understand we gather this metadata from the DMO interfaces instead now. If the XML no longer serves this (or any) purpose, we should get rid of it (or at least drop the unnecessary content).

If I have overlooked something, please let me know.

Eric Faulhaber wrote:

Igor, I'm re-familiarizing myself with MetadataManager for changes we need for #3912. Can you please help me understand why we have MetadataManager$SystemTable.isIgnoredInMetaXML(), instead of just eliminating the use of the XML file (or at least most of its contents)? Most (probably >99.9% for most projects) of the content of the XML file is about the _File, _Field, _Index, Index-Field, _File-Trig, _Field-Trig, and _Filelist metadata tables, but these are all ignored during the metadata database's population, in this code:

[...]

Originally, the only purpose in generating the XML file during conversion was to populate the metadata database at server startup. I understand we gather this metadata from the DMO interfaces instead now. If the XML no longer serves this (or any) purpose, we should get rid of it (or at least drop the unnecessary content).

If I have overlooked something, please let me know.

Eric,
You're right - this XML data are obsolete now. I've postponed the complete elimination of the export/import metadata to/from XML until the new population of all meta-tables is completed. It seems that now we can do this.