Re: [HACKERS] SSL renegotiation
On Thu, Jul 11, 2013 at 4:20 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I'm having a look at the SSL support code, because one of our customers reported it behaves unstably when the network is unreliable. I have yet to reproduce the exact problem they're having, but while reading the code I notice this in be-secure.c:secure_write() : The recap of my experiences you requested... I first saw SSL renegotiation failures on Ubuntu 10.04 LTS (Lucid) with openssl 0.9.8 (something). I think this was because SSL renegotiation had been disabled due to due to CVE 2009-3555 (affecting all versions before 0.9.8l). I think the version now in lucid is 0.9.8k with fixes for SSL renegotiation, but I haven't tested this. The failures I saw with no-renegotiation-SSL for streaming replication looked like this: On the master: 2012-06-25 16:16:26 PDT LOG: SSL renegotiation failure 2012-06-25 16:16:26 PDT LOG: SSL error: unexpected record 2012-06-25 16:16:26 PDT LOG: could not send data to client: Connection reset by peer On the hot standby: 2012-06-25 11:12:11 PDT FATAL: could not receive data from WAL stream: SSL error: sslv3 alert unexpected message 2012-06-25 11:12:11 PDT LOG: record with zero length at 1C5/95D2FE00 Now I'm running Ubuntu 12.04 LTS (Precise) with openssl 1.0.1, and I think all the known renegotiation issues have been dealt with. I still get failures, but they are less informative: postgres@[unknown]:19761 2013-03-15 03:55:12 UTC LOG: SSL renegotiation failure -- Stuart Bishop stu...@stuartbishop.net http://www.stuartbishop.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] pgbench patches
Hello Tatsuo,
For me, the error message is not quite right, because progress == 0
case is considered error as well in your patch. I sugges you change
the error message something like:
thread progress delay (-P) must be positive number (%s)\n,
Please find attached a new version with an updated message.
--
Fabien.diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c
index 8dc81e5..23ee53c 100644
--- a/contrib/pgbench/pgbench.c
+++ b/contrib/pgbench/pgbench.c
@@ -74,7 +74,7 @@ static int pthread_join(pthread_t th, void **thread_return);
#include pthread.h
#else
/* Use emulation with fork. Rename pthread identifiers to avoid conflicts */
-
+#define PTHREAD_FORK_EMULATION
#include sys/wait.h
#define pthread_tpg_pthread_t
@@ -164,6 +164,8 @@ bool use_log; /* log transaction latencies to a file */
bool use_quiet; /* quiet logging onto stderr */
int agg_interval; /* log aggregates instead of individual
* transactions */
+int progress = 0; /* thread progress report every this seconds */
+int progress_nclients = 0; /* number of clients for progress report */
bool is_connect; /* establish connection for each transaction */
bool is_latencies; /* report per-command latencies */
int main_pid; /* main process id used in log filename */
@@ -352,6 +354,7 @@ usage(void)
(default: simple)\n
-n, --no-vacuum do not run VACUUM before tests\n
-N, --skip-some-updates skip updates of pgbench_tellers and pgbench_branches\n
+ -P, --progress NUM show thread progress report every NUM seconds\n
-r, --report-latencies report average latency per command\n
-s, --scale=NUM report this scale factor in output\n
-S, --select-onlyperform SELECT-only transactions\n
@@ -2119,6 +2122,7 @@ main(int argc, char **argv)
{log, no_argument, NULL, 'l'},
{no-vacuum, no_argument, NULL, 'n'},
{port, required_argument, NULL, 'p'},
+ {progress, required_argument, NULL, 'P'},
{protocol, required_argument, NULL, 'M'},
{quiet, no_argument, NULL, 'q'},
{report-latencies, no_argument, NULL, 'r'},
@@ -2202,7 +2206,7 @@ main(int argc, char **argv)
state = (CState *) pg_malloc(sizeof(CState));
memset(state, 0, sizeof(CState));
- while ((c = getopt_long(argc, argv, ih:nvp:dqSNc:j:Crs:t:T:U:lf:D:F:M:, long_options, optindex)) != -1)
+ while ((c = getopt_long(argc, argv, ih:nvp:dqSNc:j:Crs:t:T:U:lf:D:F:M:P:, long_options, optindex)) != -1)
{
switch (c)
{
@@ -2357,6 +2361,16 @@ main(int argc, char **argv)
exit(1);
}
break;
+ case 'P':
+progress = atoi(optarg);
+if (progress = 0)
+{
+ fprintf(stderr,
+ thread progress delay (-P) must be positive (%s)\n,
+ optarg);
+ exit(1);
+}
+break;
case 0:
/* This covers long options which take no argument. */
break;
@@ -2482,6 +2496,7 @@ main(int argc, char **argv)
* changed after fork.
*/
main_pid = (int) getpid();
+ progress_nclients = nclients;
if (nclients 1)
{
@@ -2733,6 +2748,11 @@ threadRun(void *arg)
int nstate = thread-nstate;
int remains = nstate; /* number of remaining clients */
int i;
+ /* for reporting progress: */
+ int64 thread_start = INSTR_TIME_GET_MICROSEC(thread-start_time);
+ int64 last_report = thread_start;
+ int64 next_report = last_report + progress * 100;
+ int64 last_count = 0;
AggVals aggs;
@@ -2896,6 +2916,68 @@ threadRun(void *arg)
st-con = NULL;
}
}
+
+#ifdef PTHREAD_FORK_EMULATION
+ /* each process reports its own progression */
+ if (progress)
+ {
+ instr_time now_time;
+ int64 now;
+ INSTR_TIME_SET_CURRENT(now_time);
+ now = INSTR_TIME_GET_MICROSEC(now_time);
+ if (now = next_report)
+ {
+/* generate and show report */
+int64 count = 0;
+int64 run = now - last_report;
+float tps, total_run, latency;
+
+for (i = 0 ; i nstate ; i++)
+ count += state[i].cnt;
+
+total_run = (now - thread_start) / 100.0;
+tps = 100.0 * (count - last_count) / run;
+latency = 1000.0 * nstate / tps;
+
+fprintf(stderr, progress %d: %.1f s, %.1f tps, %.3f ms lat\n,
+ thread-tid, total_run, tps, latency);
+
+last_count = count;
+last_report = now;
+next_report += progress * 100;
+ }
+ }
+#else
+ /* progress report by thread 0 for all threads */
+ if (progress thread-tid == 0)
+ {
+ instr_time now_time;
+ int64 now;
+ INSTR_TIME_SET_CURRENT(now_time);
+ now = INSTR_TIME_GET_MICROSEC(now_time);
+ if (now = next_report)
+ {
+/* generate and show report */
+int64 count = 0;
+int64 run = now - last_report;
+float tps, total_run, latency;
+
+for (i = 0 ; i progress_nclients ; i++)
+ count += state[i].cnt;
+
+total_run = (now - thread_start) / 100.0;
+tps = 100.0 * (count - last_count) / run;
+latency =
Re: [HACKERS] psql tab completion for updatable foreign tables
On 11 July 2013 00:03, Bernd Helmle maili...@oopsware.de wrote: --On 8. Juli 2013 16:04:31 + Dean Rasheed dean.a.rash...@gmail.com wrote: * pg_relation_is_updatable is only available in 9.3, whereas psql may connect to older servers, so it needs to guard against that. Oh of course, i forgot about this. Thanks for pointing out. * If we're doing this, I think we should do it for auto-updatable views at the same time. There was concern that pg_relation_is_updatable() would end up opening every relation in the database, hammering performance. I now realise that these tab-complete queries have a limit (1000 by default) so I don't think this is such an issue. I tested it by creating 10,000 randomly named auto-updatable views on top of a table, and didn't see any performance problems. Here's an updated patch based on the above points. Okay, are you adding this to the september commitfest? OK, I've done that. I think that it's too late for 9.3. Regards, Dean -- 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] big test separation POC
Hi Fabien, While applying latest version of the patch (regress-big-v4.patch) on latest PostgreSQL version i encountered following errors: a) Using git: $git apply --index regress-big-v4.patch regress-big-v4.patch:10: trailing whitespace. $(srcdir)/parallel_schedule $(srcdir)/parallel_big_schedule $(srcdir)/resultmap regress-big-v4.patch:18: trailing whitespace. # installcheck vs check: regress-big-v4.patch:19: trailing whitespace. # - whether test is run against installed or compiled version regress-big-v4.patch:20: trailing whitespace. # test schedules: parallel, parallel_big, standby regress-big-v4.patch:21: trailing whitespace. # serial schedules can be derived from parallel schedules fatal: git apply: bad git-diff - expected /dev/null on line 97 b) Using patch: $patch -d. -p1 regress-big-v4.patch (Stripping trailing CRs from patch.) patching file src/test/regress/GNUmakefile (Stripping trailing CRs from patch.) patching file src/test/regress/parallel_big_schedule (Stripping trailing CRs from patch.) patching file src/test/regress/serial_schedule Reversed (or previously applied) patch detected! Assume -R? [n] Is that a problem ? -- 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] big test separation POC
While applying latest version of the patch (regress-big-v4.patch) on latest PostgreSQL version i encountered following errors: [...] Is that a problem ? Yes and no:-) My understanding is that there is a conflict because of commits between this patch and head: a file that this patch deletes (it is derived by make rules) has been updated. It seems that git is not too good at detecting this and providing a meaningful message. Please find attached an updated version which seemed to work for me. Note that this is really a POC. How to derive a file is under discussion: it has been suggested that the unix shell approach would not work on Windows. I've suggested perl or python (which version?) but I'm not sure that it is okay either. -- Fabien.diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefile index d5935b6..8a39f7d 100644 --- a/src/test/regress/GNUmakefile +++ b/src/test/regress/GNUmakefile @@ -86,7 +86,7 @@ regress_data_files = \ $(wildcard $(srcdir)/output/*.source) \ $(filter-out $(addprefix $(srcdir)/,$(input_files)),$(wildcard $(srcdir)/sql/*.sql)) \ $(wildcard $(srcdir)/data/*.data) \ - $(srcdir)/parallel_schedule $(srcdir)/serial_schedule $(srcdir)/resultmap + $(srcdir)/parallel_schedule $(srcdir)/parallel_big_schedule $(srcdir)/resultmap install-tests: all install install-lib installdirs-tests $(MAKE) -C $(top_builddir)/contrib/spi install @@ -137,19 +137,43 @@ tablespace-setup: ## Run tests ## +# installcheck vs check: +# - whether test is run against installed or compiled version +# test schedules: parallel, parallel_big, standby +# serial schedules can be derived from parallel schedules + +derived_schedules = serial_schedule serial_big_schedule + +serial_%: parallel_% + echo # this file is generated automatically, do not edit! $@ + egrep '^(test|ignore):' $ | \ + while read op list ; do \ + for test in $$list ; do \ + echo $$op $$test ; \ + done ; \ + done $@ + REGRESS_OPTS = --dlpath=. $(EXTRA_REGRESS_OPTS) +# before installation, parallel check: all tablespace-setup - $(pg_regress_check) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(TEMP_CONF) $(EXTRA_TESTS) + $(pg_regress_check) $(REGRESS_OPTS) $(MAXCONNOPT) $(TEMP_CONF) \ + --schedule=$(srcdir)/parallel_schedule $(EXTRA_TESTS) -installcheck: all tablespace-setup - $(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/serial_schedule $(EXTRA_TESTS) +# after installation, serial +installcheck: all tablespace-setup serial_schedule + $(pg_regress_installcheck) $(REGRESS_OPTS) \ + --schedule=serial_schedule $(EXTRA_TESTS) +# after installation, parallel installcheck-parallel: all tablespace-setup - $(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS) + $(pg_regress_installcheck) $(REGRESS_OPTS) $(MAXCONNOPT) \ + --schedule=$(srcdir)/parallel_schedule $(EXTRA_TESTS) +# after installation standbycheck: all - $(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/standby_schedule --use-existing + $(pg_regress_installcheck) $(REGRESS_OPTS) \ + --schedule=$(srcdir)/standby_schedule --use-existing # old interfaces follow... @@ -157,11 +181,19 @@ runcheck: check runtest: installcheck runtest-parallel: installcheck-parallel -bigtest: all tablespace-setup - $(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/serial_schedule numeric_big +bigtest: installcheck-big + +# test = after installation, serial +installcheck-big: all tablespace-setup serial_schedule serial_big_schedule + $(pg_regress_installcheck) $(REGRESS_OPTS) \ + --schedule=serial_schedule \ + --schedule=serial_big_schedule +# check = before installation, parallel bigcheck: all tablespace-setup - $(pg_regress_check) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) numeric_big + $(pg_regress_check) $(REGRESS_OPTS) $(MAXCONNOPT) \ + --schedule=$(srcdir)/parallel_schedule \ + --schedule=$(srcdir)/parallel_big_schedule ## @@ -173,6 +205,6 @@ clean distclean maintainer-clean: clean-lib rm -f $(OBJS) refint$(DLSUFFIX) autoinc$(DLSUFFIX) dummy_seclabel$(DLSUFFIX) rm -f pg_regress_main.o pg_regress.o pg_regress$(X) # things created by various check targets - rm -f $(output_files) $(input_files) + rm -f $(output_files) $(input_files) $(derived_schedules) rm -rf testtablespace rm -rf $(pg_regress_clean_files) diff --git a/src/test/regress/parallel_big_schedule b/src/test/regress/parallel_big_schedule new file mode 100644 index 000..9434abf --- /dev/null +++ b/src/test/regress/parallel_big_schedule @@ -0,0 +1 @@ +test: numeric_big diff --git a/src/test/regress/serial_schedule b/src/test/regress/serial_schedule deleted file mode 100644 index 3ad289f..000 --- a/src/test/regress/serial_schedule +++ /dev/null @@ -1,141 +0,0 @@ -# src/test/regress/serial_schedule -# This should probably be in an order similar to parallel_schedule. -test: tablespace -test:
[HACKERS] mvcc catalo gsnapshots and TopTransactionContext
Hi, since the mvcc catalog patch has gone in we require all users of systable_* to be in a valid transaction since the snapshot is copied via CopySnapshot() in RegisterSnapshot(). Which we call in systable_beginscan(). CopySnapshot() allocates the copied snapshot in TopTransactionContext. There doesn't seem be an explicitly stated rule that we cannot use the syscaches outside of a transaction - but effectively that's required atm. Not having investigated at all so far I am not sure how much in core code that breaks, but it certainly broke some out of tree code of mine (bidirectional replication stuff, bdr branch on git.pg.o). Is that acceptable? Greetings, Andres Freund -- Andres Freund 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] how to find out whether a view is updatable
On 19 June 2013 18:12, Peter Eisentraut pete...@gmx.net wrote: On 6/19/13 11:50 AM, Dean Rasheed wrote: On 19 June 2013 15:22, Peter Eisentraut pete...@gmx.net wrote: We still don't have any support for this in psql, do we? No, but at least we now have an API that psql can use. There are still a number of questions about the best way to display it in psql. Should it be another column in \d+'s list of relations? Should it appear in \d+ for a single relation? Should it distinguish updatable from insertable and deletable? Should tab-completion also be modified? Currently I'm thinking yes, yes, no, yes. I would be satisfied with no, yes, no, no. Although I don't know what tab completion changes you have in mind. Here's a patch that does that for foreign tables and views. It regards updatable as support for *any* of the DML operations. Bernd Helmle has written a patch for tab completion. Regards, Dean psql-describe.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] docbook-xsl version for release builds
On Wed, Jul 10, 2013 at 2:39 AM, Peter Eisentraut pete...@gmx.net wrote: I would like to start using a newer version of docbook-xsl for the release builds. This is currently used for building the man pages. The latest release is 1.78.1 and fixes a few formatting errors. How do we do that? We could just take the latest Debian package and stick it on borka. I don't have a problem with this also targeting maintenance releases, but maybe there are other ideas. If it's safe to switch on the old ones as well, it sounds doable. If we need different toolchains, that's going to be a serious pain. Have you verified that it's fine with the old ones as well, or are you jsut assuming? Second, when you say the latest debian package, you mean grab the one from sid? I didn't see anything in backports, but maybe I'm missing something? -- 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] Improvement of checkpoint IO scheduler for stable transaction responses
Hi,l I create fsync v3 v4 v5 patches and test them. * Changes - Add considering about total checkpoint schedule in fsync phase (v3 v4 v5) - Add considering about total checkpoint schedule in write phase (v4 only) - Modify some implementations from v3 (v5 only) I use linear combination method for considering about total checkpoint schedule which are write phase and fsync phase. V3 patch was considered about only fsync phase, V4 patch was considered about write phase and fsync phase, and v5 patch was considered about only fsync phase. Test result is here. Benchmark setting and server are same as previous test. '-*' shows checkpoint_completion_target in each tests. And all tests which are except 'fsync v3_disabled' set 'checkpointer_fsync_delay_ratio=1' and 'checkpointer_fsync_delay_threshold=1000'. 'fsync v3_disabled' set 'checkpointer_fsync_delay_ratio=0' and 'checkpointer_fsync_delay_threshold= -1'. V5 patch is testing now:-), but it will be same score as v3 patch. * Result ** DBT-2 result | NOTPM | 90%tile | Average | S.Deviation | Maximum -+---+-+-+-+ fsync v3-0.7 | 3649.02 | 9.703 | 4.226 | 3.853 | 21.754 fsync v3-0.9 | 3694.41 | 9.897 | 3.874 | 4.016 | 20.774 fsync v3-0.7_disabled| 3583.28 | 10.966 | 4.684 | 4.866 | 31.545 fsync v4-0.7 | 3546.38 | 12.734 | 5.062 | 4.798 | 24.468 fsync v4-0.9 | 3670.81 | 9.864 | 4.130 | 3.665 | 19.236 ** Average checkpoint duration (sec) (Not include during loading time) | write_duration | sync_duration | total | punctual to checkpoint schedule -++---++ fsync v3-0.7 | 296.6 | 251.8898 | 548.48 | OK fsync v3-0.9 | 292.086| 276.4525 | 568.53 | OK fsync v3-0.7_disabled| 303.5706 | 155.6116 | 459.18 | OK fsync v4-0.7 | 273.8338 | 355.6224 | 629.45 | OK fsync v4-0.9 | 329.0522 | 231.77| 560.82 | OK ** Increase of checkpoint duration (%) (Reference point is 'fsync v3-0.7_disabled'.) | write_duration | sync_duration | total -++---+--- fsync v3-0.7 | 97.7% | 161.9%| 119.4% fsync v3-0.9 | 96.2% | 177.7%| 123.8% fsync v3-0.7_disabled| 100.0% | 100.0%| 100.0% fsync v4-0.7 | 90.2% | 228.5%| 137.1% fsync v4-0.9 | 108.4% | 148.9%| 122.1% * Examination ** DBT-2 result V3 patch seems good result which is be faster response time about 10%-30% and inclease NOTPM about 5% than no sleep(fsync v3-0.7_disabled), and v4 patch is not good result. However, 'fsync v4-0.9' is same score as v3 patch when more large checkpoint_completion_target. I think that considering about checkpoint schedule about write phase and fsync phase makes more harsh in IO schedule. Because write phase IO schedule is more strict than normal write phase. And it is also bad in fsync phase and concern latter. ** Average checkpoint duration All methods are punctual to checkpoint schedule. In enabling fsync sleep, it is longer fsync time, however total time are much the same as no sleep. 'fsync v4-0.7 ' becomes very bad sync duration and total time. It indicates that changing checkpoint_completion_target is very delicate. It had not better change write phase scheduling, the same as it used to. At write phase in normal setting , it have sufficiently time for punctual to checkpoint schedule. And I think that many user want to be compatible with old version. What do you think about these patches? Best regards, -- Mitsumasa KONDO NTT Open Source Software Center diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c index fdf6625..d09fe4f 100644 --- a/src/backend/postmaster/checkpointer.c +++ b/src/backend/postmaster/checkpointer.c @@ -143,14 +143,16 @@ static CheckpointerShmemStruct *CheckpointerShmem; */ int CheckPointTimeout = 300; int CheckPointWarning = 30; +int CheckPointerFsyncDelayThreshold = -1; double CheckPointCompletionTarget = 0.5; +double CheckPointerFsyncDelayRatio = 0.0; /* * Flags set by interrupt handlers for later service in the main loop. */ static volatile sig_atomic_t got_SIGHUP = false; -static volatile sig_atomic_t checkpoint_requested = false; -static volatile sig_atomic_t shutdown_requested = false; +extern volatile sig_atomic_t checkpoint_requested = false; +extern volatile sig_atomic_t shutdown_requested = false; /* * Private state @@ -168,8 +170,6 @@ static pg_time_t last_xlog_switch_time; /* Prototypes for private functions */ static void CheckArchiveTimeout(void); -static bool IsCheckpointOnSchedule(double progress); -static bool
Re: [HACKERS] robots.txt on git.postgresql.org
On Wed, Jul 10, 2013 at 9:36 AM, Magnus Hagander mag...@hagander.net wrote: We already run this, that's what we did to make it survive at all. The problem is there are so many thousands of different URLs you can get to on that site, and google indexes them all by default. There's also https://support.google.com/webmasters/answer/48620?hl=en which lets us control how fast the Google crawler crawls. I think it's adaptive though so if the pages are slow it should be crawling slowly -- greg -- 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] robots.txt on git.postgresql.org
On 2013-07-11 14:43:21 +0100, Greg Stark wrote: On Wed, Jul 10, 2013 at 9:36 AM, Magnus Hagander mag...@hagander.net wrote: We already run this, that's what we did to make it survive at all. The problem is there are so many thousands of different URLs you can get to on that site, and google indexes them all by default. There's also https://support.google.com/webmasters/answer/48620?hl=en which lets us control how fast the Google crawler crawls. I think it's adaptive though so if the pages are slow it should be crawling slowly The problem is that gitweb gives you access to more than a million pages... Revisions: git rev-list --all origin/master|wc -l = 77123 Branches: git branch --all|grep origin|wc - Views per commit: commit, commitdiff, tree So, slow crawling isn't going to help very much. Greetings, Andres Freund -- Andres Freund 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] robots.txt on git.postgresql.org
On Thu, Jul 11, 2013 at 3:43 PM, Greg Stark st...@mit.edu wrote: On Wed, Jul 10, 2013 at 9:36 AM, Magnus Hagander mag...@hagander.net wrote: We already run this, that's what we did to make it survive at all. The problem is there are so many thousands of different URLs you can get to on that site, and google indexes them all by default. There's also https://support.google.com/webmasters/answer/48620?hl=en which lets us control how fast the Google crawler crawls. I think it's adaptive though so if the pages are slow it should be crawling slowly Sure, but there are plenty of other search engines as well, not just google... Google is actually reasonably good at scaling back it's own speed, in my experience. Which is not true of all the others. Of course, it's also got the problem of it then taking a long time to actually crawl the site, since there are so many different URLs... -- 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] SSL renegotiation
On Thu, Jul 11, 2013 at 1:13 AM, Sean Chittenden s...@chittenden.org wrote: , I suppose two things can be done: 1. Quit the connection With my Infosec hat on, this is the correct option - force the client back in to compliance with whatever the stated crypto policy is through a reconnection. 2. Carry on pretending nothing happened. This is almost never correct in a security context (all errors or abnormalities must boil up). I think 2 is correct in the vast majority of cases (as it looks like is being done now). That is a correct statement in that most code disregards renegotiation, but that is because there is a pragmatic assumption that HTTPS connections will be short lived. In the case of PostgreSQL, there is a good chance that a connection will be established for weeks or months. In the case of Apache, allowing a client to renegotiate every byte would be a possible CPU DoS, but I digress And, allowing the client to refuse to renegotiate leaves the relevant vulnerability unpatched. Renegotiation was introduced to patch a vulnerability in which, without renegotiation, there was the possibility of an attacker gaining knowledge of session keys (and hence the ability to intercept the stream). I think 2 is not viable in this context. Only 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] Patch for fail-back without fresh backup
On Tue, Jul 9, 2013 at 11:45 PM, Sawada Masahiko sawada.m...@gmail.com wrote: On Sun, Jul 7, 2013 at 4:27 PM, Sawada Masahiko sawada.m...@gmail.com wrote: I found a bug which occurred when we do vacuum, and have fixed it. yesterday (8th July) Improve scalability of WAL insertions patch is committed to HEAD. so v2 patch does not apply to HEAD now. I also have fixed it to be applicable to HEAD. please find the attached patch. Regards, --- Sawada Masahiko I have fixed that master server doesn't waits for the WAL to be flushed to disk of standby when master server execute FlushBuffer(), and have attached v4 patch. please find the attached patch. Regards, --- Sawada Masahiko failback_safe_standby_v4.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls
I've attached a revised version that fixes the issues above: changing a reference of the form: OVER w into: OVER (w) Fixed (and I've updated the tests). It's bad form to modify a list while iterating through it. Fixed We shouldn't create an arbitrary number of duplicate windows Fixed Is there a problem with having two windowdefs in the p_windowdefs list with the same name ... You'll have to be a little careful that any other code knows that names can be duplicated in the list though. I'm not sure I really can verify this - as I'm not sure how much contrib / other third-party code has access to this data structure. I'd prefer to be cautious and just create a child window if needed. I think we should get rid of the bitmapset entirely ... Instead of the bitmapset, we can keep track of two offsets I've modified leadlag_common so it uses your suggested algorithm for constant offsets (although it turns out you only need to keep a single int64 index in the context). This algorithm calls WinGetFuncArgInPartition at least twice per row, once to check whether the current row is null (and so check if we have to move the leading / lagged index forward) and either once to get leading / lagging value or more than once to push the leading / lagged value forwards to the next non-null value. I've kept the bitmap solution for the non-constant offset case (i.e. the random partition access case) as I believe it changes the cost of calculating the lead / lagged values for every row in the partition to O(partition size) - whereas a non-caching scan-the-partition solution would be O(partition size * partition size). Is that OK? Thanks - Nick lead-lag-ignore-nulls.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: patch submission: truncate trailing nulls from heap rows to reduce the size of the null bitmap [Review]
On Thu, Jun 27, 2013 at 10:12 PM, Josh Berkus j...@agliodbs.com wrote: Yeah, I think that's be bigger question. Ok, I'll start working on a new test case. Here's my thinking on performance tests: 1. run pgbench 10 times both with and without the patch. See if there's any measurable difference. There should not be. I don't even see the point in extensive empirical results. Empirical results are somewhat useful for measuring the cpu cycle cost when the optimization doesn't give any benefit. That should be one fairly simple test and my understanding is that it's been done and shown no significant cost. When the optimization does kick in it saves space. Saving space is something we can calculate the effect precisely of and don't need empirical tests to validate. I would still want to check that it actually works as expected of course but that's a matter of measuring the row sizes, not timing lengthy pgbench runs. Neither of these address Tom's concerns about API changes and future flexibility. I was assigned this patch in the rreviewers list and my inclination would be to take it but I wasn't about to overrule Tom. If he says he's ok with it then I'm fine going ahead and reviewing the code. If I still have commit bits I could even commit it. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Patch for reserved connections for replication users
Hi,
this patch introduces a new configuration flag
replication_reserved_connections to reserve connection slots for
replication in the same way superuser_reserved_connections works for
superusers.
This helps in cases where the application opens connections until
max_connections is reached. A slave would not be able to connect to the
master now and would just be down. With this patch the slave is able to
connect.
This option does not influence the superuser_reserved_connections, so
new slaves are not able to open new connections when the reserved
replication slots are filled.
The only thing this patch is missing are tests. Where should I put them
into the source tree?
thank you,
Stefan Radomskidiff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
new file mode 100644
index 6a4d15f..cd6264e
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
*** include 'filename'
*** 530,535
--- 530,562
/listitem
/varlistentry
+ varlistentry id=guc-replication_reserved_connections
+ xreflabel=replication_reserved_connections
+ termvarnamereplication_reserved_connections/varname/term
+ (typeinteger/type)/term
+ indexterm
+primaryvarnamereplication_reserved_connections/ configuration parameter/primary
+ /indexterm
+ listitem
+para
+ Determines the number of connection quoteslots/quote that
+ are reserved for connections by productnamePostgreSQL/
+ replication users. At most xref linkend=guc-max-connections
+ connections can ever be active simultaneously. Whenever the
+ number of active concurrent connections is at least
+ varnamemax_connections/ minus
+ varnamesuperuser_reserved_connections/varname minus
+ varnamereplication_reserved_connections/varname, new
+ connections will be accepted only for replication and superusers.
+/para
+
+para
+ The default value is zero connections. The value must be less
+ than the value of varnamemax_connections/varname. This
+ parameter can only be set at server start.
+/para
+ /varlistentry
+
varlistentry id=guc-unix-socket-directories xreflabel=unix_socket_directories
termvarnameunix_socket_directories/varname (typestring/type)/term
indexterm
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
new file mode 100644
index 496192d..ce37b0d
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
*** char *ListenAddresses;
*** 224,229
--- 224,238
* count against the limit.
*/
int ReservedBackends;
+ /*
+ * ReservedReplicationBackends is the number of backends reserved for
+ * replication user use. Like ReservedBackends the number will be taken out
+ * of the pool size given by MaxBackends so the number available to
+ * non-superusers is
+ * (MaxBackends - ReservedBackends - ReservedReplicationBackends).
+ * This option does not block superusers.
+ */
+ int ReservedReplicationBackends;
/* The socket(s) we're listening to. */
#define MAXLISTEN 64
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
new file mode 100644
index 2c7f0f1..d0f8f91
*** a/src/backend/utils/init/postinit.c
--- b/src/backend/utils/init/postinit.c
*** InitPostgres(const char *in_dbname, Oid
*** 700,707
}
/*
! * The last few connections slots are reserved for superusers. Although
! * replication connections currently require superuser privileges, we
* don't allow them to consume the reserved slots, which are intended for
* interactive use.
*/
--- 700,716
}
/*
! * The last few connections slots are reserved for superusers and replication.
! */
! if (!am_superuser
! (ReservedReplicationBackends 0 || ReservedBackends 0)
! !HaveNFreeProcs(ReservedReplicationBackends + ReservedBackends))
! ereport(FATAL,
! (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
! errmsg(remaining connection slots are reserved for replication and superuser connections)));
!
! /*
! * Although replication connections currently require superuser privileges, we
* don't allow them to consume the reserved slots, which are intended for
* interactive use.
*/
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
new file mode 100644
index 5aefd1b..b03d722
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*** static struct config_int ConfigureNamesI
*** 1633,1638
--- 1633,1647
3, 0, MAX_BACKENDS,
NULL, NULL, NULL
},
+ {
+ {replication_reserved_connections, PGC_POSTMASTER, CONN_AUTH_SETTINGS,
+ gettext_noop(Sets the number of connection slots reserved for replication.),
+ NULL
+ },
+ ReservedReplicationBackends,
+ 1, 0, MAX_BACKENDS,
+ NULL, NULL, NULL
+ },
/*
* We
Re: [HACKERS] [PATCH] big test separation POC
Fabien COELHO escribió: Note that this is really a POC. How to derive a file is under discussion: it has been suggested that the unix shell approach would not work on Windows. I've suggested perl or python (which version?) but I'm not sure that it is okay either. The other option, suggested by Andres somewhere, is to have a new parameter to pg_regress, something like --run-serially. So you would use the same parallel schedule file, but serially instead of following the parallel specification. -- Álvaro Herrerahttp://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] Regex pattern with shorter back reference does NOT work as expected
Jeevan Chalke jeevan.cha...@enterprisedb.com writes: Following example does not work as expected: -- Should return TRUE but returning FALSE SELECT 'Programmer' ~ '(\w).*?\1' as t; This is clearly broken, but I'm uncomfortable with the proposed patch. As written, it changes behavior for both the shortest-match-preferred and longest-match-preferred cases; but you've offered no evidence that the longest-match case is broken. Maybe it is --- it's sure not obvious why it's okay to abandon the search early in this case. But I think we'd have been likely to hear about it before now if there were a matching failure in that path, since longest-preferred is so much more widely used. I think we should either convince ourselves that the longest-preferred case is also broken (preferably with a test case), or understand why it isn't. Such understanding would probably also teach us how to fix the shortest-preferred case in a way that doesn't give up early search exit. 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] big test separation POC
On 07/11/2013 09:19 AM, Alvaro Herrera wrote: Fabien COELHO escribió: Note that this is really a POC. How to derive a file is under discussion: it has been suggested that the unix shell approach would not work on Windows. I've suggested perl or python (which version?) but I'm not sure that it is okay either. The other option, suggested by Andres somewhere, is to have a new parameter to pg_regress, something like --run-serially. So you would use the same parallel schedule file, but serially instead of following the parallel specification. Ok, this sounds like it needs a *lot* of discussion before it's a patch. Marking returned with feedback, and we'll discuss it until September (or beyond). -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
On Fri, Jul 5, 2013 at 2:19 PM, Amit Kapila amit.kap...@huawei.com wrote: On Wednesday, July 03, 2013 2:58 AM Alvaro Herrera wrote: Amit Kapila escribió: I have changed the file name to postgresql.auto.conf and I have changed the name of SetPersistentLock to AutoFileLock. Zoltan, Could you please once check the attached Patch if you have time, as now all the things are resolved except for small doubt in syntax extensibility which can be changed based on final decision. There are a bunch of whitespace problems, as you would notice with git diff --check or git show --color. Could you please clean that up? Fixed all whitespaces. Also, some of the indentation in various places is not right; perhaps you could fix that by running pgindent over the source files you modified (this is easily visible by eyeballing the git diff output; stuff is misaligned in pretty obvious ways.) Fixed, by running pgindent Random minor other comments: + use of xref linkend=SQL-ALTER SYSTEM this SGML link cannot possibly work. Please run make check in the doc/src/sgml directory. Fixed, make check passes now. Examples in alter_system.sgml have a typo SYTEM. Same file has or or. Fixed. Also in that file, The name of an configuration parameter that exist in filenamepostgresql.conf/filename. the parameter needn't exist in that file to be settable, right? Changed to below text: Name of a settable run-time parameter. Available parameters are documented in xref linkend=runtime-config. refnamediv refnameALTER SYSTEM/refname refpurposechange a server configuration parameter/refpurpose /refnamediv Perhaps permanently change ..? Not changed. Also, some parameters require a restart, not a reload; maybe we should direct the user somewhere else to learn how to freshen up the configuration after calling the command. + /* skip auto conf temporary file */ + if (strncmp(de-d_name, + PG_AUTOCONF_FILENAME ., + sizeof(PG_AUTOCONF_FILENAME)) == 0) + continue; What's the dot doing there? Fixed, removed dot. + if ((stat(AutoConfFileName, st) == -1)) + ereport(elevel, + (errmsg(configuration parameters changed with ALTER SYSTEM command before restart of server + will not be effective as \%s\ file is not accessible., PG_AUTOCONF_FILENAME))); + else + ereport(elevel, + (errmsg(configuration parameters changed with ALTER SYSTEM command before restart of server + will not be effective as default include directive for \%s\ folder is not present in postgresql.conf, PG_AUTOCONF_DIR))); These messages should be split. Perhaps have the will not be effective in the errmsg() and the reason as part of errdetail()? Okay, changed as per suggestion. Also, I'm not really sure about doing another stat() on the file after parsing failed; I think the parse routine should fill some failure context information, instead of having this code trying to reproduce the failure to know what to report. Maybe something like the SlruErrorCause enum, not sure. Similarly, + /* + * record if the file currently being parsed is postgresql.auto.conf, + * so that it can be later used to give warning if it doesn't parse + * it. +*/ + snprintf(Filename,sizeof(Filename),%s/%s, PG_AUTOCONF_DIR, PG_AUTOCONF_FILENAME); + ConfigAutoFileName = AbsoluteConfigLocation(Filename, ConfigFileName); + if (depth == 1 strcmp(ConfigAutoFileName, config_file) == 0) + *is_config_dir_parsed = true; this seems very odd. The whole is_config_dir_parsed mechanism smells strange to me, really. I think the caller should be in charge of keeping track of this, but I'm not sure. ParseConfigFp() would have no way of knowing, and in one place we're calling that routine precisely to parse the auto file. Changed by adding new enum AutoConfErrorCause. Now is_config_dir_parsed is removed from code. Kindly let me know if this way is better than previous? Apart from above I have fixed one issue in function AlterSystemSetConfigFile(), called FreeConfigVariables(). I got the following compile warnings. guc.c:5187: warning: no previous prototype for 'validate_conf_option' preproc.y:7746.2-31: warning: type clash on default action: str != The make installcheck failed when I ran it against the server with wal_level = hot_standby. The regression.diff is *** 27,35 (1 row) show wal_level; ! wal_level ! --- ! minimal (1 row) show authentication_timeout; --- 27,35 (1 row) show wal_level; ! wal_level ! - ! hot_standby (1 row) show authentication_timeout; The regression test of ALTER SYSTEM calls pg_sleep(1) six times. Those who dislike the regression
Re: [HACKERS] docbook-xsl version for release builds
On 7/11/13 5:55 AM, Magnus Hagander wrote: If it's safe to switch on the old ones as well, it sounds doable. If we need different toolchains, that's going to be a serious pain. Have you verified that it's fine with the old ones as well, or are you jsut assuming? I tested it and it's fine. Second, when you say the latest debian package, you mean grab the one from sid? I didn't see anything in backports, but maybe I'm missing something? Yes, take the sid package. There likely won't be a backport because it's just plain text files in the package. -- 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] big test separation POC
The other option, suggested by Andres somewhere, is to have a new parameter to pg_regress, something like --run-serially. After looking at the source, ISTM that this option already exists under a different signature: --max-connections 1 So you would use the same parallel schedule file, but serially instead of following the parallel specification. Yep. And there is nothing to do, which is even better:-) -- Fabien. -- 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] mvcc catalo gsnapshots and TopTransactionContext
On Thu, 2013-07-11 at 10:28 +0200, Andres Freund wrote: There doesn't seem be an explicitly stated rule that we cannot use the syscaches outside of a transaction - but effectively that's required atm. Aren't there other things that already required that before the MVCC catalog snapshot patch went in? For instance, if you get a syscache miss, you have to load from the catalogs, meaning you need to acquire a lock. I've seen problems related to that in the past: http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=ac63dca607e8e22247defbc8fe03b6baa3628c42 Regards, Jeff Davis -- 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] mvcc catalo gsnapshots and TopTransactionContext
Andres Freund and...@2ndquadrant.com writes: since the mvcc catalog patch has gone in we require all users of systable_* to be in a valid transaction since the snapshot is copied via CopySnapshot() in RegisterSnapshot(). It never has been, and never will be, allowed to call the catcache code without being in a transaction. What do you think will happen if the requested row isn't in cache? A table access, that's what, and that absolutely requires being in a transaction. 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] Support for REINDEX CONCURRENTLY
On Thu, Jul 11, 2013 at 5:11 PM, Michael Paquier michael.paqu...@gmail.com wrote: I am resending the patches after Fujii-san noticed a bug allowing to even drop valid toast indexes with the latest code... While looking at that, I found a couple of other bugs: - two bugs, now fixed, with the code path added in tablecmds.c to allow the manual drop of invalid toast indexes: -- Even a user having no permission on the parent toast table could drop an invalid toast index -- A lock on the parent toast relation was not taken as it is the case for all the indexes dropped with DROP INDEX - Trying to reindex concurrently a mapped catalog leads to an error. As they have no relfilenode, I think it makes sense to block reindex concurrently in this case, so I modified the core patch in this sense. This patch status has been changed to returned with feedback. -- Michael -- 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] [PERFORM] In progress INSERT wrecks plans on table
On 07/10/2013 10:09 PM, Abhijit Menon-Sen wrote: At 2013-07-10 09:47:34 -0700, j...@agliodbs.com wrote: Due to the apparent lack of performance testing, I'm setting this back to needs review. The original submission (i.e. the message linked from the CF page) includes test results that showed a clear performance improvement. Here's an excerpt: I didn't see that, and nobody replied to my email. So, where are we with this patch, then? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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] Preventing tuple-table leakage in plpgsql
It looks like to me when AtEOSubXact_SPI is called the _SPI_current-connectSubId is always 1 (since it is only set when SPI_connect is called, which is only once for plpgsql), but the CurrentSubTransactionId is incremented each time a subtransaction is started. As a result, the memory for procCxt is only freed when I presume the TopTransaction is aborted or committed. Should SPI_connect be called again after the subtransaction is created? And SPI_finish before the subtransaction is committed or aborted? On Thu, Jul 11, 2013 at 8:46 PM, Chad Wagner chad.wag...@gmail.com wrote: It looks like to me exec_stmt_block creates a subtransaction if the block has an exception handler by calling BeginInternalSubTransaction. Then inside the PG_TRY it calls exec_stmts which runs the actual body of the begin block. If an exception is thrown then I presume we are hitting the PG_CATCH block where it calls RollbackAndReleaseCurrentSubTransaction. Inside RollbackAndReleaseCurrentSubTransaction it calls AtEOSubXact_SPI which frees the procCxt where the tuptabcxt is allocated. Looking at it seems to suggest that the memory allocated under tuptabcxt should be freed when we abort the subtransaction? Or did I miss something here? BTW, hacking pl_exec and moving the tubtabcxt under CurTransactionContext does also seem to resolve the memory issue. Which suggests that somewhere along the way AtEOSubXact_SPI is not called when the subtransaction is aborted by the catch block, that or I got lost in the code. On Fri, Jul 5, 2013 at 6:39 PM, Noah Misch n...@leadboat.com wrote: On Fri, Jul 05, 2013 at 02:47:06PM -0400, Tom Lane wrote: Bug #8279 exhibits an intra-transaction memory leak in a plpgsql function that repeatedly traps an error. The cause of the leak is that the SPITupleTable created during exec_stmt_execsql is never cleaned up. (It will get cleaned up at function exit, but that's not soon enough in this usage.) One approach we could take is to insert PG_TRY blocks to ensure that SPI_freetuptable is called on error exit from such functions. (plpython seems to have adopted this solution already.) However, that tends to be pretty messy notationally, and possibly could represent a noticeable performance hit depending on what you assume about the speed of sigsetjmp(). Moreover, fixing known trouble spots this way does nothing to guard against similar errors elsewhere. I, too, find that strategy worth avoiding as long as practical. So I'm inclined to propose that SPI itself should offer some mechanism for cleaning up tuple tables at subtransaction abort. We could just have it automatically throw away tuple tables made in the current subtransaction, or we could allow callers to exercise some control, perhaps by calling a function that says don't reclaim this tuple table automatically. I'm not sure if there's any real use-case for such a call though. I suppose that would be as simple as making spi_dest_startup() put the tuptabcxt under CurTransactionContext? The function to prevent reclamation would then just do a MemoryContextSetParent(). Is there good reason to believe that SPI tuptables are the main interesting PL/pgSQL allocation to make a point of freeing promptly, error or no error? I wonder if larger sections of pl_exec.c could run under CurTransactionContext. It's also not very clear to me if tuple tables should be thrown away automatically at subtransaction commit. We could do that, or leave things alone, or add some logic to emit warning bleats about unreleased tuple tables (comparable to what is done for many other resource types). If we change anything about the commit case, I think we run significant risk of breaking third-party code that works now, so maybe it's best to leave that alone. That's not clear to me, either. The safe thing would be to leave the default unchanged but expose an API to override the tuptable parent context. Initially, only PL/pgSQL would use it. It might also be worth debating whether to back-patch such a fix. This issue has been there ever since plpgsql grew the ability to trap errors, so the lack of previous reports might mean that it's not worth taking risks to fix such leaks in back branches. I agree that could go either way; let's see what the patch looks like. Thanks, nm -- Noah Misch EnterpriseDB http://www.enterprisedb.com
Re: [HACKERS] Preventing tuple-table leakage in plpgsql
It looks like to me exec_stmt_block creates a subtransaction if the block has an exception handler by calling BeginInternalSubTransaction. Then inside the PG_TRY it calls exec_stmts which runs the actual body of the begin block. If an exception is thrown then I presume we are hitting the PG_CATCH block where it calls RollbackAndReleaseCurrentSubTransaction. Inside RollbackAndReleaseCurrentSubTransaction it calls AtEOSubXact_SPI which frees the procCxt where the tuptabcxt is allocated. Looking at it seems to suggest that the memory allocated under tuptabcxt should be freed when we abort the subtransaction? Or did I miss something here? BTW, hacking pl_exec and moving the tubtabcxt under CurTransactionContext does also seem to resolve the memory issue. Which suggests that somewhere along the way AtEOSubXact_SPI is not called when the subtransaction is aborted by the catch block, that or I got lost in the code. On Fri, Jul 5, 2013 at 6:39 PM, Noah Misch n...@leadboat.com wrote: On Fri, Jul 05, 2013 at 02:47:06PM -0400, Tom Lane wrote: Bug #8279 exhibits an intra-transaction memory leak in a plpgsql function that repeatedly traps an error. The cause of the leak is that the SPITupleTable created during exec_stmt_execsql is never cleaned up. (It will get cleaned up at function exit, but that's not soon enough in this usage.) One approach we could take is to insert PG_TRY blocks to ensure that SPI_freetuptable is called on error exit from such functions. (plpython seems to have adopted this solution already.) However, that tends to be pretty messy notationally, and possibly could represent a noticeable performance hit depending on what you assume about the speed of sigsetjmp(). Moreover, fixing known trouble spots this way does nothing to guard against similar errors elsewhere. I, too, find that strategy worth avoiding as long as practical. So I'm inclined to propose that SPI itself should offer some mechanism for cleaning up tuple tables at subtransaction abort. We could just have it automatically throw away tuple tables made in the current subtransaction, or we could allow callers to exercise some control, perhaps by calling a function that says don't reclaim this tuple table automatically. I'm not sure if there's any real use-case for such a call though. I suppose that would be as simple as making spi_dest_startup() put the tuptabcxt under CurTransactionContext? The function to prevent reclamation would then just do a MemoryContextSetParent(). Is there good reason to believe that SPI tuptables are the main interesting PL/pgSQL allocation to make a point of freeing promptly, error or no error? I wonder if larger sections of pl_exec.c could run under CurTransactionContext. It's also not very clear to me if tuple tables should be thrown away automatically at subtransaction commit. We could do that, or leave things alone, or add some logic to emit warning bleats about unreleased tuple tables (comparable to what is done for many other resource types). If we change anything about the commit case, I think we run significant risk of breaking third-party code that works now, so maybe it's best to leave that alone. That's not clear to me, either. The safe thing would be to leave the default unchanged but expose an API to override the tuptable parent context. Initially, only PL/pgSQL would use it. It might also be worth debating whether to back-patch such a fix. This issue has been there ever since plpgsql grew the ability to trap errors, so the lack of previous reports might mean that it's not worth taking risks to fix such leaks in back branches. I agree that could go either way; let's see what the patch looks like. Thanks, nm -- Noah Misch EnterpriseDB http://www.enterprisedb.com
