Fix tuple concurrently deleted error with multiple continuous aggregates

[gayyappan]

When we have multiple continuous aggregates defined on
the same hypertable, they could try to delete the hypertable
invalidation logs concurrently. Resolve this by serializing
invalidation log deletes by hypertable id.

Fixes #1940

[codecov]

Codecov Report

Merging #2051 into master will increase coverage by 0.12%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##           master    #2051      +/-   ##
==========================================
+ Coverage   90.08%   90.20%   +0.12%     
==========================================
  Files         211      211              
  Lines       33679    33644      -35     
==========================================
+ Hits        30340    30349       +9     
+ Misses       3339     3295      -44     

Impacted Files	Coverage Δ
tsl/src/continuous_aggs/materialize.c	92.90% <86.66%> (-0.18%)	⬇️
src/loader/bgw_message_queue.c	84.51% <0.00%> (-2.59%)	⬇️
src/import/planner.c	70.30% <0.00%> (+11.12%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d649d49...1b22ac8. Read the comment docs.

[JLockerman]

where do we take a session lock for the materialization_invalidation_log? The only ones I see we take, and the only ones mentioned in the comment on line 156 are: the materialization table, the partial view and the raw hypertable...

[gayyappan]

fixed the comment. It should be session lock for materialization table.

[erimatnor]

What is a session lock? I guess it is confusing because it sounds like the lock is held for the duration of the session. Is it even possible to hold locks across multiple transactions unless it is an advisory lock? The documentation says: "Once acquired, a lock is normally held until the end of the transaction."

[gayyappan]

the session lock is for the duration of the materialization - spans the transactions.

[cevian]

Looks ok as long as the catalog scan does not release the row lock

[cevian]

is the row lock also released at by catalog scanning code, or no?

[gayyappan]

tuple locks are released at the end of the txn.

[erimatnor]

I didn't understand the following sentence in the commit message: "Resolve this be serializing
invalidation log deletes by hypertable id." You might want to take a look at that.

Also think we should always lock instead of conditionally based on whether there is more than one continuous aggregate. I imagine a second aggregate can be concurrently created and refreshed, which means we end up with the same situation although this is not visible to the first job.

You should also take a look at how you are handling locking on the invalidation threshold table. I believe there is an issue there since you are keeping the row-exclusive lock on the table until the end of the TX (in addition to the tuple lock). This seems to negate the purpose of the tuple lock and is much more heavy-handed.

[erimatnor]

Not sure we should throw a generic error for all lock results here. Let's say we get TM_Updated (the row was updated by other tx)---this doesn't seem unlikely and is not necessarily "unexpected". Perhaps we should have a less jarring error for that case at least?

[gayyappan]

Anything other than TM_Ok is not valid here. Knowing the specific failure doesn't seem to be helpful since we have to abort the transaction anyway.

[erimatnor]

But the user might see this error and an update doesn't seem like it would be "unexpected". Maybe not common, but not unexpected either.

[erimatnor]

mixed declaration and code. I suggest you move this above the ScanKeyInit. It should be safe to initialize the scankey after this declaration even if the scankey is referenced by the scanctx.

[erimatnor]

I suggest you check the return value here (num tuples found) and raise an error in case of not 1.

[erimatnor]

Isn't it more safe to always lock here? Could there be a case when cagg_2 is created in another transaction, but not yet visible to this transaction? If cagg_2 is immediately refreshed, I imagine that you won't lock in that case, although you still have concurrent refreshes.

[erimatnor]

I guess we could in theory handle this without locking. A concurrently deleted tuple might not strictly be an error since it doesn't really matter which job deleted the tuple. If the job that experience the error aimed to delete the tuple itself, then I guess it is fine for that job to proceed as if it actually succeeded.

This can, of course, be left as a future optimization. But I still figured it is worth pointing this out. Maybe leave a comment.

[erimatnor]

I don't understand why we keep the table lock here (in RowExclusiveLock) mode. That seems to negate the purpose of taking the tuple lock. I guess RowExclusiveLock doesn't conflict with itself, but does it block other stuff that we shouldn't block?

Or, am I missing something here?

[gayyappan]

changed to AccessShare based on our discssion.

[erimatnor]

Lock still held here. I thought that the table lock was going to be released here since tuple lock is held and that is the actual serialization point.

[gayyappan]

I misunderstood that - modified so that we only have the tuple lock.

[erimatnor]

Why is this lock RowExclusiveLock? Are we modifying this table in addition to taking the tuple lock? I suggest adding a comment on why this is the appropriate lock mode since it is non-obvious.

[k-rus]

I am not sure it is safe to not lock with one cagg. Also will the test be deterministic?

[k-rus]

Suggested change

	* Prevent this by serializing on the raw hypertable row
	* Prevent this by serializing on the raw hypertable row.

[k-rus]

Suggested change

	/* lock row corresponding to hypertable id in
	/* Lock row corresponding to hypertable id in

[k-rus]

I don't follow the comment. Specifically, block what?

[k-rus]

Is it?:

Suggested change

	* block till lock is acquired.
	* block continuous_aggs_invalidation_threshold table till lock is acquired.

[gayyappan]

The process is waiting on the lock till it is acquired.

[k-rus]

Thank you for clarifying. I suggest to fix the grammar, so it is easier to read later. I guess comma or and is missing.

[k-rus]

Do I understand correctly that the lock is only obtained when there are more than one cagg is defined, so there is a risk of concurrency? What's about a corner case when second cagg is created after materialising the first one passed this line? Can it be a race condition if meterializing the first one is slow or interrupted?
Or is this condition to deal with a deadlock?

[gayyappan]

@k-rus @erimatnor Yes, the corner case is possible. But not fully convinced that we should deal with the error for the corner case. The locks have a concurrency cost. So acquiring a lock only if it is necessary to do so seems to be a good trade off to me. ( There is an additional optimization possible here : a first time cagg does not have to deal with any invalidations since it has to fully materialize all the data. But we don't optimize for that case. Then this code might actually not be exercised in this scenario).

[erimatnor]

Do we know how big this cost is and do we throw a reasonable error when we hit the corner case? If not, I think we should cover the corner-case too and care about "correctness" prior to optimizing. We can always optimize further at a later time.

[k-rus]

I would like to add: which PG processes will be affected by this cost? Will insertion to hypertable or querying hypertable be affected by this concurrency overhead?

[k-rus]

Since they are complete sentences:

Suggested change

	/* acquire lock explicitly.
	* the catalog scanning code , releases it at end of the call
	/* Acquire lock explicitly.
	* The catalog scanning code , releases it at end of the call.

[k-rus]

Will it be more efficient, but still correct to call catalog_get_table_id(catalog, CONTINUOUS_AGGS_INVALIDATION_THRESHOLD) once, store in a variable, and use in both places, here and below?

[k-rus]

This declaration is placed after calling ScanKeyInit. Would it be better to do all declarations before the call?

[gayyappan]

IIRC we said that is okay to mix declarations and code. We are on C99.

[erimatnor]

More of a suggestion. But FWIW, we haven't used mixed-declaration-and-code in most of our other code. Not the appropriate place to discuss decisions. But this is suggested by two reviewers and seems like an easy fix.

[gayyappan]

I think the struct initialization is more readable here. scanctx depends on scankey. That's why there is mixed declaration in this block

[k-rus]

get_lowest_invalidated_time_for_hypertable contains similar code with variable declaration first:

	ScanKeyData scankey[1];
	ScannerCtx scanctx;

	ScanKeyInit(&scankey[0],
				Anum_continuous_aggs_invalidation_threshold_pkey_hypertable_id,
				BTEqualStrategyNumber,
				F_INT4EQ,
				Int32GetDatum(ts_hypertable_relid_to_id(hypertable_relid)));
	scanctx = (ScannerCtx){
		.table = catalog_get_table_id(catalog, CONTINUOUS_AGGS_INVALIDATION_THRESHOLD),
		.index = catalog_get_index(catalog,
								   CONTINUOUS_AGGS_INVALIDATION_THRESHOLD,
								   CONTINUOUS_AGGS_INVALIDATION_THRESHOLD_PKEY),
		.nkeys = 1,
		.scankey = scankey,
		.tuple_found = &invalidation_tuple_found,
		.filter = NULL,
		.data = &min_val,
		.lockmode = AccessShareLock,
		.scandirection = ForwardScanDirection,
		.result_mctx = NULL,
	};

I don't have strong opinion, but there might be a test, which will fail declaration after code. However, I might be mistaking.

[gayyappan]

Thanks for the pointer.

[k-rus]

I don't follow why a lock needs to be obtained if the entire table is locked until the end of the transaction.

[k-rus]

I think I understand now that the tuple is locked for the session, which is longer than the transaction, right? Where is the lock on the tuple released?

[gayyappan]

tuple locks are released at the end of the transaction.

[k-rus]

So is it really necessary to obtain the tuple lock if the table is already locked for the same duration?

[gayyappan]

Yes, since a concurrent transaction will not conflict on the table lock. The tuple lock helps serialize the transactions. This begs the question is the tuple lock sufficient? : I believe that you need a table lock before you acquire the tuple lock. (I am going to change the table lock from RowExclusive to AccessShare)

[k-rus]

Is it deterministic that Refresh1 will be executed first after the lock is released and then Refresh2?

[gayyappan]

No, cannot be guaranteed. But if Refresh1 starts before Refresh2, it would acquire the released lock first. The sessions are generally started in the order in which they appear in the spec file.

[k-rus]

Does this test reproduce the bug before the fix?

[gayyappan]

All this test proves is the refresh gets serialized on the hypertable row lock. This is true for all the concurrency tests. They do not necessarily "catch" all bugs with the code.

[k-rus]

@gayyappan Is this PR related to any public issue?

[erimatnor]

Approving, but I suggest you go through the nits and try to address them.

[erimatnor]

Using the function name in error messages is redundant as elog already tracks this and the function and line number can be shown if the right log configuration is set. I think it is better to formulate a good error message to describe the error, e.g.: "duplicate catalog entry for hypertable "%s"". As a last resort, use the __func__ macro as that also handles function name changes.

[erimatnor]

Is anything but 1 expected here? Isn't 0 (or less) also errors?

[gayyappan]

could be 0 if there are no entries in the table yet for that hypertable.

[erimatnor]

Lock still held here. I thought that the table lock was going to be released here since tuple lock is held and that is the actual serialization point.