Add recompression spin lock #8018
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8018 +/- ##
==========================================
+ Coverage 80.06% 82.14% +2.07%
==========================================
Files 190 253 +63
Lines 37181 47209 +10028
Branches 9450 11894 +2444
==========================================
+ Hits 29770 38780 +9010
- Misses 2997 3724 +727
- Partials 4414 4705 +291 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tsl/src/compression/compression.c
Outdated
| break; | ||
| } | ||
|
|
||
| (void) WaitLatch(MyLatch, |
There was a problem hiding this comment.
Just for my understanding, why do we manually build a spinlock here? One reason I can imagine is that we want a predictable timeout and WaitOnLock doesn't have an explicit interface for that. I am wondering whether we could change the LockTimeout instead. This would give us better progress because we'd be on the wait queue, and also interrupt the interfering autovacuum tasks.
There was a problem hiding this comment.
There are two main goals with this PR: 1. Introduce a timeout so that compress_chunk doesn't block indefinitely. 2. Increase concurrency of DML operations in the uncompressed chunk.
I think we should avoid getting into the wait queue since that would block other DML operations.
There was a problem hiding this comment.
I think we should avoid getting into the wait queue since that would block other DML operations.
Isn't the purpose of the spin lock to block other dml operations?
There was a problem hiding this comment.
If we're in the wait queue, we'll block any new DML operations, which would reduce concurrency.
Since we use a spin lock, we won't do this.
kpan2034
force-pushed
the
add-recompression-spin-lock
branch
4 times, most recently
from
1d20981 to
5866acb
Compare
kpan2034
force-pushed
the
add-recompression-spin-lock
branch
from
5866acb to
193afc3
Compare
|
Removed the GUC based on discussion with @antekresic. There is already a GUC in place to require heavy locks during recompression, that can be used to avoid the spin-lock - |
Recompression tries to take an ExclusiveLock at the end of compression in order to update the chunk status from partial to fully compressed. This is mandatory when unique constraints are present in the table. Otherwise recompression is aborted. This adds a spin-lock when acquiring an ExclusiveLock only in the case of unique constraints being present on the table.
kpan2034
force-pushed
the
add-recompression-spin-lock
branch
from
193afc3 to
a43ce3e
Compare
kpan2034
force-pushed
the
add-recompression-spin-lock
branch
from
546a0fe to
ec3c134
Compare
|
Not sure why the code coverage check is still failing. The line should definitely be hit since a |
kpan2034
force-pushed
the
add-recompression-spin-lock
branch
from
5698591 to
2abcdb3
Compare
| if (ConditionalLockRelation(uncompressed_chunk_rel, ExclusiveLock)) | ||
| { | ||
| try_updating_chunk_status(uncompressed_chunk, uncompressed_chunk_rel); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Wondering if this should be after the waitlatch since you will enter in this branch after failure on the first ConditionalLockRelation. Does it make sense?
There was a problem hiding this comment.
I did it this way because I wanted to keep the structure consistent with how Postgres implements a similar spin lock.
Recompression tries to take an ExclusiveLock at the end of
compression in order to update the chunk status from partial to fully
compressed. This is mandatory when unique constraints are present in
the table. Otherwise recompression is aborted.
This adds a spin-lock during the ExclusiveLock only in the case of
unique constraints being present on the table.
This functionality isbehind a GUC to allow users to maintain old behaviour.