Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ The rest are keyword args with the following mappings:
- `premake` -> `p_premake`. Required: `false`. Partman defaults to `4`.
- `start_partition` -> `p_start_partition`. Required: `false`. Partman defaults to the current timestamp.

> **Note:** We have chosen to require PostgreSQL 11+ and hardcode `p_type` to `native` for simplicity, as previous PostgreSQL versions are end-of-life.
> **Note:** We have chosen to require PostgreSQL 11+ and hardcode `p_type` to `native` (`range` in the case of Partman 5) for simplicity, as previous PostgreSQL versions are end-of-life.

Additionally, this method allows you to configure a subset of attributes on the record stored in the [part\_config](https://github.com/pgpartman/pg_partman/blob/master/doc/pg_partman.md#tables) table.
These options are delegated to the `unsafe_partman_update_config` method to update the record:
Expand All @@ -397,7 +397,7 @@ safe_create_partitioned_table :table, type: :range, partition_key: :created_at d
t.timestamps null: false
end

safe_partman_create_parent :table, partition_key: :created_at, interval: "weekly"
safe_partman_create_parent :table, partition_key: :created_at, interval: "1 week"
```

With custom overrides:
Expand All @@ -415,7 +415,7 @@ end

safe_partman_create_parent :table,
partition_key: :created_at,
interval: "weekly",
interval: "1 week",
template_table: :table_template,
premake: 10,
start_partition: Time.current + 1.month,
Expand Down Expand Up @@ -589,6 +589,7 @@ end
- `prefer_single_step_column_addition_with_default`: If true, raise an error when adding a column and separately setting a constant default value for that column in the same migration. Default: `true`
- `allow_force_create_table`: If false, the `force: true` option to ActiveRecord's `create_table` method is disallowed. Default: `false`
- `infer_primary_key_on_partitioned_tables`: If true, the primary key for partitioned tables will be inferred on PostgreSQL 11+ databases (identifier column + partition key columns). Default: `true`
- `partman_5_compatibility_mode`: If true, `safe_partman_create_parent` will raise an error if the user provides an interval that is [not supported by Partman 5](https://github.com/pgpartman/pg_partman/blob/v5.2.4/sql/functions/create_parent.sql#L86-L96). If the interval is supported, the method will ensure table name suffixes match the Partman 5 format (`YYYYMMDD`, `YYYYMMDD_HTH24MISS`). Default: `false`
Copy link
Contributor

Choose a reason for hiding this comment

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

When will we plan to flip this to true? I'm not sure we'd want to wait until a major version of this gem, but...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In reality, I wouldn't expect there to be a ton of users in the community utilizing the partman functionality in this gem. On the BT side of things, I'm imagining we would flip this to true in our initializers very soon. So I think we should just follow semantic versioning and wait until 3.0 to flip the default to true. Honestly, in 3.0 we could probably just remove this config entirely and drop support for partman 4


### Rake Tasks

Expand Down
15 changes: 14 additions & 1 deletion lib/pg_ha_migrations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ module PgHaMigrations
:allow_force_create_table,
:prefer_single_step_column_addition_with_default,
:infer_primary_key_on_partitioned_tables,
:partman_5_compatibility_mode,
)

def self.config
Expand All @@ -22,7 +23,8 @@ def self.config
true,
false,
true,
true
true,
false,
)
end

Expand All @@ -44,6 +46,17 @@ def self.configure
retention_keep_table
]

PARTMAN_UNSUPPORTED_INTERVALS = %w[
yearly
quarterly
monthly
weekly
daily
hourly
half-hour
quarter-hour
]

# Safe versus unsafe in this context specifically means the following:
# - Safe operations will not block for long periods of time.
# - Unsafe operations _may_ block for long periods of time.
Expand Down
14 changes: 14 additions & 0 deletions lib/pg_ha_migrations/safe_statements.rb
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,16 @@ def safe_partman_create_parent(

raise PgHaMigrations::MissingExtensionError, "The pg_partman extension is not installed" unless partman_extension.installed?

if partman_extension.major_version >= 5 || PgHaMigrations.config.partman_5_compatibility_mode
if PgHaMigrations::PARTMAN_UNSUPPORTED_INTERVALS.include?(interval)
raise PgHaMigrations::InvalidMigrationError,
"Special partition interval values (#{interval}) are no longer supported. " \
"Please use a supported interval time value from core PostgreSQL " \
"#{(partman_extension.major_version < 5 ? "or turn partman 5 compatibility mode off " : "")}" \
"(https://www.postgresql.org/docs/current/datatype-datetime.html#DATATYPE-INTERVAL-INPUT)"
end
end

formatted_start_partition = nil

if start_partition.present?
Expand Down Expand Up @@ -531,6 +541,10 @@ def safe_partman_create_parent(
}.compact

unsafe_partman_update_config(table, **update_config_options)

if PgHaMigrations.config.partman_5_compatibility_mode && partman_extension.major_version < 5
unsafe_partman_standardize_partition_naming(table)
end
end

def safe_partman_update_config(table, **options)
Expand Down
16 changes: 8 additions & 8 deletions spec/partman_renaming_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -431,13 +431,13 @@ def up

expect(before_part_config).to have_attributes(
partition_interval: "P7D",
datetime_string: "IYYY\"w\"IW",
datetime_string: "YYYY_MM_DD",
partition_type: "partman",
automatic_maintenance: "on",
)

expect(TestHelpers.partitions_for_table(:foos3, exclude_default: true))
.to all(match(/^foos3_p\d{4}w\d{2}$/))
.to all(match(/^foos3_p\d{4}_\d{2}_\d{2}$/))

expect do
migration.suppress_messages { migration.migrate(:up) }
Expand All @@ -450,13 +450,13 @@ def up

expect(after_part_config).to have_attributes(
partition_interval: "P7D",
datetime_string: "IYYY\"w\"IW",
datetime_string: "YYYY_MM_DD",
partition_type: "partman",
automatic_maintenance: "on",
)

expect(TestHelpers.partitions_for_table(:foos3, exclude_default: true))
.to all(match(/^foos3_p\d{4}w\d{2}$/))
.to all(match(/^foos3_p\d{4}_\d{2}_\d{2}$/))
end

it "raises error when complex partition interval provided" do
Expand Down Expand Up @@ -520,13 +520,13 @@ def up

expect(before_part_config).to have_attributes(
partition_interval: "PT0.5S",
datetime_string: "IYYY\"w\"IW",
datetime_string: "YYYY_MM_DD",
partition_type: "native",
automatic_maintenance: "on",
)

expect(TestHelpers.partitions_for_table(:foos3, exclude_default: true))
.to all(match(/^foos3_p\d{4}w\d{2}$/))
.to all(match(/^foos3_p\d{4}_\d{2}_\d{2}$/))

expect do
migration.suppress_messages { migration.migrate(:up) }
Expand All @@ -539,13 +539,13 @@ def up

expect(after_part_config).to have_attributes(
partition_interval: "PT0.5S",
datetime_string: "IYYY\"w\"IW",
datetime_string: "YYYY_MM_DD",
partition_type: "native",
automatic_maintenance: "on",
)

expect(TestHelpers.partitions_for_table(:foos3, exclude_default: true))
.to all(match(/^foos3_p\d{4}w\d{2}$/))
.to all(match(/^foos3_p\d{4}_\d{2}_\d{2}$/))
end

it "raises error and rolls back transaction when statement timeout exceeded" do
Expand Down
18 changes: 14 additions & 4 deletions spec/pg_ha_migrations_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,6 @@
end

describe "config" do
after(:each) do
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unrelated cleanup, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sort of. We moved this hook into the spec helper so the config universally gets reset after each test. We did this so we could easily set PgHaMigrations.config.partman_5_compatibility_mode = true in our new tests. However... looking more closely, it appears that other tests simply stub out values for config options. Should we follow that pattern and revert this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we follow that pattern and revert this change?

We decided that, yes, we should. Pushed up a commit to address

PgHaMigrations.instance_variable_set(:@config, nil)
end

describe "disable_default_migration_methods" do
it "is set to true by default" do
expect(PgHaMigrations.config.disable_default_migration_methods).to be(true)
Expand Down Expand Up @@ -83,6 +79,20 @@
expect(PgHaMigrations.config.infer_primary_key_on_partitioned_tables).to be(false)
end
end

describe "partman_5_compatibility_mode" do
it "is set to false by default" do
expect(PgHaMigrations.config.partman_5_compatibility_mode).to be(false)
end

it "can be overriden to true" do
PgHaMigrations.configure do |config|
config.partman_5_compatibility_mode = true
end

expect(PgHaMigrations.config.partman_5_compatibility_mode).to be(true)
end
end
end

PgHaMigrations::AllowedVersions::ALLOWED_VERSIONS.each do |migration_klass|
Expand Down
Loading