Skip to content

Conversation

@manuelurenah
Copy link
Contributor

@manuelurenah manuelurenah commented May 30, 2019

Description

  • In order to handle more advance features in amphora we added new columns in the public tables of postgres db.
  • Adds migration scripts to add createdAt and updatedAt columns with default values
  • Adds migration scripts to add siteId column and update each entry on the table
  • This handles the first part of Additional Columns #33

QA

In order to be able to test this feature, you should link this package and clay/amphora#665 in a clay project. I've been using https://github.com/clay/clay-starter to develop this feature, that's a good starting point.

Testing steps

  1. Go inside a clay project directory (i.e.: clay-starter)
  2. Spin up an instance of clay with make
  3. Open another terminal window or tab and make sure adminer is up and running using docker-compose up -d adminer
  4. Make your way into the app directory (or where the package.json file is located) and link the previously mentioned packages with npm link amphora && npm link amphora-storage-postgres (make sure the packages are in the correct branch and linked)
  5. Restart the clay instance with rs. After it's done, you should a message stating Migrations Complete
  6. Go into http://localhost:8080 an login the db (system, server, username and database are all postgres; password is example)
  7. Check that the schemas of all public tables have the following columns: created_at, which should default to the date the row was created; updated_at, which should default to the date the row was created and update itself if the data changes; and site_id, which should contain the site slug
  8. Bootstrap the starter data with make boostrap (where the Makefile is located)
  9. You should be able to see in all the public tables new entries with all the previous columns plus the new ones.
  10. You should be able to update, publish, archive and everything else without ay problems

@manuelurenah manuelurenah changed the title Feature: Adds createdAt, updatedAt and siteId columns in postgres [WIP] Feature: Adds createdAt, updatedAt and siteId columns in postgres Jun 5, 2019
@manuelurenah manuelurenah changed the title [WIP] Feature: Adds createdAt, updatedAt and siteId columns in postgres Feature: Adds createdAt, updatedAt and siteId columns in postgres Jun 6, 2019
$$ LANGUAGE plpgsql;

ALTER TABLE IF EXISTS uris
ADD COLUMN IF NOT EXISTS created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(),
Copy link
Member

Choose a reason for hiding this comment

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

I think the NOT NULL constraint on these columns has the potential to have a major performance penalty in postgres versions before 11. If anyone is running an earlier version, we'll have to break this step down in to a few steps.

Copy link
Member

@james-owen james-owen left a comment

Choose a reason for hiding this comment

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

I think this change would require dropping the migrations table and rebuilding it.

var commands = [], url;

for (let i = 0; i < ops.length; i++) {
for (let i = 0, opsLength = ops.length; i < opsLength; i++) {

Choose a reason for hiding this comment

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

Why are we using opsLength here and not ops.length?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done for loop optimization, we assign the value of length to a property to avoid having to look it up at every iteration. You can read more about it here: https://jaysoo.ca/2009/12/23/javascript-optimizing-loops/

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.

4 participants