FWD » Core Development » Database

Consider this test:

def var hb as handle.
def var hq as handle.

create buffer hb for table "book".
create query hq.
hq:add-buffer(hb).
hq:query-prepare("for each book fields (isbn) no-lock").
hq:query-open().
hq:get-first().
message hb::book-title. // error

For static queries, FIELDS and EXCEPT can appear only at DEFINE QUERY. In dynamic query case, FWD appends them to the OPEN QUERY statement, from where they get ignored.

More, FWD doesn't convert properly the DEFINE QUERY ... FIELDS case, either:

define query q for book fields (isbn).
open query q for each book.
get first q.
message book.book-title.

This was found in standalone tests.

Committed on 7999a, revision 14833.
- DynamicQueryHelper, added methods to traverse fields and except phrases of record phrase of the query string, and set include and exclude variables of AbstractQuery.
- in CompundQuery, added methods to set include and exclude of child components.

Ovidiu: Please review.

Review of 7999a, r14833.

Good job. The goal of the task was reached. But I have some things to note, mainly because of performance. They may not seem as much, but they add to global application speed:

• CompoundQuery.java:
  • history header: try keeping the information compact. The two entries are highly redundant;
  • there is a common typo in the new method names setIncludedCompoents() / setExcludedCompoents;
  • use an local variable to cache included.get(buffers[j].getDMOProxy()) value used in 2 places;
  • the code in these methods are pretty much identical. I think we can create a parametrised method to merge both of them;
• DynamicQueryHelper.java:
  • history header: see above. The closing tag */ is broken. Luckily it is closed by the copyright comment;
  • the import list contains new unneeded and unwanted entries;
  • line 650 and 663: the cast to AbstractQuery is not needed, the include() and exclude() methods are declared in P2JQuery interface;
  • the for loops at lines 644 and 657 can be replaces with a bit faster iterations over the Map's entry set. For example:
    

                Set<Map.Entry<Buffer, String[]>> kvPairs = include.entrySet();
                for (Map.Entry<Buffer, String[]> pair : kvPairs)
                {
                   String[] fields = pair.getValue();
    
                   if (fields != null)
                   {
                      BufferImpl buffer = (BufferImpl) pair.getKey();
                      query0.include(buffer.buffer().getDMOProxy(), fields);
                   }
                }
    

    The creation and iteration of KeySet and EntrySet are very similar but in the latter case the Map.get() call is replaced by the simple getValue() getter. Also, note the include.get(buffer) was invoked twice in initial code;
  • in methods collectFieldsList() and collectExcludeList():
    • as above they share pretty much of their code so they are candidate for merging into a single one, to avoid code duplication.
    • line 1452: I think the correct code is propList[j] = field.name; (the property name), not fieldAast.getText() (the legacy field name);
    • does collectFieldsList() throw any Exception?
    • the errMsg. Please be sure the message is the same as in OE, including inner spaces and final dot. Please split it on left aligned substrings which do not pass the standard line limit (110);
    • the int nrChildren = chAst.getNumImmediateChildren(); / for (int i = 1; i < nrChildren; i++) / Aast bufParamAast = chAst.getChildAt(i); is not recommended from PoV of performance. getNumImmediateChildren() iterate children to count them, the getChildAt(i) iterates them again. Use instead a iteration starting with getFirstChild() and advance with getNextSibling. This is probably the fastest way. Alternatively, there is getImmediateChild(int type, Aast start) method in AnnotatedAst but it's slower. The same stands for the inner loop;
    • AFAIK, there can be only one KW_FIELD / KW_EXCEPT sub-node of RECORD_PHRASE. In this case it's normal to leave the outer loop as soon as the (first) node was encountered and processed;
    • when computing the bufname. Instead of bufname.contains("."), please compute bufname.lastIndexOf(".") first (returns -1 if the needle is not present in the stack), cache it and reuse in substring.

Eduard, please address the review to get this done.

Committed on 7999a, revision 14837.

Addressed review in #7999-7. Also for second case in #7999-1, with DEFINE QUERY ... FIELDS, added support at conversion phase to add include/exclude method call for query variable.

Eduard, I rebased 7999a to latest trunk. It is now at rev. 14958. Please do the testing of it with POC and a large customer application regression tests.
I see changes in database_access.rules. Are these mandatory and require reconversion?

Ovidiu, please review latest 7999a.

Alexandru Lungu wrote:

I see changes in database_access.rules. Are these mandatory and require reconversion?

It is required for support of DEFINE QUERY ... FIELDS, because for now a fields/except clause inside a DEFINE QUERY is not taken into consideration at conversion phase. And it requires reconversion.
I tested conversion of Hotel_Gui with this change.

Review of 7999a/14954-14958.
The code looks promising, most of the issues below are low priority. but there are a couple of things in DynamicQueryHelper which seem unfinished. Has the code been incorrectly merged when rebased?

• please update the copyright year in file header to all affected files, regardless of the date in H section;
• database_access.rules:
  • line 420: routines exposed in CommonAstSupport are automatically available to TRPL code, no need to create a special worker. Actually, I have not seen it in use.
  • line 1739: variable name, typo bufnameJavanmae instead of bufnameJavaname ?
• CompoundQuery.java:
  • typo: setIncludedCompoents, setFieldsCompoents, nrCompoents
  • line 3487-3492: I guess a more expressive (and compact) initialization for fieldsCompMap would be:
    Map<DataModelObject, Property[]> fieldsCompMap = isInclude ? included : excluded;
  • setFieldsCompoents() can be private I think;
• DynamicQueryHelper.java:
  • line 92: invalid multi-line comment terminator. Does this compile?
  • lines 153, 520, 521, 1309, 1316: rogue empty lines
  • line 1324: @param for token parameter is missing from method javadoc
  • line 1351: we have getImmediateChild(tokenType) but your code is locally optimized to seek a specific token type and I agree with it;
  • line 1368: byLegacyName() requires that the argument to be normalized (lowercase). You may test with an exclude/field name with not matching casing;
  • line 1377: in case of an invalid field name does OE attempt to recover by looking for another list of fields? is it possible to have multiple fields/exclude options?
  • in parse() method, I think something is missing. The include and exclude maps are defined (line 347) and at lines 637/653 they are checked for null. Where is the private method exclude() called?
• PreselectQuery.java: missing H entry

Ovidiu Maxiniuc wrote:

• line 1368: byLegacyName() requires that the argument to be normalized (lowercase). You may test with an exclude/field name with not matching casing;

I tested with not matching cases, and it works fine with byLegacyName.

• line 1377: in case of an invalid field name does OE attempt to recover by looking for another list of fields? is it possible to have multiple fields/exclude options?

I checked in OE, it is not possible to have multiple fields/except options.

• in parse() method, I think something is missing. The include and exclude maps are defined (line 347) and at lines 637/653 they are checked for null. Where is the private method exclude() called?

It is missing, I do not why it was deleted at some point.

 include = collectFields(pAst, buffers, ProgressParserTokenTypes.KW_FIELD);
 exclude = collectFields(pAst, buffers, ProgressParserTokenTypes.KW_EXCEPT);

I tested this changed on POC, and it was causing some regressions:
1) in query 2 consecutive buffers, where referenced to the same persistence table. for each Customer1 fields (name), each Customer2 fields (name), like in this case Customer1 and Customer2 reference the same table Customer.

This issue is solved by changes in FqlToSqlConverter.

2) a issue related to #8259.

Commited on 7999a, revision 14959.

When the changes from this task are regression tested, please also include the following scenario mentioned in #10763:

def query q1 for a fields (a1).
open query q1 for each a no-lock.
get first q1 exclusive-lock.
message a.a1 a.a2.

We need to verify whether the get first q1 exclusive-lock operation reloads the full record.
This is important because an exclusive-lock on a query defined with a fields clause should trigger a full record fetch, even though the query was originally opened with NO-LOCK.

I have rebased the 7999a, to latest trunk. rev. 16321.

Added changes to handle the case when the query AST is retrieved from cache. Plus the #7999-14 is also handled.

I think the way by which we handle the LOCK type change and all fields retrieval is not right.

We retrieve all fields regardless of the FIELDS/EXCEPT clause, if the LOCK type set by the query is different then LockType.NONE. Which is not correct, because LockType.SHARE lock could also retrieve partial records. Only LockType.EXCLUSIVE lock retrieve the al the fields.

An update of lock does not necessary means that all fields should be retrieved.

NO-LOCK -> SHARE-LOCK - partial fields are retrieved.
NO-LOCK -> EXCLUSIVE-LOCK - all fields are retrieved.
SHARE-LOCK -> EXCLUSIVE-LOCK - all fields are retrieved.
SHARE-LOCK -> NO-LOCK -  partial fields are retrieved.
EXCLUSIVE-LOCK -> NO-LOCK - partial fields are retrieved.
EXCLUSIVE-LOCK -> SHARE-LOCK - partial fields are retrieved.

Things get very fuzzy when 2 buffers to the same table are used:

define buffer book1 for book.
define buffer book2 for book.

def query q1 for book1 fields (isbn), book2 fields (isbn).
open query q1 for each book1 NO-LOCK, each book2.
get first q1.
message book1.book-title book2.book-title.

NO-LOCK on the first buffer will cause retrieval of all fields in both buffers.

open query q1 for each book1, each book2 NO-LOCK.

However NO-LOCK on second buffer does not trigger the same behaviour.

Lorian/Artur: please advice on this topic.

+ added Artur and Lorian as watchers

