Re: [HACKERS] SE-PostgreSQL Specifications

[KaiGai Kohei]

As Peter Eisentraut pointed out, we are not on the phase
to discuss about user documentations yet.

It is a reasonable idea to discuss correct specifications
of SE-PostgreSQL from the viewpoint of the developers.
Then, it will the a good source for the upcoming user docs.

For the recent a few days, I've worked to write and edit
the specification (partially copied from the draft of user
documentation) for the development purpose.

 http://wiki.postgresql.org/wiki/SEPostgreSQL_Development

I would like to have a discussion for its design specifications
and make clear anything unclear for pgsql folks (including the
quality of English).

Because this wikipage intends to provide the specifications
from the viewpoint of developers, it describes features more
detailed than the user visible documentation.

I intended to describe the features correctly, but it does not
mean perfect, needless to say.
Please feel free to ask / point out me, if something unclear or
not-easy understandable.

Thanks,


KaiGai Kohei wrote:
 Here is the initial draft of SE-PostgreSQL specifications:
 
   http://wiki.postgresql.org/wiki/SEPostgreSQL_Draft
 
 I've described it from the scratch again with paying attention
 for the people knowing nothing about SELinux.
 In some points, it uses comparison between the database privilege
 mechanism and SE-PostgreSQL for easy understanding.
 
 Please point out, if ...
 - Its composition can be improved.
 - Here is not enough introductions for what user wants to know.
 - Here is too much explanations, more brief one will be available.
 - Here is not easy understandable for database folks.
 - Here is not enough English quality.
 - And so on...
 
 In addition, I would like to fix its specifications during the discussion.
 
 Thanks,


-- 
OSS Platform Development Division, NEC
KaiGai Kohei kai...@ak.jp.nec.com

-- 
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] mixed, named notation support

[Pavel Stehule]

2009/7/31 Bernd Helmle maili...@oopsware.de:
 --On Montag, Juli 27, 2009 15:24:12 +0200 Bernd Helmle
 maili...@oopsware.de wrote:

 Hi,

 I sending a little bit modified version - I removed my forgotten
 comment in gram.y

 Thanks, i'll look on it asap.

 Looks good now.

 Here is a slightly edited reviewed patch version. I've edited the docs and
 fixed some spelling errors in a few error messages. It seems the patch
 mangled some source comments in namespace.c, i've rearranged them and tried
 to explain the purpose of VerifyCandidateNamedNotation(). I'm continuing
 reviewing this but can't get on it again until sunday.

 Pavel, can you have a look at the docs part of this patch and watch out for
 any gotchas?


Today, I'll look on it.

Pavel

 --
  Thanks

                   Bernd

-- 
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: make plpgsql IN args mutable (v1)

[Pavel Stehule]

2009/7/30 Steve Prentice prent...@cisco.com:
 Since I didn't get completely shot out of the water and a couple people
 seemed to think it was helpful, I'm submitting this patch for consideration
 in the next commitfest.

 This patch changes plpgsql IN parameters so they are mutable. Previously,
 they were being forced constant. This patch modifies the plpgsql.sql
 regression test and corresponding .out file. The regression test also makes
 sure the passed in parameter does not get changed in the calling function.

 I decided not to update the docs for this change because the docs don't
 currently indicate that an IN parameter is constant and I didn't want to
 encourage it because it isn't universally considered good programming
 practice to assign to an IN parameter. If others think we need a doc change
 for this, I'll update the patch.

 The following function will compile with this patch:

  create or replace function param_assign_test(a int, val int) returns void
 as $$
  begin
    a := val;
  end
  $$ language plpgsql;


This behave is in conflict with PL/SQL, what should do some problems.
I thing, so I understand well, why this behave is in PL/SQL. It hasn't
sense in plpgsql, because OUT and INOUT params has little bit
different syntax (calling) and nobody will do similar bugs (perhaps).
What is interesting - this behave is in conformity with SQL/PSM, where
parameters are mutable too.

I am for it. PL/pgSQL doesn't promise compatibility with PL/SQL and
this change should to help some beginners (and this limit is
artificial and unnecessary).

Regards
Pavel Stehule


 This function would have failed to compile previously.

 -Steve





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



-- 
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] [v8.5] Security checks on largeobjects

[KaiGai Kohei]

Nobody may remember, I also proposed a patch to support access
controls on largeobject.

It was suggested that largeobject stores its contents on the TOAST
relations and provides interfaces to read/write them partially.

For example:
 == lo_xxx() interfaces ==

 A new version of loread() and lowrite() are necessary to access
 a part of toasted data within user defined tables. It can be defined
 as follows:

   loread(Blob data, int32 offset, int32 length)
   lowrite(Blob data, int32 offset, Bytea data)

But, I found an another problem when we tried to update TOAST chunks
partially.

Because the toast pointer contains the total size of a series of
chunks, not only chunk_id and toastrelid, we cannot expand the size
of TOAST'ed data.

See below, toast pointer is defined as follows.

  struct varatt_external
  {
  int32   va_rawsize; /* Original data size (includes header) */
  int32   va_extsize; /* External saved size (doesn't) */
  Oid va_valueid; /* Unique ID of value within TOAST table */
  Oid va_toastrelid;  /* RelID of TOAST table containing it */
  };

These fields are inlined within the main tuple, and the contents
of TOAST datum is stored in the toast relation in separation.

If we can update the toast data partially with expanding its total size
(e.g, write 10,000 bytes from the offset 6,000 of the 12,000 bytes verlena),
it is also necessary to update the main tuple, because its va_rawsize and
va_extsize should be also updates, although va_valueid and va_toastrelid
can keep as is.

I wonder why the va_extsize and va_rawsize should be included within the
toast pointer. It is fundamentally computable using a pair of va_valueid
and va_toastrelid.
(I guess it is due to the performance reason. If we need to check whether
it is compressed or not, a flag varible can provide enough hint.)

So, we need to consider more how to implement partial writable verlena
on the user tables.

However, as far as existing largeobject interfaces, I believe it is possible
to implement them using partial writable verlena.
Because largeobject identifier is obvious when we user lo_*() interfaces,
so we can update the main tuple within pg_largeobject also, if the total
length of the verlena was changed on partial writes.

Thus, I would like to implement the feature with focusing on the existing
largeobject interfaces at the first step.
Partial writable TOAST in the user relation will come on the next.
(It also enables to keep the patch size smaller than all-in-a-patch.)

In summary, I try to implement it on the next commit fest.
- GRANT / REVOKE on largeobjects
- DAC access controls on largeobejcts
- It puts the contents of largeobject on TOAST relation.
  (including hole support)
- It provides largeobejct interfaces to write it partially.

Thanks,

KaiGai Kohei wrote:
 I summarized the design proposal and issues currently we have.
 
 I would like to see any comments corresponding to the proposition.
 Especially, selection of the snapshot is a headach issue for me.
 
 
 This project tries to solve two items listed at:
   http://wiki.postgresql.org/wiki/Todo#Binary_Data
 
  * Add security checks for large objects
  * Allow read/write into TOAST values like large objects
 
 = Introduction =
 
 We need to associate a metadata for a certain largeobject to
 implement security checks for largeobjects. However, the data
 structure of largeobjects are not suitable to manage its
 metadata (such as owner identifier, database acls ...) on
 a certain largeobject, because a largeobject is stored as
 separated page frames in the pg_largeobject system catalog.
 Thus, we need to revise the data structure to manage a certain
 largeobject.
 
 An interesting fact is a similarity of data structure between
 TOAST table and pg_lageobject.
 A TOAST relation is declared as follows:
   pg_toast_%u (
   chunk_idoid,
   chunk_seq   int4,
   chunk_data  bytea,
   unique(chunk_id, chunk_seq)
   )
 
 Definition of the pg_largeobject is as follows:
   pg_largeobject(
   loidoid,
   pageno  int4,
   databytea,
   unique(loid, pageno)
   )
 
 They have an identical data structure, so it is quite natural
 to utilize TOAST mechanism to store pagef rames of largeobject.
 
 = Design =
 
 In my plan, role of the pg_largeobject will be changed to
 manage metadata of largeobjects, and it will be redefined
 as follows:
 
   CATALOG(pg_largeobject,2613)
   {
   Oidloowner; /* OID of the owner */
   Oidlonsp;   /* OID of the namespace */
   aclitemloacl[1];/* ACL of the largeobject */
   Blob   lodata;  /* Contents of the largeobject */
   } FormData_pg_largeobejct;
 
 For access controls purpose, its ownership, namespace and ACLs
 are necessary. In addition, the Blob is a new type which support
 to read/write a its partial TOAST value.
 
 The current lo_xxx() 

Re: [HACKERS] ECPG support for struct in INTO list

[Boszormenyi Zoltan]

Michael Meskes írta:
 On Fri, Jul 17, 2009 at 03:58:21PM +0200, Boszormenyi Zoltan wrote:
   
 Attached is the short example I can reproduce with.
 The version I used was final PostgreSQL 8.4.0, without our
 extensions posted already. I added an indication to ecpg_type_name():

 [z...@db00 ecpg-test]$ ecpg -C INFORMIX test28.pgc
 ...
 But you are right about the supposed to work part,
 if I modify it the way below, it works:
 

 Thanks for spotting this. It appears to be a bug in Informix compatibility
 mode. Without -C INFORMIX it works nicely. Thus I guess we have to find and 
 fix
 the bug. Your patch probably/hopefully is not needed.

 Michael
   

