Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(PDB-5672) Periodically analyze reports table #4004

Merged
merged 4 commits into from
Oct 3, 2024

Conversation

rbrw
Copy link
Contributor

@rbrw rbrw commented Sep 24, 2024

No description provided.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

src/puppetlabs/puppetdb/cli/services.clj Outdated Show resolved Hide resolved
src/puppetlabs/puppetdb/cli/services.clj Outdated Show resolved Hide resolved
@austb
Copy link
Contributor

austb commented Sep 24, 2024

The test failure is a NPE in the new test. Perhaps the data it is looking for does not exist in PG 14?

@rbrw rbrw force-pushed the pdb-5672-analyze-parent-partitions branch from 2311b05 to daecbcf Compare September 25, 2024 19:45
@rbrw rbrw force-pushed the pdb-5672-analyze-parent-partitions branch from daecbcf to 9d7a7ae Compare September 25, 2024 21:16
@rbrw rbrw force-pushed the pdb-5672-analyze-parent-partitions branch from 9d7a7ae to d925940 Compare September 25, 2024 22:25
(defn analyze-partitioned-tables [db shutdown-for-ex]
;; This assertion isn't a requirement, but it makes it obvious that
;; the function is next.jdbc "safe", e.g. no nested tranactions.
(assert (not (sql/db-find-connection db)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should leave this assert in because it throws an Error (not an Exception) and this runs inside a ScheduledThreadPoolExecutor. If we fail to catch the Error ourselves, it will cause us to stop analyzing entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right -- was planning to change that to "something else" before we finished, but may just remove it.

@austb austb self-requested a review September 27, 2024 19:09
@rbrw rbrw force-pushed the pdb-5672-analyze-parent-partitions branch from d925940 to 8d1f620 Compare September 27, 2024 22:32
postgres (up to at least 16) never analyzes partitioned table parents
Nearly fixed in 14, but removed before release:
  https://www.postgresql.org/about/news/postgresql-14-beta-1-released-2213/
  https://www.postgresql.org/about/news/postgresql-14-rc-1-released-2309/

Assumes the analysis runs quickly enough to avoid needing any enforced
serialization, and to avoid (with the current single threaded
executor) ever delaying gc enough to matter.
@rbrw rbrw force-pushed the pdb-5672-analyze-parent-partitions branch from 8d1f620 to af3348d Compare September 27, 2024 22:33
@austb austb removed the don't merge label Oct 3, 2024
@austb austb marked this pull request as ready for review October 3, 2024 18:08
@austb austb requested review from a team as code owners October 3, 2024 18:08
@austb austb merged commit b4a2312 into puppetlabs:main Oct 3, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants