FWD » Core Development » Database

During stress testing of a customer application, IGNORE_FIELDS_ERROR was unexpectedly thrown

Caused by: java.lang.reflect.InvocationTargetException
    at jdk.internal.reflect.GeneratedMethodAccessor729.invoke(Unknown Source)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:569)
    at com.goldencode.p2j.util.Utils.invoke(Utils.java:1799)
    at com.goldencode.p2j.persist.RandomAccessQuery.executeImpl(RandomAccessQuery.java:5764)
    ... 71 more
Caused by: java.lang.RuntimeException
    at com.goldencode.p2j.persist.orm.BaseRecord.checkIncomplete(BaseRecord.java:1148)
    at com.goldencode.p2j.persist.Record._getCharacter(Record.java:918)

I think these are the potential callers of record getters:
1. FieldReference getObject
2. LockTableUpdater invoke - can't be incomplete
3. RecordBuffer invoke - already handled
4. TemporaryBuffer createRecords - can't be incomplete
5. RecordBuffer copy
6. MetadataManager setValue
7. RandomAccessQuery executeImpl
8. MetaTableWrapper invoke
9. RecordBuffer copyCommonFields

I am not sure which of the callers above cannot call the getters on an incomplete record (apart from the mentioned ones).

Constantin, to answer your question.

is it guaranteed that the placeholder record still exists in the table - in other words, could this record have been deleted and loading it will fail?

4GL allows fields only for preselected and presorted fetches, you cannot delete a record while it's being iterated.
I think we are safe to reload the full records if a getter fails due to an incomplete record being read since in that moment we will be in a for or preselect block.

I've seen in the OG task #11301 the following line:

Record placeholder = referenceRecord != null ? referenceRecord : buffer.getSnapshot();

and the comment that one of these may be incomplete.

I wanted to share some knowledge here:

• The RAQ may use a referenceRecord when doing FIND NEXT/PREV or an invalidate AQ does GET NEXT/PREV. The snapshot is used if there is no reference record, so the last record loaded in the buffer is used.
  • From my investigation, getSnapshot is a very fragile workaround, mostly because one can simply load another record, change it and release it before using NEXT/PREV. Which in this case, the placeholder will be null, so that NEXT will provide the first row. I faced this issue just recently in #10962 - invalidated multi-table AQ was not setting any reference record and the buffer was released, so that NEXT was providing FIRST instead.
  • I wouldn't bet on getSnapshot at all :)
