horizon: Modify tx submission system to work with RO database

[2opremio]

This PR modifies the ingestion system to append to the following table when a transaction is filtered out

-- used to temporarily store filtered-out transactions
-- needed by the transaction system
CREATE TABLE history_transactions_filtered_tmp (
    created_at timestamp NOT NULL DEFAULT NOW()
) INHERITS (history_transactions);

The submission system queries both history_transactions and history_transactions_filtered_tmp to obtain transaction submission results.

The ingestion system is in charge of garbage-collecting old history_transactions_filtered_tmp

This allows the transaction submission system to connect safely to a read-only database.

As a disadvantage, the ingestion system will always need to insert transaction entries into the DB. However, since the history_transactions_filtered table will be much smaller, this will hopefully take a small amount of time. Also, the DB queries from the transaction submission system will be longer since they now query two tables.

As an advantage, ingestion shouldn't be impacted when filtering is disabled.

WIP (still untested, but hopefully code-complete)

TODO:

• fix tests
• add new tests
• meassure ingestion times when filtering is enabled
• meassure transaction submission times

[sreuland]

I'm starting to review, @bartekn , @2opremio wanted to check on viability of alternate approach with attempt of more transparent change above the functional layer if possible. In tx sub system, allocate it's own new db session pool instance, and configure it from a new optional TXSUB_DATABASE_URL param, the default behavior would be if param is absent then use DATABASE_URL for net effect of current behavior which assumes DATABASE_URL is RO/RW, but for the advanced deployments with different worker roles, could configure DATABASE_URL=RO DB and TXSUB_DATABASE_URL=RW DB.

[bartekn]

LGTM. I'll check it more carefully after a discussion under my comments and tests. I wonder if we can create at least one test that ensures that txsub works correctly in readonly access to the DB. We could create a new user with readonly access to all tables and start two Horizon instances from integration tests.

[bartekn]

TransactionByHash method is also used in actions/transaction.go that implements /transactions/{id} endpoint and TransactionsByHashesSinceLedger is only used in txsub. Maybe instead of querying both tables we could create a duplicate method of TransactionByHash for txsub temp table and completely remove access to history_transactions in TransactionsByHashesSinceLedger. I think this will simplify queries a lot but most importantly it won't show filtered txs in /transactions/{id}.

[sreuland]

ok, what it we do that and refactor a bit, create txsub.TransactionsByHash and txsub. TransactionsByHashesSinceLedger to drive the point that these are specific to txsub concerns and ingest references these?

[2opremio]

@bartekn missed that transactionbyhash was used at two places. Are you suggesting that we make two separate query calls in the txsub system? If it doesn’t impact performance by much then it probably makes sense.

BTW, I refactored the query to remove the need of using a union when querying a single table. Do you recall why it was done that way?

[sreuland]

followed up on this suggestion with PreFilteredTransactionByHash and PreFilteredTransactionsByHashesSinceLedger

[bartekn]

I think there's one big disadvantage of inheriting: if we ever modify history_transactions we need to remember to update history_transactions_filtered_tmp. I wonder if the method used in txsub_results: serializing history.Transaction is actually better.

[sreuland]

by 'need to remember to update tmp table' do you mean for potential data coercion needed related to new schema changes? if new schema changes don't require post data coercion, then the tmp table should pick up new schema at same time as parent table. Do you think serializing tx's here also could evolve to a potential performance hit, if filtering is on then the majority of tx's end up getting dropped and traveling through this tmp table.

[sreuland]

@bartekn @2opremio , Bartek, you're on the right path, after running tests and getting failures on query results having more than they should, discovered that the parent table in INHERITS, still gets inserted the 'base' row into it, and then the child table gets a row also but just with any additional columns, so, duplicates were ending up in the history_transactions table after inserting into history_transactions_filtered_tmp

just for temporary, I did a work around of CREATE TABLE history_transactions_filtered_tmp AS SELECT FROM history_transactions.. here:
06f2759#diff-162d7edb094b93a37754b517c3f766bf4ff2cc5f329e3be6994bcaaa4b0d6c71R10

We need to decide if this is acceptable or look into revert history_transactions_filtered_tmp to have serialized history_transaction and any directly searchable columns.

[2opremio]

I think this is acceptable as long as we add a test that avoids we overlook updating both tables identically when there is a schema change (e.g. by checking that only a column is different).

[sreuland]

Ok, I updated the comment on migration sql for following this convention on the tmp table - 9feaac2 . Will look into include that as test coverage along with RO user context on tests.

[2opremio]

I added the schema test.

[bartekn]

allocate it's own new db session pool instance, and configure it from a new optional TXSUB_DATABASE_URL param, the default behavior would be if param is absent then use DATABASE_URL for net effect of current behavior which assumes DATABASE_URL is RO/RW, but for the advanced deployments with different worker roles, could configure DATABASE_URL=RO DB and TXSUB_DATABASE_URL=RW DB

I think it will complicate deployments because it creates another dimension in possible DB configuration matrix. Right now the rule is simple: ingesting is RW, non-ingesting is RO and it's easy to reason about. When we add proper integration tests (that run front-end code on RO connection) we will be able to prevent mistakes in the future.

[sreuland]

I wonder if we can create at least one test that ensures that txsub works correctly in readonly access to the DB. We could create a new user with readonly access to all tables and start two Horizon instances from integration tests.

@bartekn , I'll look into doing this from test, do you think it's best to try this with regular unit test framework or integration test framework?

[2opremio]

I wonder if we can create at least one test that ensures that txsub works correctly in readonly access to the DB

I would bite the bullet and change all tests so that endpoints are queried through a read-only user (e.g. action tests)

[sreuland]

I would bite the bullet and change all tests so that endpoints are queried through a read-only user (e.g. action tests)

I'll start looking at that after getting current tests to pass as-is, almost there with recent commit, 06f2759, getting one last issue across many tests of
could not run migrations up on test db: pq: relation "history_transactions_filtered_tmp" already exists handling 57_txsub_read_only.sql, a little strange, I don't get the error if I run individual tests by name.

[sreuland]

unit tests go test ./services/horizon/.... are passing now, working on integration tests, seeing some fails related to Timeouts on tx subs.

[sreuland]

resolved the remaining integration test txsub timeouts, tests should be passing now, ran locally and looked good.

[sreuland]

@2opremio @bartekn , the r/o db(user role with only select granted) is in place on tests, per the team discussion, have retargeted the PR at a pending 2.18.1 rel branch instead of master.

[bartekn]

Is WHERE ledger_sequence IS NULL there to ensure the result contains no rows?

[sreuland]

yes, it's declared NOT NULL in DDL, so should be effective for this intent.

[bartekn]

Maybe to make it future proof (though I have no idea why and if ledger_sequence will even be NULL) we could change it to WHERE 1=0? Nit, really.

[bartekn]

Not part of this PR but can we change it to Debug or remove it? It's possible that plenty of txs will be filtered and it will spam the log.

[sreuland]

yes, noticed that, done, 1488013

[bartekn]

Legitimate err (no NoRows) is ignored here.

[sreuland]

that was the intent of flow for this redundant search, anything other than 'found tx in filtering tmp table' should proceed to the next attempt to query for same tx in history table, after which it then inspects err for differentiation between not found and other error, is that approach sensible?

[bartekn]

I understand this. The problem is that if PreFilteredTransactionByHash returns a valid error it will be ignored. In other words err can be something different than ErrNoRows.

[sreuland]

oh yes, that is a nuance, I've trapped it and added explicit tests on that err trap also, thanks

9b156d1