Re: [v9.2] LEAKPROOF attribute of FUNCTION (Re: [HACKERS] [v9.2] Fix Leaky View Problem)

2012-02-15 Thread Kohei KaiGai
The attached patch is additional regression tests of ALTER FUNCTION with
LEAKPROOF based on your patch.
It also moves create_function_3 into the group with create_aggregate and so on.

Thanks,

2012/2/14 Kohei KaiGai kai...@kaigai.gr.jp:
 2012/2/14 Robert Haas robertmh...@gmail.com:
 On Tue, Feb 14, 2012 at 4:55 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 I could not find out where is the origin of grammer conflicts, although
 it does not conflict with any options within ALTER FUNCTION.

 Do you think the idea of ALTER ... NOT LEAKPROOF should be
 integrated within v9.2 timeline also?

 Yes.  Did you notice that I attached a patch to make that work?  I'll
 commit that today or tomorrow unless someone comes up with a better
 solution.

 Yes. I'll be available to work on the feature based on this patch.
 It was a headache of mine to implement alter statement to add/remove
 leakproof attribute.

 I also think we ought to stick create_function_3 into one of the
 parallel groups in the regression tests, if possible.  Can you
 investigate that?

 Not yet. This test does not have dependency with other tests,
 so, I'm optimistic to run create_function_3 concurrently.

 Me, too.

 I tried to move create_function_3 into the group of create_view and
 create_index, then it works correctly.

 Thanks,
 --
 KaiGai Kohei kai...@kaigai.gr.jp



-- 
KaiGai Kohei kai...@kaigai.gr.jp


pgsql-v9.2-alter-function-leakproof-regtest.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [v9.2] LEAKPROOF attribute of FUNCTION (Re: [HACKERS] [v9.2] Fix Leaky View Problem)

2012-02-15 Thread Robert Haas
On Wed, Feb 15, 2012 at 6:14 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 The attached patch is additional regression tests of ALTER FUNCTION with
 LEAKPROOF based on your patch.
 It also moves create_function_3 into the group with create_aggregate and so 
 on.

Committed, thanks.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [v9.2] LEAKPROOF attribute of FUNCTION (Re: [HACKERS] [v9.2] Fix Leaky View Problem)

2012-02-14 Thread Kohei KaiGai
2012/2/14 Robert Haas robertmh...@gmail.com:
 On Mon, Feb 13, 2012 at 7:51 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 I rebased the patch due to the updates of pg_proc.h.

 Please see the newer one. Thanks,

 Thanks, committed.  I think, though, that some further adjustment is
 needed here, because you currently can't do ALTER FUNCTION ... NO
 LEAKPROOF, which seems unacceptable.  It's fairly clear why not,
 though: you get a grammar conflict, because the parser allows this:

 create or replace function z() returns int as $$select 1$$ language
 sql set transaction not deferrable;

 However, since that syntax doesn't actually work, I'm thinking we
 could just refactor things a bit to reject that at the parser stage.
 The attached patch adopts that approach.  Anyone have a better idea?

I could not find out where is the origin of grammer conflicts, although
it does not conflict with any options within ALTER FUNCTION.

Do you think the idea of ALTER ... NOT LEAKPROOF should be
integrated within v9.2 timeline also?

 I also think we ought to stick create_function_3 into one of the
 parallel groups in the regression tests, if possible.  Can you
 investigate that?

Not yet. This test does not have dependency with other tests,
so, I'm optimistic to run create_function_3 concurrently.

Thanks,
-- 
KaiGai Kohei kai...@kaigai.gr.jp

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [v9.2] LEAKPROOF attribute of FUNCTION (Re: [HACKERS] [v9.2] Fix Leaky View Problem)

2012-02-14 Thread Robert Haas
On Tue, Feb 14, 2012 at 4:55 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 I could not find out where is the origin of grammer conflicts, although
 it does not conflict with any options within ALTER FUNCTION.

 Do you think the idea of ALTER ... NOT LEAKPROOF should be
 integrated within v9.2 timeline also?

