Re: [HACKERS] to_json(NULL) should to return JSON null instead NULL
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
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
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
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
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
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
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
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
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
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
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)
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
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?
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?
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
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
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?
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?
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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))
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))
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
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
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)
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)
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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)
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)
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)
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
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