Re: [HACKERS] to_json(NULL) should to return JSON null instead NULL

2015-09-01 Thread Yeb Havinga
On 30/08/15 04:57, Andrew Dunstan wrote:
> 
> 
> On 08/29/2015 04:27 PM, Tom Lane wrote:

>> I'm not necessarily against changing it --- but it doesn't seem entirely
>> black-and-white to me, and we do now have a couple of versions worth
>> of precedent we'd be breaking with.
>>
>> If we do vote to change it, I'd want to do so now (ie in 9.5) rather than
>> create yet another year's worth of precedent.
>>
>>
> 
> I agree with pretty much all of this. My fairly strong inclination is to
> leave it as it is and document the behaviour more clearly. Changing it
> seems likely to introduce a different inconsistency which is harder to
> understand.


This thread reminds me of the decision we had to make when implementing
ISO healthcare datatypes with its own NullFlavors in PostgreSQL, see
section 3.2 of http://arxiv.org/pdf/1003.3370v1.pdf for a discussion.

TLDR: even though semantically there might be overlap in the two kinds
of nulls, there are two mechanisms to represent 'value at present
unknown': in the heaptuple and in the datatype varlena Datum storage,
each with their own IS NULL / STRICT vs isnull and other functions. We
decided that trying to merge both null representing mechanisms would
probably lead to an incomplete merge, and thus many unexpected problems,
and that therefore a clean separation would be easiest to explain and
work with.

regards,
Yeb Havinga



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


Re: [HACKERS] pgaudit - an auditing extension for PostgreSQL

2015-02-17 Thread Yeb Havinga
Hi list,

On 20/01/15 23:03, Jim Nasby wrote: On 1/20/15 2:20 PM, Robert Haas wrote:
 On Tue, Jan 20, 2015 at 1:05 AM, Abhijit
 Menon-Sena...@2ndquadrant.com  wrote:
 So when I'm trying to decide what to audit, I have to:
 
  (a) check if the current user is mentioned in .roles; if so,
 audit.
 
  (b) check if the current user is a descendant of one of the roles
  mentioned in .roles; if not, no audit.
 
  (c) check for permissions granted to the root role and see if
 that
  tells us to audit.
 
 Is that right? If so, it would work fine. I don't look forward to
 trying
 to explain it to people, but yes, it would work (for anything you could
 grant permissions for).
 I think this points to fundamental weakness in the idea of doing this
 through the GRANT system.  Some people are going want to audit
 everything a particular user does, some people are going to want to
 audit all access to particular objects, and some people will have more
 complicated requirements.  Some people will want to audit even
 super-users, others especially super-users, others only non
 super-users.  None of this necessarily matches up to the usual
 permissions framework.

 +1. In particular I'm very concerned with the idea of doing this via
 roles, because that would make it trivial for any superuser to disable
 auditing.

Rejecting the audit administration through the GRANT system, on the
grounds that it easy for the superuser to disable it, seems unreasonable
to me, since superusers are different from non-superusers in a
fundamental way.

The superuser operates on the administration level of the database, in
contrast with users that need access to the actual information in the
data to perform their job. An organization that wants to implement an
auditing strategy needs to think in different terms to audit
user/application level actions, and administrator/superuser actions.
Consequently it should be no surprise when PostgreSQL mechanisms for
auditing behave different for superusers vs non superusers.

The patch as it is, is targeted at auditing user/application level
access to the database, and as such it matches the use case of auditing
user actions.

Auditing superuser access means auditing beyond the running database.
The superuser can dump a table, and pipe this data everywhere outside of
the auditing domain. I cannot begin to imagine the kind of security
restrictions you'd need to audit what happens with data after libpq has
produced the results. My best guess would be to incorporate some kind of
separation of duty mechanism; only allow certain superuser operations
with two people involved.

regards,
Yeb Havinga


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


Re: [HACKERS] RLS Design

2014-07-02 Thread Yeb Havinga

On 01/07/14 21:51, Robert Haas wrote:

On Tue, Jul 1, 2014 at 3:20 PM, Dean Rasheed dean.a.rash...@gmail.com wrote:


That seems like a pretty strong argument.

If RLS quals are instead regarded as constraints on access, and
multiple policies apply, then it seems that the quals should now be
combined with AND rather than OR, right?

Yeah, maybe.  I intuitively feel that OR would be more useful, so it
would be nice to find a design where that makes sense.
Looking at the use cases we described earlier in 
http://www.postgresql.org/message-id/attachment/32196/mini-rim.sql I see 
more OR than AND, for instance 'if the row is sensitive then the user 
must be related to the row' which translates to (NOT sensitive) OR the 
user is related.


An addition to that rule could be a breakglass method or other reasons 
to access, e.g.
(NOT sensitive) OR user is related OR break glass OR legally required 
access.



   But it depends
a lot, in my view, on what syntax we end up with.  For example,
suppose we add just one command:

ALTER TABLE table_name FILTER [ role_name | PUBLIC ] USING qual;

If the given role inherits from multiple roles that have different
filters, I think the user will naturally expect all of the filters to
be applied.


Suppose a building administrator gives a single person that has multiple 
roles multiple key cards to access appropriate rooms in a building. You 
could draw a venn diagram of the rooms those key cards open, and the 
intuition here probably is that the person can enter any room if one of 
the key cards matches, not all cards.



But you could do it other ways.  For example:

ALTER TABLE table_name [ NO ] ROW LEVEL SECURITY;
ALTER TABLE table_name GRANT ROW ACCESS TO role_name USING qual;

If a table is set to NO ROW LEVEL SECURITY then it behaves just like
it does now: anyone who accesses it sees all the rows, restricted to
those columns for which they have permission.  If the table is set to
ROW LEVEL SECURITY then the default is to show no rows.  The second
command then allows access to a subset of the rows for a give role
name.  In this case, it is probably logical for access to be combined
via OR.


regards,
Yeb Havinga



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


Re: [HACKERS] json(b) equality rules

2014-04-03 Thread Yeb Havinga

On 2014-04-03 09:40, Oleg Bartunov wrote:

Sure, we don't follow. I mean should we add to documentation
such matrices.

Oleg

On Thu, Apr 3, 2014 at 11:32 AM, Oleg Bartunov obartu...@gmail.com wrote:

Hi there,

I'm wondering if we should follow all js equility rules as
nicely visualized in
http://strilanc.com/visualization/2014/03/27/Better-JS-Equality-Table.html

Oleg



+1

I was a bit curious what the result would be. A quick inspection of the 
query results below gave the impression that the matrix would probably 
show a diagonal line. Even though the table is not necessary as a 
reference to strange equality rules, a table of equality showing a 
diagonal will be easy to remember.


regards,
Yeb

drop table testjsonb;
create table testjsonb(a jsonb);
insert into testjsonb (a) values ('true');
insert into testjsonb (a) values ('false');
insert into testjsonb (a) values ('1');
insert into testjsonb (a) values ('0');
insert into testjsonb (a) values ('-1');
insert into testjsonb (a) values ('true');
insert into testjsonb (a) values ('false');
insert into testjsonb (a) values ('1');
insert into testjsonb (a) values ('0');
insert into testjsonb (a) values ('');
insert into testjsonb (a) values ('null');
insert into testjsonb (a) values ('undefined');
insert into testjsonb (a) values ('Infinity');
insert into testjsonb (a) values ('-Infinity');
insert into testjsonb (a) values ('[]');
insert into testjsonb (a) values ('{}');
insert into testjsonb (a) values ('[{}]');
insert into testjsonb (a) values ('[0]');
insert into testjsonb (a) values ('[1]');
insert into testjsonb (a) values ('NaN');

select  a.a, b.a, a.a = b.a
fromtestjsonb a, testjsonb b



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


Re: [HACKERS] Row-security on updatable s.b. views

2014-03-07 Thread Yeb Havinga

On 05/03/14 15:44, Craig Ringer wrote:

On 03/05/2014 05:25 PM, Yeb Havinga wrote:

Maybe a naive thought, but shouldn't all plans that include a table with
an RLS clause be invalidated when the session role switches, regardless
of which users from and to?

Only if the plan is actually accessed when under a different user ID.
Consider SECURITY DEFINER functions; you don't want to flush all cached
plans just because you ran a SECURITY DEFINER function that doesn't even
share any statements with the outer transaction.

Hmm good point.


I've also got some concerns about the user visible API; I'm not sure it
makes sense to set row security policy for row reads per-command in
PostgreSQL, since we have the RETURNING clause. Read-side policy should
just be FOR READ.

Hmm but FOR READ would mean new keywords, and SELECT is also a concept
known to users. I didn't find the api problematic to understand, on the
contrary.

Would you expect that FOR SELECT also affects rows you can see to
UPDATE, INSERT, or DELETE?

Yes.

Because that's what it would have to mean, really. Otherwise, you could
just use `UPDATE thetable SET id = id RETURNING *` (or whatever) to read
the rows out if you had UPDATE rights. Or do the same with DELETE.

With RETURNING, it doesn't make much sense for different statements to
have different read access. Can you think of a case where it'd be
reasonable to deny SELECT, but allow someone to see the same rows with
`UPDATE ... RETURNING` ?


It might be an idea to add the SELECT RLS clause for DML
queries that contain a RETURNING clause.

That way lies madness: A DML statement that affects *a different set of
rows* depending on whether or not it has a RETURNING clause.
If you state it like that, it sounds like a POLA violation. But the 
complete story is: A user is allowed to UPDATE a set of rows from a 
table that is not a subsect of the set of rows he can SELECT from the 
table, iow he can UPDATE rows he is not allowed to SELECT. This can lead 
to unexpected results: When the user issues an UPDATE of the table 
without a returning clause, all rows the user may UPDATE are affected. 
When the user issues an UPDATE of the table with a returning clause, all 
rows the user may UPDATE and SELECT are affected.


So the madness comes from the fact that it is allowed to define RLS that 
allow to modify rows you cannot select. Either prevent these conditions 
(i.e. proof that all DML RLS qual implies the SELECT qual, otherwise 
give an error on DML with a RETURNING clause), or allow it without 
violating the RLS rules but accept that a DML with RETURNING is 
different from a DML only.


regards,
Yeb



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


Re: [HACKERS] Row-security on updatable s.b. views

2014-03-06 Thread Yeb Havinga

On 06/03/14 02:56, Craig Ringer wrote:

On 03/06/2014 04:56 AM, Yeb Havinga wrote:


If you state it like that, it sounds like a POLA violation. But the
complete story is: A user is allowed to UPDATE a set of rows from a
table that is not a subsect of the set of rows he can SELECT from the
table, iow he can UPDATE rows he is not allowed to SELECT.




Is there a compelling use case for this? Where it really makes sense to
let users update/delete rows they cannot see via row security? We
support it in the table based permissions model, but it's possible to do
it with much saner semantics there. And with row security, it'll be
possible with security definer functions. I intend to add a row
security exempt flag for functions down the track, too.

Use case: https://en.wikipedia.org/wiki/Bell-La_Padula_model - being 
able to write up and read down access levels.


So for instance in healthcare, a data enterer may enter from hand 
written notes sensitive data (like subject has aids) in the electronic 
health record, without having general read access of the level of 
sensitivity of aids diagnosis. I think what is important in use cases 
like this, is that at data entry time, the actual data is still at the 
desk, so having data returned for inserts in the running transaction 
might not be problematic. As most EHR's today are additive in nature, 
future additions about the aids conditions would be inserts as well, no 
updates required. For updates my best guess would be that allowing the 
command to run with rls permissions different from the select is not 
required.


regards,
Yeb



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


Re: [HACKERS] Row-security on updatable s.b. views

2014-03-05 Thread Yeb Havinga

On 2014-03-05 04:02, Craig Ringer wrote:

On 03/04/2014 09:41 PM, Yeb Havinga wrote:

On 04/03/14 02:36, Craig Ringer wrote:

I've pushed an update to the branch with the fix for varno handling.
Thanks. It's tagged rls-9.4-upd-sb-views-v8 .

I've almost run out of time to spend on row security for this
commitfest, unfortunately. I'm putting a blog together with a current
status update. Frustrating, as it's coming together now.

Open issues include:

- Passing plan inval items from rewriter into planner
- COPY support pending
- Clear syntax in DDL

Most of the rest are solved; it's actually looking pretty good.

Hi Craig,

I've tested the results from the minirim.sql that was posted earlier,
and the v8 gives the same results as v4 :-)

Good to hear.

The main known issue remaining is plan invalidation. Row security needs
to create PlanInvalItems during _rewrite_ and also needs to set a
PlannerGlobal field for the user ID the plan depends on. If it fails to
do this then the superuser will sometimes run queries and have
row-security applied, non-superusers might skip row security when it
should be applied. This seems to be the cause of foreign key check
issues I've observed, too.

That's not trivial, because right now the rewriter only returns a Query
node. It doesn't have anywhere to stash information that's global across
the whole query, and adding fields to Query for the purpose doesn't seem
ideal, since it's also used for subqueries, and in the planner.


I looked up the Query structure and steps of e.g. exec_simple_query(), 
but ISTM that Query would be the place to store a used id. After all it 
is meta data about the query. Duplication of this information in the 
presence of subqueries seems less ugly to me than trying to evade 
duplication by wrapping a structure around a query list.


Maybe a naive thought, but shouldn't all plans that include a table with 
an RLS clause be invalidated when the session role switches, regardless 
of which users from and to?




I've also got some concerns about the user visible API; I'm not sure it
makes sense to set row security policy for row reads per-command in
PostgreSQL, since we have the RETURNING clause. Read-side policy should
just be FOR READ.


Hmm but FOR READ would mean new keywords, and SELECT is also a concept 
known to users. I didn't find the api problematic to understand, on the 
contrary. It might be an idea to add the SELECT RLS clause for DML 
queries that contain a RETURNING clause.


regards,
Yeb



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


Re: [HACKERS] Row-security on updatable s.b. views

2014-03-04 Thread Yeb Havinga

On 04/03/14 02:36, Craig Ringer wrote:

On 02/25/2014 01:28 AM, Dean Rasheed wrote:

On 13 February 2014 04:12, Craig Ringer cr...@2ndquadrant.com wrote:

It's crashing while pulling up the query over emp (hl7.employee) and
part (hl7.participation).

Given the simplicity of what the row-security code its self is doing,
I'm wondering if this is a case that isn't handled in updatable s.b.
views. I'll look into it.

I'm not sure how much further you've got with this, but I think the
issue is that the securityQuals that you're adding don't refer to the
correct RTE. When adding securityQuals to an RTE, they are expected to
have Vars whose varno matches the rt_index of the RTE (see for example
the code in rewriteTargetView() which calls ChangeVarNodes() on
viewqual before adding the qual to securityQuals or the main query
jointree). prepend_row_security_quals() doesn't appear to have any
similar code, and it would need to be passed the rt_index to do that.

Thanks for the pointer. That was indeed the issue.

I've pushed an update to the branch with the fix for varno handling.
Thanks. It's tagged rls-9.4-upd-sb-views-v8 .

I've almost run out of time to spend on row security for this
commitfest, unfortunately. I'm putting a blog together with a current
status update. Frustrating, as it's coming together now.

Open issues include:

- Passing plan inval items from rewriter into planner
- COPY support pending
- Clear syntax in DDL

Most of the rest are solved; it's actually looking pretty good.


Hi Craig,

I've tested the results from the minirim.sql that was posted earlier, 
and the v8 gives the same results as v4 :-)


Thanks for all the work!
Yeb



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


Re: [HACKERS] Row-security on updatable s.b. views

2014-03-04 Thread Yeb Havinga

On 04/03/14 02:36, Craig Ringer wrote:

On 02/25/2014 01:28 AM, Dean Rasheed wrote:

On 13 February 2014 04:12, Craig Ringer cr...@2ndquadrant.com wrote:

It's crashing while pulling up the query over emp (hl7.employee) and
part (hl7.participation).

Given the simplicity of what the row-security code its self is doing,
I'm wondering if this is a case that isn't handled in updatable s.b.
views. I'll look into it.

I'm not sure how much further you've got with this, but I think the
issue is that the securityQuals that you're adding don't refer to the
correct RTE. When adding securityQuals to an RTE, they are expected to
have Vars whose varno matches the rt_index of the RTE (see for example
the code in rewriteTargetView() which calls ChangeVarNodes() on
viewqual before adding the qual to securityQuals or the main query
jointree). prepend_row_security_quals() doesn't appear to have any
similar code, and it would need to be passed the rt_index to do that.

Thanks for the pointer. That was indeed the issue.

I've pushed an update to the branch with the fix for varno handling.
Thanks. It's tagged rls-9.4-upd-sb-views-v8 .

I've almost run out of time to spend on row security for this
commitfest, unfortunately. I'm putting a blog together with a current
status update. Frustrating, as it's coming together now.

Open issues include:

- Passing plan inval items from rewriter into planner
- COPY support pending
- Clear syntax in DDL

Most of the rest are solved; it's actually looking pretty good.


Hi Craig,

I've tested the results from the minirim.sql that was posted earlier, 
and the v8 gives the same results as v4 :-)


Thanks for all the work!
Yeb



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


Re: [HACKERS] Row-security on updatable s.b. views

2014-02-11 Thread Yeb Havinga

On 2014-02-11 09:36, Craig Ringer wrote:

On 02/06/2014 10:19 PM, Craig Ringer wrote:

On 02/06/2014 12:43 PM, Craig Ringer wrote:

1. Try (again) to do row-security in the rewriter. This was previously
impossible because of the definition of row-security behaviour around
inheritance, but with the simplified inheritance model now proposed I
think it's possible.

Thanks to the simplified requirements for inheritance, this turns out to
be fairly easy. There's a version rewritten to use the rewriter in the tag:

rls-9.4-upd-sb-views-v6

on https://github.com/ringerc/postgres.git

... which was totally wrong, and I blame lack of sleep for it ever
getting pushed. I didn't understand the rewriter as well as I thought.

v7 applies row-security quals in fireRIRrules .



New tag:

rls-9.4-upd-sb-views-v6


Hi Craig,

This looks to be the same v6 version as the initial rewriter version.
https://github.com/ringerc/postgres/commits/rls-9.4-upd-sb-views-v6

regards,
Yeb



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


Re: [HACKERS] Row-security on updatable s.b. views

2014-02-11 Thread Yeb Havinga

On 2014-02-11 12:09, Craig Ringer wrote:

On 02/11/2014 06:05 PM, Yeb Havinga wrote:

On 2014-02-11 09:36, Craig Ringer wrote:

On 02/06/2014 10:19 PM, Craig Ringer wrote:

On 02/06/2014 12:43 PM, Craig Ringer wrote:

1. Try (again) to do row-security in the rewriter. This was previously
impossible because of the definition of row-security behaviour around
inheritance, but with the simplified inheritance model now proposed I
think it's possible.

Thanks to the simplified requirements for inheritance, this turns out to
be fairly easy. There's a version rewritten to use the rewriter in
the tag:

 rls-9.4-upd-sb-views-v6

on https://github.com/ringerc/postgres.git

... which was totally wrong, and I blame lack of sleep for it ever
getting pushed. I didn't understand the rewriter as well as I thought.

v7 applies row-security quals in fireRIRrules .
New tag:

rls-9.4-upd-sb-views-v6

Hi Craig,

This looks to be the same v6 version as the initial rewriter version.
https://github.com/ringerc/postgres/commits/rls-9.4-upd-sb-views-v6

Whoops, wrong paste.

rls-9.4-upd-sb-views-v7


Hi Craig,

I compared output of psql -ef of the minirim.sql script posted earlier 
in http://www.postgresql.org/message-id/52f54927.1040...@gmail.com 
between v4 and v7.


Not everything is ok.

  Seq Scan on patient  (cost=0.00..29589.31 rows=495 width=52)
Filter: (SubPlan 1)
SubPlan 1
@@ -555,7 +592,7 @@
  -  Materialize  (cost=26.39..570.62 rows=1014 width=4)
-  Subquery Scan on act (cost=26.39..565.55 
rows=1014 width=4)
  -  Nested Loop Semi Join 
(cost=26.39..555.41 rows=1014 width=108)
-   Join Filter: (((part.act = act_1.id) 
AND (emp_2.pgname = (current_user())::text)) OR (NOT 
((act_1.confidentialitycode)::text[] @ '{s}'::text[])))
+   Join Filter: (((part.act = act_1.id) 
AND (emp_2.pgname = (current_user())::text)) OR (NOT 
((act_1.effectivetime)::text[] @ '{s}'::text[])))
-  Append  (cost=0.00..31.19 
rows=1019 width=108)
  -  Seq Scan on act act_1  
(cost=0.00..1.59 rows=59 width=108)


@@ -587,12 +624,8 @@
 FROM patient, person, organization
 WHERE patient.player = person.id
 AND patient.scoper = organization.id;
- id | vipcode |   name   |  birthtime  | name
-+-+--+-+
- 10 | | John Doe | 1963-04-01 00:00:00 | Community Health and 
Hospitals
- 16 | | John Doe | 1963-04-01 00:00:00 | Community Mental 
Health Clinic

-(2 rows)
-
+psql:/home/m/minirim2.sql:409: ERROR:  attribute 6 has wrong type
+DETAIL:  Table has type tsrange, but query expects _confidentialitycode.


@@ -629,7 +662,4 @@
 SET SESSION AUTHORIZATION sigmund;
 SET
 SELECT * FROM test;
- id | classcode | moodcode | code | confidentialitycode | effectivetime
-+---+--+--+-+---
-(0 rows)
-
+psql:/home/m/minirim2.sql:439: connection to server was lost


regards,
Yeb Havinga



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


Review of RLS on inheritance schema HL7 RIM (was Re: [HACKERS] Row-security on updatable s.b. views)

2014-02-07 Thread Yeb Havinga

On 06/02/14 15:19, Craig Ringer wrote:


Thanks to the simplified requirements for inheritance, this turns out to
be fairly easy. There's a version rewritten to use the rewriter in the tag:

rls-9.4-upd-sb-views-v6

on https://github.com/ringerc/postgres.git


Hi Craig, list,

This is review of the current RLS patch on a database schema that uses 
inheritance in the 'classical' sense (not partitioning). The review was 
done on rls-9.4-upd-sb-views-v4 and hence all comments are about that 
version. Comparing output of the minisql script between v4 and v6 gives 
differences, as v6 seems to be WIP.


Our goal is to implement the HL7 Reference Information Model (RIM) in 
PostgreSQL. A fine-grained access control on the tables would have a 
practical use in the context of RIM. So, we have made some preliminary 
tests of the Row Security patch for such a specific data model. For the 
purpose of reviewing RLS, we have restricted the full RIM to just a few 
tables which we call the mini-RIM. It is important to observe that the 
RIM uses inheritance, and we use PostgreSQL inheritance to implement the 
RIM's inheritance. More details about the RIM are presented below.


In the attached SQL script we list a mini-RIM, along with examples of 
RLS enforcement.


General comments about RLS applied on (a minimalistic version of) the 
RIM can be summarized as follows:


1. The current RLS implementation works for use cases where 
confidentiality attributes are specified in the inheritance root 
relation. Since security labelling in the RIM is done on 
confidentialitycodes that are present in the inheritance roots (e.g., 
Role and Act), the current RLS works for the RIM.


2. Infinite recursion is well captured in case of recursive restrictions 
to tables.


3. RLS syntax is readable and easy to use.

4. Documentation needs work.

5. Subqueries in RLS quals can be pulled up, so opens the ability for 
fast processing.



Overall from a users perspective the patch gave a solid impression.

regards,
Yeb Havinga
Albana Gaba
Henk-Jan Meijer

