Skip to content

Conversation

zzet
Copy link
Contributor

@zzet zzet commented Oct 10, 2012

What about this solution?

  1. After patch AR Postgres users nothing to lose, since Postgres is no limited the :text column type. Now we can safely eat from schema.rb which was generated for mysql
  2. Added support for using string FK with integer PK

That's all.

@NARKOZ
Copy link
Contributor

NARKOZ commented Oct 10, 2012

👍

@dzaporozhets
Copy link
Contributor

Yeah! I like this solution
👍

@zzet
Copy link
Contributor Author

zzet commented Oct 10, 2012

I now clean code.

Copy link
Contributor

Choose a reason for hiding this comment

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

not sqllite but sqlite

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed. Sorry

dzaporozhets added a commit that referenced this pull request Oct 10, 2012
@dzaporozhets dzaporozhets merged commit 2d2ffc6 into gitlabhq:master Oct 10, 2012
@dzaporozhets
Copy link
Contributor

merged :)

@zzet
Copy link
Contributor Author

zzet commented Oct 10, 2012

@randx, thx, или по-русски говоря - большое, человеческое спасибо :)

@zzet zzet mentioned this pull request Oct 10, 2012
@dzaporozhets
Copy link
Contributor

@zzet :) или по русски "улыбочка"

Copy link
Contributor

Choose a reason for hiding this comment

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

INOUT - TYPO?

Copy link

Choose a reason for hiding this comment

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

No, INOUT is correct (cf. docs):

Parameters
WITH INOUT
Indicates that the cast is an I/O conversion cast, performed by invoking the output function of the source data type, and passing the resulting string to the input function of the target data type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vsizov this solution not good, but it is better, what we can do at this moment.

INOUT - correct, but needed with super user. For many other solutions - it's awful, but it is normal for Gitlab as the decision itself - the product and the infrastructure is controlled by administrators.

@dmke thanks for answer

@jirutka
Copy link
Contributor

jirutka commented Oct 12, 2012

These are basically the same fixes proposed in #554 which were rejected as “hacks”…

There’s still the same issue with the CAST of integer – you need admin rights in PostgreSQL (user “postgres” by default) to do that and it’s not propagated (and can not be) to schema.rb so you have to run migrations to apply it (rake gitlab:app:setup is not sufficient). However, this solution works, I’m using it as well, but it’s not nice at all. It should be noted that it’s a bug in ARel / Rails with polymorphic associations which causes this problem.

There’s still the issue with wiki pages I’ve described in #1489 which you didn’t respond to. You also should know that this functionality is not covered by tests, that’s why it seems to be ok now…

I’m also very unhappy with your final answer to #554 – this is not about “hacking” GitLab to work on PostgreSQL but fixing it to work on any proper SQL database! It’s simply broken and it works on MySQL and SQLite just because it violates SQL standard. However, this is mainly ARel’s fault, not GitLab’s.

Sorry, this kinda raised my blood pressure this morning.

@zzet
Copy link
Contributor Author

zzet commented Oct 12, 2012

As @jirutka said:

It should be noted that it’s a bug in ARel / Rails with polymorphic associations which causes this problem.

Do you know, what one developer make PR to Rails with fix this bug?

I'm control this moment.

As @jirutka said:

There’s still the same issue with the CAST of integer – you need admin rights in PostgreSQL (user “postgres” by default) to do that

Administrator rights are not required to do so, if you implement a function for conversion.

As @jirutka said:

and it’s not propaged (and can not be) to schema.rb so you have to run migrations to apply it

Bad solution... Zero migration may be, but not run migration.

As @jirutka said:

There’s still the issue with wiki pages I’ve described in #1489 which you didn’t respond to. You also should know that this funcionality is not covered by tests, that’s why it seems to be ok now…

I think, that some messages not to public publications

As @jirutka said:

I’m also very unhappy with your final answer to #554 – this is not about “hacking” GitLab to work on PostgreSQL but fixing it to work on any proper SQL database! It’s simply broken and it works on MySQL and SQLite just because it violates SQL standard. However, this is mainly ARel’s fault, not GitLab’s.

This is the best solution that can be used at this time. What about other databases - what exactly are you talking about? Or are you just the way you wrote? Let's be objective. This pull solves the problem of compatibility for Postgres and destroys what was now. Our team understands that this is not the best solution of those that can be and we do Research, on this issue, as you can support 3 of DBMSs such hacks. AR will patch up until not fix a bug in the rails that on CAST - it is a disgusting decision, I was disgusted by it, but what do you suggest?

For the future - in Russia there is a saying. "Criticize - offer. Suggest - do."

When we needed support Postgres - we had a discussion with a dense Gitlab developers on the issue and found a common ground. You tried to understand them and understand what they do not like?

thx

vsizov added a commit that referenced this pull request Oct 12, 2012
@williscool
Copy link

So the cast migration didn't work for me

but logging in with

RAILS_ENV=production rails db

can then running that cast CREATE CAST (integer AS text) WITH INOUT AS IMPLICIT;

did

@zzet
Copy link
Contributor Author

zzet commented Oct 27, 2012

@williscool you read #1666 (comment) tred?

currently required to use Postgres superuser

@williscool
Copy link

Yeah I did see that. Don't know that I knew exactly what it meant though.

But I'm debating in my head whether it would be better to not have that migration and just tell people to log into the console and do it.

I say this because I think the migrations are rolling back because this fails. And it would be easier to make sure that doesn't happens and then do the type cast yourself as that user.

Kind of like a final Warning: postgres support is experimental at this point

But of course:

  1. anyone technical enough to want to run postgres will figure it out anyway
  2. its up to you guys. This is a great product as is that you guys have done a tremendous job on and I'm happy to have

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

7 participants