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

Draft: migration generator & unlogged tables for PG #20

Closed
wants to merge 2 commits into from

Conversation

skatkov
Copy link
Contributor

@skatkov skatkov commented Feb 26, 2023

This PR introduces couple of changes:

  • generates migration in rails, that adds table required for cache to work
  • introduces unlogged tables for postrgesql

I got stuck after running rails generate active_support:cache:database:install proper migration file coudn't be found. I seem to messed up something with folders/names.

@skatkov skatkov marked this pull request as draft February 26, 2023 23:23
@skatkov skatkov changed the title first caching version Draft: first caching version Feb 26, 2023
@skatkov skatkov changed the title Draft: first caching version Draft: migration generator & unlogged tables Feb 27, 2023
@skatkov skatkov changed the title Draft: migration generator & unlogged tables Draft: migration generator & unlogged tables for PG Feb 27, 2023
@@ -11,6 +11,12 @@ Tested with:
- SQlite3
- MySQL/MariaDB

## Installation
Add this lin to you application's Gemfile:
Copy link
Member

Choose a reason for hiding this comment

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

sorry, typo + there needs to be a space after the header

@@ -11,6 +11,12 @@ Tested with:
- SQlite3
- MySQL/MariaDB

## Installation
Add this lin to you application's Gemfile:
`gem 'fork-activesupport-cache-database'`
Copy link
Member

Choose a reason for hiding this comment

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

could you pls use remote the fork-?

Copy link
Member

Choose a reason for hiding this comment

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

also, this should be a code block (not inline code), use three backticks pls and leave spaces before and after.

Add this lin to you application's Gemfile:
`gem 'fork-activesupport-cache-database'`
And run:
`rails generate cache:database:install`
Copy link
Member

Choose a reason for hiding this comment

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

same here, three backticks with spaces around pls

end

def copy_migrations
if postgresql?
Copy link
Member

Choose a reason for hiding this comment

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

k, I would prefer to remove this, instead use a single migration file, see notes below

# - No replication to read replicas
#
# Since we're working with cache here, these drawbacks are accaptable. But you can turn that off, just comment next line out.
ActiveRecord::Base.connection.execute("ALTER TABLE activesupport_cache_entries SET UNLOGGED")
Copy link
Member

Choose a reason for hiding this comment

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

make this file your default migration, please just comment out this line too and add a # PostgreSQL only comment somewhere above. I still feel that UNLOGGED tables is not what the majority of people would want, so it should be commented out by default

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.

2 participants