Relax locking requirements for collecting table stats #455

msakrejda · 2023-09-13T19:08:11Z

Currently, if tables are locked in AccessExclusiveMode, we skip
collecting most table structure information and table and column
stats. We do this since the lock would conflict with the lock the
collector needs and potentially cause cascading locking problems and
impact the application. However, we're not checking the locktype, so
the collector also avoids collecting stats for tables with
AccessExclusiveMode locktype = 'tuple' locks, which do not actually
block collection. These can be fairly common, e.g., if a transaction
issues a SELECT FOR UPDATE query while another transaction has
already obtained a SELECT FOR UPDATE lock.

Relax the locking requirements by ignoring AccessExclusiveMode
locktype = 'tuple' locks when collecting schema structure and stats.

msakrejda

I tested this manually on all affected queries with an outstanding locktype = 'tuple' AccessExclusiveLock. It'd be nice to add this to our integration tests, but locking doesn't currently have coverage and I don't see a great way to add that. Thoughts?

msakrejda · 2023-09-13T19:09:30Z

input/postgres/relation_stats.go

@@ -14,7 +14,7 @@ const relationStatsSQLInsertsSinceVacuumFieldDefault string = "0"

 const relationStatsSQL = `
 WITH locked_relids AS (
-	SELECT DISTINCT relation relid FROM pg_catalog.pg_locks WHERE mode = 'AccessExclusiveLock' AND relation IS NOT NULL
+	SELECT DISTINCT relation relid FROM pg_catalog.pg_locks WHERE mode = 'AccessExclusiveLock' AND relation IS NOT NULL AND locktype <> 'tuple'


I'm not sure if locktype <> 'tuple' or locktype = 'relation' makes more sense. There are a lot of different locktypes so I decided to be more conservative, but I could be convinced.

Uhmm, looking at the code, only relation, extend, page, tuple locktype can have relation column for pg_locks (look at values[2] or nulls[2]):
https://github.com/postgres/postgres/blob/e0b2eed047df9045664da6f724cb42c10f8b12f0/src/backend/utils/adt/lockfuncs.c#L246-L354

Among this, feels like page is also somewhat similar to tuple, in terms of it won't be locking pg_table_size(). I'm not so sure about extend, but looking at the purpose of this lock (https://docs.aws.amazon.com/AmazonRDS/latest/AuroraUserGuide/apg-waits.lockextend.html), maybe it also won't cause problems with pg_table_size()?

If you agree, maybe using locktype = 'relation' makes more sense.

Nice, thanks for diving into the Postgres source to investigate this. I think given this, locktype = 'relation' looks reasonable. I think the only concern there is if a future version of Postgres adds a lock type similar to, but distinct from, locktype = 'relation', and we start trying to collect data when we shouldn't. Alternately, we could change this to locktype NOT IN ('extend', 'page', 'tuple') to be a little more future-proof, but that gets a little wordy. Any thoughts on this @lfittl?

Hmm, good question. I feel like this part of Postgres changes rarely (if at all), so I'm not too worried about future proofing, but it feels slightly safer to say "these are the locktypes that are not a problem, even when its an AccessExclusiveLock", i.e. I think locktype NOT IN ('extend', 'page', 'tuple') might indeed be better, even if its wordy.

That said, is there a way for us to verify that its safe to ignore relation extends? (extend = table is grown to add new pages -- I assume that the locks taken for that don't conflict with relation size / catalog entries, but would be good to verify somehow)

I think it'd be hard to control the timing of an extend lock. But maybe we could note what locks the collector needs when running these queries, take those manually, and then try to extend a relation?

I've tried to read a bit more of the Postgres source, and my understanding is because relation extension uses a separate lock tag (LOCKTAG_RELATION_EXTEND, see https://github.com/postgres/postgres/blob/master/src/include/storage/lock.h#L195) it doesn't conflict with the AccessShareLock taken with the LOCKTAG_RELATION lock tag as part of us calling pg_relation_size.

So for now I think we can assume that all three of these are safe to ignore, and its just locktype = 'relation' that's a problem.

input/postgres/relations.go

keiko713

Left comment re: locktype, though this is already good start, so I'm fine to merge as is too.

lfittl

Overall looks good, but I think we could adjust to locktype = 'relation' as discussed to ignore page and extend locks as well.

lfittl · 2023-09-29T22:59:00Z

input/postgres/relation_stats.go

@@ -14,7 +14,7 @@ const relationStatsSQLInsertsSinceVacuumFieldDefault string = "0"

 const relationStatsSQL = `
 WITH locked_relids AS (
-	SELECT DISTINCT relation relid FROM pg_catalog.pg_locks WHERE mode = 'AccessExclusiveLock' AND relation IS NOT NULL
+	SELECT DISTINCT relation relid FROM pg_catalog.pg_locks WHERE mode = 'AccessExclusiveLock' AND relation IS NOT NULL AND locktype <> 'tuple'


I've tried to read a bit more of the Postgres source, and my understanding is because relation extension uses a separate lock tag (LOCKTAG_RELATION_EXTEND, see https://github.com/postgres/postgres/blob/master/src/include/storage/lock.h#L195) it doesn't conflict with the AccessShareLock taken with the LOCKTAG_RELATION lock tag as part of us calling pg_relation_size.

So for now I think we can assume that all three of these are safe to ignore, and its just locktype = 'relation' that's a problem.

Currently, if tables are locked in AccessExclusiveMode, we skip collecting most table structure information and table and column stats. We do this since the lock would conflict with the lock the collector needs and potentially cause cascading locking problems and impact the application. However, we're not checking the locktype, so the collector also avoids collecting stats for tables with AccessExclusiveMode locktype = 'tuple' locks, which do not actually block collection. These can be fairly common, e.g., if a transaction issues a `SELECT FOR UPDATE` query while another transaction has already obtained a `SELECT FOR UPDATE` lock. Relax the locking requirements by ignoring AccessExclusiveMode locktype = 'tuple' locks when collecting schema structure and stats.

The latter should be sufficient to avoid locking problems and allows us to ignore other kinds of locks.

benoittgt · 2023-10-13T12:20:16Z

Thanks. This patch fixed our issue.

msakrejda · 2023-10-19T16:16:56Z

@benoittgt great, glad to hear it!

msakrejda requested a review from a team September 13, 2023 19:08

msakrejda commented Sep 13, 2023

View reviewed changes

keiko713 approved these changes Sep 14, 2023

View reviewed changes

lfittl approved these changes Sep 29, 2023

View reviewed changes

msakrejda added 2 commits October 2, 2023 08:44

Change lock check from locktype <> 'tuple' to locktype = 'relation'

a2581b5

The latter should be sufficient to avoid locking problems and allows us to ignore other kinds of locks.

msakrejda force-pushed the relax-locking-requirements branch from ffdcddf to a2581b5 Compare October 2, 2023 15:47

msakrejda merged commit 54cbc68 into main Oct 2, 2023
3 checks passed

msakrejda deleted the relax-locking-requirements branch October 2, 2023 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relax locking requirements for collecting table stats #455

Relax locking requirements for collecting table stats #455

msakrejda commented Sep 13, 2023

msakrejda left a comment

msakrejda Sep 13, 2023

keiko713 Sep 14, 2023

msakrejda Sep 14, 2023

lfittl Sep 14, 2023

msakrejda Sep 14, 2023

lfittl Sep 29, 2023

keiko713 left a comment

lfittl left a comment

lfittl Sep 29, 2023

benoittgt commented Oct 13, 2023

msakrejda commented Oct 19, 2023

Relax locking requirements for collecting table stats #455

Relax locking requirements for collecting table stats #455

Conversation

msakrejda commented Sep 13, 2023

msakrejda left a comment

Choose a reason for hiding this comment

msakrejda Sep 13, 2023

Choose a reason for hiding this comment

keiko713 Sep 14, 2023

Choose a reason for hiding this comment

msakrejda Sep 14, 2023

Choose a reason for hiding this comment

lfittl Sep 14, 2023

Choose a reason for hiding this comment

msakrejda Sep 14, 2023

Choose a reason for hiding this comment

lfittl Sep 29, 2023

Choose a reason for hiding this comment

keiko713 left a comment

Choose a reason for hiding this comment

lfittl left a comment

Choose a reason for hiding this comment

lfittl Sep 29, 2023

Choose a reason for hiding this comment

benoittgt commented Oct 13, 2023

msakrejda commented Oct 19, 2023