@aws-cdk/aws-redshift : Table; new tableColumns ordering not preserved on UPDATE

Romanmc72 commented 2 years ago

Describe the bug

When using the Table construct in the @aws-cdk/aws-redshift module, if you have a table that already exists, then you attempt to modify that table by adding more than one column, then the ordering of those columns is not guaranteed. It works correctly when creating tables because the entire operation is run in one CREATE TABLE SQL statement, however the addition of more than 1 new column at a time will create the columns out of order in a potentially erratic and unpredictable manner. The reason for this is because the addition of new columns is done with parallel asynchronous calls of ALTER TABLE <tableName> ADD COLUMN <columnName> <dataType> SQL statements. This can be seen here executed where the ALTER TABLE statements are added to an array and then here where those statements are executed in an asynchronous fashion against the redshift data API. So whichever request arrives first to the API wins instead of the statements being executed as the developer would expect, being the order they specified in their CDK code.

Expected Behavior

As a user of the Typescript version of this library, Arrays in typescript come ordered (as they do in most languages) so when I have an ordered array of column definitions, my expectation is that that order is preserved when applied to my table and that the columns in my CDK code match the columns in my Redshift table in terms of data type and ordering.

Current Behavior

Currently, if I have a table and add more than one new column to that table at a time, the order of those columns will at random not match with the order I place them in my tableColumns array, even between deployments to different isolated environments that utilized the same templates.

Reproduction Steps

If you only add 2 or 3 columns then there is a chance that they just happen to resolve in the correct order. I have added 10 here and am able to reproduce this bug every time I run it (ran 4 times). The code below worked for me as-is.

With the CDK, create a Redshift Cluster and a table inside of that Redshift Cluster.
With the CDK, Add a table to that redshift cluster with at least 1 column in it.

import {
  Cluster,
  ClusterType,
  NodeType,
  Table,
} from '@aws-cdk/aws-redshift';
import { Vpc } from '@aws-cdk/aws-ec2';
import * as cdk from '@aws-cdk/core';

const APP = new cdk.App();

class RedshiftStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props: cdk.StackProps) {
    super(scope, id, props);
    const vpc = new Vpc(this, 'RedshiftVpc');
    const databaseName = 'testdatabase';
    const cluster = new Cluster(this, databaseName, {
      clusterName: 'testcluster',
      clusterType: ClusterType.SINGLE_NODE,
      defaultDatabaseName: databaseName,
      masterUser: {
        masterUsername: 'admin_user',
      },
      nodeType: NodeType.DC2_LARGE,
      publiclyAccessible: false,
      removalPolicy: cdk.RemovalPolicy.DESTROY,
      vpc: vpc,
    });
    new Table(this, 'TestTable', {
      removalPolicy: cdk.RemovalPolicy.DESTROY,
      cluster,
      databaseName,
      tableName: 'public.test',
      tableColumns: [
        {
          name: 'column_1',
          dataType: 'TEXT',
        },
      ],
    });
  }
}

new RedshiftStack(APP, 'RedshiftStack', {});

Deploy the changes from the above 2 steps.

npm run build && cdk deploy RedshiftStack

4.. Add 10 new columns to that original table.

import {
  Cluster,
  ClusterType,
  NodeType,
  Table,
} from '@aws-cdk/aws-redshift';
import { Vpc } from '@aws-cdk/aws-ec2';
import * as cdk from '@aws-cdk/core';

const APP = new cdk.App();

class RedshiftStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props: cdk.StackProps) {
    super(scope, id, props);
    const vpc = new Vpc(this, 'RedshiftVpc');
    const databaseName = 'testdatabase';
    const cluster = new Cluster(this, databaseName, {
      clusterName: 'testcluster',
      clusterType: ClusterType.SINGLE_NODE,
      defaultDatabaseName: databaseName,
      masterUser: {
        masterUsername: 'admin_user',
      },
      nodeType: NodeType.DC2_LARGE,
      publiclyAccessible: false,
      removalPolicy: cdk.RemovalPolicy.DESTROY,
      vpc: vpc,
    });
    new Table(this, 'TestTable', {
      removalPolicy: cdk.RemovalPolicy.DESTROY,
      cluster,
      databaseName,
      tableName: 'public.test',
      tableColumns: [
        {
          name: 'column_1',
          dataType: 'TEXT',
        },
        {
          name: 'column_2',
          dataType: 'TEXT',
        },
        {
          name: 'column_3',
          dataType: 'TEXT',
        },
        {
          name: 'column_4',
          dataType: 'TEXT',
        },
        {
          name: 'column_5',
          dataType: 'TEXT',
        },
        {
          name: 'column_6',
          dataType: 'TEXT',
        },
        {
          name: 'column_7',
          dataType: 'TEXT',
        },
        {
          name: 'column_8',
          dataType: 'TEXT',
        },
        {
          name: 'column_9',
          dataType: 'TEXT',
        },
        {
          name: 'column_10',
          dataType: 'TEXT',
        },
        {
          name: 'column_11',
          dataType: 'TEXT',
        },
      ],
    });
  }
}

