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

Fix "IF NOT EXISTS" Postgres 8 incompatibility #2228

Merged
merged 1 commit into from
Sep 8, 2017

Conversation

angelinemm
Copy link
Contributor

Fix Postgres 8 incompatibility

Description

Commit a41e4c4 introduced a postgres 8 incompatibility by using IF NOT EXISTS in the sql query that creates the marker table. It was done to simplify code and getting rid of exception handling.

IF NOT EXISTS is supported only since postgres 9.1

Motivation and Context

Postgres 8 is arguably quite an old version but it's the version that amazon's redshift is based on.
It makes sense for companies that use redshift in production to use postgres 8 in development environment.

Have you tested this? If so, how?

I ran the tests suggested here: https://github.com/spotify/luigi/blob/master/CONTRIBUTING.rst
Also this commits is a revert, so it reintroduces code that was already used before.

the closest postgres for redshift is postgres 8.
this means that companies still use postgres 8 to mock a local redshift
on their dev environment.
@mention-bot
Copy link

@angelinemm, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Tarrasch, @ant1j and @smehan to be potential reviewers.

Copy link
Collaborator

@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

Firstly, thanks for submitting your first PR! Welcome!

Secondly, given your use case for Postgres 8 this revert makes sense. While I prefer the cleanliness of if not exists, the use of this older version of postgres is valid (although I'm not sure you'd run into problems running most Redshift SQL in a Postgres 9.x environment).

@dlstadther
Copy link
Collaborator

@ant1j Any strong opinions against reverting your PR #2064 ?

@ant1j
Copy link
Contributor

ant1j commented Sep 8, 2017

If it helps compatibility, no problem.

@dlstadther dlstadther merged commit 65a6293 into spotify:master Sep 8, 2017
@dlstadther
Copy link
Collaborator

Thanks @angelinemm for contributing!

This was referenced Jun 29, 2022
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