teoljungberg / fx

Versioned database functions and triggers for Rails
MIT License
725 stars 71 forks source link

Can't use function with parameters #7

Open timothee-alby opened 7 years ago

timothee-alby commented 7 years ago

Thanks for this, as a user of scenic, I was happy to find a similar tool for functions :)

Unfortunately, this doesn't work for functions with parameters: although creating function isn't a problem, dropping function isn't possible so migrations can't be reverted.

I believe the problem comes from Fx::Adapters::Postgres#drop_function as the brakets (()) are added automatically. I have functions with parameters and the function signature must be specified to drop it (because one can create different functions with the same name but different parameters).

A fix could be that when the function name is specified as a string, the brackets are not added? If backward-compatibility is a concern, the alternatively could be to pass a list of parameters as an option?

teoljungberg commented 7 years ago

Hi @timothee-alby!

That's a good use-case that I didn't consider, I first built this to resolve my issues around using triggers and functions combined. But F(x) should work with all kinds of functions.

After doing some quick reading, it seems we can leverage pg_get_function_identity_arguments to get the arguments and therefore we can determine the function signature as a whole. It will require a lookup in the database, but I'd deem it's necessary. What do you think?

I'll see if I can open a PR for you to try out and review to resolve this issue.

teoljungberg commented 7 years ago

I've opened https://github.com/teoljungberg/fx/pull/8 to hopefully resolve the issue.

timothee-alby commented 7 years ago

Thank you, that was quick!

The problem is that I have multiple functions with the same name but different signatures (yes, this is a bit insane, I know...) That's why I suggested passing the arguments manually

teoljungberg commented 7 years ago

Thank you, that was quick!

I aim to please.

The problem is that I have multiple functions with the same name but different signatures (yes, this is a bit insane, I know...) That's why I suggested passing the arguments manually

That should be taken care of automatically, since the entire function signature is what we're using to identify it. But, if you can have some varying examples I can use to sanity test this. That would be much obliged.

timothee-alby commented 7 years ago

Say you create 2 functions with the same name:

CREATE FUNCTION foo(bar INTEGER) RETURNS INTEGER AS 'select 1' LANGUAGE SQL;
CREATE FUNCTION foo(bar INTEGER, baz VARCHAR) RETURNS INTEGER AS 'select 2' LANGUAGE SQL;

Then arguments_for_function will fail because the func_oid CTE returns 2 rows:

SELECT oid FROM pg_proc WHERE proname = 'foo';
  oid
--------
 144958
 144959
(2 rows)
teoljungberg commented 7 years ago

Ah, Now I see the issue. You're absolutely right.

I think passing the parameters in some datastructure makes sense. And we'll default to () if no parameters are given.

I'll ponder on this for a bit, and update corresponding PR.

teoljungberg commented 7 years ago

Hi @timothee-alby - sorry for ghosting on you. I started poking around at this, but got distracted by other things.

This turned out to be a larger problem than I thought, so this takes some refactorings- but it's going to make this work much smoother.

I'll update the issue as soon as I've made progress!

mkarklins commented 4 years ago

Just wanted to check if there has been any progress made?

johanb commented 4 years ago

@timothee-alby have you found a work-around for this ?

johanb commented 4 years ago

Never mind, I see there is support for this on master branch.

Matchlighter commented 4 years ago

I couldn't see how this is supported on the master branch as of now. (Am I missing something staring me in the face?) However, in the meantime, I've come up with this workaround to at least get reversible migrations (something our CI insists on):

class CreateFunctionNaturalsort < ActiveRecord::Migration[5.0]
  def up
    create_function :naturalsort
  end

  def down
    execute "DROP FUNCTION naturalsort(text);"
  end
end
teoljungberg commented 1 year ago

I've done the above too when needed, it's not pretty - but it works.