Re: [HACKERS] New version of money type
* D'Arcy J.M. Cain (darcy@druid.net) wrote: On Thu, 28 Sep 2006 12:44:24 -0400 Stephen Frost [EMAIL PROTECTED] wrote: I'm not sure about 'money' in general but these claims of great performance improvments over numeric just don't fly so easily with me. numeric isn't all *that* much slower than regular old integer in the tests that I've done. Numeric has been shown to be as good or better than money in I/O operations. Where money shines is in internal calculations. Which may be an area which could be improved on for numeric, or even a numeric64 type added for it. I'm not entirely sure there's a huge amount to gain there either though... Thanks, Stephen signature.asc Description: Digital signature
[HACKERS] array_accum aggregate
Greetings,
The array_accum example aggregate in the user documentation works
reasonably on small data sets but doesn't work too hot on large ones.
http://www.postgresql.org/docs/8.1/static/xaggr.html
Normally I wouldn't care particularly much but it turns out that PL/R
uses arrays for quite a bit (eg: histograms and other statistics
functions). I've also heard other complaints about the performance of
arrays, though I'm not sure if those were due to array_accum or
something else.
Long story short, I set out to build a faster array_accum. Much to my
suprise and delight, we already *had* one. accumArrayResult() and
makeArrayResult()/construct_md_array() appear to do a fantastic job.
I've created a couple of 'glue' functions to expose these functions so
they can be used in an aggregate. I'm sure they could be improved
upon and possibly made even smaller than they already are (90 lines
total for both) but I'd like to throw out the idea of including them
in core. The aggregate created with them could also be considered for
inclusion though I'm less concerned with that. I don't expect general
PostgreSQL users would have trouble creating the aggregate- I don't
know that the average user would be able or willing to write the C
functions.
For comparison, the new functions run with:
time psql -c select aaccum(generate_series) from
generate_series(1,100); /dev/null
4.24s real 0.34s user 0.06s system
Compared to:
time psql -c select array_accum(generate_series) from
generate_series(1,100); /dev/null
...
Well, it's still running and it's been over an hour.
The main differences, as I see it, are: accumArrayResult() works in
chunks of 64 elements, and uses repalloc(). array_accum uses
array_set() which works on individual elements and uses
palloc()/memcpy(). I appriciate that this is done because for most
cases of array_set() it's not acceptable to modify the input and am
not suggesting that be changed. An alternative might be to modify
array_set() to check if it is in an aggregate and change its behavior
but adding the seperate functions seemed cleaner and much less
intrusive to me.
Please find the functions attached.
Thanks,
Stephen
#include postgres.h
#include fmgr.h
#include utils/array.h
#include nodes/execnodes.h
PG_MODULE_MAGIC;
PG_FUNCTION_INFO_V1(aaccum_sfunc);
Datum
aaccum_sfunc(PG_FUNCTION_ARGS)
{
int32 totlen;
bytea *storage;
Datum element;
ArrayBuildState *astate = NULL;
AggState *aggstate;
/* Make sure we are in an aggregate. */
if (!fcinfo-context || !IsA(fcinfo-context, AggState))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg(Can not call aaccum_sfunc as a non-aggregate)));
aggstate = (AggState*) fcinfo-context;
/* Initial call just passes in NULLs, so just allocate memory
* and get set up. */
if (PG_ARGISNULL(0)) {
storage = (bytea*) palloc(VARHDRSZ+sizeof(ArrayBuildState*));
storage-vl_len = VARHDRSZ+sizeof(ArrayBuildState*);
astate = NULL;
memcpy(storage-vl_dat,astate,sizeof(astate));
} else {
storage = PG_GETARG_BYTEA_P(0);
}
memcpy(astate,storage-vl_dat,sizeof(astate));
element = PG_GETARG_DATUM(1);
astate = accumArrayResult(astate, element, PG_ARGISNULL(1),
get_fn_expr_argtype(fcinfo-flinfo, 1),
aggstate-aggcontext);
memcpy(storage-vl_dat,astate,sizeof(astate));
PG_RETURN_BYTEA_P(storage);
}
PG_FUNCTION_INFO_V1(aaccum_ffunc);
Datum
aaccum_ffunc(PG_FUNCTION_ARGS)
{
int dims[1];
int lbs[1];
bytea *storage;
ArrayBuildState *astate;
AggState *aggstate;
ArrayType *result;
/* Make sure we are in an aggregate. */
if (!fcinfo-context || !IsA(fcinfo-context, AggState))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg(Can not call aaccum_sfunc as a non-aggregate)));
aggstate = (AggState*) fcinfo-context;
if (PG_ARGISNULL(0)) PG_RETURN_ARRAYTYPE_P(NULL);
storage = (bytea*) PG_GETARG_BYTEA_P(0);
memcpy(astate,storage-vl_dat,sizeof(astate));
dims[0] = astate-nelems;
lbs[0] = 1;
result = construct_md_array(astate-dvalues,
astate-dnulls,
1,
dims,
lbs,
astate-element_type,
astate-typlen,
astate-typbyval,
astate-typalign);
PG_RETURN_ARRAYTYPE_P(PointerGetDatum(result));
}
---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings
Re: [HACKERS] array_accum aggregate
* Tom Lane ([EMAIL PROTECTED]) wrote: Stephen Frost [EMAIL PROTECTED] writes: Long story short, I set out to build a faster array_accum. Much to my suprise and delight, we already *had* one. accumArrayResult() and makeArrayResult()/construct_md_array() appear to do a fantastic job. I've created a couple of 'glue' functions to expose these functions so they can be used in an aggregate. I'm sure they could be improved upon and possibly made even smaller than they already are (90 lines total for both) but I'd like to throw out the idea of including them in core. The aggregate created with them could also be considered for inclusion though I'm less concerned with that. Since you've set up the functions to only be usable inside an aggregate, I don't see much of a reason why we wouldn't provide the aggregate too. Sure, I don't see a reason these functions would be useful outside of an aggregate in the form they need to be in for the aggregate, either.. It looks like it should work to have just one polymorphic aggregate definition, eg, array_accum(anyelement) returns anyarray. I was hoping to do that, but since it's an aggregate the ffunc format is pre-defined to require accepting the 'internal state' and nothing else, and to return 'anyelement' or 'anyarray' one of the inputs must be an 'anyelement' or 'anyarray', aiui. That also implied to me that the type passed in was expected to be the type passed out, which wouldn't necessairly be the case here as the internal state variable is a bytea. It might be possible to do away with the internal state variable entirely though and make it an anyelement instead, if we can find somewhere else to put the pointer to the ArrayBuildState. As far as coding style goes, you're supposed to use makeArrayResult() with accumArrayResult(), not call construct_md_array() directly. And copying the ArrayBuildState around like that is just plain bizarre... I had attempted to use makeArrayResult() originally and ran into some trouble with the MemoryContextDelete() which is done during it. The context used was the AggState context and therefore was deleted elsewhere. That might have been a misunderstanding on my part though since I recall fixing at least one or two bugs afterwards, so it may be possible to go back and change to using makeArrayResult(). That'd certainly make the ffunc smaller. As for ArrayBuildState, I'm not actually copying the structure around, just the pointer is being copied around inside of the state transistion variable (which is a bytea). I'm not sure where else to store the pointer to the ArrayBuildState though, since multiple could be in play at a given time during an aggregation, aiui. Does the thing work without memory leakage (for a pass-by-ref datatype) in a GROUP BY situation? I expected that using the AggState context would handle free'ing the memory at the end of the aggregation as I believe that's the context used for the state variable itself as well. Honestly, I wasn't entirely sure how to create my own context or if that was the correct thing to do in this case. I'll see about changing it around to do that if it's acceptable to have a context created for each instance of a group-by aggregate, and I can figure out how. :) I'm not sure about memory leakage during a Sort+GroupAgg.. I don't know how that's done currently either, honestly. It seems like memory could be free'd during that once the ffunc is called, and an extra memory context could do that, is that what you're referring to? Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] array_accum aggregate
* Stephen Frost ([EMAIL PROTECTED]) wrote: For comparison, the new functions run with: time psql -c select aaccum(generate_series) from generate_series(1,100); /dev/null 4.24s real 0.34s user 0.06s system Compared to: time psql -c select array_accum(generate_series) from generate_series(1,100); /dev/null ... Well, it's still running and it's been over an hour. Just to follow-up on this, the result was: 7601.36s real 0.36s user 0.02s system Or about 2 hours. Enjoy, Stephen signature.asc Description: Digital signature
Re: [HACKERS] array_accum aggregate
* Tom Lane ([EMAIL PROTECTED]) wrote:
Stephen Frost [EMAIL PROTECTED] writes:
I was hoping to do that, but since it's an aggregate the ffunc format is
pre-defined to require accepting the 'internal state' and nothing else,
and to return 'anyelement' or 'anyarray' one of the inputs must be an
'anyelement' or 'anyarray', aiui.
Hmm ... I hadn't been thinking about what the state type would need to
be, but certainly bytea is a lie given what you're really doing.
Indeed. I've updated the functions quite a bit to clean things up,
including: Added many more comments, removed the unnecessary 'storage*'
pointer being used, created my own structure for tracking state
information, created a seperate memory context (tied to the AggContext),
correctly handle NULL values, and changed the ffunc to use
makeArrayResult.
I also tried just tried using polymorphic types for the functions and
for the aggregate and it appeared to just work:
create function aaccum_sfunc (anyarray, anyelement) returns anyarray
language 'C' AS 'aaccum.so', 'aaccum_sfunc'
;
create function aaccum_ffunc (anyarray) returns anyarray language
'C' AS '/data/sfrost/postgres/arrays/aaccum.so', 'aaccum_ffunc'
;
create aggregate aaccum (
sfunc = aaccum_sfunc,
basetype = anyelement,
stype = anyarray,
finalfunc = aaccum_ffunc
);
select aaccum(generate_series) from generate_series(1,5);
aaccum
-
{1,2,3,4,5}
(1 row)
(test is a table with one varchar column, abc)
select aaccum(abc) from test;
aaccum
-
{a,b,c}
(1 row)
(Added a column called 'hi', set to 'a', added b,b and c,b)
select hi,aaccum(abc) from test group by hi;
hi | aaccum
+-
b | {b,c}
a | {a,b,c}
(2 rows)
It makes some sense that it would work as an 'anyarray' is just a
variable-length type internally and so long as nothing else attempts to
make sense out of our 'fake array' everything should be fine.
The latest version also appears to be a bit faster than the prior
version. I'm going to be running a very large query shortly using
this aaccum and will report back how it goes. Please let me know if
there are any other improvments or changes I should make. I'd like to
submit this to -patches w/ the appropriate entries to have it be
included in the core distribution. Is it acceptable to reuse the
'array_accum' name even though it was used in the documentation as an
example? I'm thinking yes, but wanted to check.
Thanks!
Stephen
#include postgres.h
#include fmgr.h
#include utils/array.h
#include utils/memutils.h
#include nodes/execnodes.h
PG_MODULE_MAGIC;
/* Structure for storing our pointers to the
* ArrayBuildState for the array we are building
* and the MemoryContext in which it is being
* built. Note that this structure is
* considered a bytea externally and therefore
* must open with an int32 defining the length. */
typedef struct {
int32 vl_len;
ArrayBuildState *astate;
MemoryContext arrctx;
} aaccum_info;
/* The state-transistion function for our aggregate. */
PG_FUNCTION_INFO_V1(aaccum_sfunc);
Datum
aaccum_sfunc(PG_FUNCTION_ARGS)
{
aaccum_info *ainfo;
AggState *aggstate;
/* Make sure we are in an aggregate. */
if (!fcinfo-context || !IsA(fcinfo-context, AggState))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg(Can not call aaccum_sfunc as a non-aggregate)));
aggstate = (AggState*) fcinfo-context;
/* Initial call passes NULL in for our state variable.
* Allocate memory and get set up. */
if (PG_ARGISNULL(0)) {
/* Allocate memory to hold the pointers to the ArrayBuildState
* and the MemoryContext where we are building the array. Note
* that we can do this in the CurrentMemoryContext because when
* we return the storage bytea will be copied into the AggState
* context by the caller and passed back to us on the next call. */
ainfo = (aaccum_info*) palloc(sizeof(aaccum_info));
ainfo-vl_len = sizeof(aaccum_info);
ainfo-astate = NULL;
/* New context created which will store our array accumulation.
* The parent is the AggContext for this query since it needs to
* persist for the same timeframe as the state value.
* The state value holds the pointers to the ArrayBuildState and this
* MemoryContext through the aaccum_info structure. */
ainfo-arrctx = AllocSetContextCreate(aggstate-aggcontext, ArrayAccumCtx,
ALLOCSET_DEFAULT_MINSIZE,
ALLOCSET_DEFAULT_INITSIZE,
ALLOCSET_DEFAULT_MAXSIZE);
} else {
/* Our state variable is non-null, therefore it must be an existing
* ainfo structure. */
ainfo = (aaccum_info*) PG_GETARG_BYTEA_P(0);
}
/* Pull the element to be added and pass
Re: [HACKERS] array_accum aggregate
* Stephen Frost ([EMAIL PROTECTED]) wrote: I'm going to be running a very large query shortly using this aaccum and will report back how it goes. It went *very* well, actually much better than I had originally expected. This query used to take over 12 hours to complete (about 11 of which was after the main table involved was sorted). With this new aaccum in place the whole query only took about an hour, most of which was the sort and join required by the query. The aggregation (aaccum) and r_hist() (R histogram function generating PNGs) took only a few minutes. Thanks! Stephen signature.asc Description: Digital signature
[HACKERS] WIP: Column-level Privileges
Greetings, First, a quick digression into what I've gleaned from the spec in terms of implementation, from SQL2003: -- Table-level grants. The spec basically says that a table-level -- grant is as if the same grant was done on all existing and future -- columns. (I looked at GRANT and ALTER ADD COLUMN definitions to -- discover that) GRANT SELECT, INSERT ON tab1 TO role1; -- Column-level grants. These apply when accessing the column, and -- are independent of table-level grants. If you have a -- column-level grant to a table, you can access that column even if -- you do not have table-level rights to same table. GRANT SELECT (col1, col2), INSERT (col1, col2) ON tab1 TO role2; -- Column-level revokes. Mostly what you would expect, but be aware -- that table-level grants 'with admin option' apply when revoking -- column-level grants on that table. REVOKE SELECT (col1, col2), INSERT (col1, col2) ON tab1 FROM role2; -- Table-level revokes. A table-level revoke of a given privilege -- applies to all columns as well. So if you have INSERT on col1 -- and I do a 'REVOKE INSERT ON tab1 FROM you;', your column-level -- permissions will also be removed. REVOKE SELECT, INSERT ON tab1 FROM role1; One of the implications from all of this is that we're going to have to keep column-level and table-level permissions seperate and distinct from each other. An upshot from this is that it probably more closely matches what people expect from how we've handled things in the past. Also, as long as we do table-level checks first, people who don't use column-level permissions shouldn't see much of performance hit. It'll be a bit slower on failure cases because it'll do per-column checks for ACLs which aren't defined. I'm on the fence as to if that's a big enough concern to warrant a flag in pg_class. Attached is the current state of the patch. The grammer/parser changes have been tested and seem to work reasonably well so far. The aclchk.c changes are probably broken at the moment. I've been frustrated with implementing what the spec calls for and what it means for the code. Specifically, I don't like the number of nested loops to get the per-privilege column lists into per-column ACL masks, and dealing with multiple relations at a time. I also don't like the amount of what seems like mostly duplicated code between the table-level permissions handling and the column-level handling, but I might be a little pedantic there. Working the column-level permissions into other parts of the code has also proven challenging since you need a (Oid,AttrNumber) combination to identify a column instead of just an Oid like everything expects. I've yet to even start in on the changes necessary to get the column information down to the executor, unfortunately. Comments welcome, apologies for it not being ready by 9/1. I'm planning to continue working on it tomorrow, and throughout September as opportunity allows (ie: when Josh isn't keeping me busy). Thanks, Stephen colprivs_wip.20080901.diff.gz 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] Extending grant insert on tables to sequences
* Jaime Casanova ([EMAIL PROTECTED]) wrote: On Fri, Aug 22, 2008 at 10:19 PM, Bruce Momjian [EMAIL PROTECTED] wrote: Added to September commit fest. updating the patch with one that only extends inserts. though, i haven't look at the col level privs patch yet. At least initially I wasn't planning to support column-level privileges for sequences, so I don't think it will affect you much. Do people think it makes sense to try and support that? As your patch appears more ready-for-commit than the column-level privileges patch, I wouldn't worry about what code might have to move around, that'll be for me to deal with in a re-sync with HEAD once your patch is committed. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] WIP: Column-level Privileges
Greetings, * Stephen Frost ([EMAIL PROTECTED]) wrote: Comments welcome, apologies for it not being ready by 9/1. I'm planning to continue working on it tomorrow, and throughout September as opportunity allows (ie: when Josh isn't keeping me busy). Here is an updated patch. I've added column-level privilege information to the psql output as an additional column for \dp, but it depends on the array_accum() aggregate which isn't included (yet). This output matches more what I would expect, but I'm open to comments. An example of what it looks like is here, for the next few days: http://pgsql.privatepaste.com/871z3IheMr I havn't tested everything, but basic column-level grant/revoke should work now. This includes: grammer, parser, catalog changes. It also properly does the 'revoke all column-level when called as a table-level revoke' SQL spec requirement. It does not yet have proper dependency handling. Next I'm planning to work on adding some regression tests for grant/revoke commands and catalog updates and make sure those all work correctly. Then I'll probably go through the dependency handling, and last will be working on the changes to implement the permission checks. Thanks, Stephen colprivs_wip.20080902.diff.gz Description: Binary data signature.asc Description: Digital signature
Re: [HACKERS] pg_dump roles support
* Tom Lane ([EMAIL PROTECTED]) wrote: =?ISO-8859-1?Q?Benedek_L=E1szl=F3?= [EMAIL PROTECTED] writes: pg_dumpall now just passes the --role option to pg_dump. What do you think, is it enough or it should issue the SET ROLE TO ... command in its own session too? I think it would have to, in the general case. Consider the possibility that someone has restricted access to the system catalogs, for instance. I would agree with this. pg_dumpall should do the 'set role' in its session as well. You have missed an important component of Stephen's original proposal, which was the point that something similar is needed on the restore side. This is a little bit tricky since the context at restore time is not necessarily the same as the context at dump time. When using an archive file it's not a problem: the behavior can be driven off a --role switch to pg_restore, and this is independent of what pg_dump did. In a dump to plain text, though, I'm not sure what to do. The simplest design would have pg_dump's --role switch control both what it does in its own connection to the source database, and what it puts into the output script. I'm not sure that's adequate though. This makes sense to me and I feel it's adequate. If necessary, people can post-process their .sql files using sed or something similar. That's made reasonably easy by having a 'set role' in the .sql file. I actively dislike the idea that pg_restore would modify the input stream from a text file, even if it was passed a --role switch. Is it worth having two different switches for the two cases? If we think it's a corner case to need different role IDs, we could just leave it like that and tell anyone who needs different behaviors that they have to go through an archive file and pg_restore. Stephen, you were the one who wanted this in the first place, what's your use-cases look like? My primary use cases are performing a pg_dump when logging in as one user but needing the permissions of another role, followed by loading the data into another system when logging in as one user and needing to set role first to another. In at least 90% of those cases, that role is postgres, and in the other 10% most, if not all, are the same role on both sides. There are a few cases where we might change the restore-as role away from the dumped-as role, but we're happy to use pg_restore to handle that, or take care of changing the role in the .sql file (which is what we tend to use, honestly) using sed or similar. Alot of this is driven from the fact that we don't allow admins to remotely connect directly as postgres (akin to disabling remote root logins in sshd_config via PermitRootLogin, and for the same reasons). They must authenticate and connect as their own user first and then use 'set role postgres;' to gain superuser rights. Not being able to have pg_dump do that set role has been quite frustrating as we use it extensively for transferring data between systems. Some other review nitpicking: I agree with the other comments. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Extending grant insert on tables to sequences
* Tom Lane ([EMAIL PROTECTED]) wrote: Stephen Frost [EMAIL PROTECTED] writes: * Jaime Casanova ([EMAIL PROTECTED]) wrote: updating the patch with one that only extends inserts. though, i haven't look at the col level privs patch yet. At least initially I wasn't planning to support column-level privileges for sequences, so I don't think it will affect you much. Do people think it makes sense to try and support that? USAGE certainly wouldn't be column-level in any case --- it'd be a privilege on the sequence as such. That end of it isn't the problem; the problem is that column-level privileges on the table make it hard to decide when to grant rights on the sequence, as I pointed out last time round: http://archives.postgresql.org/pgsql-hackers/2008-07/msg00624.php Ah, obviously I hadn't read far enough back about this patch. I agree that sequence USAGE should be granted when insert is granted on any column. One suggestion is that as the SQL spec indicates that a table-level revoke implies a revoke on all columns, we could have the revokation of the sequence permissisons done only on table-level revokation of insert and not on any individual column-level insert, even if that was the last column which insert rights were granted on. I have to admit that I'm not a big fan of that though because a given state on the table wouldn't imply a particular state for the sequence- it would depend on how you got there. The way the code is currently laid out for the column-level privileges, it wouldn't be that difficult to go through all of the other columns and check if this was the last insert being revoked, but I don't particularly like that either, and it strikes me as 99% of the time being wasted effort. I guess if we could check for and only go through that effort when there is a sequence in place with implicit grants it might not be too bad. As your patch appears more ready-for-commit than the column-level privileges patch, I wouldn't worry about what code might have to move around, that'll be for me to deal with in a re-sync with HEAD once your patch is committed. I think that's backwards. The above message raises serious concerns about whether the USAGE-granting patch can be implemented at all in the presence of column-level privileges. I think the right thing is to get column privileges in and then see if it's possible to implement USAGE-granting compatibly. I don't want to commit a patch that is clearly going to be broken when (not if) column privileges arrive. Now that I understand the situation better, I agree with you on this. I hadn't realized this patch was about implicit grants on sequnces. Sorry for the noise. Thanks, Stephen signature.asc Description: Digital signature
Re: Column level privileges was:(Re: [HACKERS] Extending grant insert on tables to sequences)
Jaime, * Jaime Casanova ([EMAIL PROTECTED]) wrote: On 7/25/08, Stephen Frost [EMAIL PROTECTED] wrote: Yes, I'm working on it hi, any work on it? may i help? If you look at the commitfest, I've posted my WIP so far there. Most of the grammer, parser, and catalog changes are there. There's a couple of bugs in that code that I'm working to run down but otherwise I think it's pretty good. I do need to add in the dependency tracking as well though, and that's what I'm planning to work on next. A piece which can be broken off pretty easily is adding support to track the columns used through to the executor so we can check the permissions in the right place. You should review Tom's #2 comment here: http://archives.postgresql.org/pgsql-patches/2008-05/msg00111.php Let me know if you'll be able to work on this or not. If not then I'll get to it after I'm happy with the other pieces of the patch. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] WIP: Column-level Privileges
Hi Markus, * Markus Wanner ([EMAIL PROTECTED]) wrote: Stephen Frost wrote: Comments welcome, apologies for it not being ready by 9/1. I'm planning to continue working on it tomorrow, and throughout September as opportunity allows (ie: when Josh isn't keeping me busy). I'm trying to review this patch. I could at least compile it successfully for now. That's a start at least. :) There have already been some comments from Tom [1]. You've mostly answered that you are going to fix these issues, but I'm pretty unclear on what the current status is (of patch 09/02). Can you please elaborate on what's done and what not? I would suggest you review the updated patch (linked off the wiki page) here: http://archives.postgresql.org/message-id/[EMAIL PROTECTED] It includes my comments about what's done and what's not. I reiterated much of it here as well: http://archives.postgresql.org/pgsql-hackers/2008-09/msg00263.php As this is still marked as WIP on the wiki page: what needs to be done until you consider it done? As mentioned in above, regression tests, documentation updates, dependency handling, and actually implementing the permission checks all remain. What I'm looking for feedback on are the changes to the grammer, parser, catalog changes, psql output, etc. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Proposed patch: make pg_dump --data-only consider FK constraints
* Gregory Stark ([EMAIL PROTECTED]) wrote: The other reason to think NOTICE might be better is that it's something which, if it occurs once, will always occur for that database. So a sysadmin will become inured to seeing WARNING on his backups. Are there any other warning conditions which could occur spontaneously that this would mask? Impartial on NOTICE vs. WARNING, it could go either way for me. One minor thought -- surely the main use case for data-only dumps is for importing into another brand of database. In which case the message seems a bit awkward -- it could talk generically about disabling or dropping the constraints and then have a hint to indicate how to do that with Postgres. I have to disagree strongly with this. We have multiple PG instances and often have cause to copy between them using data-only pg_dump. On the other side, I've never used pg_dump (data-only or not) to generate something to load data into a different database. Thanks, Stephen signature.asc Description: Digital signature
[HACKERS] [Patch Review] Copy column storage parameters on CREATE TABLE LIKE/INHERITS
Greetings, I've reviewed the patch here: http://archives.postgresql.org/message-id/[EMAIL PROTECTED] and am happy to report that it looks to be in good order. It has documentation and regression updates, is in context diff format, patches against current CVS with only some minor fuzz, and appears to work as advertised. I looked over the patch and could easily follow what was going on, did some additional testing beyond the regression tests, and reviewed the documentation changes. My only comment on this patch is that the documentation updates might include a link back to Section 53.2 for more information, and/or to the SET STORAGE portion of the alter table/alter column command documentation, since the only other 'storage' reference in create table's documentation is about table storage parameters. Thanks! Stephen signature.asc Description: Digital signature
Re: Column level privileges was:(Re: [HACKERS] Extending grant insert on tables to sequences)
Jaime, * Stephen Frost ([EMAIL PROTECTED]) wrote: * Jaime Casanova ([EMAIL PROTECTED]) wrote: On 7/25/08, Stephen Frost [EMAIL PROTECTED] wrote: Yes, I'm working on it hi, any work on it? may i help? If you look at the commitfest, I've posted my WIP so far there. Most of the grammer, parser, and catalog changes are there. There's a couple of bugs in that code that I'm working to run down but otherwise I think it's pretty good. I do need to add in the dependency tracking as well though, and that's what I'm planning to work on next. I've now added dependency tracking and worked out a few kinks in the code, both existing previously and from adding the dep tracking. I'd really like to simplify things in aclchk.c, perhaps by factoring out more common bits into functional pieces, but it's been kind of a bear so far. The dependency tracking is being done by continuing to treat the table as a single entity and just figuring out the total set (including all column-level permissions) of roles for the entire table, rather than introducing the sub-object concept. This requires a bit of extra effort when doing DDLs and GRANTs but simplifies the dependency tracking itself, especially since we have to keep track of both table-level permissions and column-level permissions seperately. I'm open to other suggestions/comments. If people feel the sub-object is a better approach, it would get somewhat more awkward because we'd have to handle the relation-level dependencies as well as the column-level ones. Not impossible to do, of course, but a bit more complicated than how it was done originally. A piece which can be broken off pretty easily is adding support to track the columns used through to the executor so we can check the permissions in the right place. Jamie, have you had a chance to work on this? It's next on my list and I'll start working on it tonight unless you've had a chance to get to it. Please let me know. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Proposal: move column defaults into pg_attribute along with attacl
* Tom Lane ([EMAIL PROTECTED]) wrote: I can think of a way around that: represent a default expression using classid = OID of pg_attribute, objid = OID of table, objsubid = column attnum. This is distinct from the column itself, which is represented with classid = OID of pg_class. It seems pretty ugly and potentially confusing though. Also there would be a compatibility issue for clients that examine pg_depend. Is it ugly enough to scuttle the whole concept of merging pg_attrdef into pg_attribute? Being able to handle a setup like that (though in pg_shdepend) might be necessary anyway, depending on the approach we're happiest with for column-level acl dependencies. Right now I've avoided it by just going through all of the columns and the table level grantors/grantees and listing them all when updating the dependencies. That keeps the dependency system simple but complicates other things for it. I posed a question previously about how people felt and don't recall there being any response as yet. Certainly, if we move to objid=table OID, objsubid=column attnum in pg_shdepend, it strikes me that we should do the same in pg_depend where appropriate, otherwise it'll just be a confusing inconsistancy. Honestly, I really disliked the code which assumed pg_attribute had no NULLable/toastable columns and used what seemed like pretty gruesome hacks to create pg_attribute structures. I'd also really like to get away from things which approached pg_attribute in that way, such as pg_attrdef. If we were to accept the pg_attrdef approach, why aren't we doing a pg_attracl table instead of adding a column to pg_attribute? Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Proposal: move column defaults into pg_attribute along with attacl
* Tom Lane ([EMAIL PROTECTED]) wrote: Stephen Frost [EMAIL PROTECTED] writes: If we were to accept the pg_attrdef approach, why aren't we doing a pg_attracl table instead of adding a column to pg_attribute? That's actually not an unreasonable question. If you were to do that then you could attach OIDs to the attribute ACLs, which might be a nicer representation in pg_shdepend than you were thinking of using. What bugs me about this is that it comes across as poor database design- both of these really are attributes of a column. We're creating seperate tables for each so we can induce a cleaner ID for them, which just isn't the right approach imv. This would also be another table to go deal with when a column is removed, and a less-than-obvious place to look for this information from the user's perspective. It's also the case that the items in these tables and the columns they're attached to really are one-to-one, there's no many-to-one or one-to-many relationship between them.. At the end of the day, this approach feels like more of a kludge to me to keep the dependency system simple rather than making the dependency system support the real-world system layout, which is that columns don't have their own IDs. Maybe we could approach this another way- what about creating a new table which is pg_attrcolids that has both pg_attrdef and pg_attracl rolled into it? Then at least we're accepting that we need a distinct ID for columns, but keeping them in one specific place? Is there a reason we would need a seperate ID for each? It also strikes me to wonder about possible future support for re-ordering columns, though I don't immediately see a way to use this as a step towards supporting that. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Proposal: move column defaults into pg_attribute along with attacl
Tom, * Tom Lane ([EMAIL PROTECTED]) wrote: Markus Wanner [EMAIL PROTECTED] writes: ISTM that we should at least combine defaults and ACLs then, as proposed by Stephen. Huh? Maybe I missed something, but I didn't think that was suggested anywhere. I had suggested a single table, with an OID, which would house anything that needed a seperate OID for columns (defaults and ACLs currently) in [EMAIL PROTECTED] It's not a completely thought-through solution, just something that struck me as a more general way of handling these situations (assuming we have more in the future and don't want to give each one its own table). If putting them together implies we have to complicate things to add some way to seperate them then it might not be worth it. Having a seperate table for each means we can use the table's OID to seperate them though. I still dislike this possible continued growth of the catalogs. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Proposal: move column defaults into pg_attribute along with attacl
Markus, * Markus Wanner ([EMAIL PROTECTED]) wrote: What does the subobject column for pg_shdepend buy us? Tracking column-level ACL dependencies rather than having those dependencies only be at the table-level. This complicates pg_shdepend some, but simplifies the dependency handling in the ACL area and in handling table/column drops. I'm still not a fan of having column-level deps handled differently between pg_shdepend and pg_depend, but that's not something which has to be addressed directly by the column-level privs patch. Perhaps once it's done I'll do a proof-of-concept for removing pg_attdef. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Proposal: move column defaults into pg_attribute along with attacl
* Markus Wanner ([EMAIL PROTECTED]) wrote: Stephen Frost wrote: * Markus Wanner ([EMAIL PROTECTED]) wrote: What does the subobject column for pg_shdepend buy us? Tracking column-level ACL dependencies rather than having those dependencies only be at the table-level. This complicates pg_shdepend some, but simplifies the dependency handling in the ACL area and in handling table/column drops. With a separate table? Or as part of pg_attribute (which can handle NULL and VARLENA attributes now?) As part of pg_attribute.. Having a seperate table would be an alternative to adding a column to pg_shdepend. Stephen signature.asc Description: Digital signature
Re: [HACKERS] The Axe list
* Gregory Stark ([EMAIL PROTECTED]) wrote: Robert Haas [EMAIL PROTECTED] writes: CREATE AGGREGATE array_accum (anyelement) CREATE OR REPLACE FUNCTION array_enum(anyarray) Have you actually tried these functions on large data sets? They're not in the same performance league as intagg. Your array_accum is O(n^2)! array_accum itself may also end up in core, if I have my dithers. It makes psql's job to display column-level privs in a nice way alot easier, and there's been quite a few cases where I've used it outside of that, along with others. It'd be nice to have there. That said, once it's in, we really need to rework it to not suck(tm). I wrote a patch to do just that quite some time ago, but it required some things in core that were ugly to expose to any aggregation function (as I recall). If we made that only available to built-in's, then we might be able to have array_accum in core *and* have it be fast. :) It's a goal anyway. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] array_agg and array_accum (patch)
Jeff, * Jeff Davis ([EMAIL PROTECTED]) wrote: 2) ARRAY_ACCUM() -- Returns empty array on no input, and includes NULL inputs. Excellent.. I added it the easy way (from the online docs), but that's clearly not at all efficient and was going to try and fix it, for psql to use with the column-level privs patch. It'd be great to use a more efficient mechanism like this, and to remove adding it from my patch (erm, it's only one line currently, but it would have been alot more eventually :). I havn't actually reviewed the code at all, but +1 in general to adding this to core. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Please make sure your patches are on the wiki page
KaiGai, et al, * KaiGai Kohei ([EMAIL PROTECTED]) wrote: Stephen, what is the status of your efforts? I've now got it passing the base regression tests with the actual logic included in the path. That doesn't mean it works completely, of course, but I feel like I'm making progress. Feedback, as always, is appreciated. The latest one I could found is the colprivs_wip.20080902.diff.gz. Do you have any updated one? Please find the latest attached, against current CVS head as of a few minutes ago (including the change to heap_modify_tuple). I'll also update the commitfest wiki w/ this once it's hit the archives. Thanks, Stephen colprivs_wip.20081101.diff.gz Description: Binary data signature.asc Description: Digital signature
Re: [HACKERS] WIP: Column-level Privileges
Markus, * Markus Wanner ([EMAIL PROTECTED]) wrote: Sorry, this took way longer than planned. Beleive me, I understand. :) testdb=# GRANT TRUNCATE (single_col) ON test TO malory; GRANT This has been fixed in the attached patch. Some privilege regression tests currently fail with your patch, but I think that's expected. All regression tests should pass now. Documentation and new regression tests for column level privileges are still missing. If you want, Stephen, I can work on that. If you could work on the documentation, that'd be great! I've updated the regression tests to include some testing of the column level privileges. Feel free to suggest or add to them though, and if you find anything not working as you'd expect, please let me know! There are a few outstanding items that I can think of- The error-reporting could be better (eg, give the specific column that you don't have rights on, rather than just saying you don't have rights on the relation), but I wasn't sure if people would be happy with the change to existing error messages that would imply. Basically, rather than getting told you don't have rights on the relation, you would be told you don't have rights on the first column in the relation that you don't have the necessary rights on. It's a simple change, if people are agreeable to it. Having it give the table-level message only when there aren't column-level privileges is possible, but makes the code rather ugly.. Documentation, of course. More testing, more review, etc, etc, making sure everything is working as expected, more complex queries than what I've done to make sure things happen correctly. Tom has me rather nervous based on his previous comments about the rewriter/optimizer causing problems, and I barely touched them.. I also wonder if you could use joins or something to extract information about columns you're not supposed to have access to, or where clauses, etc.. Anyhow, updated patch attached. Thanks, Stephen colprivs_wip.2008110102.diff.gz Description: Binary data signature.asc Description: Digital signature
Re: [HACKERS] WIP: Column-level Privileges
Markus, et al, * Stephen Frost ([EMAIL PROTECTED]) wrote: I also wonder if you could use joins or something to extract information about columns you're not supposed to have access to, or where clauses, etc.. welp, I've done some additional testing and there's good news and bad, I suppose. The good news is that when relations are join'd, they go through expandRelation, which adds all the columns in that relation to the 'required' set, so you have to have rights to all columns on a table to join against it in the normal way. On the other hand, you can just select out the columns you have access to in a subquery and then join against *that* and it works. updates with where clauses and inserts-with-selects seem to work correctly though, which is nice. A case I just realized might be an issue is doing a 'select 1 from x;' where you have *no* rights on x, or any columns in it, would still get you the rowcount. That might not be too hard to fix though, I'll look into it tomorrow sometime. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] WIP: Column-level Privileges
* Tom Lane ([EMAIL PROTECTED]) wrote: Stephen Frost [EMAIL PROTECTED] writes: ... A case I just realized might be an issue is doing a 'select 1 from x;' where you have *no* rights on x, or any columns in it, would still get you the rowcount. Well, if you have table-level select on x, I would expect that to work, even if your privs on every column of x are revoked. If the patch doesn't get this right then it needs more work ... Table-level select on x is equivilant to having column-level select on every column, per the spec. The issue here, that I'm planning to fix shortly, is that you could get a rowcount without having table-level or column-level select rights on the table. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] WIP: Column-level Privileges
* Tom Lane ([EMAIL PROTECTED]) wrote: Stephen Frost [EMAIL PROTECTED] writes: ... A case I just realized might be an issue is doing a 'select 1 from x;' where you have *no* rights on x, or any columns in it, would still get you the rowcount. Well, if you have table-level select on x, I would expect that to work, even if your privs on every column of x are revoked. If the patch doesn't get this right then it needs more work ... Attached patch has this fixed and still passes all regression tests, etc. Thanks, Stephen colprivs_wip.2008110201.diff.gz Description: Binary data signature.asc Description: Digital signature
Re: [HACKERS] WIP: Column-level Privileges
Markus, * Markus Wanner ([EMAIL PROTECTED]) wrote: Stephen Frost wrote: Attached patch has this fixed and still passes all regression tests, etc. Do you have an up-to-date patch laying around? The current one conflicts with some CVS tip changes. No, not yet. I suspect the array_agg patch is conflicting (which is a good thing, really) and the addition of array_agg by my patch can now be removed. Testing should be done to ensure nothing changed wrt output, of course, but I'm not too worried. I can probably update it this weekend, depending on how things are going with the newborn (she's only 4 days old at this point, after all.. :). Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] WIP: Column-level Privileges
Alvaro, * Alvaro Herrera ([EMAIL PROTECTED]) wrote: I had a look at aclchk.c and didn't like your change to objectNamesToOids; seems rather baroque. I changed it per the attached patch. I've incorporated this change. Moreover I didn't very much like the way aclcheck_error_col is dealing with two or one % escapes. I think you should have a separate routine for the column case, and prepend a dummy string to no_priv_msg. I can do this, not really a big deal. Why is there a InternalGrantStmt.rel_level? Doesn't it suffice to check whether col_privs is NIL? No, a single statement can include both relation-level and column-level permission changes. The rel_level flag is there to indicate if there are any relation-level changes. Nothing else indicates that. Is there enough common code in ExecGrant_Relation to justify the way you have it? Can the common be refactored in a better way that separates the two cases more clearly? I've looked at this a couple of times and I've not been able to see a good way to do that. I agree that there's alot of common code and it seems like there should be a way to factor it out, but there are a number of differences that make it difficult. If you see something I'm missing, please let me know. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Adding support for SE-Linux security
* Robert Haas (robertmh...@gmail.com) wrote: One of the major and fundamental stumbling blocks we've run into is that every solution we've looked at so far seems to involve adding SE-Linux-specific checks in many places in the code. I've really got to take exception to this. I've only been following along and not really participating because, to be honest, I'm frustrated with how this has gone down. In the end there were at least two patches, in my view, that *didn't* involve adding SE-Linux-specific checks everywhere. The patch that I reviewed that got thrown out by Tom, and the original PGACE framework. Both of those added alot of *hooks*, because they were necessary, but nothing made those hooks particularly SELinux-specifc. We're hearing alot about things being SELinux-specific from people who also profess to not know SELinux. Indeed, as I recall, the patch I reviewed was primairly trying to just addess pulling out the hooks necessary for the existing PostgreSQL security model. Very little of it was SE-Linux specific *anything*. I contend that while the specific hooks which would be added *in places that don't already have checks* (in most places, the hook was added to replace an existing check) are hooks that then make sense for SELinux, they would also make sense for other frameworks. They may also not be a complete list, but once the *framework* is in place, adding new hooks (presuming another model would like a hook somewhere that SELinux and the existing PG security framework don't) should not require some kind of forklift upgrade. It would be nice if it were possible to use the exist permissions-checking functions and have them check a few more things while they're at it, but it's looking like that won't be feasible, or at least no one's come up with a plausible design yet. The problem is that the existing permissions-checking is done all over the place. That's not ideal from the get-go, in my opinion. Unfortuantely, when we moved all of the permissions-checking to a single place, it required knowing about alot of the backend, which Tom took exception to. I agree that's frustrating, but I don't see it as a particular reason to throw out the entire concept of a modular security framework. Perhaps we need to better expose just those pieces the security framework cares about from the other parts of the backend- it's entirely likely that the reason it has to know about everything is that, due to where all the existing security checks are, they just (ab)used the fact that they had access to that information at hand for that object type. Consequently there are checks spread throughout the code, which definitely complicates both validation and maintenance. One question I have is - are the places where those checks are placed specific to SE-Linux, or would they be applicable to any sort of label-based MAC? The patch which I worked with Kaigai on was specifically geared to first try to get a patch which addressed the existing PG security model in a modular way, to allow additional hooks to be added later in places which needed them, and to provide the information for other models to make a decision about the permission. I don't feel it was particularly SE-Linux specific at all, but rather a first step towards being able to support something beyond the model we have today (anything..). Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] SE-PostgreSQL/Lite Review
* Greg Smith (g...@2ndquadrant.com) wrote: I personally feel that Steven Frost's recent comments here about how the PostgreSQL code makes this harder than it should be really cuts to the core of a next step here. The problem facing us isn't is SEPostgreSQL the right solution for providing external security checks?; it's how can the PostgreSQL code be improved so that integrating external security is easier? Looking Thanks for that support, Greg. This was what I was principally trying to do with KaiGai and the commitfest patch I reviewed of his last round. Unfortunately, the committer comments I received on that patch didn't help us outline a path forward, just declared that the approach in the current patch wasn't workable. My, now much more optimistic thanks to our meeting, view is that the concept of abstracting the access controls is solid and a necessary first step; but we need to find a better way to implement it. Also thanks to our discussion, I've got a much better handle on how SELinux and the general secuirty community views PostgreSQL (and the Linux kernel for that matter)- it's an application which consists of a set of object managers. That then leads into an approach to address at least some of Tom's comments: Let's start by taking the patch I reviewed and splitting up security/access_control.c along object lines. Of course, the individual security/object_ac.c files would only include the .h's that are necessary. This would be a set of much smaller and much more managable files which only know about what they should know about- the object type they're responsible for. Clearly, code comments also need to be reviewed and issues with them addressed. I'm also not a fan of the skip permissions check arguments, which are there specifically to address cascade deletions, which is a requirment of our PG security model. I'd love to see a better solution and am open to suggestions or thoughts about what one would be. I know one of KaiGai's concerns in this area was performance, butas we often do for PG, perhaps we should consider that second and first consider what it would look like if we ignored performance concerns. This would change skip permissions check to what object is being deleted, and am I a sub-object of that?. I don't feel like I've explained that well enough, perhaps someone else could come up with another solution, or maybe figure out a better way to describe what I'm trying to get with this. Regarding the special-purpose shims- I feel there should be a way for us to handle them better. This might be a good use-case for column-level privileges in pg_catalog. That's pure speculation at the moment tho, I havn't re-looked at those shims lately to see if that even makes sense. I don't like them either though, for what it's worth. Regarding contrib modules- I view them as I do custom code which the user writes and loads through dlopen() into the backend process- there should be a way for it to do security right, but it ultimately has to be the responsibility of the contrib module. Admins can review what contrib modules have been made SELinux/etc aware and choose to install only those which they trust. Maybe Bruce or Steven can champion that work. See above? ;) I've had enough of a break from this and our discussion has revitalized me. I certainly welcome Bruce's comments, as well as anyone else. I have to be honest and say that I'm not optimistic that this is possible or even a good idea to accomplish in the time remaining during this release. While I agree with you, I wish you hadn't brought it up. :) Mostly because I feel like it may discourage people from wanting to spend time on it due to desire to focus on things which are likely to make it into the next release. That being said, I don't feel that'll be an issue for KaiGai or myself, but getting someone beyond us working on this would really be great, especially yourself and/or Bruce. On my side, in addition to helping coordinate everyone pushing in the same direction, I'll also continue trying to shake out some sponsorship funding for additional work out of the people in this country it would benefit. It seems I'm going to keep getting pulled into the middle of this area regularly anyway. Thank you for that. I'm planning to do the same and will certainly let people know, to the extent possible, of anything I'm able to dig up. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Adding support for SE-Linux security
Tom, * Tom Lane (t...@sss.pgh.pa.us) wrote: It's been perfectly clear since day one, and was reiterated as recently as today http://archives.postgresql.org/message-id/4b21757e.7090...@2ndquadrant.com that what the security community wants is row-level security. Yes, they do want row-level security. That being said, KaiGai, and others, have pointed out time and time over again that SEPG without row-level security is still useful. Additionally, I see absolutely no way that PG would accept a full SEPG+PGACE+row-level security, etc, patch in as one whole patch, ever. I have extreme doubt it would even be something done over one *release*. That all aside, for the moment, I feel that we should begin a 'two-prong' attack here. First, continue down the path that I've started to lay out for SEPG. Second, let's hash out a design for row-level security using the existing PG security model; ideally using the best features and design decisions of the numerous row-level security systems already implemented by the major SQL vendors today. I'll start a new thread on this specific topic to hopefully pull out anyone who's focus is more on that than on SEPG. The proposals to make SEPostgres drive regular SQL permissions never came out of anyone from that side, they were proposed by PG people looking for a manageable first step. I do not believe this to be accurate. Josh, were you able to find any public documentation on Trusted Rubix (is that the right name?)? The RDBMS security policy hashed out on the SELinux list during the discussion of Rubix and SEPG certainly included support for table-level objects, did it not? I expect that the SELinux list contributors would have pointed out if they didn't feel that was at all valuable. Perhaps what is at issue is the terminology being used here though, or the approach to enforment being considered. Part of the discussion at the BWPUG meeting involved the option for SEPG to be a more-restrictive only model in it's implementation. Essentially, this means that all permissions handling is done the same as it is today, except that once the PG model has decided an action is allowed, SEPG kicks in and does any additional checking of the action being requested it wants and may deny it. At the end of the day, I don't feel that it really changes the architecture of the code though. Perhaps users of SELinux will always want that, but the argument we've heard time and time again here is that this should be a generalized approach that other security managers could hook into and use. To do that, I feel we first have to start with the PG model, which *does* care about all the SQL permissions. Let's extract the various complaints and concerns about SELinux that have been thrown around this list and instead consider our first client of the PG modular security framework to be the existing PG SQL permissions system. If we can agree to that, then it's clear that we can't just hand-wave the requirement that it be capable of driving the regular SQL permissions. Whatever you might believe about the potential market for SEPostgres, you should divide by about a hundred as long as it's only an alternate interface to SQL permissions. See particularly here: http://wiki.postgresql.org/wiki/SEPostgreSQL_Review_at_the_BWPUG#Revisiting_row-level_security Without it, it's questionable whether committing the existing stripped-down patch really accomplishes anything --- how much clearer can they be? Again, let's please address row-level security first at the PG level. That was recommended previously by many on this list and is clearly a useful feature which can stand alone in any case. If you're not prepared to assume that we're going to do row level security, it's not apparent why we should be embarking on this course at all. And if you do assume that, I strongly believe that my effort estimate above is on the optimistic side. I do assume we're going to do row level security, but I do not feel that we need to particularly put one in front of the other. I also feel that SEPG will be valuable even without row-level security. One of the realms that we discussed at BWPUG for this is PCI compliance. I'm hopeful Josh will have an opportunity to review the PCI compliance cheat-sheet that I recall Robert Treat offering and comes to agreement that SEPG w/o row-level security would greatly improve our ability to have a PCI compliant system backed with PG. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] SE-PostgreSQL/Lite Review
KaiGai, * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: (1) Whether the framework should host the default PG model, not only enhanced security features, or not? This patch tried to host both of the default PG model and SELinux. But, the default PG model does not have same origin with label-based mandatory access controls, such as SELinux, from a historical angle. So, this patch needed many of unobvious changes to the core routines. I certainly understand what you're saying here. However, I feel that these changes to the core of PG are good, and are taking PG in the right direction to have a much clearer and better implemented security infrastructure. The fact that you've found a number of odd cases (duplicate checking, odd failure cases due to checks being done at various parts of the process, etc) is a testement that this is bringing the PG code forward even without the addition of SELinux support. I realize that makes it more work for you and I don't envy you that. (2) Whether the framework should be comprehensive, or not? This patch tried to replace all the default PG checks in the core routines from the begining. It required to review many of unobvious changes in the core routine. Because I've been repeatedly pointed out to keep changeset smaller, I'm frightened for the direction. The coverage of the latest SE-PgSQL/Lite patch is only databases, schemas, tables and columns. I think it is a good start to deploy security hooks on the routines related to these objects, rather than comprehensive support from the begining. I don't believe we will get support to commit a framework which is intended to eventually support the PG model (following from my answer to #1) in a partial form. Perhaps it would be good to split the patch up on a per-object-type basis, to make it simpler/easier to review (as in, patch #1: database_ac.c + changes to core for it; patch #2: table_ac.c + changes to core for it, etc; earlier patches assumed to be already done in later patches), but the assumption will almost certainly be that they will all be committed to wonderful CVS at the same time. (3) In the future, whether we should allow multiple enhanced securities at the same time, or not? It was not clear in the previous commit fest. But, in fact, Linux kernel does not support multiple security features in same time, because it just makes security label management complex. (It also have another reasons, but just now unlear.) I don't think we have any good reason to activate multiple enhanced securities at same time. To be honest, I feel that this will just fall out once we get through doing #1. I've just heard from Casey (author of the SMACK security manager, an alternative to SELinux) that the PGACE framework (which is what we suggested he look at) would be easily modified to support SMACK. I feel the same would be true of what we're talking about in #1. If you disagree with that, please let me know. I think most of folks can agree with (2) and (3). But I expect opinion is divided at (1). For example, if we host the default PG checks to create a new database, all the existing checks shall be moved as follows: http://code.google.com/p/sepgsql/source/browse/trunk/pgsec/src/backend/security/access_control.c?r=2273#1430 In addition, it has to return the security context to be assigned on the new database, when it hosts SELinux. Even if this rework is limited to four kind of object classes, I'm worry about it overs an acceptable complexity. I feel that what was at issue is that the complexity of access_control.c was far too great- because *everything* was in that one file. Splitting access_control.c into smaller files would make them more managable, and if we implement them all in a similar manner using a similar policy for function names, arguments, etc, then once the first object type is reviewed, the others will go more easily for the reviwer. If the security framework does *not* support the SQL model, I think we'll get push back from the community that it clearly couldn't be used by any other security manager generically without alot of potential changes and additional hooks that could end up bringing us all the way to being capable of supporting the SQL model anyway. I'm certainly willing to hear core indicate otherwise though. Thanks again, KaiGai. Stephen signature.asc Description: Digital signature
Re: [HACKERS] Adding support for SE-Linux security
Magnus, * Magnus Hagander (mag...@hagander.net) wrote: On Fri, Dec 11, 2009 at 05:45, Tom Lane t...@sss.pgh.pa.us wrote: It's been perfectly clear since day one, and was reiterated as recently as today http://archives.postgresql.org/message-id/4b21757e.7090...@2ndquadrant.com that what the security community wants is row-level security. The If that is true, then shouldn't we have an implementation of row level security *first*, and then an implementation of selinux hooks that work with this row level security feature? Rather than first doing selinux hooks, then row level security, which will likely need new and/or changed hooks... The proposal we're currently grappling with is to pull all the various checks which are sprinkled through our code into a single area. Clearly, if that work is done before we implement row-level security, then the patch for row-level security will just add it's checks in the security/ area and it'd be then easily picked up by SELinux, etc. I'm not convinced that row level security is actually that necessary (though it's a nice feature, with or without selinux), but if it is, it seems we are approaching the problem from the wrong direction. It has to be implemented independent of the security/SELinux/etc changes in any case, based on what was said previously.. So I don't particularly understand why it matters a great deal which one happens first. They're independently useful features, though both are not nearly as good on their own as when they are combined. Sorry, I just don't see this as a cart-before-the-horse case. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Adding support for SE-Linux security
David, * David P. Quigley (dpqu...@tycho.nsa.gov) wrote: So I downloaded and read through the PCI DSS document (74 pages is pretty light compared to NFSv4.1 hehe...) and There are several areas there where I think strong access controls in the database will not only fulfill the requirement but provide much stronger guarantees than can be provided from the application server alone. Thanks for taking a look! That sounds like excellent news. My apologies for attributing the action item to the wrong individual. :) The requirements in section 7 can definitely benefit from SEPG. I don't mean to be a pain, and we're all busy, but perhaps you could include a short description of what 'requirements in section 7' are.. It would help keep the mailing list archive coherent, and be simpler for folks who aren't familiar with PCI to play along. A link to the specific PCI DSS document you looked at would be an alternative, tho not as good as a 'dumbed-down' description. ;) That would at least avoid confusion over which document, since I presume there's more than one out there. Thanks again for looking over this! Treat, you've dealt alot with PCI in your commercial work; could you comment on this for the benefit of the list? I don't doubt David in the least, but it never hurts to have someone as lucky as yourself in frequent dealings with PCI compliance to provide any additional insight. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Adding support for SE-Linux security
* David P. Quigley (dpqu...@tycho.nsa.gov) wrote: On Fri, 2009-12-11 at 09:32 -0500, Robert Haas wrote: I think that we should try to move the PG default checks inside the hook functions. If we can't do that cleanly, it's a good sign that the hook functions are not correctly placed to enforce arbitrary security policy. Furthermore, it defeats what I think would be a good side goal here, which is to better modularize the existing code. So from the meeting on Wednesday I got the impression that Steve already did this. However it was rejected because too much information was need to be passed around. KaiGai did all the work, but it was my suggestion to go down this route and I reviewed KaiGai's patch to do it. The specific 'review/rejection' email is here: http://archives.postgresql.org/message-id/10495.1255627...@sss.pgh.pa.us I gathered and I could be wrong but the reason for this is that instead of developing proper abstractions for the security code people made use of whatever local stuff was available to them at that location. That's certainly one concern I continue to have, but as I've been rereading the threads I'm less confident it's a huge problem- the issue seemed to be more about the single access_control.c knowing about the entire PG world. With the X-ACE work that Eamon Walsh did he did exactly what you're saying. He figured out the object model for the X-Server and created the hook framework. In the merge of the hook framework he also moved the existing X server security model into the framework. It's great to hear specific examples of other projects which have gone through this headache and come out the other side better for it. What I would suggest is that you develop a version of that patch that is stripped down to apply to only a single object type (databases? tables and columns - these might have to be together??) and which addresses Tom's criticisms from the last time around, and post that (on a new thread) for discussion. That will be much easier to review (and I will personally commit to reviewing it) but should allow us to flush out some of the design issues. If we can get agreement on that as a concept patch, we can move on to talking about which object types should be included in a committable version of that patch. They may have been said before but what exactly are the design issues? Unfortunately, design isn't nearly as well defined a term as one would hope. I believe KaiGai's latest suggestion (which is probably what his original PGACE implementation was, but I'm going to avoid looking, the community has enough egg on it's face already wrt this :/) is a good approach, along with splitting the huge access_control.c file into per-object pieces. That's a design change, tho perhaps not the kind of one others who have commented on the design were thinking about when they made those statements. Basically, there's the design of the code layout and how each piece knows about the other pieces (through header files, etc), and then there's the design of the function calls/ABI and actual code paths which are taken when the code is executed (which doesn't particularly care how the code is laid out in the source tree). I feel like the design issues raised have been more about the former and less about the latter. The main concern I hear is that people are worried that this is an SELinux specific design. I heard at the meeting on Wednesday that the Trusted Extensions people looked at the framework and said it meets their needs as well. If thats the case where does the concept that the design is SELinux specific stem from? We've asked Casey Schaufler the developer of another label based MAC system for Linux to look at the hooks as well and make a statement about their usability. Hope I didn't steal your thunder wrt Casey! Thanks again. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] SE-PostgreSQL/Lite Review
KaiGai,
* KaiGai Kohei (kai...@kaigai.gr.jp) wrote:
As Rober Haas already suggested in another message, my patch in the last
commit fest is too large. It tried to rework anything in a single patch.
The per-object-type basis make sense for me.
Agreed.
In my cosmetic preference, ace_ is better than ac_. The 'e' means
extendable, and ace feels like something cool. :-)
No complaints here.. I just hope this doesn't end up being *exactly*
the same as your original PGACE patches.. I'd feel terrible if we
weren't able to at least improve something with this extremely long and
drawn our process!
And I'd like to store these files in backend/security/ace/*.c, because
backend/security will also store other security modules!
This is perfectly reasonable in my view. No complaints here.
src/
+ backend/
+ security/
+ ace/ ... access control framework
+ sepgsql/ ... selinux specific code
+ smack/ ... (upcoming?) smack specific code
+ solaristx/ ... (upcoming?) solaris-tx specific code
Looks good to me. Perhaps we'll have a smack/ patch showing up very
shortly as well..
The reason why I prefer the default PG check + one enhanced security at
most is simplification of the security label management.
If two label-based enhanced securities are enabled at same time,
we need two field on pg_class and others to store security context.
Ah, yes, I see your point must more clearly now. This sounds reasonable
to me.
In addition, OS allows to choose one enhanced security at most eventually.
In my image, the hook should be as:
Value *
ac_database_create([arguments ...])
{
/*
* The default PG checks here.
* It is never disabled
*/
/* enhanced security as follows */
#if defined(HAVE_SELINUX)
if (sepgsql_is_enabled())
return sepgsql_database_create(...);
#elif defined(HAVE_SMACK)
if (smack_is_enabled())
return smack_database_create(...);
#elif defined(HAVE_SOLARISTX)
if (soltx_is_enabled())
return soltx_database_create(...);
#endif
return NULL;
}
We can share a common image image?
If all of these security modules make sense as only-more-restrictive,
then I have no problem with this approach. Honestly, I'm fine with the
initial hooks looking as above in any case, since it provides a clear
way to deal with switching out the 'default PG checks' if someone
desires it- you could #if around that block as well. As it's the
principle/primary, and I could see only-more-restrictive being more
popular, I don't mind having that code in-line in the hook. The check
itself is still being brought out and into the security/ framework.
I do think that, technically, there's no reason we couldn't allow for
multiple only-more-restrictive models to be enabled and built in a
single binary for systems which support it. As such, I would make those
just #if defined() rather than #elif. Let it be decided at runtime
which are actually used, otherwise it becomes a much bigger problem for
packagers too.
Thanks!
Stephen
signature.asc
Description: Digital signature
Re: [HACKERS] SE-PostgreSQL/Lite Review
Greg, * Greg Smith (g...@2ndquadrant.com) wrote: I think we need a two pronged attack on this issue. Eventually I think someone who wants this feature in there will need to sponsor someone (and not even necessarily a coder) to do a sizable round of plain old wording cleanup on the comment text of the patch. For the benefit of this list (this was already discussed some at the BWPUG meeting), I agree with Greg on this and am actively looking to try and help (either directly in my spare time or indirectly as a sponsor). If anyone else is interested or can do so as well, that would be great! Please speak up! Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Adding support for SE-Linux security
* Robert Haas (robertmh...@gmail.com) wrote: I'll stop here because I see that Stephen Frost has just sent an insightful email on this topic as well. Hmm, maybe that's the Steve you were referring to. I have doubts- but then I don't ever see my comments as insightful for some reason. ;) Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Adding support for SE-Linux security
Robert, * Robert Haas (robertmh...@gmail.com) wrote: I actually have an idea how to solve the problem in this particular case, but I'm reluctant to say what it is because I'm not sure if I'm right, and at any rate *I don't want to write this patch*. As far as crap goes, I'd have to put this at the top. If you're not willing to share ideas, then I may have to reconsider my personal feeling on if you should be a committer or not. No one is asking you to write the patch. We all know that we can be wrong (I tend to be more wrong than most), and we all hate to jerk people around, but I feel it's far worse to self-censor discussion on ideas. It's also about the worst form of rock-management that I think I could come up with in an open source community. If you don't share your idea, yet you feel that it's right, and see nothing to dissuade you from that position (after all, we can't present an argument for or against it if we don't know what it is), then I find it likely that you're going to constantly be comparing patches presented to the ideal one in your head based on your idea and we'd never get there. If you're not willing to discuss your idea, then I would ask that you not be involved in either this discussion or review of the patch. Rock-management, for those not familiar with the term, is essentially asking I need a rock, and then, when presented with a rock saying sorry, that's not the rock I wanted. This doesn't provide any insight into what kind of rock you're looking for. Rest of the I don't want to write it whine omitted. No one's asking you to. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Adding support for SE-Linux security
All, * Robert Haas (robertmh...@gmail.com) wrote: If we design a security abstraction layer, the interfaces need to really be abstraction boundaries. Passing the table OID and then also the tablespace OID because PG DAC needs that to make its access control decision is crap. Now, to address the small useful bit of this mail- I tend to agree with this. I'm not convinced there's an alternative, but I'd like to throw out a couple of my ideas on how it could be addressed. I'd like to solicit for feedback on these. First off, we have alot of the information available from the catalog. Basically, given an OID, we should be able to find out the other information associated with that OID (such as what kind of object it is, what tablespace it resides in, etc). OID isn't sufficient, however, as we know from the dependency system. To address this, we would need OID and SubOID. Now, any information which we can derive from those should not be included in the API. To be honest, I don't think we've actually been all that bad about this, but I'll reserve any definitive answer until I've gone back through the API we have specifically thinking about this issue. On further thought, I'm probably wrong and should have caught this during my review. Sorry. Second, the information we *don't* have from above is generally information about what the requesting action is. For example, when changing ownership of an object, we can't possibly use introspection to find out the role which is on the receiving end, since at that time we've just learned it from the parser. Now, we might build an entire new object, pass the result of this action OID to the security system and ask it can we change OID X into OID Y?, but I don't particularly like it. We really don't want to do any *changes* to things until after we've determined the permissions allow for it, and I'm not sure how to get around that without building an independent introspection system for what might be. Perhaps we're comfortable enough saying we'll just rollback the command if it turns out to have not been allowed but I just don't like it. Feels like a higher risk solution to me. I don't see a way to get around the second piece since what information is needed about the action is typically action-specific. Perhaps we could have an 'action-ID' (uh, we have an ID per command already, no? Probably doesn't fit the mold close enough though), and then a way to query information about what is this action trying to do?. Doesn't seem likely to be very clean though. Other thoughts? Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Adding support for SE-Linux security
David, * David P. Quigley (dpqu...@tycho.nsa.gov) wrote: So the document I read is linked below [1]. Great, thanks again. [agree with all the rest] It is definitely good to have a second opinion on this since I've just only started reading the PCI compliance documents. I'm definitely not an expert in PCI compliance but from what I've read there are definite benefits for using SEPG or PG-ACE with a special security module in making much stronger guarantees about who and what can touch the card data. Indeed. The other nice piece about getting the opinion of Treat (or others who have to deal with PCI) is that while the PCI documentation says what you're supposed to do, the PCI folks also have auditing requirments (as in, a third-party vendor has to audit your system, and there are required scans and scanning tools, etc) which don't always marry up to what they say they require. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Adding support for SE-Linux security
* David P. Quigley (dpqu...@tycho.nsa.gov) wrote: Yea I never asked Stephen if he goes by Stephen or Steve when I met him on Wednesday. I guess calling him Steve is me being a bit presumptuous :) Oh, either is fine, tho people will probably follow a bit better if you say Stephen. As a reminder- KaiGai's the one who did all the actual code, I just tried to steer him in a direction I thought made sense to make it easier to get core to accept it, and reviewed the results of his work. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] SE-PostgreSQL/Lite Review
Josh, * Joshua Brindle (met...@manicmethod.com) wrote: Stephen Frost wrote: I do think that, technically, there's no reason we couldn't allow for multiple only-more-restrictive models to be enabled and built in a single binary for systems which support it. As such, I would make those just #if defined() rather than #elif. Let it be decided at runtime which are actually used, otherwise it becomes a much bigger problem for packagers too. It isn't just a case of using #if and it magically working. You'd need a system to manage multiple labels on each object that can be addressed by different systems. So instead of having an object mapped only to system_u:object_r:mydb_t:s15 you'd also have to have it mapped to, eg., ^ for Smack. I'm not sure I see that being a problem.. We're going to have references in our object managers which make sense to us (eg: table OIDs) and then a way of mapping those to some label (or possibly a set of labels, as you describe). We might want to reconsider the catalog structure a bit if we want to support more than one at a time, but I don't see it as a huge problem to support more than one label existing for a given object. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Adding support for SE-Linux security
* Robert Haas (robertmh...@gmail.com) wrote: OK, it's clear that I've handled this badly. Sorry. My fear (however unjustified) was that someone would go and rewrite the patch based on an opinion that I express whether they agree with it or not. That's always going to be a risk in an open-discussion environment. Additionally, that's exactly what happened to me last go round- KaiGai rewrote the patch based on my ideas and suggestions, and the result was summarairly tossed out by Tom. Did it suck? Yes, heavily, and it frustrated me to the point that I specifically asked to not be the reviewer for SEPG during the next commitfest. At the same time, what KaiGai or others spend time on is up to them (and/or their employers). I sincerely hope that even if you suggest an approach down the road unrelated to this on some other patch you're reviewing, and then you see the results and say whoah, that's horrible, and should never be committed, that you understand none of us would want you to commit it. Sharing your ideas or putting out suggestions isn't a commitment on your part that you'll commit the results when someone else rights it. Heck, I bet you've been down that road on your own projects and come to the realization at the end of err, bad idea and not committed it. Allow me to say, my apologies, I feel like I may have over-reacted a bit for my part as well. So with that said, the idea I had was to try to pass around pre-existing data structures related to the objects on which access control decisions are being made, rather than Oids. That thought had crossed my mind as well, but I wasn't convinced that would actually be seen as a signifigantly different API to just having the arguments passed inline... Then again, using structures does allow you to add to them without having to modify the function definitions, and would allow David's suggestion of using function pointers to work, which we do in some other specific cases. I guess I'm curious if we (PG) have any particular feeling one way or the other about function pointers; I just don't recall seeing them used terribly often and would worry about that they might be passively discouraged? It does have a bit of a rock management feel to it and I really want to see if we can find a way to break that cycle. Agreed. It's been a point of frustration for me, but I've been trying to work with it so long as we at least get some constructive critisim back (Tom's review of the patch I reviewed fell into the questionable category for me on that call, which is what really frustrated me the most about it). A cyclic approach is typical in all software development, it's when information stops flowing about why something doesn't meet expectations or requirments that progress breaks down. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Adding support for SE-Linux security
* Robert Haas (robertmh...@gmail.com) wrote: On Fri, Dec 11, 2009 at 2:11 PM, Stephen Frost sfr...@snowman.net wrote: Second, the information we *don't* have from above is generally information about what the requesting action is. For example, when changing ownership of an object, we can't possibly use introspection to find out the role which is on the receiving end, since at that time we've just learned it from the parser. I'm not sure that I follow what you're saying here. I think maybe it would help to discuss a concrete example, which is why I proposed a concept patch. Or perhaps you'd like just to pick out a specific case to discuss. Hrm, I thought I had given a specific example. Didn't do a good job of it, apparently. Let me try to be a bit more clear: ALTER TABLE x OWNER TO y; If given the table OID, there's a ton of information we can then pull about the table- the tablespace, the owner, the schema, the columns, the privileges, etc, etc. What we can't possibly figure out from the OID is the value of y. Yet, in the PG security model, the value of y matters! You have to know what y is to check if y has 'create' rights on the schema. If it doesn't (and the user executing the command isn't the superuser) then the request (under the PG model) is denied. Does that help clarify my example case? Object creation seems to be one of the tougher cases here. When you're altering or dropping an existing object, the decision is likely to be based (in any system) on the properties of that object. When you're creating an object, the decision will of course have to be based on the properties of something else, but different access control models might not agree on the value of something else. It's not immediately clear to me how to deal with that, but again it's hard for me to discuss it in the abstract. That sounds like a pretty good example to me too, to be honest. It goes back to the same issue though- that's information you get from the command that's trying to be run and isn't available from existing objects. Using structures to track this information, allowing the function arguments to remain identical, is certainly something which can be done, and probably done with a minimal amount of fuss. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Adding support for SE-Linux security
Stephen (great name!), * Stephen Smalley (s...@tycho.nsa.gov) wrote: Reference: http://www.usenix.org/event/sec02/wright.html http://lxr.linux.no/#linux+v2.6.32/include/linux/security.h The XACE framework for the X server is described by: http://www.x.org/releases/X11R7.5/doc/security/XACE-Spec.html The FreeBSD MAC framework is described by: http://www.freebsd.org/doc/en/books/arch-handbook/mac.html Thanks for these pointers! I'm sure KaiGai has probably already done a review of these, but I'll take a look and would ask others who are interested to please do the same. Seeing the different options that people have gone with (as opposed to just the SELinux/kernel version) will undoubtably be helpful in deciding the best approach forward. Thanks again! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Adding support for SE-Linux security
* Robert Haas (robertmh...@gmail.com) wrote: If I don't tell you how to write the patch, you can't accuse me of moving the goalposts (of course I've now discovered the pitfalls of that approach as well...). Indeed, we also yell and scream when we don't know which direction the goalposts are in. ;) That thought had crossed my mind as well, but I wasn't convinced that would actually be seen as a signifigantly different API to just having the arguments passed inline... If you'll forgive me for saying so, this is exactly the sort of thinking that I think is killing us. Who cares how it will be seen? erp. mea culpa. I meant that I didn't think of it as being enough of a novelty for you to be suggesting-it-but-not-telling-us.. It just kind of came off as too-obvious, however, the reality is that I didn't understand your suggestion, see below. Then again, using structures does allow you to add to them without having to modify the function definitions, Personal opinion time, but I think that without having to modify the function definitions is a CRITICAL design component. Furthermore, adding to them [the structures] shouldn't be something that only happens when we decide to support a new security model, or we're little better off than if we just passed a kajillion arguments. Again, I don't want to overstate my confidence in this point, but it feels to me like we need to pass PRE-EXISTING data structures that are already being used for other things and happen to be the right stuff to enable access control decisions, and to which fields that are likely to be needed are likely to get added anyway. Ah, now that makes alot of sense.. Unfortunately, having been involved in at least some of this code in the past, sadly I don't think we have such pre-existing data structures for the information that's primairly at issue here. Specifically, the data structures we tend to have pre-existing are the ones that come from the catalog and would fall out from an OID+SubOID based API. Sure, we could pass those structures in instead of using SysCache to pull the data out, but that's not really how we 'typically' do things in the code base from my experience, and I don't really see it having alot of advantage. Using OID+SubOID and SysCache *will* pick up new things as they're added to the catalogs- for the information *in* the catalogs. There are very few data structures outside of the parse tree which include the information from the parse tree.. And to be honest, the ones that *are* out there don't typically have the world's best structure for use by this kind of a framework (thinking back to specifically what's passed around for column-level privs..). But it's difficult to know whether this approach can be made to work without trying it, and there are bound to be problem cases that need to be thought about, and that thinking will be more likely to lead to a good result if it happens in the community, rather than by KaiGai or any other single person in isolation. I agree with this- one issue is, unfortunately, an overabundance from KaiGai of code-writing man-power. This is an odd situation for this community, in general, so we're having a hard time coming to grasp with it. KaiGai, can you hold off on implementing any of these approaches until we can hammer down something a bit better for you to work from as a baseline? I'll start with Robert's suggestion of a single-object example case and throw out some example code for people to shoot down of different approachs. After a few iterations of that, with comments from all (KaiGai, you're welcome to comment on it too, of course), we'll turn you loose on it again to implement fully (if you're still willing to). Following along that though, as we'd like this to be the design forum, when you come across problems or issues implementing it, could you come back to this forum and explain the issue and why it doesn't fit, as soon as you hit it? What you tend to do is disappear for a while, then come back, instead, with a full patch which works, but has places where you've gone down an unexpected path because you found a case which wasn't covered, designed a solution for it which kinda fits and then implemented it immediately. I feel that's something I've encouraged you to do, unfortunately, and my apologies for that. I'm trying to get time from my employer (and my wife) to dedicate to this, to hopefully avoid that happening in the future. I'm going to vote fairly strongly against inserting function pointers at the outset. I think that we should look at the first phase of this project as an attempt to restructure the code so that the access control decisions are isolated from the rest of the code. *If* we can do that successfully, I think it will represent good progress all on its own. Inserting function pointers is something that can be done later if it turns out to be useful with a very small, self-contained patch.
Re: [HACKERS] Adding support for SE-Linux security
* Robert Haas (robertmh...@gmail.com) wrote: On Fri, Dec 11, 2009 at 4:26 PM, Stephen Frost sfr...@snowman.net wrote: Does that help clarify my example case? That case doesn't seem terribly problematic to me. It seems clear that we'll want to pass some information about both x and y. What is less clear is exactly what the argument types will be, and the right answer probably depends somewhat on the structure of the existing code, which I have not looked at. Allow me to assist- y is never in a structure once you're out of the parser: ATExecChangeOwner(Oid relationOid, Oid newOwnerId, bool recursing) I expect you'll find this is more the rule than the exception to alot of the existing PG security model, because much of it's responsibility is in what I'll call the DDL (under commands/) aspects. The DML pieces (under the executor) are a bit better about this, specifically you could pass in a RangeTblEntry, exactly how ExecCheckRTEPerms handles it. Actually, in a fit of barely-contained mirth, it strikes me that PG really has already done what you're suggesting for the 'hard' part- and the RangeTblEntry plus ExecCheckRTEPerms is exactly it. It's all the *other* checks we do for the PG security model, under commands/, that are the problem here. The parts of the code that, to be honest, I think all us database geeks have historically cared alot less about. What I'm more concerned about is if the access control decision in this case were based on u for PG DAC, v for SE-PostgreSQL, and w for Robert Haas's Personal Defensive System. If that's the case, and our function signature looks like (x,y,u,v,w), the we should worry. Right, I understand that. What I offer in reply is that we focus our attention on using the OID+SubOID approach, as I'm suggesting, to the fullest extent possible through that mechanism, and appreciate that the other arguments passed to the function are derived specifically from the parser and therefore unlikely to be changed until and unless we change the base syntax of the command and calling function, at which time we may have to add arguments to the function signature. This would continue at least until we get to the point where we decide that the caller needs to be changed because it's got a huge function sig, and move it to something like the structure of the executor and the equivilant of ExecCheckRTEPerms() would get updated along with it, at that time. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Adding support for SE-Linux security
* Robert Haas (robertmh...@gmail.com) wrote: Allow me to assist- y is never in a structure once you're out of the parser: Well this is why you're writing the patch and not me. :-) Sure, just trying to explain why your suggestion isn't quite the direction that probably makes the most sense.. At least for args which come from the parser. What exactly do you mean by a SubOID? I'm not really following that part. Sorry, it's basically things like attnum, check the dependency code for example usage. It's this is something we need to track due to dependencies but it's at a level below OID, or perhaps a component of an object which has an OID- specifically: a column of a table. The table has an OID, but the column does not. To uniquely identify the column you need both the OID and the 'SubOID'/attnum. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Adding support for SE-Linux security
* Tom Lane (t...@sss.pgh.pa.us) wrote: Robert Haas robertmh...@gmail.com writes: What exactly do you mean by a SubOID? I'm not really following that part. I assume he's talking about the object reference representation used in pg_depend, which is actually class OID + object OID + sub-object ID. The only object type that has sub-objects at the moment is tables, wherein the sub-objects are columns and the sub-object IDs are column numbers. The sub-object ID is zero for all other cases. You're right, of course, but for some reason I thought that there was another usage of it besides just the table/column case. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Adding support for SE-Linux security
* Stephen Frost (sfr...@snowman.net) wrote: * Tom Lane (t...@sss.pgh.pa.us) wrote: I assume he's talking about the object reference representation used in pg_depend, which is actually class OID + object OID + sub-object ID. The only object type that has sub-objects at the moment is tables, wherein the sub-objects are columns and the sub-object IDs are column numbers. The sub-object ID is zero for all other cases. You're right, of course, but for some reason I thought that there was another usage of it besides just the table/column case. Ah, I realize what I was thinking about now.. The dependency system has two flavors (pg_depend and pg_shdepend). We had SubIDs for columns in pg_depend but not pg_shdepend- until column-level privs were added which meant we could have roles depend on columns (due to privileges on that column). To clarify- pg_depend is for database dependencies while pg_shdepend is for cluster dependencies. Thanks, Stephen signature.asc Description: Digital signature
[HACKERS] Row-Level Security
Greetings, I'll start a new thread on this specific topic to hopefully pull out anyone who's focus is more on that than on SEPG. Row-Level security has been implemented in a number of existing commercial databases. There exists an implementation of row-level security for PostgreSQL today in the form of SEPostgres. I believe there is a signfigant user base who would like RLS without SELinux (or perhaps with some other security manager). As it is a useful feature indepenent of SELinux, it should be implemented in a way which doesn't depend on SELinux in any way. I've started a wiki page to discuss this here: http://wiki.postgresql.org/wiki/RLS I'd like to start a discussion about RLS for PG- design, user-interface, syntax, capabilities, on-disk format changes, etc. For starters, I think we shoud review the existing RLS implementations. To that end, I've added a number of articles about them to the wiki. I think the next step is to start summarizing how those operate and important similarities and differences between them. Our goal, of course, is to take the best of what's out there. Please comment, update the wiki, let us know you're interested in this.. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Row-Level Security
KaiGai, * KaiGai Kohei (kai...@kaigai.gr.jp) wrote: Please comment, update the wiki, let us know you're interested in this.. Good start, however, could you defer the discussion after the Feb-15? My hands are now full in the security framework and SE-PgSQL/Lite. :( While I'm glad you're enthusiastic and interested in this too, I don't believe we need to delay this initial discussion. To be honest, I think we really need to get some input and interest from others as well. I'll do my best to make sure the wiki is updated with information and links to any signifigant threads on the lists. I don't expect to be writing any serious code by Feb 15th on this anyway. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Adding support for SE-Linux security
* Bruce Momjian (br...@momjian.us) wrote: I am not replying to many of these emails so I don't appear to be brow-beating (forcing) the community into accepting this features. I might be brow-beating the community, but I don't want to _appear_ to be brow-beating. ;-) My apologies if I come across this way- I don't intend to... But I'm also very enthusiastic about this. Also, it's become a much more personal issue for me due to this: http://csrc.nist.gov/news_events/documents/omb/draft-omb-fy2010-security-metrics.pdf OMB is now looking to include label-based security in their metrics. This directly impacts some of the PG-based systems I run. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Row-Level Security
KaiGai, * KaiGai Kohei (kai...@kaigai.gr.jp) wrote: The reason why I put on the security hook in ExecScan() is to avoid the problem that row-cost user defined function can be evaluated earlier than row-level security policy. (I believed it was a well-known problem at that time yet.) So, I didn't want to append it before optimization. This is a problem which needs to be addressed and fixed independently. I also believe this matter should be resolved when we provide row-level security stuff, because it is a security feature. This issue should be fixed first, not as part of some large-scale patch. If you have thoughts or ideas about how to address this problem as it relates to views, I think you would find alot of people willing to listen and to discuss it. This must be independent of SELinux, independent of row-level security, and isn't something based on any of the patches which have been submitted so far. None of them that I've seen resolve this problem in a way that the community is willing to accept. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Row-Level Security
KaiGai, * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: IIRC, one headache issue is that user may provide well indexable conditions, such as SELECT * FROM view_x WHERE id = 1234. In this case, if we strictly keep the order of evaluation between inside and outside of the view, its performance penalty will over reasonable tradeoff to the better security. Someone pointed out user given conditions which can be replaced by index scan are trusted, so all we need to focus on are conditions which need to check for each tuples. I also think it is a right direction, as long as we can restrict who can define index access method in appropriate way. It sounds like that might help, but I feel that a whole solution will be more complex than just differentiating between seq scan nodes and index scan ones. If we can focus on the order of evaluation on the non-indexed conditions, the point is order_qual_clauses() which sort the given qual list based on the cost evaluation. If we can mark condition node a flag which means this node come from inside of view or row-level policy, it is not difficult to keep evaluation order. Identifying where this matters is important. Anyone have suggestions on how to do that? There was some discussion on IRC about that but it didn't really go anywhere. I don't like the idea of presuming the user will always want to limit the planner in this way. Perhaps we can convince ourselves, once we have an implementation, that it doesn't poorly affect performance (the primary reason to avoid constraining the planner), or that it's what our users would really want (I might be able to buy off on this..), but I doubt it. A couple of options about how the user could ask us to constrain the planning to eliminate this issue are, off-hand: Global GUC which enables/disables Attribute of the view, perhaps indicated as 'CREATE SECURITY VIEW' or similar Something in the definition of the WHERE clause, eg: select * from x where security(q = 50); Anyone have thoughts about this? Perhaps it's too early to discuss this anyway, just trying to keep the discussion moving in some way. However, it is just my quick idea. It might miss something. We need to consider the matter for more details... I agree, this needs more thought and input from others who are very familiar with the planner, executor, etc. Additionally, this needs to be done before we can really go anywhere with row-level security. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Adding support for SE-Linux security
Bruce, * Bruce Momjian (br...@momjian.us) wrote: You are fine. I was just saying that at a time I was one of the few loud voices on this, and if this is going to happen, it will be because we have a team that wants to do this, not because I am being loud. I see the team forming nicely. Not to rain down on the parade too much here, but I have to disagree about a team forming nicely. That's, unfortunately, what it looks like from the 10k-foot level. Indeed, it looks like we're making good headway to get some kind of support into core from that level. The reality is that we've barely started and really have still got quite a ways to go and it would really be useful to bring in additional resources on this. I wouldn't consider myself to be that additional resource unless and until I can get funding for dedicated time (either my own or someone else's). I've got a few action items that I'm planning to resolve in the next few weeks, but I've been involved in this for over a year now and it hasn't made much progress, overall, in that time. So, for anyone else who's interested in label-based security happening for PostgreSQL (for whatever reason, masochisim perfectly acceptable), please speak up and offer to help. We could use it. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Row-Level Security
* Robert Haas (robertmh...@gmail.com) wrote: I think there was a previous discussion of this when Heikki first posted the issue to -hackers. There was, it's now linked off the http://wiki.postgresql.org/wiki/RLS page (as well as this thread). Feel free to add other threads, update with your thoughts, summarize what the thread conclusions were, etc... Otherwise I'll have to. ;) (Seriously, I'm planning to, but if you could take a peek at what I've put up there so far, I wouldn't complain). Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] remove redundant ownership checks
KaiGai, * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: The patch was not attached... This patch either does too much, or not enough. I would either leave the Assert() in-place as a double-check (I presume that's why it was there in the first place, and if that Assert() fails then our assumption about the permissions check being already done on the object in question would be wrong, since the check is done against the passed-in 'rel' and the assert is that 'rel' and 'ruletup-ev_class' are the same; if they're not, then we might need to do perms checking on ruletup-ev_class) Or Remove the now-unused variable eventRelationOid. Overall, I agree with removing this check as it's already done by ATSimplePermissions() and we don't double-check the permissions in the other things called through ATExecCmd() (though there are cases where specific commands have to do *additional* checks beyond what ATSimplePermissions() does.. it might be worth looking into what those are and thinking about if we should move/consolidate/etc them, or if it makes sense to leave them where they are). Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] remove redundant ownership checks
* Tom Lane (t...@sss.pgh.pa.us) wrote: KaiGai Kohei kai...@ak.jp.nec.com writes: [ patch to remove EnableDisableRule's permissions check ] I don't particularly like this patch, mainly because I disagree with randomly removing permissions checks without any sort of plan about where they ought to go. The goal of this was to increase consistancy with the rest of the code, in particular, ATPrepCmd checks ownership rights on the table, and anything which wants to check permissions beyond that has to do it independently. Do I like that? No, not really. There are two principal entry points in rewriteDefine.c (the other one being DefineQueryRewrite), and currently they both do permissions checks. DefineQueryRewrite gets called out of tcop/utility.c (either under a CREATE VIEW or a CREATE RULE). tcop/utility.c expects the functions it calls to to handle permissions checking (except in the one case it doesn't call a function- T_IndexStmt). Interestingly, DefineIndex, while it handles a number of other permissions checks, doesn't do checks to ensure the caller is the table owner- it expects callers to have done that (which happens both in tcop/utility.c for CREATE INDEX and in ATPrepCmd for ALTER TABLE ...). There is also a third major function RenameRewriteRule, currently ifdef'd out because it's not used, which is commented as Note that it lacks a permissions check, indicating that somebody (possibly me, I don't recall for sure) thought it was surprising that there wasn't such a check there. This is sensible if you suppose that this file implements rule utility commands that are more or less directly called by the user, which is in fact the case for DefineQueryRewrite, and it's not obvious why it wouldn't be the case for EnableDisableRule. Since that's a globally exposed function, it's quite possible that there's third-party code calling it and expecting it to do the appropriate permissions check. (A quick look at Slony, in particular, would be a good idea here.) Personally I find it suprising that things called from ATExecCmd() expect some permissions checks to have been done already. If we're going to start moving these checks around we need a very well-defined notion of where permissions checks should be made, so that everyone knows what to expect. I have not seen any plan for that. Removing one check at a time because it appears to not be necessary in the code paths you've looked at is not a plan. What I suggested previously, though it may be naive, is to do the permissions checks when you actually have enough information to do them. At the moment we do a 'simple' check in ATPrepCmd (essentially, ownership of the relation) and then any more complicated checks have to be done by the function which actually understands what's going on enough to know what else needs to be checked (eg: ATAddForeignKeyConstraint). As I see it, you've mainly got three steps: Parsing the command (syntax, basic understanding) Validation (check for object existance, get object info, etc) Implementation (do the requested action) I would put the permissions checking between Validation and Implementation, ideally at the 'top' of Implementation. This, imv, is pretty similar to how we handle DML commands today- parsing/validation are done first but then the permissions checks aren't done until we actually go to run the command (of course, this also deals with prepared queries and the like). Right now what we have are a bunch of checks scattered around during Validation, as we come across things we think should be checked (gee, we know the table the user referenced, let's check if he owns it now). Of course, this patch isn't doing that because it was intended to make the existing code consistant, not institute a new policy for how permissions checking should be done. The big downside about trying to define a new policy is that it's alot more difficult to get agreement on, and there are likely to be exceptions brought out that might make the policy appear to be ill-formed. Do people think this is a sensible approach? Are there known exceptions where this won't work or would cause problems? Is it possible to make that kind of deliniation in general? Should we work up an example patch which moves some of these checks in that direction? Thanks for your comments. Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] remove redundant ownership checks
* Robert Haas (robertmh...@gmail.com) wrote: Disentangling that seems like a job and a half. Indeed it will be, but I think it would be a good thing to actually have a defined point at which permissions checking is to be done. Trying to read the code and figure out what permissions you need to perform certain actions, when some of those checks are done as 'prep work' far up the tree, isn't fun. Makes validation of the checks we say we do in the documentation more difficult too. Not to mention that if we want to allow more granular permission granting for certain operations, it gets even uglier.. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] update_process_title=off and logger, wal, ... processes
* Rod Taylor (rod.tay...@gmail.com) wrote: I (stupidly?) installed PostgreSQL into a hostile environment which didn't like this and decided to kill the processes as a result. Unfortunately, I cannot change the environment. That's not hostile, that's broken. Stephen signature.asc Description: Digital signature
Re: [HACKERS] RFC: PostgreSQL Add-On Network
* Josh Berkus (j...@agliodbs.com) wrote: What I'm getting from your e-mail, Dave, is If it doesn't solve all problems for everyone in the world from Day 1, it's not worth doing. It's my experience that the way to get OSS software that works is to build a little bit at a time, each delivery of which is useful to some people and solves some problems. Projects which attempt to paint the whole world never get anywhere. +1 from me. I was just discussing this with some other folks at my company and had to make the point that if all you do is show the CEO a big fat $ figure, he's gonna say no. If you actually want to make it happen, you have to show a progression to get there from where we are today, with the incremental costs required along the way and the functionality that you get for that. David's proposal is designed to be something which he can get done *this year*, possibly before 8.5 is released, and be built on later. It'll be useful to a substantial number of our users, and will be an improvement on what we have now. Sounds excellent to me. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] RFC: PostgreSQL Add-On Network
* Dave Page (dp...@pgadmin.org) wrote: Because if we (PostgreSQL) are going to support this effort, then it should not ignore such a huge percentage of our installation base. Not doing it day 1 is not ignoring. It's using what resources *are* being made available to the best extent we can. If you're offering to do the work for Windows, great! It would make sense to build a C version when the other issues with supporting Windows are solved. At that point, the specification for the client should be well-worn enough to make doing the C version not a halt to further development. Until then, it doesn't matter. So you have to rewrite the code. Seems like a waste of effort. So the options are some Perl code that works for quite a few users or.. nothing because he's not a C hacker or doesn't want to write it in C? Sounds like #1 is a win to me. If David's happy to write it in C to begin with (presuming he has to write anything- if there's existing Perl code that does 90% of what he needs, you're asking for alot more), great. I'm even happy to encourage him to do that if it's anywhere close to the same level of effort. No. The essence is, 'If you're going to do it in a way that will never work for maybe 50% or more of PostgreSQL installations, then you have fundamental design issues to overcome'. And my vote is that you have to start somewhere and I strongly disagree that what you're concerned with are serious *design* issues. What David has described includes alot of implementation details, let's not confuse the two. If the server-side had to be scrapped entirely and rewritten to support Windows, you might have a leg to stand on. If adding Windows support is an incremental change to the existing system (as a whole, which, yes, I'd consider the port of a perl client app to C to be an incremental change), then it's not a design issue. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] damage control mode
* David Fetter (da...@fetter.org) wrote: OK, we have a proposal on the table to bump some patches from this CommitFest to free up more committer resources, particularly Tom, to work on Hot Standby and Streaming Replication and attempt to accelerate the process of getting 8.5 out the door. This proposal needs input from the community. The affected patches are: I don't see any need to thump the panic button today. If we *must* have SR and it's not in by the 15th, let's do another Commitfest rather than jack the people who played by the rules. Playing by the rules isn't a guarantee, sorry. We have to prioritize at some point or we'll never get it done. It seems like typically we do that when we're past the point where we had originally planned to release and only do it because of that pressure. I would have flipped the priority of the two top items (I'd rather have writeable CTE than a new listen/notify), but that doesn't change that I think they're under SR. I'm gonna have to +1 bumping the lower priority items in favor of having an on-time release. To be honest, and I don't intend this as a slight to anyone, but I'm already worried that just getting HS into a state where everyone is comfortable releasing it to the wild and having users trust their data to 8.5 is going to be a serious effort between now and release. Perhaps if we discover HS and SR go in alot more easily than expected we could revisit the decision to bump things, but I don't like encouraging false hope. Would things really happen that way? I tend to doubt it, since I think after we get HS and SR done there's going to be an appropriate push for beta. Just my 2c from reading the list and history. I'm happy to be wrong. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] RFC: PostgreSQL Add-On Network
Dave, * Dave Page (dp...@pgadmin.org) wrote: Right - but the buildfarm isn't a feature being offered to end users. And this network isn't a feature of the core code either, nor, do I believe, is it being designed in a way that would require an overhaul down the road to support Windows. To be honest, I really don't feel this compares to features in the core code to begin with- I feel like generally we don't like things which only work on a subset of platforms because it's a small incremental change, and typically just requires good programming practice, to make it work for all supported platforms. That isn't true for this. I'm not saying it should be supported from day 1, but I think the initial plan will make it very difficult to add Windows support later without a great deal of rewriting/redesign. It's lack of forward planning I was objecting to. I disagree with this, but in the end I don't think that it really matters. If all we have resources for to work on this is a Perl expect, then by golly let that be a dependency for the initial work. If there are other resources who are willing to write the initial version in C, or what have you, let them step up and offer. This isn't a zero-sum game here and I'm strongly against telling someone please don't work on this when it's something which will be of benefit to many users. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Setting oom_adj on linux?
* Magnus Hagander (mag...@hagander.net) wrote: Do we need to make the value configurable? I'd certainly find it interesting to set backends to say 5 or something like that, that makes them less likely to be killed than any old oops opened too big file in an editor-process, but still possible to kill if the system is *really* running out of memory. We do need to make it configurable, in at least the sense of being able to control if it's done or not. There are some environments where you won't be able to set it. Perhaps just handling failure gracefully would work, but I'd be happier if you could just disable it in the config file. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Setting oom_adj on linux?
* Tom Lane (t...@sss.pgh.pa.us) wrote: I don't want to go to the trouble of creating (and documenting) a configure option for this. Much less a GUC ;-) Requiring a custom build to disable it would be horrible, in my view. Or, at best, just means that the packagers won't enable it, which obviously would be less than ideal. Sorry if it's a pain, but I think it needs to either be configurable or not done. As I said before, it definitely needs to handle failure gracefully, but I worry that even that won't be sufficient in some cases. Just thinking about how we run PG under VServers and Linux Containers and whatnot, we ran into some issues with OpenSSH trying to monkey with proc values and I'd really hate to run into the same issues with PG. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Setting oom_adj on linux?
* Tom Lane (t...@sss.pgh.pa.us) wrote: Alex Hunsaker bada...@gmail.com writes: On Fri, Jan 8, 2010 at 07:27, Tom Lane t...@sss.pgh.pa.us wrote: Then, somebody who wants the feature would build with, say, -DLINUX_OOM_ADJ=0 or another value if they want that. Here is a stab at that. Anybody have an objection to this basic approach? I'm in a bit of a hurry to get something like this into the Fedora RPMs, so barring objections I'm going to review this, commit it into HEAD, and then make a back-ported patch I can use with 8.4 in Fedora. Whoah, I would caution against doing that without being very confident it won't break when installed under things like VServer, Linux Containers, SELinux configurations, etc, when back-porting it. I don't expect people would be too pleased to discover their nice, simple, no-expected-issues upgrade of a minor point release to all of a sudden mean their database doesn't start anymore.. Sorry I havn't got time right now to run down the issue I had with OpenSSH doing something similar, and it might have just been poor coding in OpenSSH, but I wanted to voice my concern. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] damage control mode
* Kevin Grittner (kevin.gritt...@wicourts.gov) wrote: It also seems to call into question the wisdom of annual releases. If we had a two-year cycle which had three times as much in it, would that be an improvement, or not? At the moment, my vote would be how 'bout we discuss this post-8.5?. Maybe if we postpone the discussion till then, we'll actually be able to chew through everything and release close to on-time, otherwise I wonder if a thread like this won't end up sucking up too much in terms of our resources right at crunch time.. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Setting oom_adj on linux?
Alex, * Alex Hunsaker (bada...@gmail.com) wrote: As long as the VM/container you are running under wont kill postmaster for trying to access proc-- the patch I posted should work fine. It just ignores any error (I assumed you might be running in a chroot without proc or some such). As I recall, oom_adj wasn't visible in the container because you explicitly set what proc entries processes can have access to when using VServers, and OpenSSH didn't handle that cleanly. Guess what I'm just saying is don't just assume everything is as it would be on a 'normal' system when dealing with /proc and friends, and, of course, test, test, test when you're talking about back-porting things. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Setting oom_adj on linux?
* Tom Lane (t...@sss.pgh.pa.us) wrote: Alex Hunsaker bada...@gmail.com writes: Sure this was openssh? I just looked through the entire cvs history for opensshp and found 0 references to 'oom' let alone 'oom_adj'. Maybe something distro specific? FWIW, I see no evidence that sshd on Fedora does anything to change its oom score --- the oom_adj file reads as zero for both the parent daemon and its children. Kinda scary to realize the OOM killer could easily lock me out of boxes I run headless. There were a few issues, as it turns out, the particularly annoying one was in the init script which caused upgrades to fail due to sshd not being restarted, bug report here: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=473573 The other issue was with a Debian-specific patch which was applied to OpenSSH which basically just created noise in the log file, bug report here: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=487325 In the end, the problem was with errors being returned from attempts to modify oom_adj. As long as we can just ignore those hopefully there won't be any issues. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] RADIUS authentication
Magnus, * Magnus Hagander (mag...@hagander.net) wrote: The attached patch implements RADIUS authentication (RFC2865-compatible). Great! We have a few environments which use RADIUS auth, nice that PG might be able to use that auth method in the future. I'm not a fan of having the shared secret stored in a 'regular' config file. Could you support, or maybe just change it to, breaking that out into another file? Perhaps something simimlar to how pam_radius_auth works, where you can also list multiple servers? http://freeradius.org/pam_radius_auth/ Would also allow using the same file for multiple RADIUS-based servers.. I know pg_hba.conf can just be set to have minimal permissions (and is on Debian), but that's the kind of file that tends to end up in things like subversion repositories or puppet configs where they aren't treated as carefully since, generally, what's in them doesn't come across as super-sensetive. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] remove redundant ownership checks
* Jaime Casanova (jcasa...@systemguards.com.ec) wrote: if it's not broken, then it doesn't need a fix... if that code is causing a hole in security then i said remove it, if not... what's the problem in let it be until we know exactly what the plan is? The chances of getting concensus on an overarching big plan are slim to none, which essentially means it's not going to get changed. I've suggested an approach and had no feedback on it. What's probably needed, to get attention anyway, is a patch which starts to move us in the right direction. That's going to be more than a 6-line patch, but doesn't have to be the whole thing either. For my 2c, I think a patch which attacks 'AT_PrepCmd' and rips out all of the owner checks from it and moves them to appropriate places (that is to say, per my proposal, very shortly before the 'work' is actually done, which is also often where the *other* permission checks are done, for those pieces which require more than just a simple owner check) would probably be the first step. Of course, the code between AT_PrepCmd and where the permissions checks are moved to would need to be reviewed and vetted to make sure there isn't anything being done that shouldn't be done without permission, but, honestly, I don't recall seeing much of that. We're actually pretty good about having a gather info stage followed by a implement change stage. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] remove redundant ownership checks
* Alex Hunsaker (bada...@gmail.com) wrote: On Wed, Jan 13, 2010 at 12:54, Tom Lane t...@sss.pgh.pa.us wrote: I'm a little worried by Stephen's plan, mainly because I'm concerned that it would lead to ALTER TABLE taking exclusive lock on a table long before it gets around to checking permissions. Still, that's just extending a window that exists now. Im of the opinion if we are going to be meddling with the permission checks in this area one of the goals should be close or at least tighten up that window. So you cant lock a table you dont have permission to (either via LOCK or ALTER TABLE). (Ignoring the issues of concurrent permission changes of course...) Trying to minimize that makes the permissions checking a royal mess by making it have to happen all over the place, after every little bit of information is gathered. I'm not a fan of that because of both concerns about making sure it's correct and actually matches our documention, as well as any possibility of making it a pluggable framework. At the moment, we're doing permissions checks on the main table before we even know if the other tables referenced in the command exist. I don't think we're talking about a serious difference in time here either, to be honest. Not to mention that if you don't have access to the schema, you wouldn't be able to take a lock on the table at all, so I'm really not sure how big a deal this is.. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] remove redundant ownership checks
* Alex Hunsaker (bada...@gmail.com) wrote: On Wed, Jan 13, 2010 at 13:46, Alex Hunsaker bada...@gmail.com wrote: Im of the opinion if we are going to be meddling with the permission checks [...] Specifically: http://archives.postgresql.org/pgsql-hackers/2009-05/msg00566.php Sounds like most solutions would put us back to exactly what you were trying to fix. :( Maybe its a moot point. But I figured while we are talking about ALTER permissions Maybe I missed it, but I don't see anything that said ALTER TABLE was changed or fixed to address this concern. It might make the timing increase some, and it'd be interesting in any case to see just what the timing change looked like, but I don't know that it's really all that important.. At least if the timing didn't change much we could claim that this didn't affect this use-case, but I don't believe it shouldn't be done if it does. I don't see a way to actually *fix* the problem of not allowing someone who doesn't have all the right permissions to not lock the table at all. Taking a share lock and then trying to upgrade it isn't a good idea either. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] remove redundant ownership checks
* KaiGai Kohei (kai...@ak.jp.nec.com) wrote: Some of ALTER TABLE operations take multiple permission checks, not only ownership of the relation to be altered. Yes, exactly my point. Those places typically just presume that the owner check has already been done. For example, ALTER TABLE with SET TABLESPACE option also need ACL_CREATE permission on the new tablespace, not only ownership of the relation. It means we need to gather two information before whole of the permission checks. (1) OID of the relation to be altered. (2) OID of the tablespace to be set. Right. I would say that we should wait until we have all the necessary information to do the permissions checking, and then do it all at that point, similar to how we handle DML today. In my understanding, Stephen suggests that we should, ideally, rip out permission logic from the code closely-combined with the steps of implementation. This might be a confusion due to language, but I think of the implementation as being the work. The check on permissions on the tablespace that you're describing above is done right before the work. For this case, specifically, ATPrepSetTableSpace() takes the action on line 6754: tab-newTableSpace = tablespaceId; Prior to that, it checks the tablespace permissions (but not the table permissions, since they've been checked already). I would suggest we add a call to ATSimplePermissions() in ATPrepSetTableSpace at line 6745- right after the /* Check its permissions */ comment (which would be changed to check permissions for ALTER TABLE x SET TABLESPACE y). Of course, it does not mean all the checks should be moved just before simple_heap_update(). No, I would have it earlier than simple_heap_update(), we don't need to go building the structures and whatnot needed to call simple_heap_update(). For this specific case though, I'm a bit torn by the fact that the work associated with changing the tablespace can actually happen in two distinct places- either through ATExecSetTableSpace, or in ATRewriteTables directly. ATExecSetTableSpace would actually be a good candidate rather than in the 'prep' stage, if all tablespace changes were done there. The 'prep' stage worries me a bit since I'm not sure if all permissions checking is currently, or coulde be, done at that point, and I'd prefer that we use the same approach for permissions checking throughout the code- for example, it's either done in 'phase 3' (where we're going through the subcommands) or all done in 'phase 1/2', where we're setting things up. However, it is a separate topic for this patch. This patch just tries to remove redundant checks, and integrate them into a common place where most of ALTER TABLE option applies its permission checks on. I don't believe we can make the case that it's a distinct topic based on the feedback received. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] 8.5 vs. 9.0
* Dave Page (dp...@pgadmin.org) wrote: Wait for it 9.0. Sure, tell us now, after we've all already had to submit our 8.5-related talks for PGCon... ;) Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] RADIUS secret in file
* Magnus Hagander (mag...@hagander.net) wrote: IIRC Stephen had some other reason, but I'll leave it to him to fill that in :-) I was really looking for multi-server support as well, and support for a config-file format that's commonly used for RADIUS. I'll take a whack at doing that this evening. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Repetition of warning message while REVOKE
All,
* Joshua D. Drake (j...@commandprompt.com) wrote:
On Thu, 2010-03-04 at 11:23 -0500, Tom Lane wrote:
I'm not sure offhand about a reasonable way to rearrange the code to
avoid duplicate messages.
Perhaps just add what can't be revoked? meaning:
WARNING: no privileges could be revoked for tbl for column foo
Then they aren't actually duplicate.
Yeah, they really aren't, after all. I don't know how we could
rearrange the code to prevent it- we're checking and trying to change
privileges on each of the columns in the table, after all.
Attached is a patch to add column name to the error message when it's a
column-level failure. I'm not really thrilled with it, due to the
expansion of code and addition of a bunch of conditionals, but at least
this isn't a terribly complicated function..
In the process of trying to build/run regression tests, but having
some build issues w/ HEAD (probably my fault).
Thanks,
Stephen
Index: src/backend/catalog/aclchk.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/catalog/aclchk.c,v
retrieving revision 1.163
diff -c -r1.163 aclchk.c
*** src/backend/catalog/aclchk.c 26 Feb 2010 02:00:35 - 1.163
--- src/backend/catalog/aclchk.c 5 Mar 2010 01:18:42 -
***
*** 304,327
if (is_grant)
{
if (this_privileges == 0)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg(no privileges were granted for \%s\, objname)));
! else if (!all_privs this_privileges != privileges)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg(not all privileges were granted for \%s\, objname)));
}
else
{
if (this_privileges == 0)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg(no privileges could be revoked for \%s\, objname)));
! else if (!all_privs this_privileges != privileges)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg(not all privileges could be revoked for \%s\, objname)));
}
return this_privileges;
--- 304,365
if (is_grant)
{
if (this_privileges == 0)
! {
! if (objkind == ACL_KIND_COLUMN colname)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg(no privileges were granted for \%s\, for column \%s\,
! objname, colname)));
! else
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg(no privileges were granted for \%s\, objname)));
! }
! else
! {
! if (!all_privs this_privileges != privileges)
! {
! if (objkind == ACL_KIND_COLUMN colname)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg(not all privileges were granted for \%s\, for column \%s\,
! objname, colname)));
! else
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg(not all privileges were granted for \%s\, objname)));
! }
! }
}
else
{
if (this_privileges == 0)
! {
! if (objkind == ACL_KIND_COLUMN colname)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg(no privileges could be revoked for \%s\, for column \%s\,
! objname, colname)));
! else
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg(no privileges could be revoked for \%s\, objname)));
! }
! else
! {
! if (!all_privs this_privileges != privileges)
! {
! if (objkind == ACL_KIND_COLUMN colname)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg(not all privileges could be revoked for \%s\, for column \%s\,
! objname, colname)));
! else
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg(not all privileges could be revoked for \%s\, objname)));
! }
! }
}
return this_privileges;
signature.asc
Description: Digital signature
Re: [HACKERS] Repetition of warning message while REVOKE
* Tom Lane (t...@sss.pgh.pa.us) wrote:
One thought is that the column cases should be phrased more like
no privileges could be revoked for column foo of table bar
Check the messages associated with DROP cascading for the canonical
phrasing here, but I think that's what it is.
Looks like 'for column foo of relation bar' is more typical, so
that's what I did in the attached patch. I also cleaned up a few other
things I noticed in looking through the various messages/comments.
Thanks!
Stephen
? src/backend/regex/.regcomp.c.swp
? src/pl/plpgsql/src/pl_scan.c
Index: src/backend/catalog/aclchk.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/catalog/aclchk.c,v
retrieving revision 1.163
diff -c -r1.163 aclchk.c
*** src/backend/catalog/aclchk.c 26 Feb 2010 02:00:35 - 1.163
--- src/backend/catalog/aclchk.c 5 Mar 2010 13:16:48 -
***
*** 304,327
if (is_grant)
{
if (this_privileges == 0)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg(no privileges were granted for \%s\, objname)));
! else if (!all_privs this_privileges != privileges)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg(not all privileges were granted for \%s\, objname)));
}
else
{
if (this_privileges == 0)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg(no privileges could be revoked for \%s\, objname)));
! else if (!all_privs this_privileges != privileges)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg(not all privileges could be revoked for \%s\, objname)));
}
return this_privileges;
--- 304,365
if (is_grant)
{
if (this_privileges == 0)
! {
! if (objkind == ACL_KIND_COLUMN colname)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg(no privileges were granted for column \%s\ of relation \%s\,
! colname, objname)));
! else
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg(no privileges were granted for \%s\, objname)));
! }
! else
! {
! if (!all_privs this_privileges != privileges)
! {
! if (objkind == ACL_KIND_COLUMN colname)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg(not all privileges were granted for column \%s\ of relation \%s\,
! colname, objname)));
! else
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg(not all privileges were granted for \%s\, objname)));
! }
! }
}
else
{
if (this_privileges == 0)
! {
! if (objkind == ACL_KIND_COLUMN colname)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg(no privileges could be revoked for column \%s\ of relation \%s\,
! colname, objname)));
! else
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg(no privileges could be revoked for \%s\, objname)));
! }
! else
! {
! if (!all_privs this_privileges != privileges)
! {
! if (objkind == ACL_KIND_COLUMN colname)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg(not all privileges could be revoked for column \%s\ of relation \%s\,
! colname, objname)));
! else
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg(not all privileges could be revoked for \%s\, objname)));
! }
! }
}
return this_privileges;
***
*** 1657,1664
/*
* The GRANT TABLE syntax can be used for sequences and non-sequences,
* so we have to look at the relkind to determine the supported
! * permissions. The OR of table and sequence permissions were already
! * checked.
*/
if (istmt-objtype == ACL_OBJECT_RELATION)
{
--- 1695,1702
/*
* The GRANT TABLE syntax can be used for sequences and non-sequences,
* so we have to look at the relkind to determine the supported
! * permissions. The OR of relation and sequence permissions were
! * already checked.
*/
if (istmt-objtype == ACL_OBJECT_RELATION)
{
***
*** 3046,3052
case ACLCHECK_NO_PRIV:
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! errmsg(permission denied for column %s of relation %s,
colname, objectname)));
break;
case ACLCHECK_NOT_OWNER:
--- 3084,3090
case ACLCHECK_NO_PRIV:
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! errmsg(permission denied for column \%s\ of relation \%s\,
colname, objectname)));
break;
case ACLCHECK_NOT_OWNER:
signature.asc
Description: Digital signature
Re: [HACKERS] Proposal: access control jails (and introduction as aspiring GSoC student)
* Joseph Adams (joeyadams3.14...@gmail.com) wrote: I propose adding application-level access control to PostgreSQL via a jails concept. In a nutshell, a jail is created as part of the database definition (typically exposing a free variable for the current user). When a jail is activated for a session, the only accesses allowed are those indicated in the jail itself. A jail cannot be exited without closing the session. If used properly, jails make it possible to safely execute untrusted SQL code (though one may not want to, citing the principle of least privilege). I guess my initial reaction to this is that you can use roles, views, and pl/pgsql (security definer) functions to achieve this. This does have an interesting intersection with row-level security concepts and that's definitely a project that I'd like to see happen at some point in PG. Not sure if you've considered this, but you can do a 'set role' at the start of a session and then use CURRENT_ROLE in view definitions and in other places. You can also make it so that the user who is logging in (eg 'www-data') doesn't have any rights to anything, except the ability to 'set role' to other roles. Note that, with any of this, you need to consider pooled database connections. Unfortunately, it's still pretty expensive to establish a new database connection to PG. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Proposal: access control jails (and introduction as aspiring GSoC student)
* Robert Haas (robertmh...@gmail.com) wrote: Sometimes it would be nice to conditionalize queries on a value other than the authenticated role. I really wish we had some kind of SQL variable support. Talking out of my rear end: I certainly agree- having variable support in the backend would definitely be nice. I'd want it to be explicit and distinct from GUCs though, unlike the situation we have w/ psql right now. All that said, I'm not really a huge fan of write-your-own-authorization-system in general. If the existing authorization system isn't sufficient for what you want, then let's improve it. There may be specific cases where what's needed is particularly complex, but that's what security definer functions are for.. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Thoughts on pg_hba.conf rejection
Simon, * Simon Riggs (si...@2ndquadrant.com) wrote: So instead of the typical reject instruction we also add a rejectverbose instruction that has a more verbose message. Docs would describe it as an additional instruction to assist with debugging a complex pg_hba.conf, with warning that if used it may assist the bad guys also. Erm, so we'd add an option for this? That strikes me as pretty excessive. Not to be a pain, but I feel like the 'connection not authorized' argument plus a hint makes alot more sense. pg_hba.conf rejects entry for host... connection not authorized for host X user Y database Z HINT: Make sure your pg_hba.conf has the entries needed and the user has CONNECT privileges for the database Or something along those lines (I added the other CONNECT issue because it's one I've run into in the past.. :). I do also wonder if we should consider having the error that's reported to the log differ from that which is sent to the user.. I realize that's a much bigger animal and might not help the novice, but it could help with debugging complex pg_hba's without exposing info to possible bad guys. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] INSERT and parentheses
* Robert Haas (robertmh...@gmail.com) wrote: The first version is a lot more common and as it turns out, is sometimes very hard to spot. This patch attaches a HINT message to these two cases. The message itself could probably be a lot better, but I can't think of anything. Thoughts? I suggest adding it to the next CommitFest. Since I've never been bitten by this, I can't get excited about the change, but I'm also not arrogant enough to believe that everyone else's experiences are the same as my own. Not to be a pain, but the hint really is kind of terrible.. It'd probably be better if you included somewhere that the insert appears to be a single column with a record-type rather than multiple columns of non-composite type.. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] max_standby_delay considered harmful
* Simon Riggs (si...@2ndquadrant.com) wrote: I guarantee that if that proposal goes in, people will complain about that also. Last minute behaviour changes are bad news. I don't object to adding something, just don't take anything away. It's not like the code for it is pages long or anything. I have to disagree with this. If it goes into 9.0 this way then we're signing up to support it for *years*. With something as fragile as the existing setup (as outlined by Tom), that's probably not a good idea. We've not signed up to support the existing behaviour at all yet- alpha's aren't a guarentee of what we're going to release. The trade off is HA or queries and two modes make sense for user choice. The option isn't being thrown out, it's just being made to depend on something which is alot easier to measure while still being very useful for the trade-off you're talking about. I don't really see a downside to this, to be honest. Perhaps you could speak to the specific user experience difference that you think there would be from this change? +1 from me on Tom's proposal. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] max_standby_delay considered harmful
* Robert Haas (robertmh...@gmail.com) wrote: On Mon, May 3, 2010 at 11:37 AM, Tom Lane t...@sss.pgh.pa.us wrote: I'm inclined to think that we should throw away all this logic and just have the slave cancel competing queries if the replay process waits more than max_standby_delay seconds to acquire a lock. What if we somehow get into a situation where the replay process is waiting for a lock over and over and over again, because it keeps killing conflicting processes but something restarts them and they take locks over again? It seems hard to ensure that replay will make adequate progress with any substantially non-zero value of max_standby_delay under this definition. That was my first question too- but I reread what Tom wrote and came to a different conclusion: If the reply process waits more than max_standby_delay to acquire a lock, then it will kill off *everything* it runs into from that point forward, until it's done with whatever is currently available. At that point, the 'timer' would reset back to zero. When/how that timer gets reset was a question I had, but I feel like until nothing is available makes sense and is what I assumed Tom was thinking. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] max_standby_delay considered harmful
Simon, * Simon Riggs (si...@2ndquadrant.com) wrote: Tom's proposed behaviour (has also been proposed before) favours the avoid query cancellation route though could lead to huge amounts of lag. My impression of Tom's suggestion was that it would also be a maximum amount of delay which would be allowed before killing off queries- not that it would be able to wait indefinitely until no one is blocking. Based on that, I don't know that there's really much user-seen behaviour between the two, except in 'oddball' situations, where there's a time skew between the servers, or a large lag, etc, in which case I think Tom's proposal would be more likely what's 'expected', whereas what you would get with the existing implementation (zero time delay, or far too much) would be a 'gotcha'.. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] max_standby_delay considered harmful
* Simon Riggs (si...@2ndquadrant.com) wrote: If recovery waits for max_standby_delay every time something gets in its way, it should be clear that if many things get in its way it will progressively fall behind. There is no limit to this and it can always fall further behind. It does result in fewer cancelled queries and I do understand many may like that. Guess I wasn't very clear in my previous description of what I *think* the change would be (Tom, please jump in if I've got this wrong..). Recovery wouldn't wait max_standby_delay every time; I agree, that would be a big change in behaviour and could make it very difficult for the slave to keep up. Rather, recovery would proceed as normal until it encounters a lock, at which point it would start a counting down from max_standby_delay, if the lock is released before it hits that, then it will move on, if another lock is encoutered, it would start counting down from where it left off last time. If it hits zero, it'll cancel the other query, and any other queries that get in the way, until it's caught up again completely. Once recovery is fully caught up, the counter would reset again to max_standby_delay. That is *significantly* different from how it works now. (Plus: If there really was no difference, why not leave it as is?) Because it's much more complicated the way it is, it doesn't really work as one would expect in a number of situations, and it's trying to guarantee something that it probably can't. The bottom line is this is about conflict resolution. There is simply no way to resolve conflicts without favouring one or other of the protagonists. Whatever mechanism you come up with that favours one will, disfavour the other. I'm happy to give choices, but I'm not happy to force just one kind of conflict resolution. I don't think anyone is trying to get rid of the knob entirely; you're right, you can't please everyone all the time, so there has to be some kind of knob there which people can adjust based on their particular use case and system. This is about what exactly the knob is and how it's implemented and documented. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] max_standby_delay considered harmful
* Aidan Van Dyk (ai...@highrise.ca) wrote: * Heikki Linnakangas heikki.linnakan...@enterprisedb.com [100510 06:03]: A problem with using the name max_standby_delay for Tom's suggestion is that it sounds like a hard limit, which it isn't. But if we name it something like: I'ld still rather an if your killing something, make sure you kill enough to get all the way current behaviour, but that's just me I agree with that comment, and it's more like what max_standby_delay was. That's what I had thought Tom was proposing initially, since it makes a heck of alot more sense to me than just keep waiting, just keep waiting... Now, if it's possible to have things queue up behind the recovery process, such that the recovery process will only wait up to timeout * # of locks held when recovery started, that might be alright, but that's not the impression I've gotten about how this will work. Of course, I also want to be able to have a Nagios hook that checks how far behind the slave has gotten, and a way to tell the slave oook, you're too far behind, just forcibly catch up right *now*. If I could use reload to change max_standby_delay (or whatever) and I can figure out how long the delay is (even if I have to update a table on the master and then see what it says on the slave..), I'd be happy. That being said, I do think it makes more sense to wait until we've got a conflict to start the timer, and I rather like avoiding the uncertainty of time sync between master and slave by using WAL arrival time on the slave. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Query execution plan from 8.3 - 8.4
Brendan, * Brendan Hill (brend...@jims.net) wrote: Getting significantly lower performance on a specific query after upgrading from 8.3 - 8.4 (windows). I'm not expecting a quick fix from the mail lists, but I would appreciate any indications as to where else I could look or what tools I could employ to investigae further. Details below. For starters, this probably should go to -perform instead of -hackers, and it would be much more useful to have EXPLAIN ANALYZE results rather than just EXPLAIN. Thanks, Stephen signature.asc Description: Digital signature
[HACKERS] [PATCH] Add SIGCHLD catch to psql
Greetings,
Toying around with FETCH_COUNT today, I discovered that it didn't do
the #1 thing I really wanted to use it for- query large tables without
having to worry about LIMIT to see the first couple hundred records.
The reason is simple- psql ignores $PAGER exiting, which means that it
will happily continue pulling down the entire large table long after
you've stopped caring, which means you still have to wait forever.
The attached, admittedly quick hack, fixes this by having psql catch
SIGCHLD's using handle_sigint. I've tested this and it doesn't
appear to obviously break other cases where we have children (\!, for
example), since we're not going to be running a database query when
we're doing those, and if we are, and the child dies, we probably want
to *stop* anyway, similar to the $PAGER issue.
Another approach that I considered was fixing various things to deal
cleanly with write's failing to $PAGER (I presume the writes *were*
failing, since less was in a defunct state, but I didn't actually
test). This solution was simpler, faster to code and check, and alot
less invasive (or so it seemed to me at the time).
Anyway, this makes FETCH_COUNT alot more useful, and, in my view, the
current behaviour of completely ignoring $PAGER exiting is a bug.
Thanks,
Stephen
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index f605c97..dcab436 100644
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
*** NoticeProcessor(void *arg, const char *m
*** 188,194
/*
* Code to support query cancellation
*
! * Before we start a query, we enable the SIGINT signal catcher to send a
* cancel request to the backend. Note that sending the cancel directly from
* the signal handler is safe because PQcancel() is written to make it
* so. We use write() to report to stderr because it's better to use simple
--- 188,194
/*
* Code to support query cancellation
*
! * Before we start a query, we enable SIGINT and SIGCHLD signals to send a
* cancel request to the backend. Note that sending the cancel directly from
* the signal handler is safe because PQcancel() is written to make it
* so. We use write() to report to stderr because it's better to use simple
*** NoticeProcessor(void *arg, const char *m
*** 208,213
--- 208,218
* catcher to longjmp through sigint_interrupt_jmp. We assume readline and
* fgets are coded to handle possible interruption. (XXX currently this does
* not work on win32, so control-C is less useful there)
+ *
+ * SIGCHLD is also caught and handled the same to deal with cases where a user's
+ * PAGER or other child process exits. Otherwise, we would just keep sending
+ * data to a dead/zombied process. This won't typically matter except when
+ * FETCH_COUNT is used.
*/
volatile bool sigint_interrupt_enabled = false;
*** void
*** 259,264
--- 264,272
setup_cancel_handler(void)
{
pqsignal(SIGINT, handle_sigint);
+
+ /* Also send SIGCHLD signals, to catch cases where the user exits PAGER */
+ pqsignal(SIGCHLD, handle_sigint);
}
#else /* WIN32 */
signature.asc
Description: Digital signature
Re: [HACKERS] [PATCH] Add SIGCHLD catch to psql
* Tom Lane (t...@sss.pgh.pa.us) wrote: A saner approach, which would also help for other corner cases such as out-of-disk-space, would be to check for write failures on the output file and abandon the query if any occur. I had considered this, but I'm not sure we really need to catch *every* write failure. Perhaps just catching if the '\n' at the end of a row fails to be written out would be sufficient? Then turning around and setting cancel_query might be enough.. I'll write that up and test if it works. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] Add SIGCHLD catch to psql
* Tom Lane (t...@sss.pgh.pa.us) wrote: If you're combining this with the FETCH_COUNT logic then it seems like it'd be sufficient to check ferror(fout) once per fetch chunk, and just fall out of that loop then. I don't want psql issuing query cancels on its own authority, either. Attached is a patch that just checks the result from the existing fflush() inside the FETCH_COUNT loop and drops out of that loop if we get an error from it. Thanks! Stephen diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index f605c97..fae1e5a 100644 *** a/src/bin/psql/common.c --- b/src/bin/psql/common.c *** ExecQueryUsingCursor(const char *query, *** 982,987 --- 982,988 char fetch_cmd[64]; instr_time before, after; + int flush_error; *elapsed_msec = 0; *** ExecQueryUsingCursor(const char *query, *** 1098,1106 /* * Make sure to flush the output stream, so intermediate results are ! * visible to the client immediately. */ ! fflush(pset.queryFout); /* after the first result set, disallow header decoration */ my_popt.topt.start_table = false; --- 1099,1109 /* * Make sure to flush the output stream, so intermediate results are ! * visible to the client immediately. We check the results because ! * if the pager dies/exits/etc, there's no sense throwing more data ! * at it. */ ! flush_error = fflush(pset.queryFout); /* after the first result set, disallow header decoration */ my_popt.topt.start_table = false; *** ExecQueryUsingCursor(const char *query, *** 1108,1114 PQclear(results); ! if (ntuples pset.fetch_count || cancel_pressed) break; } --- ,1117 PQclear(results); ! if (ntuples pset.fetch_count || cancel_pressed || flush_error) break; } signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] Add SIGCHLD catch to psql
* Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@snowman.net writes: Attached is a patch that just checks the result from the existing fflush() inside the FETCH_COUNT loop and drops out of that loop if we get an error from it. I thought it might be about that simple once you went at it the right way ;-). However, I'd suggest checking ferror(pset.queryFout) as well as the fflush result. It's not clear to me whether fflush should be counted on to report an error that actually occurred in a previous fwrite. (It's also unclear why fflush isn't documented to set the stream error indicator on failure, but it isn't.) Sure, I can add the ferror() check. Patch attached. My man page (Debian/Linux) has this to say about fflush(): DESCRIPTION The function fflush() forces a write of all user-space buffered data for the given output or update stream via the stream’s underlying write function. The open status of the stream is unaffected. If the stream argument is NULL, fflush() flushes all open output streams. For a non-locking counterpart, see unlocked_stdio(3). RETURN VALUE Upon successful completion 0 is returned. Otherwise, EOF is returned and the global variable errno is set to indicate the error. ERRORS EBADF Stream is not an open stream, or is not open for writing. The function fflush() may also fail and set errno for any of the errors specified for the routine write(2). CONFORMING TO C89, C99. Thanks, Stephen diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index f605c97..25340f6 100644 *** a/src/bin/psql/common.c --- b/src/bin/psql/common.c *** ExecQueryUsingCursor(const char *query, *** 982,987 --- 982,988 char fetch_cmd[64]; instr_time before, after; + int flush_error; *elapsed_msec = 0; *** ExecQueryUsingCursor(const char *query, *** 1098,1106 /* * Make sure to flush the output stream, so intermediate results are ! * visible to the client immediately. */ ! fflush(pset.queryFout); /* after the first result set, disallow header decoration */ my_popt.topt.start_table = false; --- 1099,1109 /* * Make sure to flush the output stream, so intermediate results are ! * visible to the client immediately. We check the results because ! * if the pager dies/exits/etc, there's no sense throwing more data ! * at it. */ ! flush_error = fflush(pset.queryFout); /* after the first result set, disallow header decoration */ my_popt.topt.start_table = false; *** ExecQueryUsingCursor(const char *query, *** 1108,1114 PQclear(results); ! if (ntuples pset.fetch_count || cancel_pressed) break; } --- ,1124 PQclear(results); ! /* Check if we are at the end, if a cancel was pressed, or if ! * there were any errors either trying to flush out the results, ! * or more generally on the output stream at all. If we hit any ! * errors writing things to the stream, we presume $PAGER has ! * disappeared and stop bothring to pull down more data. ! */ ! if (ntuples pset.fetch_count || cancel_pressed || flush_error || ! ferror(pset.queryFout)) break; } signature.asc Description: Digital signature
[HACKERS] Bug with ordering aggregates?
Greetings, This doesn't seem right to me: postgres=# select postgres-# string_agg(column1::text order by column1 asc,',') postgres-# from (values (3),(4),(1),(2)) a; string_agg 1234 (1 row) I'm thinking we should toss a syntax error here and force the 'order by' to be at the end of any arguments to the aggregate. Alternatively, we should actually make this work like this one does: postgres=# select postgres-# string_agg(column1::text,',' order by column1 asc) postgres-# from (values (3),(4),(1),(2)) a; string_agg 1,2,3,4 (1 row) Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Bug with ordering aggregates?
* Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@snowman.net writes: This doesn't seem right to me: postgres=# select postgres-# string_agg(column1::text order by column1 asc,',') postgres-# from (values (3),(4),(1),(2)) a; string_agg 1234 (1 row) Looks fine to me: you have two ordering columns (the second rather useless, but that's no matter). Ah, yeah, guess I'll just complain that having the order by look like it's an argument to an aggregate makes things confusing. Not much to be done about it though. Thanks, Stephen signature.asc Description: Digital signature
[HACKERS] Documentation Bug/Misnomer?
Greetings, Under: http://developer.postgresql.org/pgdocs/postgres/runtime-config-file-locations.html We have: ident_file (string) Specifies the configuration file for ident authentication (customarily called pg_ident.conf). This parameter can only be set at server start. That's not really accurate anymore, is it? It's referring to the username maps now, which are used for Ident, GSSAPI, SSL, etc.. Thanks, Stephen signature.asc Description: Digital signature