Yes.  Did you notice that I attached a patch to make that work?  I'll
commit that today or tomorrow unless someone comes up with a better
solution.

 I also think we ought to stick create_function_3 into one of the
 parallel groups in the regression tests, if possible.  Can you
 investigate that?

 Not yet. This test does not have dependency with other tests,
 so, I'm optimistic to run create_function_3 concurrently.

Me, too.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [v9.2] LEAKPROOF attribute of FUNCTION (Re: [HACKERS] [v9.2] Fix Leaky View Problem)

2012-02-14 Thread Kohei KaiGai
2012/2/14 Robert Haas robertmh...@gmail.com:
 On Tue, Feb 14, 2012 at 4:55 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 I could not find out where is the origin of grammer conflicts, although
 it does not conflict with any options within ALTER FUNCTION.

 Do you think the idea of ALTER ... NOT LEAKPROOF should be
 integrated within v9.2 timeline also?

 Yes.  Did you notice that I attached a patch to make that work?  I'll
 commit that today or tomorrow unless someone comes up with a better
 solution.

Yes. I'll be available to work on the feature based on this patch.
It was a headache of mine to implement alter statement to add/remove
leakproof attribute.

 I also think we ought to stick create_function_3 into one of the
 parallel groups in the regression tests, if possible.  Can you
 investigate that?

 Not yet. This test does not have dependency with other tests,
 so, I'm optimistic to run create_function_3 concurrently.

 Me, too.

I tried to move create_function_3 into the group of create_view and
create_index, then it works correctly.

Thanks,
-- 
KaiGai Kohei kai...@kaigai.gr.jp

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [v9.2] LEAKPROOF attribute of FUNCTION (Re: [HACKERS] [v9.2] Fix Leaky View Problem)

2012-02-13 Thread Robert Haas
On Mon, Feb 13, 2012 at 7:51 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 I rebased the patch due to the updates of pg_proc.h.

 Please see the newer one. Thanks,

Thanks, committed.  I think, though, that some further adjustment is
needed here, because you currently can't do ALTER FUNCTION ... NO
LEAKPROOF, which seems unacceptable.  It's fairly clear why not,
though: you get a grammar conflict, because the parser allows this:

create or replace function z() returns int as $$select 1$$ language
sql set transaction not deferrable;

However, since that syntax doesn't actually work, I'm thinking we
could just refactor things a bit to reject that at the parser stage.
The attached patch adopts that approach.  Anyone have a better idea?

I also think we ought to stick create_function_3 into one of the
parallel groups in the regression tests, if possible.  Can you
investigate that?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


not-leakproof.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [v9.2] LEAKPROOF attribute of FUNCTION (Re: [HACKERS] [v9.2] Fix Leaky View Problem)

2012-01-25 Thread Robert Haas
On Sun, Jan 22, 2012 at 5:12 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 2012/1/21 Robert Haas robertmh...@gmail.com:
 On Sat, Jan 21, 2012 at 3:59 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 I marked the default leakproof function according to the criteria that
 does not leak contents of the argument.
 Indeed, timestamp_ne_timestamptz() has a code path that rises
 an error of timestamp out of range message. Is it a good idea to
 avoid mark leakproof on these functions also?

 I think that anything which looks at the data and uses that as a basis
 for whether or not to throw an error is non-leakproof.  Even if
 doesn't directly leak an arbitrary value, I think that leaking even
 some information about what the value is no good.  Otherwise, you
 might imagine that we would allow /(int, int), because it only leaks
 in the second_arg = 0 case.  And you might imagine we'd allow -(int,
 int) because it only leaks in the case where an overflow occurs.  But
 of course the combination of the two allows writing something of the
 form 1/(a-constant) and getting it pushed down, and now you have the
 ability to probe for an arbitrary value.  So I think it's just no good
 to allow any leaking at all: otherwise it'll be unclear how safe it
 really is, especially when combinations of different functions or
 operators are involved.

 OK. I checked list of the default leakproof functions.

 Functions that contains translation between date and timestamp(tz)
 can raise an error depending on the supplied arguments.
 Thus, I unmarked leakproof from them.

 In addition, varstr_cmp() contains translation from UTF-8 to UTF-16 on
 win32 platform; that may raise an error if string contains a character that
 is unavailable to translate.
 Although I'm not sure which case unavailable to translate between them,
 it seems to me hit on the basis not to leak what kind of information is
 no good. Thus, related operator functions of bpchar and text got unmarked.
 (Note that bpchareq, bpcharne, texteq and textne don't use it.)

Can you rebase this?  It seems that the pg_proc.h and
select_views{,_1}.out hunks no longer apply cleanly.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [v9.2] LEAKPROOF attribute of FUNCTION (Re: [HACKERS] [v9.2] Fix Leaky View Problem)

2012-01-21 Thread Robert Haas
On Sat, Jan 21, 2012 at 3:59 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 I marked the default leakproof function according to the criteria that
 does not leak contents of the argument.
 Indeed, timestamp_ne_timestamptz() has a code path that rises
 an error of timestamp out of range message. Is it a good idea to
 avoid mark leakproof on these functions also?

I think that anything which looks at the data and uses that as a basis
for whether or not to throw an error is non-leakproof.  Even if
doesn't directly leak an arbitrary value, I think that leaking even
some information about what the value is no good.  Otherwise, you
might imagine that we would allow /(int, int), because it only leaks
in the second_arg = 0 case.  And you might imagine we'd allow -(int,
int) because it only leaks in the case where an overflow occurs.  But
of course the combination of the two allows writing something of the
form 1/(a-constant) and getting it pushed down, and now you have the
ability to probe for an arbitrary value.  So I think it's just no good
to allow any leaking at all: otherwise it'll be unclear how safe it
really is, especially when combinations of different functions or
operators are involved.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [v9.2] LEAKPROOF attribute of FUNCTION (Re: [HACKERS] [v9.2] Fix Leaky View Problem)

