Updates Not Recorded When Long Running Transactions Are Open

[kjs222]

First, thanks for making this available. My team has been using this versioning function for a while now.

Recently, we've noticed an issue with versioning in the following circumstances:

• high load
• an insert immediately followed by an update

In that scenario, on occasion, the update succeeds without triggering a write to the version table.

I am able to reproduce the issue without high load by initiating a long running transaction on an unrelated table before I execute the insert/update:

BEGIN;
update <some-other-table> ...;
select pg_sleep(100);
COMMIT;

The version is not written because the function will bail out on this block:

    -- Ignore rows already modified in this transaction
    transaction_info := txid_current_snapshot();
    IF OLD.xmin::text >= (txid_snapshot_xmin(transaction_info) % (2^32)::bigint)::text
    AND OLD.xmin::text <= (txid_snapshot_xmax(transaction_info) % (2^32)::bigint)::text THEN
      IF TG_OP = 'DELETE' THEN
        RETURN OLD;
      END IF;

      RETURN NEW;
    END IF;

The long-running transaction is making the snapshot's transaction range inclusive of the initial insert (e.g. OLD.xmin) even though it is already completed and therefore not present in the snapshot's xip_list. All subsequent transactions initiated and completed after the long-running transition initiated would also be inclusive in that snapshot range.

I don't have a full appreciation for the original intent of that block (beyond the comment itself), but for my use case, I think the condition could be updated to check whether the transition id of the OLD record is the same as the current transaction. Something like:

    -- Ignore rows already modified in this transaction
    transaction_info := txid_current();
    IF OLD.xmin::text = (transaction_info % (2^32)::bigint)::text THEN
      IF TG_OP = 'DELETE' THEN
        RETURN OLD;
      END IF;

      RETURN NEW;
    END IF;

Another possibility may be txid_visible_in_snapshot(bigint, txid_snapshot) - although I wonder if that may also be too greedy. The fact that the transaction id of the OLD record is still visible, may not, in and of itself suggest that it is inappropriate to also write a version on this transaction (e.g. I may be updating it from a different transaction id).

Thanks again!

[RicardoMurad]

Hi @paolochiodi First thanks again for making it available!

I reproduced the scenario described above by @kjs222 and fall into another issue.
When we have many parallel updates in the same row some of them are discarded by the block:

    transaction_info := txid_current_snapshot();
    IF OLD.xmin::text >= (txid_snapshot_xmin(transaction_info) % (2^32)::bigint)::text
    AND OLD.xmin::text <= (txid_snapshot_xmax(transaction_info) % (2^32)::bigint)::text THEN
      IF TG_OP = 'DELETE' THEN
        RETURN OLD;
      END IF;

      RETURN NEW;
    END IF;

Eg:

INSERT INTO subscriptions (name, state) VALUES ('test_21', '0')

and

UPDATE subscriptions SET state = '1' WHERE name = 'test_21';
UPDATE subscriptions SET state = '2' WHERE name = 'test_21';
UPDATE subscriptions SET state = '3' WHERE name = 'test_21';
UPDATE subscriptions SET state = '4' WHERE name = 'test_21';
UPDATE subscriptions SET state = '5' WHERE name = 'test_21';
UPDATE subscriptions SET state = '6' WHERE name = 'test_21';

The subscriptions table:

|name   |state|sys_period                        |
|-------|-----|----------------------------------|
|test_21|6    |["2021-09-07 18:15:30.686998-03",)|

The subscriptions_history table:

|name   |state|sys_period                                                       |
|-------|-----|-----------------------------------------------------------------|
|test_21|0    |["2021-09-07 18:15:26.163734-03","2021-09-07 18:15:30.686998-03")|

But If I try to run the updates series without parallelism the history table is filled correctly with 6 rows. I'm not sure the reasons this validation was added but for my scenários seems ok to have one history per transaction.

Could you clarify this point and we can send a PR for the project.

many thanks!

[sjurba]

Having the same issue with deletions. The best solution for us is to remove the entire block at it for us is an unnecessary optimization. The only effect is that if you modify the same row multiple times in the same transaction, you will get on entry pr. modification in the history. We never do this in our code, and even if we did, we would not care. IMHO this block should be removed from the script unless it is fixed, as loosing history is much worse than having too much history, especially when it is so unpredictable as it is now.

[paolochiodi]

Sorry for taking so long, but I was finally able to wrap my head around this issue - and I think I have some updates.

tl;dr

1. @kjs222 is right: check is too eager, should only check for current transaction
2. Check is staying in place (after the fix)

This extension was created as a drop-in replacement for an existing native extension: https://github.com/arkhipov/temporal_tables

That part of code preventing multiple history items in the same transaction has been there in the original extension since initial commit, and extended to deletes (and slightly refactored) with this commit arkhipov/temporal_tables@b563fb3

The change is intentional, as tests for the case have also been added.

Given the above, I'm inclined to keep the check to avoid inserting multiple history rows in the same transaction.

However, I see that piece of code is bugged - because it not only prevent multiple history rows in the same transaction, it prevents history rows from other transactions still running.
I'll fix that in a PR.

[paolochiodi]

I have implemented the fix originally suggested by @kjs222 in #29

[simoneb]

@kjs222 did you have a chance to look into this?

[github-actions]

🎉 This issue has been resolved in version 0.4.2 🎉

The release is available on:

• GitHub release

Your optic bot 📦🚀