From 37e3845e6afdb91bfd77940770da74ecd07ca698 Mon Sep 17 00:00:00 2001 From: Hadi Moshayedi Date: Mon, 5 Oct 2020 10:47:09 -0700 Subject: [PATCH] Address Nils feedback --- cstore_tableam.c | 96 ++++++++++++++++++++++---------- expected/am_vacuum_vs_insert.out | 9 ++- specs/am_vacuum_vs_insert.spec | 4 +- 3 files changed, 76 insertions(+), 33 deletions(-) diff --git a/cstore_tableam.c b/cstore_tableam.c index 59df86fb2..f6b51b770 100644 --- a/cstore_tableam.c +++ b/cstore_tableam.c @@ -46,7 +46,6 @@ * * These are the same values from src/backend/access/heap/vacuumlazy.c */ -#define VACUUM_TRUNCATE_LOCK_CHECK_INTERVAL 20 /* ms */ #define VACUUM_TRUNCATE_LOCK_WAIT_INTERVAL 50 /* ms */ #define VACUUM_TRUNCATE_LOCK_TIMEOUT 5000 /* ms */ @@ -68,6 +67,8 @@ static void CStoreTableAMObjectAccessHook(ObjectAccessType access, Oid classId, objectId, int subId, void *arg); static bool IsCStoreTableAmTable(Oid relationId); +static bool ConditionalLockRelationWithTimeout(Relation rel, LOCKMODE lockMode, + int timeout, int retryInterval); static void TruncateCStore(Relation rel, int elevel); @@ -622,7 +623,6 @@ static void TruncateCStore(Relation rel, int elevel) { PGRUsage ru0; - int lock_retry = 0; BlockNumber old_rel_pages = 0; BlockNumber new_rel_pages = 0; DataFileMetadata *metadata = NULL; @@ -634,45 +634,46 @@ TruncateCStore(Relation rel, int elevel) pgstat_progress_update_param(PROGRESS_VACUUM_PHASE, PROGRESS_VACUUM_PHASE_TRUNCATE); + /* - * We need an ExclusiveLock to do the truncation. - * Loop until we acquire a lock or retry threshold is reached. + * We need access exclusive lock on the relation in order to do + * truncation. If we can't get it, give up rather than waiting --- we + * don't want to block other backends, and we don't want to deadlock + * (which is quite possible considering we already hold a lower-grade + * lock). + * + * The decisions for AccessExclusiveLock and conditional lock with + * a timeout is based on lazy_truncate_heap in vacuumlazy.c. */ - while (true) + if (!ConditionalLockRelationWithTimeout(rel, AccessExclusiveLock, + VACUUM_TRUNCATE_LOCK_TIMEOUT, + VACUUM_TRUNCATE_LOCK_WAIT_INTERVAL)) { - if (ConditionalLockRelation(rel, AccessExclusiveLock)) - { - break; - } - /* - * Check for interrupts while trying to (re-)acquire the exclusive - * lock. + * We failed to establish the lock in the specified number of + * retries. This means we give up truncating. */ - CHECK_FOR_INTERRUPTS(); - - if (++lock_retry > (VACUUM_TRUNCATE_LOCK_TIMEOUT / - VACUUM_TRUNCATE_LOCK_WAIT_INTERVAL)) - { - /* - * We failed to establish the lock in the specified number of - * retries. This means we give up truncating. - */ - ereport(elevel, - (errmsg("\"%s\": stopping truncate due to conflicting lock request", - RelationGetRelationName(rel)))); - return; - } - - pg_usleep(VACUUM_TRUNCATE_LOCK_WAIT_INTERVAL * 1000L); + ereport(elevel, + (errmsg("\"%s\": stopping truncate due to conflicting lock request", + RelationGetRelationName(rel)))); + return; } RelationOpenSmgr(rel); old_rel_pages = smgrnblocks(rel->rd_smgr, MAIN_FORKNUM); RelationCloseSmgr(rel); - /* loop over stripes and find max used block */ + /* + * Get metadata as viewed in latest snapshot. Reading metadata in transaction + * snapshot is not enough, since stripes could have been created between + * current transaction start and lock acquisition time. Ignoring those + * stripes can destory data. + */ + PushActiveSnapshot(GetLatestSnapshot()); metadata = ReadDataFileMetadata(rel->rd_node.relNode); + PopActiveSnapshot(); + + /* loop over stripes and find max used block */ foreach(stripeMetadataCell, metadata->stripeMetadataList) { StripeMetadata *stripe = lfirst(stripeMetadataCell); @@ -710,6 +711,43 @@ TruncateCStore(Relation rel, int elevel) } +/* + * ConditionalLockRelationWithTimeout tries to acquire a relation lock until + * it either succeeds or timesout. It doesn't enter wait queue and instead it + * sleeps between lock tries. + * + * This is based on the lock loop in lazy_truncate_heap(). + */ +static bool +ConditionalLockRelationWithTimeout(Relation rel, LOCKMODE lockMode, int timeout, + int retryInterval) +{ + int lock_retry = 0; + + while (true) + { + if (ConditionalLockRelation(rel, lockMode)) + { + break; + } + + /* + * Check for interrupts while trying to (re-)acquire the lock + */ + CHECK_FOR_INTERRUPTS(); + + if (++lock_retry > (timeout / retryInterval)) + { + return false; + } + + pg_usleep(retryInterval * 1000L); + } + + return true; +} + + static bool cstore_scan_analyze_next_block(TableScanDesc scan, BlockNumber blockno, BufferAccessStrategy bstrategy) diff --git a/expected/am_vacuum_vs_insert.out b/expected/am_vacuum_vs_insert.out index 8ef78bfa4..ae23d9a26 100644 --- a/expected/am_vacuum_vs_insert.out +++ b/expected/am_vacuum_vs_insert.out @@ -10,8 +10,9 @@ step s1-begin: step s1-insert: INSERT INTO test_vacuum_vs_insert SELECT i, 2 * i FROM generate_series(1, 3) i; +s2: INFO: "test_vacuum_vs_insert": stopping truncate due to conflicting lock request step s2-vacuum: - VACUUM test_vacuum_vs_insert; + VACUUM VERBOSE test_vacuum_vs_insert; step s1-commit: COMMIT; @@ -39,11 +40,15 @@ step s1-insert: INSERT INTO test_vacuum_vs_insert SELECT i, 2 * i FROM generate_series(1, 3) i; step s2-vacuum-full: - VACUUM FULL test_vacuum_vs_insert; + VACUUM FULL VERBOSE test_vacuum_vs_insert; step s1-commit: COMMIT; +s2: INFO: vacuuming "public.test_vacuum_vs_insert" +s2: INFO: "test_vacuum_vs_insert": found 0 removable, 6 nonremovable row versions in 1 pages +DETAIL: 0 dead row versions cannot be removed yet. +CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s. step s2-vacuum-full: <... completed> step s2-select: SELECT * FROM test_vacuum_vs_insert; diff --git a/specs/am_vacuum_vs_insert.spec b/specs/am_vacuum_vs_insert.spec index 57105e1dd..ac2d83667 100644 --- a/specs/am_vacuum_vs_insert.spec +++ b/specs/am_vacuum_vs_insert.spec @@ -29,12 +29,12 @@ session "s2" step "s2-vacuum" { - VACUUM test_vacuum_vs_insert; + VACUUM VERBOSE test_vacuum_vs_insert; } step "s2-vacuum-full" { - VACUUM FULL test_vacuum_vs_insert; + VACUUM FULL VERBOSE test_vacuum_vs_insert; } step "s2-select"