From 03731759e6e861858ef6b7556733cb496b2d0c22 Mon Sep 17 00:00:00 2001 From: Santiago Bosio Date: Fri, 5 Apr 2024 13:50:10 -0300 Subject: [PATCH 1/5] Move command 'pg:settings:auto-explain:log-analyze' to CLI --- .../pg/settings/auto-explain/log-analyze.ts | 34 ++++++++++++ packages/cli/src/lib/pg/setter.ts | 53 ++++++++++++++++++ packages/cli/src/lib/pg/types.ts | 10 ++++ .../auto-explain/log-analyze.unit.test.ts | 54 +++++++++++++++++++ .../settings/auto_explain_log_analyze.js | 26 --------- packages/pg-v5/index.js | 1 - .../commands/settings/settings.unit.test.js | 24 --------- 7 files changed, 151 insertions(+), 51 deletions(-) create mode 100644 packages/cli/src/commands/pg/settings/auto-explain/log-analyze.ts create mode 100644 packages/cli/src/lib/pg/setter.ts create mode 100644 packages/cli/test/unit/commands/pg/settings/auto-explain/log-analyze.unit.test.ts delete mode 100644 packages/pg-v5/commands/settings/auto_explain_log_analyze.js diff --git a/packages/cli/src/commands/pg/settings/auto-explain/log-analyze.ts b/packages/cli/src/commands/pg/settings/auto-explain/log-analyze.ts new file mode 100644 index 0000000000..7812b8d914 --- /dev/null +++ b/packages/cli/src/commands/pg/settings/auto-explain/log-analyze.ts @@ -0,0 +1,34 @@ +import {Args} from '@oclif/core' +import heredoc from 'tsheredoc' +import {PGSettingsCommand, boolean} from '../../../../lib/pg/setter' +import {FormationSetting, Setting} from '../../../../lib/pg/types' + +export default class LogAnalyze extends PGSettingsCommand { + static topic = 'pg' + // static command = 'settings:auto-explain:log-analyze' + static description = heredoc(` + Shows actual run times on the execution plan. + This is equivalent to calling EXPLAIN ANALYZE. + + WARNING: EXPLAIN ANALYZE will be run on ALL queries, not just logged queries. This can cause significant performance impacts to your database and should be used with caution. + `) + + static args = { + database: Args.string(), + value: Args.string(), + } + + protected settingsName = 'auto_explain.log_analyze' as FormationSetting + + protected convertValue(val: string): boolean { + return boolean(val) + } + + protected explain(setting: Setting) { + if (setting.value) { + return 'EXPLAIN ANALYZE execution plans will be logged.' + } + + return 'EXPLAIN ANALYZE execution plans will not be logged.' + } +} diff --git a/packages/cli/src/lib/pg/setter.ts b/packages/cli/src/lib/pg/setter.ts new file mode 100644 index 0000000000..b9fc08c72d --- /dev/null +++ b/packages/cli/src/lib/pg/setter.ts @@ -0,0 +1,53 @@ +import {Command, flags} from '@heroku-cli/command' +import {ux} from '@oclif/core' +import {addonResolver} from '../addons/resolve' +import host from './host' +import {essentialPlan} from './util' +import {FormationSetting, Setting, SettingsResponse} from './types' + +export function boolean(value: string): boolean { + switch (value) { + case 'true': case 'TRUE': case 'ON': case 'on': + return true + case 'false': case 'FALSE': case 'OFF': case 'off': + return false + default: + throw new TypeError('Invalid value. Valid options are: a boolean value') + } +} + +export abstract class PGSettingsCommand extends Command { + protected abstract settingsName: FormationSetting + protected abstract convertValue(val: string): unknown + protected abstract explain(setting: Setting): string + + static flags = { + app: flags.app({required: true}), + remote: flags.remote(), + } + + public async run(): Promise { + const {flags, args} = await this.parse() + const {app} = flags + const {value, database} = args as {value: string | undefined, database: string | undefined} + + const db = await addonResolver(this.heroku, app, database || '') + + if (essentialPlan(db)) ux.error('You can’t perform this operation on Essential-tier databases.') + + if (value) { + const {body: settings} = await this.heroku.patch(`/postgres/v0/databases/${db.id}/config`, { + hostname: host(), + body: {[this.settingsName]: this.convertValue(value)}, + }) + const setting = settings[this.settingsName] + ux.log(`${this.settingsName.replace(/_/g, '-')} has been set to ${setting.value} for ${db.name}.`) + ux.log(this.explain(setting)) + } else { + const {body: settings} = await this.heroku.get(`/postgres/v0/databases/${db.id}/config`, {hostname: host()}) + const setting = settings[this.settingsName] + ux.log(`${this.settingsName.replace(/_/g, '-')} is set to ${setting.value} for ${db.name}.`) + ux.log(this.explain(setting)) + } + } +} diff --git a/packages/cli/src/lib/pg/types.ts b/packages/cli/src/lib/pg/types.ts index 13113bbe7e..17b6cde5f7 100644 --- a/packages/cli/src/lib/pg/types.ts +++ b/packages/cli/src/lib/pg/types.ts @@ -63,3 +63,13 @@ export type CredentialsInfo = { export type MaintenanceApiResponse = { message: string, } +export type FormationSetting = 'log_lock_waits' | 'log_connections' | 'log_min_duration_statement' | 'log_statement' | 'track_functions' | + 'pgbouncer_max_client_conn' | 'pg_bouncer_max_db_conns' | 'pg_bouncer_default_pool_size' | 'auto_explain' | 'auto_explain.log_min_duration' | + 'auto_explain.log_analyze' | 'auto_explain.log_triggers' | 'auto_explain.log_buffers' | 'auto_explain.log_verbose' | + 'auto_explain.log_nested_statements' +export type Setting = { + value: T + desc: string + default: T +} +export type SettingsResponse = Record> diff --git a/packages/cli/test/unit/commands/pg/settings/auto-explain/log-analyze.unit.test.ts b/packages/cli/test/unit/commands/pg/settings/auto-explain/log-analyze.unit.test.ts new file mode 100644 index 0000000000..dacf182f36 --- /dev/null +++ b/packages/cli/test/unit/commands/pg/settings/auto-explain/log-analyze.unit.test.ts @@ -0,0 +1,54 @@ +import {expect} from '@oclif/test' +import * as nock from 'nock' +import {stdout} from 'stdout-stderr' +import heredoc from 'tsheredoc' +import runCommand from '../../../../../helpers/runCommand' +import Cmd from '../../../../../../src/commands/pg/settings/auto-explain/log-analyze' +import * as fixtures from '../../../../../fixtures/addons/fixtures' + +describe('pg:settings:auto-explain:log-analyze', () => { + const addon = fixtures.addons['dwh-db'] + let api: nock.Scope + + beforeEach(() => { + api = nock('https://api.heroku.com') + .post('/actions/addons/resolve', { + app: 'myapp', + addon: 'test-database', + }).reply(200, [addon]) + }) + + afterEach(() => { + nock.cleanAll() + }) + + it('updates settings for auto_explain.log_analyze with value', async () => { + const pg = nock('https://api.data.heroku.com') + .patch(`/postgres/v0/databases/${addon.id}/config`).reply(200, {'auto_explain.log_analyze': {value: true}}) + + await runCommand(Cmd, ['--app', 'myapp', 'test-database', 'true']) + + api.done() + pg.done() + + expect(stdout.output).to.equal(heredoc(` + auto-explain.log-analyze has been set to true for ${addon.name}. + EXPLAIN ANALYZE execution plans will be logged. + `)) + }) + + it('shows settings for auto_explain.log_analyze with no value', async () => { + const pg = nock('https://api.data.heroku.com') + .get(`/postgres/v0/databases/${addon.id}/config`).reply(200, {'auto_explain.log_analyze': {value: false}}) + + await runCommand(Cmd, ['--app', 'myapp', 'test-database']) + + api.done() + pg.done() + + expect(stdout.output).to.equal(heredoc(` + auto-explain.log-analyze is set to false for ${addon.name}. + EXPLAIN ANALYZE execution plans will not be logged. + `)) + }) +}) diff --git a/packages/pg-v5/commands/settings/auto_explain_log_analyze.js b/packages/pg-v5/commands/settings/auto_explain_log_analyze.js deleted file mode 100644 index cb47e77fc9..0000000000 --- a/packages/pg-v5/commands/settings/auto_explain_log_analyze.js +++ /dev/null @@ -1,26 +0,0 @@ -'use strict' - -const cli = require('heroku-cli-util') -const settings = require('../../lib/setter') - -function explain(setting) { - if (setting.value) { - return 'EXPLAIN ANALYZE execution plans will be logged.' - } - - return 'EXPLAIN ANALYZE execution plans will not be logged.' -} - -module.exports = { - topic: 'pg', - command: 'settings:auto-explain:log-analyze', - description: 'Shows actual run times on the execution plan.', - help: `This is equivalent to calling EXPLAIN ANALYZE. - -WARNING: EXPLAIN ANALYZE will be run on ALL queries, not just logged queries. This can cause significant performance impacts to your database and should be used with caution. -`, - needsApp: true, - needsAuth: true, - args: [{name: 'value', optional: true}, {name: 'database', optional: true}], - run: cli.command({preauth: true}, settings.generate('auto_explain.log_analyze', settings.boolean, explain)), -} diff --git a/packages/pg-v5/index.js b/packages/pg-v5/index.js index 7796eb6418..8d0ccb329c 100644 --- a/packages/pg-v5/index.js +++ b/packages/pg-v5/index.js @@ -28,7 +28,6 @@ exports.commands = flatten([ require('./commands/reset'), require('./commands/settings'), require('./commands/settings/auto_explain'), - require('./commands/settings/auto_explain_log_analyze'), require('./commands/settings/auto_explain_log_buffers'), require('./commands/settings/auto_explain_log_min_duration'), require('./commands/settings/auto_explain_log_nested_statements'), diff --git a/packages/pg-v5/test/unit/commands/settings/settings.unit.test.js b/packages/pg-v5/test/unit/commands/settings/settings.unit.test.js index 1f8b9c9de4..44cf6648ea 100644 --- a/packages/pg-v5/test/unit/commands/settings/settings.unit.test.js +++ b/packages/pg-v5/test/unit/commands/settings/settings.unit.test.js @@ -120,30 +120,6 @@ describe('pg:settings', () => { .then(() => expect(cli.stdout).to.equal('auto-explain is set to for postgres-1.\nExecution plans of queries will not be logged for future connections.\n')) }) - it('shows settings for auto_explain_log_analyze with value', () => { - setupSettingsMockData('auto_explain.log_analyze') - cmd = proxyquire('../../../../commands/settings/auto_explain_log_analyze', { - settings: proxyquire.noCallThru().load('../../../../lib/setter', { - './fetcher': fetcher, - }), - }) - pg.get('/postgres/v0/databases/1/config').reply(200, settingsResult) - return cmd.run({args: {database: 'test-database', value: ''}, flags: {}}) - .then(() => expect(cli.stdout).to.equal('auto-explain.log-analyze is set to test_value for postgres-1.\nEXPLAIN ANALYZE execution plans will be logged.\n')) - }) - - it('shows settings for auto_explain_log_analyze with no value', () => { - setupSettingsMockData('auto_explain.log_analyze', '') - cmd = proxyquire('../../../../commands/settings/auto_explain_log_analyze', { - settings: proxyquire.noCallThru().load('../../../../lib/setter', { - './fetcher': fetcher, - }), - }) - pg.get('/postgres/v0/databases/1/config').reply(200, settingsResult) - return cmd.run({args: {database: 'test-database', value: ''}, flags: {}}) - .then(() => expect(cli.stdout).to.equal('auto-explain.log-analyze is set to for postgres-1.\nEXPLAIN ANALYZE execution plans will not be logged.\n')) - }) - it('shows settings for auto_explain_log_buffers with value', () => { setupSettingsMockData('auto_explain.log_buffers') cmd = proxyquire('../../../../commands/settings/auto_explain_log_buffers', { From 941b4d757dfc6cc4e7c6217948a3dd93fdc55e29 Mon Sep 17 00:00:00 2001 From: Santiago Bosio Date: Fri, 5 Apr 2024 14:26:35 -0300 Subject: [PATCH 2/5] Removing commented static property --- .../cli/src/commands/pg/settings/auto-explain/log-analyze.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/cli/src/commands/pg/settings/auto-explain/log-analyze.ts b/packages/cli/src/commands/pg/settings/auto-explain/log-analyze.ts index 7812b8d914..2936bb8cb0 100644 --- a/packages/cli/src/commands/pg/settings/auto-explain/log-analyze.ts +++ b/packages/cli/src/commands/pg/settings/auto-explain/log-analyze.ts @@ -5,7 +5,6 @@ import {FormationSetting, Setting} from '../../../../lib/pg/types' export default class LogAnalyze extends PGSettingsCommand { static topic = 'pg' - // static command = 'settings:auto-explain:log-analyze' static description = heredoc(` Shows actual run times on the execution plan. This is equivalent to calling EXPLAIN ANALYZE. From 7a66cbc25e86d120ff4c544c8f70b60762930819 Mon Sep 17 00:00:00 2001 From: Santiago Bosio Date: Mon, 8 Apr 2024 10:18:39 -0300 Subject: [PATCH 3/5] Fixing default add-on resolution --- packages/cli/src/lib/pg/setter.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli/src/lib/pg/setter.ts b/packages/cli/src/lib/pg/setter.ts index b9fc08c72d..880b9f4330 100644 --- a/packages/cli/src/lib/pg/setter.ts +++ b/packages/cli/src/lib/pg/setter.ts @@ -31,7 +31,7 @@ export abstract class PGSettingsCommand extends Command { const {app} = flags const {value, database} = args as {value: string | undefined, database: string | undefined} - const db = await addonResolver(this.heroku, app, database || '') + const db = await addonResolver(this.heroku, app, database || 'DATABASE_URL') if (essentialPlan(db)) ux.error('You can’t perform this operation on Essential-tier databases.') From 58b47477cf47d5c94edb3a12f0ddeb598f87fcd6 Mon Sep 17 00:00:00 2001 From: Santiago Bosio Date: Mon, 8 Apr 2024 10:44:31 -0300 Subject: [PATCH 4/5] Renaming types to align with previous naming scheme. --- .../cli/src/commands/pg/settings/auto-explain/log-analyze.ts | 4 ++-- packages/cli/src/lib/pg/setter.ts | 4 ++-- packages/cli/src/lib/pg/types.ts | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/commands/pg/settings/auto-explain/log-analyze.ts b/packages/cli/src/commands/pg/settings/auto-explain/log-analyze.ts index aca112c19d..debd1fcd37 100644 --- a/packages/cli/src/commands/pg/settings/auto-explain/log-analyze.ts +++ b/packages/cli/src/commands/pg/settings/auto-explain/log-analyze.ts @@ -1,7 +1,7 @@ import {Args} from '@oclif/core' import heredoc from 'tsheredoc' import {PGSettingsCommand, booleanConverter, BooleanAsString} from '../../../../lib/pg/setter' -import {FormationSetting, Setting} from '../../../../lib/pg/types' +import {SettingKey, Setting} from '../../../../lib/pg/types' export default class LogAnalyze extends PGSettingsCommand { static topic = 'pg' @@ -17,7 +17,7 @@ export default class LogAnalyze extends PGSettingsCommand { value: Args.string(), } - protected settingKey = 'auto_explain.log_analyze' as FormationSetting + protected settingKey = 'auto_explain.log_analyze' as SettingKey protected convertValue(val: BooleanAsString): boolean { return booleanConverter(val) diff --git a/packages/cli/src/lib/pg/setter.ts b/packages/cli/src/lib/pg/setter.ts index c06b253b12..26f0dd50ce 100644 --- a/packages/cli/src/lib/pg/setter.ts +++ b/packages/cli/src/lib/pg/setter.ts @@ -3,10 +3,10 @@ import {ux} from '@oclif/core' import {addonResolver} from '../addons/resolve' import host from './host' import {essentialPlan} from './util' -import {FormationSetting, Setting, SettingsResponse} from './types' +import {SettingKey, Setting, SettingsResponse} from './types' export abstract class PGSettingsCommand extends Command { - protected abstract settingKey: FormationSetting + protected abstract settingKey: SettingKey protected abstract convertValue(val: string): unknown protected abstract explain(setting: Setting): string diff --git a/packages/cli/src/lib/pg/types.ts b/packages/cli/src/lib/pg/types.ts index 17b6cde5f7..3b5b449d86 100644 --- a/packages/cli/src/lib/pg/types.ts +++ b/packages/cli/src/lib/pg/types.ts @@ -63,7 +63,7 @@ export type CredentialsInfo = { export type MaintenanceApiResponse = { message: string, } -export type FormationSetting = 'log_lock_waits' | 'log_connections' | 'log_min_duration_statement' | 'log_statement' | 'track_functions' | +export type SettingKey = 'log_lock_waits' | 'log_connections' | 'log_min_duration_statement' | 'log_statement' | 'track_functions' | 'pgbouncer_max_client_conn' | 'pg_bouncer_max_db_conns' | 'pg_bouncer_default_pool_size' | 'auto_explain' | 'auto_explain.log_min_duration' | 'auto_explain.log_analyze' | 'auto_explain.log_triggers' | 'auto_explain.log_buffers' | 'auto_explain.log_verbose' | 'auto_explain.log_nested_statements' @@ -72,4 +72,4 @@ export type Setting = { desc: string default: T } -export type SettingsResponse = Record> +export type SettingsResponse = Record> From 424c70cd849e07fecf55713c13eff96a508fa5ce Mon Sep 17 00:00:00 2001 From: Santiago Bosio Date: Mon, 8 Apr 2024 11:09:02 -0300 Subject: [PATCH 5/5] Refactoring previously migrated settings commands --- packages/cli/src/commands/pg/settings/auto-explain.ts | 7 ++++--- .../commands/pg/settings/auto-explain/log-buffers.ts | 7 ++++--- .../pg/settings/auto-explain/log-min-duration.ts | 11 ++++++----- packages/cli/src/lib/pg/setter.ts | 3 ++- 4 files changed, 16 insertions(+), 12 deletions(-) diff --git a/packages/cli/src/commands/pg/settings/auto-explain.ts b/packages/cli/src/commands/pg/settings/auto-explain.ts index d34047c2fd..934bca2054 100644 --- a/packages/cli/src/commands/pg/settings/auto-explain.ts +++ b/packages/cli/src/commands/pg/settings/auto-explain.ts @@ -1,7 +1,8 @@ import {flags} from '@heroku-cli/command' import {Args} from '@oclif/core' import heredoc from 'tsheredoc' -import {booleanConverter, PGSettingsCommand, type Setting, type SettingKey} from '../../../lib/pg/setter' +import {BooleanAsString, booleanConverter, PGSettingsCommand} from '../../../lib/pg/setter' +import {Setting, SettingKey} from '../../../lib/pg/types' // ref: https://www.postgresql.org/docs/current/auto-explain.html export default class AutoExplain extends PGSettingsCommand { @@ -26,11 +27,11 @@ export default class AutoExplain extends PGSettingsCommand { protected settingKey: SettingKey = 'auto_explain' - protected convertValue(val: boolean): boolean { + protected convertValue(val: BooleanAsString): boolean { return booleanConverter(val) } - protected explain(setting: Setting): string { + protected explain(setting: Setting): string { if (setting.value) { return 'Execution plans of queries will be logged for future connections.' } diff --git a/packages/cli/src/commands/pg/settings/auto-explain/log-buffers.ts b/packages/cli/src/commands/pg/settings/auto-explain/log-buffers.ts index 6ecfd2147c..e5f27a57c7 100644 --- a/packages/cli/src/commands/pg/settings/auto-explain/log-buffers.ts +++ b/packages/cli/src/commands/pg/settings/auto-explain/log-buffers.ts @@ -1,6 +1,7 @@ import {Args} from '@oclif/core' import heredoc from 'tsheredoc' -import {PGSettingsCommand, type Setting, type SettingKey, booleanConverter} from '../../../../lib/pg/setter' +import {BooleanAsString, PGSettingsCommand, booleanConverter} from '../../../../lib/pg/setter' +import {Setting, SettingKey} from '../../../../lib/pg/types' export default class LogBuffersWaits extends PGSettingsCommand { static topic = 'pg' @@ -16,11 +17,11 @@ export default class LogBuffersWaits extends PGSettingsCommand { protected settingKey: SettingKey = 'auto_explain.log_buffers' - protected convertValue(val: boolean): boolean { + protected convertValue(val: BooleanAsString): boolean { return booleanConverter(val) } - protected explain(setting: Setting) { + protected explain(setting: Setting) { if (setting.value) { return 'Buffer statistics have been enabled for auto_explain.' } diff --git a/packages/cli/src/commands/pg/settings/auto-explain/log-min-duration.ts b/packages/cli/src/commands/pg/settings/auto-explain/log-min-duration.ts index 031492b0ee..0c2c43a408 100644 --- a/packages/cli/src/commands/pg/settings/auto-explain/log-min-duration.ts +++ b/packages/cli/src/commands/pg/settings/auto-explain/log-min-duration.ts @@ -1,6 +1,7 @@ import {Args} from '@oclif/core' import heredoc from 'tsheredoc' -import {PGSettingsCommand, type Setting, type SettingKey} from '../../../../lib/pg/setter' +import {PGSettingsCommand, numericConverter} from '../../../../lib/pg/setter' +import {Setting, SettingKey} from '../../../../lib/pg/types' export default class LogMinDuration extends PGSettingsCommand { static topic = 'pg' @@ -14,13 +15,13 @@ export default class LogMinDuration extends PGSettingsCommand { value: Args.integer(), } - protected settingKey:SettingKey = 'auto_explain.log_min_duration' + protected settingKey: SettingKey = 'auto_explain.log_min_duration' - protected convertValue(val: number): number { - return val + protected convertValue(val: string): number { + return numericConverter(val) } - protected explain(setting: Setting) { + protected explain(setting: Setting) { if (setting.value === -1) { return 'Execution plan logging has been disabled.' } diff --git a/packages/cli/src/lib/pg/setter.ts b/packages/cli/src/lib/pg/setter.ts index 26f0dd50ce..ecfd57a001 100644 --- a/packages/cli/src/lib/pg/setter.ts +++ b/packages/cli/src/lib/pg/setter.ts @@ -36,6 +36,7 @@ export abstract class PGSettingsCommand extends Command { const {body: settings} = await this.heroku.get(`/postgres/v0/databases/${db.id}/config`, {hostname: host()}) const setting = settings[this.settingKey] ux.log(`${this.settingKey.replace(/_/g, '-')} is set to ${setting.value} for ${db.name}.`) + ux.log(this.explain(setting)) } } } @@ -59,7 +60,7 @@ export const booleanConverter = (value: BooleanAsString) => { } } -export const numericConverter = (value: string | number) => { +export const numericConverter = (value: string) => { const n = Number(value) if (!Number.isFinite(n)) { throw new TypeError('Invalid value. Valid options are: a numeric value')