• NEXT/PREV is not using the last loaded record in the buffer by the query / FIND! According to #9030. NEXT/PREV uses the last searched record. I know this sounds confusing, but bear with me:
  • If you search for a specific record with f1 = x and f2 > y and you find one (x, z), then this will be used for the upcoming NEXT, whatever you continue doing with the buffer. But most importantly, the reference record should technically save only the indexed fields used by NEXT. There is absolutely no need to store all rows and it is not correct to store only FIELDS.
  • If you search for a specific record with f1 = x and f2 > y and you find none, then the query goes off-end / buffer is released, but the upcoming NEXT with look for a record with f1 = x and f2 > y, even if its WHERE is TRUE (e.g. FIND NEXT tt WHERE f1 = x AND f2 > y. FIND NEXT tt! So if you plan to insert a new record, then it will be found only if it has f1 = x and f2 > y, although the last loaded record didn't necessary had f2 > y. In other words, the NEXT/PREV uses the last searched record. If you insert one with f2 <= y, it will not be found.
  • In #9030 I had a prototype where I replace referenceRecord with a special class instance that snapshots the indexed fields of the last loaded buffer or a virtual cursor representing the last "serched" record.
  • In neither of the cases, the FIELDS is used or snapshot honored.

• it looks tempting to reload the record used as a placeholder, but theoretically speaking, the placeholder is just a "view" over the last "searched" record. This searched record might:
  • exist
  • not exist (as it was deleted)
  • not be a real record, but a template
  • other cases? locked, incomplete, updated, etc.
• the real fix for this would be #9030 where we get rid of referenceRecord / snapshot are Records and replace them as virtual record (AFAIK is a Record class per se to match typing, but it has a controlled lifecycle and scope).

Theoretically: reference record is a cursor inside the index that might stay in between records, having a certain constraint to accomplish on NEXT/PREV.

Alex, please take a look at 11301c.

Artur Școlnic wrote:

Constantin, to answer your question.

is it guaranteed that the placeholder record still exists in the table - in other words, could this record have been deleted and loading it will fail?

4GL allows fields only for preselected and presorted fetches, you cannot delete a record while it's being iterated.
I think we are safe to reload the full records if a getter fails due to an incomplete record being read since in that moment we will be in a for or preselect block.

Do you have a test which shows when FWD switches to RAQ a FOR EACH? Then, it can use something like:

for each book fields (isbn) no-lock:
   pause.  // pause on the record you want to delete

   // on pause, another client just does 'delete book.' on that record 
   // the point is: find when FWD switches the query to RAQ, and try to delete the reference record
end. 

Constantin Asofiei wrote:

Alex, please take a look at 11301c.

+ Alexandru

Teodor, I forgot to mention something, this issue is visible only during load testing with 100 concurrent clients. The problem is not that FWD does not honor fields, but that it does not honor -rereadfields.

I know, but I have linked Eduard's task to be aware of NO-LOCK/SHARE-LOCK/EXCLUSIVE-LOCK behavior when using FIELDS and the fact that when the pk is not being found from Session cache, it fetches the entire record.

Alex, please take a look at 11301c.

Will do now.

Artur Școlnic wrote:

4GL allows fields only for preselected and presorted fetches, you cannot delete a record while it's being iterated.

java.lang.NullPointerException: NULL instance used when invoking method public abstract com.goldencode.p2j.util.character com.goldencode.dataset.dmo.fwd.Book.getBookTitle()>
        at com.goldencode.p2j.util.Utils.invoke(Utils.java:1796)
        at com.goldencode.p2j.persist.RecordBuffer$Handler.invoke(RecordBuffer.java:13433)
        at com.goldencode.p2j.persist.$__Proxy5.getBookTitle(Unknown Source)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

I am not sure this is right:

• buffer.loadRecord(placeholder);! is not looking right. placeholder is (usually) a record snapshot with DMO state as COPY. I don't think loading a COPY record in a buffer is right, especially if it is going to be used in a persistence context (like the bullet below).
• buffer.reload will enforce reloading the placeholder. But as mentioned, the placeholder is a COPY. Technically speaking, it is not bound to any persistence record, so the reload might fail. Even if it won't fail, it might simply work by chance. The placeholder, although its type is Record, it is not a DB record per-se. It might be filled with garbage stale data. Even its recid might be inconsistent: might have been reclaimed, might have been deleted, might have not existed at all. There is no requirement to have a valid recid in the COPY - you can simply have it as -1. This is a placeholder to compute the right NEXT/PREV; it is no necessarily an existing record, so one should not assume that loading/reloading the placeholder will yield a TRACKED record.

From my POV, the placeholder in the first place SHOULD have been minimally hydrated.

For example, presuming that the index is (f1, f2):

FIND FIRST x FIELDS (f1) NO-LOCK.
FIND NEXT x NO-LOCK.

Disclaimer: this is not a test-case in OE; it is pseudo-code to describe the situation.

This should have saved a snapshot / reference record that had both f1 and f2 hydrated. The upcoming NEXT would use that to determine the next record. Also, I do not think this issue is related to IGNORE_FIELDS_ERROR at all. The program above should work even without rereadfields. OE has a cursor on the respective index, so it implicitly has access to the index fields without rereadfields. The same should hold for FWD.

for each Book:
    delete Book.
end.

do transaction:
    create Book.
    assign 
        Book.book-id = 1
        Book.book-title = "title" 
        Book.dttz = now.
        Book.isbn = "1234".

    release Book.
end.

DO PRESELECT each Book fields (book-id isbn) no-lock:
    FIND NEXT Book.
    pause. //book is deleted by another client
    message Book.isbn. // is present
    message Book.book-title. // cannot be read and raises missing fields error
end.

Alexandru Lungu wrote:

This should have saved a snapshot / reference record that had both f1 and f2 hydrated.

Does that mean that the initial record should have been fully hydrated? If yes, when?

DO PRESELECT each Book fields (book-id isbn) no-lock:

What index is used in this statement?

message Book.book-title. // cannot be read and raises missing fields error

is book-title part of the index used by DO PRESELECT?

Also, I am not entirely sure how this code is using RAQ. I would imagine this code converts to a PreselectQuery, right?

Alexandru Lungu wrote:

DO PRESELECT each Book fields (book-id isbn) no-lock:

What index is used in this statement?

book-id only.

message Book.book-title. // cannot be read and raises missing fields error

is book-title part of the index used by DO PRESELECT?

no

Also, I am not entirely sure how this code is using RAQ. I would imagine this code converts to a PreselectQuery, right?

Yes, I cannot replicate the issue with a test case and I am not sure how we end up in RAQ from a for each/preselect.

Yes, I cannot replicate the issue with a test case and I am not sure how we end up in RAQ from a for each/preselect.

for each is invalidated if you change a record. You can check xfer/testcases adaptive-(no)scroll* tests for examples. If you change/delete/insert a record in "the way" of a FOR EACH (aka on the index that is used by the FOR EACH), the query is invalidated and operated in dynamic mode, using an underlying RAQ that calls upon next. So basically, results are gathered one-by-one using a NEXT algorithm: get the next record > than the placeholder and so on.

Artur Școlnic wrote:

[...]
I don't think 4gl hydrates all the fields on find.

That's what it looks like from your testcase. The record is partially loaded in buffer by FIND NEXT Book. using the options FIELDS (book-id isbn) from DO PRESELECT query. Only when the other fields are expressly requested by MESSAGE Book.book-title. and 4GL realizes that they are not in-memory will attempt to load them and finds that the record is no longer valid.

This have maybe more logic in it than the previous line (MESSAGE Book.isbn.), where the stale information of a no-longer existing record is printed on screen.

But who am I to comment on the logic of 4GL? FWD must mimic the original behaviour. And, in this case, we also take note whether (all?) the indexed components are automatically fetched initially, or they are treated just like the other fields, as Alex noted.

I manage to come up with a recreate in a single session:
Book should have at least 2 records.

define buffer buf for Book.
define var fl as logical init true.

for each book fields(isbn) no-lock:
    message isbn. 
    if fl = true then do:
        find last buf no-error.
        if avail buf then buf.book-id = buf.book-id + 1.
        fl = false.
    end.
end.

Caused by: java.lang.reflect.InvocationTargetException
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:569)
    at com.goldencode.p2j.util.Utils.invoke(Utils.java:1799)
    at com.goldencode.p2j.persist.RandomAccessQuery.executeImpl(RandomAccessQuery.java:5764)
    ... 37 more