My previous patch on this broke make check in ecpg,
so ignore that. Your comment that it's a bug in Informix-mode
made me look around more. Find the attached patch I came up with.
Now my previous test code works and produces similar C code
as without -C INFORMIX. Can it be this simple?
Can you see anything wrong with this approach?

Best regards,
Zoltán Böszörményi

-- 
Bible has answers for everything. Proof:
But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil. (Matthew 5:37) - basics of digital technology.
May your kingdom come - superficial description of plate tectonics

--
Zoltán Böszörményi
Cybertec Schönig  Schönig GmbH
http://www.postgresql.at/

diff -dcrpN pgsql85dev.4string/src/interfaces/ecpg/preproc/ecpg.header pgsql85dev.5struct/src/interfaces/ecpg/preproc/ecpg.header
*** pgsql85dev.4string/src/interfaces/ecpg/preproc/ecpg.header	2009-07-15 11:19:06.0 +0200
--- pgsql85dev.5struct/src/interfaces/ecpg/preproc/ecpg.header	2009-07-31 11:38:50.0 +0200
*** adjust_informix(struct arguments *list)
*** 266,271 
--- 266,273 
  			ptr-variable = new_variable(cat_str(4, make_str((), mm_strdup(ecpg_type_name(ptr-variable-type-type)), make_str( *)(ECPG_informix_get_var(), mm_strdup(temp)), ECPGmake_simple_type(ptr-variable-type-type, ptr-variable-type-size, ptr-variable-type-lineno), 0);
  			sprintf(temp, %d, (, ecpg_informix_var++);
  		}
+ 		else if (ptr-variable-type-type == ECPGt_struct || ptr-variable-type-type == ECPGt_union)
+ 			continue;
  		else
  		{
  			ptr-variable = new_variable(cat_str(4, make_str(*(), mm_strdup(ecpg_type_name(ptr-variable-type-type)), make_str( *)(ECPG_informix_get_var(), mm_strdup(temp)), ECPGmake_simple_type(ptr-variable-type-type, ptr-variable-type-size, ptr-variable-type-lineno), 0);

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

[HACKERS] More thoughts on sorting

[PFC]

There was a thread some time ago about sorting... it kind of died...
I did some tests on a desktop (Postgres 8.3.7, kubuntu, Core 2 dual core,  
4GB RAM, RAID1 of 2 SATA disks)


Quick conclusions :

- grabbing the stuff to sort can be IO bound of course (not here)
- for short strings (average 12 bytes), sort is CPU-bound in strcoll()
- for longer strings (average 120 bytes), sort is even more CPU-bound in  
strcoll()
- strcoll() time seems to depend on the length of the strings, not the  
place where a difference occurs (grokking glibc source code confirms)


See detailed test procedure below.

Locale is fr_FR.UTF-8 and database is UNICODE
All strings are ASCII, they are mostly alphanumeric references.
There are 391469 strings.
min length 6 chars
max length 80 chars
avg length 11.82 chars

We have a table test with (id INTEGER PRIMARY KEY, TEXT, BYTEA ), and  
contents of TEXT and BYTEA are identical.
We have a table test2 which contains the same thing as test, except the id  
and a 100-character constant are appended to the strings to make them  
longer.



Test Procedure :

Grab test data from :
http://home.peufeu.com/pg/test_data_references.copy.gz

 Sorting with Python

Sorting all string converted to unicode (from utf8) using strcoll() and  
correct locale

= 5.8 s

With longer strings (as in table test2 below )
= 8 s


 Postgres

To get query timings, I use \t and SELECT * FROM test ORDER BY id OFFSET
391468; which avoids EXPLAIN ANALYZE overhead, it just prints the last  
row from the results. Timings are a bit shorter than EXPLAIN ANALYZE  
gives, and I checked the plans, they are all sorts.


-- Create test table and load it
BEGIN;
CREATE TABLE test1 (t TEXT NOT NULL);
\copy test1 FROM test_data_references.copy
CREATE TABLE test (id SERIAL PRIMARY KEY, t TEXT NOT NULL, b BYTEA NOT
NULL );
INSERT INTO test (t,b) SELECT t,t::BYTEA FROM test1;
DROP TABLE test1;
ALTER TABLE test DROP CONSTRAINT test_pkey;
CREATE TABLE test2 (id INTEGER NOT NULL, t TEXT NOT NULL, b BYTEA NOT NULL
);
INSERT INTO test2 SELECT id,
(t || id || 'This is a dummy text of length 100  
bytes') AS t,
(t || id || 'This is a dummy text of length 100  
bytes')::BYTEA

AS b
 FROM test;
COMMIT;

\d test

SHOW work_mem;
-- 16MB
SHOW maintenance_work_mem;
-- 512MB

\timing
-- cache it really well
SELECT count(*) FROM test;
SELECT count(*) FROM test;
SELECT count(*) FROM test;
-- 391469
-- Temps : 87,033 ms

SELECT * FROM test ORDER BY id OFFSET 391468;
-- Temps : 918,893 ms

SELECT id FROM test ORDER BY id OFFSET 391468;
-- Temps : 948,015 ms

Interpretation :
- Time for hauling around extra data (SELECT * instead of SELECT id) is  
not significant.
- Sorting by integers is quite fast (not THAT fast though, MySQL below is  
3x faster when selecting just 'id' and 2x slower when SELECT *, hum.)


SELECT * FROM test ORDER BY b OFFSET 391468;
-- Temps : 2145,555 ms

SELECT id FROM test ORDER BY b OFFSET 391468;
-- Temps : 2152,273 ms

Interpretation :
- Time for hauling around extra data (SELECT * instead of SELECT id) is  
not significant.
- Sorting by BYTEA is just a memcmp(), it is strange that is it 2x slower  
than ints. Probably the varlena stuff, I guess.

- See ridiculous MySQL results using a BLOB below which are 10x slower

SELECT * FROM test ORDER BY t OFFSET 391468;
-- Temps : 7305,373 ms

SELECT id FROM test ORDER BY t OFFSET 391468;
-- Temps : 7345,234 ms

Interpretation :
- Time for hauling around extra data (SELECT * instead of SELECT id) is  
not significant.

- Sorting localized TEXT really is SLOW !
- The little test above calling strcoll() from Python confirms the  
slowness is in strcoll()
- MySQL (see below) seems to be much faster (about equal to postgres) on  
VARCHAR, and 2x slower on TEXT (hum...)


BEGIN;
CREATE INDEX test_id ON test( id );
-- Temps : 555,718 ms

CREATE INDEX test_b ON test( b );
-- Temps : 1762,263 ms

CREATE INDEX test_t ON test( t );
-- Temps : 6274,624 ms

ROLLBACK;

Interpretation :
- maintenance_work_mem is much higher than work_mem so the sorts are  
faster, but the slowness in localized text sorting subsists...



SELECT count(*) FROM test2;
-- 391469
-- Temps : 114,669 ms

SELECT * FROM test2 ORDER BY id OFFSET 391468;
-- Temps : 1788,246 ms

SELECT id FROM test2 ORDER BY id OFFSET 391468;
-- Temps : 989,238 ms

Interpretation :
- Time for hauling around extra data (SELECT * instead of SELECT id) IS  
significant this time due to the extra string lengths.


SELECT * FROM test2 ORDER BY b OFFSET 391468;
-- Temps : 2906,108 ms

SELECT id FROM test2 ORDER BY b OFFSET 391468;
-- Temps : 2554,931 ms

SELECT * FROM test2 ORDER BY t OFFSET 391468;
-- Temps : 10637,649 ms

SELECT id FROM test2 ORDER BY t OFFSET 391468;
-- Temps : 10322,480 ms

Interpretation :
- Note : the strings are longer, however they are sortable only by looking  
at 

Re: [HACKERS] mixed, named notation support

[Pavel Stehule]

2009/7/31 Bernd Helmle maili...@oopsware.de:
 --On Montag, Juli 27, 2009 15:24:12 +0200 Bernd Helmle
 maili...@oopsware.de wrote:

 Hi,

 I sending a little bit modified version - I removed my forgotten
 comment in gram.y

 Thanks, i'll look on it asap.

 Looks good now.

 Here is a slightly edited reviewed patch version. I've edited the docs and
 fixed some spelling errors in a few error messages. It seems the patch
 mangled some source comments in namespace.c, i've rearranged them and tried
 to explain the purpose of VerifyCandidateNamedNotation(). I'm continuing
 reviewing this but can't get on it again until sunday.

 Pavel, can you have a look at the docs part of this patch and watch out for
 any gotchas?


I looked there and I thing it's very good. I have only one idea about
samples in docs. Mathematical function isn't too much readable. Maybe
some string concation or returning record should be more readable.

create or replace function foo(a varchar, b varchar, uppercase boolean = false)
returns varchar as $$
  select when uppercase then 'a=' || a ', b=' || b
   else upper('a=' || a ', b=' || b) end;
$$ language sql immutable strict;

select foo('Hello','World');
select foo('Hello', World', true);
select foo('Hello' as a, 'World' as b);
select foo('Hello', 'World', true as uppercase);
select foo(true as uppercase, 'World' as b, 'Hello' as b);

or some similar

Thank You very much
Pavel

 --
  Thanks

                   Bernd

-- 
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] 8.4 win32 shared memory patch

[Magnus Hagander]

On Fri, Jul 31, 2009 at 14:41, Kevin Fieldkevinjamesfi...@gmail.com wrote:
 On Wed, Jul 29, 2009 at 19:52, Kevin Fieldkevinjamesfi...@gmail.com
 wrote:
  On Win2k3 Std SP2, the service won't start once I've applied the
  patch.  In the log, I get:
 
  %t LOG:  CreateProcess call failed: A blocking operation was
  interrupted by a call to WSACancelBlockingCall.

 Now, that's just strange :-O

 First of all, the code from this patch hasn't even executed when that
 error pops up... So it really shouldn't be that one. Second, that
 error message just makes no sense in the context. However, the
 errorcode 2 makes a bit more sense - that's a file not found error.
 It's still strange that it would show up, but it makes sense in the
 context of CreateProcess(). Are you sure that there weren't any old
 processes hanging around when you tried it? Would it be possible for
 you to test by doing stop - reboot - replace file - start and see
 if the issue remains?

 I did this last night as requested.  This time, nothing about
 CreateProcess, perhaps it's a separate issue.  All the log said this
 time was:

 %t LOG:  received fast shutdown request
 %t LOG:  aborting any active transactions
 %t LOG:  autovacuum launcher shutting down
 %t LOG:  shutting down
 %t LOG:  database system is shut down

 That's the entire file.  Attempting to start the service, I almost
 immediately get an error 1067, the process terminated unexpectedly.

If there is nothing in the logfile (make sure you're looking at the
correct one - it may have created a new one), check the Windows
Eventlog. That's where we'll put data if the issues show up before we
have started the logger.


-- 
 Magnus Hagander
 Self: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

-- 
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] SE-PostgreSQL Specifications

[Stephen Frost]

KaiGai,

* KaiGai Kohei (kai...@ak.jp.nec.com) wrote:
 For the recent a few days, I've worked to write and edit
 the specification (partially copied from the draft of user
 documentation) for the development purpose.
 
  http://wiki.postgresql.org/wiki/SEPostgreSQL_Development

Thanks for doing this.  I've taken a quick glance through it and will
review it more later today.  The next step, I believe, is to document
the changes which need to be made to PG, at a high level, to support
this specification.  

I think the initial goal should be to make changes mainly to aclchk.c to
add the SELinux hooks.  I'd like to hear from some committers about this
idea: I think we should also be looking to move more of the permissions
checking which is done in other parts of the code (eg:
ATExecChangeOwner()) into aclchk.c, which is where I would argue it
belongs.  Doing so will make it much easier to add other hooks, should
the need arise, and would minimize the amount of code 'touched' to add
SELinux support.

Strategy for code changes:
  Patch #1: Move permissions checks currently implemented in other parts
of the code (eg: tablecmds.c:ATExecChangeOwner()) into
aclchk.c.
  Patch #2: Change the aclchk.c function APIs, if necessary, to add
additional information required for SELinux (eg: the 'old'
owner).
  Patch #3: Add SELinux hooks into aclchk.c functions.

This initial round, again, should focus on just those
controls/permissions which PostgreSQL already supports.  At this time,
do not stress over finding every if(superuser()) and moving it to
aclchk.c.  Let's deal with the clear situations, such as tablecmds.c
lines 6322-6342 (that entire block, including the if(!superuser()),
should be ripped out and made a function in aclchk.c which is called
with necessary args).  We can go back and clean up the other places
where we have explicit superuser() checks later, if necessary.

I've finally had a chance to look through the last set of proposed
patches some, and I've noticed some other issues that need to be
addressed:

- Let's avoid the changes to heaptuple.c for now.  That's really for
  much later down the road when we implement row-level security.
- I would expect the dependency system to be used to handle deleting
  things from pg_security, etc, when an object has been deleted (eg: a
  table was dropped).
- Conversly, when new entries need to be added to pg_security, they
  should be done at the same level as other items being added to the
  catalog.  In places like createdb(), where it's all one big function,
  I would recommend splitting out the existing catalog update into a
  separate function, which then makes it much clearer that the code is
  doing: update pg_database table, update pg_security table, update
  pg_dependency table.  That should be done as a separate patch, of
  course.  Remember, we're trying to make small incremental changes that
  make sense by themselves but at the same time will reduce the effort
  required to add SELinux later.
- In terms of cacheing the results, is there any way we could use
  SysCache for this?  And just handle the cacheing in the hooks, etc,
  from aclchk.c?  I dislike having the security labels added to
  every tuple until we actually implement row level security.  It adds
  alot of unnecessary complexity to the initial implementation.
  Note that I'm referring to the results from the kernel/syscalls, I
  realize you're using SysCache properly for the entries in pg_security
  already, and that's good.

Guess that's a start on the implementation design, which I feel is the
next step after specification.  Perhaps you could start a wiki page on
it which includes my comments?  I'm in meetings for the next couple of
hours, but will resume looking at this after lunch, US/Eastern time.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] More thoughts on sorting

[Tom Lane]

PFC li...@peufeu.com writes:
 - for short strings (average 12 bytes), sort is CPU-bound in strcoll()
 - for longer strings (average 120 bytes), sort is even more CPU-bound in  
 strcoll()

No news there.  If you are limited by the speed of text comparisons,
consider using C locale.

regards, tom lane

-- 
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] ALTER TABLE ... ALTER COLUMN ... SET DISTINCT

[Dimitri Fontaine]

Hi,

Finally some update on the patch.

Le 18 juil. 09 à 20:55, Robert Haas a écrit :

This is one of the things that I hate about the requirement to post
context diffs: filterdiff, at least for me, strips out the git tags
that indicate the base rev of the patch.


Yes, and as I didn't have the time to install filterdiff I've attached  
a revision of your patch in git format which adresses the problem I  
mentioned, in a tarball also containing raw notes, tests, and  
regression.{out,diffs}.


mqke check is failing on opr_sanity test in what looks like an  
ordering issue, but I didn't feel confident enough to adapt the .out  
to force the regression into passing.



 - style issue, convert at PQgetvalue() time


Ah, good catch.


Fixed in the updated version, which uses a float4 variable in pg_dump  
and strtod() rather than atof. This last point is maybe overkill as  
I'm using:
	tbinfo-attdistinct[j] = strtod(PQgetvalue(res, j, i_attdistinct),  
(char **)NULL);


 What about adding the following before the switch, to do like  
surrounding

code?
   Assert(IsA(newValue, Integer) || IsA(newValue, Float));


Not a good plan.  In my experience, gcc doesn't like switch ()
statements over enums that don't list all the values, unless they have
a default clause; it masterminds by giving you a warning that you've
inadvertently left out some values.


I've left this part alone but still wonders about this, which is a new  
user visible error message kind:

default:
elog(ERROR, unrecognized node type: %d,
 (int) nodeTag(newValue));

Given your revised version I'll try to play around with ndistinct  
behavior

and exercise dump and restore, etc, but for now I'll pause my work.


I failed to have 0 to allow for analyze to compute a value again, as  
shown in the raw notes in attachement:


dim=# alter table foo alter column x set distinct 1;
ALTER TABLE
...
dim=# alter table foo alter column x set distinct 0;
ALTER TABLE
dim=# analyze verbose foo;
INFO:  analyzing public.foo
INFO:  foo: scanned 4 of 4 pages, containing 1000 live rows and 0  
dead rows; 1000 rows in sample, 1000 estimated total rows

ANALYZE
dim=# select attname, attdistinct from pg_attribute where attrelid =  
'foo'::regclass and attname = 'x';

 attname | attdistinct
-+-
 x   |   0
(1 row)

What I understand from the doc part of your work contradicts what I  
see here...


Regards,
--
dim

Will mark as Waiting on Author, as I need Robert to tell me what to do  
next.




ndistinct.tgz
Description: Binary data






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

Re: [HACKERS] Review: Revise parallel pg_restore's scheduling heuristic

[Tom Lane]

Kevin Grittner kevin.gritt...@wicourts.gov writes:
 Rebased to correct for pg_indent changes.

Thanks for doing that.  Attached is a further small improvement that
gets rid of the find_ready_items() scans.  After re-reading the patch
I realized that it wasn't *really* avoiding O(N^2) behavior ... but
this version does.

regards, tom lane



binIBkZHijFgU.bin
Description: alternate-parallel-restore-3.patch.gz

-- 
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] SE-PostgreSQL Specifications

[KaiGai Kohei]

Stephen Frost wrote:
 KaiGai,
 
 * KaiGai Kohei (kai...@ak.jp.nec.com) wrote:
 For the recent a few days, I've worked to write and edit
 the specification (partially copied from the draft of user
 documentation) for the development purpose.

  http://wiki.postgresql.org/wiki/SEPostgreSQL_Development
 
 Thanks for doing this.  I've taken a quick glance through it and will
 review it more later today.  The next step, I believe, is to document
 the changes which need to be made to PG, at a high level, to support
 this specification.  

Thanks for your help.

I also think it is necessary to describe what changes are needed on
the core pgsql except for security routines.

It is just an example:
http://wiki.postgresql.org/wiki/SEPostgreSQL_Development#security_hooks_.28example.29

I believe such kind of specifications are necessary at the next.

 I think the initial goal should be to make changes mainly to aclchk.c to
 add the SELinux hooks.  I'd like to hear from some committers about this
 idea: I think we should also be looking to move more of the permissions
 checking which is done in other parts of the code (eg:
 ATExecChangeOwner()) into aclchk.c, which is where I would argue it
 belongs.  Doing so will make it much easier to add other hooks, should
 the need arise, and would minimize the amount of code 'touched' to add
 SELinux support.
 
 Strategy for code changes:
   Patch #1: Move permissions checks currently implemented in other parts
 of the code (eg: tablecmds.c:ATExecChangeOwner()) into
   aclchk.c.
   Patch #2: Change the aclchk.c function APIs, if necessary, to add
 additional information required for SELinux (eg: the 'old'
   owner).
   Patch #3: Add SELinux hooks into aclchk.c functions.

Thanks for your idea. However, it seems to me this approach eventually
requires bigger changeset than what the current security hooks doing.
In addition, it cannot solve the differences in behavior due to the
security model on which they stand.

For example, when we create a new table, the native database privilege
mechanism checks its permission on the schema object which the new
table blongs to. On the other hand, SELinux requires to check
db_table:{create} permission on the newly created table itself.

In my understanding, the purpose of the design specification is to make
clear for database folks what the security hooks doin and why the security
hooks are deployed here.

Linux kernel is a good case in success.
Needless to say, most of kernel developer are not security experts.
But they defined an explicit specification of the security hooks called
LSM, and put these hooks on the strategic points in the kernel.
(The very long source code comments describes what is the purpose of this
security hook, what arguments should be given and so on for each security
hooks.)

 This initial round, again, should focus on just those
 controls/permissions which PostgreSQL already supports.  At this time,
 do not stress over finding every if(superuser()) and moving it to
 aclchk.c.  Let's deal with the clear situations, such as tablecmds.c
 lines 6322-6342 (that entire block, including the if(!superuser()),
 should be ripped out and made a function in aclchk.c which is called
 with necessary args).  We can go back and clean up the other places
 where we have explicit superuser() checks later, if necessary.

At least, it works correctly as far as the native database privilege
mechanism. If the specification of the security hooks is clear, I think
the following manner is better than modifying DAC mechanism.

  if (!superuser())
  {
  if (pg_xxx_aclcheck() != ACLCHECK_OK)
  aclcheck_error();
  }
+ sepgsqlCheck();

 I've finally had a chance to look through the last set of proposed
 patches some, and I've noticed some other issues that need to be
 addressed:
 
 - Let's avoid the changes to heaptuple.c for now.  That's really for
   much later down the road when we implement row-level security.

Yes, I can agree to postpone pg_security and corresponding facilities
until row-level security.

 - I would expect the dependency system to be used to handle deleting
   things from pg_security, etc, when an object has been deleted (eg: a
   table was dropped).

In the patch for v8.4.x series (with fullset functionality), it is
already implemented. Any entries within pg_security referenced by the
dropped table is reclaimed, when we drop a table.
The matter of orphan entries are already solved.

 - Conversly, when new entries need to be added to pg_security, they
   should be done at the same level as other items being added to the
   catalog.  In places like createdb(), where it's all one big function,
   I would recommend splitting out the existing catalog update into a
   separate function, which then makes it much clearer that the code is
   doing: update pg_database table, update pg_security table, update
   pg_dependency table.  That should be 

[HACKERS] Occasional failures on buildfarm member eukaryote

[Tom Lane]

I've noticed that every so often eukaryote reports a regression failure
with just this one diff:

*** 
/data/markwkm/local/pgfarmbuild-cell/HEAD/pgsql.4654/src/test/regress/expected/plpgsql.out
  Fri Jul 31 04:00:51 2009
--- 
/data/markwkm/local/pgfarmbuild-cell/HEAD/pgsql.4654/src/test/regress/results/plpgsql.out
   Fri Jul 31 04:22:38 2009
***
*** 2041,2046 
--- 2041,2047 
  (1 row)
  
  reset statement_timeout;
+ ERROR:  canceling statement due to statement timeout
  select * from foo;
   f1 
  

The latest example is at
http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=eukaryotedt=2009-07-31%2004:00:02
but there are quite a few earlier cases.  It's repeatable enough that
I think we should try to fix it, if only to reduce noise in the
buildfarm reports.

One possibility is that eukaryote is just remarkably heavily loaded
and increasing the statement_timeout value being used in this test
(currently 2 seconds) would make the failure go away.  I'm not sure
I believe this theory though, because there are lots of fairly slow
machines in the buildfarm, but eukaryote seems to be the only one
that's showing this type of failure.

If it's not that, I think this must indicate some weird
platform-specific issue with timeout handling; but I can't think what,
since it seems to be running stock Fedora 8.

Thoughts?  Can you make this machine available for closer examination?

regards, tom lane

-- 
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] Using results from INSERT ... RETURNING

[Marko Tiikkaja]

On 7/19/2009, Tom Lane t...@sss.pgh.pa.us wrote:
 The way that I think this should be approached is
 (1) a code-refactoring patch that moves INSERT/UPDATE/DELETE control
 into plan nodes; then
 (2) a feature patch that makes use of that to expose RETURNING in CTEs.

I've been working on this and here's a patch I came up with. It's a
WIP that tries to
put together my thoughts about this. It works only for INSERT and DELETE
and
probably breaks with triggers.

In the attached patch, an Append node isn't added for inheritance sets.
Instead,
the UPDATE/DELETE node tries to take care of choosing the correct result
relation.
I tried to keep estate-es_result_relations as it is in order not to
break anything that
relies on it (for example ExecRelationIsTargetRelation) but
estate-es_result_relation_info
doesn't point to the target relation of the DML node any more. This was
replaced with
a pointer in DmlState to make it possible to add more DML nodes in the
future. Also
the result relation info initialization was moved to the node, because
InitResultRelInfo
needs to know the type of operation that is going to be performed on the
result relation.
Currently that info isn't available at the top level, so I went this
way. I'm not happy with it,
but couldn't come up with better ideas. Currently the result relation
for SELECT FOR
UPDATE/SHARE isn't initialized anywhere, so that won't work.

The patch doesn't do this, but the idea was that if the DML node has a
RETURNING
clause, the node returns the projected tuple and ExecutePlan sends it to
the DestReceiver.
In cases where there is no RETURNING clause, the node would return a
dummy tuple.

Comments are welcome.

Regards,
Marko Tiikkaja

-- 
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] Using results from INSERT ... RETURNING

[Marko Tiikkaja]

On 7/31/2009, Marko Tiikkaja marko.tiikk...@cs.helsinki.fi wrote:
 ..

I seem to be having problems with my email client. The patch should be
attached this time. Sorry for the noise.

Regards,
Marko Tiikkaja


patch3
Description: Binary data

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

Re: [HACKERS] Review: Revise parallel pg_restore's scheduling heuristic

[Kevin Grittner]

Tom Lane t...@sss.pgh.pa.us wrote: 
 Kevin Grittner kevin.gritt...@wicourts.gov writes:
 Rebased to correct for pg_indent changes.
 
 Thanks for doing that.
 
No problem.  I think I still owe you a few.  :-)
 
 Attached is a further small improvement that gets rid of the
 find_ready_items() scans.  After re-reading the patch I realized
 that it wasn't *really* avoiding O(N^2) behavior ... but this
 version does.
 