I guess my question is why we check if the lock is different then LockType.NONE

=== modified file 'src/com/goldencode/p2j/persist/Persistence.java'
--- old/src/com/goldencode/p2j/persist/Persistence.java    2025-08-18 13:28:13 +0000
+++ new/src/com/goldencode/p2j/persist/Persistence.java    2025-11-06 15:39:50 +0000
@@ -685,6 +685,7 @@
 ** 218 LS  20250728          Modified 'tenantChanged' to skip buffer release if the tenant name has not changed.
 ** 219 OM  20250731          Dropped dead code related to old technology no longer in use.
 ** 220 LS  20250722          Changed 'load' to allow skipping full hydration.
+** 221 LS  20251106          Set 'partialFields' to null when the lock is not NO-LOCK.
 */

 /*
@@ -2638,7 +2639,11 @@
       RecordLockContext lockContext = local.getRecordLockContext();
       boolean resetLock = false;
       boolean vstTable = false;
-      
+
+      if (partialFields != null && lockType != LockType.NONE)
+      {
+         partialFields = null;
+      }
       if (temporary)
       {
          updateLock = false;

I think that can be changed to lockType.isExclusive()

Eduard Soltan wrote:

I have made a set of tests for the case when 2 buffers to the same table are used.

And actually the single case when the records are selected partially is this one:

[...]

In rest the records are always fetched fully.

I think this could be handled separately.

For now this branch is ready for review.

Could you review the branch?

Rebased 7999a to trunk rev. 16447.

This branch will work for the large majority of case.

There is only a small case that is not handled by this branch, having 2 buffers to the same table (BOTH have FIELDS or EXCEPT clause, first one and the second one have NO-LOCK). To solve this issue, it would require to add additional overhead of getting previous queries information.

I think this case should be handled in a separate branch, this branch could be reviewed.

While working on #11298, I have discovered that optimized CQ components does not have the included and excluded maps populated.
This is what I have changed to fix this (CQ.serverJoinPreselect and CQ.serverJoinAdaptive):

=== modified file 'src/com/goldencode/p2j/persist/CompoundQuery.java'
--- old/src/com/goldencode/p2j/persist/CompoundQuery.java    2026-04-06 12:34:41 +0000
+++ new/src/com/goldencode/p2j/persist/CompoundQuery.java    2026-04-06 13:49:32 +0000
@@ -5437,6 +5437,8 @@
          {
             return null;
          }
+         optimizedQuery.included = included;
+         optimizedQuery.excluded = excluded;

          return optimizedQuery;
       }
@@ -5494,6 +5496,8 @@
             optimizedQuery.addComponent(serverCopy);
             qcList.add(serverCopy);
          }
+         optimizedQuery.included = included;
+         optimizedQuery.excluded = excluded;

          return optimizedQuery;
       }

Teodor, keep in mind that excluded/included fields belong to a single component. So, if you join multiple components, how do you qualify the fields so you know which field belongs to which table?

Rebased 7999a to trunk rev. 16737.

- if (partialFields != null && lockType != LockType.NONE)
+ if (partialFields != null && lockType.isExclusive())

If the lock is exclusive, OE retrieves all data for a certain record. This change you propose makes SHARE locks behave different than exclusive. But what if the lock is being upgraded? Do we actually refetch the row on lock upgrade and ignore the FIELDS? My concern is not to reach a state where the record is exclusively locked, but only a part of fields are hydrated; I don't think this is a valid state in OE.

I see #7999-18, just make sure that plenty of unit tests cover this. We have faced a customer with an issue where FWD didn't fully hydrated a record on EXCLUSIVE-LOCK.

+ if (!isClientWhere() &&
+ (!fullRecords && component.isIdOnly() || forceOnlyPk) &&
+ included null &&
+ excluded null)

I don't know what to think about this change. These are cases of EXCLUSIVE-LOCK (and SHARE-LOCK) where we do projection queries. Any FIELDS selected here may become stale and incompatible with exclusive locking that ensures that we always lock the most up-to-date image of a record.

With this change, SHARE-LOCK and EXCLUSIVE-LOCK will retrieve FIELDS instead of projection. For SHARE-LOCK, I guess this is fine, but for EXCLUSIVE-LOCK we need to ensure included and excluded are always null. Also, the upgrading of lock should refetch the row with the latest data.

boolean doFetch = (!fullRec || updateLock || dmo.isStale()) && !dirtyCopy;

I think this change can impact the query execution a lot. TBH, this is the scariest change out of all.

• It is dropping the isResultsCached check. Is this correct? I much like that we can get results from cached results directly, but are they subject to STALE?
• Previously to your changes, temporary is dropped. I guess temporary records are never locked / stale or projected, so it is fine.

+ public boolean isStale()

I am skeptic about exposing isStale. This method synchronization. From the moment you check it to the moment you do an action a DMO can become stale. In your case doFetch may be false because the dmo is not stale, but by the time you reach if (doFetch), the DMO may become stale and you would have wanted doFetch to be true in fact.

What is the drive for doFetch change and the need for isStale?

Review of 7999a / r16750

This branch tricked me with a [merge] node at r16743 so the changes were incomplete at first, but things got clearer after identifying the correct branching revision.
I could not find severe issues, especially in the conversion (both static and dynamic). The following couple of issues is just nitpicking:

• database_access.rules:873: the definition of openQueryRef could be moved to 1923 to narrow its scope;
• PreselectQuery.java: 5935: && are not aligned to same screen column.

1. partialFields reset only for exclusive locks

What I measured:

FIELDS(customerName) NO-LOCK,        read an unlisted field  ->  error 8826
FIELDS(customerName) SHARE-LOCK,     read an unlisted field  ->  error 8826
FIELDS(customerName) EXCLUSIVE-LOCK, read an unlisted field  ->  value returned

So on this evidence OpenEdge does honour the field list under a share lock, and reads the whole record only under an exclusive one. If that holds generally, it suggests the current trunk behaviour is stricter than the 4GL for SHARE, and that the narrowing in this branch matches it.

The same pattern holds across 64 read-only combinations of per-buffer and GET lock, all matching OpenEdge.

2. Lock upgrade and refetch

This one was a genuine gap. RecordBuffer.upgradeLock() took the lock and returned without re-reading, so assigning a field under a share lock could leave the record exclusively locked and still incomplete. OpenEdge appears to re-read the whole record as part of that upgrade:

FIELDS(customerName) SHARE-LOCK, assign (book.f1 = 'something'), then read an unlisted field  ->  value returned
EXCEPT(customerEmail) SHARE-LOCK, assign (book.f1 = 'something'), then read the excluded one  ->  value returned

Addressed with a new RecordBuffer.upgradeLockForUpdate(), called from BaseRecord.lockForUpdate(): it takes the lock and then reloads the record if it is incomplete. Reading after the lock also gives the latest committed image, which is the property the review was asking for.

DELETE keeps the plain upgradeLock(). Nothing on the delete path needs a field other than the primary key, and OpenEdge does not appear to complete the record there either - a DELETE trigger sees exactly the projection, and FWD and OpenEdge produce identical output for that case.

3. Projection query under EXCLUSIVE-LOCK

On staleness: under an exclusive lock Persistence.load discards partialFields and re-reads the row after the lock has been taken, so the buffer ends up holding the latest committed image. The 16 exclusive cells of the matrix match OpenEdge. I could not find a case where a stale projection survives.

COmmitted on 7999a, rev. 16753.

• [MAJOR] functional DynamicQueryHelper.parse (lines 866-898): the new FIELDS/EXCEPT include/exclude application runs unconditionally, immediately before the pre-existing if (processor.delayedExecute() && query0 != null) guard on the next statement. On the QUERY-PREPARE path delayedExecute() unconditionally returns true, so at this point interpreter.interpret("execute") has not run and query0 is a constructed-but-uninitialized query (sort null, components null). AdaptiveQuery.include() merges the sort/index fields into the included set via collectIndexedFields(), which parses sort; with sort still null it contributes nothing, so a prepare string such as FOR EACH customer FIELDS(name) WHERE ... BY customer.city ends up including only name and never city — initialize() does not rebuild included, so the truncated set is what the query executes with, producing an incomplete DMO missing the ordering field. This is exactly what the AdaptiveQuery.include() override exists to prevent, and it hits every dynamic FIELDS/EXCEPT query with a BY clause on a permanent table. Apply include/exclude inside the onQueryOpen listener after interpret("execute") for the delayed case, guarding on query0 != null there.
• [MAJOR] functional CompoundQuery.setFieldsComponents: propagation assigns the raw map directly via comp.setInclude(newMap), bypassing AdaptiveQuery.include(), which merges the BY/sort-index fields into the include list via collectIndexedFields() ("necessary in order to add the indexed fields to the included fields in an incomplete DMO"). AbstractQuery.setInclude/setExclude are plain field assignments with no polymorphic hook, and neither CompoundQuery nor DynamicQuery overrides include(). Trigger: a converted multi-buffer OPEN QUERY q FOR EACH a, EACH b ... BY b.f where the FIELDS list for b omits f — the component loads an INCOMPLETE record lacking the sort column, and when the AdaptiveQuery switches from dynamic to preselect mode, prepareParameters() reads the sort property straight off the snapshot record, bypassing BaseRecord.checkIncomplete(), so breakValue is silently null/default and the re-issued query returns the wrong band of rows (skipped/duplicated records) with no error. Fix by applying the merge when propagating — e.g. override setInclude in AdaptiveQuery to run collectIndexedFields and union it into the incoming map, null-guarding the case where sort is not yet set.

These 2 seem less or more the same. There is indeed an important integration of fields with the fields used for indexing. An AQ that gets invalidated and reaches a RAQ will use the indexed fields to resolve records from the primary database against the record in the dirty or use the indexed fields to build up the active bundle.

For instance, an invalidated query that does next will use something like where ... and idxf1 > ? to gather the next record, using the last fetched record as a reference row. We decided at some point that in FWD the indexed fields can't be excluded by EXCLUDE or FIELDS. So, any query that uses FIELD or EXCLUDE will retrieve records with recid, the specified list of fields and always the indexed fields used by the order by. I am not sure if other use cases occured in the mean-time, but the invalidation one required collectIndexedFields. Otherwise, the idxf1 > ? will be resolved to a non-read field, yielding an error when doing NEXT (or PREV).

• [MAJOR] functional PreselectQuery.coreFetch (lines ~7620 and ~7676): both p.load(..., dmo == null ? null : dmo._getReadFields()) calls hand the row DMO's live readFields BitSet down the stack un-cloned. Persistence.load only nulls partialFields for lockType.isExclusive() and otherwise forwards the same object, and Session.getImpl mutates it in place (partialFields.andNot(dmo.readFields)) whenever the session-cached instance for that id is a different INCOMPLETE DMO than the one held in the current result row. Concrete, fully-reachable trigger: a FIELDS-restricted browse under a non-exclusive lock whose result rows hold full Record objects; the row's DMO is evicted from the session cache by the routine BufferManager.evictDMOIfUnused/session LRU path while still referenced by the result row; the same id is re-read with a different FIELDS subset, producing a new cached INCOMPLETE instance; the next coreFetch of the original row then has its own readFields silently cleared by the andNot, after which BaseRecord.checkIncomplete raises a spurious "missing from FIELDS phrase" runtime error (or, with reread-fields enabled, a hidden extra reload) for fields that were in fact read — aborting a running 4GL program. The codebase already guards against exactly this elsewhere (SQLQuery.hydrateRecordImpl clones the BitSet before mutating). Pass (BitSet) dmo._getReadFields().clone() at both call sites, or — preferably — have Session.getImpl copy partialFields before the andNot so every caller is fixed at once.

This is a very good point. The bitset is a mutable data structure. It think it would be safer to not allow _getReadFields to escape the instance and always clone the bit-set when getting the readFields. Check for other users that may maliciously use this getter.

• [MINOR] performance CompoundQuery.retrieveImpl: setFieldsComponents(true)/setFieldsComponents(false) are invoked from retrieveImpl, the per-row retrieval path used by both retrieve() and the preselectResults() loop, not the query-open path (propagation cannot simply move into open(), since that method runs before maybeOptimize() and must not call it). Whenever a FIELDS/EXCEPT clause is active, each row re-walks every component's getRecordBuffers() (which allocates a fresh array on every call) and reallocates a fresh IdentityHashMap per matching component, even though the maps are invariant after the first call — maybeOptimize() is idempotent and included/excluded are only ever set at query setup. The cost is real but modest (a handful of short-lived allocations per row, negligible next to the per-row DB fetch). The common non-FIELDS case is unaffected (both calls return immediately on a null map). Add a run-once guard inside retrieveImpl (cleared in open()) instead of re-propagating every row.
• [MINOR] performance DynamicQueryHelper.parse (lines 866-898): the include/exclude application loops test include.get(bufferAlias) != null / exclude.get(bufferAlias) != null, repeating a hash lookup for a value already held as pair.getValue(). This runs on the common path, including level-1 cache hits, whenever the prepare string carried a FIELDS/EXCEPT clause, and the guard is tautological — collectFields only ever stores a non-null array, so the branch can never be false. Test pair.getValue() != null instead (or drop the guard).

Reasonable points about performance. Whatever we can do only once at opening time should be done there.

• [MINOR] functional DynamicQueryHelper.prepare: in the dataset-sibling loop, bufferNames.put(b.buffer().getDMOAlias(), b.buffer()) reuses the outer loop variable b instead of the sibling buffer buf (dsBuffers.get(i).ref()) — a copy-paste defect (the line is byte-identical to the outer-loop registration two lines above). Dataset sibling buffers are consequently never registered in bufferNames; every dataset/FILL prepare over a valid dataset buffer hits this. It does not currently NPE the FIELDS/EXCEPT lookup — collectFields only ever keys on buffers already proven to be in the buffers list by validateBuffers — but the map is wrong by construction, and the subsequent substBuffers containsKey dedup misses those siblings (currently masked only because LinkedHashSet identity-dedups the same BufferImpl instances). Use buf.buffer().getDMOAlias() / buf.buffer().

Can't tell much about this finding per-se, but I am most certain that we have faces (many) issues in the past with the aliasing of buffers (sending buffers as parameters, using them in dataset, binding to other procedures, etc.). This whole DMOAlias saga should be carefully tested.

Conversion

• [MAJOR] functional database_access.rules.record_phrase FIELDS/EXCEPT emission: the buffer reference emitted for the new include/exclude call omits the static-buffer dereference (.get()) that every sibling rule in this file applies for a static buffer/temp-table. bufnameJavaname comes from execLib("get_javaname", fieldsClause.getPrevSibling()), and get_javaname has no static awareness — it only returns the localname annotation or the javaname of the refid target. Trigger: DEFINE STATIC TEMP-TABLE tt FIELD f AS CHAR. DEFINE QUERY q FOR tt FIELDS(f). OPEN QUERY q FOR EACH tt. — the pre-existing addBuffer rule resolves the same node through add_static_access and emits q.addBuffer(tt.get(), true), while the new block emits q.include(tt, "f") on the next line of generated code, which will not compile since a static buffer converts to a ContextLocal wrapper. Resolve the buffer reference through add_static_access (or the %s.get() suffix) the same way the pre-existing ascent rule does for the inline-FIELDS case.
• [MAJOR] functional database_access.rules.record_phrase FIELDS/EXCEPT emission: the new walk rule does not suppress the pre-existing ascent rule that also emits include/exclude for a FIELDS/EXCEPT clause carried directly on the OPEN QUERY's own record phrase, so both can fire for the same buffer. Trigger: DEFINE QUERY q FOR customer FIELDS(name). OPEN QUERY q FOR EACH customer EXCEPT(city). — legal 4GL, since the two clauses are matched by unrelated grammar productions (def_query_stmt and open_query_stmt). The new rule grafts q.include(customer, "name") while the pre-existing ascent rule grafts q.exclude(customer, "city"), leaving both included and excluded populated for the same DMO; because AbstractQuery.collectFields uses computeIfAbsent and only ever sets slots, a FIELDS clause on both DEFINE and OPEN yields the union of fields rather than the OPEN QUERY clause taking precedence, and when both included and excluded are non-null for one DMO, RandomAccessQuery.getPartialFields silently discards the include set and rebuilds from exclude alone — producing a wrong partial-field fetch set. The two paths must be made mutually exclusive: the new rule should skip a record phrase that already carries its own FIELDS/EXCEPT child, letting the OPEN QUERY clause win.
• [MINOR] functional database_access.rules.record_phrase FIELDS/EXCEPT emission, while loop: the loop that emits each field of the clause has no node-type or null guard on the fieldname annotation, unlike the sibling emission rule which guards with evalLib("fieldtype", ...). The post-parse fixup that strips non-field entries only applies when the clause's parent is a record_phrase, but a DEFINE QUERY's field list is parented by KW_FOR, so it is never sanitized. Trigger: the grammar's own documented "undocumented feature" where a bare record/table name is matched instead of an lvalue list (e.g. DEFINE QUERY q FOR customer FIELDS(customer).), or a field name shadowed by a variable/widget of the same name — both produce a clause child with no fieldname annotation. This does not NPE or corrupt data: createJavaAst(java.string, null, ref) emits the literal Java string "null", AbstractQuery.collectFields logs a warning and leaves that slot unresolved at runtime, but the buffer still gets a (wrong) entry in included, silently narrowing the projection to id/indexed fields only. Guard the loop body with the same evalLib("fieldtype", field.type) test used by the sibling rule.
• [MINOR] functional database_access.rules.is_define_query_buffer: the new fieldsClauses recording is nested inside the addBuffer rule and inherits that rule's unrelated guard excluding shared, non-NEW query declarations. Trigger: an importing procedure containing DEFINE SHARED QUERY q FOR customer FIELDS(name). OPEN QUERY q FOR EACH customer. — legal 4GL (DEFINE [[NEW] SHARED] QUERY ... FIELDS/EXCEPT) — where the guard evaluates false (KW_SHARED present, KW_NEW absent), so the recording rule body never runs; since the DEFINE QUERY's FIELDS node is unconditionally hidden elsewhere, the clause disappears with no diagnostic and the shared query opens with full records instead of the declared projection. The FIELDS/EXCEPT recording should be its own rule guarded only on the DEFINE-QUERY-buffer condition, not on the SHARED/NEW guard.

Not entirely sure about these, especially what "DEFINE STATIC TEMP-TABLE@ is :) Maybe you or Ovidiu can confirm or deny these bullets.

• [MINOR] performance PreselectQuery.coreFetch: passing dmo._getReadFields() unconditionally (including in the updateLock branch, which previously forced null) buys no reduction in the SQL Loader executes, since Loader.load() always runs the precomputed full-column loadSql regardless of partialFields — partialFields only decides which returned columns get copied into the DMO. On a session-cache miss, the row is now left INCOMPLETE, and the first later access to a field outside the projection forces a second, identical full-column SELECT for that row (or, via -rereadfields, another full load) — an extra round trip that did not happen before. This now also applies under a plain SHARE lock (paired with the Persistence lock-type change), whereas before the lock forced a full/null-partialFields load. Note this trades a genuine win on the cache-hit path (skips a previously-forced reload) for a loss on the cache-miss/widening path; consider reducing the actual SQL projection so a partial read costs less, rather than loading full columns into a record left incomplete.

• This is interesting; we emit full SQLs, but cherry-pick results based on the partialFields marker. I doubt this is a usual hit, because the session-cache will usually hit as the record was just hydrated from the result-set. I think this is indeed MINOR and can be deferred to another task. It is worth tho to put a debug breakpoint and check how often is this hit with a non-empty partialFields. It may be a separate optimization opportunity. It is important however to confirm it is not nullifying the changes in #7999 to actually parse FIELDS to end-up not using them in the SQL.

Style

• [MINOR] style DynamicQueryHelper: the import cleanup correctly drops the now-redundant explicit import java.util.List; / import java.util.Map; (covered by the existing import java.util.*;), but it also inserts a new blank line between import java.util.logging.*; and import antlr.*; that does not exist elsewhere in this file or in sibling files (AbstractQuery, CompoundQuery, PreselectQuery). Remove the extra blank line to match the established convention.

• [MINOR] style database_access.rules: the new local <variable> declarations (fieldsClause, bufnameAnnotation, bufnameJavaname, field, strField) are not column-aligned as required by this file's convention of padding name="..." so type= starts in the same column across a block of declarations.

• [MINOR] style database_access.rules: the new worker registration <worker class="com.goldencode.p2j.pattern.CommonAstSupport" namespace="cat" /> is never referenced anywhere in the file (no cat. usage) — CommonAstSupport is already registered under the common namespace elsewhere, making this a dead, unused declaration that should be removed.

• [MINOR] style PreselectQuery.assembleComponent: the new multi-line boolean condition does not fully align its && operators at the line ends:
  

  if (!isClientWhere()                                       &&
      (!fullRecords && component.isIdOnly() || forceOnlyPk) &&
      !hasFieldList)
  

  
  Line 1's && ends at column 67; line 2's && ends one column short at column 66. Add one space before the && on line 2 to align.

Alexandru Lungu wrote:

These 2 seem less or more the same. There is indeed an important integration of fields with the fields used for indexing. An AQ that gets invalidated and reaches a RAQ will use the indexed fields to resolve records from the primary database against the record in the dirty or use the indexed fields to build up the active bundle.

For instance, an invalidated query that does next will use something like where ... and idxf1 > ? to gather the next record, using the last fetched record as a reference row. We decided at some point that in FWD the indexed fields can't be excluded by EXCLUDE or FIELDS. So, any query that uses FIELD or EXCLUDE will retrieve records with recid, the specified list of fields and always the indexed fields used by the order by. I am not sure if other use cases occured in the mean-time, but the invalidation one required collectIndexedFields. Otherwise, the idxf1 > ? will be resolved to a non-read field, yielding an error when doing NEXT (or PREV).

The dynamic application moved into applyFieldsClauses(), called immediately for the FIND path (where execute has already been interpreted) and from inside the onQueryOpen listener right after interpret("execute") for the delayed one. Since the listener is only registered when query0 != null, the null case you flagged is covered by construction. I also added a null guard on the buffer lookup, which could have NPE'd on an unresolved alias.

setInclude and setExclude in AdaptiveQuery now route through a new applyIndexedFields() helper. It copies before writing, because setFieldsComponents shares the parent query's Property[] arrays and mutating them in place would corrupt the compound's own map, and it returns the original map untouched when there's nothing to add.

Two things I found while in there that weren't in the review. First, EXCEPT had the identical hole in reverse — nothing stopped EXCEPT idxf1 from dropping an index field, since exclude() was never overridden the way include() was, and PreselectQuery.assembleComponent nulls out any property present in xprops without checking. So exclude() is now overridden too, stripping index fields rather than adding them, and when every listed field turns out to be an index field the clause is skipped entirely rather than registered empty (an empty clause would suppress the id-only reduction and force a full-record fetch for no reason). Second, collectIndexedFields() had two latent NPEs: the catch around SortCriterion.parse logs and falls through, leaving sortCriteria null for the size() call on the next line, and an unresolvable field would NPE on p.name.

This is a very good point. The bitset is a mutable data structure. It think it would be safer to not allow _getReadFields to escape the instance and always clone the bit-set when getting the readFields. Check for other users that may maliciously use this getter.

BaseRecord._getReadFields() now returns (BitSet) readFields.clone() (null-safe) instead of the live field — the bit set can't escape the instance, which fixes both coreFetch call sites without touching them.
Session.getImpl now clones partialFields before partialFields.andNot(dmo.readFields), so it stops mutating a caller-owned object. Behaviour-neutral: no caller reads it back afterwards.

• [MINOR] performance CompoundQuery.retrieveImpl: setFieldsComponents(true)/setFieldsComponents(false) are invoked from retrieveImpl, the per-row retrieval path used by both retrieve() and the preselectResults() loop, not the query-open path (propagation cannot simply move into open(), since that method runs before maybeOptimize() and must not call it). Whenever a FIELDS/EXCEPT clause is active, each row re-walks every component's getRecordBuffers() (which allocates a fresh array on every call) and reallocates a fresh IdentityHashMap per matching component, even though the maps are invariant after the first call — maybeOptimize() is idempotent and included/excluded are only ever set at query setup. The cost is real but modest (a handful of short-lived allocations per row, negligible next to the per-row DB fetch). The common non-FIELDS case is unaffected (both calls return immediately on a null map). Add a run-once guard inside retrieveImpl (cleared in open()) instead of re-propagating every row.
• [MINOR] performance DynamicQueryHelper.parse (lines 866-898): the include/exclude application loops test include.get(bufferAlias) != null / exclude.get(bufferAlias) != null, repeating a hash lookup for a value already held as pair.getValue(). This runs on the common path, including level-1 cache hits, whenever the prepare string carried a FIELDS/EXCEPT clause, and the guard is tautological — collectFields only ever stores a non-null array, so the branch can never be false. Test pair.getValue() != null instead (or drop the guard).

Reasonable points about performance. Whatever we can do only once at opening time should be done there.

CompoundQuery — setFieldsComponents no longer runs per row. Guarded in retrieveImpl on component-list identity (fieldsPropagatedTo != comps), not a run-once flag cleared in open(), because components is nulled in three places (close(), addDynamicFilter(), clearDynamicFilters()) — a flag would have missed the latter two and left new joined components without the maps. Also nulled in close() next to components = null.

DynamicQueryHelper — the redundant include.get(bufferAlias) != null lookup is gone; the block became applyFieldsClauses() during the ordering fix and tests pair.getValue() != null, plus a rb != null guard that prevents an NPE on an unresolved alias.

• [MINOR] functional DynamicQueryHelper.prepare: in the dataset-sibling loop, bufferNames.put(b.buffer().getDMOAlias(), b.buffer()) reuses the outer loop variable b instead of the sibling buffer buf (dsBuffers.get(i).ref()) — a copy-paste defect (the line is byte-identical to the outer-loop registration two lines above). Dataset sibling buffers are consequently never registered in bufferNames; every dataset/FILL prepare over a valid dataset buffer hits this. It does not currently NPE the FIELDS/EXCEPT lookup — collectFields only ever keys on buffers already proven to be in the buffers list by validateBuffers — but the map is wrong by construction, and the subsequent substBuffers containsKey dedup misses those siblings (currently masked only because LinkedHashSet identity-dedups the same BufferImpl instances). Use buf.buffer().getDMOAlias() / buf.buffer().

Can't tell much about this finding per-se, but I am most certain that we have faces (many) issues in the past with the aliasing of buffers (sending buffers as parameters, using them in dataset, binding to other procedures, etc.). This whole DMOAlias saga should be carefully tested.

DynamicQueryHelper — the dataset-sibling loop now registers buf.buffer().getDMOAlias(), buf.buffer() instead of re-registering the outer b.

Style

• [MINOR] style DynamicQueryHelper: the import cleanup correctly drops the now-redundant explicit import java.util.List; / import java.util.Map; (covered by the existing import java.util.*;), but it also inserts a new blank line between import java.util.logging.*; and import antlr.*; that does not exist elsewhere in this file or in sibling files (AbstractQuery, CompoundQuery, PreselectQuery). Remove the extra blank line to match the established convention.

• [MINOR] style database_access.rules: the new local <variable> declarations (fieldsClause, bufnameAnnotation, bufnameJavaname, field, strField) are not column-aligned as required by this file's convention of padding name="..." so type= starts in the same column across a block of declarations.

• [MINOR] style database_access.rules: the new worker registration <worker class="com.goldencode.p2j.pattern.CommonAstSupport" namespace="cat" /> is never referenced anywhere in the file (no cat. usage) — CommonAstSupport is already registered under the common namespace elsewhere, making this a dead, unused declaration that should be removed.

• [MINOR] style PreselectQuery.assembleComponent: the new multi-line boolean condition does not fully align its && operators at the line ends:
  [...]
  Line 1's && ends at column 67; line 2's && ends one column short at column 66. Add one space before the && on line 2 to align.

Fixed all of that in 7999a rev. 16756.

[MAJOR] functional database_access.rules.record_phrase FIELDS/EXCEPT emission: the buffer reference emitted for the new include/exclude call omits the static-buffer dereference (.get()) that every sibling rule in this file applies for a static buffer/temp-table. bufnameJavaname comes from execLib("get_javaname", fieldsClause.getPrevSibling()), and get_javaname has no static awareness — it only returns the localname annotation or the javaname of the refid target. Trigger: DEFINE STATIC TEMP-TABLE tt FIELD f AS CHAR. DEFINE QUERY q FOR tt FIELDS. OPEN QUERY q FOR EACH tt. — the pre-existing addBuffer rule resolves the same node through add_static_access and emits q.addBuffer(tt.get(), true), while the new block emits q.include(tt, "f") on the next line of generated code, which will not compile since a static buffer converts to a ContextLocal wrapper. Resolve the buffer reference through add_static_access (or the %s.get() suffix) the same way the pre-existing ascent rule does for the inline-FIELDS case.

static buffer dereference. The buffer node is now resolved through its refid and passed to add_static_access, mirroring the addBuffer rule:

  <rule>bufferNode.isAnnotation("refid")
     <action>
        astid = execLib("add_static_access",
                        getAst(getReferenceNoteLong(bufferNode.id, "refid")),
                        ref.id)
     </action>
  </rule>

[MAJOR] functional database_access.rules.record_phrase FIELDS/EXCEPT emission: the new walk rule does not suppress the pre-existing ascent rule that also emits include/exclude for a FIELDS/EXCEPT clause carried directly on the OPEN QUERY's own record phrase, so both can fire for the same buffer. Trigger: DEFINE QUERY q FOR customer FIELDS. OPEN QUERY q FOR EACH customer EXCEPT. — legal 4GL, since the two clauses are matched by unrelated grammar productions (def_query_stmt and open_query_stmt). The new rule grafts q.include(customer, "name") while the pre-existing ascent rule grafts q.exclude(customer, "city"), leaving both included and excluded populated for the same DMO; because AbstractQuery.collectFields uses computeIfAbsent and only ever sets slots, a FIELDS clause on both DEFINE and OPEN yields the union of fields rather than the OPEN QUERY clause taking precedence, and when both included and excluded are non-null for one DMO, RandomAccessQuery.getPartialFields silently discards the include set and rebuilds from exclude alone — producing a wrong partial-field fetch set. The two paths must be made mutually exclusive: the new rule should skip a record phrase that already carries its own FIELDS/EXCEPT child, letting the OPEN QUERY clause win.

I think this is wrong.

I get the following error: FIELDS/EXCEPT belong on DEFINE QUERY, not on OPEN QUERY. (3638) when define FIELDS/EXCEPT phrase in the OPEN statement.

[MINOR] functional database_access.rules.record_phrase FIELDS/EXCEPT emission, while loop: the loop that emits each field of the clause has no node-type or null guard on the fieldname annotation, unlike the sibling emission rule which guards with evalLib("fieldtype", ...). The post-parse fixup that strips non-field entries only applies when the clause's parent is a record_phrase, but a DEFINE QUERY's field list is parented by KW_FOR, so it is never sanitized. Trigger: the grammar's own documented "undocumented feature" where a bare record/table name is matched instead of an lvalue list (e.g. DEFINE QUERY q FOR customer FIELDS.), or a field name shadowed by a variable/widget of the same name — both produce a clause child with no fieldname annotation. This does not NPE or corrupt data: createJavaAst(java.string, null, ref) emits the literal Java string "null", AbstractQuery.collectFields logs a warning and leaves that slot unresolved at runtime, but the buffer still gets a (wrong) entry in included, silently narrowing the projection to id/indexed fields only. Guard the loop body with the same evalLib("fieldtype", field.type) test used by the sibling rule.

Guarded the field-emission loop in database_access.rules so non-field nodes are skipped:

  <while>field != null
     <rule>evalLib("fieldtype", field.type)
        <action>strField = field.getAnnotation("fieldname")</action>
        <action>createJavaAst(java.string, strField, ref)</action>
     </rule>

     <action>field = field.getNextSibling()</action>
  </while>

I reviewed r16757.

• The most problematic issue I noticed being fixed was the b / buf swapped. 👍
• related to exclusion of indexed properties. As Alex said, we agreed that these properties cannot be excluded in a query. I did not comment because I thought we take care of this at a lower level. It seems that that was not the case. The code adds a bit of complexity but it is necessary. BTW, in CompoundQuery there is the fieldsPropagatedTo member field which is an ArrayList. It is assigned and compared to result of getComponents() (using = operator). I wonder whether the correct way to compare is to iterate it and compare each element since getComponents() is not stable (changes with dynamic filters) so the setFieldsComponents() might be called more than once.
• in RecordBuffer:9695, a typo: does acquires sound better instead of takes?

Ovidiu Maxiniuc wrote:

• related to exclusion of indexed properties. As Alex said, we agreed that these properties cannot be excluded in a query. I did not comment because I thought we take care of this at a lower level. It seems that that was not the case. The code adds a bit of complexity but it is necessary. BTW, in CompoundQuery there is the fieldsPropagatedTo member field which is an ArrayList. It is assigned and compared to result of getComponents() (using = operator). I wonder whether the correct way to compare is to iterate it and compare each element since getComponents() is not stable (changes with dynamic filters) so the setFieldsComponents() might be called more than once.

The check sits in retrieveImpl(). Identity is one reference compare per row; element-wise is O(components) per row. The redundant propagation it would avoid happens only when `components` is invalidated, (per dynamic-filter change or re-open), not per row. So we would be paying on the hot path to save work on a cold one.

Committed typo fix and fieldsPropagatedTo invalidation in CompoundQuery.initialize in rev. 16758.

Eduard Soltan wrote:

7999a was merged to trunk rev. 16764 and archived.

Eduard,

Unfortunately this commit introduces regression in big customer M application when trying to access PB:

...
26/09/18 23:26:55.433+0300 |  SEVERE | com.goldencode.p2j.util.ErrorManager | ThreadName:Conversation [00000075:bogus-gui_user], Session:00000117, ThreadId:00000107, User:bogus | Field IdPrj from mpl_opd record (recid 130430975) was missing from FIELDS phrase. (8826)
26/09/18 23:26:56.598+0300 |  SEVERE | com.goldencode.p2j.util.ErrorManager | ThreadName:Conversation [00000075:bogus-gui_user], Session:00000117, ThreadId:00000107, User:bogus | ** Unable to update mpl_opd Field. (142)
...

Can you please take a look. Because this is a kind of stopper for further application update.

What OpenEdge actually delivers¶

I think the rule is whatever OpenEdge has to ship to the client in order to make the client side join becomes readable; what it never ships stays missing. A sibling's predicate and an OF relation have to reach the client to parameterize the second query. A buffer's own where clause and a sort key are resolved in the database, so nothing is shipped and the field stays refused.

Committed on 7999a, rev. 16766.

Eduard, just one thing to check. Are the FIND triggers related to the partial FIELDS? If you do a FIND trigger, would the record reported by that trigger be partially hydrated ... I would think so. But make sure there is no quirk there like "fully hydrate if the session has find triggers".

Is TENANT-WHERE relevant? I would expect that it works like WHERE from the POV of field selection.

I don't think I have other objections. Please test these and lets proceed with review and testing.

Eduard Soltan wrote:

All the cases are handled in 7999a, rev. 16769. Please review.

Do you mean 7999b, not a, right? The a version was archived after merge in trunk. Or I missed for something?

Eugenie Lyzenko wrote:

Do you mean 7999b, not a, right? The a version was archived after merge in trunk. Or I missed for something?

Sorry, committed to 7999b.

Review of 7999b / r16769

• AbstractJoin.java:
  • line 5: (c) year not updated
• CompoundQuery.java:
  • line 345: missing ♯ for the new H entry;
  • lines 4602, 4608: this hybrid approach of chopping the line is not encouraged. More than that, all of them can stay on same line without overflowing the 110 chars limit;
  • line 4706: expression can be simplified. BufferReference extends DataModelObject so !(dmo instanceof DataModelObject) implies !(dmo instanceof BufferReference). The left operand can be safely dropped. This becomes more evident with the cast at line 4719;
• PreselectQuery.java:
  • line 8408: CAN-FIND can also be converted to select count(recid)... = 1 for unique cases. The current where.contains("exists") will cover only the non unique (first / last) cases.

Ovidiu, functionally, do you have any other questions/concerns?

• line length is 110, so please align the method parameters on the parenthesis, one under the other.
• PreselectQuery is missing history number.

Constantin Asofiei wrote:

Ovidiu, functionally, do you have any other questions/concerns?

No. The code looks fine to me.

Eduard.

When do you think the revision 16770 be in trunk? It is tested and working good.

I need to schedule one known remote system updates.

Eugenie Lyzenko wrote:

Eduard.

When do you think the revision 16770 be in trunk? It is tested and working good.

I need to schedule one known remote system updates.

It has to regression tested. I think some where early next week.

Some AI feedback below. I am generating some tests as well to check functionality.

• [MINOR] functional PreselectQuery.applySubselectFields: A multi-component PreselectQuery with FIELDS gets only the CAN-FIND widening. It misses the cross-component predicate fields and the OF-relation fields that CompoundQuery.applyJoinPredicateFields adds. In a PRESELECT the predicate is inline FQL and the OF join is the component's own AbstractJoin, not a FieldReference argument. The conversion emits include/exclude on the preselect query itself, and a native DO PRESELECT is not wrapped in a CompoundQuery, so nothing else widens the map. applySubselectFields never reads comp.getJoin().getInverseRelationProperties(), which this branch adds for exactly this purpose. Trigger: DO PRESELECT EACH customer FIELDS(name) NO-LOCK, EACH order OF customer NO-LOCK:, then reading customer.custnum in the body, raises 8826. The equivalent FOR EACH works. This contradicts the branch's own rationale that the 4GL ships join fields whether or not the join is pushed down.
• [MINOR] functional AdaptiveQuery.createSimpleQuery: The CAN-FIND widening only happens inside PreselectQuery.assembleFQL(). AdaptiveQuery.initialize() and open() call earlyInvalidate(). With intra-session dirty sharing on (the default) and a dirty component, that calls forceDynamicOperation() before assembleFQL() has ever run. The dynamic RandomAccessQuery (or compound query) then gets overrideIncludeFields(included)/overrideExcludeFields(excluded) without the sub-SELECT fields. Trigger: in a transaction, create or update a customer, then run FOR EACH customer FIELDS(name) NO-LOCK WHERE CAN-FIND(FIRST order OF customer) and read a CAN-FIND-referenced field that is not in the index. It still raises 8826, while the preselect path works. Apply the widening to the maps handed to the dynamic query as well, e.g. in createSimpleQuery or setInclude/setExclude.
• [MINOR] functional RandomAccessQuery: The branch adds no counterpart to PreselectQuery.applySubselectFields. RandomAccessQuery.getHelper passes the included/excluded arrays unchanged to FQLHelper.assembleSelectClause, which adds only the sort-index fields. So a field that the CAN-FIND exists() reads off the outer buffer is never fetched. Trigger: a single-table FOR FIRST customer FIELDS(name) NO-LOCK WHERE CAN-FIND(FIRST order WHERE order.salesrep = customer.salesrep), then reading customer.salesrep, still raises 8826, unlike the equivalent FOR EACH. The same gap applies to a FIRST/LAST component of a compound FOR EACH that keeps its own CAN-FIND and is not optimized into a server join, because CompoundQuery.applyJoinPredicateFields covers only arguments and OF joins, not sub-selects. The field has to be outside the sort index, because index fields are always projected.
• [MINOR] functional FQLPreprocessor.collectSubselectProperties: Every ALIAS.PROPERTY under a SUBSELECT is recorded against the alias text, including aliases that the subselect's own FROM binds. canfind_scope_prep.rules lets a CAN-FIND on an already-referenced buffer reuse the outer buffer, and where_clause.rules writes the subselect alias from that buffer's javaname. The inner fields are therefore added to the outer projection. Trigger: FOR EACH customer FIELDS(name) NO-LOCK WHERE CAN-FIND(FIRST customer WHERE customer.balance > 1000) becomes exists(from Customer as customer where customer.balance > ?). applySubselectFields looks up "customer" and adds balance to the outer projection. It is fetched for no reason and becomes readable, although the outer query never reads it; OE behaviour for this exact case was not checked. Skip aliases equal to the FROM alias of the nearest enclosing SUBSELECT.
• [MINOR] performance CompoundQuery.keepQueryField: Every call that changes the map runs result.get(dmo).clone(), even when the array is already a private copy from an earlier call for the same buffer. So a buffer that needs several fields restored has its Property[] cloned once per field. Trigger: an OF join with a multi-field relation none of whose fields are in the FIELDS list. The cost is tiny, since it runs only at projection setup and not per row. Clone only when result.get(dmo) == original.get(dmo). Optional cleanup.
• [MINOR] style PreselectQuery.keepProjectionField: The wrapped signature breaks right after the opening parenthesis and indents the parameters by a flat 12 spaces. The other multi-line signatures in the file, and the sibling helpers this diff adds to CompoundQuery, align continuation parameters after the opening parenthesis.

Alexandru Lungu wrote:

Some AI feedback below. I am generating some tests as well to check functionality.

I will commit my tests to testcases project.

Committed on testcases project.

There are some know issues (31 test are expected to fail):

1. Error 396 comes back as 142 when assigning under NO-LOCK (21 tests): NoLockFieldsAssignRaises396 and the 20 TestLockMatrix cells where the effective lock is NO-LOCK, i.e. GET … NO-LOCK, or cust1 NO-LOCK with no GET lock. OpenEdge raises 396. RecordBuffer.upgradeLock() does raise it, but BaseRecord.lockForUpdate() catches it and always reports 142.
2. A client-side presort drops the FIELDS clause (6 tests): these are the TestFieldsLockUpgrade tests that sort client-side (BREAK BY, DESCENDING, BY an expression). OpenEdge raises 8826 for the unlisted field; FWD returns 0 (no error) because PresortQuery loads full records and ignores FIELDS/EXCEPT. This comes from r16319 (#10921). A plain BY <field> is sent to SQL and works.
3. Reading an unlisted field reports "8826,142" instead of just 8826 (4 tests): 3 in TestFieldsReadErrorCodes, 1 in TestFieldsUpdateErrorCodes. The catch around RecordBuffer.java:13782 handles getters and setters the same way, so a read is reported as a failed update (142). This matches the customer log that prompted these tests.

I committed the tests generated in tests/query/fields_except. Out of the ordinary:

• TestStaticMultiBufferByClause throws ArrayIndexOutOfBoundsException: Index 1 out of bounds for length 1 in PreselectQuery.coreFetch.
• TestStatementsInsideFieldsLoop.testCanFindStatement. is a case where CAN-FIND is nested and uses a field not listed in FIELDS. In OE there is no error and can't find it. In FWD it finds it.

There are other tests failing, but mostly because we server DMOs from session cache that have more fields hydrated that requested by queries. We can accept this mismatch.

While looking at review points in #7999-68. I noticed something in CAN-FIND behaviour in WHERE clause and it is a bit different then initially thought:

1. Normally, FIELDS is obeyed. You can read only the fields you listed. Reading any other field raises 8826. This holds for FOR EACH, FOR FIRST, FOR LAST, FIRST/LAST components, and DO PRESELECT.

2. Exception: a CAN-FIND in a FOR loop's WHERE clause. If a table's own WHERE clause contains a CAN-FIND, OpenEdge ignores that table's FIELDS list and fetches the whole record. Every field of that table is then readable. (It was assumend that only join components are brought)
- It doesn't matter which table the CAN-FIND reads, what it compares, or whether it's NOT, UNIQUE, or inside an OR.
- It works the same with EXCEPT and with SHARE-LOCK.
- It affects only the table whose WHERE contains the CAN-FIND. The other tables in the same FOR keep their FIELDS lists.

3. The exception doesn't apply to DO PRESELECT. There, FIELDS is always obeyed, CAN-FIND or not. Even the field the CAN-FIND itself uses is not readable afterwards.

The CAN-FIND logic should be corrected a bit.

• using the AbstractQuery.INCOMPLETE_RECORD_ERROR marker for identifying a specific situation (missing field from FIELD clause). It comes in the same package with the new incompleteArgument error message and the additional (incomplete) output parameter of AbstractQuery.preprocessSubstitutionArguments(). I think the whole construct doesn't look too strong. But I admit that the problem to be solved is not elementary and also it should work. We (me included, of course) should think of something sturdier.
• in FQLPreprocessor.collectAliasProperties(), the scanning algorithm iterates the nodes in two direction: the first is normal traversal, but when isBoundBySubselect() is evaluated, there is another navigation up the tree. Normally, this should happen fast enought, but it will be multiple times, for maybe 95% of the nodes (next.getType() != ALIAS). For larger trees, this might be noticeable. So I am thinking some dynamic programing may come in handy here: to store intermediary interesting nodes as we descend with the main iterator. Please confirm first that the number of tree traversals is as large as I understand.

I am open to discussion on these two issues.

• AbstractQuery.java:
  • needs a H entry (that should be 121);
  • the new preprocessSubstitutionArguments() method returns true at the end even if INCOMPLETE_RECORD_ERROR was encountered and added to errHlp. I think it should return success instead;
• FindQuery.java, QueryComponent.java and ErrorManager.java: also need a H entries;
• AdaptiveQuery.java: has two H entries, but no number (should be 184).

Ovidiu Maxiniuc wrote:

TBH, there are a few of choices I am not a fan of:

• using the AbstractQuery.INCOMPLETE_RECORD_ERROR marker for identifying a specific situation (missing field from FIELD clause). It comes in the same package with the new incompleteArgument error message and the additional (incomplete) output parameter of AbstractQuery.preprocessSubstitutionArguments(). I think the whole construct doesn't look too strong. But I admit that the problem to be solved is not elementary and also it should work. We (me included, of course) should think of something sturdier.

A soultion could be:

1. Open a slot. When preprocessSubstitutionArguments evaluates an argument, the buffer manager already increments a "query depth" counter. That increment now also pushes an empty slot onto a stack. The slot means "a query is listening for this kind of read right now".
2. Report, don't raise. BaseRecord.checkIncomplete() is the one place that notices a read of a left-out field. It builds an IncompleteRecordRead, a small value with table, field and recid, and asks the buffer manager to take it. If a slot is open, the buffer manager stores the read in it and the getter simply returns unknown. If no slot is open, meaning an ordinary DISPLAY or ASSIGN, checkIncomplete() raises the 8826 itself, as before.
3. Take and report later. After the argument's evaluation, preprocessSubstitutionArguments takes the read out of the slot. If there is one, the argument becomes unknown and the read is returned to the query. The query keeps it and, when it runs, calls raise() on it, which records or throws the 8826 according to the statement's own NO-ERROR mode, and finds nothing. A CAN-FIND just answers NO.

Committed on 7999b,rev. 16776.

1) The rule is unchanged: if a WHERE argument reads a field the FIELDS phrase left out, the query finds nothing and reports 8826 in its statement's own mode, and a CAN-FIND answers NO. What changed is how the read gets from the record to the query.

- orm/IncompleteRecordRead.java, new and still unversioned: a small value with table, field and recid, plus getMessage() and raise(). It owns the 8826 number and text.
- orm/BaseRecord.java: checkIncomplete() offers the read to the buffer manager. If a query is resolving its arguments, the buffer manager keeps it and the getter returns unknown. Otherwise it raises 8826 on the spot, as before.
- BufferManager.java: the depth counter opens a slot per level. reportIncompleteRead() fills the innermost slot, takeIncompleteRead() empties it.
- AbstractQuery.java: While an argument is evaluated, the buffer manager keeps a slot open. If the argument reads a field the FIELDS phrase left out, the record does not raise 8826; it drops a small description of that read into the slot instead. After evaluating each argument, the method looks into the slot. If a read is there, it replaces the argument's value with unknown and remembers the read. When all arguments are done, it returns that read to the caller, or null if no argument read a left-out field.

The caller, the query, then raises the 8826 when it runs, or answers NO for a CAN-FIND.

- RandomAccessQuery.java, QueryComponent.java: keep the read and call raise() when they run. FindQuery needed no change.

2) The alias-collection walk in FQLPreprocessor, and where its result is used (two files)

- FQLPreprocessor.java: collectAliasProperties() no longer climbs the tree from every alias, and no longer drops references to an alias a sub-select binds itself, because OpenEdge ships such a field too. A new static namedProperties(where) gives the same answer from clause text alone.
- CompoundQuery.java: keepNamedFields() keeps the fields a component's WHERE clause names of another component's buffer, which only a CAN-FIND reusing that buffer name produces. Before, only FieldReference arguments and OF joins were considered.

Committed on 7999b, rev. 16779.

Fixed some AI generated review.

Review of 7999b / r16779

• AbstractQuery.java
  • please help me clarify: during preprocessSubstitutionArguments(), if an argument failed (with INCOMPLETE_RECORD_ERROR in particular), the rest of the list is still evaluated?
  • at line 589: ErrorConditionException is detected. The cause is not a IncompleteRecordRead. null is returned. The caller will/should interpret this as success? The old code returned false if any error occurred. OTOH, from my searches, for all previous usages of this method the returned value was ignored.
• FQLPreprocessor.java
  • the new namedProperties(String). I understand this will do a new parsing of the where? Indeed, it tries to benefit from the existing astCache. We should extract the common code from parse(). I do not fully get why isBoundBySubselect(next) is not necessary anymore.
• TemporaryBuffer.java
  • the moved lines in removeRecords() may cause semantic differences. For example, the execution may return before clearBuffers(); and release(true); have the chance to be executed.
• IncompleteRecordRead.java
  • raise(): I find this a bit strange:

          try {
             ErrorManager.recordOrThrowError(ERROR_NUMBER, getMessage(), false);
          } catch (ErrorConditionException exc) {
              ...
              throw exc;
          }

    It's like throwing the ball of a kendama toy: you know it will not go far; sometimes it will be caught in the pin, sometimes not.

Secondary note: somehow, there is still a 7999a. But there is also the archived version of it. 🤷‍♂️

Ovidiu Maxiniuc wrote:

• AbstractQuery.java
  • please help me clarify: during preprocessSubstitutionArguments(), if an argument failed (with INCOMPLETE_RECORD_ERROR in particular), the rest of the list is still evaluated?
  • at line 589: ErrorConditionException is detected. The cause is not a IncompleteRecordRead. null is returned. The caller will/should interpret this as success? The old code returned false if any error occurred. OTOH, from my searches, for all previous usages of this method the returned value was ignored.

Yes, the rest of the list is still evaluated. A read of a left-out field is no exception: that argument becomes unknown like any other failed one, the read is remembered, the remaining arguments are evaluated, and the query raises the 8826 afterwards, or answers NO for a CAN-FIND. When several arguments met a read, the first one is the one reported.

OpenEdge behaviour: a FIND whose where clause first reads the left-out field and then calls a user-defined function that counts its calls. The function was called once, so OpenEdge evaluates the other elements of the where clause after the read of an unlisted field as well. FWD gives the same count.

• FQLPreprocessor.java
  • the new namedProperties(String). I understand this will do a new parsing of the where? Indeed, it tries to benefit from the existing astCache. We should extract the common code from parse().

- A compound query, the client-side join of FOR EACH customer ..., EACH item ..., propagates its FIELDS or EXCEPT map to its components in setFieldsComponents, on the first fetch. That is where applyJoinPredicateFields widens the map with the fields one component's clause names of another buffer, and where keepNamedFields needs the alias.property list of each component's where clause.
- The natural source of that list is the component's FQLPreprocessor, which computes it in preprocess() from the parsed clause and serves it through aliasProperties(alias). PreselectQuery.adjustProjection uses exactly that, because by the time it runs each component has built its preprocessor as part of assembling the FQL.
- In CompoundQuery the components have not built their preprocessors yet at that point. Asking a component for one from there creates it, and creating it does two things too early: it resolves the component's substitution arguments, and it adds the component's aliases to the not-nullable alias set the components share, in an order that differs from the assembly order. That set decides which null checks the generated SQL gets, so the query itself would come out different.

I am not sure, maybe there is a better way to do that.

I do not fully get why isBoundBySubselect(next) is not necessary anymore.

What isBoundBySubselect was doing:

isBoundBySubselect(ref) took an alias.property reference, climbed to each enclosing SUBSELECT node, and looked at that sub-select's FROM and JOIN phrases. If one of them declared an alias with the same name as the reference, it returned true and the reference was not collected. Three shapes cover it:

1. Reference outside any sub-select. item.itemNum = ? and customer.customerAddress = ? in a component's clause. No SUBSELECT ancestor, so false: collected. This is the ordinary join predicate.
2. Correlated reference inside a sub-select. exists(from Custnote as custnote where custnote.noteText = customer.customerAddress), the FQL of can-find(first custnote where custnote.noteText = customer.customerAddress). The sub-select binds custnote, not customer, so customer.customerAddress is a reference to the outer buffer: false, collected. OpenEdge ships that field, and FWD did too.
3. Reference to the alias the sub-select itself binds. exists(from Customer as customer where upper(customer.customerEmail) = 'MAIL-9026'), the FQL of can-find(first customer where customer.customerEmail = "mail-9026") inside a FOR EACH that already reads customer. The sub-select's FROM declares customer, the reference names customer: true, excluded. Nested one level deeper, can-find(first custnote where ... and can-find(first customer where customer.customerEmail = custnote.customerAddress)), the inner sub-select binds customer, so the same exclusion applied through the climb.

But I found out that this is logic got as a wrong OE matching behaviour:

for each customer fields(customerName) no-lock where customer.customerNum = 9026,
     each item no-lock
        where item.itemNum = 97856
          and can-find(first customer where customer.customerEmail = "mail-9026"):
     display customer.customerEmail.
  end.

You can observe that this one is case the third case, where the CAN-FIND reuses customer buffer from the first component.

The first buffer is read with FIELDS, so normally only customerName is fetched and reading any other field of that record raises error 8826. The second buffer's where clause contains a CAN-FIND that mentions customer.customerEmail. The question is whether that mention makes OE fetch customerEmail for the first buffer too, so that the DISPLAY works and it does.

This settle the question and purpose of isBoundBySubselect is outdated.

• TemporaryBuffer.java
  • the moved lines in removeRecords() may cause semantic differences. For example, the execution may return before clearBuffers(); and release(true); have the chance to be executed.

Fixed.

• IncompleteRecordRead.java
  • raise(): I find this a bit strange:[...]It's like throwing the ball of a kendama toy: you know it will not go far; sometimes it will be caught in the pin, sometimes not.

This logic was actually triggered from incorrect OE measurement.

An error a raises in a nested FOR EACH's where clause is charged to the block enclosing the FOR EACH statement, which ends its iteration, exactly as for a FIND. The marked exception made FWD leave the inner block and continue after it, one block too low.

The read is raised as the query is built. The error therefore lands in the enclosing block, as in OpenEdge, in every mode: silent, CATCH and THROW are all above that point. raise() is a plain recordOrThrowError call again, with no catch, ErrorManager and ConditionException are untouched, and the FOR EACH never retries because it never sees the error. FindQuery defers the raise to the find itself, so a CAN-FIND still answers NO.

Committed on 7999b, rev. 16780. Fix for IncompleteRecordRead error handling and TemporaryBuffer issue.

I generated:

• FIELD-LIST phrase
• and its private counterpart that can be found listed in Knowledge Base

In order to document the behaviors detected in #7999, cross-checked against other Redmine tasks, trunk source code, VCS and testcases.
Eduard, feel free to take a look on these pages for any out of the ordinary statement. Mind that these state truth based on trunk and recognize #7999 as a task that mentions issues that are not yet fixed in trunk. After merging, I will regenerate these pages.

All know applications passed regression testing with this branch changes.

Edurad, please:

• list the places where tests for #7999 can be found in testcases
• list the limitations (i.e. mismatches with OE) that still occur in FWD (e.g. returning full DMO from session cache even if a record with restricted set of fields is requested).

The changes needed in 7999a.

- the FIELDS/EXCEPT lists are stored in the level-1/level-2 query caches (the ParametrizedJast include/exclude maps). Before that revision they were keyed by the Buffer instance (include.get(buffer)). On a cache hit the query runs with different Buffer instances, so those keys no longer match. The fix was to key the lists by DMO alias, a string that stays the same between runs. Applying then needs a lookup from alias to the current call's RecordBuffer.

Fix for this issue is the following patch:

--- src/com/goldencode/p2j/persist/DynamicQueryHelper.java    2026-10-02 15:05:18.613924912 +0300
+++ src/com/goldencode/p2j/persist/DynamicQueryHelper.java    2026-10-02 15:05:18.616924906 +0300
@@ -429,11 +432,13 @@
                                  List<Buffer> substBuffers)
    {
       Set<Buffer> allBuffersSet = new LinkedHashSet<>();
+      Set<String> allBufferNames = new HashSet<>();
       Map<String, RecordBuffer> bufferNames = new HashMap<>();
       for (int j = 0; j < buffers.size(); j++)
       {
          BufferImpl b = (BufferImpl) buffers.get(j);
          allBuffersSet.add(((BufferImpl) b).ref());
+         allBufferNames.add(b.buffer().getLegacyName().toLowerCase());
          bufferNames.put(b.buffer().getDMOAlias(), b.buffer());

          // when a dataset buffer is added to a query, all 'sibling' buffers are made available to the query,
@@ -445,6 +450,7 @@
             {
                BufferImpl buf = dsBuffers.get(i).ref();
                allBuffersSet.add(buf);
+               allBufferNames.add(buf.buffer().getLegacyName().toLowerCase());
                bufferNames.put(buf.buffer().getDMOAlias(), buf.buffer());
             }
          }
@@ -454,10 +460,11 @@
          for (int i = 0; i < substBuffers.size(); i++)
          {
             BufferImpl buf = ((BufferImpl) substBuffers.get(i)).ref();
-            String bufName = buf.buffer().getDMOAlias();
-            if (!bufferNames.containsKey(bufName))
+            String bufName = buf.buffer().getLegacyName().toLowerCase();
+            if (!allBufferNames.contains(bufName))
             {
                allBuffersSet.add(buf);
+               allBufferNames.add(bufName);
                bufferNames.put(buf.buffer().getDMOAlias(), buf.buffer());
             }
          }

Eduard, feel free to create a 7999c whenever changes are ready. I will help with testing. I found another module in the customer application I am testing - with this same problem, so I can properly validate a fix.

Please archive 7999a; it is still active.
7999c fixes the issues in the module that I used to discover the regression.

Eduard, please assess:

list the places where tests for #7999 can be found in testcases
list the limitations (i.e. mismatches with OE) that still occur in FWD (e.g. returning full DMO from session cache even if a record with restricted set of fields is requested).

Alexandru Lungu wrote:

Eduard, please assess:

list the places where tests for #7999 can be found in testcases
list the limitations (i.e. mismatches with OE) that still occur in FWD (e.g. returning full DMO from session cache even if a record with restricted set of fields is requested).

It is assessed in the documentation, do you want that also posted here?

On my part, testing is complete with 7999c. I worked on more tests and discovered:

• Failing tests: TestFieldsRecordUpgradeCompletion#testDefineQueryShareLockAssignRefetches, TestFieldsRecordUniqueValidation#testCollidingAssignRejected
  • Cause: In BaseRecord.setDatum (orm/BaseRecord.java:912-952), assigning the same value skips the lock upgrade, so the partial record is never completed. When a rejected ASSIGN is undone, it restores a placeholder taken before the record was completed. The agent believes, without running it, that this persists an unknown date.
  • Where to fix: Runtime: on a partial record, call lockForUpdate() first. setAllData needs the same change.
• Failing tests: TestFieldsExceptKeywordForms#testReadUnderEmptyParensDefineQuery
  • Cause: An empty DEFINE QUERY … FIELDS () converts to a bare include(customer), which means "indexed fields only". post_parse_fixups.xml:853-870 drops an empty list only on record phrases.
  • Where to fix: Conversion
• Failing tests: TestNestedUnlistedFieldRead (2 tests)
  • Cause: When a nested WHERE fails on an unlisted field, IncompleteRecordRead.raise() (orm/IncompleteRecordRead.java:150-153) reports number 8826. OE reports 0, with "(8826)" only in the text.
  • Where to fix: Runtime
• Failing tests: TestFieldsExceptListErrors (two 7328 tests)
  • Cause: FWD deliberately fails QUERY-PREPARE on an unknown or abbreviated FIELDS/EXCEPT name. OE records 7328 and still returns yes.
  • Where to fix: Runtime (DynamicQueryHelper.java:1888-1898)

Eduard, please assess #7999-104 and lets handle any of the issues you confirm on a 7999d. I want 7999c in trunk asap as it fixes a regression from the previous branches. Let me know when testing on your side for 7999c is ready.

Added tests to testcases/1929.

Feel free to test this last 7999c against whatever you see fit.

Eduard, ready to merge 7999c?

The testing performed Chui, ETF and large gui application smoke tests and unit tests.

I am waiting on another large gui app tests.

Eduard Soltan wrote:

I am waiting on another large gui app tests.

Actually I got an response from that too.

Eduard Soltan wrote:

Question, how can I delete 7999a branch from devsrv? It was created by accident, merged to trunk version was archived.

Simply removing the folder from p2j/active folder is enough. I did it for you while I was on devsrv01.

Status changed from Merge Pending to Test

Eduard, as mentioned, please assess #7999-104 when you get the chance. If they are valid issues about FIELD/EXCEPT, then I prefer to have them fixed in this task. I plan to port these changes to a customer (#11848) and we need to ensure there are no limitation as far as we could test. If you find any of the bullets reasonable, please move this back in WIP.

AI review:

• [MAJOR] functional RecordBuffer.copy: The new InvocationTargetException unwrapping, which hands off to copyFieldError, never sees the 15747 on the BUFFER-COPY statement or method path, so an integer overflow is still not reported as 5368. Trigger: BUFFER-COPY (statement via BufferCopy, or method via BufferImpl.bufferCopy) from an int64 field holding a value above 2147483647 into a same-named integer field. The data handlers differ, so DatumAccess.isDirectAccess is false and the copy goes through copy(srcProxy, dstProxy, ...). The destination setter then runs in RecordBuffer.Handler.invoke, where integer.checkBounds raises 15747. The handler's catch (InvocationTargetException) maps every setter ConditionException to recordOrThrowError(142, ...), so copy unwraps a 142, which copyFieldError rethrows. The 4GL program gets 15747 followed by a thrown 142 ERROR, not 5368 with a FALSE result. The unwrap only sees the real 15747 on the COPY-TEMP-TABLE path in TemporaryBuffer, where the destination is a raw TempRecord. Let the original 15747 pass through Handler.invoke while bulkCopy is set, as the new 8826 exception does, or give copyFieldError another way to see the original cause.
• [MINOR] functional RecordBuffer.copy: In the BUFFER-COPY method form, checkCopyDestination runs the getPropsMap supplier for a non-NEW NO-LOCK destination, and getPropsMap has side effects. For a subscripted EXCEPT element it calls ErrorManager.displayError(12414, ...) and returns null. checkCopyDestination treats a null map as "may proceed". fastCopy declines because forcePair is set (hasExcept), so the main getPropsMap call runs and shows 12414 a second time, with a second addRaisedCondition. The 396/5368 pair is never raised. Trigger: hDst:BUFFER-COPY(hSrc, "f[1]") where hDst holds a NO-LOCK database record that is not NEW. A bad PAIRS list (9034/9035) is recorded twice the same way under NO-ERROR. The statement form is not affected, because it passes createCopyMap as the supplier. Compute the props map once and reuse it, or return false when the pre-check map is null.

From my POV:

• There are zseveral questionable places that may need logging:
  • if (!completeForUpdate()) return false returns directly silently. I think a log is suitable, but more than this I want to understand if a 4GL compatible error should have been thrown instead.
  • !checkCopyDestination(dstBuf, ... return new logical(false) is also something that may need error condition instead? Also, use the false singleton in logical

+      // TODO: the 4GL order of the statement is assumed case-insensitive, as its identifiers are; no
+      //       measured case so far had field names differing only in case

There is this todo left.

Alexandru Lungu wrote:

From my POV:

• There are zseveral questionable places that may need logging:
  • if (!completeForUpdate()) return false returns directly silently. I think a log is suitable, but more than this I want to understand if a 4GL compatible error should have been thrown instead.

completeForUpdate() only fails when lockForUpdate() fails. That method always raises the error first, with ErrorManager.recordOrThrowError: 396 for a NO-LOCK record, otherwise 142.

• !checkCopyDestination(dstBuf, ... return new logical(false) is also something that may need error condition instead? Also, use the false singleton in logical

ErrorManager.recordOrThrowError is called first, and in NO-ERROR mode it records the error and returns FALSE. This was tested in OE.

There is this todo left.

THere is nothing to do here, after some OE tests it is confirmed that FWD behaviour matches. Fixed the comment.

Fixed all review issues in 7999d, rev. 16792.