From b7d27729633dfdc8b34cafdba2d9ce284cbdb8e3 Mon Sep 17 00:00:00 2001 From: Austin Blatt Date: Wed, 16 Oct 2024 15:50:06 -0700 Subject: [PATCH 1/3] partitioning: make date-suffix return lower-case string postgres will coerce unquoted table names to lowercase, so have this function return lowercase from the start --- src/puppetlabs/puppetdb/scf/partitioning.clj | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/puppetlabs/puppetdb/scf/partitioning.clj b/src/puppetlabs/puppetdb/scf/partitioning.clj index e3f356596e..afa6f41e70 100644 --- a/src/puppetlabs/puppetdb/scf/partitioning.clj +++ b/src/puppetlabs/puppetdb/scf/partitioning.clj @@ -1,6 +1,7 @@ (ns puppetlabs.puppetdb.scf.partitioning "Handles all work related to database table partitioning" (:require + [clojure.string :refer [lower-case]] [puppetlabs.i18n.core :refer [trs]] [puppetlabs.puppetdb.jdbc :as jdbc] [schema.core :as s]) @@ -32,8 +33,8 @@ (defn date-suffix [date] - (let [formatter (.withZone (DateTimeFormatter/BASIC_ISO_DATE) (ZoneId/of "UTC"))] - (.format date formatter))) + (let [formatter (.withZone DateTimeFormatter/BASIC_ISO_DATE (ZoneId/of "UTC"))] + (lower-case (.format date formatter)))) (defn to-zoned-date-time [date] From 9348675c718c2901ed50a2a89e245e0ded7b2016 Mon Sep 17 00:00:00 2001 From: Austin Blatt Date: Wed, 16 Oct 2024 14:46:34 -0700 Subject: [PATCH 2/3] (GH-4013) Handle finalizing partition detach in GC The process of detaching a partition is a two transaction process. When the first transaction succeeds, the partition is now "pending". The second transaction needs an ACCESS EXCLUSIVE lock on the partition and can therefore sometimes fail. When this happens, subsequent GCs will fail because only one pending partition detachment is allowed. To handle this, catch the SQLException and finalize the pending detach operation. If that was a different partition from the partition we are trying to remove, retry the detach operation that failed. --- documentation/release_notes_7.markdown | 11 ++++ src/puppetlabs/puppetdb/jdbc.clj | 1 + src/puppetlabs/puppetdb/scf/storage.clj | 37 +++++++++++-- .../puppetlabs/puppetdb/cli/services_test.clj | 53 ++++++++++++++++++- 4 files changed, 96 insertions(+), 6 deletions(-) diff --git a/documentation/release_notes_7.markdown b/documentation/release_notes_7.markdown index 0ab9657cbf..d28cab699d 100644 --- a/documentation/release_notes_7.markdown +++ b/documentation/release_notes_7.markdown @@ -15,6 +15,17 @@ canonical: "/puppetdb/latest/release_notes.html" # PuppetDB: Release notes +## PuppetDB 7.20.1 + +Released TBD. + +### Bug fixes + +* Fixed an issue with report garbage collection where a partition would become + partially detached and block future garbage collection progress. Garbage + collection will now finalize the partition detach operation and remove the + table. ([GitHub #4013](https://github.com/puppetlabs/puppetdb/issues/4013)) + ## PuppetDB 7.20.0 Released October 22 2024 diff --git a/src/puppetlabs/puppetdb/jdbc.clj b/src/puppetlabs/puppetdb/jdbc.clj index 35dd15bafd..bc222d0451 100644 --- a/src/puppetlabs/puppetdb/jdbc.clj +++ b/src/puppetlabs/puppetdb/jdbc.clj @@ -24,6 +24,7 @@ (or ({:admin-shutdown "57P01" :invalid-regular-expression "2201B" :program-limit-exceeded "54000" + :not-in-prerequisite-state "55000" :lock-not-available "55P03" :query-canceled "57014"} kw-name) diff --git a/src/puppetlabs/puppetdb/scf/storage.clj b/src/puppetlabs/puppetdb/scf/storage.clj index 8459b94e36..ccfe979581 100644 --- a/src/puppetlabs/puppetdb/scf/storage.clj +++ b/src/puppetlabs/puppetdb/scf/storage.clj @@ -1673,6 +1673,22 @@ (let [{current-db-version :version} (sutils/db-metadata)] (not (neg? (compare current-db-version pg14-db))))) +(defn finalize-pending-detach + "Finalize a previously failed detach operation. A partitioned table can + only have one partition pending detachment at any time." + [parent] + (let [pending (->> ["SELECT inhrelid::regclass AS child + FROM pg_catalog.pg_inherits + WHERE inhparent = ?::regclass AND inhdetachpending = true" + parent] + jdbc/query-to-vec + first + :child)] + (when pending + (log/info (trs "Finalizing detach for partition {0}" pending)) + (jdbc/do-commands (format "ALTER TABLE %s DETACH PARTITION %s FINALIZE" parent pending)) + (str pending)))) + (defn prune-daily-partitions "Either detaches or drops obsolete day-oriented partitions older than the date. Deletes or detaches only the oldest such candidate if @@ -1698,14 +1714,27 @@ candidates (->> (partitioning/get-partition-names table-prefix) (filter expired?) sort) - drop-one (fn [table] + detach (fn detach [parent child] + (jdbc/do-commands-outside-txn + (format "alter table %s detach partition %s concurrently" parent child))) + drop-one (fn drop-one [table] (update-lock-status status-key inc) (try! (if just-detach? - (jdbc/do-commands-outside-txn - (format "alter table %s detach partition %s concurrently" table-prefix table)) + (let [ex (try + (detach table-prefix table) + (catch SQLException ex + (if (= (jdbc/sql-state :not-in-prerequisite-state) (.getSQLState ex)) + ex + (throw ex))))] + (when (instance? SQLException ex) + (let [finalized-table (finalize-pending-detach table-prefix)] + (when-not (= finalized-table table) + ;; Retry, unless the finalized partition detach was + ;; for the same table + (detach table-prefix table))))) (jdbc/do-commands - (format "drop table if exists %s cascade" table))) + (format "drop table if exists %s cascade" table))) (finally (update-lock-status status-key dec)))) drop #(if incremental? diff --git a/test/puppetlabs/puppetdb/cli/services_test.clj b/test/puppetlabs/puppetdb/cli/services_test.clj index d64ecf8114..6011c64384 100644 --- a/test/puppetlabs/puppetdb/cli/services_test.clj +++ b/test/puppetlabs/puppetdb/cli/services_test.clj @@ -5,7 +5,8 @@ [puppetlabs.puppetdb.command.constants :as cmd-consts] [puppetlabs.puppetdb.lint :refer [ignore-value]] [puppetlabs.puppetdb.scf.partitioning - :refer [get-temporal-partitions]] + :refer [get-temporal-partitions] + :as part] [puppetlabs.trapperkeeper.testutils.logging :refer [with-log-output logs-matching @@ -607,4 +608,52 @@ :report-ttl (time/parse-period "1d") :resource-events-ttl (time/parse-period "1d") :db-lock-status db-lock-status}) - (is (empty? (jdbc/query ["SELECT * FROM reports"]))))))))) + (is (empty? (jdbc/query ["SELECT * FROM reports"])))) + + ;; This test is not applicable unless our Postgres version is new enough + ;; to support the concurrent partition detach feature. + (when (scf-store/detach-partitions-concurrently?) + (testing "a partition stuck in the pending state is finalized and removed" + (let [old-ts (-> 2 time/days time/ago) + partition-table (format "reports_%s" + (part/date-suffix (part/to-zoned-date-time (time/to-timestamp old-ts)))) + lock-acquired (promise) + partition-pending-detach (promise)] + (store-report (time/to-string old-ts)) + (store-report (to-string (now))) + + (future + ;; Create a query that will block the ACCESS EXCLUSIVE lock needed + ;; by the second transaction of the concurrent detach below + (jdbc/with-transacted-connection *read-db* + (jdbc/with-db-transaction [] + (jdbc/query [(format "select * from %s" partition-table)]) + (deliver lock-acquired partition-table) + + ;; wait for partition detach to fail + @partition-pending-detach))) + + ;; Wait until we are sure that the detach partition operation will be blocked + @lock-acquired + + (try + (jdbc/do-commands-outside-txn + "SET statement_timeout = 100" + (format "ALTER TABLE reports DETACH PARTITION %s CONCURRENTLY" partition-table)) + (catch java.sql.SQLException _) + (finally + (deliver partition-pending-detach partition-table) + (jdbc/do-commands-outside-txn "SET statement_timeout = 0"))) + + (is (= [{:inhdetachpending true}] + (jdbc/query ["select inhdetachpending from pg_catalog.pg_inherits where inhparent = 'reports'::regclass and inhrelid = ?::regclass" partition-table]))) + + (svcs/sweep-reports! *db* {:incremental? false + :report-ttl (time/parse-period "1d") + :resource-events-ttl (time/parse-period "1d") + :db-lock-status db-lock-status}) + + (is (empty? + (jdbc/query ["SELECT tablename FROM pg_tables WHERE tablename = ?" partition-table]))) + + (jdbc/do-commands "DELETE FROM reports"))))))))) From 7d8691490c15673e6c15ca76df12bc8de566aa8d Mon Sep 17 00:00:00 2001 From: Austin Blatt Date: Wed, 16 Oct 2024 14:47:30 -0700 Subject: [PATCH 3/3] (GH-4013) Clean up stranded partitions If a partition is fully detached, but fails to be dropped during its GC operation, subsequent GC operations will not see that partition at all. It will be stranded and PuppetDB will never remove it. During GC, search for stranded partitions that need to be removed and add them to the list of partitions that need to be dropped. There is no structural way to tell the difference between a non-partitioned table and a detached partition table. This PR uses a regular expression, which means that PuppetDB cannot have any non-partitioned tables matching the regular expressions used to identify stranded partitions. --- documentation/release_notes_7.markdown | 4 ++ src/puppetlabs/puppetdb/scf/storage.clj | 42 ++++++++++++++----- .../puppetlabs/puppetdb/cli/services_test.clj | 27 ++++++++++-- 3 files changed, 59 insertions(+), 14 deletions(-) diff --git a/documentation/release_notes_7.markdown b/documentation/release_notes_7.markdown index d28cab699d..b84aaad4a9 100644 --- a/documentation/release_notes_7.markdown +++ b/documentation/release_notes_7.markdown @@ -25,6 +25,10 @@ Released TBD. partially detached and block future garbage collection progress. Garbage collection will now finalize the partition detach operation and remove the table. ([GitHub #4013](https://github.com/puppetlabs/puppetdb/issues/4013)) +* Fixed an issue with report garbage collection where a partition would be + detached, but the table was never deleted. Garbage collection will now + identify and clean-up these tables. + ([GitHub #4013](https://github.com/puppetlabs/puppetdb/issues/4013)) ## PuppetDB 7.20.0 diff --git a/src/puppetlabs/puppetdb/scf/storage.clj b/src/puppetlabs/puppetdb/scf/storage.clj index ccfe979581..f755adf867 100644 --- a/src/puppetlabs/puppetdb/scf/storage.clj +++ b/src/puppetlabs/puppetdb/scf/storage.clj @@ -1689,6 +1689,23 @@ (jdbc/do-commands (format "ALTER TABLE %s DETACH PARTITION %s FINALIZE" parent pending)) (str pending)))) +(defn find-stranded-partitions + "Identify tables that match the child format of a partitioned table (like reports_historical) + that are not present in the pg_inherits table. These partitions have been detached, but failed + to be deleted. + + Tables that are not partitioned will also not be in the pg_inherits table, so you MUST + write a child-format that does not match any non-partitioned tables. + + Returns a list of strings. Each string is a stranded partition that should be removed." + [child-format] + (->> [(str "SELECT tablename" + " FROM pg_tables WHERE tablename ~ ?" + " AND tablename NOT IN (SELECT inhrelid::regclass::text FROM pg_catalog.pg_inherits)") + child-format] + jdbc/query-to-vec + (map (comp str :tablename)))) + (defn prune-daily-partitions "Either detaches or drops obsolete day-oriented partitions older than the date. Deletes or detaches only the oldest such candidate if @@ -1774,7 +1791,7 @@ "Drops the given set of tables. Will throw an SQLException termination if the operation takes much longer than PDB_GC_DAILY_PARTITION_DROP_LOCK_TIMEOUT_MS." [old-partition-tables update-lock-status status-key] - (let [drop #(doseq [table old-partition-tables] + (let [drop #(doseq [table (distinct old-partition-tables)] (try (update-lock-status status-key inc) (jdbc/do-commands @@ -1798,9 +1815,10 @@ ;; PG14+ (let [detached-tables (detach-daily-partitions "resource_events" date incremental? - update-lock-status :write-locking-resource-events)] + update-lock-status :write-locking-resource-events) + stranded-tables (find-stranded-partitions "^resource_events_\\d\\d\\d\\d\\d\\d\\d\\dz$")] (jdbc/with-db-transaction [] - (drop-partition-tables! detached-tables + (drop-partition-tables! (concat detached-tables stranded-tables) update-lock-status :write-locking-resource-events))))) (defn cleanup-dropped-report-certnames @@ -1853,13 +1871,15 @@ ;; PG14+ ;; Detach partition concurrently must take place outside of a transaction. (let [detached-resource-event-tables - (detach-daily-partitions "resource_events" effective-resource-events-ttl - incremental? update-lock-status - :write-locking-resource-events) + (detach-daily-partitions "resource_events" effective-resource-events-ttl + incremental? update-lock-status + :write-locking-resource-events) + stranded-events-tables (find-stranded-partitions "^resource_events_\\d\\d\\d\\d\\d\\d\\d\\dz$") detached-report-tables - (detach-daily-partitions "reports" report-ttl - incremental? update-lock-status - :write-locking-reports)] + (detach-daily-partitions "reports" report-ttl + incremental? update-lock-status + :write-locking-reports) + stranded-reports-tables (find-stranded-partitions "^reports_\\d\\d\\d\\d\\d\\d\\d\\dz$")] ;; Now we can delete the partitions with less intrusive locking. (jdbc/with-db-transaction [] ;; Nothing should acquire locks on the detached tables, but to be safe, acquire @@ -1869,9 +1889,9 @@ ;; force a resource-events GC. prior to partitioning, this would have happened ;; via a cascade when the report was deleted, but now we just drop whole tables ;; of resource events. - (drop-partition-tables! detached-resource-event-tables + (drop-partition-tables! (concat detached-resource-event-tables stranded-events-tables) update-lock-status :write-locking-resource-events) - (drop-partition-tables! detached-report-tables + (drop-partition-tables! (concat detached-report-tables stranded-reports-tables) update-lock-status :write-locking-reports) ;; since we cannot cascade back to the certnames table anymore, go clean up ;; the latest_report_id column after a GC. diff --git a/test/puppetlabs/puppetdb/cli/services_test.clj b/test/puppetlabs/puppetdb/cli/services_test.clj index 6011c64384..eac87f5fa9 100644 --- a/test/puppetlabs/puppetdb/cli/services_test.clj +++ b/test/puppetlabs/puppetdb/cli/services_test.clj @@ -584,7 +584,7 @@ (with-test-db (let [config (-> (create-temp-config) (assoc :database *db* :read-database *read-db*) - (assoc-in [:database :gc-interval] "0.01")) + (assoc-in [:database :gc-interval] "60")) store-report #(sync-command-post (svc-utils/pdb-cmd-url) example-certname "store report" @@ -610,7 +610,7 @@ :db-lock-status db-lock-status}) (is (empty? (jdbc/query ["SELECT * FROM reports"])))) - ;; This test is not applicable unless our Postgres version is new enough + ;; These tests are not applicable unless our Postgres version is new enough ;; to support the concurrent partition detach feature. (when (scf-store/detach-partitions-concurrently?) (testing "a partition stuck in the pending state is finalized and removed" @@ -656,4 +656,25 @@ (is (empty? (jdbc/query ["SELECT tablename FROM pg_tables WHERE tablename = ?" partition-table]))) - (jdbc/do-commands "DELETE FROM reports"))))))))) + (jdbc/do-commands "DELETE FROM reports"))) + + (testing "a detached partition that was not removed is cleaned up by gc" + (let [old-ts (-> 2 time/days time/ago) + partition-table (format "reports_%s" + (part/date-suffix (part/to-zoned-date-time (time/to-timestamp old-ts))))] + (store-report (time/to-string old-ts)) + (store-report (to-string (now))) + + ;; Strand the partition before calling GC + (jdbc/do-commands-outside-txn + (format "ALTER TABLE reports DETACH PARTITION %s CONCURRENTLY" partition-table)) + + (svcs/sweep-reports! *db* {:incremental? false + :report-ttl (time/parse-period "1d") + :resource-events-ttl (time/parse-period "1d") + :db-lock-status db-lock-status}) + + (is (empty? + (jdbc/query ["SELECT tablename FROM pg_tables WHERE tablename = ?" partition-table]))) + + (jdbc/do-commands "DELETE FROM reports")))))))))