#4338 pg schema upgrade #4375

paulovitin · 2017-11-24T19:39:29Z

Fix this problem #4338

When the Postgres Storage Adapter perform the initialization, and the "Volatile Classes Schemas" perform the table creation, the schemaUpgrade method execute a diff between database columns and the Schemas in code. If has new columns, create then through method addFieldIfNotExists.

montymxb · 2017-11-24T20:22:48Z

I just retriggered the build, should be ok. I still need to compare this against the actual upgrade issue and verify that it does in fact rectify that issue.

codecov · 2017-11-24T20:31:47Z

Codecov Report

Merging #4375 into master will increase coverage by <.01%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master    #4375      +/-   ##
==========================================
+ Coverage   92.78%   92.79%   +<.01%     
==========================================
  Files         118      118              
  Lines        8373     8384      +11     
==========================================
+ Hits         7769     7780      +11     
  Misses        604      604

Impacted Files	Coverage Δ
...dapters/Storage/Postgres/PostgresStorageAdapter.js	`97.01% <93.75%> (+0.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc6a2fd...4b348ad. Read the comment docs.

montymxb · 2017-11-25T00:48:08Z

spec/PostgresStorageAdapter.spec.js

+
+    adapter.createTable(className, schema)
+      .then(() => {
+        schema.fields.expiration_interval = { type:'Number' };


Just to be pedantic can we** getColumns** and check before the schema is upgraded to ensure that it's not present before hand?

montymxb · 2017-11-25T00:53:25Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

+            return list;
+          }
+
+          const field = schema.fields[fieldName];


Can you elaborate on this change here?

I did not understand what you said with "elaborate on this change here".. :)

Hmm, I'll reword it. Could you explain the reason you made the changes above here? I'm not entirely sure why you added this at first glance.

Specifically what about the prior functionality without ...!{}.hasOwnProperty.call(schema.fields, fieldName).. wasn't working and needed to be changed?

montymxb

Overall this is super! I got around to checking the upgrade issue and this handles it without a problem 👍 . Just a test thing and a question from me.

montymxb

Excellent, thanks for making those changes.

I'm going to update against the master to factor in appveyor. No guarantees on that, as it may be still in the works for windows support...

paulovitin · 2017-11-26T22:49:27Z

@montymxb I need to do something here?

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

paulovitin · 2017-12-04T13:29:34Z

@montymxb How are we staying here?

montymxb

@paulovitin We're looking better now. Got caught up with other things on github and beyond 😄 . Didn't realize that the last merge against master corrected those issues we were seeing.

Two things are left. One is that the reported test coverage is not yet up to spec, currently at 89.47%. We'll need you to account for untested paths to bring that to 100% or as close as you reasonable can.

The second being the note about using transactions. As for those I'm actually not too familiar with whether it would be beneficial to implement them here. @vitaly-t, per @dplewis 's note about potentially utilizing transactions in this PR, do you think there would be an added benefit to doing so?

vitaly-t · 2017-12-04T23:57:57Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

+  schemaUpgrade(className, schema) {
+    debug('schemaUpgrade', { className, schema });
+    return this._client.tx('schemaUpgrade', t =>
+      t.any(`SELECT column_name FROM information_schema.columns WHERE table_name = '${className}'`)


do not format the values manually, use the query-formatting engine for it

vitaly-t · 2017-12-04T23:58:29Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

+    )
+      .then(columns => {
+        if (!columns) {
+          return Promise.resolve();


this condition will never happen, see the API for method any.

vitaly-t · 2017-12-04T23:59:03Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

+          return Promise.resolve();
+        }
+
+        const currentColumns = columns.map(item => item.column_name);


perhaps should just use method map of the database instead.

vitaly-t · 2017-12-04T23:59:42Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

+
+        columnsToCreate.forEach(fieldName => {
+          const type = schema.fields[fieldName];
+          promise.push(this.addFieldIfNotExists(className, fieldName, type));


this is bad connection usage, must use a transaction here.

vitaly-t · 2017-12-05T00:01:10Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

+          promise.push(this.addFieldIfNotExists(className, fieldName, type));
+        });
+
+        return Promise.all(promise);


Never use Promise.all on database operations, see: https://github.com/vitaly-t/pg-promise/wiki/Common-Mistakes#tasks-versus-rootdirect-queries

paulovitin · 2017-12-05T01:33:37Z

@vitaly-t I updated the code. Can you take a look?

@montymxb coverage achived! :)

montymxb · 2017-12-07T23:48:17Z

@paulovitin thanks for getting the coverage up! I'll take a look at these changes now...

montymxb · 2017-12-08T00:14:23Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

+    return this._client.tx('schemaUpgrade', t => {
+      return t.any('SELECT column_name FROM information_schema.columns WHERE table_name = $<className>', { className })
+        .then(columns => {
+          const currentColumns = columns.map(item => item.column_name);


So I see this is the only bit you didn't change here. As is this is passing our tests and looks good. Perhaps it's fine as is. @vitaly-t do you think this is important enough to change or are we good as is?

@vitaly-t Ding? Zing? Ping?

This code should be replaced with ES6 generators, the way it was done for all tasks and transaction in the code.

@dplewis ping back at ya 😄

montymxb

Looks good. I want to double check on the one change that wasn't made before I can consider approving this.

paulovitin · 2017-12-08T01:11:22Z

@montymxb ok. If needs more changes, let me know

vitaly-t · 2017-12-30T13:16:19Z

spec/PostgresStorageAdapter.spec.js

+const getColumns = (client, className) => {
+  return client.any('SELECT column_name FROM information_schema.columns WHERE table_name = $<className>', { className })
+    .then(columns => {
+      if (!columns.length) {


What you do in this .then block is pointless, i.e. if the condition matches, the you already end up with an empty array.

vitaly-t · 2017-12-30T13:17:14Z

spec/PostgresStorageAdapter.spec.js

@@ -1,6 +1,17 @@
 const PostgresStorageAdapter = require('../src/Adapters/Storage/Postgres/PostgresStorageAdapter');
 const databaseURI = 'postgres://localhost:5432/parse_server_postgres_adapter_test_database';

+const getColumns = (client, className) => {


This whole method can be implemented with a single line, using method map.

return client.map('SELECT column_name FROM information_schema.columns WHERE table_name = $<className>', { className }, a => a.column_name);

Also, I'm not sure we should have methods that take client as parameter like that. See how all the other methods are done, they take an optional connection context as the last parameter.

This is a pretty simple one. We should setup map and save some lines 😉. You've already done quite a lot so let me know if you can get around to this @paulovitin . If not I can put in an additional commit myself.

vitaly-t · 2017-12-30T13:24:58Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

+    debug('schemaUpgrade', { className, schema });
+
+    return this._client.tx('schemaUpgrade', t => {
+      return t.any('SELECT column_name FROM information_schema.columns WHERE table_name = $<className>', { className })


Method map should be used here.

vitaly-t · 2017-12-30T13:26:11Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

+            addFields.push(this.addFieldIfNotExists(className, fieldName, type));
+          });
+
+          if (!addFields.length) {


Inefficient use of promises here. Can simply skip returning anything for the same effect.

vitaly-t · 2017-12-30T15:04:12Z

This needs a few updates + conflicts resolved.

vitaly-t · 2017-12-30T15:17:39Z

Correction. No, it needs a lot of rewriting, especially in the light of the conflicts.

I have tried to update it, but the conflicts with the later changes stopped me from being able to finish.

@paulovitin You need to resolve the conflicts before I can help with the code refactoring.

Adds: - schemaUpgrade method which check the fields to add and delete from old schema;

Adds: - Add PostgresStorageAdapter.schemaUpgrade spec tests: creates a table, simulates the addition of a new field and checks if its present in the database Chores: - ESLint eol-last fix;

rewriting method schemaUpgrade

vitaly-t · 2017-12-30T17:09:04Z

Ok, I'm done here, for now :) @paulovitin I hope I didn't miss anything ;)

vitaly-t · 2017-12-30T17:27:02Z

@montymxb I've done my part here, do you want to re-review it? 😄

dplewis · 2017-12-30T18:42:06Z

LGTM!

montymxb · 2017-12-31T01:33:01Z

@vitaly-t absolutely 👍 . Thanks for taking a look, postgres via node isn't exactly one of my strong suites 😆 .

montymxb

Relevant notes are in the review. You've been very helpful with accommodating to our requests, just wanted to thank you on that 😄 . If you think you can get around to these last ones here I believe we should be at a point to take this in. I would be willing to put in the last set of changes, should it be necessary.

Code is functional and tests indicate proper behavior. Anything that is non-critical can be applied in later PRs once we have this in. Big deal here is making sure we get this in place to prevent upgrade crashes as originally noticed.

montymxb · 2017-12-31T01:39:49Z

spec/PostgresStorageAdapter.spec.js

@@ -1,6 +1,17 @@
 const PostgresStorageAdapter = require('../src/Adapters/Storage/Postgres/PostgresStorageAdapter');
 const databaseURI = 'postgres://localhost:5432/parse_server_postgres_adapter_test_database';

+const getColumns = (client, className) => {


This is a pretty simple one. We should setup map and save some lines 😉. You've already done quite a lot so let me know if you can get around to this @paulovitin . If not I can put in an additional commit myself.

montymxb · 2017-12-31T01:41:03Z

spec/PostgresStorageAdapter.spec.js

@@ -19,4 +30,100 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
    expect(adapter._client.$pool.ending).toEqual(true);
    done();
  });
+
+  it('schemaUpgrade, upgrade the database schema when schema change', done => {


Just a typo

upgrade the database schema when the schema changes

montymxb · 2017-12-31T01:42:13Z

spec/PostgresStorageAdapter.spec.js

+      .catch(error => done.fail(error));
+  });
+
+  it('schemaUpgrade, matain correct schema', done => {


typo here too

maintains correct schema

montymxb · 2017-12-31T01:47:28Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

@@ -1579,14 +1595,17 @@ export class PostgresStorageAdapter {
  }

  performInitialization({ VolatileClassesSchemas }) {
+    // TODO: This method needs to be rewritten to make proper use of connections (@vitaly-t)


Regarding this TODO we'll want to at least rework this in with transactions as mentioned below.

I will do it 😉

P.S. As for the last item, the TODO thing, it definitely should be done as a separate PR. I will take care of it after this PR.

Noted! @paulovitin you're good for that one then, we'll address that down the road.

vitaly-t · 2017-12-31T02:09:31Z

A-a-and we're back into conflicts 😢

While we were fixing this PR, the code was turned into some sort of TypeScript, holly 💩 ! 😂

@paulovitin man, your PR is jinxed 😂

montymxb · 2017-12-31T02:25:17Z

Hmm, probably related to #4349 being recently merged?

flovilmart · 2017-12-31T03:03:11Z

@vitaly-t yup, intoduced types for the DB adapters and I had many conflicts to survive as well :)

vitaly-t · 2017-12-31T03:05:21Z

Well, I volunteer @paulovitin to resolve the conflicts yet again 😂 I'm not doing it 😂

I'm drinking to the New Year already, as I am Russian 😄 🍻

dplewis · 2017-12-31T03:09:05Z

@vitaly-t we have a very diverse team. New Years Goal: let’s get the team together sometime. Florent set it up. Let’s go to a conference or something

flovilmart · 2017-12-31T03:09:25Z

@vitaly-t no need to do it! Just fixed them from the phone :)

flovilmart · 2017-12-31T03:10:49Z

We have plenty of contrived on open collective, we could probab’y do something to help everyone get there :)

dplewis · 2017-12-31T04:05:15Z

@flovilmart Make it happen captain

vitaly-t · 2017-12-31T18:51:19Z

I did re-run of those flimsy tests a number of times till they finally all came through 😂

montymxb · 2018-01-02T22:51:38Z

spec/PostgresStorageAdapter.spec.js

+const getColumns = (client, className) => {
+  return client.map('SELECT column_name FROM information_schema.columns WHERE table_name = $<className>', { className }, a => a.column_name);
+};
+


Didn't realize the above was finally changed, alright!

montymxb · 2018-01-02T22:51:54Z

spec/PostgresStorageAdapter.spec.js

@@ -19,4 +23,100 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
    expect(adapter._client.$pool.ending).toEqual(true);
    done();
  });
+
+  it('schemaUpgrade, upgrade the database schema when schema changes', done => {


typo fixed...

montymxb · 2018-01-02T22:52:00Z

spec/PostgresStorageAdapter.spec.js

+      .catch(error => done.fail(error));
+  });
+
+  it('schemaUpgrade, maintain correct schema', done => {


typo fixed...

montymxb

Sweet, thanks @vitaly-t for those commits. Finally took a look and was going to do it myself, only to see it's already been done!

All the nits from before look to be taken care of.

montymxb · 2018-01-03T05:08:58Z

Thanks again @paulovitin for putting up the PR!

@vitaly-t

* Method to upgrade schemas in Postgres; Adds: - schemaUpgrade method which check the fields to add and delete from old schema; * Remove the columns delete in schemaUpgrade method; * ESLint fix and PostgresStorageAdapter.schemaUpgrade spec test Adds: - Add PostgresStorageAdapter.schemaUpgrade spec tests: creates a table, simulates the addition of a new field and checks if its present in the database Chores: - ESLint eol-last fix; * Add check columns before and after schema upgrade, and remove the unnecessary piece of code Add: - Check the right columns is present before schema upgrade and the new field is not, then check if the right columns is present and the new field; Remove: - Piece of code unnecessary because it not need to remove diff columns; * Optimize the schemaUpgrade method following @vitaly-t instructions, and more tests; * If the class does not have any columns and needs an upgrade the code would return without doing so. Fixed this. Chore: - Allows class with no column to be upgraded; - Test for class with no columns being upgraded; * Update PostgresStorageAdapter.js rewriting method schemaUpgrade * Update PostgresStorageAdapter.spec.js * Update PostgresStorageAdapter.spec.js

montymxb self-assigned this Nov 24, 2017

montymxb reviewed Nov 25, 2017

View reviewed changes

montymxb suggested changes Nov 25, 2017

View reviewed changes

montymxb reviewed Nov 25, 2017

View reviewed changes

dplewis reviewed Nov 26, 2017

View reviewed changes

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js Show resolved Hide resolved

montymxb suggested changes Dec 4, 2017

View reviewed changes

vitaly-t reviewed Dec 4, 2017

View reviewed changes

vitaly-t reviewed Dec 5, 2017

View reviewed changes

montymxb reviewed Dec 8, 2017

View reviewed changes

vitaly-t reviewed Dec 30, 2017

View reviewed changes

paulovitin added 3 commits December 30, 2017 13:39

Method to upgrade schemas in Postgres;

ce984b8

Adds: - schemaUpgrade method which check the fields to add and delete from old schema;

Remove the columns delete in schemaUpgrade method;

c87d4d5

ESLint fix and PostgresStorageAdapter.schemaUpgrade spec test

9ed3536

Adds: - Add PostgresStorageAdapter.schemaUpgrade spec tests: creates a table, simulates the addition of a new field and checks if its present in the database Chores: - ESLint eol-last fix;

Update PostgresStorageAdapter.js

6f5db46

rewriting method schemaUpgrade

montymxb suggested changes Dec 31, 2017

View reviewed changes

vitaly-t added 2 commits December 31, 2017 01:59

Update PostgresStorageAdapter.spec.js

fe54648

Update PostgresStorageAdapter.spec.js

6ceee07

Merge branch 'master' into fix/4338-pg-schema-upgrade

d80eb2f

Merge branch 'master' into fix/4338-pg-schema-upgrade

4b348ad

montymxb reviewed Jan 2, 2018

View reviewed changes

montymxb approved these changes Jan 2, 2018

View reviewed changes

vitaly-t merged commit cb8f038 into parse-community:master Jan 3, 2018

dplewis mentioned this pull request Jan 9, 2018

Schema Upgrade Issue (Postgres) #4338

Closed

mtrezza mentioned this pull request Jun 25, 2020

Add idempotency #6748

Merged

9 tasks

dplewis mentioned this pull request May 8, 2022

refactor: upgrade @graphql-tools/links from 8.2.11 to 8.2.13 #7989

Closed

cbaker6 mentioned this pull request Jun 25, 2023

refactor: Improve Postgres init to reuse connection #8663

Open

1 task

#4338 pg schema upgrade #4375

#4338 pg schema upgrade #4375

Conversation

paulovitin commented Nov 24, 2017

montymxb commented Nov 24, 2017

codecov bot commented Nov 24, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulovitin Nov 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montymxb left a comment

Choose a reason for hiding this comment

montymxb left a comment

Choose a reason for hiding this comment

paulovitin commented Nov 26, 2017

paulovitin commented Dec 4, 2017

montymxb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulovitin commented Dec 5, 2017 • edited Loading

montymxb commented Dec 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitaly-t Dec 30, 2017 • edited Loading

Choose a reason for hiding this comment

montymxb left a comment

Choose a reason for hiding this comment

paulovitin commented Dec 8, 2017

Choose a reason for hiding this comment

vitaly-t Dec 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitaly-t commented Dec 30, 2017 • edited Loading

vitaly-t commented Dec 30, 2017 • edited Loading

vitaly-t commented Dec 30, 2017 • edited Loading

vitaly-t commented Dec 30, 2017 • edited Loading

dplewis commented Dec 30, 2017

montymxb commented Dec 31, 2017

montymxb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitaly-t Dec 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitaly-t commented Dec 31, 2017 • edited Loading

montymxb commented Dec 31, 2017

flovilmart commented Dec 31, 2017

vitaly-t commented Dec 31, 2017 • edited Loading

dplewis commented Dec 31, 2017

flovilmart commented Dec 31, 2017

flovilmart commented Dec 31, 2017

dplewis commented Dec 31, 2017

vitaly-t commented Dec 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montymxb left a comment

Choose a reason for hiding this comment

montymxb commented Jan 3, 2018

codecov bot commented Nov 24, 2017 •

edited

Loading

paulovitin Nov 25, 2017 •

edited

Loading

montymxb left a comment •

edited

Loading

paulovitin commented Dec 5, 2017 •

edited

Loading

vitaly-t Dec 30, 2017 •

edited

Loading

vitaly-t Dec 30, 2017 •

edited

Loading

vitaly-t commented Dec 30, 2017 •

edited

Loading

vitaly-t commented Dec 30, 2017 •

edited

Loading

vitaly-t commented Dec 30, 2017 •

edited

Loading

vitaly-t commented Dec 30, 2017 •

edited

Loading

vitaly-t Dec 31, 2017 •

edited

Loading

vitaly-t commented Dec 31, 2017 •

edited

Loading

vitaly-t commented Dec 31, 2017 •

edited

Loading