I'll run a fresh set of benchmarks.
 
By the way, I muffed the setup of last night's benchmarks, so no new
information there, except that in the process of reviewing the attempt
I discovered I was guilty of making a false assertion yesterday, based
on remembering incorrectly.  The logs show that the six hour dump to
custom format was over the LAN.  :-(  I hope I didn't waste too much
of people's time by saying otherwise.  I'll try to get some numbers on
the same-box dump soon.  (Note to self: never, ever trust your memory;
always confirm.)
 
-Kevin

-- 
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] Revised signal multiplexer patch

[Tom Lane]

Fujii Masao masao.fu...@gmail.com writes:
 But there is one issue; the extra search is always required to send a notify
 interrupt. This is because pg_listener doesn't have a backend ID and we
 cannot pass it to SendProcSignal. In order to solve this issue, we should
 newly add backend ID field into pg_listener?

Hmm.  I'm not tremendously concerned about that --- the LISTEN/NOTIFY
code has been on the agenda for a complete rewrite for a long time now,
and I keep hoping pg_listener will go away entirely sometime soon.
I don't feel a need to go and fix a marginal performance issue there.

regards, tom lane

-- 
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] 8.4 win32 shared memory patch

[Magnus Hagander]

On Fri, Jul 31, 2009 at 19:29, Kevin Fieldkevinjamesfi...@gmail.com wrote:
      %t LOG:  received fast shutdown request
      %t LOG:  aborting any active transactions
      %t LOG:  autovacuum launcher shutting down
      %t LOG:  shutting down
      %t LOG:  database system is shut down
     
      That's the entire file.  Attempting to start the service, I
  almost
      immediately get an error 1067, the process terminated
  unexpectedly.

     If there is nothing in the logfile (make sure you're looking at
 the
     correct one - it may have created a new one), check the Windows
     Eventlog. That's where we'll put data if the issues show up
 before we
     have started the logger.


 The event viewer says:

 The description for Event ID ( 0 ) in Source ( PostgreSQL ) cannot be
 found. The local computer may not have the necessary registry
 information or message DLL files to display messages from a remote
 computer. You may be able to use the /AUXSOURCE= flag to retrieve this
 description; see Help and Support for details. The following
 information is part of the event: pg_ctl: could not find postgres
 program executable

 And yes, I renamed it correctly...

