FWD » Core Development » Database

Eric Faulhaber wrote:

We also need to review edit 3821c/12004, which changed the index selection portion of query conversion to ignore word indices. I am concerned this change may have had unwanted side effects to downstream conversion, besides removing the unwanted effect on the ORDER BY clause of the target query.

Actuallу, after recent changes regarding sorting restoring the logic with word indices selection, should be easy. Moreover, it can make the logic for deciding if the word table should be selected for implicit sorting more consistent.

The exploration and possible use of database native support to implement CONTAINS support is considered part of this effort, unless other improvements can be devised to bring the performance of the current implementation more in line with that of the 4GL's implementation.

I think we should test native full text search now, to see if it can eliminate the performance bottlenecks (e.g. #8454) we are finding in our implementation (SQL UDFs + special tables for each index). Is the current implementation well encapsulated so that a prototype could be "plugged in"?

For FTS support, we have to_tsvector in PostgreSQL to get the 'words'; does this split into words in the same way as OpenEdge?

We need to ensure we have broad coverage of how OpenEdge splits text into words; we need to look into our UDF's implementation into words, any existing testcases, and expand them as needed.

Stefanel, please see the above.

I don't think PostgreSQL to_tsvector splits words in the same way as OE does. But I'm thinking if we can use array_to_tsvector instead. So we have our custom word array that is output of our words UDF, that we save in a tsvector column.

Like:

SELECT array_to_tsvector(
    ARRAY['Johns', 'ab', 'cd', '12.34', 'abc_def', 'user@example', 'com', 'x']
) as word_index;

word_index (tsvector)
----------
'12.34' 'Johns' 'ab' 'abc_def' 'cd' 'com' 'user@example' 'x'

word_index @@ to_tsquery('english', '(ab & c) | x')

• "english" - will use an English dictionary to split the target string into lexemes. These are 'root' forms of a word (no plurals, no conjugation/declination) and the not-interesting words (like prepositions and articles) are dropped. This is NOT what we want to use.
• "simple" - the target text is converted to lowercase and stripped of stop words, but it does not undergo stemming. It is not splitting the text strictly by spaces, but uses Postgres's standard built-in tokenization engine, so we may not get the exact output to match 4GL's. We need to adjust it a bit. And we can do this by replacing the problematic characters from source text by the spaces (' '), which are safe.

I have created 5219a from trunk/16671 and committed in revision 16672 the first version of CONTAINS implementation using PostgreSQL tsvector columns and GIN indexes.

These are exciting results! But please help me understand what 5219a (legacy) represents (compared to trunk). Thanks.

Eric Faulhaber wrote:

These are exciting results! But please help me understand what 5219a (legacy) represents (compared to trunk). Thanks.

Ok, so legacy means the usual word tables like how are they in trunk but with some optimizations made on how the SQL query is built in FqlToSqlConverter.

Stefanel Pezamosca wrote:

[...]
The changes also improves the performance further for the legacy word-table implementation by:

• Removing the obsolete relevance-weight aggregation and implicit CONTAINS result ordering code, because this never worked right and just added useless complexity. The legacy text word-table SQL is also simplified to query word tables directly without redundant joins to the parent table.

How does our results-sorting behavior for CONTAINS queries compare to the 4GL behavior, currently with trunk and now with 5219a? I suspect we probably need additional unit tests in this area.

Eric Faulhaber wrote:

Stefanel Pezamosca wrote:

[...]
The changes also improves the performance further for the legacy word-table implementation by:

• Removing the obsolete relevance-weight aggregation and implicit CONTAINS result ordering code, because this never worked right and just added useless complexity. The legacy text word-table SQL is also simplified to query word tables directly without redundant joins to the parent table.

How does our results-sorting behavior for CONTAINS queries compare to the 4GL behavior with trunk and now with 5219a? I suspect we probably need additional unit tests in this area.

I need to do more testing on the sorting behavior, but based on an initial online search, word indexes do not appear to follow any defined order.
Also, even if the sorting is influenced by CONTAINS in some way, the resulting order seems fairly random and unintuitive. In most cases, I expect it to be overridden by explicit ORDER BY clauses anyway.

enabled only by a system property

Please never use properties for FWD configuration. Server cfg should be in the directory.

Ok. I had the impression from your comment about what was removed that we were previously attempting to implement some implicit, relevance-based sorting.

Greg Shah wrote:

enabled only by a system property

Please never use properties for FWD configuration. Server cfg should be in the directory.

Ok, I copied how useCTE4Contains or others are set in PosgreSQLDialect. So, I need a configuration option to enable tsvector usage both for conversion and runtime.

Stefanel Pezamosca wrote:

Greg Shah wrote:

enabled only by a system property

Please never use properties for FWD configuration. Server cfg should be in the directory.

Ok, I copied how useCTE4Contains or others are set in PosgreSQLDialect. So, I need a configuration option to enable tsvector usage both for conversion and runtime.

Which never should have been implemented but wasn't caught at the time.

Eric Faulhaber wrote:

Ok. I had the impression from your comment about what was removed that we were previously attempting to implement some implicit, relevance-based sorting.

I believe this was the intended behavior, but it does not appear to work as expected and differs from the 4GL implementation. I’ll run additional tests to better understand the behavior. That said, I’m concerned that replicating a sorting approach without a clear or predictable sort order may not be very user-friendly.

I have been experimenting with creating the tsvector column directly in the base table. I also moved most of the PostgreSQL-specific implementation from FqlToSqlConverter to the dialect class, and replaced P2JPostgreSQLDialect.useTsvector4Contains with a property named tsvector-contains that can be read from p2j.cfg.xml and directory.xml. I guess I could add a new parameter to the Table or Column Parameter during conversion to preserve the tsvector-contains state?

Adding this to schema_table_* after the create table statement creates the new tsvector column with the tsvector__ prefix:

alter table words
   add column if not exists tsvector__words tsvector
   generated always as (udf.words_to_tsvector(coalesce(words, ''), true)) stored;

The udf.words_to_tsvector() function uses our custom udf.words function. For this to work, we need to make sure the UDFs are updated before schema_table_* is run during the import process:

CREATE OR REPLACE FUNCTION udf.words_to_tsvector(txt text, toUpperCase boolean)
RETURNS tsvector
LANGUAGE sql
IMMUTABLE STRICT PARALLEL SAFE
AS $function$
   SELECT array_to_tsvector(udf.words(txt, toUpperCase, toUpperCase));
$function$;

I also added this index to the schema_index_* DDL:

create index if not exists idx__tsvector__words__words on words using gin (tsvector__words);

With this FQL:

select words.recid, words.words from Words__Impl__ as words where (contains(words.words, ?0))   order by words.recno asc, words.recid asc;

We can generate this simpler SQL regardless of the contains expression:

select words__imp0_.recid, words__imp0_.words as col0_0_ from words words__imp0_ where (words__imp0_.tsvector__words @@ cast(upper(?) as tsquery)) order by words__imp0_.recno asc, words__imp0_.recid asc limit ? offset ?

These are some cold performance results. Compared with #5219-13, the results are better. After the Bitmap Index Scan, PostgreSQL uses a Bitmap Heap Scan that takes most of the time.

The bad performace above is because of how FWD handles the query. It's selecting a list of RECIDs first and then iterates each RECID to get the actual data.

I got some progress working on making some testcases to test 5219a.
I have a pipeline working where the tests are written, copied to a machine with OE installed, tested with OE both simple procedures and ABLUnit. If any error occurs, they are fixed and rerun, then copied to a template fwd project, compiled and tested.
Finally, the result are compared between OE and FWD and if there are differences I can start a multi phase process to try and fix each difference one at a time.
The current test-suite I have, helped me find and fix some issues like client crashes and wrongly handled syntax errors. I'm trying to aggregate more cases.
I didn't run the tests using trunk yet, but with 5219a all of them passed exactly like in OE. I also have to test other projects because there are some changes on how CONTAINS syntax errors gets handled, and they might be risky, so I'm not sure if I want to really keep them.

To the degree that we have a bunch of native Java test code for this (I recall that may be how Igor wrote them), each scenario should be re-implemented as a 4GL ABLUnit test. Those can be used along with the new tests you are writing, which should also be ABLUnit.

Please use bzr-issues (available in the latest gcd_systems) to identify additional Redmine tasks that have made changes in this area. For each of those, extract testcases and turn them into ABLUnit tests.

Should I separate each specific case into separate methods? (Like: naturalOrderFollowsTheLeadingGroup01, naturalOrderFollowsTheLeadingGroup02, etc?)

This is indeed what we prefer. Can it be done relatively quickly? If so, please do it.

Greg Shah wrote:

Should I separate each specific case into separate methods? (Like: naturalOrderFollowsTheLeadingGroup01, naturalOrderFollowsTheLeadingGroup02, etc?)

This is indeed what we prefer. Can it be done relatively quickly? If so, please do it.

I have done it, there are 1429 tests and 26 failures with 5219a now.

Stefanel Pezamosca wrote:

Aside from the failing test cases related to natural ordering without an explicit BY clause, which I don’t expect to matter in most scenarios, since custom sorting is usually selected based on an index, the changes in 5219a seem quite stable.

That said, 5219a includes a fairly large set of changes to improve how we handle dynamic query parsing and error handling especially for CONTAINS. If this feels too risky or not appropriate to commit as is, I can extract only the critical functional fixes and move the remaining changes into 5219b.

Please review 5219a. I’ll continue testing to make sure the changes remain stable.

To enable CONTAINS conversion with FTS (tsvector) this is needed in p2j.cfg.xml:
[...]

Is there a migration script for existing databases? Or is running word_reindex via ImportWorker enough?

Constantin Asofiei wrote:

Please review 5219a. I’ll continue testing to make sure the changes remain stable.

To enable CONTAINS conversion with FTS (tsvector) this is needed in p2j.cfg.xml:
[...]

Is there a migration script for existing databases? Or is running word_reindex via ImportWorker enough?

word_reindex is only used when word_tables are used. If fts4contains is false then no migration is needed, 5219a is compatible with current database structure from trunk.

If fts4contains is true. Then schema_word_tables_ are not generating anymore. Everything will be generated in schema_table_* and schema_index_*. At the moment the only migration stepts will be to update UDFs (for the new udf.words_to_tsvector used, see #5219-24). After the UDF update we need to run all ALTER TABLE statements generated in schema_table_* and the GIN index definitions from schema_index_*.

Stefanel,

I did a review of 5219a. The update is OK. I have nothing to comment. It is actually very good. Thumbs up!

At the same time Greg asked me last week to do a brainstorm and compose some edge-case test-cases to have an strong baseline for possible future changes. Now I have 100+ hand-crafted tests. I encountered some issues which I have fixed (actually errors, in TRPL, to be handled via CompileException). However, I was able to create some testcases which fail to convert statically (the code is not valid in Java) and in dynamic mode it fails at an unexpected level (the JAST tree gets to be processed by the RuntimeInterpreter, but the problem should have been detected earlier, at dynamic conversion, when the specific JAST was constructed).

Ovidiu Maxiniuc wrote:

At the same time Greg asked me last week to do a brainstorm and compose some edge-case test-cases to have an strong baseline for possible future changes. Now I have 100+ hand-crafted tests. I encountered some issues which I have fixed (actually errors, in TRPL, to be handled via CompileException). However, I was able to create some testcases which fail to convert statically (the code is not valid in Java) and in dynamic mode it fails at an unexpected level (the JAST tree gets to be processed by the RuntimeInterpreter, but the problem should have been detected earlier, at dynamic conversion, when the specific JAST was constructed).

Thank you for the review.
Are these issues a regression of 5219a or they are also in trunk, or something else ?

Here is the final results after running some local performance testcases. There are 2 tables used with a milion records each. There is some error margin of few ms, but overall the performance gain is still noticeable for some cases.

I think the big times reported in this second table are because the test uses a FOR EACH without NO LOCK.

I tested also 2 projects with 5219a in word table mode and I also asked Razvan and Artur to test their apps. Didn't get any issues at the moment.
I will add some change to ImportWorker word_reindex target to work with FTS/tsvector implementation.

Stefanel, please get a table with an word index (with extent field also) and post the DDL (and associated triggers, etc) with trunk and 5219a.

What is the reasoning behind making the new approach optional/conditional? If the changes don't cause regressions and are uniformly better, then we want this to be the ONLY word index approach.

At the moment the only migration stepts will be to update UDFs (for the new udf.words_to_tsvector used, see #5219-24). After the UDF update we need to run all ALTER TABLE statements generated in schema_table_* and the GIN index definitions from schema_index_*.

We need a simple process for this. If the customer has to implement this themselves, it will be potentially error prone and will cause issues that will take time to figure out. Instead, we need our own migration process to be available as part of the change.

Greg Shah wrote:

What is the reasoning behind making the new approach optional/conditional? If the changes don't cause regressions and are uniformly better, then we want this to be the ONLY word index approach.

I think to help with backward compatibility and migration until all the clients are using this mode? This can be easily changed to activate by default for postgresql databases if wanted.

We need a simple process for this. If the customer has to implement this themselves, it will be potentially error prone and will cause issues that will take time to figure out. Instead, we need our own migration process to be available as part of the change.

Working on it.

I think to help with backward compatibility and migration until all the clients are using this mode? This can be easily changed to activate by default for postgresql databases if wanted.

OK, we will have to track the customers and when each has migrated then we will eliminate this flag.

How does this get configured at runtime?

Greg Shah wrote:

How does this get configured at runtime?

Runtime can also read from cfg/p2j.cfg.xml that is saved in project.jar, so the same flag.

No, we don't want to do that. The flag for runtime needs to be in the directory.

Greg Shah wrote:

No, we don't want to do that. The flag for runtime needs to be in the directory.

Greg, I don't think we should do this in directory, p2j.cfg.xml and directory will need to be in sync - as the DDL for word tables and triggers and such will be different. We can't switch runtime from one approach to the other.

OK, but I dislike this intensely. The use of p2j.cfg.xml at runtime is something we never should have had in the first place. I intend to murder it sooner rather than later. This dependency now make my assassination job harder.