fx icon indicating copy to clipboard operation
fx copied to clipboard

Can't use function with parameters

Open timothee-alby opened this issue 8 years ago • 12 comments

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?

timothee-alby avatar Jan 17 '17 14:01 timothee-alby

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 avatar Jan 18 '17 10:01 teoljungberg

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

teoljungberg avatar Jan 18 '17 10:01 teoljungberg

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

timothee-alby avatar Jan 18 '17 11:01 timothee-alby

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.

teoljungberg avatar Jan 18 '17 12:01 teoljungberg

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)

timothee-alby avatar Jan 18 '17 12:01 timothee-alby

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 avatar Jan 18 '17 15:01 teoljungberg

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!

teoljungberg avatar Mar 17 '17 10:03 teoljungberg

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

mkarklins avatar Jan 14 '20 06:01 mkarklins

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

johanb avatar Jul 29 '20 11:07 johanb

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

johanb avatar Jul 29 '20 11:07 johanb

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

Matchlighter avatar Aug 31 '20 17:08 Matchlighter

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

teoljungberg avatar Jan 21 '23 12:01 teoljungberg