Check permissions on it. If you moved it at some point, it may have
the wrong permissions. They should be the same as for the other .EXEs
in that directory.

-- 
 Magnus Hagander
 Self: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

-- 
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] 8.4 win32 shared memory patch

[Kevin Field]

 On Wed, Jul 29, 2009 at 19:52, Kevin Fieldkevinjamesfi...@gmail.com
 wrote:
  On Win2k3 Std SP2, the service won't start once I've applied the
  patch.  In the log, I get:
 
  %t LOG:  CreateProcess call failed: A blocking operation was
  interrupted by a call to WSACancelBlockingCall.

 Now, that's just strange :-O

 First of all, the code from this patch hasn't even executed when that
 error pops up... So it really shouldn't be that one. Second, that
 error message just makes no sense in the context. However, the
 errorcode 2 makes a bit more sense - that's a file not found error.
 It's still strange that it would show up, but it makes sense in the
 context of CreateProcess(). Are you sure that there weren't any old
 processes hanging around when you tried it? Would it be possible for
 you to test by doing stop - reboot - replace file - start and see
 if the issue remains?

I did this last night as requested.  This time, nothing about
CreateProcess, perhaps it's a separate issue.  All the log said this
time was:

%t LOG:  received fast shutdown request
%t LOG:  aborting any active transactions
%t LOG:  autovacuum launcher shutting down
%t LOG:  shutting down
%t LOG:  database system is shut down