Portavita B.V. The Netherlands

BACKGROUND ON THE REFERENCE INFORMATION MODEL:

To understand how The HL7 Reference Information Model (RIM) uses 
PostgreSQL inheritance, it is helpful to understand the meaning of the 
content of the parent and child tables. This section describes the 
background of the RIM, and describes a few classes of the “Act” hierarchy.


The HL7 RIM[1] is not just yet another information model. It is a 
mature, standard information model that has been used and refined over 
the course of many years [2][3]. Its purpose is to capture detailed 
information for medical records. Pivotal in the RIM is its action-based 
modelling, based on ideas that can be traced back to the American 
philosopher C.S. Peirce. A direct line from Peirce’s Logic of Relatives 
to the foundations of relational databases has been introduced in [4].

Versions of the RIM are now being released as an ANSI standard.

An illustration of the RIM is available at 
http://www.healthintersections.com.au/wp-content/uploads/2011/05/RIM.png
The RIM is a set of UML classes, each containing one or more attributes. 
The classes are an abstraction of subjects or other concepts that are 
relevant within the healthcare domain. To avoid a model with a huge 
number of classes, the RIM defines six core classes whereas the vast 
majority of the classes are defined as specializations based on the core 
ones. The specialization classes inherit all the properties of the 
generalization classes while adding specific attributes of its own. To 
make matters concrete, let us look at Act class.


“Act”: Looking at the right hand side of the RIM illustration referenced 
above, we can see the class “Act” and its specializations, and this is 
the focal point for the RIM’s action based modeling. Description from 
the standard: “Acts are the pivot of the RIM: domain information and 
process records are represented primarily in Acts. Any profession or 
business, including healthcare, is primarily constituted of intentional 
and occasionally non-intentional actions, performed and recorded by 
responsible actors. An Act-instance is a record of such an action.” 
Notable attributes of “Act” are:


“id” - A unique identifier for the Act. Each Act is associated with a 
unique id. All specialization of Act inherit this id. This means that if 
there is, for example, an instance of Observation with id 5, there exist 
no other acts with id 5. In fact, since technically in the RIM all 
identifiers stem from a single infrastructure root, the identifiers are 
globally unique: there exists a single object with id 5. This single 
object is an instance of Observation, and since Observation is a 
specialization of Act, it is also an instance of Act.


“classCode” – The major class of Acts to which an Act-instance belongs. 
The allowed codes in classCode form a hierarchical code system. In the 
2011 RIM, there are 124 different class codes

Re: [HACKERS] Row-security on updatable s.b. views

2014-02-06 Thread Yeb Havinga

On 2014-02-06 05:43, Craig Ringer wrote:

Based on Tom's objections, another approach is presented in
rls-9.4-upd-sb-views-v5 on  g...@github.com:ringerc/postgres.git . The
Query node is used to record the recursive expansion parent list
instead, and copying is avoided.


Cannot fetch or clone.

github on web says This repository is temporarily unavailable. The 
backend storage is temporarily offline. Usually this means the storage 
server is undergoing maintenance. Please contact support if the problem 
persists.


regards,
Yeb



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


Re: [HACKERS] Prohibit row-security + inheritance in 9.4?

2014-01-31 Thread Yeb Havinga

On 2014-01-31 16:05, Stephen Frost wrote:

* Yeb Havinga (y.t.havi...@mgrid.net) wrote:

IMHO, there is another way to implement this, other than the
procedure to override the child-rel-quals with the ones from the
parent. At DDL time, synchronize quals on the parent with rls quals
of the childs. Isn't this also what happens with constraints?

No, we're not going to do that.  We don't do it for GRANT and I don't
think it makes sense to do it here.
This reasoning could go either way. GRANT is on a complete set of rows. 
This is a restriction on the level of individual rows, and in that 
sense, it is more like a row-level CHECK constraint.

If we wanted to make them the same then we'd throw out the ability to do
any kind of changes or actions on the child and then we'd have actual
partitioning.  We don't have that though, we have inheiritance.


I fail to understand this, probably because I do not have a partition 
use case for inheritance, but rather an information model that is more 
ontology like. The more specific childs get down the inheritance tree, 
more columns they get, and their requirements might differ completely in 
nature from their siblings, and make no sense at all as well when 
specified at the level of the parent (or even impossible, since the 
parent does not have all the columns).



Then during expansion of the range table, no code is needed to
ignore child rls quals and copy parent rels to child rels.

This is what's already implemented and isn't a huge amount of code to
begin with, so I don't see this as being an argument against having the
flexibility.


It would seem to me that any additional logic that can be avoided during 
planning is a good thing. Also, the argument that something is already 
implemented, does not itself make it good to commit.


What do you mean with 'having the flexibility' and why is that good?



Also, the security policy applied would be invariant to the route
through which the rows were accessed:

You could also get this by simply only allowing access to the parent and
not granting any privileges on the children.


That might work for partitioning, but not for use cases where childs 
have more columns than parents.

- directly to the child row: child rls quals and parent quals (by
propagate at ddl) are applied.
- through the parent: child rls quals and parent quals applied.

If you want them to be the same then you can implement this yourself
without having PG force it on you.


I suggested it as  a solution for a requirement worded upthread as It 
makes absolutely zero sense, in my head anyway, to have rows returned 
when querying the parent which should NOT be returned based on the quals 
of the parent. without disregarding rls-quals on childs.


regards,
Yeb Havinga



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


Re: [HACKERS] Prohibit row-security + inheritance in 9.4?

2014-01-31 Thread Yeb Havinga

On 2014-01-31 15:10, Stephen Frost wrote:

* Craig Ringer (cr...@2ndquadrant.com) wrote:

On 01/31/2014 09:01 AM, Stephen Frost wrote:
The only case prevented is one where access to the child via the parent
shows rows that the parent's row-security qual would hide, because the
child's qual doesn't.

It makes absolutely zero sense, in my head anyway, to have rows returned
when querying the parent which should NOT be returned based on the quals
of the parent.


IMHO, there is another way to implement this, other than the procedure 
to override the child-rel-quals with the ones from the parent. At DDL 
time, synchronize quals on the parent with rls quals of the childs. 
Isn't this also what happens with constraints?


Then during expansion of the range table, no code is needed to ignore 
child rls quals and copy parent rels to child rels.


Also, the security policy applied would be invariant to the route 
through which the rows were accessed:
- directly to the child row: child rls quals and parent quals (by 
propagate at ddl) are applied.

- through the parent: child rls quals and parent quals applied.

regards,
Yeb Havinga



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


Re: [HACKERS] [v9.4] row level security

2013-12-18 Thread Yeb Havinga

On 2013-12-14 05:40, Craig Ringer wrote:

I find the heirachical and non-heirachical label security model used in
Teradata to be extremely interesting and worthy of study.

The concept is that there are heirachical label policies (think
classified, unclassified, etc) or non-heirachical (mutually
exclusive labels). A user can see a row if they have all the policies
used in a table, and each of the user's policy values is sufficient to
see the row. For non-heiracical policies that means the user and row
must have at least one label in common; for heiracical policies it means
the user label must be greater than or equal to the row label.

That model lets you quite flexibly and easily build complex security
models. It'd work well with PostgreSQL, too: We could implement
something like non-heirachical policies using a system varbit column
that's added whenever a non-heirachical policy gets added to a table,
and simply AND it with the varbit on the user's policy to see if they
can access the row. Or use a compact fixed-width bitfield. Heirachical
policies would use a 1 or 2 byte int system column to store the label.


Is is necessary to fix the attribute type of the security label? The 
label model described above seems to restrictive to support e.g. the 
labels described on p18 of the 'healthcare classification system' (HCS) 
(http://www.hl7.org/documentcenter/public/wg/secure/3.%20HCS%20Guide%20Final%202013%200322%20JMD.pdf). 
The HCS security label is based on the NIST's FIPS188 standard / 
http://www.itl.nist.gov/fipspubs/fip188.htm


regards,
Yeb Havinga



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


Re: [HACKERS] Transaction-lifespan memory leak with plpgsql DO blocks

2013-11-15 Thread Yeb Havinga

On 2013-11-14 22:23, Tom Lane wrote:


So after some experimentation I came up with version 2, attached.


Thanks for looking into this! I currently do not have access to a setup 
to try the patch. A colleague of mine will look into this next week.


Thanks again,
Yeb Havinga


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


Re: [HACKERS] Possible memory leak with SQL function?

2013-09-16 Thread Yeb Havinga

On 2013-09-13 18:32, Robert Haas wrote:

On Thu, Sep 12, 2013 at 5:29 AM, Yeb Havinga yebhavi...@gmail.com wrote:

Is the following known behaviour, or should I put some time in writing a
self contained test case?

We have a function that takes a value and returns a ROW type. With the
function implemented in language SQL, when executing this function in a
large transaction, memory usage of the backend process increases.
MemoryContextStats showed a lot of SQL function data. Debugging
init_sql_fcache() showed that it was for the same function oid each time,
and the oid was the function from value to ROW type.

When the function is implemented in PL/pgSQL, the memory usage was much
less.

I'm sorry I cannot be more specific at the moment, such as what is 'much
less' memory with a PL/pgSQl function, and are there as many SQL function
data's as calls to the SQL function, because I would have to write a test
case for this. I was just wondering, if this is known behavior of SQL
functions vs PL/pgSQL functions, or could it be a bug?

It sounds like a bug to me, although I can't claim to know everything
there is to know about this topic.

I spent some time writing a test case, but failed to make a test case 
that showed the memory difference I described upthread, in contrast, in 
the test below, the SQL function actually shows a smaller memory 
footprint than the plpgsql counterpart. This test case only demonstrates 
that in a long running transaction, calling sql or plpgsql functions 
causes increasing memory usage that is not released until after commit.


callit.sql:
--
DO
$$
DECLARE  b text;
 i int;
BEGIN
--   SELECT 'a' into b; -- memory constant
   i := fp('a'); -- memory increases
--   i := fs('a'); -- memory increases but slow
END;
$$ LANGUAGE plpgsql;
-

sqlvsplpgsql.sql:
-
CREATE OR REPLACE FUNCTION fp (a text)
 RETURNS int
 AS $$
DECLARE result int;
BEGIN
SELECT 10 INTO result;
RETURN result;
END;
$$
LANGUAGE plpgsql;
CREATE OR REPLACE FUNCTION fs (a text)
 RETURNS int
 AS $$
 SELECT 10;
$$
LANGUAGE sql;
\i callit.sql
-


rm /tmp/ff /tmp/ff2 ; cp callit.sql /tmp/ff ; cat /tmp/ff /tmp/ff  
/tmp/ff2; cat /tmp/ff2 /tmp/ff2  /tmp/ff; cat /tmp/ff /tmp/ff  
/tmp/ff2; cat /tmp/ff2 /tmp/ff2  /tmp/ff;cat /tmp/ff /tmp/ff  
/tmp/ff2; cat /tmp/ff2 /tmp/ff2  /tmp/ff;cat /tmp/ff /tmp/ff  
/tmp/ff2; cat /tmp/ff2 /tmp/ff2  /tmp/ff;cat /tmp/ff /tmp/ff  
/tmp/ff2; cat /tmp/ff2 /tmp/ff2  /tmp/ff;cat /tmp/ff /tmp/ff  
/tmp/ff2; cat /tmp/ff2 /tmp/ff2  /tmp/ff;cat /tmp/ff /tmp/ff  
/tmp/ff2; cat /tmp/ff2 /tmp/ff2  /tmp/ff


psql -1 postgres -f /tmp/ff

Then watch htop in another terminal.


--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data



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


[HACKERS] Possible memory leak with SQL function?

2013-09-12 Thread Yeb Havinga

Hello list,

Is the following known behaviour, or should I put some time in writing a 
self contained test case?


We have a function that takes a value and returns a ROW type. With the 
function implemented in language SQL, when executing this function in a 
large transaction, memory usage of the backend process increases. 
MemoryContextStats showed a lot of SQL function data. Debugging 
init_sql_fcache() showed that it was for the same function oid each 
time, and the oid was the function from value to ROW type.


When the function is implemented in PL/pgSQL, the memory usage was much 
less.


I'm sorry I cannot be more specific at the moment, such as what is 'much 
less' memory with a PL/pgSQl function, and are there as many SQL 
function data's as calls to the SQL function, because I would have to 
write a test case for this. I was just wondering, if this is known 
behavior of SQL functions vs PL/pgSQL functions, or could it be a bug?


--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] Parallell Optimizer

2013-06-12 Thread Yeb Havinga

On 2013-06-07 19:09, FredDaniPandoraAquiles wrote:
I asked a while ago in this group about the possibility to implement a 
parallel planner in a multithread way, and  the replies were that the 
proposed approach couldn't be implemented, because the postgres is not 
thread-safe. With the new feature Background Worker Processes, such 
implementation would be possible? If yes, do you can see possible 
problems in implement this approach, for example, the bgprocess can't 
access some planning core functions like make_join_rel, acess them in 
parallel and so on. I want start a research to work in a parallel 
planner in postgres, I succeeded in in the DBMS H2, but my first 
option still is the postgres, and any help is welcome.


The topic has been researched and experimented with on PostgreSQL 8.3, 
described in a vldb-2008 paper called Parallelizing Query Optimization, 
available on http://www.vldb.org/pvldb/1/1453882.pdf


regards,
Yeb

PS: apologies for redundant posting.



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


Re: [HACKERS] JSON NULLs

2013-02-11 Thread Yeb Havinga

On 2013-02-10 16:03, Andrew Dunstan wrote:

On 02/10/2013 05:43 AM, Yeb Havinga wrote:
3. I was wondering how to access the first author from this json 
snippet:


{
 id: QZr82w_eSi8C,
 etag: KZ+JsrkCdqw,
 volumeInfo: {
  title: Heads Up Software Construction,
  authors: [
   Dan Malone,
   Dave Riles
  ],




try:

   json_get_path_as_text(document,  'volumeInfo', 'authors', '0')


There are other ways to spell this, too:

   json_get_path_as_text(document,  variadic
   '{volumeInfo,authors,0}'::text[])


or

   document-'{volumeInfo,authors,0}'::text[]


That works very nice, thanks!


I'm actually wondering if we should use different operator names for 
the get_path*op functions so we wouldn't need to type qualify the path 
argument. Maybe ? and ? although I'm reluctant to use ? in an 
operator given the recent JDBC discussion. Or perhaps # and #.


different operator name: +1.

thanks
Yeb



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


Re: [HACKERS] JSON NULLs

2013-02-10 Thread Yeb Havinga

On 2013-02-08 15:15, Andrew Dunstan wrote:




Revised patch attached. The problem also existed with the get*_as_text 
functions (and their operators). Some additional regression tests are 
added to test these cases.


Hi,

I did some minor things with the patch today.

1. thanks for the work on the json type, great to see it in Postgres and 
also more functions on it!


2.
during compile on

jsonfuncs.c: In function `each_object_field_end':
jsonfuncs.c:1151:13: warning: assignment makes integer from pointer 
without a cast


yeb@unix:~/ff$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/x86_64-linux-gnu/gcc/x86_64-linux-gnu/4.5.2/lto-wrapper
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu/Linaro 
4.5.2-8ubuntu4' --with-bugurl=file:///usr/share/doc/gcc-4.5/README.Bugs 
--enable-languages=c,c++,fortran,objc,obj-c++ --prefix=/usr 
--program-suffix=-4.5 --enable-shared --enable-multiarch 
--with-multiarch-defaults=x86_64-linux-gnu --enable-linker-build-id 
--with-system-zlib --libexecdir=/usr/lib/x86_64-linux-gnu 
--without-included-gettext --enable-threads=posix 
--with-gxx-include-dir=/usr/include/c++/4.5 
--libdir=/usr/lib/x86_64-linux-gnu --enable-nls --with-sysroot=/ 
--enable-clocale=gnu --enable-libstdcxx-debug 
--enable-libstdcxx-time=yes --enable-plugin --enable-gold 
--enable-ld=default --with-plugin-ld=ld.gold --enable-objc-gc 
--disable-werror --with-arch-32=i686 --with-tune=generic 
--enable-checking=release --build=x86_64-linux-gnu 
--host=x86_64-linux-gnu --target=x86_64-linux-gnu

Thread model: posix
gcc version 4.5.2 (Ubuntu/Linaro 4.5.2-8ubuntu4)

3. I was wondering how to access the first author from this json snippet:

{
 id: QZr82w_eSi8C,
 etag: KZ+JsrkCdqw,
 volumeInfo: {
  title: Heads Up Software Construction,
  authors: [
   Dan Malone,
   Dave Riles
  ],


and played a bit with json_get_path_as_text(document,  'volumeInfo', 
'authors') that accepts a list of keys as arguments. Have you thought 
about an implementation that would accept a single path argument like 
'volumeInfo.authors[0]' ? This might be more powerful and easy to use, 
since the user does not need to call another function to get the first 
element from the author array, and the function call does not need to be 
changed when path lenghts change.


My apologies if this has been discussed before - I've gone through 
threads from nov 2012 but did not find a previous discussion about this 
topic.


regards,
Yeb Havinga



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


Re: [HACKERS] Review of Row Level Security

2012-12-19 Thread Yeb Havinga

On 2012-12-19 18:25, Robert Haas wrote:

On Tue, Dec 18, 2012 at 3:39 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote:

postgres= INSERT INTO t1 VALUES (4,'ddd');
INSERT 0 1
postgres= INSERT INTO t1 VALUES (5,'eee');
ERROR:  new row for relation t1 violates row-secirity
DETAIL:  Failing row contains (5, eee).

I've argued against this before - and maybe I should drop my
objection, because a number of people seem to be on the other side.
But I still think there will be some people who don't want this
behavior.  Right now, for example, you can give someone INSERT but not
SELECT permission on a table, and they will then be able to put rows
into the table that they cannot read back.  Similarly, in the RLS
case, it is not necessarily undesirable for a user to be able to
insert a row that they can't read back; or for them to be able to
update a row from a value that they can see to one that they cannot.
Some people will want to prohibit that, while others will not.


Maybe it is an idea to provide different RLS expressions for read and 
write. I remember reading a scenario (it might be well known in security 
land) where it is possible to write to authorization levels = users 
level, and read levels = the users level. In this setup Kevin's address 
example is possible, a user could write to e.g. the highest level, but 
then not read it anymore if his own level was lower than the highest. 
This setup also shows that to implement it, one would need a different 
expression for read and write (or the rls expression should know the 
query's commandtype).


regards,
Yeb



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


Re: [HACKERS] Global Sequences

2012-10-16 Thread Yeb Havinga

On 2012-10-15 23:33, Simon Riggs wrote:

So, proposal is to allow nextval() allocation to access a plugin,
rather than simply write a WAL record and increment. If the plugin is
loaded all sequences call it (not OIDs).
+1. It is currently impossible to alter nextvals behaviour, without 
making changes in core. It is possible to define an alternative 
implementation and try to force to use it by using the search_path, but 
serial datatypes are always bound to pg_catalog.nextval(). This would 
enable every distributed PostgreSQL system to make a cleaner 
implementation for global sequences than they currently have, and would 
also encourage reuse of distributed nextval implementations.


regards,
Yeb

--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] [RFC] Interface of Row Level Security

2012-05-31 Thread Yeb Havinga

On 2012-05-30 21:26, Kohei KaiGai wrote:

If we would have an ideal optimizer, I'd still like the optimizer to
wipe out redundant clauses transparently, rather than RLSBYPASS
permissions, because it just controls all-or-nothing stuff.
For example, if tuples are categorized to unclassified, classified or
secret, and RLS policy is configured as:
   ((current_user IN ('alice', 'bob') AND X IN ('unclassified',
'classified')) OR (X IN 'unclassified)),
superuser can see all the tuples, and alice and bob can see
up to classified tuples.
Is it really hard to wipe out redundant condition at planner stage?
If current_user is obviously 'kaigai', it seems to me the left-side of
this clause can be wiped out at the planner stage.


The query's RLS policy would be simpler if the RLS policy function that 
returns the WHERE clause would take the user as argument, so its result 
does not contain user conditionals.


IF (current_user IN ('alice', 'bob')
THEN
  RETURN X IN ('unclassified', 'classified'))
ELSE
  RETURN X IN ('unclassified')
END IF;


regards,
Yeb



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


Re: [HACKERS] bugfix for cursor arguments in named notation

2012-04-05 Thread Yeb Havinga

On 2012-04-04 17:10, Tom Lane wrote:

Yeb Havingayebhavi...@gmail.com  writes:

Using a cursor argument name equal to another plpgsql variable results
in the error:
cursor .. has no argument named 

I think a better way would be to temporarily set
plpgsql_IdentifierLookup to IDENTIFIER_LOOKUP_DECLARE, so as to suppress
variable name lookup in the first place.  The patch as you have it seems
to me to make too many assumptions about what name lookup might produce.




Thank you for looking at it. Attached is a patch that implements your 
suggestion.


regards,
Yeb

From b762f447615795c65136d02495cb72f4a1293af3 Mon Sep 17 00:00:00 2001
From: Willem  Yeb w...@mgrid.net
Date: Thu, 5 Apr 2012 09:53:38 +0200
Subject: [PATCH] Fix cursor has no argument named  error.

---
 src/pl/plpgsql/src/gram.y |3 +++
 src/test/regress/expected/plpgsql.out |   18 ++
 src/test/regress/sql/plpgsql.sql  |   15 +++
 3 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/src/pl/plpgsql/src/gram.y b/src/pl/plpgsql/src/gram.y
index 5a555af..0f37c4f 100644
--- a/src/pl/plpgsql/src/gram.y
+++ b/src/pl/plpgsql/src/gram.y
@@ -3445,9 +3445,12 @@ read_cursor_args(PLpgSQL_var *cursor, int until, const char *expected)
 		if (tok1 == IDENT  tok2 == COLON_EQUALS)
 		{
 			char   *argname;
+			IdentifierLookup oldlookup = plpgsql_IdentifierLookup;
 
 			/* Read the argument name, and find its position */
+			plpgsql_IdentifierLookup = IDENTIFIER_LOOKUP_DECLARE;
 			yylex();
+			plpgsql_IdentifierLookup = oldlookup;
 			argname = yylval.str;
 
 			for (argpos = 0; argpos  row-nfields; argpos++)
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 5455ade..56cfa57 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -2420,6 +2420,24 @@ select namedparmcursor_test8();
  0
 (1 row)
 
+-- cursor parameter named the same as other plpgsql variables
+create or replace function namedparmcursor_test9(p1 int) returns int4 as $$
+declare
+  c1 cursor (p1 int, p2 int) for
+select count(*) from tenk1 where thousand = p1 and tenthous = p2;
+  n int4;
+  p2 int4 := 1006;
+begin
+  open c1 (p1 := p1,  p2 := p2);
+  fetch c1 into n;
+  return n;
+end $$ language plpgsql;
+select namedparmcursor_test9(6);
+ namedparmcursor_test9 
+---
+ 1
+(1 row)
+
 --
 -- tests for raise processing
 --
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index f577dc3..6b9795d 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -2053,6 +2053,21 @@ begin
 end $$ language plpgsql;
 select namedparmcursor_test8();
 
+-- cursor parameter named the same as other plpgsql variables
+create or replace function namedparmcursor_test9(p1 int) returns int4 as $$
+declare
+  c1 cursor (p1 int, p2 int) for
+select count(*) from tenk1 where thousand = p1 and tenthous = p2;
+  n int4;
+  p2 int4 := 1006;
+begin
+  open c1 (p1 := p1,  p2 := p2);
+  fetch c1 into n;
+  return n;
+end $$ language plpgsql;
+
+select namedparmcursor_test9(6);
+
 --
 -- tests for raise processing
 --
-- 
1.7.1


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


[HACKERS] bugfix for cursor arguments in named notation

2012-04-04 Thread Yeb Havinga
Using a cursor argument name equal to another plpgsql variable results 
in the error:

cursor .. has no argument named 

The attached patch fixes that.

Instead of solving the issue like is done in the patch, another way 
would be to expose internal_yylex() so that could be used instead of 
yylex() by read_cursor_args when reading the argument name, and would 
always return the argument name in yylval.str.


--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

From 47c451cbf188ac2aff9784bff73bc7fb7b846d26 Mon Sep 17 00:00:00 2001
From: Willem  Yeb w...@mgrid.net
Date: Wed, 4 Apr 2012 11:30:41 +0200
Subject: [PATCH] Fix cursor has no argument named  error.

When a cursor argument name coincided with another plpgsql variable name,
yylex() returns it not as str but as a wdatum.
---
 src/pl/plpgsql/src/gram.y |6 --
 src/test/regress/expected/plpgsql.out |   18 ++
 src/test/regress/sql/plpgsql.sql  |   15 +++
 3 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/src/pl/plpgsql/src/gram.y b/src/pl/plpgsql/src/gram.y
index 5a555af..2d52278 100644
--- a/src/pl/plpgsql/src/gram.y
+++ b/src/pl/plpgsql/src/gram.y
@@ -3447,8 +3447,10 @@ read_cursor_args(PLpgSQL_var *cursor, int until, const char *expected)
 			char   *argname;
 
 			/* Read the argument name, and find its position */
-			yylex();
-			argname = yylval.str;
+			if (yylex() == T_DATUM)
+argname = yylval.wdatum.ident;
+			else
+argname = yylval.str;
 
 			for (argpos = 0; argpos  row-nfields; argpos++)
 			{
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 5455ade..56cfa57 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -2420,6 +2420,24 @@ select namedparmcursor_test8();
  0
 (1 row)
 
+-- cursor parameter named the same as other plpgsql variables
+create or replace function namedparmcursor_test9(p1 int) returns int4 as $$
+declare
+  c1 cursor (p1 int, p2 int) for
+select count(*) from tenk1 where thousand = p1 and tenthous = p2;
+  n int4;
+  p2 int4 := 1006;
+begin
+  open c1 (p1 := p1,  p2 := p2);
+  fetch c1 into n;
+  return n;
+end $$ language plpgsql;
+select namedparmcursor_test9(6);
+ namedparmcursor_test9 
+---
+ 1
+(1 row)
+
 --
 -- tests for raise processing
 --
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index f577dc3..6b9795d 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -2053,6 +2053,21 @@ begin
 end $$ language plpgsql;
 select namedparmcursor_test8();
 
+-- cursor parameter named the same as other plpgsql variables
+create or replace function namedparmcursor_test9(p1 int) returns int4 as $$
+declare
+  c1 cursor (p1 int, p2 int) for
+select count(*) from tenk1 where thousand = p1 and tenthous = p2;
+  n int4;
+  p2 int4 := 1006;
+begin
+  open c1 (p1 := p1,  p2 := p2);
+  fetch c1 into n;
+  return n;
+end $$ language plpgsql;
+
+select namedparmcursor_test9(6);
+
 --
 -- tests for raise processing
 --
-- 
1.7.1


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


[HACKERS] Recent MinGW postgres builds with -O2 do not pass regression tests

2012-03-18 Thread Yeb Havinga
When building a minimal PostgreSQL under the latest mingw (2018), 
make check will give a few dozen fails with the server exiting on code 
2. The build is fine when -O2 is removed from the CFLAGS. This behaviour 
is present on all revs on the REL9_1_STABLE branch that I tested, among 
which were 9.1.3 and 9.1.0.


$ gcc -v
Using built-in specs.
COLLECT_GCC=C:\MinGW\bin\gcc.exe
COLLECT_LTO_WRAPPER=c:/mingw/bin/../libexec/gcc/mingw32/4.6.1/lto-wrapper.exe
Target: mingw32
Configured with: ../gcc-4.6.1/configure 
--enable-languages=c,c++,fortran,objc,ob
j-c++ --disable-sjlj-exceptions --with-dwarf2 --enable-shared 
--enable-libgomp -
-disable-win32-registry --enable-libstdcxx-debug 
--enable-version-specific-runti

me-libs --build=mingw32 --prefix=/mingw
Thread model: win32
gcc version 4.6.1 (GCC)

regards,
Yeb


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


Re: [HACKERS] [v9.2] Add GUC sepgsql.client_label

2012-03-16 Thread Yeb Havinga

On 2012-03-15 21:45, Robert Haas wrote:

On Wed, Mar 14, 2012 at 11:10 AM, Kohei KaiGaikai...@kaigai.gr.jp  wrote:

If it is ready to commit, please remember the credit to Yeb's volunteer
on this patch.

Done.

In the patch with copy-editing documentation following that commit, at 
in at their option, s/in// ? Also 'rather than .. as mandated by the 
system': I'm having trouble parsing 'as'. It is also unclear to me what 
'system' means: selinux or PostgreSQL, or both? I suspect it is 
PostgreSQL, since selinux is still enforcing / 'mandating' it's policy. 
What about rather than that the switch is controlled by the PostgreSQL 
server, as in the case of a trusted procedure.


+Dynamic domain transitions should be considered carefully, because they
+allow users to switch their label, and therefore their privileges, in
+at their option, rather than (as in the case of a trusted procedure)
+as mandated by the system.

--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] EquivalenceClasses and subqueries and PlaceHolderVars, oh my

2012-03-15 Thread Yeb Havinga

On 2012-03-15 02:29, Tom Lane wrote:

 explain select * from
  (select thousand as t1, tenthous as t2 from tenk1 a
   union all
   select 42 as t1, 42 as t2 from tenk1 b) c
 order by t1, t2;

 There is an EquivalenceClass for each of t1 and t2, and if we don't
 do something like wrapping the constants with distinct PHVs, then
 add_child_rel_equivalences will end up pushing identical constants into
 both ECs, thus totally bollixing the fundamental rule that any expression
 should match at most one EC.

I'm having a hard time imagining that add_child_rel_equivalences is not 
just plain wrong. Even though it will only add child equivalence members 
to a parent eq class when certain conditions are met, isn't it the case 
that since a union (all) is addition of tuples and not joining, any kind 
of propagating restrictions on a append rel child member to other areas 
of the plan can cause unwanted results, like the ones currently seen?


regards,
Yeb Havinga


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


Re: [HACKERS] [v9.2] Add GUC sepgsql.client_label

2012-03-11 Thread Yeb Havinga

On 2012-03-10 10:39, I  wrote:


I can probably write some docs tomorrow.



Attached is v5 of the patch, with is exactly equal to v4 but with added 
documentation.


Some other notes.

- Robert asked why sepgsql_setcon with NULL to reset the value to the 
initial client label was supported. Maybe this could be a leftover from 
the initial implementation as GUC variable?
- earlier I suggested preventing setting a new client label from a 
trusted procedure, however I just read in the original post that this 
was how the current usecase of Joshua is set up. Suggestion withdrawn.


--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

diff --git a/contrib/sepgsql/expected/label.out b/contrib/sepgsql/expected/label.out
index bac169f..e967c7c 100644
--- a/contrib/sepgsql/expected/label.out
+++ b/contrib/sepgsql/expected/label.out
@@ -26,6 +26,11 @@ CREATE FUNCTION f4 () RETURNS text
 AS 'SELECT sepgsql_getcon()'
 LANGUAGE sql;
 SECURITY LABEL ON FUNCTION f4()
+IS 'system_u:object_r:sepgsql_nosuch_trusted_proc_exec_t:s0';
+CREATE FUNCTION f5 (text) RETURNS bool
+	AS 'SELECT sepgsql_setcon($1)'
+LANGUAGE sql;
+SECURITY LABEL ON FUNCTION f5(text)
 IS 'system_u:object_r:sepgsql_regtest_trusted_proc_exec_t:s0';
 --
 -- Tests for default labeling behavior
@@ -100,6 +105,223 @@ SELECT sepgsql_getcon();	-- client's label must be restored
 (1 row)
 
 --
+-- Test for Dynamic Domain Transition
+--
+-- validation of transaction aware dynamic-transition
+SELECT sepgsql_getcon();	-- confirm client privilege
+  sepgsql_getcon  
+--
+ unconfined_u:unconfined_r:unconfined_t:s0:c0.c25
+(1 row)
+
+SELECT sepgsql_setcon('unconfined_u:unconfined_r:unconfined_t:s0:c0.c15');
+ sepgsql_setcon 
+
+ t
+(1 row)
+
+SELECT sepgsql_getcon();
+  sepgsql_getcon  
+--
+ unconfined_u:unconfined_r:unconfined_t:s0:c0.c15
+(1 row)
+
+SELECT sepgsql_setcon(NULL);	-- failed to reset
+ERROR:  SELinux: security policy violation
+SELECT sepgsql_getcon();
+  sepgsql_getcon  
+--
+ unconfined_u:unconfined_r:unconfined_t:s0:c0.c15
+(1 row)
+
+BEGIN;
+SELECT sepgsql_setcon('unconfined_u:unconfined_r:unconfined_t:s0:c0.c12');
+ sepgsql_setcon 
+
+ t
+(1 row)
+
+SELECT sepgsql_getcon();
+  sepgsql_getcon  
+--
+ unconfined_u:unconfined_r:unconfined_t:s0:c0.c12
+(1 row)
+
+SAVEPOINT svpt_1;
+SELECT sepgsql_setcon('unconfined_u:unconfined_r:unconfined_t:s0:c0.c9');
+ sepgsql_setcon 
+
+ t
+(1 row)
+
+SELECT sepgsql_getcon();
+ sepgsql_getcon  
+-
+ unconfined_u:unconfined_r:unconfined_t:s0:c0.c9
+(1 row)
+
+SAVEPOINT svpt_2;
+SELECT sepgsql_setcon('unconfined_u:unconfined_r:unconfined_t:s0:c0.c6');
+ sepgsql_setcon 
+
+ t
+(1 row)
+
+SELECT sepgsql_getcon();
+ sepgsql_getcon  
+-
+ unconfined_u:unconfined_r:unconfined_t:s0:c0.c6
+(1 row)
+
+SAVEPOINT svpt_3;
+SELECT sepgsql_setcon('unconfined_u:unconfined_r:unconfined_t:s0:c0.c3');
+ sepgsql_setcon 
+
+ t
+(1 row)
+
+SELECT sepgsql_getcon();
+ sepgsql_getcon  
+-
+ unconfined_u:unconfined_r:unconfined_t:s0:c0.c3
+(1 row)
+
+ROLLBACK TO SAVEPOINT svpt_2;
+SELECT sepgsql_getcon();		-- should be 's0:c0.c9'
+ sepgsql_getcon  
+-
+ unconfined_u:unconfined_r:unconfined_t:s0:c0.c9
+(1 row)
+
+ROLLBACK TO SAVEPOINT svpt_1;
+SELECT sepgsql_getcon();		-- should be 's0:c0.c12'
+  sepgsql_getcon  
+--
+ unconfined_u:unconfined_r:unconfined_t:s0:c0.c12
+(1 row)
+
+ABORT;
+SELECT sepgsql_getcon();		-- should be 's0:c0.c15'
+  sepgsql_getcon  
+--
+ unconfined_u:unconfined_r:unconfined_t:s0:c0.c15
+(1 row)
+
+BEGIN;
+SELECT sepgsql_setcon('unconfined_u:unconfined_r:unconfined_t:s0:c0.c8');
+ sepgsql_setcon 
+
+ t
+(1 row)
+
+SELECT sepgsql_getcon();
+ sepgsql_getcon  
+-
+ unconfined_u:unconfined_r:unconfined_t:s0:c0.c8
+(1 row)
+
+SAVEPOINT svpt_1;
+SELECT sepgsql_setcon('unconfined_u:unconfined_r:unconfined_t:s0:c0.c4');
+ sepgsql_setcon 
+
+ t
+(1 row)
+
+SELECT sepgsql_getcon();
+ sepgsql_getcon

Re: [HACKERS] [v9.2] Add GUC sepgsql.client_label

2012-03-11 Thread Yeb Havinga

On 2012-03-11 11:33, Yeb Havinga wrote:

On 2012-03-10 10:39, I  wrote:


I can probably write some docs tomorrow.



Attached is v5 of the patch, with is exactly equal to v4 but with 
added documentation.


s/literalcube(1) == '(1)'/literal// in that patch please



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


Re: [HACKERS] [v9.2] Add GUC sepgsql.client_label

2012-03-10 Thread Yeb Havinga

On 2012-03-09 21:49, Robert Haas wrote:

On Tue, Mar 6, 2012 at 9:14 AM, Kohei KaiGaikai...@kaigai.gr.jp  wrote:

[ new patch ]

Are we absolutely certain that we want the semantics of
sepgsql_setcon() to be transactional?  Because if we made them
non-transactional, this would be a whole lot simpler, and it would
still meet the originally proposed use case, which is to allow the
security context of a connection to be changed on a one-time basis
before handing it off to a client application.


It would meet the original use case, but outside of that use case it 
would be very easy to get POLA violations. Imagine a transaction like

1- do stuff under label A
2- setcon to B
3- do stuff under label B

When that transaction fails due to a serialization error, one would 
expect that when the transaction is replayed, the initial actions are 
executed under label A. If it was B, or any other further label in the 
original transaction, it would be very hard to develop software in user 
space that could cope with this behaviour.

As a separate but related note, the label management here seems to be
excessively complicated.  In particular, it seems to me that the
semantics of sepgsql_get_client_label() become quite muddled under
this patch.  An explicitly-set label overrides the default label, but
a trusted procedure's temporary label overrides that.  So if you enter
a trusted procedure, and it calls sepgsql_setcon(), it'll change the
label, but no actual security transition will occur; then, when you
exit the trusted procedure, you'll get the new label in place of
whatever the caller had before.  That seems like one heck of a
surprising set of semantics.


I agree that sepgsql_get_*client*_label does not really match with a 
trusted procedure temporarily changing it.

It seems to me that it would make more sense to view the set of
security labels in effect as a stack.  When we enter a trusted
procedure, it pushes a new label on the stack; when we exit a trusted
procedure, it pops the top label off the stack.  sepgsql_setcon()
changes the top label on the stack.  If we want to retain
transactional semantics, then you can also have a toplevel label that
doesn't participate in the stack; a commit copies the sole item on the
stack into the toplevel label, and transaction start copies the
toplevel label into an empty stack.


Yes the additions be sepgsql_setcon look like a stack for pushing. 
However, the current code that rollbacks the pending list to a certain 
savepoint matches code from e.g. StandbyReleaseLocks(), so having a 
concept like this as a list is not unknown to the current codebase.

   In the current coding, I think
client_label_peer is redundant with client_label_committed - once the
latter is set, the former is unused, IIUC


client_label_peer is used on reset of the client label, i.e. calling 
sepgsql_setcon with NULL.



  - and what I'm proposing is
that client_label_func shouldn't be separate, but rather should mutate
the stack of labels maintained by client_label_pending.


The drawback of having trusted procedures push things on this stack is 
that it would add a memory alloc and size overhead when calling trusted 
functions, especially for recursive functions.



The docs need updating, both to reflect the version bump in
sepgsql-regtest.te and to describe the actual feature.


I can probably write some docs tomorrow.

regards,
Yeb Havinga


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


Re: [HACKERS] [v9.2] Add GUC sepgsql.client_label

2012-03-10 Thread Yeb Havinga

On 2012-03-10 14:06, Robert Haas wrote:

On Sat, Mar 10, 2012 at 4:39 AM, Yeb Havingayebhavi...@gmail.com  wrote:

As a separate but related note, the label management here seems to be
excessively complicated.  In particular, it seems to me that the
semantics of sepgsql_get_client_label() become quite muddled under
this patch.  An explicitly-set label overrides the default label, but
a trusted procedure's temporary label overrides that.  So if you enter
a trusted procedure, and it calls sepgsql_setcon(), it'll change the
label, but no actual security transition will occur; then, when you
exit the trusted procedure, you'll get the new label in place of
whatever the caller had before.  That seems like one heck of a
surprising set of semantics.

I agree that sepgsql_get_*client*_label does not really match with a trusted
procedure temporarily changing it.

I'm not complaining about the name of the function; I'm complaining
about the semantics.


The semantics are muddled because the client labels are mixed with 
labels from trusted procedures. The stack you described upthread may 
also exhibit surprising behaviour. If a trusted procedure is called, 
it's label is pushed onto the stack. Suppose it pushes another label by 
calling sepgsql_setcon(). In the stack case, this is now the top of the 
stack and the result of sepgsql_get_client_label(). The procedure 
exists. What should now be the result of sepgsql_get_client_label()? 
Since labels are managed by a stack, we can only pop what's on top and 
need to pop twice, so we've lost the label pushed onto the stack by the 
trusted function, which means that trusted procedures cannot be used to 
change client labels beyond their own scope, but other functions would.


Maybe this semantics muddling of trusted procedures and setting the 
client label can be avoided by denying changing the client label with 
sepgsql_setcon() from a trusted procedure, which would also make some sense:


ERROR: cannot override the client label of a trusted procedure during 
execution.


--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] [v9.2] Add GUC sepgsql.client_label

2012-03-09 Thread Yeb Havinga

On 2012-03-06 15:14, Kohei KaiGai wrote:

In case of sepgsql_setcon() being invoked with null argument
to reset security label of the client, but not committed yet,
the last item of the client_label_pending has null label.
(It performs as a mark of a security label being reset.)
Yes, I see that now. Another solution could be to append 
client_label_peer on the pending list instead of NULL, but maybe this is 
not important enough to discuss. I tried to do some testing by first 
transition into a smaller MLS context, then reset to the original again, 
but that is not allowed by the regtest policy. I searched the internet 
for 30 minutes about how to write a allow rule that would allow e.g. the 
transition from c1.c4 back to c1.c1023 but failed.


two other minor nitpicks

-- * contrib/sepgsql/label.c
-+ * contrib/sepgsqlet/label.c

typo here..

-+  /* Append the supplied new_label on the pending list. */
++  /*
++   * Append the supplied new_label on the pending list until
++   * the current transaction is committed.
++   */

the 'until the current transaction is committed' part is something going 
on outside of sepgsql_set_client_label() - this function just appends to 
the list, always. That the list is reset on transaction commit/abort 
time, is done and also already documented elsewhere. A new reader could 
be confused to not find transaction related things in 
sepgsql_set_client_label().


regards,
Yeb

--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] [v9.2] Add GUC sepgsql.client_label

2012-03-03 Thread Yeb Havinga

On 2012-02-24 17:25, Yeb Havinga wrote:

On 2012-02-23 12:17, Kohei KaiGai wrote:

2012/2/20 Yeb Havingayebhavi...@gmail.com:

On 2012-02-05 10:09, Kohei KaiGai wrote:
The attached part-1 patch moves related routines from hooks.c to 
label.c
because of references to static variables. The part-2 patch 
implements above

mechanism.


I took a short look at this patch but am stuck getting the 
regression test

to run properly.

First, patch 2 misses the file sepgsql.sql.in and therefore the 
creation

function command for sepgsql_setcon is missing.


Thanks for your comments.

I added the definition of sepgsql_setcon function to sepgsql.sql.in 
file,

in addition to patch rebasing.


Very brief comments due to must leave keyboard soon:

I read the source code and played a bit with setcon and the debugger, 
no strange things found.


Code comments / questions:


I took the liberty to change a few things, mostly comments, in the 
attached patch:


maybe client_label_committed is a better name for client_label_setcon?


this change was made.


Is the double negation in the sentence below intended?


several comments were changed / moved. There is now one place where te 
behaviour of the different client_label variables are explained.




sepgsql_set_client_label(), maybe add a comment to !new_label that it 
is reset to the peer label.


done.



Is the assert client_label_peer != NULL in sepgsql_get_client_label 
necessary?
new_label == NULL / pending_label-label == NULL means use the peer 
label. Why not use the peer label instead?


It turned out that pending_label-label is invariantly non null. Changed 
code to assume that and added some Asserts.




set_label: if new_label == current label according to getcon, is it 
necessary to add to the pending list?


this question still remains. Maybe there would be reasons to hit selinux 
with the question: can I change from A-A.


sepgsql_subxact_callback(), could this be made easier to read by just 
taking llast(client_label_pending), assert that plabel-subid == 
mySubId and then list_delete on pointer of that listcell?


no this was a naieve suggestion, which fails in any case of a 
subtransaction with not exactly one call to sepgsql_setcon()


Some comments contain typos, I can spend some time on this, though I'm 
not a native english speaker so it won't be perfect.


sgml documentation must still be added. If time permits I can spend some 
time on that tomorrow.


regards,
Yeb Havinga


--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

diff --git a/contrib/sepgsql/expected/label.out b/contrib/sepgsql/expected/label.out
index bac169f..e967c7c 100644
--- a/contrib/sepgsql/expected/label.out
+++ b/contrib/sepgsql/expected/label.out
@@ -26,6 +26,11 @@ CREATE FUNCTION f4 () RETURNS text
 AS 'SELECT sepgsql_getcon()'
 LANGUAGE sql;
 SECURITY LABEL ON FUNCTION f4()
+IS 'system_u:object_r:sepgsql_nosuch_trusted_proc_exec_t:s0';
+CREATE FUNCTION f5 (text) RETURNS bool
+	AS 'SELECT sepgsql_setcon($1)'
+LANGUAGE sql;
+SECURITY LABEL ON FUNCTION f5(text)
 IS 'system_u:object_r:sepgsql_regtest_trusted_proc_exec_t:s0';
 --
 -- Tests for default labeling behavior
@@ -100,6 +105,223 @@ SELECT sepgsql_getcon();	-- client's label must be restored
 (1 row)
 
 --
+-- Test for Dynamic Domain Transition
+--
+-- validation of transaction aware dynamic-transition
+SELECT sepgsql_getcon();	-- confirm client privilege
+  sepgsql_getcon  
+--
+ unconfined_u:unconfined_r:unconfined_t:s0:c0.c25
+(1 row)
+
+SELECT sepgsql_setcon('unconfined_u:unconfined_r:unconfined_t:s0:c0.c15');
+ sepgsql_setcon 
+
+ t
+(1 row)
+
+SELECT sepgsql_getcon();
+  sepgsql_getcon  
+--
+ unconfined_u:unconfined_r:unconfined_t:s0:c0.c15
+(1 row)
+
+SELECT sepgsql_setcon(NULL);	-- failed to reset
+ERROR:  SELinux: security policy violation
+SELECT sepgsql_getcon();
+  sepgsql_getcon  
+--
+ unconfined_u:unconfined_r:unconfined_t:s0:c0.c15
+(1 row)
+
+BEGIN;
+SELECT sepgsql_setcon('unconfined_u:unconfined_r:unconfined_t:s0:c0.c12');
+ sepgsql_setcon 
+
+ t
+(1 row)
+
+SELECT sepgsql_getcon();
+  sepgsql_getcon  
+--
+ unconfined_u:unconfined_r:unconfined_t:s0:c0.c12
+(1 row)
+
+SAVEPOINT svpt_1;
+SELECT sepgsql_setcon('unconfined_u:unconfined_r:unconfined_t:s0:c0.c9');
+ sepgsql_setcon 
+
+ t
+(1 row)
+
+SELECT sepgsql_getcon();
+ sepgsql_getcon  
+-
+ unconfined_u:unconfined_r:unconfined_t:s0:c0.c9
+(1 row)
+
+SAVEPOINT svpt_2;
+SELECT sepgsql_setcon('unconfined_u:unconfined_r:unconfined_t:s0:c0.c6');
+ sepgsql_setcon 
+
+ t
+(1

Re: [HACKERS] Patch to allow domains over composite types

2012-02-27 Thread Yeb Havinga

On 2012-02-27 12:49, Asif Rehman wrote:

Hi Yeb Havinga,

I was digging archives to see anyone worked on supporting domain's 
over composite type and found your patch, but that was pulled back. 
According to commitfest comments it needs some more work...


There were some issues with using the domains from pl/pgsql, which could 
probably made to work, but I didn't investigate it because at the time 
the use case for which is was needed was solved in a different way.


Are you going to submit the updated patch?


There is no updated patch, sorry.

regards,
Yeb


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


Re: [HACKERS] [v9.2] Add GUC sepgsql.client_label

2012-02-24 Thread Yeb Havinga

On 2012-02-23 12:17, Kohei KaiGai wrote:

2012/2/20 Yeb Havingayebhavi...@gmail.com:

So maybe this is because my start domain is not s0-s0:c0.c1023

However, when trying to run bash or psql in domain
unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 I get permission
denied.

Distribution is FC15, sestatus
SELinux status: enabled
SELinuxfs mount:/selinux
Current mode:   enforcing
Mode from config file:  enforcing
Policy version: 24
Policy from config file:targeted


The default security policy does not permit dynamic domain transition
even if unconfined domain, in contradiction to its name.
(IMO, it is fair enough design to avoid single point of failure like root user.)

The security policy of regression test contains a set of rules to reduce
categories assigned to unconfined domain.
So, could you try the following steps.
1. Build the latest policy
 % make -f /usr/share/selinux/devel/Makefile -C contrib/sepgsql
2. Install the policy module
 % sudo semodule -i contrib/sepgsql/sepgsql-regtest.pp
3. Turn on the sepgsql_regression_test_mode
 % sudo setsebool -P sepgsql_regression_test_mode=1

I believe it allows to switch security label of the client, as long as we try to
reduce categories.


I remember these commands from the sepgsql contrib module documentation 
(though the semodule invocation in the documentation is with -u and the 
setsebool does not have the -P flag). semodule -l showed I had already 
installed version 1.04.


I just repeated all steps with the new patch, and get the same result:

LOG:  SELinux: denied { dyntransition } 
scontext=unconfined_u:unconfined_r:unconfined_t:s0 
tcontext=unconfined_u:unconfined_r:unconfined_t:s0:c0.c15 tclass=process
STATEMENT:  SELECT 
sepgsql_setcon('unconfined_u:unconfined_r:unconfined_t:s0:c0.c15');


[mgrid@mgfedora sepgsql]$ getsebool sepgsql_regression_test_mode
sepgsql_regression_test_mode -- on
[root@mgfedora sepgsql]# semodule -l | egrep 'pgsql|postgres'
postgresql  1.12.1
sepgsql-regtest 1.04

Do I need Fedora 16 to run it?


--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] [v9.2] Add GUC sepgsql.client_label

2012-02-24 Thread Yeb Havinga

On 2012-02-24 14:20, Kohei KaiGai wrote:


It seems to me you try to expand categories of the client.
The log saids sepgsql_setcon() tries to switch to ...:s0:c0.c15 from ...:s0.
It is not an admitted operations because of increasion of categories.


Yes I had my eye on the missing c0.c1023 before but couldn't remember 
changing it, so wrongfully assumed that it would be semantically 
equivalent to c0.c1023.

LOG:  SELinux: denied { dyntransition }
scontext=unconfined_u:unconfined_r:unconfined_t:s0
tcontext=unconfined_u:unconfined_r:unconfined_t:s0:c0.c15 tclass=process

May I see your /etc/selinux/targeted/seusers ?

I think __default__ entry is configured to unconfined_u:s0, instead of
unconfined_u:s0:c0.c1023 as default.

In my environment, it is configured as follows:

   [root@iwashi ~]# cat /etc/selinux/targeted/seusers
   # This file is auto-generated by libsemanage
   # Do not edit directly.

   system_u:system_u:s0-s0:c0.c1023
   root:unconfined_u:s0-s0:c0.c1023
   __default__:unconfined_u:s0-s0:c0.c1023=== (*)



[mgrid@mgfedora ~]$ cat /etc/selinux/targeted/seusers
# This file is auto-generated by libsemanage
# Do not edit directly.

system_u:system_u:s0-s0:c0.c1023
root:unconfined_u:s0-s0:c0.c1023
__default__:unconfined_u:s0-s0:c0.c1023

but still

[mgrid@mgfedora ~]$ id -Z
system_u:unconfined_r:unconfined_t:s0
(I changed bash to run in the unconfined_u context before starting the 
regression test)


and

[root@mgfedora targeted]# id -Z
system_u:unconfined_r:unconfined_t:s0

When I created a new test user, it's selinux context showed the c0.c1023 
- I don't know what's fishy about the mgrid user and root that causes 
c0.c1023 to be absent. Maybe I should reinstall this virtual machine. 
After setting the user mgrid on s0-s0:c0.c1023 with


semanage login -a -S targeted -s unconfined_u -r s0-s0:c0.c1023 mgrid

the regression tests pass :-)

test label... ok
test dml  ... ok
test create   ... ok
test misc ... ok

I'll continue reviewing the patch.

--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] [v9.2] Add GUC sepgsql.client_label

2012-02-24 Thread Yeb Havinga

On 2012-02-24 15:17, Yeb Havinga wrote:
I don't know what's fishy about the mgrid user and root that causes 
c0.c1023 to be absent. 


more info:

In shells started in a x environment under Xvnc, id -Z shows the 
system_u and c0.c1023 absent.


In shells started from the ssh daemon, the id -Z matches what it should 
be according to the seusers file: unconfined_u and c0.c1023 present.


-- Yeb



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


Re: [HACKERS] [v9.2] Add GUC sepgsql.client_label

2012-02-24 Thread Yeb Havinga

On 2012-02-23 12:17, Kohei KaiGai wrote:

2012/2/20 Yeb Havingayebhavi...@gmail.com:

On 2012-02-05 10:09, Kohei KaiGai wrote:

The attached part-1 patch moves related routines from hooks.c to label.c
because of references to static variables. The part-2 patch implements above
mechanism.


I took a short look at this patch but am stuck getting the regression test
to run properly.

First, patch 2 misses the file sepgsql.sql.in and therefore the creation
function command for sepgsql_setcon is missing.


Thanks for your comments.

I added the definition of sepgsql_setcon function to sepgsql.sql.in file,
in addition to patch rebasing.


Very brief comments due to must leave keyboard soon:

I read the source code and played a bit with setcon and the debugger, no 
strange things found.


Code comments / questions:

this comment below is a lie, because setcon is set by 
sepgsql_xact_callback()

maybe client_label_committed is a better name for client_label_setcon?

static char *client_label_setcon= NULL;/* set by sepgsql_setcon() */

Is the double negation in the sentence below intended?

+ * Neither of them has no special state, the security label being 
initialized

+ * at database-logon time shall be returned.

Is the assert client_label_peer != NULL in sepgsql_get_client_label 
necessary?


sepgsql_set_client_label(), maybe add a comment to !new_label that it is 
reset to the peer label.


new_label == NULL / pending_label-label == NULL means use the peer 
label. Why not use the peer label instead?


set_label: if new_label == current label according to getcon, is it 
necessary to add to the pending list?


sepgsql_subxact_callback(), could this be made easier to read by just 
taking llast(client_label_pending), assert that plabel-subid == mySubId 
and then list_delete on pointer of that listcell?


Some comments contain typos, I can spend some time on this, though I'm 
not a native english speaker so it won't be perfect.


regards,
Yeb Havinga

--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] leakproof

2012-02-20 Thread Yeb Havinga

On 2012-02-20 06:37, Don Baccus wrote:

On Feb 19, 2012, at 7:24 PM, Tom Lane wrote:


It's not clear to me whether pure/leakproof functions are meant to be a
strict subset of immutable functions

Superset, not subset, unless my guessing is wrong.  How could pure be a subset of 
immutable?
If immutable functions are not necessarily leakproof/pure, and all 
leakproof/pure functions are immutable.


If the latter is not the case, pure leads to confusion as well.

What about discreet?

-- Yeb


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


Re: [HACKERS] [v9.2] Add GUC sepgsql.client_label

2012-02-20 Thread Yeb Havinga

On 2012-02-05 10:09, Kohei KaiGai wrote:
The attached part-1 patch moves related routines from hooks.c to 
label.c because of references to static variables. The part-2 patch 
implements above mechanism. 


I took a short look at this patch but am stuck getting the regression 
test to run properly.


First, patch 2 misses the file sepgsql.sql.in and therefore the creation 
function command for sepgsql_setcon is missing.


When that was solved, ./test_psql failed on the message that the psql 
file may not be of object type unconfined_t. Once it was changed to 
bin_t, the result output for the domain transition gives differences on 
this output (the other parts of label.sql were ok) :


--
-- Test for Dynamic Domain Transition
--
-- validation of transaction aware dynamic-transition
/usr/bin/runcon: /usr/local/pgsql/bin/psql: Permission denied
/usr/bin/runcon: /usr/local/pgsql/bin/psql: Permission denied
/usr/bin/runcon: /usr/local/pgsql/bin/psql: Permission denied

However when I connect to the regression database directly, I can 
execute the first setcon command but get
regression=# SELECT 
sepgsql_setcon('unconfined_u:unconfined_r:unconfined_t:s0:c0.c12');

ERROR:  SELinux: security policy violation

logfile shows
LOG:  SELinux: denied { dyntransition } 
scontext=unconfined_u:unconfined_r:unconfined_t:s0 
tcontext=unconfined_u:unconfined_r:unconfined_t:s0:c0.c12 tclass=process


So maybe this is because my start domain is not s0-s0:c0.c1023

However, when trying to run bash or psql in domain 
unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 I get permission 
denied.


Distribution is FC15, sestatus
SELinux status: enabled
SELinuxfs mount:/selinux
Current mode:   enforcing
Mode from config file:  enforcing
Policy version: 24
Policy from config file:targeted

--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] [v9.2] Add GUC sepgsql.client_label

2012-01-31 Thread Yeb Havinga

On 2012-01-30 19:19, Robert Haas wrote:

On Sun, Jan 29, 2012 at 7:27 AM, Kohei KaiGaikai...@kaigai.gr.jp  wrote:

However, I also assume one other possible use-case; the originator
has privileges to translate 10 of different domains, but disallows to
revert it. In this case, RESET without permission checks are harmful.
(This scenario will be suitable with multi-category-model.)
Of course, we already have a trusted procedure mechanism which is 
intended to support temporary changes to the effective security label, 
so you might say, hey, people shouldn't do that. But they might. And I 
wouldn't like to bet that's the only case that could be problematic 
either. What about a setting in postgresql.conf? You could end up 
being asked to set the security label to some other value before 
you've initialized it. What about SET LOCAL? It's not OK to fail to 
revert that at transaction exit. Hence my suggestion of a function: if 
you use functions, you can implement whatever semantics you want. 


What about always allowing a transition to the default / postgresql.conf 
configured client label, so in the case of errors, or RESET, the 
transition to this domain is hardcoded to succeed. This would also be 
useful in conjunction with a connection pooler. This would still allow 
the option to prevent a back transition to non-default client labels.


--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] [v9.2] Add GUC sepgsql.client_label

2012-01-31 Thread Yeb Havinga

On 2012-01-31 14:06, Robert Haas wrote:

On Tue, Jan 31, 2012 at 4:36 AM, Yeb Havingayebhavi...@gmail.com  wrote:

What about always allowing a transition to the default / postgresql.conf
configured client label, so in the case of errors, or RESET, the transition
to this domain is hardcoded to succeed. This would also be useful in
conjunction with a connection pooler. This would still allow the option to
prevent a back transition to non-default client labels.

I don't think you can make that work, because someone can still
attempt to RESET to a different value, and it's still not safe to make
that fail.


But the idea is that if that different value is a (possibly changed into 
a new) allowed default value, a RESET to that new default value will be 
allowed, by definition, because it is a default value.



--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] [v9.2] Add GUC sepgsql.client_label

2012-01-31 Thread Yeb Havinga

On 2012-01-31 15:28, Robert Haas wrote:


*scratches head*

I'm not sure I follow you.  If you're saying that we can make this
work by always allowing the value to be reset, then I agree with you,
but I'm not sure those are the semantics KaiGai wants.  For instance,
if a connection pooler does:

SET sepgsql.client_label = 'bob_t';

...and then hands off to the client, the client can then do:

RESET sepgsql.client_label;
SET sepgsql.client_label = 'alice_t';

and that's bad.


Hmm yes this is a problem. Reading the original post better, it is also 
not the intended behaviour to support repeatable client_label switches.


However, single-directed domain transition from bigger-privileges to 
smaller-privileged domain by users' operation is also supported on 
operating system, and useful feature to restrict applications capability 
at beginning of the session.


--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] Multithread Query Planner

2012-01-23 Thread Yeb Havinga

On 2012-01-13 21:14, Frederico wrote:

Hi folks.

Is there any restriction in create and start threads inside Postgres?

I'm trying to develop a multithread planner, and some times is raised a 
exception of access memory.

I'm debugging the code to see if is a bug in the planner, but until now, I 
still not found. I tried to use the same memory context of root process and 
create a new context to each new thread, but doesn't worked.


Any tips?


Not sure if it is of any use to you, but the vldb paper 'Parallelizing 
Query Optimization' http://www.vldb.org/pvldb describes a experimental 
implementation in PostgreSQL.


regards,
Yeb


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


Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters

2011-12-14 Thread Yeb Havinga

On 2011-12-13 18:34, Tom Lane wrote:

Heikki Linnakangasheikki.linnakan...@enterprisedb.com  writes:

Attached is a patch with those changes. I also I removed a few of the
syntax error regression tests, that seemed excessive, plus some general
naming and comment fiddling. I'll apply this tomorrow, if it still looks
good to me after sleeping on it.

However, I'm still concerned about whether this approach gives
reasonable error messages in cases where the error would be
detected during parse analysis of the rearranged statement.
The regression test examples don't cover such cases, and I'm
too busy right now to apply the patch and check for myself.
What happens for example if a named parameter's value contains
a misspelled variable reference, or a type conflict?


I tested this and seems to be ok:

regression=# select namedparmcursor_test1(2, 2) as Should be 
false,

   namedparmcursor_test1(20, 20) as Should be true;
ERROR:  column yy does not exist
LINE 1: SELECT x AS param1, yy AS param12;

regression=# select namedparmcursor_test1(2, 2) as Should be 
false,

   namedparmcursor_test1(20, 20) as Should be true;
ERROR:  invalid input syntax for integer: 2011-11-29 19:26:10.029084
CONTEXT:  PL/pgSQL function namedparmcursor_test1 line 8 at OPEN

regards,
Yeb Havinga

last error was created with

create or replace function namedparmcursor_test1(int, int) returns 
boolean as $$

declare
c1 cursor (param1 int, param12 int) for select * from rc_test where 
a  param1 and b  param12;

y int := 10;
x timestamp := now();
nonsense record;
begin
open c1(param12 := $1, param1 := x);
end
$$ language plpgsql;


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


Re: [HACKERS] patch for type privileges

2011-12-13 Thread Yeb Havinga

On 2011-12-12 20:53, Peter Eisentraut wrote:

On sön, 2011-12-11 at 21:21 +0200, Peter Eisentraut wrote:

* Cannot restrict access to array types. After revoking usage from the
element type, the error is perhaps a bit misleading. (smallint[] vs
smallint)

postgres=  create table a (a int2[]);
ERROR:  permission denied for type smallint[]

OK, that error message should be improved.

Fixing this is easy, but I'd like to look into refactoring this a bit.
Let's ignore that for now; it's easy to do later.


My experience with ignoring things for now is not positive.

* The information schema view 'attributes' has this additional condition:
AND (pg_has_role(t.typowner, 'USAGE')
 OR has_type_privilege(t.oid, 'USAGE'));

What happens is that attributes in a composite type are shown, or not,
if the current user has USAGE rights. The strange thing here, is that
the attribute in the type being show or not, doesn't match being able to
use it (in the creation of e.g. a table).

Yeah, that's a bug.  That should be something like

AND (pg_has_role(c.relowner, 'USAGE')
  OR has_type_privilege(c.reltype, 'USAGE'));

And fix for that included.


Confirmed that this now works as expected.

I have no remarks on the other parts of the patch code.

After puzzling a bit more with the udt and usage privileges views, it is 
clear that they should complement each other. That might be reflected by 
adding to the 'usage_privileges' section a link back to the 
'udt_privileges' section.


I have no further comments on this patch.

regards,
Yeb Havinga



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


Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters

2011-12-12 Thread Yeb Havinga

On 2011-12-11 16:26, Yeb Havinga wrote:

On 2011-12-06 17:58, Kevin Grittner wrote:

Kevin Grittnerkgri...@wicourts.gov  wrote:

Yeb Havingayebhavi...@gmail.com  wrote:



I personally tend to believe it doesn't even need to be an error.
There is no technical reason not to allow it. All the user needs
to do is make sure that the combination of named parameters and
the positional ones together are complete and not overlapping.



If there are no objections, I suggest that Yeb implement the mixed
notation for cursor parameters.


Hearing no objections -- Yeb, are you OK with doing this, and do you
feel this is doable for this CF?


Attach is v6 of the patch, allowing mixed mode and with new test cases 
in the regression tests. One difference with calling functions 
remains: it is allowed to place positional arguments after named 
parameters. Including that would add code, but nothing would be gained.


Forgot to copy regression output to expected - attached v7 fixes that.

-- Yeb

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
new file mode 100644
index f33cef5..ee2e3a3
*** a/doc/src/sgml/plpgsql.sgml
--- b/doc/src/sgml/plpgsql.sgml
*** OPEN curs1 FOR EXECUTE 'SELECT * FROM '
*** 2823,2833 
 /para
   /sect3
  
! sect3
   titleOpening a Bound Cursor/title
  
  synopsis
! OPEN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional;
  /synopsis
  
   para
--- 2823,2833 
 /para
   /sect3
  
! sect3 id=plpgsql-open-bound-cursor
   titleOpening a Bound Cursor/title
  
  synopsis
!  OPEN replaceablebound_cursorvar/replaceable optional ( optional replaceableargname/replaceable := /optional replaceableargument_value/replaceable optional, .../optional ) /optional;
  /synopsis
  
   para
*** OPEN replaceablebound_cursorvar/repla
*** 2847,2856 
--- 2847,2867 
   /para
  
   para
+   Cursors may be opened using either firsttermpositional/firstterm
+   or firsttermnamed/firstterm notation.  Similar to calling
+   functions, described in xref linkend=sql-syntax-calling-funcs, it
+   is also allowed to mix positional and named notation.  In positional
+   notation, all arguments are specified in order.  In named notation,
+   each argument's name is specified using literal:=/literal to
+   separate it from the argument expression.
+  /para
+ 
+  para
Examples (these use the cursor declaration examples above):
  programlisting
  OPEN curs2;
  OPEN curs3(42);
+ OPEN curs3(key := 42);
  /programlisting
   /para
  
*** COMMIT;
*** 3169,3186 
  
  synopsis
  optional lt;lt;replaceablelabel/replaceablegt;gt; /optional
! FOR replaceablerecordvar/replaceable IN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional LOOP
  replaceablestatements/replaceable
  END LOOP optional replaceablelabel/replaceable /optional;
  /synopsis
  
   The cursor variable must have been bound to some query when it was
!  declared, and it emphasiscannot/ be open already.  The
!  commandFOR/ statement automatically opens the cursor, and it closes
!  the cursor again when the loop exits.  A list of actual argument value
!  expressions must appear if and only if the cursor was declared to take
!  arguments.  These values will be substituted in the query, in just
!  the same way as during an commandOPEN/.
   The variable replaceablerecordvar/replaceable is automatically
   defined as type typerecord/ and exists only inside the loop (any
   existing definition of the variable name is ignored within the loop).
--- 3180,3203 
  
  synopsis
  optional lt;lt;replaceablelabel/replaceablegt;gt; /optional
! FOR replaceablerecordvar/replaceable IN replaceablebound_cursorvar/replaceable optional ( optional replaceableargname/replaceable := /optional replaceableargument_value/replaceable optional, .../optional ) /optional LOOP
  replaceablestatements/replaceable
  END LOOP optional replaceablelabel/replaceable /optional;
  /synopsis
  
   The cursor variable must have been bound to some query when it was
!  declared, and it emphasiscannot/ be open already.  The commandFOR/
!  statement automatically opens the cursor, and it closes the cursor again
!  when the loop exits.  The cursor's arguments may be supplied in either
!  firsttermpositional/firstterm or firsttermnamed/firstterm
!  notation.  A list of actual argument value expressions must appear if and
!  only if the cursor was declared to take arguments.  These values will be
!  substituted in the query, in just the same way as during an
!  commandOPEN/command described in xref
!  linkend=plpgsql-open-bound-cursor.
!/para
! 
!para
   The variable replaceablerecordvar/replaceable

Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters

2011-12-11 Thread Yeb Havinga

On 2011-12-06 17:58, Kevin Grittner wrote:

Kevin Grittnerkgri...@wicourts.gov  wrote:

Yeb Havingayebhavi...@gmail.com  wrote:



I personally tend to believe it doesn't even need to be an error.
There is no technical reason not to allow it. All the user needs
to do is make sure that the combination of named parameters and
the positional ones together are complete and not overlapping.



If there are no objections, I suggest that Yeb implement the mixed
notation for cursor parameters.


Hearing no objections -- Yeb, are you OK with doing this, and do you
feel this is doable for this CF?


Attach is v6 of the patch, allowing mixed mode and with new test cases 
in the regression tests. One difference with calling functions remains: 
it is allowed to place positional arguments after named parameters. 
Including that would add code, but nothing would be gained.


regards,
Yeb Havinga


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


Re: [HACKERS] patch for type privileges

2011-12-10 Thread Yeb Havinga

On 2011-12-07 19:59, Peter Eisentraut wrote:
Two excellent finds. Here is an updated patch with fixes. 


Thanks.. I'm sorry I cannot yet provide a complete review, but since the 
end of the commitfest is near, I decided to mail them anyway instead of 
everything on dec 15.


* ExecGrant_type() prevents 'grant usage on domain' on a type, but the 
converse is possible.


postgres=# create domain myint as int2;
CREATE DOMAIN
postgres=# grant usage on type myint to public;
GRANT

* Cannot restrict access to array types. After revoking usage from the 
element type, the error is perhaps a bit misleading. (smallint[] vs 
smallint)


postgres= create table a (a int2[]);
ERROR:  permission denied for type smallint[]

* The patch adds the following text explaining the USAGE privilege on types.

  For types and domains, this privilege allow the use of the type or
  domain in the definition of tables, functions, and other schema objects.

Since other paragraphs in USAGE use the word 'creation' instead of 
'definition', I believe here the word 'creation' should be used too.  
IMHO it would also be good to describe what the USAGE privilege is not, 
but might be expected since it is such a generic term. USAGE on type: 
use of the type while creating new dependencies to the type, not usage 
in the sense of instantiating values of the type. If there are existing 
dependencies, revoking usage privileges will not return any warning and 
the dependencies still exist. Also other kinds of exceptions could be 
noted, such as the exception for array types and casts. The example you 
gave in the top mail about why restricting access to types can be 
useful, such as preventing that owners are prevented changing their 
types because others have 'blocked' them by their usage, is something 
that could also help readers of the documentation understand why 
privileges on types are useful for them (or not).


* The information schema view 'attributes' has this additional condition:
  AND (pg_has_role(t.typowner, 'USAGE')
   OR has_type_privilege(t.oid, 'USAGE'));

What happens is that attributes in a composite type are shown, or not, 
if the current user has USAGE rights. The strange thing here, is that 
the attribute in the type being show or not, doesn't match being able to 
use it (in the creation of e.g. a table). Maybe that is not intended, 
but I would expect it matching:


postgres=# create user c;
CREATE ROLE
postgres=# create type t as (a int2);
CREATE TYPE
postgres=# \c - c
You are now connected to database postgres as user c.
postgres= select udt_name,attribute_name from 
information_schema.attributes;

 udt_name | attribute_name
--+
 t| a
(1 row)

postgres= \c -
You are now connected to database postgres as user c.
postgres= \c - postgres
You are now connected to database postgres as user postgres.
postgres=# revoke usage on type int2 from public;
REVOKE
postgres=# \c - c
You are now connected to database postgres as user c.
postgres= select udt_name,attribute_name from 
information_schema.attributes;

 udt_name | attribute_name
--+
(0 rows)

postgres= create table m (a t);
CREATE TABLE
postgres= insert into m values (ROW(10));
INSERT 0 1
postgres=

Conversely:

postgres=# grant usage on type int2 to public;
GRANT
postgres=# revoke usage on type t from public;
REVOKE
postgres=# \c - c
You are now connected to database postgres as user c.
postgres= select udt_name,attribute_name from 
information_schema.attributes;

 udt_name | attribute_name
--+
 t| a
(1 row)

postgres= create table m2 (a t);
ERROR:  permission denied for type t
postgres=


regards,
Yeb Havinga


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


Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters

2011-12-07 Thread Yeb Havinga

On 2011-12-06 17:58, Kevin Grittner wrote:

Kevin Grittnerkgri...@wicourts.gov  wrote:

If there are no objections, I suggest that Yeb implement the mixed
notation for cursor parameters.


Hearing no objections -- Yeb, are you OK with doing this, and do you
feel this is doable for this CF?


It is not a large change, I'll be able to do it tomorrow if nothing 
unexpected happens, or monday otherwise.


regards,
Yeb


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


Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters

2011-12-05 Thread Yeb Havinga

Kevin,

Thank you for your review!

On 2011-12-03 21:53, Kevin Grittner wrote:


(1)  Doc changes:

   (a) These contain some unnecessary whitespace changes which make it
   harder to see exactly what changed.


This is probably caused because I used emacs's fill-paragraph (alt+q) 
command, after some changes. If this is against policy, I could change 
the additions in such a way as to cause minor differences, however I 
believe that the benefit of that ends immediately after review.



   (b) There's one point where cursors should be possessive
   cursor's.

   (c) I think it would be less confusing to be consistent about
   mentioning positional and named in the same order each
   time. Mixing it up like that is harder to read, at least for
   me.


Good point, both changed.


(2)  The error for mixing positional and named parameters should be
moved up.  That seems more important than too many arguments or
provided multiple times if both are true.


I moved the error up, though I personally tend to believe it doesn't 
even need to be an error. There is no technical reason not to allow it. 
All the user needs to do is make sure that the combination of named 
parameters and the positional ones together are complete and not 
overlapping. This is also in line with calling functions, where mixing 
named and positional is allowed, as long as the positional arguments are 
first. Personally I have no opinion what is best, include the feature or 
give an error, and I removed the possibility during the previous commitfest.



(3)  The -- END ADDITIONAL TESTS comment shouldn't be added to the
regression tests.


This is removed, also the -- START ADDITIONAL TESTS, though I kept the 
tests between those to comments.



The way this parses out the named parameters and then builds the
positional list, with some code from read_sql_construct() repeated in
read_cursor_args() seems a little bit clumsy to me, but I couldn't
think of a better way to do it.


Same here.

The new patch is attached.

regards
Yeb Havinga

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
new file mode 100644
index f33cef5..a3e6540
*** a/doc/src/sgml/plpgsql.sgml
--- b/doc/src/sgml/plpgsql.sgml
*** OPEN curs1 FOR EXECUTE 'SELECT * FROM '
*** 2823,2833 
 /para
   /sect3
  
! sect3
   titleOpening a Bound Cursor/title
  
  synopsis
! OPEN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional;
  /synopsis
  
   para
--- 2823,2833 
 /para
   /sect3
  
! sect3 id=plpgsql-open-bound-cursor
   titleOpening a Bound Cursor/title
  
  synopsis
!  OPEN replaceablebound_cursorvar/replaceable optional ( optional replaceableargname/replaceable := /optional replaceableargument_value/replaceable optional, .../optional ) /optional;
  /synopsis
  
   para
*** OPEN replaceablebound_cursorvar/repla
*** 2847,2856 
--- 2847,2867 
   /para
  
   para
+   Cursors may be opened using either firsttermpositional/firstterm
+   or firsttermnamed/firstterm notation. In contrast with calling
+   functions, described in xref linkend=sql-syntax-calling-funcs, it
+   is not allowed to mix positional and named notation. In positional
+   notation, all arguments are specified in order. In named notation,
+   each argument's name is specified using literal:=/literal to
+   separate it from the argument expression.
+  /para
+ 
+  para
Examples (these use the cursor declaration examples above):
  programlisting
  OPEN curs2;
  OPEN curs3(42);
+ OPEN curs3(key := 42);
  /programlisting
   /para
  
*** COMMIT;
*** 3169,3186 
  
  synopsis
  optional lt;lt;replaceablelabel/replaceablegt;gt; /optional
! FOR replaceablerecordvar/replaceable IN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional LOOP
  replaceablestatements/replaceable
  END LOOP optional replaceablelabel/replaceable /optional;
  /synopsis
  
   The cursor variable must have been bound to some query when it was
!  declared, and it emphasiscannot/ be open already.  The
!  commandFOR/ statement automatically opens the cursor, and it closes
!  the cursor again when the loop exits.  A list of actual argument value
!  expressions must appear if and only if the cursor was declared to take
!  arguments.  These values will be substituted in the query, in just
!  the same way as during an commandOPEN/.
   The variable replaceablerecordvar/replaceable is automatically
   defined as type typerecord/ and exists only inside the loop (any
   existing definition of the variable name is ignored within the loop).
--- 3180,3203 
  
  synopsis
  optional lt;lt;replaceablelabel/replaceablegt;gt; /optional
! FOR replaceablerecordvar/replaceable

Re: [HACKERS] patch for type privileges

2011-12-02 Thread Yeb Havinga

On 2011-12-01 22:14, Peter Eisentraut wrote:

On tor, 2011-12-01 at 14:37 +0100, Yeb Havinga wrote:

On 2011-11-29 18:47, Peter Eisentraut wrote:

On tis, 2011-11-29 at 07:07 +0200, Peter Eisentraut wrote:

On mån, 2011-11-28 at 11:41 +0100, Yeb Havinga wrote:

On 2011-11-15 21:50, Peter Eisentraut wrote:

Patch attached.

I cannot get the patch to apply, this is the output of patch -p1
--dry-run on HEAD.

I need to remerge it against concurrent range type activity.

New patch attached.

I'm looking at your patch. One thing that puzzled me for a while was
that I could not restrict access to base types (either built-in or user
defined). Is this intentional?

Works for me:

=# create user foo;
=# revoke usage on type int8 from public;
=# \c - foo
=  create table test1 (a int4, b int8);
ERROR:  permission denied for type bigint


Hmm even though I have 'revoke all on type int2 from public' in my psql 
history, I cannot repeat what I think was happening yesterday. Probably 
I was still superuser in the window I was testing with, but will never 
no until time travel is invented. Or maybe I tested with a cast.


Using a cast, it is possible to create a table with a code path through 
OpenIntoRel:


session 1:
t=# revoke all on type int2 from public;
session2 :
t= create table t2 (a int2);
ERROR:  permission denied for type smallint
t= create table t as (select 1::int2 as a);
SELECT 1
t= \d t
   Table public.t
 Column |   Type   | Modifiers
+--+---
 a  | smallint |

t=

Something different: as non superuser I get this error when restricting 
a type I don't own:


t= revoke all on type int2 from public;
ERROR:  unrecognized objkind: 6

My current time is limited but I will be able to look more at the patch 
in a few more days.


regards,
Yeb Havinga


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


Re: [HACKERS] patch for type privileges

2011-12-01 Thread Yeb Havinga

On 2011-11-29 18:47, Peter Eisentraut wrote:

On tis, 2011-11-29 at 07:07 +0200, Peter Eisentraut wrote:

On mån, 2011-11-28 at 11:41 +0100, Yeb Havinga wrote:

On 2011-11-15 21:50, Peter Eisentraut wrote:

Patch attached.

I cannot get the patch to apply, this is the output of patch -p1
--dry-run on HEAD.

I need to remerge it against concurrent range type activity.

New patch attached.


I'm looking at your patch. One thing that puzzled me for a while was 
that I could not restrict access to base types (either built-in or user 
defined). Is this intentional?


regards,
Yeb Havinga


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


Re: [HACKERS] patch for type privileges

2011-11-28 Thread Yeb Havinga

On 2011-11-15 21:50, Peter Eisentraut wrote:

Patch attached.


I cannot get the patch to apply, this is the output of patch -p1 
--dry-run on HEAD.


patching file src/include/catalog/pg_type.h
Hunk #1 succeeded at 217 (offset 1 line).
Hunk #2 succeeded at 234 (offset 1 line).
Hunk #3 succeeded at 264 (offset 1 line).
Hunk #4 succeeded at 281 (offset 1 line).
Hunk #5 FAILED at 370.
Hunk #6 FAILED at 631.
2 out of 6 hunks FAILED -- saving rejects to file 
src/include/catalog/pg_type.h.rej


I was unable to find a rev to apply the patch to do some testing: this 
one didn't work either


commit 4429f6a9e3e12bb4af6e3677fbc78cd80f160252
Author: Heikki Linnakangas heikki.linnakan...@iki.fi
Date:   Thu Nov 3 13:16:28 2011 +0200
Support range data types.

and that's strange since git log of pg_type.h shows a commit of april 
before that.


regards,
Yeb Havinga


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


Re: [HACKERS] [PATCH] Unremovable tuple monitoring

2011-11-16 Thread Yeb Havinga

On 2011-11-15 22:04, Tom Lane wrote:

Robert Haasrobertmh...@gmail.com  writes:

Oh.  I was thinking dead meant no longer visible to anyone.   But
it sounds what we call unremovable here is what we elsewhere call
recently dead.

Would have to look at the code to be sure, but I think that
nonremovable is meant to count both live tuples and
dead-but-still-visible-to-somebody tuples.

The question that I think needs to be asked is why it would be useful
to track this using the pgstats mechanisms.  By definition, the
difference between this and the live-tuple count is going to be
extremely unstable --- I don't say small, necessarily, but short-lived.
So it's debatable whether it's worth memorializing the count obtained
by the last VACUUM at all.  And doing it through pgstats is an expensive
thing.  We've already had push-back about the size of the stats table
on large (lots-o-tables) databases.  Adding another counter will impose
a performance overhead on everybody, whether they care about this number
or not.

What's more, to the extent that I can think of use-cases for knowing
this number, I think I would want a historical trace of it --- that is,
not only the last VACUUM's result but those of previous VACUUM cycles.
So pgstats seems like it's both expensive and useless for the purpose.



Before reviewing this patch I didn't even know these kind of dead rows 
could exist. Now I know it, I expect that if I wanted to know the 
current number, I would start looking at table statistics: pg_stat* or 
perhaps contrib/pgstattuple.


Looking at how that looks with transaction a the old version:

t=# begin TRANSACTION ISOLATION LEVEL repeatable read;
BEGIN
t=# select * from t;
 i  | b
+---
 10 | 2
(1 row)

in transaction b the new version:
t=# select * from t;
 i  | b
+---
 10 | 4
(1 row)

after a vacuum of t:

stat_user_table counts:
n_tup_ins | 1
n_tup_upd | 6
n_tup_del | 0
n_tup_hot_upd | 6
n_live_tup| 2
n_dead_tup| 0
n_unremovable_tup | 1

t=# select * from pgstattuple('t');
-[ RECORD 1 ]--+--
table_len  | 8192
tuple_count| 1
tuple_len  | 32
tuple_percent  | 0.39
dead_tuple_count   | 1
dead_tuple_len | 32
dead_tuple_percent | 0.39
free_space | 8080
free_percent   | 98.63

Apparently pg_stat* counts the recently_dead tuple under n_live_tup, 
else 2 is a wrong number, where pgstattuple counts recently_dead under 
dead_tuple_count. This could be a source of confusion. If there is any 
serious work considered here, IMHO at least the numbers of the two 
different sources of tuple counters should match in terminology and 
actual values. Maybe also if pgstattuple were to include the distinction 
unremovable dead tuples vs dead tuples, a log line by vacuum 
encountering unremovable dead tuples could refer to pgstattuple for 
statistics.


regards,
Yeb Havinga


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


Re: [HACKERS] [PATCH] Unremovable tuple monitoring

2011-11-16 Thread Yeb Havinga

On 2011-11-16 15:34, Tom Lane wrote:

Robert Haasrobertmh...@gmail.com  writes:

Not sure about the log line, but allowing pgstattuple to distinguish
between recently-dead and quite-thoroughly-dead seems useful.

The dividing line is enormously unstable though.  pgstattuple's idea of
RecentGlobalXmin could even be significantly different from that of a
concurrently running VACUUM.  I can see the point of having VACUUM log
what it did, but opinions from the peanut gallery aren't worth much.


I don't understand your the last remark so I want to get it clear: I 
looked up peanut gallery on the wiki. Is 'opinion from the peanut 
gallery' meant to describe my comments as patch reviewer? I'd appreciate 
brutal honesty on this point.


thanks
Yeb


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


Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters

2011-11-15 Thread Yeb Havinga

On 2011-11-14 15:45, Yeb Havinga wrote:

On 2011-10-15 07:41, Tom Lane wrote:

Yeb Havingayebhavi...@gmail.com  writes:

Hello Royce,
Thanks again for testing.

I looked this patch over but concluded that it's not ready to apply,
mainly because there are too many weird behaviors around error
reporting.


Thanks again for the review and comments. Attached is v3 of the patch 
that addresses all of the points made by Tom. In the regression test I 
added a section under --- START ADDITIONAL TESTS that might speedup 
testing.


Please disregard the previous patch: besides that it contained an unused 
function, it turned out my statement that all of Tom's points were 
addressed was not true - the attached patch fixes the remaining issue of 
putting two kinds of errors at the correct start of the current argument 
location.


I also put some more comments in the regression test section: mainly to 
assist providing testcases for review, not for permanent inclusion.


To address a corner case of the form 'p1 := 1 -- comments\n, p2 := 2' it 
was necessary to have read_sql_construct not trim trailing whitespace, 
since that results in an expression of the form '1 -- comments, 2' which 
is wrong.


regards,
Yeb Havinga

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
new file mode 100644
index f33cef5..17c04d2
*** a/doc/src/sgml/plpgsql.sgml
--- b/doc/src/sgml/plpgsql.sgml
*** OPEN curs1 FOR EXECUTE 'SELECT * FROM '
*** 2823,2833 
 /para
   /sect3
  
! sect3
   titleOpening a Bound Cursor/title
  
  synopsis
! OPEN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional;
  /synopsis
  
   para
--- 2823,2833 
 /para
   /sect3
  
! sect3 id=plpgsql-open-bound-cursor
   titleOpening a Bound Cursor/title
  
  synopsis
!  OPEN replaceablebound_cursorvar/replaceable optional ( optional replaceableargname/replaceable := /optional replaceableargument_value/replaceable optional, .../optional ) /optional;
  /synopsis
  
   para
*** OPEN replaceablebound_cursorvar/repla
*** 2847,2856 
--- 2847,2867 
   /para
  
   para
+   Cursors may be opened using either firsttermnamed/firstterm or
+   firsttermpositional/firstterm notation. In contrast with calling
+   functions, described in xref linkend=sql-syntax-calling-funcs, it
+   is not allowed to mix positional and named notation. In positional
+   notation, all arguments are specified in order. In named notation,
+   each argument's name is specified using literal:=/literal to
+   separate it from the argument expression.
+  /para
+ 
+  para
Examples (these use the cursor declaration examples above):
  programlisting
  OPEN curs2;
  OPEN curs3(42);
+ OPEN curs3(key := 42);
  /programlisting
   /para
  
*** COMMIT;
*** 3169,3186 
  
  synopsis
  optional lt;lt;replaceablelabel/replaceablegt;gt; /optional
! FOR replaceablerecordvar/replaceable IN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional LOOP
  replaceablestatements/replaceable
  END LOOP optional replaceablelabel/replaceable /optional;
  /synopsis
  
   The cursor variable must have been bound to some query when it was
!  declared, and it emphasiscannot/ be open already.  The
!  commandFOR/ statement automatically opens the cursor, and it closes
!  the cursor again when the loop exits.  A list of actual argument value
!  expressions must appear if and only if the cursor was declared to take
!  arguments.  These values will be substituted in the query, in just
!  the same way as during an commandOPEN/.
   The variable replaceablerecordvar/replaceable is automatically
   defined as type typerecord/ and exists only inside the loop (any
   existing definition of the variable name is ignored within the loop).
--- 3180,3203 
  
  synopsis
  optional lt;lt;replaceablelabel/replaceablegt;gt; /optional
! FOR replaceablerecordvar/replaceable IN replaceablebound_cursorvar/replaceable optional ( optional replaceableargname/replaceable := /optional replaceableargument_value/replaceable optional, .../optional ) /optional LOOP
  replaceablestatements/replaceable
  END LOOP optional replaceablelabel/replaceable /optional;
  /synopsis
  
   The cursor variable must have been bound to some query when it was
!  declared, and it emphasiscannot/ be open already.  The commandFOR/
!  statement automatically opens the cursor, and it closes the cursor again
!  when the loop exits.  The cursors arguments may be supplied in either
!  firsttermnamed/firstterm or firsttermpositional/firstterm
!  notation.  A list of actual argument value expressions must appear if and
!  only if the cursor was declared to take arguments.  These values

Re: [HACKERS] [PATCH] Unremovable tuple monitoring

2011-11-15 Thread Yeb Havinga

On 2011-10-05 00:45, Royce Ausburn wrote:

Attached is v2 of my patch that doesn't update CATALOG_VERSION_NO.  I've also fixed the name of an 
argument to pgstat_report_vacuum which I don't think was particularly good, and I've replace the 
word tuple with row in some docs I added for consistency.




Hello Royce,

I reviewed your patch. I think it is in good shape, my two main remarks 
(name of n_unremovable_tup and a remark about documentation at the end 
of this review) are highly subjective and I wouldn't spend time on it 
unless other people have the same opinion.


Remarks:

* rules regression test fails because pg_stat_all_tables is changed, 
pg_stat_sys_tables and pg_stat_user_tables as well, but the new 
expected/rules.out is not part of the patch.


* I'm not sure about the name n_unremovable_tup, since it doesn't convey 
it is about dead tuples and judging from only the name it might as well 
include the live tuples. It also doesn't hint that it is a transient 
condition, which vacuum verbose does with the word 'yet'.
What about n_locked_dead_tup? - this contains both information that it 
is about dead tuples, and the lock suggests that once the lock is 
removed, the dead tuple can be removed.


* The number shows correctly in the pg_stat_relation. This is a testcase 
that gives unremovable dead rows:


A:
create table b (a int);
insert into b values (1);

B:
begin transaction ISOLATION LEVEL repeatable read;
select * from b;

A:
update t set b=2 where i=10;
vacuum verbose t;

Then something similar is shown:

INFO:  vacuuming public.t
INFO:  index t_pkey now contains 1 row versions in 2 pages
DETAIL:  0 index row versions were removed.
0 index pages have been deleted, 0 are currently reusable.
CPU 0.00s/0.00u sec elapsed 0.00 sec.
INFO:  t: found 0 removable, 2 nonremovable row versions in 1 out of 1 
pages

DETAIL:  1 dead row versions cannot be removed yet.
There were 0 unused item pointers.
0 pages are entirely empty.
CPU 0.00s/0.01u sec elapsed 0.00 sec.
VACUUM
t=# \x
Expanded display is on.
t=# select * from pg_stat_user_tables;
-[ RECORD 1 ]-+--
relid | 16407
schemaname| public
relname   | t
seq_scan  | 1
seq_tup_read  | 0
idx_scan  | 1
idx_tup_fetch | 1
n_tup_ins | 1
n_tup_upd | 1
n_tup_del | 0
n_tup_hot_upd | 1
n_live_tup| 2
n_dead_tup| 0
n_unremovable_tup | 1
last_vacuum   | 2011-11-05 21:15:06.939928+01
last_autovacuum   |
last_analyze  |
last_autoanalyze  |
vacuum_count  | 1
autovacuum_count  | 0
analyze_count | 0
autoanalyze_count | 0

I did some more tests with larger number of updates which revealed no 
unexpected results.


I was puzzled for a while that n_unremovable_tup = n_dead_tup doesn't 
hold, after all, the unremovable tuples are dead as well. Neither the 
current documentation nor the documentation added by the patch do help 
in explaining the meaning of n_dead_tup and n_unremovable_tup, which may 
be clear to seasoned vacuum hackers, but not to me. In both the case of 
n_dead_tup it would have been nice if the docs mentioned that dead 
tuples are tuples that are deleted or previous versions of updated 
tuples, and that only analyze updates n_dead_tup (since vacuum cleans 
them), in contrast with n_unremovable_tup that gets updated by vacuum. 
Giving an example of how unremovable dead tuples can be caused would 
IMHO also help understanding.


thanks,
Yeb Havinga


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


Re: [HACKERS] [PATCH] Unremovable tuple monitoring

2011-11-15 Thread Yeb Havinga

On 2011-11-15 16:16, Robert Haas wrote:

On Tue, Nov 15, 2011 at 10:05 AM, Yeb Havingayebhavi...@gmail.com  wrote:

I reviewed your patch. I think it is in good shape, my two main remarks
(name of n_unremovable_tup and a remark about documentation at the end of
this review) are highly subjective and I wouldn't spend time on it unless
other people have the same opinion.

I share your opinion; it's not obvious to me what this means either.
I guess this is a dumb question, but why don't we remove all the dead
tuples?

The only case I could think of was that a still running repeatable read 
transaction read them before they were deleted and committed by another 
transaction.


--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] [REVIEW] Patch for cursor calling with named parameters

2011-11-14 Thread Yeb Havinga

On 2011-10-15 07:41, Tom Lane wrote:

Yeb Havingayebhavi...@gmail.com  writes:

Hello Royce,
Thanks again for testing.

I looked this patch over but concluded that it's not ready to apply,
mainly because there are too many weird behaviors around error
reporting.


Thanks again for the review and comments. Attached is v3 of the patch 
that addresses all of the points made by Tom. In the regression test I 
added a section under --- START ADDITIONAL TESTS that might speedup testing.



On the documentation front, the patch includes a hunk that changes the
description of DECLARE to claim that the argument names are optional,
something I see no support for in the code.  It also fails to document
that this patch affects the behavior of cursor FOR loops as well as OPEN,
since both of those places use read_cursor_args().


The declare section was removed. The cursor for loop section was changed 
to include a reference to named parameters, however I was unsure about 
OPEN as I was under the impression that was already altered.


regards,
Yeb Havinga

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
new file mode 100644
index f33cef5..6a77b75
*** a/doc/src/sgml/plpgsql.sgml
--- b/doc/src/sgml/plpgsql.sgml
*** OPEN curs1 FOR EXECUTE 'SELECT * FROM '
*** 2823,2833 
 /para
   /sect3
  
! sect3
   titleOpening a Bound Cursor/title
  
  synopsis
! OPEN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional;
  /synopsis
  
   para
--- 2823,2833 
 /para
   /sect3
  
! sect3 id=plpgsql-open-bound-cursor
   titleOpening a Bound Cursor/title
  
  synopsis
!  OPEN replaceablebound_cursorvar/replaceable optional ( optional replaceableargname/replaceable := /optional replaceableargument_value/replaceable optional, .../optional ) /optional;
  /synopsis
  
   para
*** OPEN replaceablebound_cursorvar/repla
*** 2847,2856 
--- 2847,2868 
   /para
  
   para
+   Cursors that have named parameters may be opened using either
+   firsttermnamed/firstterm or firsttermpositional/firstterm
+   notation. In contrast with calling functions, described in xref
+   linkend=sql-syntax-calling-funcs, it is not allowed to mix
+   positional and named notation. In positional notation, all arguments
+   are specified in order. In named notation, each argument's name is
+   specified using literal:=/literal to separate it from the
+   argument expression.
+  /para
+ 
+  para
Examples (these use the cursor declaration examples above):
  programlisting
  OPEN curs2;
  OPEN curs3(42);
+ OPEN curs3(key := 42);
  /programlisting
   /para
  
*** COMMIT;
*** 3169,3175 
  
  synopsis
  optional lt;lt;replaceablelabel/replaceablegt;gt; /optional
! FOR replaceablerecordvar/replaceable IN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional LOOP
  replaceablestatements/replaceable
  END LOOP optional replaceablelabel/replaceable /optional;
  /synopsis
--- 3181,3187 
  
  synopsis
  optional lt;lt;replaceablelabel/replaceablegt;gt; /optional
! FOR replaceablerecordvar/replaceable IN replaceablebound_cursorvar/replaceable optional ( optional replaceableargname/replaceable := /optional replaceableargument_value/replaceable optional, .../optional ) /optional LOOP
  replaceablestatements/replaceable
  END LOOP optional replaceablelabel/replaceable /optional;
  /synopsis
*** END LOOP optional replaceablelabel/
*** 3187,3192 
--- 3199,3209 
   Each row returned by the cursor is successively assigned to this
   record variable and the loop body is executed.
  /para
+ 
+ para
+  See xref linkend=plpgsql-open-bound-cursor on calling cursors with
+  named notation.
+ /para
 /sect2
  
/sect1
diff --git a/src/pl/plpgsql/src/gram.y b/src/pl/plpgsql/src/gram.y
new file mode 100644
index 8c4c2f7..5271ab5
*** a/src/pl/plpgsql/src/gram.y
--- b/src/pl/plpgsql/src/gram.y
*** static	PLpgSQL_expr	*read_sql_construct(
*** 67,72 
--- 67,73 
  			const char *sqlstart,
  			bool isexpression,
  			bool valid_sql,
+ 			bool trim,
  			int *startloc,
  			int *endtoken);
  static	PLpgSQL_expr	*read_sql_expression(int until,
*** for_control		: for_variable K_IN
*** 1313,1318 
--- 1314,1320 
  	   SELECT ,
  	   true,
  	   false,
+ 	   true,
  	   expr1loc,
  	   tok);
  
*** stmt_raise		: K_RAISE
*** 1692,1698 
  	expr = read_sql_construct(',', ';', K_USING,
  			  , or ; or USING,
  			  SELECT ,
! 			  true, true,
  			  NULL, tok);
  	new-params

Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters

2011-10-17 Thread Yeb Havinga

On 2011-10-15 07:41, Tom Lane wrote:

Yeb Havingayebhavi...@gmail.com  writes:

Hello Royce,
Thanks again for testing.

I looked this patch over but concluded that it's not ready to apply,
mainly because there are too many weird behaviors around error
reporting.


Tom, thanks for reviewing - getting the syntax errors to be at the exact 
location was indeed something that I thought would be near impossible, 
however the whitespace suggestion together with the others you made seem 
like a good path to go forward. Thanks for taking the time to write your 
comments, it will be a great help with making an improved version.


regards,
Yeb Havinga


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


Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters

2011-10-11 Thread Yeb Havinga

Hello Royce,

Thanks again for testing.

On 2011-10-11 13:55, Royce Ausburn wrote:


Just one small thing: it'd be nice to have an example for cursor declaration 
with named parameters.  Your patch adds one for opening a cursor, but not for 
declaring one.


Declaration of cursors with named parameters is already part of 
PostgreSQL (so it is possible to use the parameter names in the cursor 
query instead of $1, $2, etc.) and it also already documented with an 
example, just a few lines above the open examples. See curs3 on 
http://developer.postgresql.org/pgdocs/postgres/plpgsql-cursors.html



Other than that, I think the patch is good.  Everything works as advertised =)


Thanks!

--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] [REVIEW] Patch for cursor calling with named parameters

2011-10-07 Thread Yeb Havinga

On 2011-10-06 16:04, Royce Ausburn wrote:

Initial Review for patch:

http://archives.postgresql.org/pgsql-hackers/2011-09/msg00744.php


Hello Royce,

Thank you for your review.



I don't think so.  The new feature accepts opening a cursor with some 
parameter names not specified:


  open cur1(param3 := 4, 1, param1 := 5);

It seems that if a parameter is not named, its position is used to 
bind to a variable.  For example, the following fail:


psql:plsqltest.sql:26: ERROR:  cursor cur1 argument 2 param2 
provided multiple times

LINE 10:   open cur1(param3 := 4, 1, param2 := 5);

and

psql:plsqltest.sql:26: ERROR:  cursor cur1 argument 2 param2 
provided multiple times

LINE 10:   open cur1(param2 := 4, 2, param1 := 5);


I think that postgres ought to enforce some consistency here.  Use one 
way or the other, never both.


This was meant as a feature, but I can remove it.



I can also produce some unhelpful errors when I give bad syntax.  For 
example:


psql:plsqltest.sql:28: ERROR:  cursor cur1 argument 1 param1 
provided multiple times

LINE 11:   open cur1( param3 : = 4, 2, param1 := 5);
(notice the space between the : and =)


Yes, the whole of the expression before the first comma, 'param3 : = 4' 
is not recognized as parametername := symbol expression, so that 
is taken as the value of the first parameter. This value is parsed after 
all named arguments are read, and hence no meaningful error is given. If 
there was no param1 parameter name at the end, the 'multiple times' 
error would not have caused the processing to stop, and a syntax error 
at the correct : would have been given.


The same reasoning also explains the other 'multiple times' errors you 
could get, by putting a syntax error in some value.


--

  open cur1( param3 := param3 , param2 = 3, param1 := 1 );

psql:plsqltest.sql:29: ERROR:  column param2 does not exist
LINE 2: ,param2 = 3
 ^
QUERY:  SELECT 1
,param2 = 3
,param3;
CONTEXT:  PL/pgSQL function named_para_test line 7 at OPEN


This is a valid error, since the parser / SQL will try to evaluate the 
boolean expression param2 = 3, while param2 is not a defined variabele.


Again, thank you very much for your thorough review. I'll update the 
patch so mixing positional and named parameters are removed, add 
documentation, and give syntax errors before an error message indicating 
that positional and named parameters were mixed.


--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] [REVIEW] Patch for cursor calling with named parameters

2011-10-07 Thread Yeb Havinga

On 2011-10-07 12:21, Yeb Havinga wrote:

On 2011-10-06 16:04, Royce Ausburn wrote:

Initial Review for patch:

http://archives.postgresql.org/pgsql-hackers/2011-09/msg00744.php



Again, thank you very much for your thorough review. I'll update the 
patch so mixing positional and named parameters are removed, add 
documentation, and give syntax errors before an error message 
indicating that positional and named parameters were mixed.




Attach is v2 of the patch.

Mixed notation now raises an error.

In contrast with what I said above, named parameter related errors are 
thrown before any syntax errors. I tested with raising syntax errors 
first but the resulting code was a bit more ugly and the sql checking 
under a error condition (i.e. double named parameter error means there 
is one parameter in short)  was causing serious errors.


Documentation was also added, regression tests updated.

regards,

--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
new file mode 100644
index c14c34c..45081f8
*** a/doc/src/sgml/plpgsql.sgml
--- b/doc/src/sgml/plpgsql.sgml
*** END;
*** 2699,2718 
   Another way is to use the cursor declaration syntax,
   which in general is:
  synopsis
! replaceablename/replaceable optional optional NO /optional SCROLL /optional CURSOR optional ( replaceablearguments/replaceable ) /optional FOR replaceablequery/replaceable;
  /synopsis
   (literalFOR/ can be replaced by literalIS/ for
!  productnameOracle/productname compatibility.)
!  If literalSCROLL/ is specified, the cursor will be capable of
!  scrolling backward; if literalNO SCROLL/ is specified, backward
!  fetches will be rejected; if neither specification appears, it is
!  query-dependent whether backward fetches will be allowed.
!  replaceablearguments/replaceable, if specified, is a
!  comma-separated list of pairs literalreplaceablename/replaceable
!  replaceabledatatype/replaceable/literal that define names to be
!  replaced by parameter values in the given query.  The actual
!  values to substitute for these names will be specified later,
!  when the cursor is opened.
  /para
  para
   Some examples:
--- 2699,2717 
   Another way is to use the cursor declaration syntax,
   which in general is:
  synopsis
!  replaceablename/replaceable optional optional NO /optional SCROLL /optional CURSOR optional ( optional replaceableargname/replaceable /optional replaceableargtype/replaceable optional, .../optional) /optional FOR replaceablequery/replaceable;
  /synopsis
   (literalFOR/ can be replaced by literalIS/ for
!  productnameOracle/productname compatibility.)  If literalSCROLL/
!  is specified, the cursor will be capable of scrolling backward; if
!  literalNO SCROLL/ is specified, backward fetches will be rejected; if
!  neither specification appears, it is query-dependent whether backward
!  fetches will be allowed.  replaceableargname/replaceable, if
!  specified, defines the name to be replaced by parameter values in the
!  given query.  The actual values to substitute for these names will be
!  specified later, when the cursor is opened.
!  literalreplaceableargtype/replaceable/literal defines the datatype
!  of the parameter.
  /para
  para
   Some examples:
*** OPEN curs1 FOR EXECUTE 'SELECT * FROM '
*** 2827,2833 
   titleOpening a Bound Cursor/title
  
  synopsis
! OPEN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional;
  /synopsis
  
   para
--- 2826,2832 
   titleOpening a Bound Cursor/title
  
  synopsis
!  OPEN replaceablebound_cursorvar/replaceable optional ( optional replaceableargname/replaceable := /optional replaceableargument_value/replaceable optional, .../optional ) /optional;
  /synopsis
  
   para
*** OPEN replaceablebound_cursorvar/repla
*** 2854,2864 
--- 2853,2875 
commandOPEN/.
   /para
  
+  para
+   Cursors that have named parameters may be opened using either
+   firsttermnamed/firstterm or firsttermpositional/firstterm
+   notation. In contrast with calling functions, described in xref
+   linkend=sql-syntax-calling-funcs, it is not allowed to mix
+   positional and named notation. In positional notation, all arguments
+   are specified in order. In named notation, each argument's name is
+   specified using literal:=/literal to separate it from the
+   argument expression.
+  /para
+ 
  para
   Examples:
  programlisting
  OPEN curs2;
  OPEN curs3(42);
+ OPEN curs3(key := 42);
  /programlisting
 /para
   /sect3
diff --git a/src/pl/plpgsql/src/gram.y b/src/pl/plpgsql/src/gram.y
new file mode 100644
index f8e956b..b9bf888
*** a/src

[HACKERS] Satisfy extension dependency by one of multiple extensions

2011-09-23 Thread Yeb Havinga

Hello list,

I have a use case where an extension dependency can be satisfied by one 
of five other extensions. Currently I'm unable to express that in the 
extension control file, since the elements from 'requires' are currently 
searched on exact name match. The attached patch changes this behaviour 
for list elements that end with a *, into prefix matching, so that e.g. 
table* matches tablefunc.


This allows me to specify in a controlfile

requires 'vocab*'

which is satisfied by having either one of the following extensions loaded:

vocab2005
vocab2006
vocab2008
vocab2009
vocab2010

thoughts?

regards,
Yeb Havinga

--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
new file mode 100644
index 278bbcf..5bf7f8e
*** a/src/backend/commands/extension.c
--- b/src/backend/commands/extension.c
*** static void ApplyExtensionUpdates(Oid ex
*** 114,145 
  Oid
  get_extension_oid(const char *extname, bool missing_ok)
  {
! 	Oid			result;
! 	Relation	rel;
  	SysScanDesc scandesc;
  	HeapTuple	tuple;
  	ScanKeyData entry[1];
  
  	rel = heap_open(ExtensionRelationId, AccessShareLock);
  
  	ScanKeyInit(entry[0],
  Anum_pg_extension_extname,
! BTEqualStrategyNumber, F_NAMEEQ,
  CStringGetDatum(extname));
  
! 	scandesc = systable_beginscan(rel, ExtensionNameIndexId, true,
!   SnapshotNow, 1, entry);
  
! 	tuple = systable_getnext(scandesc);
  
! 	/* We assume that there can be at most one matching tuple */
! 	if (HeapTupleIsValid(tuple))
! 		result = HeapTupleGetOid(tuple);
! 	else
! 		result = InvalidOid;
  
! 	systable_endscan(scandesc);
  
  	heap_close(rel, AccessShareLock);
  
  	if (!OidIsValid(result)  !missing_ok)
--- 114,152 
  Oid
  get_extension_oid(const char *extname, bool missing_ok)
  {
! 	Oid			result   = InvalidOid;
! 	Relation	rel, idx;
  	SysScanDesc scandesc;
  	HeapTuple	tuple;
  	ScanKeyData entry[1];
+ 	int len  = strlen(extname) - 1;
+ 	boolwildcard = (extname[len] == '*');
  
  	rel = heap_open(ExtensionRelationId, AccessShareLock);
+ 	idx = index_open(ExtensionNameIndexId, AccessShareLock);
  
  	ScanKeyInit(entry[0],
  Anum_pg_extension_extname,
! BTGreaterEqualStrategyNumber,
! F_NAMEGE,
  CStringGetDatum(extname));
  
! 	scandesc = systable_beginscan_ordered(rel, idx,
! 		  SnapshotNow, 1, entry);
  
! 	if (HeapTupleIsValid(tuple = systable_getnext_ordered(scandesc, ForwardScanDirection)))
! 	{
! 		Form_pg_extension form = (Form_pg_extension) GETSTRUCT(tuple);
  
! 		if (wildcard
! 			? (strncmp(extname, NameStr(form-extname), len) == 0)
! 			: (strcmp(extname, NameStr(form-extname)) == 0))
! 			result = HeapTupleGetOid(tuple);
! 	}
  
! 	systable_endscan_ordered(scandesc);
  
+ 	index_close(idx, AccessShareLock);
  	heap_close(rel, AccessShareLock);
  
  	if (!OidIsValid(result)  !missing_ok)

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


Re: [HACKERS] Satisfy extension dependency by one of multiple extensions

2011-09-23 Thread Yeb Havinga

On 2011-09-23 14:19, Heikki Linnakangas wrote:

On 23.09.2011 14:56, Yeb Havinga wrote:

I have a use case where an extension dependency can be satisfied by one
of five other extensions. Currently I'm unable to express that in the
extension control file, since the elements from 'requires' are currently
searched on exact name match. The attached patch changes this behaviour
for list elements that end with a *, into prefix matching, so that e.g.
table* matches tablefunc.


That's going to quickly extend into even more complex rules, like foo 
OR bar, (foo OR bar) AND (foobar) etc. IIRC the extension control 
file format was modeled after some other package management system 
(.deb ?). You might want to look at the past discussions when the 
extension control file format was decided.


Ech.. 2364 hits on 'extension' in my mailbox. However I found a thread 
'extension dependency checking' that also talks about version numbers 
and the 'provides' capability you mention below.


We might want to have a system where an extension can declare that it 
provides capabilites, and then have another extension require 
those capabilities. That would be a neater solution to the case that 
there are multiple extensions that all provide the same capability.




Yes that would be neater. I can't think of anything else however than to 
add 'extprovides' to pg_extension, fill it with an explicit 'provides' 
from the control file when present, or extname otherwise, and use that 
column to check the 'requires' list on extension creation time.


--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data


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


[HACKERS] Patch for cursor calling with named parameters

2011-09-15 Thread Yeb Havinga

Hello list,

The following patch implements cursor calling with named parameters in 
addition to the standard positional argument lists.


c1 cursor (param1 int, param2 int) for select * from rc_test where a  
param1 and b  param2;

open c1($1, $2); -- this is currently possible
open c1(param2 := $2, param1 := $1); -- this is the new feature

Especially for cursors with a lot of arguments, this increases 
readability of code. This was discussed previously in 
http://archives.postgresql.org/pgsql-hackers/2010-09/msg01433.php. We 
actually made two patches: one with = and then one with := notation. 
Attached is the patch with := notation.


Is it ok to add it to the next commitfest?

regards,
Yeb Havinga, Willem Dijkstra

--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

diff --git a/src/pl/plpgsql/src/gram.y b/src/pl/plpgsql/src/gram.y
new file mode 100644
index 92b54dd..192f278
*** a/src/pl/plpgsql/src/gram.y
--- b/src/pl/plpgsql/src/gram.y
*** read_sql_expression(int until, const cha
*** 2335,2340 
--- 2335,2352 
  			  SELECT , true, true, NULL, NULL);
  }
  
+ /*
+  * Convenience routine to read a single unchecked expression with two possible
+  * terminators, returning an expression with an empty sql prefix.
+  */
+ static PLpgSQL_expr *
+ read_sql_one_expression(int until, int until2, const char *expected,
+ 		int *endtoken)
+ {
+ 	return read_sql_construct(until, until2, 0, expected,
+ 			  , true, false, NULL, endtoken);
+ }
+ 
  /* Convenience routine to read an expression with two possible terminators */
  static PLpgSQL_expr *
  read_sql_expression2(int until, int until2, const char *expected,
*** check_labels(const char *start_label, co
*** 3384,3399 
  /*
   * Read the arguments (if any) for a cursor, followed by the until token
   *
!  * If cursor has no args, just swallow the until token and return NULL.
!  * If it does have args, we expect to see ( expr [, expr ...] ) followed
!  * by the until token.  Consume all that and return a SELECT query that
!  * evaluates the expression(s) (without the outer parens).
   */
  static PLpgSQL_expr *
  read_cursor_args(PLpgSQL_var *cursor, int until, const char *expected)
  {
  	PLpgSQL_expr *expr;
! 	int			tok;
  
  	tok = yylex();
  	if (cursor-cursor_explicit_argrow  0)
--- 3396,3418 
  /*
   * Read the arguments (if any) for a cursor, followed by the until token
   *
!  * If cursor has no args, just swallow the until token and return NULL.  If it
!  * does have args, we expect to see ( expr [, expr ...] ) followed by the
!  * until token, where expr may be a plain expression, or a named parameter
!  * assignment of the form IDENT := expr. Consume all that and return a SELECT
!  * query that evaluates the expression(s) (without the outer parens).
   */
  static PLpgSQL_expr *
  read_cursor_args(PLpgSQL_var *cursor, int until, const char *expected)
  {
  	PLpgSQL_expr *expr;
! 	PLpgSQL_row *row;
! 	int tok;
! 	int argc = 0;
! 	char **argv;
! 	StringInfoData ds;
! 	char *sqlstart = SELECT ;
! 	int startlocation = yylloc;
  
  	tok = yylex();
  	if (cursor-cursor_explicit_argrow  0)
*** read_cursor_args(PLpgSQL_var *cursor, in
*** 3412,3417 
--- 3431,3439 
  		return NULL;
  	}
  
+ 	row = (PLpgSQL_row *) plpgsql_Datums[cursor-cursor_explicit_argrow];
+ 	argv = (char **) palloc0(sizeof(char *) * row-nfields);
+ 
  	/* Else better provide arguments */
  	if (tok != '(')
  		ereport(ERROR,
*** read_cursor_args(PLpgSQL_var *cursor, in
*** 3420,3429 
  		cursor-refname),
   parser_errposition(yylloc)));
  
! 	/*
! 	 * Read expressions until the matching ')'.
! 	 */
! 	expr = read_sql_expression(')', ));
  
  	/* Next we'd better find the until token */
  	tok = yylex();
--- 3442,3527 
  		cursor-refname),
   parser_errposition(yylloc)));
  
! 	for (argc = 0; argc  row-nfields; argc++)
! 	{
! 		int argpos;
! 		int endtoken;
! 		PLpgSQL_expr *item;
! 
! 		if (plpgsql_isidentassign())
! 		{
! 			/* Named parameter assignment */
! 			for (argpos = 0; argpos  row-nfields; argpos++)
! if (strncmp(row-fieldnames[argpos], yylval.str, strlen(row-fieldnames[argpos])) == 0)
! 	break;
! 
! 			if (argpos == row-nfields)
! ereport(ERROR,
! 		(errcode(ERRCODE_SYNTAX_ERROR),
! 		 errmsg(cursor \%s\ has no argument named \%s\,
! cursor-refname, yylval.str),
! 		 parser_errposition(yylloc)));
! 		}
! 		else
! 		{
! 			/* Positional parameter assignment */
! 			argpos = argc;
! 		}
! 
! 		/*
! 		 * Read one expression at a time until the matching endtoken. Checking
! 		 * the expressions is postponed until the positional argument list is
! 		 * made.
! 		 */
! 		item = read_sql_one_expression(',', ')', ,\ or \), endtoken);
! 
! 		if (endtoken == ')'  !(argc == row-nfields - 1))
! 			ereport(ERROR,
! 	(errcode(ERRCODE_SYNTAX_ERROR),
! 	 errmsg(not enough arguments for cursor \%s\,
! 			cursor

Re: [HACKERS] Patch for cursor calling with named parameters

2011-09-15 Thread Yeb Havinga

On 2011-09-15 16:31, Cédric Villemain wrote:

There exist also a mecanism to order the parameters of  'EXECUTE ...
USING ...'  (it's using a cursor), can the current work benefit to
EXECUTE USING to use named parameters ?


I looked at it a bit but it seems there is no benefit, since the dynamic 
sql handling vs the cursor declaration and opening touch different code 
paths in both the plpgsql grammar and the SPI functions that are called.


regards,
Yeb


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


Re: [HACKERS] [v9.2] Fix Leaky View Problem

2011-09-07 Thread Yeb Havinga

On 2011-09-07 16:02, Kevin Grittner wrote:

Robert Haasrobertmh...@gmail.com  wrote:


Anyone else want to bikeshed?


I'm not sure they beat TRUSTED, but:

SECURE
OPAQUE


SAVE

-- Yeb


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


Re: [HACKERS] [v9.1] sepgsql - userspace access vector cache

2011-09-06 Thread Yeb Havinga

On 2011-09-06 04:55, Robert Haas wrote:

On Mon, Sep 5, 2011 at 10:52 PM, Alvaro Herrera
alvhe...@commandprompt.com  wrote:

Excerpts from Robert Haas's message of lun sep 05 23:27:16 -0300 2011:

On Mon, Sep 5, 2011 at 9:14 AM, Yeb Havingayebhavi...@gmail.com  wrote:

I didn't see my name as one of the reviewers in the commit message. If that
is because the review was bad, I'd be interested to know what I can improve
for the next one.

No, it's because I flaked.  Sorry, Yeb.

Pity we can't use git notes.

Well, I guess there's no law that says we can't.  Should I give it a try?



I don't mind keeping the current commit message, I was just trying to 
read between the lines and that has been answered.


regards,
Yeb


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


Re: [HACKERS] [v9.1] sepgsql - userspace access vector cache

2011-09-05 Thread Yeb Havinga

On 2011-09-01 14:40, Robert Haas wrote:

  userspace avc.
I've committed this, but I still think it would be helpful to revise
that comment.  The turn boosted up is not very precise or
informative.  Could you submit a separate, comment-only patch to
improve this?


I didn't see my name as one of the reviewers in the commit message. If 
that is because the review was bad, I'd be interested to know what I can 
improve for the next one.


regards,
Yeb Havinga


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


Re: [HACKERS] Transient plans versus the SPI API

2011-08-04 Thread Yeb Havinga

On 2011-08-03 21:19, Tom Lane wrote:

Robert Haasrobertmh...@gmail.com  writes:

This seems like a good design.  Now what would be really cool is if
you could observe a stream of queries like this:
SELECT a, b FROM foo WHERE c = 123
SELECT a, b FROM foo WHERE c = 97
SELECT a, b FROM foo WHERE c = 236
...and say, hey, I could just make a generic plan and use it every
time I see one of these.  It's not too clear to me how you'd make
recognition of such queries cheap enough to be practical, but maybe
someone will think of a way...

Hm, you mean reverse-engineering the parameterization of the query?
Interesting thought, but I really don't see a way to make it practical.


See also http://archives.postgresql.org/pgsql-hackers/2010-11/msg00617.php

I don't know if any implementation can be practical - maybe the parser 
could be coerced into emitting some kind of number that's based on 
everything in the query, except constants (and whitespace), so it would 
be the same for all the queries Robert described. That could be low cost 
enough to detect of for a query's id a cached plan exists and do more 
work only in those cases.


--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] Pull up aggregate sublink (was: Parameterized aggregate subquery (was: Pull up aggregate subquery))

2011-07-27 Thread Yeb Havinga

On 2011-07-27 16:16, Robert Haas wrote:

On Tue, Jul 26, 2011 at 5:37 PM, Tom Lanet...@sss.pgh.pa.us  wrote:

Yeb Havingayebhavi...@gmail.com  writes:

A few days ago I read Tomas Vondra's blog post about dss tpc-h queries
on PostgreSQL at
http://fuzzy.cz/en/articles/dss-tpc-h-benchmark-with-postgresql/ - in
which he showed how to manually pull up a dss subquery to get a large
speed up. Initially I thought: cool, this is probably now handled by
Hitoshi's patch, but it turns out the subquery type in the dss query is
different.

Actually, I believe this example is the exact opposite of the
transformation Hitoshi proposes.  Tomas was manually replacing an
aggregated subquery by a reference to a grouped table, which can be
a win if the subquery would be executed enough times to amortize
calculation of the grouped table over all the groups (some of which
might never be demanded by the outer query).  Hitoshi was talking about
avoiding calculations of grouped-table elements that we don't need,
which would be a win in different cases.  Or at least that was the
thrust of his original proposal; I'm not sure where the patch went since
then.

This leads me to think that we need to represent both cases as the same
sort of query and make a cost-based decision as to which way to go.
Thinking of it as a pull-up or push-down transformation is the wrong
approach because those sorts of transformations are done too early to
be able to use cost comparisons.

I think you're right.  OTOH, our estimates of what will pop out of an
aggregate are so poor that denying the user to control the plan on the
basis of how they write the query might be a net negative.  :-(



Tom and Robert, thank you both for your replies. I think I'm having some 
blind spots and maybe false assumptions regarding the overal work in the 
optimizer, as it is not clear to me what 'the same sort of query' would 
look like. I was under the impression that using cost to select the best 
paths is only done per simple query, and fail to see how a total 
combined plan with pulled up subquery could be compared on cost with a 
total plan where the subquery is still a separate subplan, since the 
range tables / simple-queries to compare are different.


regards,
Yeb


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


[HACKERS] Pull up aggregate sublink (was: Parameterized aggregate subquery (was: Pull up aggregate subquery))

2011-07-26 Thread Yeb Havinga

On 2011-07-22 17:35, Hitoshi Harada wrote:

2011/7/23 Yeb Havingayebhavi...@gmail.com:

Works like a charm :-). However, now there is always a copyObject of a
subquery even when the subquery is not safe for qual pushdown. The problem
with the previous issafe was that it was only assigned for
rel-baserestrictinfo != NIL. If it is assigned before the if statement, it
still works. See attached patch that avoids subquery copy for unsafe
subqueries, and also exits best_inner_subqueryscan before palloc of
differenttypes in case of unsafe queries.


Ah, yeah, right. Too quick fix bloated my brain :P Thanks for testing!
I'll check it more.


A few days ago I read Tomas Vondra's blog post about dss tpc-h queries 
on PostgreSQL at 
http://fuzzy.cz/en/articles/dss-tpc-h-benchmark-with-postgresql/ - in 
which he showed how to manually pull up a dss subquery to get a large 
speed up. Initially I thought: cool, this is probably now handled by 
Hitoshi's patch, but it turns out the subquery type in the dss query is 
different.


The original and rewritten queries are below. The debug_print_plan 
output shows the subquery is called from a opexpr ( l_quantity, 
subquery output) and the sublink type is EXPR_SUBLINK. Looking at the 
source code; pull_up_sublink only considers ANY and EXISTS sublinks. I'm 
wondering if this could be expanded to deal with EXPR sublinks. Clearly 
in the example Tomas has given this can be done. I'm wondering if there 
are show stoppers that prevent this to be possible in the general case, 
but can't think of any, other than the case of a sublink returning NULL 
and the opexpr is part of a larger OR expression or IS NULL; in which 
case it should not be pulled op, or perhaps it could be pulled up as 
outer join.


Thoughts?

regards,
Yeb


The original query:

tpch=# explain select
sum(l_extendedprice) / 7.0 as avg_yearly
from
lineitem,
part
where
p_partkey = l_partkey
and p_brand = 'Brand#13'
and p_container = 'JUMBO PKG'
and l_quantity  (
select
0.2 * avg(l_quantity)
from
lineitem
where
l_partkey = p_partkey
)
LIMIT 1;
 QUERY PLAN

 Limit  (cost=183345309.79..183345309.81 rows=1 width=8)
   -  Aggregate  (cost=183345309.79..183345309.81 rows=1 width=8)
 -  Hash Join  (cost=2839.99..183345307.76 rows=815 width=8)
   Hash Cond: (public.lineitem.l_partkey = part.p_partkey)
   Join Filter: (public.lineitem.l_quantity  (SubPlan 1))
   -  Seq Scan on lineitem  (cost=0.00..68985.69 
rows=2399869 width=17)

   -  Hash  (cost=2839.00..2839.00 rows=79 width=4)
 -  Seq Scan on part  (cost=0.00..2839.00 rows=79 
width=4)
   Filter: ((p_brand = 'Brand#13'::bpchar) AND 
(p_container = 'JUMBO PKG'::bpchar))

   SubPlan 1
 -  Aggregate  (cost=74985.44..74985.46 rows=1 width=5)
   -  Seq Scan on lineitem  (cost=0.00..74985.36 
rows=31 width=5)

 Filter: (l_partkey = part.p_partkey)

manually rewritten variant:

tpch=# explain select
sum(l_extendedprice) / 7.0 as avg_yearly
from
lineitem,
part,
(SELECT l_partkey AS agg_partkey,
0.2 * avg(l_quantity) AS avg_quantity
  FROM lineitem GROUP BY l_partkey) part_agg
where
p_partkey = l_partkey
and agg_partkey = l_partkey
and p_brand = 'Brand#13'
and p_container = 'JUMBO PKG'
and l_quantity  avg_quantity
LIMIT 1;
   QUERY PLAN

 Limit  (cost=179643.88..179643.89 rows=1 width=8)
   -  Aggregate  (cost=179643.88..179643.89 rows=1 width=8)
 -  Hash Join  (cost=161865.21..178853.91 rows=315985 width=8)
   Hash Cond: (public.lineitem.l_partkey = part.p_partkey)
   Join Filter: (public.lineitem.l_quantity  ((0.2 * 
avg(public.lineitem.l_quantity
   -  HashAggregate  (cost=80985.04..82148.65 rows=77574 
width=9)
 -  Seq Scan on lineitem  (cost=0.00..68985.69 
rows=2399869 width=9)

   -  Hash  (cost=80849.63..80849.63 rows=2444 width=21)
 -  Hash Join  (cost=2839.99..80849.63 rows=2444 
width=21)
   Hash Cond: (public.lineitem.l_partkey = 
part.p_partkey)
   -  Seq Scan on lineitem  
(cost=0.00..68985.69 rows=2399869 width=17)
   -  Hash  (cost=2839.00..2839.00 rows=79 
width=4)
 -  Seq Scan on part  
(cost=0.00..2839.00 rows=79 width=4)
   Filter: ((p_brand = 
'Brand#13'::bpchar) AND (p_container = 'JUMBO PKG'::bpchar))




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your 

Re: [HACKERS] [v9.1] sepgsql - userspace access vector cache

2011-07-22 Thread Yeb Havinga

On 2011-07-22 11:55, Kohei Kaigai wrote:



2) Also I thought if it could work to not remember tcontext is valid, but 
instead remember the consequence,
which is that it is replaced by unlabeled. It makes the avc_cache struct 
shorter and the code somewhat
simpler.


Here is a reason why we hold tcontext, even if it is not valid.
The hash key of avc_cache is combination of scontext, tcontext and tclass. 
Thus, if we replaced an invalid
tcontext by unlabeled context, it would always make cache mishit and 
performance loss.

I see that now, thanks.

I have no further comments, and I think that the patch in it's current 
status is ready for committer.


regards,
--

Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] [v9.1] sepgsql - userspace access vector cache

2011-07-22 Thread Yeb Havinga

On 2011-07-21 11:29, Kohei Kaigai wrote:

The attached patch is revised userspace-avc patch.

List of updates:
- The GUC of sepgsql.avc_threshold was removed.
- char *ucontext of avc_cache was replaced by bool tcontext_is_valid.
- Comments added onto static variables
- Comments of sepgsql_avc_unlabeled() was revised.
- Comments of sepgsql_avc_compute() was simplified.
- Comments of sepgsql_avc_check_perms_label() also mention about
   permissive domain, that performs similar to system's permissive mode.
- selinux_status_close() become invoked on on_proc_exit() hook.
Thank you for the update, I'm looking at it right now and with a new 
look have some more questions. I took the liberty to supply a patch to 
be applied after your v5 uavc patch.


1) At a few call sites of sepgsql_avc_lookup, a null tcontext is 
detected, and then replaced by unlabeled. I moved this to 
sepgsql_avc_lookup itself.
2) Also I thought if it could work to not remember tcontext is valid, 
but instead remember the consequence, which is that it is replaced by 
unlabeled. It makes the avc_cache struct shorter and the code somewhat 
simpler.


regards,
--

Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

diff --git a/contrib/sepgsql/uavc.c b/contrib/sepgsql/uavc.c
new file mode 100644
index 57a5197..2bcfedf
*** a/contrib/sepgsql/uavc.c
--- b/contrib/sepgsql/uavc.c
*** typedef struct
*** 41,48 
  
  	bool		permissive;	/* true, if permissive rule */
  	bool		hot_cache;	/* true, if recently referenced */
- 	bool		tcontext_is_valid;
- 			/* true, if tcontext is valid */
  	char	   *ncontext;	/* temporary scontext on execution of trusted
  			 * procedure, or NULL elsewhere */
  } avc_cache;
--- 41,46 
*** sepgsql_avc_reset(void)
*** 88,94 
  
  /*
   * Reclaim caches recently unreferenced
!  */	
  static void
  sepgsql_avc_reclaim(void)
  {
--- 86,92 
  
  /*
   * Reclaim caches recently unreferenced
!  */
  static void
  sepgsql_avc_reclaim(void)
  {
*** sepgsql_avc_check_valid(void)
*** 153,159 
  /*
   * sepgsql_avc_unlabeled
   *
!  * It returns an alternative label to be applied when no label or invalid 
   * label would be assigned on objects.
   */
  static char *
--- 151,157 
  /*
   * sepgsql_avc_unlabeled
   *
!  * It returns an alternative label to be applied when no label or invalid
   * label would be assigned on objects.
   */
  static char *
*** sepgsql_avc_unlabeled(void)
*** 184,198 
  }
  
  /*
!  * sepgsql_avc_compute 
   *
   * A fallback path, when cache mishit. It asks SELinux its access control
   * decision for the supplied pair of security context and object class.
   */
  static avc_cache *
! sepgsql_avc_compute(const char *scontext, const char *tcontext, uint16 tclass)
  {
- 	char		   *ucontext = NULL;
  	char		   *ncontext = NULL;
  	MemoryContext	oldctx;
  	avc_cache	   *cache;
--- 182,195 
  }
  
  /*
!  * sepgsql_avc_compute
   *
   * A fallback path, when cache mishit. It asks SELinux its access control
   * decision for the supplied pair of security context and object class.
   */
  static avc_cache *
! sepgsql_avc_compute(const char *scontext, char *tcontext, uint16 tclass)
  {
  	char		   *ncontext = NULL;
  	MemoryContext	oldctx;
  	avc_cache	   *cache;
*** sepgsql_avc_compute(const char *scontext
*** 207,224 
  	 * Validation check of the supplied security context.
  	 * Because it always invoke system-call, frequent check should be avoided.
  	 * Unless security policy is reloaded, validation status shall be kept, so
! 	 * we also cache whether the supplied security context was valid, or not.
  	 */
  	if (security_check_context_raw((security_context_t)tcontext) != 0)
! 		ucontext = sepgsql_avc_unlabeled();
  
! 	/*
! 	 * Ask SELinux its access control decision
! 	 */
! 	if (!ucontext)
! 		sepgsql_compute_avd(scontext, tcontext, tclass, avd);
! 	else
! 		sepgsql_compute_avd(scontext, ucontext, tclass, avd);
  
  	/*
  	 * To boost up trusted procedure checks on db_procedure object
--- 204,216 
  	 * Validation check of the supplied security context.
  	 * Because it always invoke system-call, frequent check should be avoided.
  	 * Unless security policy is reloaded, validation status shall be kept, so
! 	 * we also cache the invalid tcontext replaced by the unlabeled context.
  	 */
  	if (security_check_context_raw((security_context_t)tcontext) != 0)
! 		tcontext = sepgsql_avc_unlabeled();
  
! 	/* Ask SELinux its access control decision */
! 	sepgsql_compute_avd(scontext, tcontext, tclass, avd);
  
  	/*
  	 * To boost up trusted procedure checks on db_procedure object
*** sepgsql_avc_compute(const char *scontext
*** 227,238 
  	 */
  	if (tclass == SEPG_CLASS_DB_PROCEDURE)
  	{
! 		if (!ucontext)
! 			ncontext = sepgsql_compute_create(scontext, tcontext,
! 			  SEPG_CLASS_PROCESS);
! 		else
! 			ncontext = sepgsql_compute_create(scontext, ucontext

Re: [HACKERS] Parameterized aggregate subquery (was: Pull up aggregate subquery)

2011-07-22 Thread Yeb Havinga

On 2011-07-02 10:02, Hitoshi Harada wrote:



Although I still need to think about suitable regression test case,
the patch itself can be reviewed again. You may want to try some
additional tests as you imagine after finding my test case gets
quicker.


Hello Hitoshi-san,

I took a look at your latest patch and it looks good, no comments. 
However I also tried it against current 9.2 HEAD and the test query of 
the start of this thread.


Before and after applying the patch, I get the same result for the test 
query.


postgres=# explain select m_id, sum_len from size_m m inner join(select 
m_id,

sum(length(val)) as sum_len from size_l group by m_id)l on m.id =
l.m_id where val = '10101';
QUERY PLAN
--
 Nested Loop  (cost=79392.64..82938.05 rows=100 width=12)
   Join Filter: (m.id = size_l.m_id)
   -  Seq Scan on size_m m  (cost=0.00..897.00 rows=1 width=4)
 Filter: (val = '10101'::text)
   -  GroupAggregate  (cost=79392.64..81592.15 rows=19951 width=277)
 -  Sort  (cost=79392.64..79892.64 rows=20 width=277)
   Sort Key: size_l.m_id
   -  Seq Scan on size_l  (cost=0.00..9829.00 rows=20 
width=277)


I double checked that I had applied the patch (git diff shows the 
patch), installed and restarted postgres. The database is a fresh 
created database with no edits in postgresql.conf.


regards,

--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] Parameterized aggregate subquery (was: Pull up aggregate subquery)

2011-07-22 Thread Yeb Havinga

On 2011-07-22 16:17, Hitoshi Harada wrote:


:(
I updated the patch. Could you try attached once more? The issafe
switch seems wrong.
Works like a charm :-). However, now there is always a copyObject of a 
subquery even when the subquery is not safe for qual pushdown. The 
problem with the previous issafe was that it was only assigned for 
rel-baserestrictinfo != NIL. If it is assigned before the if statement, 
it still works. See attached patch that avoids subquery copy for unsafe 
subqueries, and also exits best_inner_subqueryscan before palloc of 
differenttypes in case of unsafe queries.


regards,
--

Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
new file mode 100644
index b5be09a..6596a1c
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
*** _outTidPath(StringInfo str, TidPath *nod
*** 1557,1562 
--- 1557,1573 
  }
  
  static void
+ _outSubqueryPath(StringInfo str, SubqueryPath *node)
+ {
+ 	WRITE_NODE_TYPE(SUBQUERYPATH);
+ 
+ 	_outPathInfo(str, (Path *) node);
+ 	WRITE_NODE_FIELD(subplan);
+ 	WRITE_NODE_FIELD(subrtable);
+ 	WRITE_NODE_FIELD(subrowmark);
+ }
+ 
+ static void
  _outForeignPath(StringInfo str, ForeignPath *node)
  {
  	WRITE_NODE_TYPE(FOREIGNPATH);
*** _outRelOptInfo(StringInfo str, RelOptInf
*** 1738,1746 
  	WRITE_NODE_FIELD(indexlist);
  	WRITE_UINT_FIELD(pages);
  	WRITE_FLOAT_FIELD(tuples, %.0f);
- 	WRITE_NODE_FIELD(subplan);
- 	WRITE_NODE_FIELD(subrtable);
- 	WRITE_NODE_FIELD(subrowmark);
  	WRITE_NODE_FIELD(baserestrictinfo);
  	WRITE_NODE_FIELD(joininfo);
  	WRITE_BOOL_FIELD(has_eclass_joins);
--- 1749,1754 
*** _outNode(StringInfo str, void *obj)
*** 2949,2954 
--- 2957,2965 
  			case T_TidPath:
  _outTidPath(str, obj);
  break;
+ 			case T_SubqueryPath:
+ _outSubqueryPath(str, obj);
+ break;
  			case T_ForeignPath:
  _outForeignPath(str, obj);
  break;
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
new file mode 100644
index 6b43aee..bd3a53f
*** a/src/backend/optimizer/path/allpaths.c
--- b/src/backend/optimizer/path/allpaths.c
***
*** 31,36 
--- 31,37 
  #include optimizer/planner.h
  #include optimizer/prep.h
  #include optimizer/restrictinfo.h
+ #include optimizer/subselect.h
  #include optimizer/var.h
  #include parser/parse_clause.h
  #include parser/parsetree.h
*** has_multiple_baserels(PlannerInfo *root)
*** 677,682 
--- 678,689 
  /*
   * set_subquery_pathlist
   *		Build the (single) access path for a subquery RTE
+  *
+  * Although we build only one access path for the subquery,
+  * join search process may find another path by pushing down
+  * the nestloop param into subquery. The finding process will
+  * be done much later than here, but some common operation like
+  * preprocessing subquery are shared.
   */
  static void
  set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel,
*** set_subquery_pathlist(PlannerInfo *root,
*** 687,693 
  	bool	   *differentTypes;
  	double		tuple_fraction;
  	PlannerInfo *subroot;
! 	List	   *pathkeys;
  
  	/*
  	 * Must copy the Query so that planning doesn't mess up the RTE contents
--- 694,702 
  	bool	   *differentTypes;
  	double		tuple_fraction;
  	PlannerInfo *subroot;
! 	Plan	*subplan;
! 	List	*pathkeys;
! 	bool issafe;
  
  	/*
  	 * Must copy the Query so that planning doesn't mess up the RTE contents
*** set_subquery_pathlist(PlannerInfo *root,
*** 720,727 
  	 * XXX Are there any cases where we want to make a policy decision not to
  	 * push down a pushable qual, because it'd result in a worse plan?
  	 */
! 	if (rel-baserestrictinfo != NIL 
! 		subquery_is_pushdown_safe(subquery, subquery, differentTypes))
  	{
  		/* OK to consider pushing down individual quals */
  		List	   *upperrestrictlist = NIL;
--- 729,738 
  	 * XXX Are there any cases where we want to make a policy decision not to
  	 * push down a pushable qual, because it'd result in a worse plan?
  	 */
! 
! 	issafe = subquery_is_pushdown_safe(subquery, subquery, differentTypes);
! 
! 	if (rel-baserestrictinfo != NIL  issafe)
  	{
  		/* OK to consider pushing down individual quals */
  		List	   *upperrestrictlist = NIL;
*** set_subquery_pathlist(PlannerInfo *root,
*** 749,754 
--- 760,769 
  
  	pfree(differentTypes);
  
+ 	/* save it for later use in best_inner_subqueryscan */
+ 	if (issafe)
+ 		rel-preprocessed_subquery = copyObject(subquery);
+ 
  	/*
  	 * We can safely pass the outer tuple_fraction down to the subquery if the
  	 * outer level has no joining, aggregation, or sorting to do. Otherwise
*** set_subquery_pathlist(PlannerInfo *root,
*** 766,792 
  		tuple_fraction = root-tuple_fraction;
  
  	/* Generate the plan for the subquery */
! 	rel-subplan = subquery_planner(root-glob

Re: [HACKERS] [v9.1] sepgsql - userspace access vector cache

2011-07-21 Thread Yeb Havinga

On 2011-07-21 00:08, Robert Haas wrote:

On Wed, Jul 20, 2011 at 4:48 PM, Tom Lanet...@sss.pgh.pa.us  wrote:

Kohei Kaigaikohei.kai...@emea.nec.com  writes:

I'd like to have a discussion about syscache towards next commit-fest.
The issues may be:
  - Initial bucket allocation on most cases never be referenced.
  - Reclaim cache entries on growing up too large.

There used to be support for limiting the number of entries in a
syscache.  It got removed (cf commit
8b9bc234ad43dfa788bde40ebf12e94f16556b7f) because (1) it was remarkably
expensive to do it (extra list manipulations, etc), and (2) performance
tended to fall off a cliff as soon as you had a few more tables or
whatever than the caches would hold.  I'm disinclined to reverse that
decision.  It appears to me that the security label stuff needs a
different set of performance tradeoffs than the rest of the catalogs,
which means it probably ought to do its own caching, rather than trying
to talk us into pessimizing the other catalogs for seclabel's benefit.

I agree that we don't want to limit the size of the catcaches.  We've
been careful to design them in such a way that they won't blow out
memory, and so far there's no evidence that they do.  If it ain't
broke, don't fix it.  Having catcaches that can grow in size as needed
sounds useful to me, though.
Is there a way to dump syscache statistics like there is for 
MemoryContext..Stats (something - gdb helped me there)?


Besides that I have to admit having problems understanding why the 5MB 
cache for pg_seclabel is a problem; it's memory consumption is lineair 
only to the size of the underlying database.  (in contrast with the 
other cache storing access vectors which would have O(n*m) space 
complexity if it wouldn't reclaim space). So it is proportional to the 
number of objects in a database and in size it seems to be in the same 
order as pg_proc, pg_class and pg_attribute.


regards,

--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] [v9.1] sepgsql - userspace access vector cache

2011-07-21 Thread Yeb Havinga

On 2011-07-21 15:03, Robert Haas wrote:

On Thu, Jul 21, 2011 at 4:00 AM, Yeb Havingayebhavi...@gmail.com  wrote:

Besides that I have to admit having problems understanding why the 5MB cache
for pg_seclabel is a problem; it's memory consumption is lineair only to the
size of the underlying database.  (in contrast with the other cache storing
access vectors which would have O(n*m) space complexity if it wouldn't
reclaim space). So it is proportional to the number of objects in a database
and in size it seems to be in the same order as pg_proc, pg_class and
pg_attribute.

Fair enough.  I'm not convinced that the sheer quantity of memory use
is a problem, although I would like to see a few more test results
before we decide that definitively.  I *am* unwilling to pay the
startup overhead of initializing an extra 2048 syscache that only
sepgsql users will actually need.


Is it possible to only include the syscache on --enable-selinux 
configurations? It would imply physical data incompatibility with 
standard configurations, but that's also true for e.g. the block size.


Also, the tests I did with varying bucket sizes suggested that 
decreasing the syscache to 256 didn't show a significant performance 
decrease compared to the 2048 #buckets, for the restorecon test, which 
hits over 3000 objects with security labels. My guess is that that is a 
fair middle of the road database schema size. Are you unwilling to pay 
the startup overhead for a extra 256 syscache?


--

Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] [v9.1] sepgsql - userspace access vector cache

2011-07-21 Thread Yeb Havinga


Is it possible to only include the syscache on --enable-selinux 
configurations? It would imply physical data incompatibility with 
standard configurations, but that's also true for e.g. the block size.


Also, the tests I did with varying bucket sizes suggested that 
decreasing the syscache to 256 didn't show a significant performance 
decrease compared to the 2048 #buckets, for the restorecon test, which 
hits over 3000 objects with security labels. My guess is that that is 
a fair middle of the road database schema size. Are you unwilling to 
pay the startup overhead for a extra 256 syscache?




Hello KaiGai-san,

off-list,

I was wondering why the catalog pg_seclabel exists at all. Why not store 
the labels together with the objects (pg_class, pg_attribute etc) ? The 
syscache wouldn't be needed in that case.


regards,
Yeb



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


Re: [HACKERS] [v9.2] Fix leaky-view problem, part 2

2011-07-20 Thread Yeb Havinga

On 2011-07-09 09:14, Kohei KaiGai wrote:

OK, I'll try to modify the patch according to the flag of pg_proc design.
As long as the default of user-defined function is off, and we provide
built-in functions
with appropriate configurations, it seems to me the burden of DBA is
quite limited.


A different solution to the leaky view problem could be to check access 
to a tuple at or near the heaptuple visibility level, in addition to 
adding tuple access filter conditions to the query. This would have both 
the possible performance benefits of the query rewriting solution, as 
the everything is filtered before further processing at the heaptuple 
visibility level. Fixing leaky views is not needed because they don't 
exist in this case, the code is straightforward, and there's less change 
of future security bugs by either misconfiguration of leakproof 
functions or code that might introduce another leak path.


regards,

--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] [v9.1] sepgsql - userspace access vector cache

2011-07-20 Thread Yeb Havinga

On 2011-07-14 21:46, Kohei KaiGai wrote:

Sorry, the syscache part was mixed to contrib/sepgsql part
in my previous post.
Please see the attached revision.

Although its functionality is enough simple (it just reduces
number of system-call invocation), its performance
improvement is obvious.
So, I hope someone to volunteer to review these patches.
This is a review of the two userspace access vector cache patches for 
SE-PostgreSQL. Proofreading the source I've seen mostly cosmetic things. 
Though I have a few questions, I think the overal shape of the patch is 
good enough to mark it ready for comitter.


Remarks:

* The patches apply cleanly, compile cleanly on Fedora 15. It depends on 
libselinux-2.0.99, and that is checked on by the configure script: good.


* I run SELECT sepgsql_restorecon(NULL) and saw comparable numbers in 
speed gain and also backend process memory increase as indicated by 
KaiGai-san. The vmsize without the patch increases from running 
restorecon 3864kB, with the patch is increases 8160kB, a difference of 
~5MB. Where this change comes from is unclear to me. The avc_cache holds 
only 7 entries, and the avc memory context stats indicate it's only at 
8kB. Investigation into the SECLABELOID syscache revealed that even when 
that cache was set to a #buckets of 2, still after restorecon() the 
backend's vmsize increased about the ~5MB more.


* The size of SECLABELOID is currently set to 2048, which is equal to 
sizes of the pg_proc and pg_attribute caches and hence makes sense. The 
initial contents of pg_seclabel is 3346 entries. Just to get some idea 
what the cachesize means for restorecon performance I tested some 
different values (debugging was on). Until a size of 256, restorecon had 
comparable performance. Small drop from ~415 to ~425 from 128 to 64. 
Cache of 32 had ~435ms performance. Cache of 2 had 680ms. Without 
debugging symbols, I also got a slightly better performance on the 
restorecon call with a 1024 syscache size. This might be something to 
tweak in later versions, when there is more experience what this cache 
size means for performance on real databases.


* The cache is called userspace, presumably because it isn't cached in 
shared memory? If sepgsql is targeted at installations with many 
different kinds of clients (having different subject contexts), or 
access different objects, this is a good choice.


* Checked that the cache keeps working after errors, ok.

* uavc.c defines some static variables like lru_hint, threshold and 
unlabeled. It would be nice to have a small comment with each one that 
says what the variable represents (like is done for the avc_cache struct 
members right above it)


* I had to google SELINUX_AVD_FLAGS_PERMISSIVE to understand what was 
going on. Since the goal why it is recorded a domain is permissive, is 
to prevent flooding the log, maybe renaming avc_cache.permissive into 
avc_cache.log_violations_once would explain itself.


* selinux_status_open() is called and it's man page mentions 
selinux_status_close(). It currently unmaps memory and closes a file 
descriptor, which is done on process exit anyway. I don't have enough 
experience with these kinds of system calls to have a feeling whether it 
might change in the future (and in such cases I'd have added a call to 
selinux_status_close() on proc_exit, just to be on the safe side).


* sepgsel_avs_unlabeled(). I was confused a bit because objects can have 
a label 'unlabeled', and the comments use wording like 'when unlabeled'. 
Does this mean with no label, or with a label 'unlabeled'?


* sepgsql_avc_compute(): the large comment in the beginning is hard to 
follow since sentences seem to have been a bit mixed up.


* question: why is ucontext stored in avc_cache?

* there is a new guc parameter for the user: avc_threshold, which is 
also documented. My initial question after reading the description was: 
what are the 'items' and how can I see current usage / what are numbers 
to expect? Without that knowledge any educated change of that parameter 
would be impossible. Would it be possible to remove this guc?


* avc_init has magic numbers 384, 4096. Maybe these can be defines (with 
a small comment what it is?)


* Finally, IMO the call sites, e.g. check_relation_priviliges, have 
improved in readability with this patch.


--

Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] [v9.2] Fix leaky-view problem, part 2

2011-07-20 Thread Yeb Havinga

On 2011-07-20 16:06, Noah Misch wrote:


The SQL-level semantics of the view define the access rules in question.  How
would you translate that into tests to apply at a lower level?
I assumed the leaky view thread was about row level security, not about 
access rules to views, since it was mentioned at the RLS wiki page for 
se-pgsql. Sorry for the confusion.


regards,
Yeb


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


Re: [HACKERS] [v9.2] Fix leaky-view problem, part 2

2011-07-20 Thread Yeb Havinga

On 2011-07-20 16:15, Yeb Havinga wrote:

On 2011-07-20 16:06, Noah Misch wrote:


The SQL-level semantics of the view define the access rules in 
question.  How

would you translate that into tests to apply at a lower level?
I assumed the leaky view thread was about row level security, not 
about access rules to views, since it was mentioned at the RLS wiki 
page for se-pgsql. Sorry for the confusion.
Had to digg a bit for the wiki, it was this one : 
http://wiki.postgresql.org/wiki/RLS#Issue:_A_leaky_VIEWs_for_RLS


regards,
Yeb


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


Re: [HACKERS] [v9.1] sepgsql - userspace access vector cache

2011-07-20 Thread Yeb Havinga

On 2011-07-20 16:54, Robert Haas wrote:


As to the first point, the other big problem with adding a syscache
here is that it could get really big.  I've worried about that issue
previously, and Yev's perplexity about where the memory is going is
not too reassuring.
Yeah I just realized that #buckets  cache items stored, which makes 
some of the comments I made a bit strange. If the syscaches store 
everything that's looked up then the same 5MB memory usage under 
changing #buckets makes perfect sense.


regards,
Yeb


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


Re: [HACKERS] [v9.1] sepgsql - userspace access vector cache

2011-07-19 Thread Yeb Havinga

On 2011-07-18 22:21, Kohei KaiGai wrote:

The Scientific Linux 6 is not suitable, because its libselinux version
is a bit older
than this patch expects (libselinux-2.0.99 or later).
My recommendation is Fedora 15, instead.

Installing right now, thanks for the heads up!


/etc/selinux/targeted/contexts/sepgsql_contexts:  line 33 has invalid object
type db_blobs

It is not an error, but just a notification to inform users that
sepgsql_contexts
file contains invalid lines. It is harmless, so we can ignore them.
I don't think sepgsql.sgml should mention about this noise, because it purely
come from the problem in libselinux and refpolicy; these are external packages
from viewpoint of PostgreSQL.
This is in contradiction with the current phrase in the documentation 
that's right after the sepgsql.sql loading: If the installation process 
completes without error, you can now start the server normally. IMHO if 
there are warnings that can be ignored, it would limit confusion for 
sepgsql users if the documentation would say it at this point, e.g. If 
the installation process completes without error, you can now start the 
server normally. Warnings from errors in sepgsql_contexts, a file 
external to PostgreSQL, are harmless and can be ignored.



The point of this patch is replacement of existing mechanism...
So, it is not necessary to define a new policy for testing.

Thanks for elaborating on this.

The security label is something like user-id or ownership/object-acl in the
default database access controls. It checks a relationship between user-id
and ownership/object-acl of the target object. If this relationship allowed
particular actions like 'select', 'update' or others, it shall be allowed when
user requires these actions.
In similar way, 'db_table:select' is a type of action; 'select' on table object,
not an identifier of user or objects.
SELinux defines a set of allowed actions (such as 'db_table:select') between
a particular pair of security labels (such as 'staff_t' and 'sepgsql_table_t').
The pg_seclabel holds only security label of object being referenced.
So, you should see /selinux/class/db_*/perms to see list of permissions
defined in the security policy (but limited number of them are in use, now).
The system's default security policy (selinux-policy package) defines all the
necessary labeles, and access control rules between them.
So, we never need to modify security policy to run regression test.

The sepgsql_trusted_proc_exec_t means that functions labeled with this label
is a trusted procedure. It switches security label of the user during
execution of
this function. It is a similar mechanism like SetExec or security
definer function.

The sepgsql_ro_table_t means 'read-only' tables that disallow any
writer operations
except for administrative domains.
You can define your own policy, however, I intend to run regression test
without any modification of the default security policy.


Thank you for this clarification. I have some ideas of things that if 
they were in the documentation they'd helped me. Instead of seeking 
agreement on each item, I propose that I gather documentation additions 
in a patch later after the review, and leave it up to you guys whether 
to include them or not.


regards,
Yeb
--

Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] [v9.1] sepgsql - userspace access vector cache

2011-07-19 Thread Yeb Havinga

On 2011-07-19 12:10, Kohei Kaigai wrote:


See the attached patch, that contains other 3 documentation updates.
I looked at the patch and the additions look good, though I didn't 
actually apply it yet.


thanks
Yeb


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


Re: [HACKERS] Commitfest Status: Sudden Death Overtime

2011-07-19 Thread Yeb Havinga

On 2011-07-18 21:59, Robert Haas wrote:

There are only two patches left and I think we really ought to try to
take a crack at doing something with them.  Yeb is working on the
userspace access vector cache patch, which I think is going drag on
longer than we want keep the CommitFest open, but I'm OK with giving
it a day or two to shake out.
At the end if this day I'm nearing the 'my head a splode' moment, which 
is more caused by trying to get my brain around selinux and it's 
postgresql policy, than the actual patch to review. I've verified that 
the patch works as indicated by KaiGai-san, but reading and 
understanding the code to say anything useful about it will take a few 
more hours, which will be tomorrow.


regards,
Yeb


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


Re: [HACKERS] [v9.1] sepgsql - userspace access vector cache

2011-07-19 Thread Yeb Havinga

On 2011-07-19 22:39, Heikki Linnakangas wrote:

On 19.07.2011 12:28, Yeb Havinga wrote:

On 2011-07-18 22:21, Kohei KaiGai wrote:

The Scientific Linux 6 is not suitable, because its libselinux version
is a bit older
than this patch expects (libselinux-2.0.99 or later).
My recommendation is Fedora 15, instead.

Installing right now, thanks for the heads up!


Would it be reasonable to #ifdefs the parts that require version 
2.0.99? That's very recent so might not be available on popular 
distributions for some time, so it would be nice to not have a hard 
dependency on it. You could have autoconf rules to check for the new 
functions, and only use them if they are available.


In contrary to the subject I was under the impression the current patch 
is for the 9.2 release since it is in a commitfest for the 9.2 release 
cycle, which would make the libselinux-2.0.99 dependency less of a problem.


--

Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] [v9.1] sepgsql - userspace access vector cache

2011-07-18 Thread Yeb Havinga

Hello KaiGai-san,

I've been preparing to review this patch by reading both pgsql-hackers 
history on sepgsql, and also the RHEL 6 guide on SELinux this weekend, 
today I installed GIT HEAD with --with-selinux on Scientific Linux 6, 
developer installation, so far almost everything looking good.


These things should probably be added to the 9.1beta3 documentation branch:

1) the line with for DBNAME in ... do postgres --single etc, lacks a -D 
argument and hence gives the error:

postgres does not know where to find the server configuration file.

2) there is a dependency to objects outside of the postgresql 
installation tree in /etc/selinux/targeted/contexts/sepgsql_contexts, 
and that file has an error that is thrown when contrib/sepgsql is executed:
/etc/selinux/targeted/contexts/sepgsql_contexts:  line 33 has invalid 
object type db_blobs

(same for db_language)
I found your fix for the error on a forum on oss.tresys.com, but IMHO 
either the contrib/sepgsql should mention that the dependency exists and 
it might contain errors for (older) reference policies, or it should 
include a bugfree reference policy for sepgsql to replace the system 
installed refpolicy with (and mention that in the install documentation).


3) sepgsql is currently a bit hard to find in the documentation. 
www.postgresql.org website search doesn't find sepgsql and selinux only 
refers to an old PostgreSQL redhat bug in 2005 on bugzilla.redhat.com. I 
had to manually remember it was a contrib module. Also sepgsql isn't 
linked to at the SECURITY LABEL page. At the moment I'm unsure if I have 
seen all sepgsql related sgml-based documentation.


After fixing the refpolicy I proceeded with the contrib/sepgsql manual, 
with the goal to get something easy done, like create a top secret table 
like 'thisyearsbonusses', and a single user 'boss' and configure sepgsql 
in such a way that only the boss can access the top secret table. I've 
read the the contrib documentation, browsed links on the bottom of the 
page but until now I don't even have a clue how to proceed. Until I do 
so, I don't feel it's appropriate for me to review the avc patch.


Would you be willing to help me getting a bit started? Specific 
questions are:


1) The contrib doc under DML permissions talks about 'db_table:select' 
etc? What are these things? They are not labels since I do not see them 
listed in the output of 'select distinct label from pg_seclabel'.


2) The regression test label.sql introduces labels with types 
sepgsql_trusted_proc_exec_t, sepgsql_ro_table_t. My question is: where 
are these defined? What is their meaning? Can I define my own?


3) In the examples so far I've seen unconfined_u and system_u? Can I 
define my own?


Thanks,
Yeb Havinga


On 2011-07-14 21:46, Kohei KaiGai wrote:

Sorry, the syscache part was mixed to contrib/sepgsql part
in my previous post.
Please see the attached revision.

Although its functionality is enough simple (it just reduces
number of system-call invocation), its performance
improvement is obvious.
So, I hope someone to volunteer to review these patches.

Thanks,

2011/7/11 Kohei KaiGaikai...@kaigai.gr.jp:

I rebased the userspace access vector cache patch to the latest tree.

I'll describe the background of this patch because this thread has not been
active more than a week.
The sepgsql asks in-kernel selinux when it needs to make its access control
decison, so it always causes system call invocations.
However, access control decision of selinux for a particular pair of security
label is consistent as long as its security policy is not reloaded.
Thus, it is a good idea to cache access control decisions recently used in
userspace.
In addition, current GetSecurityLabel() always open pg_seclabel catalog and
scan to fetch security label of database objects, although it is a situation we
can utilize syscache mechanism.

The uavc-syscache patch adds a new SECLABELOID syscache.
It also redefine pg_seclabel.provide as Name, instead of Text, according to
the suggestion from Tom.
(To avoid collation conscious datatype)

The uavc-selinux-cache patch adds cache mechanism of contrib/sepgsql.
Its internal api to communicate with selinux (sepgsql_check_perms) was
replaced by newer sepgsql_avc_check_perms that checks cached access
control decision at first, prior to system call invocations.

The result of performance improvement is obvious.

* Test 2. time to run 50,000 of SELECT from empty tables
  selinux | SECLABELOID syscache
  avc   |   without |   with
-+---
  without | 185.59[s] | 178.38[s]
-+---
  with|  23.58[s] |  21.79[s]
-+---

I strongly hope this patch (and security label support on shared objects) to
get unstreamed  in this commit-fest, because it will perform as a basis of
other upcoming features.
Please volunteer the reviewing!

Thanks,

2011/7/2 Kohei KaiGaikai...@kaigai.gr.jp:

The attached patch re-defines

Re: [HACKERS] [v9.1] sepgsql - userspace access vector cache

2011-07-16 Thread Yeb Havinga

On 2011-07-14 21:46, Kohei KaiGai wrote:

Sorry, the syscache part was mixed to contrib/sepgsql part
in my previous post.
Please see the attached revision.

Although its functionality is enough simple (it just reduces
number of system-call invocation), its performance
improvement is obvious.
So, I hope someone to volunteer to review these patches.


I will be able to look at this patch next week on monday and tuesday, 
without wanting to raise any expectations about that time being enough 
for me to say anything useful. On a longer timescale, I believe that 
sepgsql is one of the most important new features of PostgreSQL and that 
I want to commit myself to spend more community work on this subject.


regards,
Yeb

--

Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] Three patches which desperately need reviewers

2011-07-15 Thread Yeb Havinga

On 2011-07-14 02:42, Josh Berkus wrote:

The first two are difficult patches, but the other two are not.  Please
volunteer to give these patches a review; we owe it to our contributors
to review everything before the end of the CF.
When is the end of the CF? (I'm strongly suspecting today, but haven't 
been able to find it with a quick search)


regards,
Yeb


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


Re: [HACKERS] Parameterized aggregate subquery (was: Pull up aggregate subquery)

2011-07-09 Thread Yeb Havinga

On 2011-07-09 16:23, Hitoshi Harada wrote:

2011/7/5 Hitoshi Haradaumi.tan...@gmail.com:

2011/7/5 Yeb Havingayebhavi...@gmail.com:

Hello Hitosh, list,


Attached is revised version.

I failed to attached the patch. I'm trying again.


I'm currently unable to test, since on holiday. I'm happy to continue
testing once returned but it may not be within the bounds of the current
commitfest, sorry.

Thanks for replying. Regarding the time remained until the end of this
commitfest, I'd think we should mark this item Returned with
Feedback if no objection appears. I will be very happy if Yeb tests
my latest patch after he comes back. I'll make up my mind around
regression test.

It seems that Yeb marked Returned with Feedback. Thanks for the
review again. Still, I'd appreciate if further review is done on my
latest patch.


Yes, I did so after you suggested to return it to feedback. I'll review 
your latest patch as soon as there is enough time to do a proper review, 
which is probably after next week.


regards,
Yeb Havinga


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


Re: [HACKERS] Parameterized aggregate subquery (was: Pull up aggregate subquery)

2011-07-05 Thread Yeb Havinga
Hello Hitosh, list,


  Attached is revised version.

 I failed to attached the patch. I'm trying again.

 I'm currently unable to test, since on holiday. I'm happy to continue
testing once returned but it may not be within the bounds of the current
commitfest, sorry.


  5) Regression tests are missing; I think at this point they'd aid in
  speeding up development/test cycles.
 
  I'm still thinking about it. I can add complex test but the concept of
  regression test focuses on small pieces of simple cases. I don't want
  take pg_regress much more than before.


IMHO, at least one query where the new code is touched is a good idea.

 I get the same error with the version from june 9 with current git HEAD.
 
  Fixed. Some variable initializing was wrong.


Thanks - will test when I am back from holiday and other duties.

regards,
Yeb


Re: [HACKERS] Parameterized aggregate subquery (was: Pull up aggregate subquery)

2011-06-30 Thread Yeb Havinga

On 2011-06-29 19:22, Hitoshi Harada wrote:

Other things are all good points. Thanks for elaborate review!
More than anything, I'm going to fix the 6) issue, at least to find the cause.


Some more questions:
8) why are cheapest start path and cheapest total path in 
best_inner_subqueryscan the same?


9) as remarked up a different thread, more than one clause could be 
pushed down a subquery. The current patch only considers alternative 
paths that have at most one clause pushed down. Is this because of the 
call site of best_inner_subqueryscan, i.e. under one join rel? Would it 
be an idea to consider, for each subquery, only a single alternative 
plan that tries to have all suitable clauses pushed down?


10) I have a hard time imagining use cases that will actually result in 
a alternative plan, especially since not all subqueries are allowed to 
have quals pushed down into, and like Simon Riggs pointed out that many 
users will write queries like this with the subqueries pulled up. If it 
is the case that the subqueries that can't be pulled up have a large 
overlap with the ones that are not pushdown safe (limit, set operations 
etc), there might be little actual use cases for this patch.


I think the most important thing for this patch to go forward is to have 
a few examples, from which it's clear that the patch is beneficial.


regards,

--

Yeb Havinga
http://www.mgrid.net/
Mastering Medical 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: [HACKERS] Parameterized aggregate subquery

2011-06-30 Thread Yeb Havinga

On 2011-06-30 09:39, Yeb Havinga wrote:


9) as remarked up a different thread, more than one clause could be 
pushed down a subquery. The current patch only considers alternative 
paths that have at most one clause pushed down. Is this because of the 
call site of best_inner_subqueryscan, i.e. under one join rel? Would 
it be an idea to consider, for each subquery, only a single 
alternative plan that tries to have all suitable clauses pushed down?


Ehm, please forget this remark, I've mistaken join rel for base rel.

-- Yeb


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


  1   2   3   >