new RedshiftStack(APP, 'RedshiftStack', {});

Deploy the changes from that step.

npm run build && cdk deploy RedshiftStack

Observe the chaotic ordering of those new columns.

You would expect to query the redshift database and see the columns in the numeric order matching what is laid out above in the tableColumns array, however you will get something more like this:

SELECT column_name
FROM information_schema.columns
WHERE table_name = 'test' AND table_schema = 'public'
ORDER BY ordinal_position ASC
;
/* 
-- This is the actual order of the columns after running the exact above code snippets
column_name |
------------+
column_1    |
column_4    |
column_7    |
column_6    |
column_10   |
column_5    |
column_11   |
column_9    |
column_2    |
column_3    |
column_8    |
*/

Feel free to edit the stack, remove the new columns, then read-add them and get a whole new order! I did exactly that and on the second attempt got the following column order:

SELECT column_name
FROM information_schema.columns
WHERE table_name = 'test' AND table_schema = 'public'
ORDER BY ordinal_position ASC
;
/* 
-- This is the actual order of the columns after running the exact same above code snippets a second time!
column_name |
------------+
column_1    |
column_5    |
column_11   |
column_9    |
column_2    |
column_4    |
column_7    |
column_10   |
column_3    |
column_8    |
column_6    |
*/

Possible Solution

There are 2 ways to solve this as I see it.

Synchronize the async calls to executeStatement() performed here.

So instead of:

await Promise.all(alterationStatements.map(statement => executeStatement(statement, tableAndClusterProps)));

it'd be more like:

// You really cannot use the `map` or `forEach` with
// `async`/`await` and expect things to run "in order"
for (let index = 0; index < alterationStatements.length; index++) {
  await executeStatement(alterationStatements[index], tableAndClusterProps);
}

This will take a while to execute and require many round trips to the API, but it would "work".

Or perhaps even better in my opinion, run the calls all together using the BatchExecuteStatement endpoint to group all of the statements together in one transaction, run in order by the redshift-data API. Currently the Table construct only leverages the ExecuteStatement endpoint on the redshift-data API. The shape of the output and response from the batch endpoint matches that of the regular execute endpoint:

// From BatchExecuteStatement
{
  "ClusterIdentifier": "testcluster",
  "CreatedAt": "2022-05-25T09:02:55.390000-05:00",
  "Database": "testdatabase",
  "DbUser": "admin_user",
  "Id": "a0836ead-c510-4ffc-941c-d3a8f5be3daa"
}
// From ExecuteStatement
{
  "ClusterIdentifier": "testcluster",
  "CreatedAt": "2022-05-25T09:03:41.297000-05:00",
  "Database": "testdatabase",
  "DbUser": "admin_user",
  "Id": "0eddb16c-510a-4a9d-afe2-722db7998ed8"
}

and the subsequent call made to waitForStatementComplete could remain unchanged as well. The shape of the response on DescribeStatement changes slightly for the case when one describes a batch execute statement, but not in a way that would break the API response to this function.

// DescribeStatement for BatchExecuteStatement
{
  "ClusterIdentifier": "testcluster",
  "CreatedAt": "2022-05-25T09:02:55.390000-05:00",
  "Duration": 252031968,
  "HasResultSet": true,
  "Id": "a0836ead-c510-4ffc-941c-d3a8f5be3daa",
  "RedshiftPid": 1076886687,
  "RedshiftQueryId": 0,
  "ResultRows": -1,
  "ResultSize": -1,
  "Status": "FINISHED",
  "SubStatements": [
    {
      "CreatedAt": "2022-05-25T09:02:55.556000-05:00",
      "Duration": 252031968,
      "HasResultSet": true,
      "Id": "a0836ead-c510-4ffc-941c-d3a8f5be3daa:1",
      "QueryString": "SELECT COUNT(*) FROM public.test",
      "RedshiftQueryId": 478136,
      "ResultRows": 1,
      "ResultSize": 20,
      "Status": "FINISHED",
      "UpdatedAt": "2022-05-25T09:02:56.393000-05:00"
    }
  ],
  "UpdatedAt": "2022-05-25T09:02:56.455000-05:00"
}
// DescribeStatement for ExecuteStatement
{
  "ClusterIdentifier": "testcluster",
  "CreatedAt": "2022-05-25T09:03:41.297000-05:00",
  "Duration": 7063568,
  "HasResultSet": true,
  "Id": "0eddb16c-510a-4a9d-afe2-722db7998ed8",
  "QueryString": "SELECT COUNT(*) FROM public.test",
  "RedshiftPid": 1076788377,
  "RedshiftQueryId": 478146,
  "ResultRows": 1,
  "ResultSize": 20,
  "Status": "FINISHED",
  "UpdatedAt": "2022-05-25T09:03:41.893000-05:00"
}

The only difference would be the existence of the SubStatements attribute on a batch statement. In effect though, you could just replace the use of the executeStatement endpoint entirely with the batchExecuteStatement one, and change one of the input properties from Sql: string to Sqls: string[] and viola, done. Of course you would need to update everywhere that calls executeStatement to pass in an array of strings not a string, but this would be pretty trivial to fix. The BatchExecuteStatement supports sending 1 or more SQL statements, so it would be compatible with the rest of the single-statement redshift-data API calls as well.

Additional Information/Context

Happy to raise the Pull Request here for this one myself. Would love to know your thoughts on switching from the ExecuteStatement command to the BatchExecuteStatement command as that seems like the best way forward here in my opinion.

Here is my package.json if that helps

{
  "name": "src",
  "version": "0.1.0",
  "bin": {
    "src": "bin/src.js"
  },
  "scripts": {
    "build": "tsc",
    "watch": "tsc -w",
    "test": "jest",
    "cdk": "cdk --profile sandbox",
    "lint": "eslint bin/src.ts lib/*.ts",
    "lint-fix": "eslint bin/src.ts lib/*.ts --fix"
  },
  "devDependencies": {
    "@aws-cdk/assert": "1.137.0",
    "@types/jest": "^26.0.10",
    "@types/node": "10.17.27",
    "@typescript-eslint/eslint-plugin": "^5.9.0",
    "@typescript-eslint/parser": "^5.9.0",
    "aws-cdk": "2.1.0",
    "eslint": "^8.6.0",
    "eslint-config-airbnb-base": "^15.0.0",
    "eslint-plugin-cdk": "^1.6.0",
    "eslint-plugin-import": "^2.25.4",
    "jest": "^26.4.2",
    "ts-jest": "^26.2.0",
    "ts-node": "^9.0.0",
    "typescript": "~3.9.7"
  },
  "dependencies": {
    "@aws-cdk/aws-ec2": "1.137.0",
    "@aws-cdk/aws-glue": "1.137.0",
    "@aws-cdk/aws-iam": "1.137.0",
    "@aws-cdk/aws-kms": "1.137.0",
    "@aws-cdk/aws-redshift": "1.137.0",
    "@aws-cdk/aws-s3": "1.137.0",
    "@aws-cdk/core": "1.137.0",
    "source-map-support": "^0.5.16"
  }
}

I did have eslint set up as you can see above but that is not required for reproducing this bug.

CDK CLI Version

2.1.0 (build f4f18b1)

Framework Version

No response

Node.js Version

v16.15.0

OS

macOS 12.3.1 (21E258)

Language

Typescript

Language Version

TypeScript (3.9.10)

Other information

here is an article on why this happens out of order when doing await Promise.all(array.map()). (there are many such articles out there if you do some light googling)

here is a small nodejs program you can run to see this mess in action for yourself:

/**
 * async "sleep" function to sleep form some number of miliseconds
 * @param {number} ms - The number of miliseconds to sleep for.
 * @return {Promise<void>} - The words of politicians
 */
function sleep(ms) {
  return new Promise((resolve) => {
    setTimeout(resolve, ms);
  });
}

/**
 * Random int, returns a random integer from 0 to 2
 * @return {number} - The random number
 */
function randint() {
  return Math.floor(Math.random() * (3 - 0) + 0);
}

/**
 * "Print It"
 * Log an output to the console with a random delay from 0 - 2 miliseconds. This
 * imitates a very small potential delay in reaching the API.
 * @param {string} it - The "it" to print in "print it"
 * @return {void}
 */
async function printIt(it) {
  await sleep(randint());
  await console.log(it);
};

/**
 * This is the "print all" function intended to print all of the things in an
 * array. It is written to mirror what happens in the Table construct when an
 * update is called to add an array of columns to a table.
 * @param {string[]} cols - The names of the columns to add to the table.
 * @return {Promise<void>} - Nothing really.
 */
function printAll(cols) {
  return Promise.all(cols.map((col) => printIt(col)))
}

/**
 * This is the "print all" function intended to print all of the things in an
 * array. It is written to show what _should_ be happening when one tries to
 * update a table using an array of columns
 * @param {string[]} cols - The names of the columns to add to the table.
 * @return {Promise<void>} - Nothing really.
 */
async function betterPrintAll(cols) {
  for (let index = 0; index < cols.length; index++) {
    await printIt(cols[index]);
  }
}

/**
 * An ordered array of columns
 */
const cols = [
  'ALTER TABLE gorilla.nems ADD COLUMN bing TEXT',
  'ALTER TABLE gorilla.nems ADD COLUMN bong TEXT',
  'ALTER TABLE gorilla.nems ADD COLUMN eff TEXT',
  'ALTER TABLE gorilla.nems ADD COLUMN ya_life TEXT',
]

/** Runs the 2 print statements to compare output */
async function main() {
  console.log("Here is the printAll() funciton, and it is rarely in order. I will run it 3 times and expect 3 different outputs.")
  console.log("#1")
  await printAll(cols);
  console.log("#2")
  await printAll(cols);
  console.log("#3")
  await printAll(cols);
  console.log("")
  console.log("Here is the betterPrintAll() function. I will run it 3 times and it will be in the same, correct order every time.")
  console.log("#1")
  await betterPrintAll(cols);
  console.log("#2")
  await betterPrintAll(cols);
  console.log("#3")
  await betterPrintAll(cols);
}

main();

peterwoodworth commented 2 years ago

Thank you for the very detailed bug description @Romanmc72,

Would you be willing to contribute and help us with this yourself? Check out our contributing guide if you're interested 🙂

Romanmc72 commented 2 years ago

Happy to help! I will take a look there and see where I can get started.

Romanmc72 commented 2 years ago

Let me know how the PR looks, happy to change if need be.

Romanmc72 commented 11 months ago

The PR raised was closed a year ago as this bug was deemed not required due to the best practice of always specifying the order of columns in any SQL one writes. This is a fair callout and as such this is not really a major bug that requires a fix.

I do want to make some additional callouts on the above use of Promise.all() and executeStatement() to run queries in parallel for a given handler. It may not be an issue in this particular case based on best practices but it does cause havoc in another, let me briefly explain relying on the reader to leverage the above examples to see what I mean. The case where we still want to synchronize and or batch some SQL as a transaction is around the UserPrivileges construct. The case where I have encountered issues is where I have more than one user created who need access to a particular table. If I grant access to those users to the same database object during the same cdk deploy then I will almost always get a concurrent transaction conflict. Why is this? Redshift (and underlying postgres) acquire a lock on the permissions for a particular table object when changing them. It grants the privilege by adding g a use to that permission then releases the lock and moves on. In the case of our handler, permissions are attempting to be granted in parallel against the same object so the first one arrives and acquires the lock and the subsequent calls attempt to do the same but fail due to a concurrent transaction conflict. This causes the update to fail and to rollback often with an update rollback failed because the rollback just does the inverse (if grant then it tries revoke or vice versa) which faces the same issue. Moving those operations to a batchExecuteStatement() call would eliminate that error entirely. As I had originally suggested moving everything to the batch statement, I now realize that would be a bad idea for the following reason. The batch execute runs in a transaction but certain DDL statements cannot be run inside a transaction. As such I think the CDK should support both the parallel and batch statement approach for different scenarios because they solve different problems. I would be happy to revise my PR or submit a new one with only these described changes in mind for the user privilege construct.

github-actions[bot] commented 5 months ago

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.

aws / aws-cdk