Caused by: java.lang.RuntimeException
    at com.goldencode.p2j.persist.orm.BaseRecord.checkIncomplete(BaseRecord.java:1148)
    at com.goldencode.p2j.persist.Record._getInteger(Record.java:163)
    at com.goldencode.dataset.dmo.fwd.Book__Impl__.getBookId(Unknown Source)
    ... 43 more

The record buffer's snapshot attribute is assigned to an incomplete record, later is is being used in RAQ as placeholder.
Making sure placeholder is not incomplete, means fully hydrating the snapshot all the time since we don't know if the record will be used in an invalidated AQ, which negates the benefits of the incomplete records.

Considering that inRAQ the getters are invoked to collect arguments for the next query, would it be acceptable to skip the getters of the missing fields or do we need absolutely all the values at that point?

When invalidating an AdaptiveQuery, can't we force load the snapshot, if is incomplete?

Considering that inRAQ the getters are invoked to collect arguments for the next query, would it be acceptable to skip the getters of the missing fields or do we need absolutely all the values at that point?

We can't lose information, the query will not navigate properly if we don't have all fields.

I tried reloading the buffer when the AQ is invalidated, it solved the issue.

Alexandru,
I committed the solution to 11301c, please review.

11301c was merged to trunk in rev 16514 to solve an immediate issue.

See #11301-165 for a set of concerns that remain to be addressed (Alex's code review).

Alexandru Lungu wrote:

Review of trunk/16514:

• there is a potential NPE in collectIndexedFields. If PersistenceException is caught, sortCriteria will be null and sortCriteria.size() will yield NPE.

Yes, but if it fails at this point, it would have failed in FQLHelper also. The error handling could be improved, but the query just would fail earlier compared to trunk/16513.

• please double-test whether indexedFields.add(p.name); is the right thing to add here.

I did.

• Is it consistent with the include converted clause?

Yes.

• Does it have the same qualified state (tt.f1 vs f1)?

It is unqualified.

• Does it have the same syntax (book-name vs bookName)?

It has the converted names, same as the converted fields clause.

• We would also need to assess exclude; we can't risk excluding indexed field. I would imagine we will also need to diff exclude list with collectIndexedFields.

If the customer explicitly excluded the index fields, yes, the fix would not work and the error will be thrown same as in trunk/16513, we can work on that, granted I don't think this is a common pattern.

• please double-test CQ (e.g. FOR EACH a FIELDS(...), FIRST b FIELDS(...)) and other places where include might get invoked. I think we recently had a fix to pass included fields from CQ to AQ (#11298 and #7999). Maybe it is worth mentioning it there.

I did not explicitly write test cases with CQ, but I am sure the regression testing covered such scenarios. I will write the tests as part of addressing the issue this task was opened for.

If the customer explicitly excluded the index fields, yes, the fix would not work and the error will be thrown same as in trunk/16513, we can work on that, granted I don't think this is a common pattern.

May or not be common; but this can lead to a client crash. It is fatal. Please fix this.

Yes, but if it fails at this point, it would have failed in FQLHelper also. The error handling could be improved, but the query just would fail earlier compared to trunk/16513.

Please ensure we won't get a NPE, wherever the place to fix this.