That's the entire file.  Attempting to start the service, I almost
immediately get an error 1067, the process terminated unexpectedly.

Kev

-- 
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] 8.4 win32 shared memory patch

[Kevin Field]

  %t LOG:  received fast shutdown request
  %t LOG:  aborting any active transactions
  %t LOG:  autovacuum launcher shutting down
  %t LOG:  shutting down
  %t LOG:  database system is shut down
 
  That's the entire file.  Attempting to start the service, I 
  almost
  immediately get an error 1067, the process terminated 
  unexpectedly.
 
 If there is nothing in the logfile (make sure you're looking at 
 the
 correct one - it may have created a new one), check the Windows
 Eventlog. That's where we'll put data if the issues show up 
 before we
 have started the logger.


The event viewer says:

The description for Event ID ( 0 ) in Source ( PostgreSQL ) cannot be 
found. The local computer may not have the necessary registry 
information or message DLL files to display messages from a remote 
computer. You may be able to use the /AUXSOURCE= flag to retrieve this 
description; see Help and Support for details. The following 
information is part of the event: pg_ctl: could not find postgres 
program executable

And yes, I renamed it correctly...

-- 
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: Revise parallel pg_restore's scheduling heuristic

[daveg]

On Thu, Jul 30, 2009 at 12:29:34PM -0500, Kevin Grittner wrote:
 Tom Lane t...@sss.pgh.pa.us wrote: 
  
  I think we've pretty much established that it doesn't make things
  *worse*, so I'm sort of inclined to go ahead and apply it.  The
  theoretical advantage of eliminating O(N^2) search behavior seems
  like reason enough, even if it takes a ridiculous number of tables
  for that to become significant.
  
 Agreed, although I'm having some concerns about whether this should
 proceed based exclusively on my benchmarks.  On a thread on the
 performance list, people are talking about restores which go several
 times faster with parallel restore (compared to a single job).  On my
 hardware, I haven't even gotten it to run twice as fast.  This means
 that parallel restore is not a good fit for servers like we have, at
 least with databases like we have, which means it's probably a poor
 environment to get benchmarks for this patch.  :-(
  
 Can we get someone who has benchmarks showing parallel restore to be
 eight times the speed of a single job to benchmark with this patch,
 just for confirmation?

I have a couple spare 32GB 4 core and 64GB 8 core servers with 15 scsi/sas
drives and dumps of production dbs in the 100GB to 500 GB range. These have
several hundred tables most with an index or few and an fkey or too many.

It will take a couple days to run a variety of tests I suppose, and I will
be away starting mid next week, but maybe I could get some done before I go.

Will the patch apply to a vanilla 8.4.0?


-dg

-- 
David Gould   da...@sonic.net  510 536 1443510 282 0869
If simplicity worked, the world would be overrun with insects.

-- 
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] Revised signal multiplexer patch

[Tom Lane]

Fujii Masao masao.fu...@gmail.com writes:
 I revised the patch according to the suggestion.

Applied with some mostly-cosmetic editorial work.

regards, tom lane

-- 
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: Revise parallel pg_restore's scheduling heuristic

[Tom Lane]

daveg da...@sonic.net writes:
 Will the patch apply to a vanilla 8.4.0?

Yeah, it should.  The line numbers in the version I just posted might
be off a little bit for 8.4.0, but patch should cope.

Be sure to make clean and recompile all of src/bin/pg_dump, else you
might have some issues.

regards, tom lane

-- 
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] SE-PostgreSQL Specifications

[Stephen Frost]

KaiGai,

* KaiGai Kohei (kai...@kaigai.gr.jp) wrote:
 Stephen Frost wrote:
  Strategy for code changes:
Patch #1: Move permissions checks currently implemented in other parts
  of the code (eg: tablecmds.c:ATExecChangeOwner()) into
  aclchk.c.
Patch #2: Change the aclchk.c function APIs, if necessary, to add
  additional information required for SELinux (eg: the 'old'
  owner).
Patch #3: Add SELinux hooks into aclchk.c functions.
 
 Thanks for your idea. However, it seems to me this approach eventually
 requires bigger changeset than what the current security hooks doing.

Yes, it almost certainly will.  However, this is really the approach
that I think we need to take.  I think what we're beginning to
understand, collectivly, is something that some of the committers, etc,
have been saying all along- implementing SELinux in PG requires alot
more changes to PG than just sprinkling hooks all over the code.  This
is because we want to do it right, for PG, and for our users.

What this means is that we need to improve the PG code first.  Then,
when we're happy with those changes, we can add the SELinux hooks and be
comfortable that they will operate correctly, do what they're intended
to, and also allow us to extend the system further for the next big
privileges thing, if that happens some day.

 In addition, it cannot solve the differences in behavior due to the
 security model on which they stand.
 
 For example, when we create a new table, the native database privilege
 mechanism checks its permission on the schema object which the new
 table blongs to. On the other hand, SELinux requires to check
 db_table:{create} permission on the newly created table itself.

That's fine.  We implement a function in aclchk.c which is essentially
pg_createTablePriv(schema,newtable).  The logic for doing the check is
moved out of tablecmds.c (or where ever) and into that function.  We can
then easily add to that function the SELinux hook.  Is there some issue
here which I'm not seeing?  If so, can you please clarify?

Also, I don't see db_table:{create} in the documentation anywhere.
There is a db_schema:{add_name}, is that what you're referring to?
How would you check the permissions on an object (and how or why would
they be different than the default, unless that's also being handled at
the creation time) which is in the process of being created?

 In my understanding, the purpose of the design specification is to make
 clear for database folks what the security hooks doin and why the security
 hooks are deployed here.

I don't think we're going to have much buy-in or success sprinkling
these hooks all over the PG source code.  It would make maintenance a
real problem for the PG folks and for those of us trying to work with
SE.  The SELinux hooks really need to be folded into PG's existing
authentication system, and if that system needs to be improved or
expanded, then that's what it requires.

 Linux kernel is a good case in success.
 Needless to say, most of kernel developer are not security experts.
 But they defined an explicit specification of the security hooks called
 LSM, and put these hooks on the strategic points in the kernel.
 (The very long source code comments describes what is the purpose of this
 security hook, what arguments should be given and so on for each security
 hooks.)

Right, we have a number of these already, and they are represented by
the aclchk.c functions.  The problem that SELinux points out is that
we've not been consistant with having aclchk.c do the permissions
checking.  Specifically on the simple stuff where we currently require
owner rights.  What we need to do is improve PG by pulling those checks
out of the areas they are, spread all around the code, and consolidating
them into aclchk.c (or a new file if that is deisred).  We're not going
to get anywhere by trying to add on more hooks all over the code.

  This initial round, again, should focus on just those
  controls/permissions which PostgreSQL already supports.  At this time,
  do not stress over finding every if(superuser()) and moving it to
  aclchk.c.  Let's deal with the clear situations, such as tablecmds.c
  lines 6322-6342 (that entire block, including the if(!superuser()),
  should be ripped out and made a function in aclchk.c which is called
  with necessary args).  We can go back and clean up the other places
  where we have explicit superuser() checks later, if necessary.
 
 At least, it works correctly as far as the native database privilege
 mechanism. If the specification of the security hooks is clear, I think
 the following manner is better than modifying DAC mechanism.
 
   if (!superuser())
   {
   if (pg_xxx_aclcheck() != ACLCHECK_OK)
   aclcheck_error();
   }
 + sepgsqlCheck();

Again, this would mean dropping sepgsql() calls all over the code,
because we don't currently have all of these permission checks 

Re: [HACKERS] machine-readable explain output v2

[Robert Haas]

On Sat, Jul 18, 2009 at 10:29 PM, Andres Freundand...@anarazel.de wrote:
 One part where I find the code flow ugly is 'did_boilerplate' in
 report_triggers/its callsites.
 I can see why it is done that way, but its not exactly obvious to read when
 you want to find out how the format looks.

Suggestions?

 Another, minor, issue is that the patch changes the FORMAT TEXT/default output
 if VERBOSE is specified (schema is added). I don't see that as a real problem
 because the format for VERBOSE is new anyway, but I thought I would mention
 it.

Verbose isn't new, but in 8.4 all it does is displays the output list
for each node.  I think there's room for verbose to include some other
types of verbosity that don't merit their own options.

I think that the choice of what information to display shouldn't
depend on what format you use to display it.  The funny thing about me
being the one to implement XML and JSON output is that I don't
actually want to use them for anything - and especially not XML.  I
implemented them because they were a good (and popular) test case for
the options facility, but they're not actually the options I want.
And if I or others add functionality in the future to gather more
information via EXPLAIN, I don't want to have to use XML or JSON to
get at them.

 XML Format:
 As discussed previously I would like the format to look a bit different - but
 everyone wants it to look different anyway, so I think its ok as one of
 multiple possible lowest common determinators... With the big advantage of
 already being implemented.

Yes, I think there's not much advantage to changing this around.
There are lots of people with lots of ideas on this, but they're all
different and AFAICS there's no clear pattern.  I think it's good to
try to match the text output as much as possible.

 I think it would be nice in the future to add some sort of
 'category={planner,timing,..}' attribute, but I think that should be
 discussed/implemented separately.

Agree that there are more things to be added.  But I haven't taken the
time to figure out exactly what.  One of things I would really like to
be able to get is the number of buckets and batches (expected and
actual) for a hash join.  Other things I've wished for:

- number of tuples discarded by a filter condition on a particular node
- amount of time spent constructing the output list of a node as
opposed to the rest of what the node does

 Documentation:
 I think it would be nice to add some more documentation about the xml format
 for application writers, but I think this should be a separate patch anyway.

Suggestions?

I have posted a new version of this patch on a separate thread; do you
have time to re-review?

...Robert

-- 
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] machine-readable explain output v2

[Andres Freund]

Hi Robert,

On Friday 31 July 2009 23:13:54 Robert Haas wrote:
 On Sat, Jul 18, 2009 at 10:29 PM, Andres Freundand...@anarazel.de wrote:
 I have posted a new version of this patch on a separate thread; do you
 have time to re-review?
Yes, I have seen it. I plan to spent some time on it tonight and/or tomorrow. 

You havent pushed out to your git mirror?

Andres

-- 
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] machine-readable explain output v2

[Andres Freund]

On Friday 31 July 2009 23:13:54 Robert Haas wrote:
 On Sat, Jul 18, 2009 at 10:29 PM, Andres Freundand...@anarazel.de wrote:
  One part where I find the code flow ugly is 'did_boilerplate' in
  report_triggers/its callsites.
  I can see why it is done that way, but its not exactly obvious to read
  when you want to find out how the format looks.
 Suggestions?
Will take a look when looking at the whole patch again.

  Another, minor, issue is that the patch changes the FORMAT TEXT/default
  output if VERBOSE is specified (schema is added). I don't see that as a
  real problem because the format for VERBOSE is new anyway, but I thought
  I would mention it.
 Verbose isn't new, but in 8.4 all it does is displays the output list
 for each node.  I think there's room for verbose to include some other
 types of verbosity that don't merit their own options.
Well, before 8.4 it was something entirely different... So its kinda new.

 I think that the choice of what information to display shouldn't
 depend on what format you use to display it.  The funny thing about me
 being the one to implement XML and JSON output is that I don't
 actually want to use them for anything - and especially not XML.  I
 implemented them because they were a good (and popular) test case for
 the options facility, but they're not actually the options I want.
 And if I or others add functionality in the future to gather more
 information via EXPLAIN, I don't want to have to use XML or JSON to
 get at them.
I am not particularly interested in XML itself as well. Just as yours my main 
interest is having the possibility to add information without causing wreckage 
all over the place
Although for some of the more complex queries (And as you know I have some 
rather ugly/complex ones...) a graphical viewer of the plans might be nice.

I am quite happy that the annoyance over a patch of mine helped you starting 
to work on this ;-) 
Thanks for all the work.

  I think it would be nice in the future to add some sort of
  'category={planner,timing,..}' attribute, but I think that should be
  discussed/implemented separately.
 Agree that there are more things to be added.  But I haven't taken the
 time to figure out exactly what.  One of things I would really like to
 be able to get is the number of buckets and batches (expected and
 actual) for a hash join.  Other things I've wished for:
I think after the patch is committed there should be a big collection of 
wishes so we can see what further infrastructure work is going to be needed...
Depending on the amount and kind of different options it might not be 
sufficient 
to simply extent struct Instrumentation/the current instrumentation 
infrastructure...


  Documentation:
  I think it would be nice to add some more documentation about the xml
  format for application writers, but I think this should be a separate
  patch anyway.
 Suggestions?
I think extending, correcting and commenting a schema like the one I provided 
sometime ago would be a good start. Anybody wanting to use the output should 
be familiar enough with that...
I can try to do some of that if somebody goes over my english afterwards...

Andres

-- 
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] machine-readable explain output v2

[Robert Haas]

On Fri, Jul 31, 2009 at 6:04 PM, Andres Freundand...@anarazel.de wrote:
 I am quite happy that the annoyance over a patch of mine helped you starting
 to work on this ;-)
 Thanks for all the work.

You're welcome, thanks for all your reviewing.  For the record, I
wasn't annoyed BY the patch; I was annoyed by the inability of the
patch to be applied.

  I think it would be nice in the future to add some sort of
  'category={planner,timing,..}' attribute, but I think that should be
  discussed/implemented separately.
 Agree that there are more things to be added.  But I haven't taken the
 time to figure out exactly what.  One of things I would really like to
 be able to get is the number of buckets and batches (expected and
 actual) for a hash join.  Other things I've wished for:
 I think after the patch is committed there should be a big collection of
 wishes so we can see what further infrastructure work is going to be needed...
 Depending on the amount and kind of different options it might not be 
 sufficient
 to simply extent struct Instrumentation/the current instrumentation
 infrastructure...

We'll have to see.  The basic options framework is already in, but I
think a more far-ranging discussion should wait until post-CommitFest,
whether explain (format ...) ... is committed by then or not.

  Documentation:
  I think it would be nice to add some more documentation about the xml
  format for application writers, but I think this should be a separate
  patch anyway.
 Suggestions?
 I think extending, correcting and commenting a schema like the one I provided
 sometime ago would be a good start. Anybody wanting to use the output should
 be familiar enough with that...
 I can try to do some of that if somebody goes over my english afterwards...

Happy to copy-edit.

...Robert

-- 
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] SE-PostgreSQL Specifications

[Robert Haas]

On Fri, Jul 31, 2009 at 5:13 PM, Stephen Frostsfr...@snowman.net wrote:
 KaiGai,

 * KaiGai Kohei (kai...@kaigai.gr.jp) wrote:
 Stephen Frost wrote:
  Strategy for code changes:
    Patch #1: Move permissions checks currently implemented in other parts
              of the code (eg: tablecmds.c:ATExecChangeOwner()) into
                      aclchk.c.
    Patch #2: Change the aclchk.c function APIs, if necessary, to add
              additional information required for SELinux (eg: the 'old'
                      owner).
    Patch #3: Add SELinux hooks into aclchk.c functions.

 Thanks for your idea. However, it seems to me this approach eventually
 requires bigger changeset than what the current security hooks doing.

 Yes, it almost certainly will.  However, this is really the approach
 that I think we need to take.  I think what we're beginning to
 understand, collectivly, is something that some of the committers, etc,
 have been saying all along- implementing SELinux in PG requires alot
 more changes to PG than just sprinkling hooks all over the code.  This
 is because we want to do it right, for PG, and for our users.

 What this means is that we need to improve the PG code first.  Then,
 when we're happy with those changes, we can add the SELinux hooks and be
 comfortable that they will operate correctly, do what they're intended
 to, and also allow us to extend the system further for the next big
 privileges thing, if that happens some day.

 In addition, it cannot solve the differences in behavior due to the
 security model on which they stand.

 For example, when we create a new table, the native database privilege
 mechanism checks its permission on the schema object which the new
 table blongs to. On the other hand, SELinux requires to check
 db_table:{create} permission on the newly created table itself.

 That's fine.  We implement a function in aclchk.c which is essentially
 pg_createTablePriv(schema,newtable).  The logic for doing the check is
 moved out of tablecmds.c (or where ever) and into that function.  We can
 then easily add to that function the SELinux hook.  Is there some issue
 here which I'm not seeing?  If so, can you please clarify?

 Also, I don't see db_table:{create} in the documentation anywhere.
 There is a db_schema:{add_name}, is that what you're referring to?
 How would you check the permissions on an object (and how or why would
 they be different than the default, unless that's also being handled at
 the creation time) which is in the process of being created?

 In my understanding, the purpose of the design specification is to make
 clear for database folks what the security hooks doin and why the security
 hooks are deployed here.

 I don't think we're going to have much buy-in or success sprinkling
 these hooks all over the PG source code.  It would make maintenance a
 real problem for the PG folks and for those of us trying to work with
 SE.  The SELinux hooks really need to be folded into PG's existing
 authentication system, and if that system needs to be improved or
 expanded, then that's what it requires.

 Linux kernel is a good case in success.
 Needless to say, most of kernel developer are not security experts.
 But they defined an explicit specification of the security hooks called
 LSM, and put these hooks on the strategic points in the kernel.
 (The very long source code comments describes what is the purpose of this
 security hook, what arguments should be given and so on for each security
 hooks.)

 Right, we have a number of these already, and they are represented by
 the aclchk.c functions.  The problem that SELinux points out is that
 we've not been consistant with having aclchk.c do the permissions
 checking.  Specifically on the simple stuff where we currently require
 owner rights.  What we need to do is improve PG by pulling those checks
 out of the areas they are, spread all around the code, and consolidating
 them into aclchk.c (or a new file if that is deisred).  We're not going
 to get anywhere by trying to add on more hooks all over the code.

  This initial round, again, should focus on just those
  controls/permissions which PostgreSQL already supports.  At this time,
  do not stress over finding every if(superuser()) and moving it to
  aclchk.c.  Let's deal with the clear situations, such as tablecmds.c
  lines 6322-6342 (that entire block, including the if(!superuser()),
  should be ripped out and made a function in aclchk.c which is called
  with necessary args).  We can go back and clean up the other places
  where we have explicit superuser() checks later, if necessary.

 At least, it works correctly as far as the native database privilege
 mechanism. If the specification of the security hooks is clear, I think
 the following manner is better than modifying DAC mechanism.

   if (!superuser())
   {
       if (pg_xxx_aclcheck() != ACLCHECK_OK)
           aclcheck_error();
   }
 + sepgsqlCheck();

 Again, this would mean 

Re: [HACKERS] SE-PostgreSQL Specifications

[KaiGai Kohei]

Stephen Frost wrote:
 KaiGai,
 
 * KaiGai Kohei (kai...@kaigai.gr.jp) wrote:
 Stephen Frost wrote:
 Strategy for code changes:
   Patch #1: Move permissions checks currently implemented in other parts
 of the code (eg: tablecmds.c:ATExecChangeOwner()) into
 aclchk.c.
   Patch #2: Change the aclchk.c function APIs, if necessary, to add
 additional information required for SELinux (eg: the 'old'
 owner).
   Patch #3: Add SELinux hooks into aclchk.c functions.
 Thanks for your idea. However, it seems to me this approach eventually
 requires bigger changeset than what the current security hooks doing.
 
 Yes, it almost certainly will.  However, this is really the approach
 that I think we need to take.  I think what we're beginning to
 understand, collectivly, is something that some of the committers, etc,
 have been saying all along- implementing SELinux in PG requires alot
 more changes to PG than just sprinkling hooks all over the code.  This
 is because we want to do it right, for PG, and for our users.
 
 What this means is that we need to improve the PG code first.  Then,
 when we're happy with those changes, we can add the SELinux hooks and be
 comfortable that they will operate correctly, do what they're intended
 to, and also allow us to extend the system further for the next big
 privileges thing, if that happens some day.

When I scraped the idea of PGACE framework, someone (IIRC, Robert Haas) asked
me whether it is possible to implement the native database privilege mechanism
on the PGACE, or not.
It seems to me your suggestion is similar to the idea of PGACE framework.
But, note that it does not mean I criticize your idea.

Let's consider the matter more using a few examples.

Example 1) ALTER TABLE xxx
The native database privilege mechanism checks the ownership of the target
table to control the ALTER TABLE. On the other hand, SE-PgSQL checks the
db_table:{setattr} permission on the target table to control the ALTER TABLE.

I believe you can notice both of checks have same purpose but differences are
on the criterions. It is the way to make their decisions in other word.

The pg_xxx_aclcheck() interfaces provide the way to make its access control
decision. The core PG code calls pg_xxx_aclcheck() to achieve its purpose
which is to apply access controls on ALTER TABLE.

Accordingly, if we avoid to put SE-PgSQL's security hooks on the tablecmd.c
directly, what we should do is to inject an abstraction layer between tablecmd.c
and aclchk.c.

For example:
  void pg_security_alter_table(Oid relid)
  {
  if (!pg_class_ownercheck(relid, GetUserId())
  aclcheck_error(...);

  if (!sepgsqlCheckTableSetattr(relid))
  selinux_error(...);
  }

Example 2) DROP TABLE xxx

The native database privilege mechanism also checks the ownership of the target
table to be dropped (except for cascaded deletion).
However, SELinux want to apply db_table:{drop} permission, instead of 
db_table:{setattr}.
It is obvious that pg_class_ownercheck() cannot be a caller of the SELinux's
security hook.

It should be:
  void pg_security_drop_table(Oid relid)
  {
  if (!pg_class_ownercheck(relid, GetUserId())
  ackcheck_error(...);

  if (!sepgsqlCheckTableDrop(relid))
  selinux_error(...);
  }

Example 3) ACL_EXECUTE

The ACL_EXECUTE permission is checked on the runtime and setup time.
The meaning of runtime is obvious. It should be checked when user's query tries 
to
execute a function.
For example, CreateConversionCommand() checks ACL_EXECUTE when user tries to 
create
a new conversion. It is available for all the users.
However, SELinux want to apply different permissions for runtime and 
installation time.

On the runtime of the function, it applies db_procedure:{execute}.
On the installation time, it applies db_procedure:{install}.
Because a user defined function is installed as a part of system internals,
it is available for every users. SELinux considers it is fundamentally different
from a user can run a function defined myself.


Because the topic is a bit abstract, it is not clear whether I can introduce
what I want to say correctly, or not.
Please feel free to ask me, if something unclear.


 In addition, it cannot solve the differences in behavior due to the
 security model on which they stand.

 For example, when we create a new table, the native database privilege
 mechanism checks its permission on the schema object which the new
 table blongs to. On the other hand, SELinux requires to check
 db_table:{create} permission on the newly created table itself.
 
 That's fine.  We implement a function in aclchk.c which is essentially
 pg_createTablePriv(schema,newtable).  The logic for doing the check is
 moved out of tablecmds.c (or where ever) and into that function.  We can
 then easily add to that function the SELinux hook.  Is there some issue
 here which I'm not seeing?  If so, can you please clarify?
 
 

Re: [HACKERS] SE-PostgreSQL Specifications

[KaiGai Kohei]

Robert Haas wrote:

FWIW, pretty much +1 from me on everything in here; I think this is
definitely going in the right direction.  It's not the size of the
patches that matter; it's the complexity and difficulty of verifying
that they don't break anything.  And it's not cumulative: three easy
patches are better than one hard one, as long as they're really
self-contained.

The idea of restructuring the aclcheck mechanism to support sepgsql
is, IMO, brilliant.


As I noted in the reply to Stephen Frost, what should be controled
(e.g, ALTER TABLE) and how to check it (e.g, ownership based control)
are different things.

If we go on the direction to restructure the current aclcheck mechanism
and to integrate entry points of security features into a single file,
I really really want an implementation independent layer which focuses
on access controls.

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

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

Re: [HACKERS] SE-PostgreSQL Specifications

[Stephen Frost]

KaiGai,

* KaiGai Kohei (kai...@kaigai.gr.jp) wrote:
 It seems to me your suggestion is similar to the idea of PGACE framework.

It is, but it's being done as incremental changes to the existing
structures, and working with them, instead of ignoring that they exist.

 Let's consider the matter more using a few examples.
 
 Example 1) ALTER TABLE xxx
 The native database privilege mechanism checks the ownership of the target
 table to control the ALTER TABLE. On the other hand, SE-PgSQL checks the
 db_table:{setattr} permission on the target table to control the ALTER TABLE.
 
 I believe you can notice both of checks have same purpose but differences are
 on the criterions. It is the way to make their decisions in other word.
 
 The pg_xxx_aclcheck() interfaces provide the way to make its access control
 decision. The core PG code calls pg_xxx_aclcheck() to achieve its purpose
 which is to apply access controls on ALTER TABLE.
 
 Accordingly, if we avoid to put SE-PgSQL's security hooks on the tablecmd.c
 directly, what we should do is to inject an abstraction layer between 
 tablecmd.c
 and aclchk.c.
 
 For example:
   void pg_security_alter_table(Oid relid)
   {
   if (!pg_class_ownercheck(relid, GetUserId())
   aclcheck_error(...);
 
   if (!sepgsqlCheckTableSetattr(relid))
   selinux_error(...);
   }

Right, something along these lines, where the function lives in aclchk.c
and is part of its overall API.  This also shows pretty clearly how, if
we needed to add other hooks into the permissions for this operation, we
could do so.  It also shows how we could *first* build:

void pg_security_alter_table(Oid relid)
{
if (!pg_class_ownercheck(relid, GetUserId())
aclcheck_error(...);
}

and make the corresponding changes in tablecmds.c, etc, and do that as a
separate patch.  Once that's done, we can review it, test it, etc, as
just an incremental change to PG.  This should be much easier/faster to
review as a patch as well, and to convince ourselves that we havn't
broken PG's current security (which is an extremely critical thing).
Additionally, when we then come back and add SELinux hooks, they will be
done in an isolated area and be additions to a system which has been
made to support such an extension.


 Example 2) DROP TABLE xxx
 
 The native database privilege mechanism also checks the ownership of the 
 target
 table to be dropped (except for cascaded deletion).
 However, SELinux want to apply db_table:{drop} permission, instead of 
 db_table:{setattr}.
 It is obvious that pg_class_ownercheck() cannot be a caller of the SELinux's
 security hook.
 
 It should be:
   void pg_security_drop_table(Oid relid)
   {
   if (!pg_class_ownercheck(relid, GetUserId())
   ackcheck_error(...);
 
   if (!sepgsqlCheckTableDrop(relid))
   selinux_error(...);
   }

Right.  Similar to above.

 Example 3) ACL_EXECUTE
 
 The ACL_EXECUTE permission is checked on the runtime and setup time.
 The meaning of runtime is obvious. It should be checked when user's query 
 tries to
 execute a function.
 For example, CreateConversionCommand() checks ACL_EXECUTE when user tries to 
 create
 a new conversion. It is available for all the users.
 However, SELinux want to apply different permissions for runtime and 
 installation time.
 
 On the runtime of the function, it applies db_procedure:{execute}.
 On the installation time, it applies db_procedure:{install}.
 Because a user defined function is installed as a part of system internals,
 it is available for every users. SELinux considers it is fundamentally 
 different
 from a user can run a function defined myself.

I'm not sure I follow how this is dramatically different from the other
examples.  We have checks in place in PG already at both runtime of the
function (user has 'execute' privilege) and at creation time (functions
are created in schemas, after all).  If there are checks that PG doesn't
do today but which SELinux wants, those can be added too..  But as we've
discussed, that should be postponed until we get this initial structure
in place to allow PG to be extensible in this way.

 Because the topic is a bit abstract, it is not clear whether I can introduce
 what I want to say correctly, or not.
 Please feel free to ask me, if something unclear.

Yeah, I don't entirely get what you're trying to say here.  Perhaps you
could try and rephrase it?

 It is described at:
   
 http://wiki.postgresql.org/wiki/SEPostgreSQL_Development#Common_object_behavior
 
 When we create a new table, SELinux's mode requires all the following
 permissions individually.
  - db_schema:{add_name}, because a table is created under a certain schema
  - db_table:{create} due to the common object behavior
  - db_column:{create} for each columns, due to the common object behavior
 
 Note that the newly created object does not exist when SE-PgSQL checks its
 db_xxx:{create} permission.
 Accordingly, it computes a default security 

Re: [HACKERS] SE-PostgreSQL Specifications

[Stephen Frost]

* KaiGai Kohei (kai...@kaigai.gr.jp) wrote:
 As I noted in the reply to Stephen Frost, what should be controled
 (e.g, ALTER TABLE) and how to check it (e.g, ownership based control)
 are different things.

 If we go on the direction to restructure the current aclcheck mechanism
 and to integrate entry points of security features into a single file,
 I really really want an implementation independent layer which focuses
 on access controls.

I think that's what I'm advocating..  If, by that, you mean we should do
it in a separate file from aclchk.c, I'm not against that.  It would
likely mean moving some things *from* aclchk.c into it, and then just
using aclchk.c for helpers to support the PG permissions.  I'm not
sure which way would be easier to handle in terms of patch review,
etc..

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Lock Wait Statistics (next commitfest)

[Mark Kirkwood]

Mark Kirkwood wrote:

Jaime Casanova wrote:
On Fri, Jul 17, 2009 at 3:38 AM, Mark 
Kirkwoodmar...@paradise.net.nz wrote:
 
With respect to the sum of wait times being not very granular, yes - 
quite
true. I was thinking it is useful to be able to answer the question 
'where
is my wait time being spent' - but it hides cases like the one you 
mention.

What would you like to see?  would max and min wait times be a useful
addition, or are you thinking along different lines?




track number of locks, sum of wait times, max(wait time).
but actually i started to think that the best is just make use of
log_lock_waits send the logs to csvlog and analyze there...

  

Right - I'll look at adding max (at least) early next week.





Patch with max(wait time).

Still TODO

- amalgamate individual transaction lock waits
- redo (rather ugly) temporary pg_stat_lock_waits in a form more like 
pg_locks




lockstats-3.patch.gz
Description: GNU Zip compressed 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] SE-PostgreSQL Specifications

[KaiGai Kohei]

Stephen Frost wrote:
 For example:
   void pg_security_alter_table(Oid relid)
   {
   if (!pg_class_ownercheck(relid, GetUserId())
   aclcheck_error(...);

   if (!sepgsqlCheckTableSetattr(relid))
   selinux_error(...);
   }
 
 Right, something along these lines, where the function lives in aclchk.c
 and is part of its overall API.  This also shows pretty clearly how, if
 we needed to add other hooks into the permissions for this operation, we
 could do so.  It also shows how we could *first* build:
 
 void pg_security_alter_table(Oid relid)
 {
   if (!pg_class_ownercheck(relid, GetUserId())
   aclcheck_error(...);
 }
 
 and make the corresponding changes in tablecmds.c, etc, and do that as a
 separate patch.  Once that's done, we can review it, test it, etc, as
 just an incremental change to PG.  This should be much easier/faster to
 review as a patch as well, and to convince ourselves that we havn't
 broken PG's current security (which is an extremely critical thing).
 Additionally, when we then come back and add SELinux hooks, they will be
 done in an isolated area and be additions to a system which has been
 made to support such an extension.

Basically, I can agree this approach.
However, it is necessary to consider the idea deeply whether it is really
possible to implement the SELinux's model correctly, or not.

For example, ExecCheckRTEPerms() checks permissions on DML statement.
It allows users to access required table and columns, if he has appropriate
permissions on the table OR all the columns in use.
But SELinux requires the client needs to be allowed on the table AND all
the columns in use.

Please note that all we need to focus on is not pg_xxx_aclcheck() routines
in other words.

 Example 3) ACL_EXECUTE

 The ACL_EXECUTE permission is checked on the runtime and setup time.
 The meaning of runtime is obvious. It should be checked when user's query 
 tries to
 execute a function.
 For example, CreateConversionCommand() checks ACL_EXECUTE when user tries to 
 create
 a new conversion. It is available for all the users.
 However, SELinux want to apply different permissions for runtime and 
 installation time.

 On the runtime of the function, it applies db_procedure:{execute}.
 On the installation time, it applies db_procedure:{install}.
 Because a user defined function is installed as a part of system internals,
 it is available for every users. SELinux considers it is fundamentally 
 different
 from a user can run a function defined myself.
 
 I'm not sure I follow how this is dramatically different from the other
 examples.  We have checks in place in PG already at both runtime of the
 function (user has 'execute' privilege) and at creation time (functions
 are created in schemas, after all).  If there are checks that PG doesn't
 do today but which SELinux wants, those can be added too..  But as we've
 discussed, that should be postponed until we get this initial structure
 in place to allow PG to be extensible in this way.

What I would like to say was how does the user defined function is used.
(However, it is the issue corresponding to the security model, not where
we should put the security hooks.)

For example, type input/output handlers are invoked without permission
checks on runtime. We never see an error due to the lack of permission
to execute textout() function. The native privilege model assumes type
input/output handlers can be trusted, because only superuser can set up
own types. (In fact, DefineType() checks superuser() at the head.)

So, rest of the routine does not check anything based on the assumption
of the superuser. See the #ifdef NOT_USER ... #endif block in DefineType().

It looks like the native database privilege mechanism does not check such
a permission, however, it implicitly assume ACL_EXECUTE permission is
always allowed for superuser.

Here, it seems to me ACL_EXECUTE has two different meanings here.
The one is obvious. It is a privilege a certain user to execute a certain
function in his query. The other is a privilege a certain user allows
everyone to execute a certain a part of the system internal stuff
(such as type input/output handlers).

The example is not dramatically different from the others, indeed.
But, this code implicitly assume the database superuser can do anyhting,
so the necessary checks are omitted from the code (because they always
return allowed).

I think it is also necessary to follow these implicit permission check.

 As I mentioned above, if (enhanced) PG's access control mechanism can
 contain all the needed SELinux hooks, I can agree to put security hooks
 inside the PG's security.
 However, note that I have a legitimate reason that we cannot put SELinux
 hooks inside the *current* pg_xxx_aclcheck() routines, to implement the
 security model of SELinux correctly.
 
 Sure, and that's fine.  I think the problem that you keep running into
 is this:  You don't want to touch the PG code too much for 

Re: [HACKERS] Revised signal multiplexer patch

[Robert Haas]

On Fri, Jul 31, 2009 at 4:27 PM, Tom Lanet...@sss.pgh.pa.us wrote:
 Fujii Masao masao.fu...@gmail.com writes:
 I revised the patch according to the suggestion.

 Applied with some mostly-cosmetic editorial work.

                        regards, tom lane

Awesome, congrats.

...Robert

-- 
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] ALTER TABLE ... ALTER COLUMN ... SET DISTINCT

[Robert Haas]

On Fri, Jul 31, 2009 at 11:09 AM, Dimitri
Fontainedfonta...@hi-media.com wrote:
 I failed to have 0 to allow for analyze to compute a value again, as shown
 in the raw notes in attachement:

I'm lost.  I think you're getting the new column attdistinct mixed up
with the existing column stadistinct.  attdistinct is always going to
have whatever value you assign it.  stadistinct will get attdistinct
!= 0 ? attdistinct : result of analyze calculation.

...Robert

-- 
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] machine-readable explain output v2

[Robert Haas]

On Fri, Jul 31, 2009 at 5:36 PM, Andres Freundand...@anarazel.de wrote:
 Hi Robert,

 On Friday 31 July 2009 23:13:54 Robert Haas wrote:
 On Sat, Jul 18, 2009 at 10:29 PM, Andres Freundand...@anarazel.de wrote:
 I have posted a new version of this patch on a separate thread; do you
 have time to re-review?
 Yes, I have seen it. I plan to spent some time on it tonight and/or tomorrow.

 You havent pushed out to your git mirror?

Ah, fixed.

Sorry, forgot I made a new local branch.

...Robert

-- 
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] Filtering dictionaries support and unaccent dictionary

[Alvaro Herrera]

Teodor Sigaev wrote:

 As for the contrib module, I think it could use a lot more function
 header comments!  Also, it would be great if it could be used separately
 from tsearch, i.e. that it provided a function unaccent(text) returns
 text that unaccented arbitrary strings (I guess it would use the default
 tsconfig).
 Umm? Module provides unaccent(text) and unaccent(regdictionary, text) 
 functions.

Sorry, I failed to notice.  Looks good.

Isn't that function leaking res pointer?  Also, I'm curious why you're
allocating 2*sizeof(TSLexeme) in unaccent_lexize ...

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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