Re: [HACKERS] WIP patch for parameterized inner paths
On 17/01/12 18:06, Tom Lane wrote: Anyway, I'm hoping to keep hacking at this for a couple more days before the CF gets into full swing, and hopefully arrive at something committable for 9.2. I'd really like to get this fixed in this cycle, since it's been a problem for several releases now. Comments? Very, nice - keep hacking! Best wishes 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] Client Messages
On 18.01.2012 07:49, Fujii Masao wrote: On Fri, Jan 6, 2012 at 1:38 AM, Jim Mlodgenskijimm...@gmail.com wrote: I have a need to send banner messages to a psql client that I can set on the server and will be displayed on any psql client that connects to the database. This would be mostly used as an additional indicator to which database you are connecting, but could also be used by people to force their users to see an security message when connecting to the database. The attached patch will allow you to execute ALTER DATABASE postgres SET client_message=E'\nBEWARE: You are connecting to a production database. If you do anything to\n bring this server down, you will be destroyed by your supreme overlord.\n\n'; And then when you connect to psql, you will see: [e3@workstation bin]$ ./psql -U user1 postgres psql (9.2devel) BEWARE: You are connecting to a production database. If you do anything to bring this server down, you will be destroyed by your supreme overlord. Type help for help. postgres= Any feedback is welcome. Adding new GUC parameter only for the purpose of warning psql users seems overkill to me. Basically we try to reduce the number of GUC parameters to make a configuration easier to a user, so I don't think that it's good idea to add new GUC for such a small benefit. It seems quite useful to me... Instead, how about using .psqlrc file and writing a warning message in it by using \echo command? That's not the same thing at all. Each client would need to put the warning in that file, and you'd get it regardless of the database you connect to. Anyway, I found one problem in the patch. The patch defines client_message as PGC_USERSET parameter, which means that any psql can falsify a warning message, e.g., by setting the environment variable PGOPTIONS to -c client_message=hoge. This seems to be something to avoid from security point of view. I don't think that's a problem, it's just a free-form message to display. But it also doesn't seem very useful to have it PGC_USERSET: if it's only displayed at connect time, there's no point in changing it after connecting. The only security problem that I can think of is a malicious server (man-in-the-middle perhaps), that sends a banner that confuses Docs for PQparameterStatus() needs adjustment, now that client_message is also one of the settings automatically reported to the client. The placement of the banner in psql looks currently like this: $ psql postgres psql (9.2devel) Hello world! Type help for help. or postgres=# \c postgres Hello world! You are now connected to database postgres as user heikki. Are we happy with that? I think it would be better to print the banner just before the prompt: psql (9.2devel) Type help for help. Hello world! postgres=# \c postgres You are now connected to database postgres as user heikki. Hello world! postgres=# Should we prefix the banner with something that makes it clear that it's a message coming from the server? Something like: psql (9.2devel) Type help for help. Notice from server: Hello world! postgres=# \c postgres You are now connected to database postgres as user heikki. Notice from server: Hello world! postgres=# -- 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] Command Triggers
Tom Lane t...@sss.pgh.pa.us writes: Dimitri Fontaine dimi...@2ndquadrant.fr writes: I still have some cleaning to do before to prepare the next patch version, such as documentation updating and dealing with rewrites of CHECK and DEFAULT column constraints in CREATE TABLE. I had to add support for the T_A_Const parser node, and now I'm about to see about adding support for the T_A_Expr one, but I can't help to wonder how the rewriter could work without them. It doesn't, and it shouldn't have to. If those nodes get to the rewriter then somebody forgot to apply parse analysis. What's your test case? I'm trying to rewrite the command string from the parse tree, and the simple example that I use to raise an ERROR is the following: create table foo (id serial, foo integer default 1, primary key(id)); I don't know if the parsetree handed by ProcessUtility() has gone through parse analysis and I don't know if that's needed to execute the commands, so maybe I have some high level function to call before walking the parse tree in my new rewriting support? Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et 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] Patch: Allow SQL-language functions to reference parameters by parameter name
On Sat, Jan 14, 2012 at 8:10 AM, Matthew Draper matt...@trebex.net wrote: I just remembered to make time to advance this from WIP to proposed patch this week... and then worked out I'm rudely dropping it into the last commitfest at the last minute. :/ The patch applies clean against master but compiles with warnings. functions.c: In function ‘prepare_sql_fn_parse_info’: functions.c:212: warning: unused variable ‘argnum’ functions.c: In function ‘sql_fn_post_column_ref’: functions.c:341: warning: implicit declaration of function ‘ParseFuncOrColumn’ functions.c:345: warning: assignment makes pointer from integer without a cast Then, I ran make check but hit a bunch of crash. Looking closer, I found the FieldSelect returned from ParseFuncOrColumn is trimmed to 32bit pointer and subsequent operation on this is broken. I found unnecessary cltq is inserted after call. 0x0001001d8288 sql_fn_post_column_ref+748:mov$0x0,%eax 0x0001001d828d sql_fn_post_column_ref+753:callq 0x100132f75 ParseFuncOrColumn 0x0001001d8292 sql_fn_post_column_ref+758:cltq 0x0001001d8294 sql_fn_post_column_ref+760:mov%rax,-0x48(%rbp) My environment: 10.8.0 Darwin Kernel Version 10.8.0: Tue Jun 7 16:32:41 PDT 2011; root:xnu-1504.15.3~1/RELEASE_X86_64 x86_64 $ gcc -v Using built-in specs. Target: i686-apple-darwin10 Configured with: /var/tmp/gcc/gcc-5666.3~6/src/configure --disable-checking --enable-werror --prefix=/usr --mandir=/share/man --enable-languages=c,objc,c++,obj-c++ --program-transform-name=/^[cg][^.-]*$/s/$/-4.2/ --with-slibdir=/usr/lib --build=i686-apple-darwin10 --program-prefix=i686-apple-darwin10- --host=x86_64-apple-darwin10 --target=i686-apple-darwin10 --with-gxx-include-dir=/include/c++/4.2.1 Thread model: posix gcc version 4.2.1 (Apple Inc. build 5666) (dot 3) (Now it occurred to me that forgetting the #include parse_func.h might hit this breakage..., so I'll fix it here and continue to test, but if you'll fix it yourself, let me know) Regards, -- Hitoshi Harada -- 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] psql \timing vs failed statements
On Wed, Jan 18, 2012 at 06:28, Noah Misch n...@leadboat.com wrote: On Tue, Jan 17, 2012 at 04:01:23PM +0100, Magnus Hagander wrote: Thus - if I were to change psql to output timing on failed queries as well, will anybody object? ;) +1 Done and applied. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Vacuum rate limit in KBps
On 01/17/2012 09:00 PM, Jim Nasby wrote: Could we expose both? On our systems writes are extremely cheap... we don't do a ton of them (relatively speaking), so they tend to just fit into BBU cache. Reads on the other hard are a lot more expensive, at least if they end up actually hitting disk. So we actually set page_dirty and page_hit the same. My thinking had been that you set as the rate tunable, and then the rates of the others can be adjusted by advanced users using the ratio between the primary and the other ones. So at the defaults: vacuum_cost_page_hit = 1 vacuum_cost_page_miss = 10 vacuum_cost_page_dirty = 20 Setting a read rate cap will imply a write rate cap at 1/2 the value. Your setup would then be: vacuum_cost_page_hit = 1 vacuum_cost_page_miss = 10 vacuum_cost_page_dirty = 1 Which would still work fine if the new tunable was a read cap. If the cap is a write one, though, this won't make any sense. It would allow reads to happen at 10X the speed of writes, which is weird. I need to go back and consider each of the corner cases here, where someone wants one of [hit,miss,dirty] to be an unusual value relative to the rest. If I can't come up with a way to make that work as it does now in the new code, that's a problem. I don't think it really is, it's just that people in that situation will need to all three upwards. It's still a simpler thing to work out than the current situation, and this is an unusual edge case. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.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] IDLE in transaction introspection
On Tue, Jan 17, 2012 at 01:43, Scott Mead sco...@openscg.com wrote: On Mon, Jan 16, 2012 at 6:10 PM, Scott Mead sco...@openscg.com wrote: On Sun, Jan 15, 2012 at 4:28 AM, Greg Smith g...@2ndquadrant.com wrote: On 01/12/2012 11:57 AM, Scott Mead wrote: Pretty delayed, but please find the attached patch that addresses all the issues discussed. The docs on this v4 look like they suffered a patch order problem here. In the v3, you added a whole table describing the pg_stat_activity documentation in more detail than before. v4 actually tries to remove those new docs, a change which won't even apply as they don't exist upstream. My guess is you committed v3 to somewhere, applied the code changes for v4, but not the documentation ones. It's easy to do that and end up with a patch that removes a bunch of docs the previous patch added. I have to be careful to always do something like git diff origin/master to avoid this class of problem, until I got into that habit I did this sort of thing regularly. gak I did a 'backwards' diff last time. This time around, I diff-ed off of a fresh pull of 'master' (and I did the diff in the right direction. Also includes whitespace cleanup and the pg_stat_replication (procpid == pid) regression fix. I'm reviewing this again, and have changed a few things around. I came up with a question, too :-) Right now, if you turn off track activities, we put command string not enabled in the query text. Shouldn't this also be a state, such as disabled? It seems more consistent to me... -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simulating Clog Contention
$ pgbench --help pgbench is a benchmarking tool for PostgreSQL. Usage: pgbench [OPTIONS]... [DBNAME] Initialization options: -i invokes initialization mode using COPY -I invokes initialization mode using INSERTs sounds usefull. what about a long extra option: --inserts like pg_dump ? pgbench -i --inserts ... -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation -- 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] Client Messages
On Wed, Jan 18, 2012 at 3:08 AM, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: On 18.01.2012 07:49, Fujii Masao wrote: On Fri, Jan 6, 2012 at 1:38 AM, Jim Mlodgenskijimm...@gmail.com wrote: I have a need to send banner messages to a psql client that I can set on the server and will be displayed on any psql client that connects to the database. This would be mostly used as an additional indicator to which database you are connecting, but could also be used by people to force their users to see an security message when connecting to the database. The attached patch will allow you to execute ALTER DATABASE postgres SET client_message=E'\nBEWARE: You are connecting to a production database. If you do anything to\n bring this server down, you will be destroyed by your supreme overlord.\n\n'; And then when you connect to psql, you will see: [e3@workstation bin]$ ./psql -U user1 postgres psql (9.2devel) BEWARE: You are connecting to a production database. If you do anything to bring this server down, you will be destroyed by your supreme overlord. Type help for help. postgres= Any feedback is welcome. Adding new GUC parameter only for the purpose of warning psql users seems overkill to me. Basically we try to reduce the number of GUC parameters to make a configuration easier to a user, so I don't think that it's good idea to add new GUC for such a small benefit. It seems quite useful to me... Instead, how about using .psqlrc file and writing a warning message in it by using \echo command? That's not the same thing at all. Each client would need to put the warning in that file, and you'd get it regardless of the database you connect to. Anyway, I found one problem in the patch. The patch defines client_message as PGC_USERSET parameter, which means that any psql can falsify a warning message, e.g., by setting the environment variable PGOPTIONS to -c client_message=hoge. This seems to be something to avoid from security point of view. I don't think that's a problem, it's just a free-form message to display. But it also doesn't seem very useful to have it PGC_USERSET: if it's only displayed at connect time, there's no point in changing it after connecting. Should we make it PGC_BACKEND? The only security problem that I can think of is a malicious server (man-in-the-middle perhaps), that sends a banner that confuses Docs for PQparameterStatus() needs adjustment, now that client_message is also one of the settings automatically reported to the client. I'll add the docs for that.. The placement of the banner in psql looks currently like this: $ psql postgres psql (9.2devel) Hello world! Type help for help. or postgres=# \c postgres Hello world! You are now connected to database postgres as user heikki. Are we happy with that? I think it would be better to print the banner just before the prompt: I like that better. I'll make that change as well. psql (9.2devel) Type help for help. Hello world! postgres=# \c postgres You are now connected to database postgres as user heikki. Hello world! postgres=# Should we prefix the banner with something that makes it clear that it's a message coming from the server? Something like: I don't think the default prefix adds much for the user. If the administrator wants to let the user know that its from the server, he can add it to the message. psql (9.2devel) Type help for help. Notice from server: Hello world! postgres=# \c postgres You are now connected to database postgres as user heikki. Notice from server: Hello world! postgres=# -- 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] Command Triggers
Dimitri Fontaine dimi...@2ndquadrant.fr writes: Tom Lane t...@sss.pgh.pa.us writes: It doesn't, and it shouldn't have to. If those nodes get to the rewriter then somebody forgot to apply parse analysis. What's your test case? I'm trying to rewrite the command string from the parse tree, and the simple example that I use to raise an ERROR is the following: create table foo (id serial, foo integer default 1, primary key(id)); That needs to go through transformCreateStmt(). The comments at the head of parse_utilcmd.c might be informative. While I've not looked at your patch, I can't escape the suspicion that this means you are trying to do the wrong things in the wrong places. Calling transformCreateStmt() from some random place is not going to make things better; it is going to break CREATE TABLE, which expects to do that for itself. 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] Measuring relation free space
On Mon, Jan 16, 2012 at 5:09 AM, Noah Misch n...@leadboat.com wrote:
pgstattuple()'s decision to treat half-dead pages like deleted pages is
better. That transient state can only end in the page's deletion.
the only page in that index has 200 records (all live 0 dead) using
half the page size (which is a leaf page and is not half dead, btw).
so, how do you justify that pgstattuple say we have just 25% of free
space?
postgres=# SELECT * from bt_page_stats('pgbench_tellers_pkey', 1);
-[ RECORD 1 ]-+-
blkno | 1
type | l
live_items | 200
dead_items | 0
avg_item_size | 16
page_size | 8192
free_size | 4148
btpo_prev| 0
btpo_next| 0
btpo| 0
btpo_flags | 3
I don't know about counting non-leaf pages
ignoring all non-leaf pages still gives a considerable difference
between pgstattuple and relation_free_space()
--
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
--
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] gistVacuumUpdate
On 13.01.2012 06:24, YAMAMOTO Takashi wrote: hi, gistVacuumUpdate was removed when old-style VACUUM FULL was removed. i wonder why. it was not practical and REINDEX is preferred? anyway, the removal seems incomplete and there are some leftovers: F_TUPLES_DELETED F_DELETED XLOG_GIST_PAGE_DELETE Hmm, in theory we might bring back support for deleting pages in the future, I'm guessing F_DELETED and the WAL record type were left in place because of that. Either that, or it was an oversight. It's also good to have the F_DELETED/F_TUPLES_DELETED around, so that new versions don't get confused if they see those set in GiST indexes that originate from an old cluster, upgraded to new version with pg_upgrade. For that purpose, a comment explaining what those used to be would've been enough, though. -- 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] IDLE in transaction introspection
On Wed, Jan 18, 2012 at 8:27 AM, Magnus Hagander mag...@hagander.netwrote: On Tue, Jan 17, 2012 at 01:43, Scott Mead sco...@openscg.com wrote: On Mon, Jan 16, 2012 at 6:10 PM, Scott Mead sco...@openscg.com wrote: On Sun, Jan 15, 2012 at 4:28 AM, Greg Smith g...@2ndquadrant.com wrote: On 01/12/2012 11:57 AM, Scott Mead wrote: Pretty delayed, but please find the attached patch that addresses all the issues discussed. The docs on this v4 look like they suffered a patch order problem here. In the v3, you added a whole table describing the pg_stat_activity documentation in more detail than before. v4 actually tries to remove those new docs, a change which won't even apply as they don't exist upstream. My guess is you committed v3 to somewhere, applied the code changes for v4, but not the documentation ones. It's easy to do that and end up with a patch that removes a bunch of docs the previous patch added. I have to be careful to always do something like git diff origin/master to avoid this class of problem, until I got into that habit I did this sort of thing regularly. gak I did a 'backwards' diff last time. This time around, I diff-ed off of a fresh pull of 'master' (and I did the diff in the right direction. Also includes whitespace cleanup and the pg_stat_replication (procpid == pid) regression fix. I'm reviewing this again, and have changed a few things around. I came up with a question, too :-) Right now, if you turn off track activities, we put command string not enabled in the query text. Shouldn't this also be a state, such as disabled? It seems more consistent to me... Sure, I was going the route of 'client_addr', i.e. leaving it null when not in use just to keep screen clutter down, but I'm not married to it. --Scott OpenSCG, http://www.openscg.com -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Generate call graphs in run-time
On 01/17/2012 11:13 AM, Joel Jacobson wrote:
Since you only care about the parentfuncid in one level, it looks like
you will only be able to get a total call graph of all possible
function calls, and not each unique call graph per transaction.
True, for my purposes (function dependencies and performance analysis)
the global graph is sufficient.
Also, why remove the parent oid when uploading the statistics to the
collector?
It would be nice to have the statistics for each function per parent,
to see where you have a bottleneck which might only be occurring in a
function when called from a specific parent.
I guess I was just lazy at the time I wrote it. But it shouldn't be too
much of an effort to store the global call graph in statistics
collector. Unique call graphs would be somewhat more complicated I
guess.
There is a patch for this and we do use it in production for occasional
troubleshooting and dependency analysis. Can't attach immediately
though -- it has some extra cruft in it that needs to be cleaned up.
I would highly appreciate a patch, don't worry about cleaning up, I
can do that, unless it's some code you can't share for other reasons.
Patch attached. It was developed against 9.1, but also applies to
HEAD but gives some fuzz and offsets.
It adds 2 GUC variables: log_function_calls and log_usage_stats. The
first just output function statistics to log (with no parent info).
With log_usage_stats enabled, it also outputs detailed function usage
plus relation usage. So you basically get output such as:
# select * from pgq.get_consumer_info();
LOG: duration: 11.153 ms statement: select * from pgq.get_consumer_info();
LOG: function call: pgq.get_consumer_info(0) calls=1 time=9726
self_time=536
LOG: USAGE STATISTICS
DETAIL: ! object access stats:
! F 1892464226 0 pgq.get_consumer_info(0) calls=1 time=9726 self_time=536
! F 1892464228 1892464226 pgq.get_consumer_info(2) calls=1 time=9190
self_time=9190
! r 167558000 pgq.queue: blks_read=28 blks_hit=28
! r 167557988 pgq.consumer: blks_read=56 blks_hit=56
! r 167558021 pgq.subscription: blks_read=54 blks_hit=50
! r 167558050 pgq.tick: blks_read=103 blks_hit=102
! i 1892464189 pgq.queue_pkey: scans=1 tup_ret=37 tup_fetch=37
blks_read=2 blks_hit=2
! i 167557995 pgq.consumer_pkey: scans=56 tup_ret=56 tup_fetch=56
blks_read=57 blks_hit=56
! i 1892464195 pgq.subscription_pkey: scans=37 tup_ret=156 tup_fetch=56
blks_read=127 blks_hit=123
! i 167558058 pgq.tick_pkey: scans=112 tup_ret=103 tup_fetch=103
blks_read=367 blks_hit=366
funcid-parentfuncid might be sufficient for performance
optimizations, but to automatically generate directional graphs of all
unique call graphs in run-time, you would need all the unique pairs of
funcid-parentfuncid as a singel column, probably a sorted array of
oids[][], example: [[1,2],[1,3],[2,4],[2,5]] if the call craph would
be {1-2, 1-3, 2-4, 2-5}.
Hmm, array would probably work, although I wonder if there is a
more elegant solution ...
regards,
Martin
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 5ed6e83..2e66020 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -42,6 +42,7 @@
#include access/twophase_rmgr.h
#include access/xact.h
#include catalog/pg_database.h
+#include catalog/pg_namespace.h
#include catalog/pg_proc.h
#include libpq/ip.h
#include libpq/libpq.h
@@ -63,7 +64,8 @@
#include utils/ps_status.h
#include utils/rel.h
#include utils/tqual.h
-
+#include utils/syscache.h
+#include utils/lsyscache.h
/* --
* Paths for the statistics files (relative to installation's $PGDATA).
@@ -113,6 +115,12 @@ bool pgstat_track_counts = false;
int pgstat_track_functions = TRACK_FUNC_OFF;
int pgstat_track_activity_query_size = 1024;
+/*
+ * Last function call parent. InvalidOid is used to indicate
+ * top-level functions.
+ */
+Oid current_function_parent = InvalidOid;
+
/* --
* Built from GUC parameter
* --
@@ -258,8 +266,11 @@ static void pgstat_read_current_status(void);
static void pgstat_send_tabstat(PgStat_MsgTabstat *tsmsg);
static void pgstat_send_funcstats(void);
static HTAB *pgstat_collect_oids(Oid catalogid);
+static void pgstat_report_functions(void);
+static void pgstat_report_usage(void);
static PgStat_TableStatus *get_tabstat_entry(Oid rel_id, bool isshared);
+static PgStat_TableStatus *new_tsa_entry(TabStatusArray *entry, Oid rel_id, bool isshared);
static void pgstat_setup_memcxt(void);
@@ -697,6 +708,12 @@ pgstat_report_stat(bool force)
last_report = now;
/*
+ * First report function calls and object usage.
+ */
+ pgstat_report_functions();
+ pgstat_report_usage();
+
+ /*
* Scan through the TabStatusArray struct(s) to find tables that actually
* have counts, and build messages to send. We have to separate shared
* relations from regular ones because the databaseid field in the message
@@ -804,9 +821,6 @@
Re: [HACKERS] patch: fix SSI finished list corruption
On 07.01.2012 02:15, Dan Ports wrote: There's a corner case in the SSI cleanup code that isn't handled correctly. It can arise when running workloads that are comprised mostly (but not 100%) of READ ONLY transactions, and can corrupt the finished SERIALIZABLEXACT list, potentially causing a segfault. The attached patch fixes it. Specifically, when the only remaining active transactions are READ ONLY, we do a partial cleanup of committed transactions because certain types of conflicts aren't possible anymore. For committed r/w transactions, we release the SIREAD locks but keep the SERIALIZABLEXACT. However, for committed r/o transactions, we can go further and release the SERIALIZABLEXACT too. The problem was with the latter case: we were returning the SERIALIZABLEXACT to the free list without removing it from the finished list. The only real change in the patch is the SHMQueueDelete line, but I also reworked some of the surrounding code to make it obvious that r/o and r/w transactions are handled differently -- the existing code felt a bit too clever. Thanks, committed! -- 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] Patch review for logging hooks (CF 2012-01)
On 01/18/2012 03:56 AM, Fujii Masao wrote: or syslog process (if you use syslog). So ISTM that there is no guarantee that the order of log messages processed by the hook is same as that of messages written to the log file. For example, imagine the case where two backends call EmitErrorReport() at the same time. Is this OK? If not, the hook might need to be in syslogger. For high volume logging I'd avoid going through the syslogger. One big issue with syslogger is that it creates a choke point - everything has to pass through it, and if it cannot keep up it starts stalling the backends. Also, in EmitErrorReport the hook gets to have access to the actual ErrorData structure -- that makes filtering and looking at message content much simpler. EmitErrorReport() can be called before shared library like Martin's pg_logforward is preloaded. Which means that the hook may miss some log messages. Is this OK? True, even though the shared or local preload libraries are loaded quite early in PostmasterMain/PostgresMain there is a chance that some messages (mostly initialization failures) are missed. I guess there is no way to avoid this with module based approach. But IMHO this is acceptable -- when the backend fails to initialise, it cannot be assumed that the logging subsystem is set up properly. I guess it deserves at least a comment though. Thanks for pointing this out. regards, Martin -- 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 review for logging hooks (CF 2012-01)
On 01/18/2012 11:12 AM, Martin Pihlak wrote: On 01/18/2012 03:56 AM, Fujii Masao wrote: or syslog process (if you use syslog). So ISTM that there is no guarantee that the order of log messages processed by the hook is same as that of messages written to the log file. For example, imagine the case where two backends call EmitErrorReport() at the same time. Is this OK? If not, the hook might need to be in syslogger. For high volume logging I'd avoid going through the syslogger. One big issue with syslogger is that it creates a choke point - everything has to pass through it, and if it cannot keep up it starts stalling the backends. Also, in EmitErrorReport the hook gets to have access to the actual ErrorData structure -- that makes filtering and looking at message content much simpler. Hmm, interesting. I don't think I've encountered a situation where backends would actually stall. But in any case, I don't think we have to be that deterministic. The only thing that needs to be absolutely guaranteed is that the log messages from a given backend are in order. Some slight fuzz between backends seems acceptable. 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] Patch review for logging hooks (CF 2012-01)
Excerpts from Andrew Dunstan's message of mié ene 18 13:27:40 -0300 2012: On 01/18/2012 11:12 AM, Martin Pihlak wrote: On 01/18/2012 03:56 AM, Fujii Masao wrote: or syslog process (if you use syslog). So ISTM that there is no guarantee that the order of log messages processed by the hook is same as that of messages written to the log file. For example, imagine the case where two backends call EmitErrorReport() at the same time. Is this OK? If not, the hook might need to be in syslogger. For high volume logging I'd avoid going through the syslogger. One big issue with syslogger is that it creates a choke point - everything has to pass through it, and if it cannot keep up it starts stalling the backends. Also, in EmitErrorReport the hook gets to have access to the actual ErrorData structure -- that makes filtering and looking at message content much simpler. Hmm, interesting. I don't think I've encountered a situation where backends would actually stall. You have to have really high velocity for this to happen. At least one customer of ours has suffered this problem (I vaguely recall a second case but I'm not really sure), having had to switch to syslog (which uses lossy sockets, with the advantage that it doesn't cause stalls). But in any case, I don't think we have to be that deterministic. The only thing that needs to be absolutely guaranteed is that the log messages from a given backend are in order. Some slight fuzz between backends seems acceptable. Agreed. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. 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] Command Triggers
Tom Lane t...@sss.pgh.pa.us writes: create table foo (id serial, foo integer default 1, primary key(id)); That needs to go through transformCreateStmt(). The comments at the head of parse_utilcmd.c might be informative. Indeed, thanks for the heads up here. While I've not looked at your patch, I can't escape the suspicion that this means you are trying to do the wrong things in the wrong places. Calling transformCreateStmt() from some random place is not going to make things better; it is going to break CREATE TABLE, which expects to do that for itself. From the comments in the file, it seems like I could either call the function where I need it on a copy of the parse tree (as made by the copyObject() function), or reshuffle either when that call happen or when the calling of the command trigger user functions happens. At the moment the trigger functions are called from standard_ProcessUtility() and are given the parse tree as handed over to that function, before the parse analysis. We can easily enough copy the parse tree and do another round of parse analysis on it only when some command triggers are going to get called. Is the cost of doing so acceptable? Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et 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] lots of unused variable warnings in assert-free builds
On sön, 2012-01-15 at 01:37 -0500, Tom Lane wrote: Peter Eisentraut pete...@gmx.net writes: I see that in some places our code already uses #ifdef USE_ASSERT_CHECKING, presumably to hide similar issues. But in most cases using this would significantly butcher the code. I found that adding __attribute__((unused)) is cleaner. Attached is a patch that cleans up all the warnings I encountered. Surely this will fail entirely on most non-gcc compilers? No, because __attribute__() is defined to empty for other compilers. We use this pattern already. Not to mention that next month's gcc may complain hey, you used this 'unused' variable. No, because __attribute__((unused)) means that the variable is meant to be possibly unused. -- 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] lots of unused variable warnings in assert-free builds
On sön, 2012-01-15 at 08:14 -0500, Andrew Dunstan wrote: It would possibly have some documentary value too. Just looking very quickly at Peter's patch, I don't really understand his assertion that this would significantly butcher the code. The worst effect would be that in a few cases we'd have to break up multiple declarations where one of the variables was in this class. That doesn't seem like a tragedy. Well, I'll prepare a patch like that and then you can judge. -- 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] Command Triggers
Excerpts from Dimitri Fontaine's message of mié ene 18 16:03:29 -0300 2012: At the moment the trigger functions are called from standard_ProcessUtility() and are given the parse tree as handed over to that function, before the parse analysis. We can easily enough copy the parse tree and do another round of parse analysis on it only when some command triggers are going to get called. Is the cost of doing so acceptable? Huh, isn't it simpler to just pass the triggers the parse tree *after* parse analysis? I don't understand what you're doing here. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. 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] lots of unused variable warnings in assert-free builds
Peter Eisentraut pete...@gmx.net writes: On sön, 2012-01-15 at 01:37 -0500, Tom Lane wrote: Surely this will fail entirely on most non-gcc compilers? No, because __attribute__() is defined to empty for other compilers. We use this pattern already. Oh, duh. In that case my only objection to doing it like this is that I'd like to see what pgindent will do with it. pgindent is not very nice about #ifdef's in variable lists (it tends to insert unwanted vertical space) so it's possible that this way will look better. 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] return values of backend sub-main functions
On lör, 2012-01-07 at 16:41 -0500, Tom Lane wrote: Peter Eisentraut pete...@gmx.net writes: I suggest that we change PostgresMain(), PostmasterMain(), BackendRun(), WalSenderMain(), and WalSndLoop() to return void as well. I agree this code is not very consistent or useful, but one question: what should the callers do if one of these functions *does* return? I was thinking of a two-pronged approach: First, add __attribute__((noreturn)) to the functions. This will cause a suitable compiler to verify on a source-code level that nothing actually returns from the function. And second, at the call site, put an abort(); /* not reached */. Together, this will make the code cleaner and more consistent, and will also help the compiler out a bit about the control flow. -- 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] Command Triggers
On Wednesday, January 18, 2012 08:17:36 PM Alvaro Herrera wrote: Excerpts from Dimitri Fontaine's message of mié ene 18 16:03:29 -0300 2012: At the moment the trigger functions are called from standard_ProcessUtility() and are given the parse tree as handed over to that function, before the parse analysis. We can easily enough copy the parse tree and do another round of parse analysis on it only when some command triggers are going to get called. Is the cost of doing so acceptable? Huh, isn't it simpler to just pass the triggers the parse tree *after* parse analysis? I don't understand what you're doing here. Parse analysis is not exactly nicely separated for utility statements. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Setting -Werror in CFLAGS
On ons, 2012-01-04 at 00:39 +, Peter Geoghegan wrote: This should work: ./configure --prefix=/home/peter/pgsql CFLAGS=-Werror -Wno-error=unused-but-set-variable However, it does not (with or without the -Wno-error): checking whether getpwuid_r takes a fifth argument... no checking whether strerror_r returns int... no checking test program... ok checking whether long int is 64 bits... no checking whether long long int is 64 bits... no configure: error: Cannot find a working 64-bit integer type. Obviously it breaks this one configure test. However, I got it to work in 2 minutes by hacking up config.log. It would be nice if someone could fix the test (and possibly later ones) so that it doesn't produce a warning/error when invoking the compiler, and it finds a 64-bit int type as expected. I've tried this in the past, it's pretty difficult to make this work throughout. I think and easier approach would be to filter out -Werror out of CFLAGS early in configure and put it back at the end. That way, it could sort of become folk wisdom that you should build Postgres with those flags during development, and maybe even, on some limited basis, on the build farm, and we'd all be sightly better off. I think in practice many developers do this anyway, so you do get that level of testing already. There is an entry in the parser Makefile to support this sort of use, which makes it look like my -Wno-error=unused-but-set-variable use would be redundant if it worked: # Latest flex causes warnings in this file. ifeq ($(GCC),yes) gram.o: CFLAGS += -Wno-error endif (although we could discriminate better within the parse here by using my flag). The -Wno-error=something flag doesn't work in older versions of GCC. But feel free to research which ones. As if to prove my point, I found this warning which I didn't previously notice, just from 5 minutes of playing around: hashovfl.c: In function ‘_hash_freeovflpage’: hashovfl.c:394:10: error: variable ‘bucket’ set but not used [-Werror=unused-but-set-variable] cc1: all warnings being treated as errors (plus others) See thread [HACKERS] lots of unused variable warnings in assert-free builds for the reason. -- 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] Command Triggers
Dimitri Fontaine dimi...@2ndquadrant.fr writes: We can easily enough copy the parse tree and do another round of parse analysis on it only when some command triggers are going to get called. Is the cost of doing so acceptable? It's not the costs I'm worried about so much as the side effects --- locks and so forth. Also, things like assignment of specific names for indexes and sequences seem rather problematic. In the worst case the trigger could run seeing foo_bar_idx1 as the name of an index to be created, and then when the action actually happens, the name turns out to be foo_bar_idx2 because someone else took the first name meanwhile. As I said, I think this suggests that you're trying to do the triggers in the wrong place. 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] Setting -Werror in CFLAGS
On tis, 2012-01-03 at 21:23 -0500, Tom Lane wrote: I see no point in -Werror whatsoever. If you aren't examining the make output for warnings, you're not following proper development practice IMO. gcc is not the only tool we use in the build process, so if you are relying on -Werror to call attention to everything you should be worrying about, you lost already. Well, cite your source. ;-) Proper software development practices, as I understand them, is that there is one command to build the thing, and that either fails or it doesn't. If we rely on people to read the build log, then we've lost already. The only particular case where this doesn't work that I can think of is that you need to examine the flex output for warnings. But that is very isolated. If you don't change the scanner source files (and perhaps one or two other things, or if you change the build system itself), then relying on the exit status of make should be enough. If not, we need to fix that. I'm also less than thrilled with the idea that whatever the gcc boys decide to make a warning tomorrow will automatically become a MUST FIX NOW for us. If you don't see why this is a problem, try building any PG release more than a few months old on latest and greatest gcc. That's why -Werror should never be the default for a postgresql build, but that doesn't mean that we can't make it a useful optional tool. (Of note here, latest-and-greatest is changing again this week, at least for Fedora, and I fully expect 4.7 to start whinging about things we never heard of before.) As a side note: I tested gcc 4.7 a few weeks ago and it doesn't introduce any new warnings. Of course, it's not final yet, but it's pretty close. -- 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] Setting -Werror in CFLAGS
On ons, 2012-01-04 at 13:44 -0500, Robert Haas wrote: I'm not thrilled about that either. Especially since they seem to be adding more and more warnings that are harder and harder to work around for issues that are less and less important. Unimportant warnings that are easily avoidable are not so bad, but... I think the reason they add all these new warnings is that a lot of software is of poor quality. A lot of software probably doesn't check any return values, so they need to see those warnings. If the last 3 out of 100 are hard to fix, that might be a small price to pay. -- 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] age(xid) on hot standby
On mån, 2012-01-16 at 22:59 -0300, Alvaro Herrera wrote: Excerpts from Tom Lane's message of lun ene 16 12:27:03 -0300 2012: Alvaro Herrera alvhe...@commandprompt.com writes: Excerpts from Peter Eisentraut's message of dom ene 15 10:00:03 -0300 2012: On ons, 2011-12-28 at 14:35 -0500, Tom Lane wrote: The trouble with using ReadNewTransactionId is that it makes the results volatile, not stable as the function is declared to be. Could we alleviate that problem with some caching within the function? Maybe if we have it be invalidated at transaction end, that could work. So each new transaction would get a fresh value. Yeah, I think that would work. So who's going to work on a patch? Peter, are you? If not, we should add it to the TODO list. Not at this very moment, but maybe in a few weeks. -- 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] Setting -Werror in CFLAGS
On 18 January 2012 19:40, Peter Eisentraut pete...@gmx.net wrote: On ons, 2012-01-04 at 13:44 -0500, Robert Haas wrote: I'm not thrilled about that either. Especially since they seem to be adding more and more warnings that are harder and harder to work around for issues that are less and less important. Unimportant warnings that are easily avoidable are not so bad, but... I think the reason they add all these new warnings is that a lot of software is of poor quality. A lot of software probably doesn't check any return values, so they need to see those warnings. If the last 3 out of 100 are hard to fix, that might be a small price to pay. I recently ran a static analysis tool, Clang Static Analyzer, on Postgres. It was an amusing distraction for half an hour, but it wasn't particularly useful. It was immediately obvious 1) Why the tool flagged certain code as possibly questionable and 2) Why it wasn't actually questionable at all. It would probably be really useful with a poor quality codebase though. As I've already said, the fact that a Clang warning successfully flagged a bug in Postgres a while back does go to show that there is some benefit to be had from all of these sorts of diagnostics. I have a rather low opinion of GCC's diagnostics though. -- Peter Geoghegan http://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] age(xid) on hot standby
Peter Eisentraut pete...@gmx.net writes:
On mån, 2012-01-16 at 22:59 -0300, Alvaro Herrera wrote:
So who's going to work on a patch? Peter, are you? If not, we should
add it to the TODO list.
Not at this very moment, but maybe in a few weeks.
BTW, it strikes me that maybe the coding should work about like this:
if (!TransactionIdIsValid(age_reference_xid))
{
age_reference_xid = GetTopTransactionIdIfAny();
if (!TransactionIdIsValid(age_reference_xid))
age_reference_xid = ReadNewTransactionId();
}
... use age_reference_xid to compute result ...
and of course code somewhere to reset age_reference_xid at end of xact.
The advantage of this is
(1) same code works on master and standby
(2) calling age() no longer requires an otherwise read-only transaction
to acquire an XID.
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] foreign key locks, 2nd attempt
Excerpts from Heikki Linnakangas's message of mar ene 17 03:21:28 -0300 2012: On 16.01.2012 21:52, Alvaro Herrera wrote: Excerpts from Heikki Linnakangas's message of lun ene 16 16:17:42 -0300 2012: On 15.01.2012 06:49, Alvaro Herrera wrote: - pg_upgrade bits are missing. I guess we'll need to rewrite pg_multixact contents in pg_upgrade. Is the page format backwards-compatible? It's not. I haven't worked out what pg_upgrade needs to do, honestly. I think we should just not copy old pg_multixact files when upgrading across this patch. Sorry, I meant whether the *data* page format is backwards-compatible? the multixact page format clearly isn't. It's supposed to be, yes. The HEAP_XMAX_IS_NOT_UPDATE bit (now renamed) was chosen so that it'd take the place of the old HEAP_XMAX_SHARE_LOCK bit, so any pages with that bit set should continue to work similarly. The other infomask bits I used were previously unused. I was initially thinking that pg_multixact should return the empty set if requested members of a multi that preceded the freeze point. That way, I thought, we would just never try to access a page originated in the older version (assuming the freeze point is set to current whenever pg_upgrade runs). However, as things currently stand, accessing an old multi raises an error. So maybe we need a scheme a bit more complex to handle this. Hmm, could we create new multixact files filled with zeros, covering the range that was valid in the old cluster? Hm, we could do something like that I guess. I'm not sure that just zeroes is the right pattern, but it should be something simple. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. 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] age(xid) on hot standby
On Wed, Jan 18, 2012 at 7:55 PM, Tom Lane t...@sss.pgh.pa.us wrote:
Peter Eisentraut pete...@gmx.net writes:
On mån, 2012-01-16 at 22:59 -0300, Alvaro Herrera wrote:
So who's going to work on a patch? Peter, are you? If not, we should
add it to the TODO list.
Not at this very moment, but maybe in a few weeks.
BTW, it strikes me that maybe the coding should work about like this:
if (!TransactionIdIsValid(age_reference_xid))
{
age_reference_xid = GetTopTransactionIdIfAny();
if (!TransactionIdIsValid(age_reference_xid))
age_reference_xid = ReadNewTransactionId();
}
... use age_reference_xid to compute result ...
and of course code somewhere to reset age_reference_xid at end of xact.
The advantage of this is
(1) same code works on master and standby
(2) calling age() no longer requires an otherwise read-only transaction
to acquire an XID.
Nice solution.
Also it illustrates that some users write code that tries to do things
on a Hot Standby that are supposed to be illegal.
If the OPs error message had returned the correct SQLCODE then it
would have been better able to handle the message.
I think we should apply the patch to return the correct SQLCODE in all
cases, even if its supposedly not possible.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training 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] [WIP] Double-write with Fast Checksums
9.2 + DW patch --- FPW off FPW on DW on/FPW off CK on CK on CK on one disk: 11078 10394 3296 [1G shared_buffers, 8G RAM] sep log disk: 13605 12015 3412 one disk: 7731 6613 2670 [1G shared_buffers, 2G RAM] sep log disk: 6752 6129 2722 On my single Hard disk test with write cache turned off I see different results than what Dan sees.. DBT2 50-warehouse, 1hr steady state with shared_buffers 1G, checkpoint_segments=128 as common settings on 8GB RAM) (checkpoints were on for all cases) with 8 Core . FPW off: 3942.25 NOTPM FPW on: 3613.37 NOTPM DW on : 3479.15 NOTPM I retried it with 2 core also and get similar results. So high evictions does have slighly higher penalty than FPW. My run somehow did not collect the background writer stats so dont have that comparison for these runs but have fixed it for the next runs. Regards, Jignesh -- 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] Command Triggers
On Wednesday, January 18, 2012 08:31:49 PM Tom Lane wrote: Dimitri Fontaine dimi...@2ndquadrant.fr writes: We can easily enough copy the parse tree and do another round of parse analysis on it only when some command triggers are going to get called. Is the cost of doing so acceptable? It's not the costs I'm worried about so much as the side effects --- locks and so forth. Also, things like assignment of specific names for indexes and sequences seem rather problematic. In the worst case the trigger could run seeing foo_bar_idx1 as the name of an index to be created, and then when the action actually happens, the name turns out to be foo_bar_idx2 because someone else took the first name meanwhile. You can't generally assume such a thing anyway. Remember there can be BEFORE command triggers. It would be easy to create a conflict there. The CREATE TABLE will trigger command triggers on CREATE SEQUENCE and ALTER SEQUENCE while creating the table. If one actually wants to do anything about those that would be the place. As I said, I think this suggests that you're trying to do the triggers in the wrong place. In my opinion it mostly shows that parse analysis of utlity statments is to intermingled with other stuff Not sure what to do about that. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch review for logging hooks (CF 2012-01)
On 01/17/2012 11:40 PM, Marti Raudsepp wrote: It seems you missed a comment, that the current implementation is also affected by client_min_messages. I think that being affected by client-specific settings is surprising. I would put the if(emit_log_hook) inside the existing if(edata-output_to_server) condition. Unless you have some reason to do it this way? I have no strong feelings about this -- if the behaviour seems surprising, lets remove it. We need to keep the if separate though -- the hook might want to omit the message from server log so the output_to_server needs to be rechecked. Updated patch attached. regards, Martin *** a/src/backend/utils/error/elog.c --- b/src/backend/utils/error/elog.c *** *** 136,141 static int errordata_stack_depth = -1; /* index of topmost active frame */ --- 136,150 static int recursion_depth = 0; /* to detect actual recursion */ + /* + * Hook for intercepting log messages. Note that the hook does not get + * called for messages that are supressed by GUC settings such as + * log_min_messages. Also note that logging hooks implemented as preload + * libraries will miss any log messages that are generated before the + * library is loaded. + */ + emit_log_hook_type emit_log_hook = NULL; + /* buffers for formatted timestamps that might be used by both * log_line_prefix and csv logs. */ *** *** 1276,1281 EmitErrorReport(void) --- 1285,1297 CHECK_STACK_DEPTH(); oldcontext = MemoryContextSwitchTo(ErrorContext); + /* + * Call hooks for server messages. Note that the hook could opt to filter + * the message so we need to recheck output_to_server afterwards. + */ + if (edata-output_to_server emit_log_hook) + emit_log_hook(edata); + /* Send to server log, if enabled */ if (edata-output_to_server) send_message_to_server_log(edata); *** a/src/include/utils/elog.h --- b/src/include/utils/elog.h *** *** 327,332 typedef struct ErrorData --- 327,334 int saved_errno; /* errno at entry */ } ErrorData; + typedef void (*emit_log_hook_type)(ErrorData *edata); + extern void EmitErrorReport(void); extern ErrorData *CopyErrorData(void); extern void FreeErrorData(ErrorData *edata); *** *** 347,352 typedef enum --- 349,355 extern int Log_error_verbosity; extern char *Log_line_prefix; extern int Log_destination; + extern PGDLLIMPORT emit_log_hook_type emit_log_hook; /* Log destination bitmap */ #define LOG_DESTINATION_STDERR 1 -- 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] Vacuum rate limit in KBps
On Jan 18, 2012, at 3:49 AM, Greg Smith wrote: On 01/17/2012 09:00 PM, Jim Nasby wrote: Could we expose both? On our systems writes are extremely cheap... we don't do a ton of them (relatively speaking), so they tend to just fit into BBU cache. Reads on the other hard are a lot more expensive, at least if they end up actually hitting disk. So we actually set page_dirty and page_hit the same. My thinking had been that you set as the rate tunable, and then the rates of the others can be adjusted by advanced users using the ratio between the primary and the other ones. So at the defaults: vacuum_cost_page_hit = 1 vacuum_cost_page_miss = 10 vacuum_cost_page_dirty = 20 Setting a read rate cap will imply a write rate cap at 1/2 the value. Your setup would then be: vacuum_cost_page_hit = 1 vacuum_cost_page_miss = 10 vacuum_cost_page_dirty = 1 Which would still work fine if the new tunable was a read cap. If the cap is a write one, though, this won't make any sense. It would allow reads to happen at 10X the speed of writes, which is weird. I need to go back and consider each of the corner cases here, where someone wants one of [hit,miss,dirty] to be an unusual value relative to the rest. If I can't come up with a way to make that work as it does now in the new code, that's a problem. I don't think it really is, it's just that people in that situation will need to all three upwards. It's still a simpler thing to work out than the current situation, and this is an unusual edge case. What about doing away with all the arbitrary numbers completely, and just state data rate limits for hit/miss/dirty? BTW, this is a case where it would be damn handy to know if the miss was really a miss or not... in the case where we're already rate limiting vacuum, could we afford the cost of get_time_of_day() to see if a miss actually did have to come from disk? -- Jim C. Nasby, Database Architect j...@nasby.net 512.569.9461 (cell) http://jim.nasby.net -- 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] Command Triggers
Alvaro Herrera alvhe...@commandprompt.com writes: Huh, isn't it simpler to just pass the triggers the parse tree *after* parse analysis? I don't understand what you're doing here. I didn't realize that the parse analysis is in fact done from within standard_ProcessUtility() directly, which means your suggestion is indeed workable. Tom Lane t...@sss.pgh.pa.us writes: It's not the costs I'm worried about so much as the side effects --- Ok, so I'm now calling the command trigger procedures once the parse analysis is done, and guess what, I'm back to the same problem as before: https://github.com/dimitri/postgres/commit/4bfab6344a554c09f7322e861f9d09468f641bd9 CREATE TABLE public.ab_foo-bar ( id serial NOT NULL, foo integer default 1, PRIMARY KEY(id) ); NOTICE: CREATE TABLE will create implicit sequence ab_foo-bar_id_seq for serial column ab_foo-bar.id NOTICE: snitch: CREATE SEQUENCE ERROR: unrecognized node type: 904 I'm not sure about the next step, and I'm quite sure I need to stop here for tonight. Any advice welcome, I'll be working on that again as soon as tomorrow. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et 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] Patch review for logging hooks (CF 2012-01)
On Wed, Jan 18, 2012 at 22:56, Martin Pihlak martin.pih...@gmail.com wrote: We need to keep the if separate though -- the hook might want to omit the message from server log so the output_to_server needs to be rechecked. Oh, yes makes sense. The updated patch looks good, marking as 'Ready for Committer' Regards, Marti -- 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] Group commit, revised
On Wed, Jan 18, 2012 at 1:23 AM, Robert Haas robertmh...@gmail.com wrote: On Tue, Jan 17, 2012 at 12:37 PM, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: I found it very helpful to reduce wal_writer_delay in pgbench tests, when running with synchronous_commit=off. The reason is that hint bits don't get set until the commit record is flushed to disk, so making the flushes more frequent reduces the contention on the clog. However, Simon made async commits nudge WAL writer if the WAL page fills up, so I'm not sure how relevant that experience is anymore. Still completely relevant and orthogonal to this discussion. The patch retains multi-modal behaviour. There's still a small but measurable effect there in master. I think we might be able to make it fully auto-tuning, but I don't think we're fully there yet (not sure how much this patch changes that equation). I suggested a design similar to the one you just proposed to Simon when he originally suggested this feature. It seems that if the WAL writer is the only one doing WAL flushes, then there must be some IPC overhead - and context switching - involved whenever WAL is flushed. But clearly we're saving something somewhere else, on the basis of Peter's results, so maybe it's not worth worrying about. It does seem pretty odd to have all the regular backends going through the WAL writer and the auxiliary processes using a different mechanism, though. If we got rid of that, maybe WAL writer wouldn't even require a lock, if there's only one process that can be doing it at a time. When we did sync rep it made sense to have the WALSender do the work and for others to just wait. It would be quite strange to require a different design for essentially the same thing for normal commits and WAL flushes to local disk. I should mention the original proposal for streaming replication had each backend send data to standby independently and that was recognised as a bad idea after some time. Same for sync rep also. The gain is that previously there was measurable contention for the WALWriteLock, now there is none. Plus the gang effect continues to work even when the database gets busy, which isn't true of piggyback commits as we use now. Not sure why its odd to have backends do one thing and auxiliaries do another. The whole point of auxiliary processes is that they do a specific thing different to normal backends. Anyway, the important thing is to have auxiliary processes be independent of each other as much as possible, which simplifies error handling and state logic in the postmaster. With regard to context switching, we're making a kernel call to fsync, so we'll get a context switch anyway. The whole process is similar to the way lwlock wake up works. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training 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] Should I implement DROP INDEX CONCURRENTLY?
On Wed, Jan 18, 2012 at 1:28 AM, Robert Haas robertmh...@gmail.com wrote: On Tue, Jan 17, 2012 at 12:06 PM, Peter Eisentraut pete...@gmx.net wrote: On mån, 2012-01-16 at 14:46 -0500, Robert Haas wrote: On Mon, Jan 16, 2012 at 2:06 PM, Peter Eisentraut pete...@gmx.net wrote: On mån, 2012-01-16 at 11:17 -0500, Robert Haas wrote: I don't see how setting indisvalid to false helps with this, because IIUC when a session sees indisvalid = false, it is supposed to avoid using the index for queries but still make new index entries when a write operation happens - but to drop an index, I think you'd need to get into a state where no one was using the index for anything at all. ISTM that one would need to set indisready to false instead. Maybe we should set both to false? Well, ready = false and valid = true doesn't make any sense. There is only just-created - ready - valid. We might as well convert that to a single char column, as you had indicated in your earlier email. But that's independent of the proposed patch. Sure, but the point is that I think if you want everyone to stop touching the index, you ought to mark it both not-valid and not-ready, which the current patch doesn't do. Thanks for the review and the important observation. I agree that I've changed the wrong column. indisready must be set false. Also agree setting both false makes most sense. Can I just check with you that the only review comment is a one line change? Seems better to make any additional review comments in one go. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training 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] Should I implement DROP INDEX CONCURRENTLY?
On Wed, Jan 18, 2012 at 5:49 PM, Simon Riggs si...@2ndquadrant.com wrote: Can I just check with you that the only review comment is a one line change? Seems better to make any additional review comments in one go. No, I haven't done a full review yet - I just noticed that right at the outset and wanted to be sure that got addressed. I can look it over more carefully, and test it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL 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] gistVacuumUpdate
hi, On 13.01.2012 06:24, YAMAMOTO Takashi wrote: hi, gistVacuumUpdate was removed when old-style VACUUM FULL was removed. i wonder why. it was not practical and REINDEX is preferred? anyway, the removal seems incomplete and there are some leftovers: F_TUPLES_DELETED F_DELETED XLOG_GIST_PAGE_DELETE Hmm, in theory we might bring back support for deleting pages in the future, I'm guessing F_DELETED and the WAL record type were left in place because of that. Either that, or it was an oversight. It's also good to have the F_DELETED/F_TUPLES_DELETED around, so that new versions don't get confused if they see those set in GiST indexes that originate from an old cluster, upgraded to new version with pg_upgrade. For that purpose, a comment explaining what those used to be would've been enough, though. the loop in gistvacuumcleanup to search F_DELETED pages seems too expensive for pg_upgrade purpose. (while it also checks PageIsNew, is it alone worth the loop?) i'm wondering because what gistVacuumUpdate used to do does not seem to be necessarily tied to the old-style VACUUM FULL. currently, no one will re-union keys after tuple removals, right? YAMAMOTO Takashi -- 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] how to create a non-inherited CHECK constraint in CREATE TABLE
On Wed, Jan 18, 2012 at 12:10 AM, Nikhil Sontakke nikkh...@gmail.com wrote: It appears that the only way to create a non-inherited CHECK constraint is using ALTER TABLE. Is there no support in CREATE TABLE planned? That looks a bit odd. There are no plans to do that AFAIR, though maybe you could convince Nikhil to write the patch to do so. That certainly doesn't meet the principle of least surprise... CREATE TABLE should support this. Well, the above was thought about during the original discussion and eventually we felt that CREATE TABLE already has other issues as well, so not having this done as part of creating a table was considered acceptable then: http://postgresql.1045698.n5.nabble.com/Check-constraints-on-partition-parents-only-tt464.html#a4647144 But, let me have a stab at it when I get some free cycles. I agree with Peter that we should have we should have CHECK ONLY. ONLY is really a property of the constraint, not the ALTER TABLE command -- if it were otherwise, we wouldn't need to store it the system catalogs, but of course we do. The fact that it's not a standard property isn't a reason not to have proper syntax for it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Strange primary key constraint influence to grouping
Hi, accidentally found a our sql typo, which runs without errors on pg 9.1, but raises error on 9.0. It seems to me, that 9.0 behaviour was correct. Reproducing case: create table aaa(id integer NOT NULL, something double precision, constraint pk_aaa primary key (id)); insert into aaa values (1, 1), (2, null); select id, case when something is not null then 'something' else 'something is null' end as status from aaa group by id; drop table aaa cascade; In PG 9.0 this script complains that: column aaa.something must appear in the GROUP BY clause or be used in an aggregate function. Sorry, don't have 9.0 at my hands, but error message is similar to quoted. Same error is raised in 9.1 when ', constraint pk_aaa primary key (id)' is commented out. With PK constraint in place, this script runs happily, without complaints. Version with observerd behaviour: PostgreSQL 9.1.1, compiled by Visual C++ build 1500, 32-bit Best regards, Grazvydas
Re: [HACKERS] Strange primary key constraint influence to grouping
On 2012-01-19 00:25, Gražvydas Valeika wrote: In PG 9.0 this script complains that: column aaa.something must appear in the GROUP BY clause or be used in an aggregate function. Sorry, don't have 9.0 at my hands, but error message is similar to quoted. Same error is raised in 9.1 when ', constraint pk_aaa primary key (id)' is commented out. With PK constraint in place, this script runs happily, without complaints. Hi, This is because PostgreSQL 9.1 added the feature of simple checking of functional dependencies for GROUP BY. The manual of 9.1 explains quite well when PostgreSQL considers there to be a functional dependency. When GROUP BY is present, it is not valid for the SELECT list expressions to refer to ungrouped columns except within aggregate functions or if the ungrouped column is functionally dependent on the grouped columns, since there would otherwise be more than one possible value to return for an ungrouped column. A functional dependency exists if the grouped columns (or a subset thereof) are the primary key of the table containing the ungrouped column. http://www.postgresql.org/docs/9.1/interactive/sql-select.html#SQL-GROUPBY Best regards, Andreas -- Andreas Karlsson -- 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] Strange primary key constraint influence to grouping
This is because PostgreSQL 9.1 added the feature of simple checking of functional dependencies for GROUP BY. The manual of 9.1 explains quite well when PostgreSQL considers there to be a functional dependency. When GROUP BY is present, it is not valid for the SELECT list expressions to refer to ungrouped columns except within aggregate functions or if the ungrouped column is functionally dependent on the grouped columns, since there would otherwise be more than one possible value to return for an ungrouped column. A functional dependency exists if the grouped columns (or a subset thereof) are the primary key of the table containing the ungrouped column. I completely agree with documentation. But my case shows that not valid expression which refers to column which is ungrouped still works in 9.1. G.
Re: [HACKERS] Measuring relation free space
On Wed, Jan 18, 2012 at 09:46:20AM -0500, Jaime Casanova wrote:
On Mon, Jan 16, 2012 at 5:09 AM, Noah Misch n...@leadboat.com wrote:
pgstattuple()'s decision to treat half-dead pages like deleted pages is
better. ?That transient state can only end in the page's deletion.
the only page in that index has 200 records (all live 0 dead) using
half the page size (which is a leaf page and is not half dead, btw).
so, how do you justify that pgstattuple say we have just 25% of free
space?
postgres=# SELECT * from bt_page_stats('pgbench_tellers_pkey', 1);
-[ RECORD 1 ]-+-
blkno | 1
type | l
live_items | 200
dead_items | 0
avg_item_size | 16
page_size | 8192
free_size | 4148
btpo_prev| 0
btpo_next| 0
btpo| 0
btpo_flags | 3
I don't know about counting non-leaf pages
ignoring all non-leaf pages still gives a considerable difference
between pgstattuple and relation_free_space()
pgstattuple() counts the single B-tree meta page as always-full, while
relation_free_space() skips it for all purposes. For tiny indexes, that can
shift the percentage dramatically.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] sepgsql's DROP Permission checks
On Wed, Jan 18, 2012 at 9:50 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
In sepgsql side, it determines a case to apply permission checks
according to the contextual information; that is same technique
when we implemented create permission.
Thus, it could checks db_xxx:{drop} permission correctly.
Why do we need the contextual information in this case? Why
can't/shouldn't the decision be made solely on the basis of what
object is targeted?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] foreign key locks, 2nd attempt
On Wed, Jan 18, 2012 at 05:18:31PM -0300, Alvaro Herrera wrote: Excerpts from Heikki Linnakangas's message of mar ene 17 03:21:28 -0300 2012: On 16.01.2012 21:52, Alvaro Herrera wrote: I was initially thinking that pg_multixact should return the empty set if requested members of a multi that preceded the freeze point. That way, I thought, we would just never try to access a page originated in the older version (assuming the freeze point is set to current whenever pg_upgrade runs). However, as things currently stand, accessing an old multi raises an error. So maybe we need a scheme a bit more complex to handle this. Hmm, could we create new multixact files filled with zeros, covering the range that was valid in the old cluster? Hm, we could do something like that I guess. I'm not sure that just zeroes is the right pattern, but it should be something simple. PostgreSQL 9.1 can have all ~4B MultiXactId on disk at any given time. We could silently ignore the lookup miss when HEAP_XMAX_LOCK_ONLY is also set. That makes existing data files acceptable while still catching data loss scenarios going forward. (It's tempting to be stricter when we know the cluster data files originated in PostgreSQL 9.2+, but I'm not sure whether that's worth its weight.) -- 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] age(xid) on hot standby
Simon Riggs si...@2ndquadrant.com writes: I think we should apply the patch to return the correct SQLCODE in all cases, even if its supposedly not possible. [ shrug... ] My opinion about that has not changed. Those are internal sanity checks, and as such, ERRCODE_INTERNAL_ERROR is exactly the right thing for them. If there are paths that can reach that code, we need to find them and plug the holes with appropriate user-facing error checks that say what it is the user is not supposed to do. In this example, if we had decided that the right answer should be for age() to not be allowed on standbys, then an error saying exactly that would be an appropriate user-facing error. You're not supposed to acquire a transaction ID is not intelligible to the average user, and giving it another error code doesn't improve that situation. 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] Strange primary key constraint influence to grouping
Gra*vydas Valeika wrote: This is because PostgreSQL 9.1 added the feature of simple checking of functional dependencies for GROUP BY. The manual of 9.1 explains quite well when PostgreSQL considers there to be a functional dependency. When GROUP BY is present, it is not valid for the SELECT list expressions to refer to ungrouped columns except within aggregate functions or if the ungrouped column is functionally dependent on the grouped columns, since there would otherwise be more than one possible value to return for an ungrouped column. A functional dependency exists if the grouped columns (or a subset thereof) are the primary key of the table containing the ungrouped column. I completely agree with documentation. But my case shows that not valid expression which refers to column which is ungrouped still works in 9.1. It is not an invalid expression in the SELECT list, because it is functionally dependent on the primary key -- that is, given a particular primary key, there is only one value the expression can have. Because of this, adding the expression to the GROUP BY list cannot change the set of rows returned by the query. It is pointless to include the expression in the GROUP BY clause, so it is not required. This allows faster query execution. This is a new feature, not a bug. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
