Ensure _revision_expired is always > _revision_created

[strk]

Closes #78

[strk]

@imincik do you want to give this change a try and see how much does it affect performances ?

[imincik]

@strk , I would be interested if there are any other similar checks we can introduce.

[imincik]

I added @dwsilk to reviewers. I can test it in BDE Processor for performance penalty, but I am not sure if it will cover all use cases.

[strk]

@imincik yes, I think we could also introduce a foreign key constraint to make sure revisions used in _revision_created and _revision_expired DO exist in table_version.revision, but I wanted to first verify the performance change with just this single constraint

[dwsilk]

Okay so this should really only affect INSERT and UPDATE on the _revision tables, right?

Basic test to compare performance over 10,000,000 records with bare minimum data:

BEGIN; 

CREATE TABLE foo(id integer, _revision_created timestamp, _revision_expired timestamp);
CREATE TABLE bar(id integer, _revision_created timestamp, _revision_expired timestamp, CHECK (_revision_created < _revision_expired));

\timing on
INSERT INTO foo(id, _revision_created, _revision_expired) SELECT generate_series(1, 10000000), '2018-05-01', NULL;
INSERT INTO bar(id, _revision_created, _revision_expired) SELECT generate_series(1, 10000000), '2018-05-01', NULL;
UPDATE foo SET _revision_expired = '2018-05-05';
UPDATE bar SET _revision_expired = '2018-05-05';
\timing off

ROLLBACK;

Result:

Command	Without CHECK	With CHECK	Performance Hit
INSERT 10,000,000 records	13430.308 ms	14061.644 ms	4.7% longer
UPDATE 10,000,000 records	30311.315 ms	30843.735 ms	1.8% longer

I would guess that BDE processor performance is the most important application that we can test. The amount of data in queries for other applications of this extension at LINZ is minimal in comparison.

[strk]

Yes it should only affect INSERT/UPDATE (while foreign key constraint would also affect DELETE on the referenced table) - those hits seem ok to me, especially as for BDE processing the inserts and updates would be MORE expensive because there would be more data to write, while the cost of comparing _revision_expired and _revision_created would be fixed...

[strk]

Thanks for the review, see how you like 721ba11

[strk]

Thanks for approval. What this PR does NOT include is an easy (automatic?) way to add the check to existing revision tables. Tables with corrupted records would fail to get the addition of such check so it may be useful to have a callable function to try at adding missing checks, raising a WARNING wherever such addition fails.

[imincik]

I will test this PR with level_5 update. @strk , please wait with merge until this is done.

[imincik]

I just started performance test of this patch (running level_0 without patch). Might be done in 4 days from now.

[imincik]

@strk , level 0 processing using latest released packages, without tested patch, finished with following error:

COMMENT
COMMENT
SET
SET
INFO:  Patch BDE - 1.0.2: Remove annotations column from bde.crs_work is already applied
INFO:  Patch BDE 1.2.0 / LOL 3.17: Remove tan column from bde.crs_transact_type is already applied
INFO:  Patch BDE 1.2.0 / LOL 3.17: Add img_id and description columns to bde.crs_stat_act_parcl is already applied
DO
SET
ERROR:  Error versioning bde.cbe_title_parcel_association. ERROR: permission denied for relation pg_depend
CONTEXT:  PL/pgSQL function inline_code_block line 50 at RAISE
ERROR: Failure enabling table versioning in database bde

Ticket created: https://app.zenhub.com/workspace/o/linz/bde-processor-deployment/issues/307

[strk]

Make sure the user who prepares the database (with table_version 1.5.0+) or the user who runs the upload (with table_version up to 1.4.x) is a superuser, or has permissions to write into pg_depend.

Example query: SELECT has_table_privilege('bde', 'pg_catalog.pg_depend', 'INSERT');

[stale]

This issue has been automatically marked as stale as there has not been any activity for sometime. The issue will be closed in 14 days if no further activity.

[imincik]

This issue is still relevant.

[stale]

This issue has been automatically marked as stale as there has not been any activity for sometime. The issue will be closed in 14 days if no further activity.

[strk]

It took 35147012.330 ms (9.7630 hours) to run LDS_MaintainSimplifiedLayers(0) with lds and bde_ext tables versioned and NO integrity check. I'm not running it again with the integrity check.

[imincik]

Where are you running those tests ? LI-exp ? Looks too long, maybe somebody was running something else there as well.

[strk]

Completed same run (yes, on li-exp) with this PR applied. Took 42038905.988 ms (~11.6 hrs)

[imincik]

Hm, look very slow ...

[strk]

We could try an INITIALLY DEFERRED constraint to see how performance changes, if at all

[strk]

Forget the last comment, it'll probably be slower to defer the check.
This PR just needs an additional commit to add the check to the tales that were created before the upgrade

[strk]

I've pushed a commit adding the constraint to already-versioned tables.
There's no handling for tables which have an expired time earlier than creation time, so in those case the upgrade would fail.

[l0b0]

Does this need an OR _revision_expired IS NULL check?

[l0b0]

See comment

[l0b0]

Closing because of no feedback. Please reopen if this can still be merged.