Re: [HACKERS] Admission Control
On 10/07/10 03:54, Kevin Grittner wrote: Mark Kirkwoodmark.kirkw...@catalyst.net.nz wrote: Purely out of interest, since the old repo is still there, I had a quick look at measuring the overhead, using 8.4's pgbench to run two custom scripts: one consisting of a single 'SELECT 1', the other having 100 'SELECT 1' - the latter being probably the worst case scenario. Running 1,2,4,8 clients and 1000-1 transactions gives an overhead in the 5-8% range [1] (i.e transactions/s decrease by this amount with the scheduler turned on [2]). While a lot better than 30% (!) it is certainly higher than we'd like. Hmmm... In my first benchmarks of the serializable patch I was likewise stressing a RAM-only run to see how much overhead was added to a very small database transaction, and wound up with about 8%. By profiling where the time was going with and without the patch, I narrowed it down to lock contention. Reworking my LW locking strategy brought it down to 1.8%. I'd bet there's room for similar improvement in the active transaction limit you describe. In fact, if you could bring the code inside blocks of code already covered by locks, I would think you could get it down to where it would be hard to find in the noise. Yeah, excellent suggestion - I suspect there is room for considerable optimization along the lines you suggest... at the time the focus was heavily biased toward a purely DW workload where the overhead vanished against large plan and execute times, but this could be revisited. Having said that I suspect a re-architect is needed for a more wideranging solution suitable for Postgres (as opposed to Bizgres or Greenplum) Cheers Mark -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE SET STATISTICS requires AccessExclusiveLock
On Fri, 2010-07-09 at 15:03 -0400, Robert Haas wrote: On Fri, Jul 9, 2010 at 1:18 PM, Simon Riggs si...@2ndquadrant.com wrote: On Fri, 2010-07-09 at 13:04 -0400, Robert Haas wrote: Tom asked what happens when two transactions attempt to do concurrent actions on the same table. Your response was that we should handle it like CREATE INDEX, and handle the update of the pg_class row non-transactionally. But of course, if you use a self-conflicting lock at the relation level, then the relation locks conflict and you never have to worry about how to update the pg_class entry in the face of concurrent updates. From memory, Tom was also worried about the prospect of people updating pg_class directly using SQL. That seems a rare, yet valid concern. Yes, and it's another another reason why we shouldn't use non-transactional updates. http://archives.postgresql.org/pgsql-hackers/2008-11/msg00744.php I've already agreed with your point that we should use SHARE UPDATE EXCLUSIVE. The point you seem to be missing is that once we make that decision, we can throw all the heap_inplace_update() stuff out the window, and the whole problem becomes much simpler. That is a point I missed. Considering this further, it seems we have two conflicting requirements 1. ALTER TABLE ... ADD FOREIGN KEY needs a SHARE mode lock if we want to run that concurrently with itself and CREATE INDEX operations during a pg_restore. This was my original goal. 2. In most other cases, SHARE UPDATE EXCLUSIVE is the most useful lock, especially during heavy operational use. Since adding an FK requires adding triggers also that puts both of the above in direct conflict. ISTM that we should follow (2) and let (1) be added to the TODO for later work, as an option. I'll followu up on (2). -- Simon Riggs www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Training and Services -- 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] [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().
On Fri, 2010-07-09 at 17:21 -0400, Tom Lane wrote: Simon Riggs si...@2ndquadrant.com writes: On Fri, 2010-07-09 at 14:01 -0400, Tom Lane wrote: Consider PREPARE followed only later by EXECUTE. Your proposal would make the PREPARE fail outright, when it currently does not. Just to avoid wasted investigation: are you saying that is important behaviour that is essential we retain in PostgreSQL, or will you hear evidence that supporting that leads to a performance decrease elsewhere? Well, I think that that problem makes moving the checks into the planner a nonstarter. But as somebody pointed out upthread, you could still get what you want by keeping a flag saying permission checks have been done so the executor could skip the checks on executions after the first. Seems like best idea. I'd still want to see some evidence showing that it's worth troubling over though. Premature optimization being the root of all evil, and all that. (In this case, the hazard we expose ourselves to seems to be security holes due to missed resets of the flag.) If we did this it would be to add one line to the code if (!perms_ok) That doesn't seem to fall into the category of evil optimization to me. I've seen you recode other parts of the executor stating reasons like shave another few cycles off the main path and that seems the case here. We shouldn't need to debate the consequences of Amhdahls law each time. Attached is a script to allow pgbench to be used to measure difference between superuser and a typical privilege model for the pgbench tables. -- Simon Riggs www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Training and Services drop user if exists ref; create user ref; drop user if exists cust; create user cust; drop user if exists report; create user report; drop user if exists xact; create user xact; /* Reference data */ GRANT INSERT, UPDATE, DELETE ON pgbench_tellers TO ref; GRANT INSERT, UPDATE, DELETE ON pgbench_branches TO ref; /* Customer maintenance */ GRANT INSERT, DELETE ON pgbench_accounts TO cust; GRANT UPDATE (bid, filler) ON pgbench_accounts TO cust; /* Reporting */ GRANT SELECT ON pgbench_accounts TO report; GRANT SELECT ON pgbench_branches TO report; GRANT SELECT ON pgbench_tellers TO report; GRANT SELECT ON pgbench_history TO report; /* Transactions */ GRANT UPDATE (abalance) ON pgbench_accounts TO xact; GRANT UPDATE (tbalance) ON pgbench_tellers TO xact; GRANT UPDATE (bbalance) ON pgbench_branches TO xact; GRANT INSERT ON pgbench_history TO xact; GRANT SELECT ON pgbench_accounts TO xact; GRANT SELECT ON pgbench_branches TO xact; GRANT SELECT ON pgbench_tellers TO xact; GRANT SELECT ON pgbench_history TO xact; -- 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] gSoC - ADD MERGE COMMAND - code patch submission
Hi, Thanks for all these feedback. I found that people have problems on running my codes, which probably comes from my nonstandard submission format. I can compile, install and initialize postgres in my own machine. The system accepts MERGE command and will throw an error when it runs into the executor, which cannot recognize the MERGE command type so far. I will make a standard patch as soon as possible. Sorry for the troubles. Yours Boxuan 2010/7/11 Tom Lane t...@sss.pgh.pa.us Peter Eisentraut pete...@gmx.net writes: On lör, 2010-07-10 at 12:45 -0400, Tom Lane wrote: I believe the project standard is to make things readable in an 80-column window --- anyone have an objection to stating that explicitly? Is that what pgindent reformats it to? pgindent tries to leave a character or two to spare, IIRC, so its target is probably 78 or thereabouts. regards, tom lane
Re: [HACKERS] [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().
Simon Riggs si...@2ndquadrant.com writes: On Fri, 2010-07-09 at 17:21 -0400, Tom Lane wrote: I'd still want to see some evidence showing that it's worth troubling over though. Premature optimization being the root of all evil, and all that. (In this case, the hazard we expose ourselves to seems to be security holes due to missed resets of the flag.) If we did this it would be to add one line to the code if (!perms_ok) That doesn't seem to fall into the category of evil optimization to me. The problem I foresee is not in the testing of the flag, it's in the setting/resetting of it. It's a reliability penalty not a performance penalty --- and any mistakes would count as security issues. Now it may be that you can offer a convincing argument that no such mistake/oversight is likely. But you haven't even tried to make that case. Even if you can show that the risk is small, it's not going to be zero, so we have to trade it off against a demonstrated performance improvement. 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] [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().
On Jul 11, 2010, at 10:44 AM, Tom Lane t...@sss.pgh.pa.us wrote: Simon Riggs si...@2ndquadrant.com writes: On Fri, 2010-07-09 at 17:21 -0400, Tom Lane wrote: I'd still want to see some evidence showing that it's worth troubling over though. Premature optimization being the root of all evil, and all that. (In this case, the hazard we expose ourselves to seems to be security holes due to missed resets of the flag.) If we did this it would be to add one line to the code if (!perms_ok) That doesn't seem to fall into the category of evil optimization to me. The problem I foresee is not in the testing of the flag, it's in the setting/resetting of it. It's a reliability penalty not a performance penalty --- and any mistakes would count as security issues. Now it may be that you can offer a convincing argument that no such mistake/oversight is likely. But you haven't even tried to make that case. Even if you can show that the risk is small, it's not going to be zero, so we have to trade it off against a demonstrated performance improvement. There's no point in going back and forth here until we have a patch and the results of some performance testing using said patch. If Simon writes one and submits it with some results, we'll consider it on the merits. I think that's all Simon is asking for, and I don't think anyone is seriously arguing anything to the contrary. Like Tom, I'm skeptical that there is much performance to be found here, but if I'm wrong, I'm happy to have someone demonstrate it. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] crash-recovery replay of CREATE TABLESPACE is broken in HEAD
I managed to crash the executor in the tablespace.sql test while working on a 9.1 patch, and discovered that the postmaster fails to recover from that. The end of postmaster.log looks like LOG: all server processes terminated; reinitializing LOG: database system was interrupted; last known up at 2010-07-11 19:30:07 EDT LOG: database system was not properly shut down; automatic recovery in progress LOG: consistent recovery state reached at 0/EE26F30 LOG: redo starts at 0/EE26F30 FATAL: directory /home/postgres/pgsql/src/test/regress/testtablespace/PG_9.1_201004261 already in use as a tablespace CONTEXT: xlog redo create ts: 127158 /home/postgres/pgsql/src/test/regress/testtablespace LOG: startup process (PID 13914) exited with exit code 1 LOG: aborting startup due to startup process failure It looks to me like those well-intentioned recent changes in this area broke the crash-recovery case. Not good. 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] patch: preload dictionary new version
2010/7/8 Tom Lane t...@sss.pgh.pa.us: For example, the dictionary-load code could automatically execute the precompile step if it observed that the precompiled copy of the dictionary was missing or had an older file timestamp than the source. There might be a problem in automatic precompiler -- Where should we save the result? OS users of postgres servers don't have write-permission to $PGSHARE in normal cases. Instead, we can store the precompiled result to $PGDATA/pg_dict_cache or so. I like the idea of a precompiler step mainly because it still gives you most of the benefits of the patch on platforms without mmap. I also like the precompiler solution. I think the most important benefit in the approach is that we don't need to declare dictionaries to be preloaded in configuration files; We can always use mmap() for all dictionary files. -- Takahiro Itagaki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch (for 9.1) string functions
2010/7/9 Pavel Stehule pavel.steh...@gmail.com:
I am sending a actualised patch
* removed concat_json
* renamed function rvsr to reverse
* functions format, sprintf and concat* are stable now (as to_char for
example)
I'd like to move all proposed functions into the core, and not to add
contrib/stringfunc.
I think those functions are very useful and worth adding in core.
* concat(), concat_ws(), reverse(), left() and right() are ready to commit.
* format() is almost ready, except consensus of NULL representation.
* sprintf() is also useful, but we cannot use swprintf() in it because
there are many problems in converting to wide chars. We should
develop mbchar-aware version of %s formatter.
* IMHO, concat_sql() has very limited use cases. Boolean and numeric
values are not quoted, but still need product-specific conversions because
some DBs prefer 1/0 instead of true/false.
Also, dblink_build_sql_insert() provides similar functionality. Will
we have both?
it worked on my station :( - Fedora 64bit
Still failed :-( I used UTF8 database with *locale=C* on 64bit Linux.
char2wchar() doesn't seem to work on C locale. We should avoid
using the function and converting mb chars to wide chars.
select sprintf('%10s %10d', 'hello', 10);
! server closed the connection unexpectedly
TRAP: FailedAssertion(!(!lc_ctype_is_c()), File: mbutils.c, Line: 715)
#0 0x0038c0c332f5 in raise () from /lib64/libc.so.6
#1 0x0038c0c34b20 in abort () from /lib64/libc.so.6
#2 0x006e951d in ExceptionalCondition (conditionName=value
optimized out, errorType=value optimized out, fileName=value
optimized out,
lineNumber=value optimized out) at assert.c:57
#3 0x006fa8bf in char2wchar (to=0x1daf188 L, tolen=16,
from=0x1da95b0 %*s, fromlen=3) at mbutils.c:715
#4 0x7f8e8c436d37 in stringfunc_sprintf (fcinfo=0x7fff9bdcd4b0)
at stringfunc.c:463
--
Itagaki Takahiro
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch (for 9.1) string functions
On Sun, Jul 11, 2010 at 10:30 PM, Itagaki Takahiro itagaki.takah...@gmail.com wrote: 2010/7/9 Pavel Stehule pavel.steh...@gmail.com: I am sending a actualised patch * removed concat_json * renamed function rvsr to reverse * functions format, sprintf and concat* are stable now (as to_char for example) I'd like to move all proposed functions into the core, and not to add contrib/stringfunc. I think those functions are very useful and worth adding in core. * concat(), concat_ws(), reverse(), left() and right() are ready to commit. * format() is almost ready, except consensus of NULL representation. * sprintf() is also useful, but we cannot use swprintf() in it because there are many problems in converting to wide chars. We should develop mbchar-aware version of %s formatter. * IMHO, concat_sql() has very limited use cases. Boolean and numeric values are not quoted, but still need product-specific conversions because some DBs prefer 1/0 instead of true/false. Also, dblink_build_sql_insert() provides similar functionality. Will we have both? I'm all in favor of putting such things in core as are supported by multiple competing products, but is that really true for all of these? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch (for 9.1) string functions
2010/7/12 Robert Haas robertmh...@gmail.com: I'm all in favor of putting such things in core as are supported by multiple competing products, but is that really true for all of these? - concat() : MySQL, Oracle, DB2 - concat_ws() : MySQL, - left(), right() : MySQL, SQL Server, DB2 - reverse() : MySQL, SQL Server, Oracle (as utl_raw.reverse) concat_sql(), format(), and sprintf() will be our unique features. -- Itagaki Takahiro -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch: preload dictionary new version
Itagaki Takahiro itagaki.takah...@gmail.com writes: 2010/7/8 Tom Lane t...@sss.pgh.pa.us: For example, the dictionary-load code could automatically execute the precompile step if it observed that the precompiled copy of the dictionary was missing or had an older file timestamp than the source. There might be a problem in automatic precompiler -- Where should we save the result? OS users of postgres servers don't have write-permission to $PGSHARE in normal cases. Instead, we can store the precompiled result to $PGDATA/pg_dict_cache or so. Yeah. Actually we'd *have* to do something like that because $PGSHARE should contain only architecture-independent files, while the precompiled files would presumably have dependencies on endianness etc. 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] Reworks of DML permission checks
(2010/07/10 5:53), Robert Haas wrote: 2010/6/14 KaiGai Koheikai...@ak.jp.nec.com: The attached patch tries to rework DML permission checks. It was mainly checked at the ExecCheckRTEPerms(), but same logic was implemented in COPY TO/FROM statement and RI_Initial_Check(). This patch tries to consolidate these permission checks into a common function to make access control decision on DML permissions. It enables to eliminate the code duplication, and improve consistency of access controls. This patch is listed on the CommitFest page, but I'm not sure if it represents the latest work on this topic. At a minimum, it needs to be rebased. I am not excited about moving ExecCheckRT[E]Perms to some other place in the code. It seems to me that will complicate back-patching with no corresponding advantage. I'd suggest we not do that.The COPY and RI code can call ExecCheckRTPerms() where it is. Maybe at some point we will have a grand master plan for how this should all be laid out, but right now I'd prefer localized changes. OK, I rebased and revised the patch not to move ExecCheckRTPerms() from executor/execMain.c. In the attached patch, DoCopy() and RI_Initial_Check() calls that function to consolidate dml access control logic. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com pgsql-v9.1-reworks-dml-checks.2.patch Description: application/octect-stream -- 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] multibyte charater set in levenshtein function
Hi, I'm reviewing Multibyte charater set in levenshtein function patch. https://commitfest.postgresql.org/action/patch_view?id=304 The main logic seems to be good, but I have some comments about the coding style and refactoring. * levenshtein_internal() and levenshtein_less_equal_internal() are very similar. Can you merge the code? We can always use less_equal_internal() if the overhead is ignorable. Did you compare them? * There are many if (!multibyte_encoding) in levenshtein_internal(). How about split the function into two funcs for single-byte chars and multi-byte chars? (ex. levenshtein_internal_mb() ) Or, we can always use multi-byte version if the overhead is small. * I prefer a struct rather than an array. 4 * m and 3 * m look like magic numbers for me. Could you name the entries with definition of a struct? /* * For multibyte encoding we'll also store array of lengths of * characters and array with character offsets in first string * in order to avoid great number of pg_mblen calls. */ prev = (int *) palloc(4 * m * sizeof(int)); * There are some compiler warnings. Avoid them if possible. fuzzystrmatch.c: In function ‘levenshtein_less_equal_internal’: fuzzystrmatch.c:428: warning: ‘char_lens’ may be used uninitialized in this function fuzzystrmatch.c:428: warning: ‘offsets’ may be used uninitialized in this function fuzzystrmatch.c:430: warning: ‘curr_right’ may be used uninitialized in this function fuzzystrmatch.c: In function ‘levenshtein_internal’: fuzzystrmatch.c:222: warning: ‘char_lens’ may be used uninitialized in this function * Coding style: Use if (m == 0) instead of if (!m) when the type of 'm' is an integer. * Need to fix the caution in docs. http://developer.postgresql.org/pgdocs/postgres/fuzzystrmatch.html | Caution: At present, fuzzystrmatch does not work well with | multi-byte encodings (such as UTF-8). but now levenshtein supports multi-byte encoding! We should mention which function supports mbchars not to confuse users. * (Not an issue for the patch, but...) Could you rewrite PG_GETARG_TEXT_P, VARDATA, and VARSIZE to PG_GETARG_TEXT_PP, VARDATA_ANY, and VARSIZE_ANY_EXHDR? Unpacking versions make the core a bit faster. -- Itagaki Takahiro -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.1] Add security hook on initialization of instance
(2010/07/09 23:52), Stephen Frost wrote: * Stephen Frost (sfr...@snowman.net) wrote: Guess my first thought was that you'd have a database-level label that would be used by SELinux to validate a connection. A second thought is labels for roles. KaiGai, can you provide your thoughts on this discussion/approach/problems? I realize it's come a bit far-afield from your original proposal. Something else which has come up but is related is the ability to support a pam_tally-like function in PG. Basically, the ability to lock users out if they've had too many failed login attempts. I wonder if we could add this hook (or maybe have more than one if necessary) in a way to support a contrib module for that. It seems to me a good idea. BTW, where do you intend to apply this pam_tally like functionality? If it tries to lock users out on the identification stage; like the pam_tally.so on operating systems, the hook should be placed on the top-half of ClientAuthentication(). On the other hand, when we tries to set up properties of a certain user's session, it needs to be placed on the authorization stage. In the PG code, InitializeSessionUserId() just performs the role to assign the authenticated user's identifier on the current session. It seems to me it is a candidate where we put a hook on the authorization stage. Of course, these are not exclusive. We can provide two hooks to provide a chance to get control on identification and authorization stages. Thanks, -- 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] get_whatever_oid, part 2
(2010/07/09 22:36), Robert Haas wrote: 2010/7/8 KaiGai Koheikai...@ak.jp.nec.com: At the part 1 patch, the object class was entirely matched with name of the system catalog. E.g, get_namespace_oid(), get_langeage_oid(). ^ | | +-- pg_namespace +-- pg_language But some of APIs in the part 2 have different object class name from their corresponding system catalog. How about the following renamings? - get_tsparser_oid() - get_ts_parser_oid() - get_tsdictionary_oid() - get_ts_dict_oid() - get_tstemplate_oid() - get_ts_template_oid() - get_tsconfiguration_oid() - get_ts_config_oid() - get_rule_oid() - get_rewrite_oid() - get_rule_oid_without_relid() - get_rewrite_oid_without_relid() I like that idea. Done, attached. I checked it, but here is nothing to comment any more. So, I marked it as ready for committer. Thanks, -- 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] log files and permissions
I checked log_file_mode GUC patch, and found a couple of Windows-specific and translation issues. * fchmod() is not available on some platforms, including Windows. fh = fopen(filename, mode); setvbuf(fh, NULL, LBF_MODE, 0); fchmod(fileno(fh), Log_file_mode); I think umask()-fopen() is better rather than fopen()-chmod(). See codes in DoCopyTo() at commands/copy.c. * How does the file mode work on Windows? If it doesn't work, we should explain it in docs. Description for .pgpass for Windows might be a help. | http://developer.postgresql.org/pgdocs/postgres/libpq-pgpass.html | On Microsoft Windows, ... no special permissions check is made. * This message format is hard to translate. ereport(am_rotating ? LOG : FATAL, (errcode_for_file_access(), (errmsg(could not create%slog file \%s\: %m, am_rotating ? new : , filename; It might look a duplication of codes, but I think this form is better because we can reuse the existing translation catalogs. if (am_rotating) ereport(FATAL, ... could not create log file ...); else ereport(LOG, ... could not open new log file ...); -- Itagaki Takahiro -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