2012-01-17 Thread Robert Haas
On Sun, Jan 8, 2012 at 10:52 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 BTW, can you also resubmit the leakproof stuff as a separate patch for
 the last CF?  Want to make sure we get that into 9.2, if at all
 possible.

 Yes, it shall be attached on the next message.

 The attached patch adds LEAKPROOF attribute to pg_proc; that
 enables DBA to set up obviously safe functions to be pushed down
 into sub-query even if it has security-barrier attribute.
 We assume this LEAKPROOF attribute shall be applied on operator
 functions being used to upgrade execute plan from Seq-Scan to
 Index-Scan.

 The default is without-leakproof attribute on creation of functions,
 and it requires superuser privilege to switch on.

The create_function_3 regression test fails for me with this applied:

*** /Users/rhaas/pgsql/src/test/regress/expected/create_function_3.out
 2012-01-17 22:09:01.0 -0500
--- /Users/rhaas/pgsql/src/test/regress/results/create_function_3.out
 2012-01-17 22:14:48.0 -0500
***
*** 158,165 
   'functext_E_2'::regproc);
 proname| proleakproof
  --+--
-  functext_e_2 | t
   functext_e_1 | t
  (2 rows)

  -- list of built-in leakproof functions
--- 158,165 
   'functext_E_2'::regproc);
 proname| proleakproof
  --+--
   functext_e_1 | t
+  functext_e_2 | t
  (2 rows)

  -- list of built-in leakproof functions
***
*** 476,485 
   'functext_F_4'::regproc);
 proname| proisstrict
  --+-
-  functext_f_1 | f
   functext_f_2 | t
   functext_f_3 | f
   functext_f_4 | t
  (4 rows)

  -- Cleanups
--- 476,485 
   'functext_F_4'::regproc);
 proname| proisstrict
  --+-
   functext_f_2 | t
   functext_f_3 | f
   functext_f_4 | t
+  functext_f_1 | f
  (4 rows)

  -- Cleanups

The new regression tests I just committed need updating as well.

Instead of contains_leakable_functions I suggest
contains_leaky_functions or contains_non_leakproof_functions, because
leakable isn't really a word (although I know what you mean).

The design of this function also doesn't seem very future-proof.  If
someone adds a new node type that can contain a function call, and
forgets to add it here, then we've got a subtle security hole.  Is
there some reasonable way to design this so that we assume
everything's dangerous except for those things we know are safe,
rather than the reverse?

I think you need to do a more careful check of which functions you're
marking leakproof - e.g. timestamp_ne_timestamptz isn't, at least
according to my understanding of the term.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers