Re: [HACKERS] Out parameters handling
On Sun, Mar 8, 2009 at 4:36 PM, Tom Lane t...@sss.pgh.pa.us wrote: Ryan Bradetich rbradet...@gmail.com writes: This is one of the things I wanted to start looking at for 8.5. My idea was to optionally use : or @ (not sure which is more popular) to specify this token is only a variable. This whole line of thought is really a terrible idea IMHO. plpgsql is supposed to follow Oracle's pl/sql syntax, not invent random syntax of its own. I believe that 80% of the problems here are occurring because we used a crude substitution method that got the priorities backwards from the way Oracle does it. Fair Enough. I just hope what every solution the community decides upon solves this problem. It is a very annoying problem to track down and I tend to get even more agitated when I figure out this is the problem. I do not want to distract from the release efforts, so I will withhold further comments until the 8.5 development cycle. Thanks, - Ryan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1704)
As I promised last week, SE-PostgreSQL patches are revised here:
[1/5] http://sepgsql.googlecode.com/files/sepgsql-core-8.4devel-r1704.patch
[2/5] http://sepgsql.googlecode.com/files/sepgsql-utils-8.4devel-r1704.patch
[3/5] http://sepgsql.googlecode.com/files/sepgsql-policy-8.4devel-r1704.patch
[4/5] http://sepgsql.googlecode.com/files/sepgsql-docs-8.4devel-r1704.patch
[5/5] http://sepgsql.googlecode.com/files/sepgsql-tests-8.4devel-r1704.patch
* List of updates:
- It is rebased to the latest CVS HEAD.
- The two reader permission (select and use) are integrated into
a single one (select) as the original design did two year's ago.
(It also enables to pick up read columns from rte-selectedCols.)
- The 'walker' code in sepgsql/checker.c is removed.
In the previous revision, SE-PostgreSQL walked on the given query
tree to pick up all the appeared tables/columns. The reason why
we needed separated walker phase is SE-PostgreSQL wanted to apply
two kind of reader permissions, but these are integrated into one.
(In addition, column-level privileges are not available when I
started to develop SE-PostgreSQL. :-))
In the currect version, SE-PostgreSQL knows what tables/columns
are appeared in the given query, from relid, selectedCols and
modifiedCols in RangeTblEntry. Then, it makes access controls
decision communicating with in-kernel SELinux.
After the existing DAC are checked, SE-PostgreSQL also checks
client's privileges on the appeared tables/columns as DAC doing.
Required privilges are follows these rules:
* If ACL_SELECT is set on rte-requiredPerms, client need to have
db_table:{select} and db_column:{select} for the tables/columns.
* If ACL_INSERT is set on rte-requiredPerms, client need to have
db_table:{insert} and db_column:{insert} for the tables/columns.
* If ACL_UPDATE is set on rte-requiredPerms, client need to have
db_table:{update} and db_column:{update} for the tables/columns.
* If ACL_DELETE is set on rte-requiredPerms, client need to have
db_table:{delete} for the tables.
This design change gives us a great benefit in code maintenance.
A trade-off is hardwired rules to modify pg_rewrite system catalog.
The correctness access controls depends on this catalog is protected
from unexpected manipulation by hand, because it stores a parsed
query tree of views.
- T_SelinuxItem is removed from include/node/nodes.h, and the codes
related to the node type is eliminated from copyfuncs.c ant others.
It was used to store all appeared tables/columns in the walker phase,
but now it is unnecessary.
- Several functions are moved to appropriate files:
- The codes related to permission bits are consolidated to
'sepgsql/perms.c' as its filename means.
(It was placed at 'avc.c' due to historical reason.)
- A few hooks (such as sepgsqlHeapTupleInsert) are moved to 'checker.c',
and rest of simple hooks are kept in 'hooks.c'.
- The scale of patches become a bit slim more.
rev.1668 rev.1704
security/Makefile| 11 - | 11
security/sepgsql/Makefile| 16 - | 16
security/sepgsql/avc.c | 1157 +++ - | 837 +
security/sepgsql/checker.c | 902 +- | 538 +++
security/sepgsql/core.c | 235 +- | 247 +
security/sepgsql/dummy.c | 37 - | 43
security/sepgsql/hooks.c | 748 - | 621 +++
security/sepgsql/label.c | 360 ++ - | 360 ++
security/sepgsql/perms.c | 295 +- | 400 ++
[kai...@saba ~]$ diffstat sepgsql-core-8.4devel-r1668.patch
:
64 files changed, 4770 insertions(+), 11 deletions(-), 4947 modifications(!)
[kai...@saba ~]$ diffstat sepgsql-core-8.4devel-r1704.patch
:
60 files changed, 4048 insertions(+), 11 deletions(-), 4944 modifications(!)
About 700 lines can be reduced in total!
I believe this revision can reduce the burden of reviewers.
Please any comments!
* An issue:
I found a new issue in this revision.
- ACL_SELECT_FOR_UPDATE and ACL_UPDATE have identical value.
In this version, SE-PostgreSQL knows what permission should be checked
via RangeTblEntry::requiredPerms, and it applies its access control
policy with translating them into SELinux's permissions.
But we have a trouble in the following query.
[kai...@saba ~]$ psql postgres
psql (8.4devel)
Type help for help.
postgres=# CREATE TABLE t1 (a int, b text)
postgres-# SECURITY_LABEL = 'system_u:object_r:sepgsql_ro_table_t:s0';
CREATE TABLE
-- NOTE: sepgsql_ro_table_t means read-only table from unpriv clients.
postgres=# INSERT INTO t1 VALUES (1, 'aaa'), (2, 'bbb');
INSERT 0 2
postgres=# \q
[kai...@saba ~]$ runcon -t sepgsql_test_t -- psql postgres
psql (8.4devel)
Type help for help.
-- NOTE: sepgsql_test_t means unpriv client.
Re: [HACKERS] Out parameters handling
2009/3/9 Ryan Bradetich rbradet...@gmail.com: On Sun, Mar 8, 2009 at 4:36 PM, Tom Lane t...@sss.pgh.pa.us wrote: Ryan Bradetich rbradet...@gmail.com writes: This is one of the things I wanted to start looking at for 8.5. My idea was to optionally use : or @ (not sure which is more popular) to specify this token is only a variable. This whole line of thought is really a terrible idea IMHO. plpgsql is supposed to follow Oracle's pl/sql syntax, not invent random syntax of its own. I believe that 80% of the problems here are occurring because we used a crude substitution method that got the priorities backwards from the way Oracle does it. Fair Enough. I just hope what every solution the community decides upon solves this problem. It is a very annoying problem to track down and I tend to get even more agitated when I figure out this is the problem. I do not want to distract from the release efforts, so I will withhold further comments until the 8.5 development cycle. We could relative simple don't add OUT variables into namespace. Personally I prefer using dynamic sql for this case - 8.4 will support RETURN QUERY EXECUTE too, but I don't see big problem in following solution. With special interpret parameter #without_out_paramnames (or some similar) we should protect nice out variables. /* out parameters are accessible via $notation */ create function foo(OUT nicevar integer) returns setof record as $$ #without_out_paramnames begin return query select nicevar from . end $$ language ... with dynamic sql it is easy too create function foo(out nicevar integer) returns ... begin return query execute 'select nicevar from ... ' end $$ language regard Pavel Stehule some special prefixes or special syntax is some what I dislike. Thanks, - Ryan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] status of remaining patches
On Sun, Mar 8, 2009 at 5:03 AM, Robert Haas robertmh...@gmail.com wrote: The original patch was submitted by Koichi Suzuki - quite a few other people have looked at it and provided comments. Simon Riggs was assigned as the original reviewer, but for some reason Dave Page removed his name from the wiki a few days ago (I'm fixing this now). That was because Simon no longer has time to review it. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1704)
KaiGai Kohei wrote: As I promised last week, SE-PostgreSQL patches are revised here: There's checks for reading/writing files with COPY, in sepgsqlCheckFileRead sepgsqlCheckFileWrite). Doesn't the OS do similar checks when the process tries to invoke the read()/write()? Is that not enough? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1704)
KaiGai Kohei wrote:
As I promised last week, SE-PostgreSQL patches are revised here:
I think I now understand what sepgsqlCheckProcedureInstall is trying to
achieve. It's trying to stop attacks where you trick another user to run
your malicious code. We had a serious vulnerability of that kind a while
ago (http://archives.postgresql.org//pgsql-hackers/2008-01/msg00268.php)
when ANALYZE and VACUUM FULL ran expression and partial index predicates
with (typically) superuser privileges.
It seems that sepgsqlCheckProcedureInstall is trying to provide a more
thorough solution to the trojan horse problem than what we did back
then. It stops you from installing an untrusted function as a type
input/output function, operator implementing function etc. Now that
could be useful on its own, quite apart from the rest of the
SE-PostgreSQL patch, in which case I'd like to see that implemented as a
separate patch, so that you can use the facility even if you're not
using SE-PostgreSQL.
Some details of that:
+ void
+ sepgsqlCheckProcedureInstall(Relation rel, HeapTuple newtup, HeapTuple oldtup)
+ {
+ /*
+* db_procedure:{install} check prevent a malicious functions
+* to be installed, as a part of system catalogs.
+* It is necessary to prevent other person implicitly to invoke
+* malicious functions.
+*/
+ switch (RelationGetRelid(rel))
+ {
+ case AggregateRelationId:
+ /*
+* db_procedure:{execute} is checked on invocations of:
+* pg_aggregate.aggfnoid
+* pg_aggregate.aggtransfn
+* pg_aggregate.aggfinalfn
+*/
+ break;
+
+ case AccessMethodRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_am, aminsert, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, ambeginscan, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amgettuple, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amgetbitmap, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amrescan, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amendscan, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, ammarkpos, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amrestrpos, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, ambuild, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, ambulkdelete, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amvacuumcleanup, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amcostestimate, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amoptions, newtup, oldtup);
+ break;
ISTM that you should just forbid any changes to pg_am in the default
policy. That's very low level stuff. If you can modify that, you can
wreck a lot of havoc, quite possibly turning it into a vulnerability
even if you can't directly install a malicious function there.
+ case AccessMethodProcedureRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_amproc, amproc, newtup, oldtup);
+ break;
+
+ case CastRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_cast, castfunc, newtup, oldtup);
+ break;
We check execute permission on the cast function at runtime.
+ case ConversionRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_conversion, conproc, newtup, oldtup);
+ break;
This ought to be unnecessary now. Only C-functions can be installed as
conversion procs, and a C function can do anything, so there's little
point in checking this anymore.
+ case ForeignDataWrapperRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_foreign_data_wrapper, fdwvalidator,
newtup, oldtup);
+ break;
Hmm, calls to fdwvalidator are not at all performance critical, so maybe
we should just check execute permission when it's called.
+ case LanguageRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_language, lanplcallfoid, newtup,
oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_language, lanvalidator, newtup,
oldtup);
+ break;
I think these need to be C-functions.
+ case OperatorRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_operator, oprcode, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_operator, oprrest, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_operator, oprjoin, newtup, oldtup);
+ break;
oprcode is checked for execute permission when the operator is used. For
oprrest and oprjoin, we could add checks into the planner where they're
called. (pg_operator.oprcom and pg_operator.oprnegate are missing?)
+ case TSParserRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_ts_parser, prsstart, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_ts_parser, prstoken, newtup, oldtup);
+
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1704)
Heikki Linnakangas wrote: KaiGai Kohei wrote: As I promised last week, SE-PostgreSQL patches are revised here: There's checks for reading/writing files with COPY, in sepgsqlCheckFileRead sepgsqlCheckFileWrite). Doesn't the OS do similar checks when the process tries to invoke the read()/write()? Is that not enough? Please note that who invokes read()/write() system calls. In this case, PostgreSQL server process invokes these system calls instead of the client process. So, operating system need to allow the PostgreSQL server process to invoke these system calls on the target files of COPY TO/FROM. In addition, SE-PostgreSQL also checks read/write permission of client process for these files. Why it is possible is client's privileges are represented in same form of operating system. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] libxml incompatibility
On 6 mar, 22:44, and...@dunslane.net (Andrew Dunstan) wrote: Holger Hoffstaette wrote: On Fri, 06 Mar 2009 14:32:25 -0600, Kenneth Marshall wrote: On Fri, Mar 06, 2009 at 02:58:30PM -0500, Andrew Dunstan wrote: Yes, I discovered this a few weeks ago. [...] Maybe someone can trace the libxml calls ... not sure how exactly ... given Alvaro's example, it doesn't seem likely to me that this is due to a call to xmlCleanupParser(), but maybe the perl code invokes by simply doing use XML::LibXML; calls that for some perverse reason. I'm able to duplicate this on Postgres 8.4 (Debian Etch, XML::LibXML from CPAN). Here's the backtrace from the crash: #0 0x082f3cf1 in MemoryContextAlloc () #1 0x082c3f8a in xml_palloc () #2 0xb7dfa548 in xmlInitCharEncodingHandlers () from /usr/lib/ libxml2.so.2 #3 0xb7e0195e in xmlInitParser () from /usr/lib/libxml2.so.2 #4 0xb7dff2ef in xmlCheckVersion () from /usr/lib/libxml2.so.2 #5 0xb573af2e in boot_XML__LibXML () from /usr/local/lib/perl/5.8.8/auto/XML/LibXML/LibXML.so #6 0xb587981b in Perl_pp_entersub () from /usr/lib/libperl.so.5.8 #7 0xb5877f19 in Perl_runops_standard () from /usr/lib/libperl.so.5.8 #8 0xb5819b6e in Perl_magicname () from /usr/lib/libperl.so.5.8 #9 0xb581a844 in Perl_call_sv () from /usr/lib/libperl.so.5.8 ... Is it supposed to be OK to call xmlCheckVersion() more than once? -- DLL -- 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] Updates of SE-PostgreSQL 8.4devel patches (r1704)
KaiGai Kohei wrote: As I promised last week, SE-PostgreSQL patches are revised here: The patch adds permission checks to SET/SHOW. If that's useful functionality, it would be nice to see that as a separate patch, not requiring SE-Linux. I think it's leaking because current_setting(), set_config() and pg_show_all_settings() functions don't perform the same checks. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] small parallel restore optimization
Alvaro Herrera wrote: Tom Lane wrote: Worst case, we could say that parallel restore isn't supported on mingw. I'm not entirely sure why we bother with that platform at all... I think you're confusing it with cygwin ... Yeah. Much as I hate working around the quirks of mingw, I definitely think we need to keep that platform. (yes, cygwin is another story, but let's not repeat that discussion now) //Magnus -- 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] small parallel restore optimization
Andrew Dunstan wrote: Alvaro Herrera wrote: Andrew Dunstan wrote: I have found the source of the problem I saw. dumputils.c:fmtId() uses a static PQExpBuffer which it initialises the first time it's called. This gets clobbered by simultaneous calls by Windows threads. I could just make it auto and set it up on each call, but that could result in a non-trivial memory leak ... it's probably called a great many times. Or I could provide a parallel version where we pass in a PQExpBuffer that we create, one per thread, and is used by anything called by the parallel code. That seems like a bit of a potential footgun, though. Could you use a different static PQExpBuffer on each thread? pthread_getspecific sort of thing, only to be used on Windows. For MSVC we could declare it with _declspec(thread) and it would be allocated in thread-local storage, but it looks like this isn't supported on Mingw. Yeah, _declspec(thread) would be the easy way to do it. But you can also use the TlsAlloc(), TlsSetValue() and friends functions directly. We do this to use TLS in ecpg. It requires some coding around, but for ecpg we did a macro that makes it behave like the posix functions (see src/interfaces/ecpg/include/ecpg-pthread-win32.h) //Magnus -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] problem inserting in GIN index
Hi, A guy just reported on pgsql-es-ayuda that he's getting ERROR: item pointer (543108,2) already exists Apparently this message only occurs on GIN, in insertItemPointer Reading that routine I cannot help but wonder -- where is gdi-btree.curitem incremented? http://archives.postgresql.org/message-id/521014.20415.qm%40web52103.mail.re2.yahoo.com -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] libxml incompatibility
David Lee Lambert wrote: On 6 mar, 22:44, and...@dunslane.net (Andrew Dunstan) wrote: Holger Hoffstaette wrote: On Fri, 06 Mar 2009 14:32:25 -0600, Kenneth Marshall wrote: On Fri, Mar 06, 2009 at 02:58:30PM -0500, Andrew Dunstan wrote: Yes, I discovered this a few weeks ago. [...] Maybe someone can trace the libxml calls ... not sure how exactly ... given Alvaro's example, it doesn't seem likely to me that this is due to a call to xmlCleanupParser(), but maybe the perl code invokes by simply doing use XML::LibXML; calls that for some perverse reason. I'm able to duplicate this on Postgres 8.4 (Debian Etch, XML::LibXML from CPAN). Here's the backtrace from the crash: #0 0x082f3cf1 in MemoryContextAlloc () #1 0x082c3f8a in xml_palloc () #2 0xb7dfa548 in xmlInitCharEncodingHandlers () from /usr/lib/ libxml2.so.2 #3 0xb7e0195e in xmlInitParser () from /usr/lib/libxml2.so.2 #4 0xb7dff2ef in xmlCheckVersion () from /usr/lib/libxml2.so.2 #5 0xb573af2e in boot_XML__LibXML () from /usr/local/lib/perl/5.8.8/auto/XML/LibXML/LibXML.so #6 0xb587981b in Perl_pp_entersub () from /usr/lib/libperl.so.5.8 #7 0xb5877f19 in Perl_runops_standard () from /usr/lib/libperl.so.5.8 #8 0xb5819b6e in Perl_magicname () from /usr/lib/libperl.so.5.8 #9 0xb581a844 in Perl_call_sv () from /usr/lib/libperl.so.5.8 ... Is it supposed to be OK to call xmlCheckVersion() more than once? You are certainly not supposed to call xmlInitParser more than once - see http://xmlsoft.org/html/libxml-parser.html#xmlInitParser Since this is being called by xmlCheckVersion(), that looks like a bug in libxml2. Even if this were fixed, however, I'm still not convinced that we'll be able to call libxml2 from perl after we've installed our memory handler (xml_palloc). cheers andrew -- 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] problem inserting in GIN index
Alvaro Herrera alvhe...@commandprompt.com writes: A guy just reported on pgsql-es-ayuda that he's getting ERROR: item pointer (543108,2) already exists Test case? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] problem inserting in GIN index
Tom Lane wrote: Alvaro Herrera alvhe...@commandprompt.com writes: A guy just reported on pgsql-es-ayuda that he's getting ERROR: item pointer (543108,2) already exists Test case? Apparently there's a crash involved ... -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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] Updates of SE-PostgreSQL 8.4devel patches (r1704)
Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes: KaiGai Kohei wrote: As I promised last week, SE-PostgreSQL patches are revised here: The patch adds permission checks to SET/SHOW. If that's useful functionality, it would be nice to see that as a separate patch, not requiring SE-Linux. My goodness. This patch seems to be going FAR beyond what I thought its charter was. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Visibility function comment addition
I have applied the following comment to summarize the visibility rules. I also added a URL about the Halloween problem. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + Index: src/backend/utils/time/tqual.c === RCS file: /cvsroot/pgsql/src/backend/utils/time/tqual.c,v retrieving revision 1.111 diff -c -c -r1.111 tqual.c *** src/backend/utils/time/tqual.c 1 Jan 2009 17:23:53 - 1.111 --- src/backend/utils/time/tqual.c 9 Mar 2009 12:39:09 - *** *** 26,31 --- 26,50 * subtransactions of our own main transaction and so there can't be any * race condition. * + * Summary of visibility functions: + * + * HeapTupleSatisfiesMVCC() + *visible to supplied snapshot, excludes current command + * HeapTupleSatisfiesNow() + *visible to instant snapshot, excludes current command + * HeapTupleSatisfiesUpdate() + *like HeapTupleSatisfiesNow(), but with user-supplied command + *counter and more complex result + * HeapTupleSatisfiesSelf() + *visible to instant snapshot and current command + * HeapTupleSatisfiesDirty() + *like HeapTupleSatisfiesSelf(), but includes open transactions + * HeapTupleSatisfiesVacuum() + *visible to any running transaction, used by VACUUM + * HeapTupleSatisfiesToast() + *visible unless part of interrupted vacuum, used for TOAST + * HeapTupleSatisfiesAny() + *all tuples are visible * * Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California *** *** 277,283 * * Note we do _not_ include changes made by the current command. This * solves the Halloween problem wherein an UPDATE might try to re-update ! * its own output tuples. * * Note: * Assumes heap tuple is valid. --- 296,302 * * Note we do _not_ include changes made by the current command. This * solves the Halloween problem wherein an UPDATE might try to re-update ! * its own output tuples, http://en.wikipedia.org/wiki/Halloween_Problem. * * Note: * Assumes heap tuple is valid. -- 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] V4 of PITR performance improvement for 8.4
Fujii Masao wrote: On Tue, Mar 3, 2009 at 1:47 PM, Fujii Masao masao.fu...@gmail.com wrote: Hi Suzuki-san, On Thu, Feb 26, 2009 at 5:03 AM, Koichi Suzuki koichi@gmail.com wrote: My reply to Gregory's comment didn't have any objections. I believe, as I posted to Wiki page, latest posted patch is okay and waiting for review. One of your latest patches doesn't match with HEAD, so I updated it. Oops! I failed in attaching the patch. This is second try. Thanks. This patch seems to be missing the new readahead.c file. I grabbed that from the previous patch version. It's annoying that we have to write *_readahead functions for each and every record type. In most record types, we already pass the information about the pages involved to XLogInsert, for full page writes. If we could change the WAL format so that that information is stored in WAL even when a full page image is taken, we could do the read-ahead for every WAL record type in a single function. Getting the API right needs some thinking, but that would be a lot nicer approach in the long run. I agree with Itagaki-san's earlier comment that we should find a way to avoid the ReadAheadHasRoom calls (http://archives.postgresql.org/message-id/20090114101659.89cd.52131...@oss.ntt.co.jp). Let's just leave enough slack in the queue, so that it never fills up. And if the unthinkable happens and it does fill up, we can just throw away the readahead requests that don't fit; they're just hints anyway. The API for ReadAheadAddEntry should include ForkNumber. Although all the readahead calls included in the patch all access the main fork, that's really just an omission that probably should be fixed even though the FSM and visibility map should become cached pretty quickly for any active tables. effective_io_concurrency setting is ignored. If it's set to 1, we should disable prefetching altogether for the sake of both robustness (let you recover even if there's a bug in the readahead code) and performance (avoid useless posix_fadvise calls, sorting etc. if there's only one spindle). The bursty queuing behavior feels pretty strange to me, though I guess it works pretty well in practice. I would've assumed a FIFO queue of WAL records, with some kind of a cache of recently issued posix_fadvise() calls. As the patch stands, it's not limited to archive recovery. The code in readahead.c seems to assume that the readahead queue will always be flushed between xlog segment switch, but that is not enforced in xlog.c. malloc() can return NULL on out of memory. Need to check that, or use palloc() instead. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] status of remaining patches
On Mar 9, 2009, at 4:53 AM, Dave Page dp...@pgadmin.org wrote: On Sun, Mar 8, 2009 at 5:03 AM, Robert Haas robertmh...@gmail.com wrote: The original patch was submitted by Koichi Suzuki - quite a few other people have looked at it and provided comments. Simon Riggs was assigned as the original reviewer, but for some reason Dave Page removed his name from the wiki a few days ago (I'm fixing this now). That was because Simon no longer has time to review it. Ah, OK, makes sense. Sorry, was not aware. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1704)
* Tom Lane (t...@sss.pgh.pa.us) wrote: Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes: KaiGai Kohei wrote: As I promised last week, SE-PostgreSQL patches are revised here: The patch adds permission checks to SET/SHOW. If that's useful functionality, it would be nice to see that as a separate patch, not requiring SE-Linux. My goodness. This patch seems to be going FAR beyond what I thought its charter was. I agree. I thought the idea was that the first round of SE-PostgreSQL additions would be to add SE hooks for permissions that PG already implements. Other permissions would then be implemented in a PG-way first, and SE hooks then added to those later. Stephen signature.asc Description: Digital signature
Re: [HACKERS] Sampling Profler for Postgres
Em Seg, 2009-03-09 às 13:55 +0900, ITAGAKI Takahiro escreveu: Therefore, I'd like to propose an profiler with sampling approach in 8.5. The attached patch is an experimental model of the profiler. Each backends reports its condtion in PgBackendStatus.st_condition and the stats collector process does polling them every seconds. Hi Takahiro! Compiled and Works fine here on Ubuntu 8.04 2.6.25.15-bd-mod #1 SMP PREEMPT Thu Nov 27 10:05:44 BRST 2008 i686 GNU/Linux d...@analise3:/srv/postgresql/HEAD$ ./bin/pgbench -i -s3 d...@analise3:/srv/postgresql/HEAD$ ./bin/pgbench -i -s3 -d postgres transaction type: TPC-B (sort of) scaling factor: 3 query mode: simple number of clients: 4 duration: 60 s number of transactions actually processed: 3730 tps = 62.090946 (including connections establishing) tps = 62.112183 (excluding connections establishing) d...@analise3:/srv/postgresql/HEAD$ ./bin/psql -c SELECT * FROM pg_diff_profiles -d postgres profid | profname | percent +--+- 15 | Network:Recv | 50.45 16 | Network:Send | 24.55 32 | Lock:Transaction |7.14 3 | CPU |5.80 20 | XLog:Flush |3.13 31 | Lock:Tuple |2.68 7 | CPU:Execute |1.79 6 | CPU:Plan |1.79 46 | LWLock:WALWrite |1.34 11 | CPU:Commit |0.89 19 | XLog:Write |0.45 (11 rows) Two questions here: 1) How will be this behavior in a syncrep environment? I don't have one here to test this, yet. 2) I couldn't find a clear way to disable it. There is one in this patch or are you planning this to future? Regards, -- Dickson S. Guedes mail/xmpp: gue...@guedesoft.net - skype: guediz http://guedesoft.net - http://planeta.postgresql.org.br -- 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] problem inserting in GIN index
2009/3/9 Alvaro Herrera alvhe...@commandprompt.com: Tom Lane wrote: Alvaro Herrera alvhe...@commandprompt.com writes: A guy just reported on pgsql-es-ayuda that he's getting ERROR: item pointer (543108,2) already exists Test case? Apparently there's a crash involved ... I asked to Gabriel. The exactly version is 8.3.6. He just deleted the indexes :( -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Emanuel Calvo Franco Sumate al ARPUG ! (www.postgres-arg.org - www.arpug.com.ar) ArPUG / AOSUG Member Postgresql Support Admin -- 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] Allow on/off as input texts for boolean.
ITAGAKI Takahiro wrote: Peter Eisentraut pete...@gmx.net wrote: ITAGAKI Takahiro wrote: Here is a patch to allow 'on' and 'off' as input texts for boolean. Regarding your FIXME comment, I think parse_bool* should be in bool.c and declared in builtins.h, which guc.c already includes. (Conceptually, the valid format of a bool should be drived by the boolean type, not the GUC system, I think.) Here is an updated patch to move parse_bool* into bool.c. I also added tests of on/off values to the regression test. applied -- 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] Updates of SE-PostgreSQL 8.4devel patches (r1704)
Heikki Linnakangas wrote: KaiGai Kohei wrote: As I promised last week, SE-PostgreSQL patches are revised here: I think I now understand what sepgsqlCheckProcedureInstall is trying to achieve. It's trying to stop attacks where you trick another user to run your malicious code. We had a serious vulnerability of that kind a while ago (http://archives.postgresql.org//pgsql-hackers/2008-01/msg00268.php) when ANALYZE and VACUUM FULL ran expression and partial index predicates with (typically) superuser privileges. It seems that sepgsqlCheckProcedureInstall is trying to provide a more thorough solution to the trojan horse problem than what we did back then. It stops you from installing an untrusted function as a type input/output function, operator implementing function etc. Now that could be useful on its own, quite apart from the rest of the SE-PostgreSQL patch, in which case I'd like to see that implemented as a separate patch, so that you can use the facility even if you're not using SE-PostgreSQL. Yes, the purpose of sepgsqlCheckProcedureInstall() is to prevent users to invoke functions installed by other malicious/untrusted one, typically known as trojan-horse. Basically, I can agree the vanilla PostgreSQL also provide similar stuff to prevent to install untrusted functions as a part of system internal codes. If we have such a facility as a basis, we can implement sepgsqlCheckProcedureInstall() hook more simple and easier to maintenance. [snip] + case ConversionRelationId: + CHECK_PROC_INSTALL_PERM(pg_conversion, conproc, newtup, oldtup); + break; This ought to be unnecessary now. Only C-functions can be installed as conversion procs, and a C function can do anything, so there's little point in checking this anymore. We should not assume only C-functions can be installed on pg_conversion (and other internal stuff), because a superuser can update system catalog by hand. Example) postgres=# CREATE OR REPLACE FUNCTION testfn() postgres-# RETURNS int LANGUAGE sql AS 'SELECT 1'; CREATE FUNCTION postgres=# UPDATE pg_conversion SET conproc = 'testfn'::regproc where oid=11276; UPDATE 1 postgres=# set client_encoding = 'sjis'; server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. The connection to the server was lost. Attempting reset: WARNING: terminating connection because of crash of another server process DETAIL: The postmaster has commanded this server process to roll back the current transaction and exit, because another server process exited abnormally and possibly corrupted shared memory. HINT: In a moment you should be able to reconnect to the database and repeat your command. Failed. ! SE-PostgreSQL intends to acquire them and apply access control policy in this case also. Aside note: sepgsqlCheckDatabaseInstallModule() check permissions on a newly installed C-library file, to prevent to load a file deployed by untrusted client. + case ForeignDataWrapperRelationId: + CHECK_PROC_INSTALL_PERM(pg_foreign_data_wrapper, fdwvalidator, newtup, oldtup); + break; Hmm, calls to fdwvalidator are not at all performance critical, so maybe we should just check execute permission when it's called. If pg_proc_aclcheck() on its invocation, it is not necessary to check on the installation time. [snip] + case OperatorRelationId: + CHECK_PROC_INSTALL_PERM(pg_operator, oprcode, newtup, oldtup); + CHECK_PROC_INSTALL_PERM(pg_operator, oprrest, newtup, oldtup); + CHECK_PROC_INSTALL_PERM(pg_operator, oprjoin, newtup, oldtup); + break; oprcode is checked for execute permission when the operator is used. For oprrest and oprjoin, we could add checks into the planner where they're called. (pg_operator.oprcom and pg_operator.oprnegate are missing?) If runtime checks are added, it is more desirable. + case TSParserRelationId: + CHECK_PROC_INSTALL_PERM(pg_ts_parser, prsstart, newtup, oldtup); + CHECK_PROC_INSTALL_PERM(pg_ts_parser, prstoken, newtup, oldtup); + CHECK_PROC_INSTALL_PERM(pg_ts_parser, prsend, newtup, oldtup); + CHECK_PROC_INSTALL_PERM(pg_ts_parser, prsheadline, newtup, oldtup); + CHECK_PROC_INSTALL_PERM(pg_ts_parser, prslextype, newtup, oldtup); + break; + + case TSTemplateRelationId: + CHECK_PROC_INSTALL_PERM(pg_ts_template, tmplinit, newtup, oldtup); + CHECK_PROC_INSTALL_PERM(pg_ts_template, tmpllexize, newtup, oldtup); + break; Not sure about these. Maybe we should add checks to where these are called. I'll check the behavior of them tomorrow. + case TypeRelationId: + CHECK_PROC_INSTALL_PERM(pg_type, typinput, newtup, oldtup); + CHECK_PROC_INSTALL_PERM(pg_type, typoutput, newtup, oldtup); + CHECK_PROC_INSTALL_PERM(pg_type, typreceive,
Re: [HACKERS] [PATCH] Regression test fix for Czech locale
Zdenek Kotala wrote: I attached fix which modify foreign_data test. It fix problem with Czech collation when numbers are ordered after letters. I retyped affected column to name datatype which uses bitwise cmp. I have chosen a different fix: rename the identifiers so the ordering problem doesn't arise. Czech locale should work now. -- 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] small parallel restore optimization
Magnus Hagander wrote: Andrew Dunstan wrote: Alvaro Herrera wrote: Andrew Dunstan wrote: I have found the source of the problem I saw. dumputils.c:fmtId() uses a static PQExpBuffer which it initialises the first time it's called. This gets clobbered by simultaneous calls by Windows threads. I could just make it auto and set it up on each call, but that could result in a non-trivial memory leak ... it's probably called a great many times. Or I could provide a parallel version where we pass in a PQExpBuffer that we create, one per thread, and is used by anything called by the parallel code. That seems like a bit of a potential footgun, though. Could you use a different static PQExpBuffer on each thread? pthread_getspecific sort of thing, only to be used on Windows. For MSVC we could declare it with _declspec(thread) and it would be allocated in thread-local storage, but it looks like this isn't supported on Mingw. Yeah, _declspec(thread) would be the easy way to do it. But you can also use the TlsAlloc(), TlsSetValue() and friends functions directly. We do this to use TLS in ecpg. It requires some coding around, but for ecpg we did a macro that makes it behave like the posix functions (see src/interfaces/ecpg/include/ecpg-pthread-win32.h) Yeah, we'll just have to call TlsAlloc to set the thread handle somewhere near program start, but that shouldn't be too intrusive. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] One less footgun: deprecating pg_dump -d
Attached is a patch to fix the annoying footgun that is pg_dump -d. Myself and
many others I know have all at one time or another done this:
psql -h localhost -U greg -d postgres
pg_dump -h localhost -U greg -d postgres dumpfile
The latter command silently succeeds, but only through the combination of -d
being an option that takes no arguments, and the fact that 'postgres' is read by
pg_dump as a separate argument indicating which database to use. Thus, your dump
file now has INSERTs when you wanted to use COPY (as you want your database
restore to take 20 minutes, not three hours).
I thought about changing -d to actually indicate the database, as in psql, but
that brings about another problem: the command above will still silently work,
but produce a dump without INSERTs. While this is good for people who meant to
leave the -d out, it's not good for people (and scripts) that DID want the -d to
work as documented. Thus, changing it will silently break those scripts (until
they try to load the schema into a non-PG database...).
The solution I came up with is to use a new letter, -I, and to deprecate -d by
having it throw an exception when used. The choice of -I seems appropriate as a
shortcut for --inserts, and (as far as I can tell) does not conflict with any
other programs (e.g. psql). Doing so will require people to rewrite any scripts
that are using -d instead of --inserts, but it seems a small price to eliminate
this nasty footgun. As a bonus, their scripts will be easier to read, as -d was
confusing at best, and hardly mnemonic.
--
Greg Sabino Mullane g...@endpoint.com g...@turnstep.com
End Point Corporation
PGP Key: 0x14964AC8
Index: doc/src/sgml/ref/pg_dump.sgml
===
RCS file: /projects/cvsroot/pgsql/doc/src/sgml/ref/pg_dump.sgml,v
retrieving revision 1.112
diff -c -r1.112 pg_dump.sgml
*** doc/src/sgml/ref/pg_dump.sgml 4 Mar 2009 11:57:00 - 1.112
--- doc/src/sgml/ref/pg_dump.sgml 9 Mar 2009 15:02:47 -
***
*** 176,182
/varlistentry
varlistentry
! termoption-d/option/term
termoption--inserts/option/term
listitem
para
--- 176,182
/varlistentry
varlistentry
! termoption-I/option/term
termoption--inserts/option/term
listitem
para
***
*** 190,196
Note that
the restore might fail altogether if you have rearranged column order.
The option-D/option option is safe against column order changes,
! though even slower.
/para
/listitem
/varlistentry
--- 190,197
Note that
the restore might fail altogether if you have rearranged column order.
The option-D/option option is safe against column order changes,
! though even slower. (The option-d/option has been deprectated, as it
! was too similar to the same option as used by psql).
/para
/listitem
/varlistentry
Index: doc/src/sgml/ref/pg_dumpall.sgml
===
RCS file: /projects/cvsroot/pgsql/doc/src/sgml/ref/pg_dumpall.sgml,v
retrieving revision 1.78
diff -c -r1.78 pg_dumpall.sgml
*** doc/src/sgml/ref/pg_dumpall.sgml 4 Mar 2009 11:57:00 - 1.78
--- doc/src/sgml/ref/pg_dumpall.sgml 9 Mar 2009 15:02:47 -
***
*** 99,105
/varlistentry
varlistentry
! termoption-d/option/term
termoption--inserts/option/term
listitem
para
--- 99,105
/varlistentry
varlistentry
! termoption-I/option/term
termoption--inserts/option/term
listitem
para
***
*** 109,114
--- 109,116
non-productnamePostgreSQL/productname databases. Note that
the restore might fail altogether if you have rearranged column order.
The option-D/option option is safer, though even slower.
+ (The option-d/option has been deprectated, as it was too similar
+ to the same option as used by psql).
/para
/listitem
/varlistentry
Index: src/bin/pg_dump/pg_dump.c
===
RCS file: /projects/cvsroot/pgsql/src/bin/pg_dump/pg_dump.c,v
retrieving revision 1.528
diff -c -r1.528 pg_dump.c
*** src/bin/pg_dump/pg_dump.c 4 Mar 2009 11:57:00 - 1.528
--- src/bin/pg_dump/pg_dump.c 9 Mar 2009 15:02:48 -
***
*** 246,256
{create, no_argument, NULL, 'C'},
{file, required_argument, NULL, 'f'},
{format, required_argument, NULL, 'F'},
- {inserts, no_argument, NULL, 'd'},
{attribute-inserts, no_argument, NULL, 'D'},
{column-inserts, no_argument, NULL, 'D'},
{host, required_argument, NULL, 'h'},
{ignore-version, no_argument, NULL, 'i'},
{no-reconnect, no_argument, NULL, 'R'},
{oids, no_argument, NULL, 'o'},
Re: [HACKERS] One less footgun: deprecating pg_dump -d
Greg Sabino Mullane g...@endpoint.com writes: The solution I came up with is to use a new letter, -I, and to deprecate -d by having it throw an exception when used. Deprecate does not mean break. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] One less footgun: deprecating pg_dump -d
On Mon, 2009-03-09 at 12:48 -0400, Tom Lane wrote: Greg Sabino Mullane g...@endpoint.com writes: The solution I came up with is to use a new letter, -I, and to deprecate -d by having it throw an exception when used. Deprecate does not mean break. Sorry Tom. Greg is correct here although I disagree with his wording. It should be removed and if someone passes -d it should throw an ERROR that says something like: ERROR: -d has been replaced by -I Greg and I are both in the field and the field consistently uses -d in the wrong way. Joshua D. Drake regards, tom lane -- PostgreSQL - XMPP: jdr...@jabber.postgresql.org Consulting, Development, Support, Training 503-667-4564 - http://www.commandprompt.com/ The PostgreSQL Company, serving since 1997 -- 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] Updates of SE-PostgreSQL 8.4devel patches (r1704)
KaiGai Kohei kai...@kaigai.gr.jp writes: Yes, the purpose of sepgsqlCheckProcedureInstall() is to prevent users to invoke functions installed by other malicious/untrusted one, typically known as trojan-horse. ... We should not assume only C-functions can be installed on pg_conversion (and other internal stuff), because a superuser can update system catalog by hand. ... SE-PostgreSQL intends to acquire them and apply access control policy in this case also. I don't think that anyone except KaiGai-san has bought into the concept that sepostgres should get to override superuser capabilities, much less that it should be trying to control semantics at this kind of level of detail. I've been convinced for awhile that the sepostgres project is going off the rails, and these last couple of exchanges just confirm the fear. This is absolutely *not* the kind of thing that we should be designing four months after feature freeze. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Prepping to break every past release...
On Mon, 2009-03-09 at 13:59 -0400, Bruce Momjian wrote: If this is the worst inconsistency you can find in our system tables after +20 years of development, I feel pretty good. I was using a single example. This would be a large project I am sure and of course we should feel good. In all I would say we are likely one of the more consistent pieces of software in terms of our age. That doesn't mean we shouldn't try to continue to improve. Sincerely, Joshua D. Drake -- PostgreSQL - XMPP: jdr...@jabber.postgresql.org Consulting, Development, Support, Training 503-667-4564 - http://www.commandprompt.com/ The PostgreSQL Company, serving since 1997 -- 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] One less footgun: deprecating pg_dump -d
On Mon, 2009-03-09 at 13:30 -0400, Tom Lane wrote: Joshua D. Drake j...@commandprompt.com writes: Sorry Tom. Greg is correct here although I disagree with his wording. It should be removed and if someone passes -d it should throw an ERROR that says something like: ERROR: -d has been replaced by -I Well, if you want to break it, we can debate about the wisdom of that. But please don't describe the patch in such a misleading way as the current thread title. That's fair. Joshua D. Drake regards, tom lane -- PostgreSQL - XMPP: jdr...@jabber.postgresql.org Consulting, Development, Support, Training 503-667-4564 - http://www.commandprompt.com/ The PostgreSQL Company, serving since 1997 -- 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] One less footgun: deprecating pg_dump -d
Joshua D. Drake j...@commandprompt.com writes: Sorry Tom. Greg is correct here although I disagree with his wording. It should be removed and if someone passes -d it should throw an ERROR that says something like: ERROR: -d has been replaced by -I Well, if you want to break it, we can debate about the wisdom of that. But please don't describe the patch in such a misleading way as the current thread title. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Prepping to break every past release...
If this is the worst inconsistency you can find in our system tables after +20 years of development, I feel pretty good. --- Joshua D. Drake wrote: Hello, Something that continues to grind my teeth about our software is that we are horribly inconsistent with our system catalogs. Now I am fully and 100% aware that changing this will break things in user land but I want to do it anyway. In order to do that I believe we need to come up with a very loud, extremely verbose method of communicating to people that 8.5 *will* break things. It seems to me that the best method would be to follow the information_schema naming conventions as information_schema is standard compliant (right?). Thoughts? Examples: postgres=# \d pg_class Table pg_catalog.pg_class Column | Type| Modifiers +---+--- relname| name | not null relnamespace | oid | not null [...] postgres=# \d pg_tables View pg_catalog.pg_tables Column| Type | Modifiers -+-+--- schemaname | name| tablename | name| postgres=# \d pg_stat_user_tables View pg_catalog.pg_stat_user_tables Column | Type | Modifiers --+--+--- relid| oid | schemaname | name | relname | name | postgres=# \d information_schema.tables View information_schema.tables Column| Type| Modifiers --+---+--- table_catalog| information_schema.sql_identifier | table_schema | information_schema.sql_identifier | table_name | information_schema.sql_identifier | -- PostgreSQL - XMPP: jdr...@jabber.postgresql.org Consulting, Development, Support, Training 503-667-4564 - http://www.commandprompt.com/ The PostgreSQL Company, serving since 1997 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- 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] Updates of SE-PostgreSQL 8.4devel patches (r1704)
On Mon, Mar 9, 2009 at 1:25 PM, Tom Lane t...@sss.pgh.pa.us wrote: KaiGai Kohei kai...@kaigai.gr.jp writes: Yes, the purpose of sepgsqlCheckProcedureInstall() is to prevent users to invoke functions installed by other malicious/untrusted one, typically known as trojan-horse. ... We should not assume only C-functions can be installed on pg_conversion (and other internal stuff), because a superuser can update system catalog by hand. ... SE-PostgreSQL intends to acquire them and apply access control policy in this case also. I don't think that anyone except KaiGai-san has bought into the concept that sepostgres should get to override superuser capabilities, much less that it should be trying to control semantics at this kind of level of detail. I'd find that VERY surprising. One of the major features of MAC systems is that the system policy trumps decisions by individual users, so root or the database superuser is confined by that policy just like everyone else. They may or may not have the ability to change the policy, but that's a separate issue. I've been convinced for awhile that the sepostgres project is going off the rails, and these last couple of exchanges just confirm the fear. I'm not sure what you mean by going off the rails. I think we are still beating our way through what Peter Eisentraut said in one of his first reviews of this patch: SE-PostgreSQL shouldn't implement MAC that isn't a mirror of existing DAC capabilities. If more capabilities are needed, the DAC side of things should be designed and implemented first. Interestingly, Heikki's latest review comments are coming back to exactly this point. So I think we have unanimity that everything that doesn't meet this criterion should be ripped out for now. But I don't see anyone arguing that those capabilities are intrinsically worthless, except possibly you, just that we won't be ready to support them in SE-PostgreSQL until we support them in some more general sense. This is absolutely *not* the kind of thing that we should be designing four months after feature freeze. On this point I am in agreement. We need very much to bring this November CommitFest to an end. Unfortunately, the pace of reviewing slowed dramatically after Thanksgiving and has since dropped to a crawl. However, since the decision to bump Hot Standby was made, things have picked up again, mostly due to a bunch of reviewing by Heikki. The thing we need to do now is make that reviewing reach some conclusion about exactly what needs to be fixed and what of it will be fixed by the author vs. by the committer. It would be easier to make the decision to bump SE-PostgreSQL if it were the only thing holding up beta, but we're not there yet. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] One less footgun: deprecating pg_dump -d
Tom Lane wrote: Greg Sabino Mullaneg...@endpoint.com writes: The solution I came up with is to use a new letter, -I, and to deprecate -d by having it throw an exception when used. Deprecate does not mean break. This '-d' thing is more than just a matter of reading the documentation. Our other command line utilities lead a person to assume (logically) that '-d' means something completely apart from what it actually does. I've made this mistake, so have most other sysadmins I know. While this change may break existing scripts, the result is that future users of Postgres will have a much less painful experience if they accidentally try to use that option. -selena -- Selena Deckelmann End Point Corporation sel...@endpoint.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] One less footgun: deprecating pg_dump -d
Tom Lane wrote: Joshua D. Drake j...@commandprompt.com writes: Sorry Tom. Greg is correct here although I disagree with his wording. It should be removed and if someone passes -d it should throw an ERROR that says something like: ERROR: -d has been replaced by -I Well, if you want to break it, we can debate about the wisdom of that. But please don't describe the patch in such a misleading way as the current thread title. +1 with breaking it, but with a better message (and let's call it breaking, not deprecating). Oh, and the patch contains what looks like two merge failures, I'm sure that wasn't intentional... //Magnus -- 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] One less footgun: deprecating pg_dump -d
Selena Deckelmann wrote: Tom Lane wrote: Greg Sabino Mullaneg...@endpoint.com writes: ... deprecate -d by having it throw an exception when used. Deprecate does not mean break. ... While this change may break existing scripts...less painful Why do people want a failure rather than warning messages being spewed to both stderr and the log files? If someone doesn't notice warnings there, I wonder if even throwing an exception would save them. -- 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] One less footgun: deprecating pg_dump -d
Magnus Hagander mag...@hagander.net wrote: +1 with breaking it, but with a better message (and let's call it breaking, not deprecating). Are you proposing to leave -D as is? -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] One less footgun: deprecating pg_dump -d
Kevin Grittner wrote: Magnus Hagander mag...@hagander.net wrote: +1 with breaking it, but with a better message (and let's call it breaking, not deprecating). Are you proposing to leave -D as is? I was :-) but maybe it's better to use -i and -I, and thus change them both? //Magnus -- 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] One less footgun: deprecating pg_dump -d
Magnus Hagander mag...@hagander.net wrote: Kevin Grittner wrote: Are you proposing to leave -D as is? I was :-) but maybe it's better to use -i and -I, and thus change them both? That's already used: -i, --ignore-version proceed even when server version mismatches pg_dump version -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] One less footgun: deprecating pg_dump -d
Kevin Grittner kevin.gritt...@wicourts.gov writes: Magnus Hagander mag...@hagander.net wrote: but maybe it's better to use -i and -I, and thus change them both? That's already used: -i, --ignore-version proceed even when server version mismatches pg_dump version Proposal: drop the short forms of these two switches entirely. Anybody who actually needs the capability can write --inserts. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] One less footgun: deprecating pg_dump -d
Tom Lane wrote: Kevin Grittner kevin.gritt...@wicourts.gov writes: Magnus Hagander mag...@hagander.net wrote: but maybe it's better to use -i and -I, and thus change them both? That's already used: -i, --ignore-version proceed even when server version mismatches pg_dump version Proposal: drop the short forms of these two switches entirely. Anybody who actually needs the capability can write --inserts. +1. I was just thinking the same thing. cheers andrew -- 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] One less footgun: deprecating pg_dump -d
Andrew Dunstan wrote: Tom Lane wrote: Kevin Grittner kevin.gritt...@wicourts.gov writes: Magnus Hagander mag...@hagander.net wrote: but maybe it's better to use -i and -I, and thus change them both? That's already used: -i, --ignore-version proceed even when server version mismatches pg_dump version Proposal: drop the short forms of these two switches entirely. Anybody who actually needs the capability can write --inserts. +1. I was just thinking the same thing. +1, that sounds like a very good idea. //Magnus -- 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] One less footgun: removing pg_dump -d
-i, --ignore-version proceed even when server version mismatches pg_dump version Proposal: drop the short forms of these two switches entirely. Anybody who actually needs the capability can write --inserts. I thought about something like that, but that would break even more existing scripts than the current patch, no? I'd be all for not using -I though, as that would not break anything. Sorry about the deprecation name, I withdraw that part Magnus: Sorry about non-mergeability, I wrote this while offline... -- Greg Sabino Mullane g...@endpoint.com End Point Corporation PGP Key: 0x14964AC8 signature.asc Description: OpenPGP digital signature
Re: [HACKERS] One less footgun: removing pg_dump -d
Greg Sabino Mullane wrote: -i, --ignore-version proceed even when server version mismatches pg_dump version Proposal: drop the short forms of these two switches entirely. Anybody who actually needs the capability can write --inserts. I thought about something like that, but that would break even more existing scripts than the current patch, no? I'd be all for not using -I though, as that would not break anything. Sorry about the deprecation name, I withdraw that part Magnus: Sorry about non-mergeability, I wrote this while offline... No, the problem is not that I get merge failures. It's that *your* merge conflicts are included in the patch itself. //Magnus -- 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] Updates of SE-PostgreSQL 8.4devel patches (r1704)
Robert Haas robertmh...@gmail.com writes: On Mon, Mar 9, 2009 at 1:25 PM, Tom Lane t...@sss.pgh.pa.us wrote: I've been convinced for awhile that the sepostgres project is going off the rails, and these last couple of exchanges just confirm the fear. I'm not sure what you mean by going off the rails. I think we are still beating our way through what Peter Eisentraut said in one of his first reviews of this patch: SE-PostgreSQL shouldn't implement MAC that isn't a mirror of existing DAC capabilities. If more capabilities are needed, the DAC side of things should be designed and implemented first. Interestingly, Heikki's latest review comments are coming back to exactly this point. So I think we have unanimity that everything that doesn't meet this criterion should be ripped out for now. But I don't see anyone arguing that those capabilities are intrinsically worthless, except possibly you, just that we won't be ready to support them in SE-PostgreSQL until we support them in some more general sense. I'm not saying that I think the capability is intrinsically worthless. What I *am* saying is that I have zero confidence in the current development process, ie one guy producing patches without any previous design discussion. What's missing is 1. Community buy-in on the objectives and user-visible semantics. 2. High-level review of the proposed implementation method. 3. Review of the coding details. We seem to be starting at #3. Now it's not really KaiGai-san's fault; the fundamental problem IMHO is that no one else is taking very much interest in the patch. But that in itself speaks volumes about whether we actually want this patch or should accept it. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1704)
On Mon, Mar 9, 2009 at 4:04 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Mon, Mar 9, 2009 at 1:25 PM, Tom Lane t...@sss.pgh.pa.us wrote: I've been convinced for awhile that the sepostgres project is going off the rails, and these last couple of exchanges just confirm the fear. I'm not sure what you mean by going off the rails. I think we are still beating our way through what Peter Eisentraut said in one of his first reviews of this patch: SE-PostgreSQL shouldn't implement MAC that isn't a mirror of existing DAC capabilities. If more capabilities are needed, the DAC side of things should be designed and implemented first. Interestingly, Heikki's latest review comments are coming back to exactly this point. So I think we have unanimity that everything that doesn't meet this criterion should be ripped out for now. But I don't see anyone arguing that those capabilities are intrinsically worthless, except possibly you, just that we won't be ready to support them in SE-PostgreSQL until we support them in some more general sense. I'm not saying that I think the capability is intrinsically worthless. What I *am* saying is that I have zero confidence in the current development process, ie one guy producing patches without any previous design discussion. What's missing is 1. Community buy-in on the objectives and user-visible semantics. 2. High-level review of the proposed implementation method. 3. Review of the coding details. We seem to be starting at #3. OK, I agree. Now it's not really KaiGai-san's fault; the fundamental problem IMHO is that no one else is taking very much interest in the patch. But that in itself speaks volumes about whether we actually want this patch or should accept it. Are you sure that this isn't just because the original patch was so enormous? If you're referring to reviewing, it's certainly easier to find someone willing to review a 100-line patch than it is to find someone willing to review a 10,000-line patch. But in terms of potential user feedback, there have been a number of people writing in about how much they would like to use this feature, and some security folks have written in with positive comments, too. It also seems to me that with Heikki's feedback this is rapidly shrinking down to a project of managable size and scope. I don't think it's there yet, and maybe it won't get there soon enough to include in 8.4, but it certainly seems to be moving in the right direction. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] One less footgun: deprecating pg_dump -d
On Mon, 2009-03-09 at 15:48 -0400, Tom Lane wrote: Kevin Grittner kevin.gritt...@wicourts.gov writes: Magnus Hagander mag...@hagander.net wrote: but maybe it's better to use -i and -I, and thus change them both? That's already used: -i, --ignore-version proceed even when server version mismatches pg_dump version Proposal: drop the short forms of these two switches entirely. Anybody who actually needs the capability can write --inserts. I could buy into that. Joshua D. Drake regards, tom lane -- PostgreSQL - XMPP: jdr...@jabber.postgresql.org Consulting, Development, Support, Training 503-667-4564 - http://www.commandprompt.com/ The PostgreSQL Company, serving since 1997 -- 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] Updates of SE-PostgreSQL 8.4devel patches (r1704)
Robert Haas robertmh...@gmail.com writes: On Mon, Mar 9, 2009 at 4:04 PM, Tom Lane t...@sss.pgh.pa.us wrote: Now it's not really KaiGai-san's fault; the fundamental problem IMHO is that no one else is taking very much interest in the patch. But that in itself speaks volumes about whether we actually want this patch or should accept it. Are you sure that this isn't just because the original patch was so enormous? If you're referring to reviewing, it's certainly easier to find someone willing to review a 100-line patch than it is to find someone willing to review a 10,000-line patch. Well, the huge size of the original patch didn't help any, for sure. But the nature of this type of problem --- particularly given the not-designed-for-it architecture we have --- is that there are going to be a lot of subtle issues and very probably a lot of places to touch. It gets even worse if you want to put performance constraints on the result. I will not have any confidence in SEPostgres until both the design and the code details have been reviewed by a fair number of experienced PG hackers; and what I see happening is that there simply aren't enough of them who care. If it were a small localized patch I might not particularly care ... but what I'm afraid of is that we'll have a monstrous patch that does severe damage to readability and modifiability of the backend, and has a bunch of bugs to boot (every one of which will qualify as a security issue when it's discovered). And on top of that, I'm still not sold that there is enough of a user base for it to justify the effort we'll have to put into it. If there were, we'd be seeing more interest in reviewing it. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] parallel restore fixes
The attached patch fixes two issues with parallel restore:
* the static buffer problem in dumputils.c::fmtId() on Windows
(solution: use thread-local storage)
* ReopenPtr() is called too often
There is one remaining bug I know of that I can reproduce: we can get
into deadlock when two tables are foreign keyed to each other. So I need
to get a bit more paranoid about dependencies.
I can't reproduce Olivier Prennant's file closing problem on Unixware.
Is it still happening after application of this patch?
cheers
andrew
Index: src/bin/pg_dump/dumputils.c
===
RCS file: /home/cvsmirror/pg/pgsql/src/bin/pg_dump/dumputils.c,v
retrieving revision 1.44
diff -c -r1.44 dumputils.c
*** src/bin/pg_dump/dumputils.c 22 Jan 2009 20:16:07 - 1.44
--- src/bin/pg_dump/dumputils.c 9 Mar 2009 22:33:32 -
***
*** 31,36
--- 31,50
static void AddAcl(PQExpBuffer aclbuf, const char *keyword,
const char *subname);
+ #ifdef WIN32
+ static DWORD tls_index;
+ #endif
+
+ void
+ init_dump_utils()
+ {
+ #ifdef WIN32
+ /* reserve one thread-local slot for the fmtId query buffer address */
+ tls_index = TlsAlloc();
+ #endif
+ }
+
+
/*
* Quotes input string if it's not a legitimate SQL identifier as-is.
***
*** 42,55
--- 56,87
const char *
fmtId(const char *rawid)
{
+ #ifdef WIN32
+ PQExpBuffer id_return;
+ #else
static PQExpBuffer id_return = NULL;
+ #endif
const char *cp;
bool need_quotes = false;
+ char *retval;
+
+ #ifdef WIN32
+ id_return = (PQExpBuffer) TlsGetValue(tls_index); /* returns 0 until set */
+ #endif
if (id_return)/* first time through? */
+ {
+ /* same buffer, just wipe contents */
resetPQExpBuffer(id_return);
+ }
else
+ {
+ /* new buffer */
id_return = createPQExpBuffer();
+ #ifdef WIN32
+ TlsSetValue(tls_index,id_return);
+ #endif
+ }
/*
* These checks need to match the identifier production in scan.l. Don't
***
*** 111,117
appendPQExpBufferChar(id_return, '\');
}
! return id_return-data;
}
--- 143,151
appendPQExpBufferChar(id_return, '\');
}
! retval = id_return-data;
! return retval;
!
}
Index: src/bin/pg_dump/dumputils.h
===
RCS file: /home/cvsmirror/pg/pgsql/src/bin/pg_dump/dumputils.h,v
retrieving revision 1.23
diff -c -r1.23 dumputils.h
*** src/bin/pg_dump/dumputils.h 22 Jan 2009 20:16:07 - 1.23
--- src/bin/pg_dump/dumputils.h 9 Mar 2009 22:33:32 -
***
*** 19,24
--- 19,25
#include libpq-fe.h
#include pqexpbuffer.h
+ extern void init_dump_utils(void);
extern const char *fmtId(const char *identifier);
extern void appendStringLiteral(PQExpBuffer buf, const char *str,
int encoding, bool std_strings);
Index: src/bin/pg_dump/pg_backup_archiver.c
===
RCS file: /home/cvsmirror/pg/pgsql/src/bin/pg_dump/pg_backup_archiver.c,v
retrieving revision 1.165
diff -c -r1.165 pg_backup_archiver.c
*** src/bin/pg_dump/pg_backup_archiver.c 5 Mar 2009 14:51:10 - 1.165
--- src/bin/pg_dump/pg_backup_archiver.c 9 Mar 2009 22:33:32 -
***
*** 3467,3478
/*
* Close and reopen the input file so we have a private file pointer
! * that doesn't stomp on anyone else's file pointer.
*
! * Note: on Windows, since we are using threads not processes, this
! * *doesn't* close the original file pointer but just open a new one.
*/
! (AH-ReopenPtr) (AH);
/*
* We need our own database connection, too
--- 3467,3486
/*
* Close and reopen the input file so we have a private file pointer
! * that doesn't stomp on anyone else's file pointer, if we're actually
! * going to need to read from the file. Otherwise, just close it
! * except on Windows, where it will possibly be needed by other threads.
*
! * Note: on Windows, since we are using threads not processes, the
! * reopen call *doesn't* close the original file pointer but just open
! * a new one.
*/
! if (te-section == SECTION_DATA )
! (AH-ReopenPtr) (AH);
! #ifndef WIN32
! else
! (AH-ClosePtr) (AH);
! #endif;
/*
* We need our own database connection, too
***
*** 3490,3495
--- 3498,3505
PQfinish(AH-connection);
AH-connection = NULL;
+ /* If we reopened the file, we are done with it, so close it now */
+ if (te-section == SECTION_DATA )
(AH-ClosePtr) (AH);
if (retval == 0 AH-public.n_errors)
Index: src/bin/pg_dump/pg_dump.c
===
RCS file: /home/cvsmirror/pg/pgsql/src/bin/pg_dump/pg_dump.c,v
retrieving revision 1.528
diff -c -r1.528 pg_dump.c
*** src/bin/pg_dump/pg_dump.c 4 Mar 2009 11:57:00 - 1.528
--- src/bin/pg_dump/pg_dump.c 9 Mar
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1704)
On Mon, 2009-03-09 at 16:39 -0400, Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: On Mon, Mar 9, 2009 at 4:04 PM, Tom Lane t...@sss.pgh.pa.us wrote: Now it's not really KaiGai-san's fault; the fundamental problem IMHO is that no one else is taking very much interest in the patch. But that in itself speaks volumes about whether we actually want this patch or should accept it. Are you sure that this isn't just because the original patch was so enormous? If you're referring to reviewing, it's certainly easier to find someone willing to review a 100-line patch than it is to find someone willing to review a 10,000-line patch. Well, the huge size of the original patch didn't help any, for sure. But the nature of this type of problem --- particularly given the not-designed-for-it architecture we have --- is that there are going to be a lot of subtle issues and very probably a lot of places to touch. It gets even worse if you want to put performance constraints on the result. I will not have any confidence in SEPostgres until both the design and the code details have been reviewed by a fair number of experienced PG hackers; and what I see happening is that there simply aren't enough of them who care. If it were a small localized patch I might not particularly care ... but what I'm afraid of is that we'll have a monstrous patch that does severe damage to readability and modifiability of the backend, and has a bunch of bugs to boot (every one of which will qualify as a security issue when it's discovered). And on top of that, I'm still not sold that there is enough of a user base for it to justify the effort we'll have to put into it. If there were, we'd be seeing more interest in reviewing it. Can't it be kept separately maintained release for a version or two, so that we will have both PostgreSQL and SE-PostgreSQL and thus have an easy way to compare both correctness and performance ? Anyone remember how did Linux implement/introduce SE Linux ? -- Hannu Krosing http://www.2ndQuadrant.com PostgreSQL Scalability and Availability Services, Consulting and Training -- 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] parallel restore fixes
Andrew Dunstan wrote: The attached patch fixes two issues with parallel restore: * the static buffer problem in dumputils.c::fmtId() on Windows (solution: use thread-local storage) * ReopenPtr() is called too often Hmm, if pg_restore is the only program that's threaded, why are you calling init_dump_utils on pg_dump and pg_dumpall? It makes me a bit nervous because there are some other programs that are linking dumputils.c (psql and some in src/bin/scripts/) and even calling fmtId. Also I think the fmtId comment needs to be updated. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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] parallel restore fixes
Andrew Dunstan and...@dunslane.net writes: + void + init_dump_utils() This should be + void + init_dump_utils(void) please. We don't do KR C around here. I'd lose the added retval variable too; that's not contributing anything. ! #endif; Semicolon is bogus here. Looks pretty sane otherwise. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1704)
Hannu Krosing ha...@2ndquadrant.com writes: Can't it be kept separately maintained release for a version or two, so that we will have both PostgreSQL and SE-PostgreSQL and thus have an easy way to compare both correctness and performance ? Actually, KaiGai-san has been distributing a patched SEPostgres on Fedora for awhile already (and it's been rather a pain in the neck I fear to keep it in sync with the regular distribution). One thing I would love to know is how many people are actually using that, but AFAIK there's no good way to find out. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] parallel restore fixes
Alvaro Herrera alvhe...@commandprompt.com writes: Hmm, if pg_restore is the only program that's threaded, why are you calling init_dump_utils on pg_dump and pg_dumpall? ... because fmtId will crash on *any* use without that. It makes me a bit nervous because there are some other programs that are linking dumputils.c (psql and some in src/bin/scripts/) and even calling fmtId. Actually, why bother with init_dump_utils at all? fmtId could be made to initialize the ID variable for itself on first call, with only one extra if-test, which is hardly gonna matter. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1704)
On Mon, 2009-03-09 at 19:45 -0400, Tom Lane wrote: Hannu Krosing ha...@2ndquadrant.com writes: Can't it be kept separately maintained release for a version or two, so that we will have both PostgreSQL and SE-PostgreSQL and thus have an easy way to compare both correctness and performance ? Actually, KaiGai-san has been distributing a patched SEPostgres on Fedora for awhile already (and it's been rather a pain in the neck I fear to keep it in sync with the regular distribution). One thing I would love to know is how many people are actually using that, but AFAIK there's no good way to find out. To point out the obvious, we are in a quandary here. Nobody argues the feature would be interesting and although I don't have use for it I could see where some people would. I also see where it would open doors for us. Is there any possibility of having it be enabled at compile time? The default would be know but those distributions that would like to make use of it could? I am actually surprised we are not seeing traction on this from SuSE and Redhat. My understanding is that they are both SE Linux supporters. Joshua D. Drake regards, tom lane -- PostgreSQL - XMPP: jdr...@jabber.postgresql.org Consulting, Development, Support, Training 503-667-4564 - http://www.commandprompt.com/ The PostgreSQL Company, serving since 1997 -- 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] Updates of SE-PostgreSQL 8.4devel patches (r1704)
Joshua D. Drake j...@commandprompt.com writes: Is there any possibility of having it be enabled at compile time? That's been assumed right along (unless you think it's okay for Postgres to stop working on every non-SELinux platform). The problem here is mostly about whether we have enough confidence in the code to put our project name on it. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] DTrace doc patch for new probes in 8.4
Patch applied. Thanks. --- Robert Lor wrote: Attached is the doc patch for the recently added probes. -Robert Index: doc/src/sgml/monitoring.sgml === RCS file: /projects/cvsroot/pgsql/doc/src/sgml/monitoring.sgml,v retrieving revision 1.63 diff -u -3 -p -r1.63 monitoring.sgml --- doc/src/sgml/monitoring.sgml 11 Nov 2008 20:06:21 - 1.63 +++ doc/src/sgml/monitoring.sgml 26 Feb 2009 22:18:31 - @@ -1009,23 +1009,25 @@ SELECT pg_stat_get_backend_pid(s.backend productnamePostgreSQL/productname provides facilities to support dynamic tracing of the database server. This allows an external utility to be called at specific points in the code and thereby trace - execution. Currently, this facility is primarily intended for use by - database developers, as it requires substantial familiarity with the code. + execution. /para para - A number of probes or trace points are already inserted - into the source code. By default these probes are not compiled into the + A number of probes or trace points are already inserted into the source + code. These probes are intended to be used by database developers and + administrators. By default the probes are not compiled into the binary, and the user needs to explicitly tell the configure script to make the probes available in productnamePostgreSQL/productname. /para para - Currently, only the DTrace utility is supported, which is available - on Solaris Express, Solaris 10, and Mac OS X Leopard. It is expected that - DTrace will be available in the future on FreeBSD. - Supporting other dynamic tracing utilities is theoretically possible by - changing the definitions for the macros in + Currently, only the + ulink url=http://opensolaris.org/os/community/dtrace/;DTrace/ulink + utility is supported, which is available + on OpenSolaris, Solaris 10, and Mac OS X Leopard. It is expected that + DTrace will be available in the future on FreeBSD and possibly other + operating systems. Supporting other dynamic tracing utilities is + theoretically possible by changing the definitions for the macros in filenamesrc/include/utils/probes.h/. /para @@ -1045,93 +1047,387 @@ SELECT pg_stat_get_backend_pid(s.backend titleBuilt-in Probes/title para - A few standard probes are provided in the source code - (of course, more can be added as needed for a particular problem). - These are shown in xref linkend=trace-point-table. + A number of standard probes are provided in the source code, and more + can certainly be added to enhance PostgreSQL's observability. There are two categories + of probes, those that are targeted toward database administrators and those for developers. + They are shown in xref linkend=admin-trace-point-table and + xref linkend=dev-trace-point-table. /para - table id=trace-point-table - titleBuilt-in Probes/title + table id=admin-trace-point-table + titleBuilt-in Probes for Administrators/title tgroup cols=3 thead row entryName/entry entryParameters/entry - entryOverview/entry + entryDescription/entry /row /thead tbody + row entrytransaction-start/entry - entry(int transactionId)/entry - entryThe start of a new transaction./entry + entry(LocalTransactionId)/entry + entryProbe that fires at the start of a new transaction. arg0 is the transaction id./entry /row row entrytransaction-commit/entry - entry(int transactionId)/entry - entryThe successful completion of a transaction./entry + entry(LocalTransactionId)/entry + entryProbe that fires when a transaction completes successfully. arg0 is the transaction id./entry /row row entrytransaction-abort/entry - entry(int transactionId)/entry - entryThe unsuccessful completion of a transaction./entry + entry(LocalTransactionId)/entry + entryProbes that fires when a transaction does not complete successfully. arg0 is the transaction id./entry +/row +row + entryquery-start/entry + entry(const char *)/entry + entryProbe that fires when the execution of a statement is started. arg0 is the query string./entry +/row +row + entryquery-done/entry + entry(const char *)/entry + entryProbe that fires when the execution of a statement is complete. arg0 is the query string./entry +/row +row + entryquery-parse-start/entry + entry(const char *)/entry + entryProbe that fires when the parsing of a query is started. arg0 is the query string./entry +/row +row + entryquery-parse-done/entry + entry(const char *)/entry + entryProbe
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1704)
On Mon, 2009-03-09 at 20:05 -0400, Tom Lane wrote: Joshua D. Drake j...@commandprompt.com writes: Is there any possibility of having it be enabled at compile time? That's been assumed right along (unless you think it's okay for Postgres to stop working on every non-SELinux platform). Good point. The problem here is mostly about whether we have enough confidence in the code to put our project name on it. This patch has been bandied about for what, two years? There is a known fork of our project that runs with it. It has a live googlecode site: http://code.google.com/p/sepgsql/ Which has lots of documentation. I also appears to be active within the SE community: http://www.nsa.gov/applications/search/index.cfm?q=se-postgresql It is also part of the Secure OS project out of Japan (as far as I can tell). I know we are a little uncomfortable here but KaiGai-San (forgive me if I type that wrong) has proven to be a contributor in his own right, jumping over every hurdle we have presented him. He is obviously sticking around for a while. If we accept this code, we lose a fork of our project (good) and we pull those people into our project (better) and hopefully they will help us mature the project over time (best). Sincerely, Joshua D. Drake regards, tom lane -- PostgreSQL - XMPP: jdr...@jabber.postgresql.org Consulting, Development, Support, Training 503-667-4564 - http://www.commandprompt.com/ The PostgreSQL Company, serving since 1997 -- 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] Updates of SE-PostgreSQL 8.4devel patches (r1704)
Joshua D. Drake wrote: http://www.nsa.gov/applications/search/index.cfm?q=se-postgresql It is also part of the Secure OS project out of Japan (as far as I can tell). I know we are a little uncomfortable here but KaiGai-San (forgive me if I type that wrong) has proven to be a contributor in his own right, jumping over every hurdle we have presented him. He is obviously sticking around for a while. KaiGai-San also submitted a patch for an unrelated bug today. :-) -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- 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] parallel restore fixes
Tom Lane wrote: It makes me a bit nervous because there are some other programs that are linking dumputils.c (psql and some in src/bin/scripts/) and even calling fmtId. Actually, why bother with init_dump_utils at all? fmtId could be made to initialize the ID variable for itself on first call, with only one extra if-test, which is hardly gonna matter. Well, the Windows reference I have suggests TlsAlloc() needs to be called early in the initialisation process ... I guess I could force it with a dummy call to fmtId() in restore_toc_entries_parallel() before it starts spawning children, so we'd be sure there wasn't a race condition, and nothing else is going to have threads so it won't matter. We'd need a long comment to that effect, though ;-) I'd lose the added retval variable too; that's not contributing anything. It is, in fact. Until I put that in I was getting constant crashes. I suspect it's something to do with stuff Windows does under the hood on function return. cheers andrew -- 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] Updates of SE-PostgreSQL 8.4devel patches (r1704)
Hannu Krosing wrote: Can't it be kept separately maintained release for a version or two, so that we will have both PostgreSQL and SE-PostgreSQL and thus have an easy way to compare both correctness and performance ? Anyone remember how did Linux implement/introduce SE Linux ? SELinux has been distributed as a part of mainlined Linux 2.6.x series for the recent five years. It can be enabled/disabled on both of compile time and system bootup time, by user's preference. SELinux is implemented as a security module on the LSM framework which provides a set of neutral hooks for any other stuffs. SE-PostgreSQL also had a similar stuff named as PGACE, but I agreed an opinion that we (pgsql-hackers) don't want in-code framework two months ago, so the PGACE has gone now. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1704)
On Mon, 2009-03-09 at 20:31 -0400, Bruce Momjian wrote: Joshua D. Drake wrote: http://www.nsa.gov/applications/search/index.cfm?q=se-postgresql It is also part of the Secure OS project out of Japan (as far as I can tell). I know we are a little uncomfortable here but KaiGai-San (forgive me if I type that wrong) has proven to be a contributor in his own right, jumping over every hurdle we have presented him. He is obviously sticking around for a while. KaiGai-San also submitted a patch for an unrelated bug today. :-) I also found some completely external references to it: http://lwn.net/Articles/242087/ http://itknowledgeexchange.techtarget.com/enterprise-linux/se-postgres-tightens-sql-security/ Sincerely, Joshua D. Drake -- PostgreSQL - XMPP: jdr...@jabber.postgresql.org Consulting, Development, Support, Training 503-667-4564 - http://www.commandprompt.com/ The PostgreSQL Company, serving since 1997 -- 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] Updates of SE-PostgreSQL 8.4devel patches (r1704)
Joshua D. Drake j...@commandprompt.com writes: I know we are a little uncomfortable here but KaiGai-San (forgive me if I type that wrong) has proven to be a contributor in his own right, Not to put too fine a point on it, but: no, he hasn't. Show me one significant patch he's contributed before/beside this one. The only thing I see in the CVS logs is that he helped Stephen Frost with column privileges; I don't recall who did how much, but in any case that patch still needed nontrivial fixes when it got to me. Frankly, what we have here is a large patch, with insanely difficult correctness requirements, written by a Postgres newbie. If it doesn't scare you, you haven't been paying attention. We have a long track record of problems with patches written by people who thought they were ready to do major backend hacking without having bitten off some smaller chunks first. Perhaps it would help you calibrate the problem if I stated that I wouldn't trust a patch for this purpose written by myself, let alone somebody who hasn't been hacking the backend for ten years. (Where this purpose means the type of control KaiGai-san seems to hope to enforce, as opposed to just plugging some additional constraints into the existing ACL-check routines.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1704)
Joshua D. Drake wrote: On Mon, 2009-03-09 at 19:45 -0400, Tom Lane wrote: Hannu Krosing ha...@2ndquadrant.com writes: Can't it be kept separately maintained release for a version or two, so that we will have both PostgreSQL and SE-PostgreSQL and thus have an easy way to compare both correctness and performance ? Actually, KaiGai-san has been distributing a patched SEPostgres on Fedora for awhile already (and it's been rather a pain in the neck I fear to keep it in sync with the regular distribution). One thing I would love to know is how many people are actually using that, but AFAIK there's no good way to find out. To point out the obvious, we are in a quandary here. Nobody argues the feature would be interesting and although I don't have use for it I could see where some people would. I also see where it would open doors for us. Is there any possibility of having it be enabled at compile time? The default would be know but those distributions that would like to make use of it could? It was the design a half year ago, but Bruce suggested me a certain feature should not be enabled/disabled by compile time options, except for library/platform dependency. In addition, he also suggested a feature should be turned on/off by configuration option, because of it enables to distribute a single binary for more wider users. SE-PostgreSQL need the libselinux to communicate the in-kernel SELinux. So, --enable-selinux is necessary on compile time, it is fair enough. If we omit it, all the sepgsql() invocations are replaced by empty macros. If we compile it with --enable-selinux, it has two working modes controled by a guc option: sepostgresql (bool). If it is disabled, all the sepgsql() invocations returns at the head of themself without doing anything. I believe this behavior follows the previous suggestion. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Pg_lesslog 1.2 released
Pg_lesslog, a PgFoundry project to reduce a size of archive log, has released new pg_lesslog 1.2. This fixes a bug of incorrect handling of XLOG_BTREE_SPLIT_R WAL record. Project home is here: http://pglesslog.projects.postgresql.org/ Download page is here: http://pgfoundry.org/frs/?group_id=1000310 -- -- Koichi Suzuki -- 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] parallel restore fixes
Andrew Dunstan and...@dunslane.net writes: Tom Lane wrote: Actually, why bother with init_dump_utils at all? Well, the Windows reference I have suggests TlsAlloc() needs to be called early in the initialisation process ... How early is early? The proposed call sites for init_dump_utils seem already long past the point where any libc-level infrastructure would think it is initialization time. I'd lose the added retval variable too; that's not contributing anything. It is, in fact. Until I put that in I was getting constant crashes. I suspect it's something to do with stuff Windows does under the hood on function return. Pardon me while I retrieve my eyebrows from the ceiling. I think you've got something going on there you don't understand, and you need to understand it not just put in a cargo-cult fix. (Especially one that's not documented and hence likely to be removed by the next person who touches the code.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Sampling Profler for Postgres
Dickson S. Guedes lis...@guedesoft.net wrote: Compiled and Works fine here on Ubuntu 8.04 2.6.25.15-bd-mod #1 SMP PREEMPT Thu Nov 27 10:05:44 BRST 2008 i686 GNU/Linux Thanks for testing. Network (or communication between pgbench and postgres) seems to be a bottleneck on your machine. Two questions here: 1) How will be this behavior in a syncrep environment? I don't have one here to test this, yet. I think it has relation with hot-standby, but not syncrep. Profiling is enabled when stats collector process is running. We already run the collector during warm-standby, so profiling would be also available on log-shipping slaves. 2) I couldn't find a clear way to disable it. There is one in this patch or are you planning this to future? Ah, I forgot sampling should be disabled when track_activities is off. I'll fix it in the next patch. Also, I'd better measure overheads by the patch. Regards, --- ITAGAKI Takahiro NTT Open Source Software Center -- 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] Updates of SE-PostgreSQL 8.4devel patches (r1704)
Perhaps it would help you calibrate the problem if I stated that I wouldn't trust a patch for this purpose written by myself, let alone somebody who hasn't been hacking the backend for ten years. (Where this purpose means the type of control KaiGai-san seems to hope to enforce, as opposed to just plugging some additional constraints into the existing ACL-check routines.) It seems to me this comment is a bit emotional... :( If we need ten more years of experimence before proposing a new security feature, all the new developers (outcome from other community) need to wait for the v8.14 (not 8.1.4) development cycle opened at 2019(?). I would like folks to review what the patch tries to do, not who submitted the patch. (And, I also would like experts to help/suggest this development.) Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] parallel restore fixes
Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: Tom Lane wrote: Actually, why bother with init_dump_utils at all? Well, the Windows reference I have suggests TlsAlloc() needs to be called early in the initialisation process ... How early is early? The proposed call sites for init_dump_utils seem already long past the point where any libc-level infrastructure would think it is initialization time. Well, I think at least it needs to be done where other threads won't be calling it at the same time. cheers andrew -- 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] Sampling Profler for Postgres
Em Ter, 2009-03-10 às 10:23 +0900, ITAGAKI Takahiro escreveu: Thanks for testing. Network (or communication between pgbench and postgres) seems to be a bottleneck on your machine. Yes, it is a very poor machine for quicktest. I'll test other environments tomorrow. Two questions here: 1) How will be this behavior in a syncrep environment? I don't have one here to test this, yet. I think it has relation with hot-standby, but not syncrep. Profiling is enabled when stats collector process is running. We already run the collector during warm-standby, so profiling would be also available on log-shipping slaves. OK. Thanks. 2) I couldn't find a clear way to disable it. There is one in this patch or are you planning this to future? Ah, I forgot sampling should be disabled when track_activities is off. I'll fix it in the next patch. Also, I'd better measure overheads by the patch. Will be very nice if I could on/off it. When done, please send us. I'd like to test it in some stress scenarios, enabling and disabling it on some environment and comparing with my old benchmarks. Regards, -- Dickson S. Guedes mail/xmpp: gue...@guedesoft.net - skype: guediz http://guedesoft.net - http://planeta.postgresql.org.br -- 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] Sampling Profler for Postgres
ITAGAKI Takahiro itagaki.takah...@oss.ntt.co.jp writes: For resource-based profilers, we have DTrace probes[1] and continue to extend them[2], but unfortunately DTrace only works on Solaris and limited platforms. FWIW, the systemtap guys are really, really close to having a working DTrace equivalent for Linux: http://gnu.wildebeest.org/diary/2009/02/24/systemtap-09-markers-everywhere/ It's not *quite* there for our purposes https://bugzilla.redhat.com/show_bug.cgi?id=488941 but I'll be surprised if I'm not dtracing on my Fedora 10 machine before the week is out. I'm not at all convinced that we should be putting effort into a homegrown, partial substitute for DTrace. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] parallel restore fixes
Andrew Dunstan and...@dunslane.net writes: Tom Lane wrote: How early is early? The proposed call sites for init_dump_utils seem already long past the point where any libc-level infrastructure would think it is initialization time. Well, I think at least it needs to be done where other threads won't be calling it at the same time. Oh, I see, ye olde race condition. Still, it seems like a bad idea to expect that we will catch every program that might call fmtId; as Alvaro notes, that's all over our client-side code. How about this: by default, fmtId uses the same logic as now (one static PQExpBuffer). If told to by a call of init_parallel_dump_utils(), which need only be called by pg_restore during its startup, then it switches to using per-thread storage. init_parallel_dump_utils can be the place that calls TlsAlloc. This is almost the same as what you suggested a couple messages back, but perhaps a bit clearer as to what's going on; and it definitely cuts the number of places we need to touch. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1704)
On Mon, Mar 9, 2009 at 1:52 AM, KaiGai Kohei kai...@ak.jp.nec.com wrote: As I promised last week, SE-PostgreSQL patches are revised here: [1/5] http://sepgsql.googlecode.com/files/sepgsql-core-8.4devel-r1704.patch [2/5] http://sepgsql.googlecode.com/files/sepgsql-utils-8.4devel-r1704.patch [3/5] http://sepgsql.googlecode.com/files/sepgsql-policy-8.4devel-r1704.patch [4/5] http://sepgsql.googlecode.com/files/sepgsql-docs-8.4devel-r1704.patch [5/5] http://sepgsql.googlecode.com/files/sepgsql-tests-8.4devel-r1704.patch has anyone noted that the links are malformed? in my browser they include the [x/5 part of the next line anyway, i have given up trying to test the functional parts of the patch (my knowledge of selinux is almost zero and is a lot of info just to understand the basics... i'm still on that but don't think will get anything for 8.4... if someone can provide some simple info on that will be great) but now i'm trying the performance impacts of it... what seems interesting is that on some queries are some little gain with the patch applied... that seems interesting 'cause i thought it will be the opposite... i want to try to isolate where is the difference... can someone explain me how can i trace that? (sorry for my ignorance but if i don't ask that ignorance will stay) -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157 -- 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] Updates of SE-PostgreSQL 8.4devel patches (r1704)
Jaime Casanova wrote:
On Mon, Mar 9, 2009 at 1:52 AM, KaiGai Kohei kai...@ak.jp.nec.com wrote:
As I promised last week, SE-PostgreSQL patches are revised here:
[1/5] http://sepgsql.googlecode.com/files/sepgsql-core-8.4devel-r1704.patch
[2/5] http://sepgsql.googlecode.com/files/sepgsql-utils-8.4devel-r1704.patch
[3/5] http://sepgsql.googlecode.com/files/sepgsql-policy-8.4devel-r1704.patch
[4/5] http://sepgsql.googlecode.com/files/sepgsql-docs-8.4devel-r1704.patch
[5/5] http://sepgsql.googlecode.com/files/sepgsql-tests-8.4devel-r1704.patch
has anyone noted that the links are malformed? in my browser they
include the [x/5 part of the next line
Above URLs might be a bit long.
I'll omit the [x/5] part on the next submission.
i want to try to isolate where is the difference... can someone
explain me how can i trace that? (sorry for my ignorance but if i
don't ask that ignorance will stay)
The sepgsql_enable_auditallow system boolean will help you to
understand what permissions are checked on the given query.
-
% make -C src/backend/security/sepgsql/policy
# su
# semodule -i src/backend/security/sepgsql/policy/sepostgresql-devel.pp
(installation of development purpose policy)
# setsebool sepgsql_enable_auditallow 1
% psql postgres
NOTICE: SELinux: granted { access }
scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c63
tcontext=unconfined_u:object_r:sepgsql_db_t:s0 tclass=db_database name=postgres
psql (8.4devel)
Type help for help.
postgres=# SELECT * FROM t1;
NOTICE: SELinux: granted { select }
scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c63
tcontext=unconfined_u:object_r:sepgsql_table_t:s0 tclass=db_table name=t1
NOTICE: SELinux: granted { select }
scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c63
tcontext=unconfined_u:object_r:sepgsql_table_t:s0 tclass=db_column name=t1.a
NOTICE: SELinux: granted { select }
scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c63
tcontext=unconfined_u:object_r:sepgsql_table_t:s0 tclass=db_column name=t1.b
NOTICE: SELinux: granted { select }
scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c63
tcontext=unconfined_u:object_r:sepgsql_table_t:s0 tclass=db_column name=t1.c
a | b | c
---+---+---
(0 rows)
postgres=# INSERT INTO t1 (a,c) VALUES (1,2);
NOTICE: SELinux: granted { insert }
scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c63
tcontext=unconfined_u:object_r:sepgsql_table_t:s0 tclass=db_table name=t1
NOTICE: SELinux: granted { insert }
scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c63
tcontext=unconfined_u:object_r:sepgsql_table_t:s0 tclass=db_column name=t1.a
NOTICE: SELinux: granted { insert }
scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c63
tcontext=unconfined_u:object_r:sepgsql_table_t:s0 tclass=db_column name=t1.c
INSERT 0 1
postgres=#
-
The meanings of each fields:
- The scontext is the client's privileges
- The tcontext is the security context of tables, columns and so on.
- The tclass shows the kind of target object.
- The name is the name of object.
I recommend you to turn off it in normal case due to noisy and disk
consumption with logs.
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei kai...@ak.jp.nec.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1704)
Tom, Frankly, what we have here is a large patch, with insanely difficult correctness requirements, written by a Postgres newbie. If it doesn't scare you, you haven't been paying attention. We have a long track record of problems with patches written by people who thought they were ready to do major backend hacking without having bitten off some smaller chunks first. If that was the case, why didn't you say it 4 months ago? It seems rather unfair to Kaigai and everyone else who worked on it to be getting cold feet about the entire concept after several months of debugging. --Josh Berkus -- 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] Updates of SE-PostgreSQL 8.4devel patches (r1704)
Josh Berkus j...@agliodbs.com writes: Frankly, what we have here is a large patch, with insanely difficult correctness requirements, written by a Postgres newbie. If it doesn't scare you, you haven't been paying attention. We have a long track record of problems with patches written by people who thought they were ready to do major backend hacking without having bitten off some smaller chunks first. If that was the case, why didn't you say it 4 months ago? It seems rather unfair to Kaigai and everyone else who worked on it to be getting cold feet about the entire concept after several months of debugging. Josh, I've had cold feet about this patch from day one, and have not been very shy about expressing it, for instance here http://archives.postgresql.org/pgsql-hackers/2008-05/msg00122.php here http://archives.postgresql.org//pgsql-hackers/2008-09/msg01662.php and here http://archives.postgresql.org//pgsql-hackers/2009-01/msg01928.php (those are just samples from long threads in each case). Despite all that arm-waving, no one besides KaiGai-san has really stepped up to work on it. That leaves me not only with worries about the patch itself, but with an extremely low estimate of the community's interest in it. And it is too big, complicated, and risky to go in if there's not strong community support for it. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1704)
Tom Lane wrote: Despite all that arm-waving, no one besides KaiGai-san has really stepped up to work on it. That leaves me not only with worries about the patch itself, but with an extremely low estimate of the community's interest in it. And it is too big, complicated, and risky to go in if there's not strong community support for it. The only reason why I separated a few major facilities (writable system columns, row-level controls, largeobject permission and so on) and reduces the scale of patches as someone required is to help SE-PostgreSQL getting merged into the v8.4. In addition, as I said before, I can provide my efforts to maintenance SE-PostgreSQL feature to the future version, once it getting merged, no need to say. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1704)
Heikki Linnakangas wrote:
+ void
+ sepgsqlCheckProcedureInstall(Relation rel, HeapTuple newtup,
HeapTuple oldtup)
+ {
[snip]
+ + case AccessMethodRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_am, aminsert, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, ambeginscan, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amgettuple, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amgetbitmap, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amrescan, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amendscan, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, ammarkpos, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amrestrpos, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, ambuild, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, ambulkdelete, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amvacuumcleanup, newtup,
oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amcostestimate, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amoptions, newtup, oldtup);
+ break;
ISTM that you should just forbid any changes to pg_am in the default
policy. That's very low level stuff. If you can modify that, you can
wreck a lot of havoc, quite possibly turning it into a vulnerability
even if you can't directly install a malicious function there.
Heikki,
My opinion is still we should check db_procedure:{install} privilege
for functions expected to be implemented by C-language.
In the basis of security, it requires security facilities should
improve confidentiality, integrity and availability in the assumption
and environment required by the facility.
For example, existing database ACL improves confidentiality and
integrity with applying DAC policy, and improves availability to
prevent to load C-library by users except for superuser.
(Here, the assumption is that database superuser is trusted.)
If we write a oid of SQL-function onto pg_am.aminsert, it will
not work correctly independent from existence of maliciou code,
but it also enables to crash the backend immediately.
It can be a damage to the availability of the backend, so I still
think we need to check this permission for functions expected to
be implemented by C-language.
NOTE: when we create a new C-function or replace its definition,
sepgsqlCheckDatabaseInstallModule() checks whether the given
loadable library file has appropriate security context, or not.
In the default security policy, only files labeled as lib_t
are allowed to load.
+ case AccessMethodProcedureRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_amproc, amproc, newtup, oldtup);
+ break;
+ + case CastRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_cast, castfunc, newtup, oldtup);
+ break;
We check execute permission on the cast function at runtime.
We have a corner case.
The ri_HashCompareOp() in ri_triggers.c can invokes castfunc without
runtime checks, if I can understand the implementation correctly.
Indeed, most cases invokes pg_proc_aclcheck() and SE-PostgreSQL also
checks db_procedure:{execute} permission in runtime.
This design requires either of install or execute should be checked
at least, so double checks are not matter.
[snip]
+ case ForeignDataWrapperRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_foreign_data_wrapper,
fdwvalidator, newtup, oldtup);
+ break;
Hmm, calls to fdwvalidator are not at all performance critical, so maybe
we should just check execute permission when it's called.
If pg_proc_aclcheck() is added on the invocation of fdwvalidator,
I'll remove install checks on it from here.
[snip]
+ case OperatorRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_operator, oprcode, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_operator, oprrest, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_operator, oprjoin, newtup, oldtup);
+ break;
oprcode is checked for execute permission when the operator is used. For
oprrest and oprjoin, we could add checks into the planner where they're
called. (pg_operator.oprcom and pg_operator.oprnegate are missing?)
For example, ExecInitAgg() set up opcode function as follows:
fmgr_info(get_opcode(eq_opr), (peraggstate-equalfn));
and it can be invoked later without checks.
I think it is a case to be checked. Indeed, pg_operator.oprcom and
pg_operator.oprnegate were missed. Thanks for your pointed out.
+ case TSParserRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_ts_parser, prsstart, newtup,
oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_ts_parser, prstoken, newtup,
oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_ts_parser, prsend, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_ts_parser, prsheadline, newtup,
oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_ts_parser, prslextype, newtup,
oldtup);
+ break;
+ + case TSTemplateRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_ts_template, tmplinit,
