Re: [HACKERS] Strange Windows problem, lock_timeout test request
2013-03-17 04:48 keltezéssel, Tom Lane írta: Boszormenyi Zoltan z...@cybertec.at writes: [ 2-lock_timeout-v37.patch ] Applied after a fair amount of additional hacking. Thank you, thank you, thank you! :-) I was disappointed to find that the patch introduced a new race condition into timeout.c, or at least broke a safety factor that had been there. The argument why enable_timeout() could skip disabling the timer interrupt on entry, if num_active_timeouts is zero, doesn't work for enable_timeouts(): as soon as you've incremented num_active_timeouts for the first new timeout, the signal handler would mess around with the data structure if it were to fire. What I did about that was to modify disable_alarm() to forcibly disable the interrupt if we're adding multiple timeouts in enable_timeouts(), even if we think no interrupt is pending. This might be overly paranoid, but since all of this is new code for 9.3 and hasn't been through any beta testing, I felt it best to preserve that safety feature. We can revisit it later if it proves to be an issue. (It's conceivable for instance that we could avoid incrementing the stored value of num_active_timeouts until we're done adding all the new timeouts; but that seemed pretty messy.) Your reasoning seems to be correct. However, if we take it to the extreme, enable_timeout_at/enable_timeout_after/enable_timeouts should also disable the interrupt handler at the beginning of the function and enabled at the end with pqsignal(). An evil soul may send SIGALRM externally to the backend processes at the proper moment and create a race condition while enable_timeout*() is in progress and the itimer is about to trigger at the same time (but doesn't since it was disabled). For the current usage pattern I'm not too sure that it matters anyway: a savings is only possible if you have enabled lock_timeout but not statement_timeout, and I'm doubtful that that will be a common use-case. I am not too sure about it. With lock_timeout in place, code migrated from Informix, MSSQL, etc. can have the same semantic behaviour. I also rearranged the handling of the LOCK_TIMEOUT interrupt some more, since I didn't see a need for it to be different from STATEMENT_TIMEOUT, and got rid of some non-C89 coding practices that didn't seem to me to be improving readability anyway. Thanks for that, too. I was too blind to see that choice, even after thinking about why the statement_timeout cannot be done from SIGALRM context and why does the code need to also send SIGINT to the process group. (To kill children, too, like scripts executed via system()...) Thanks again, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ -- 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
Please find attached the patches wanted: - 20130317_reindexdb_concurrently.patch, adding an option -c/--concurrently to reindexdb Note that I added an error inside reindexdb for options -s -c as REINDEX CONCURRENTLY does not support SYSTEM. - 20130317_dump_only_valid_index.patch, a 1-line patch that makes pg_dump not take a dump of invalid indexes. This patch can be backpatched to 9.0. On Sun, Mar 17, 2013 at 3:31 AM, Michael Paquier michael.paqu...@gmail.comwrote: On 2013/03/17, at 0:35, Fujii Masao masao.fu...@gmail.com wrote: On Wed, Mar 13, 2013 at 9:04 PM, Michael Paquier I found pg_dump dumps even the invalid index. But pg_dump should ignore the invalid index? This problem exists even without REINDEX CONCURRENTLY patch. So we might need to implement the bugfix patch separately rather than including the bugfix code in your patches. Probably the backport would be required. Thought? Hum... Indeed, they shouldn't be included... Perhaps this is already known? Note that there have been some recent discussions about that. This *problem* also concerned pg_upgrade. http://www.postgresql.org/message-id/20121207141236.gb4...@alvh.no-ip.org -- Michael 20130317_reindexdb_concurrently.patch Description: Binary data 20130317_dump_only_valid_index.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] [PATCH] Version info mixes up host and target platform when cross-compiling
I tried running PostgreSQL on the ARM64 (aka AArch64) emulator and noticed that the version() string mixes up the host and target architecture. Before: PostgreSQL 9.3devel on x86_64-pc-linux-gnu, compiled by aarch64-linux-gnu-gcc-4.7 (Ubuntu/Linaro 4.7.2-21ubuntu3) 4.7.2, 64-bit Now: PostgreSQL 9.3devel on aarch64-linux-gnu-gcc-4.7 (Ubuntu/Linaro 4.7.2-21ubuntu3) 4.7.2, compiled by x86_64-pc-linux-gnu, 64-bit In other news, I can confirm that PostgreSQL git HEAD works and passes all tests on AArch64 on Ubuntu Raring. The tests took 52 minutes to run in the emulator, but I got there. :) Obviously native spinlock code is still missing. There are no shipping processors yet so we have some time. Regards, Marti 0001-Version-info-mixes-up-host-and-target-platform-when-.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] [PATCH] Version info mixes up host and target platform when cross-compiling
On Sun, Mar 17, 2013 at 3:17 PM, Marti Raudsepp ma...@juffo.org wrote: Now: PostgreSQL 9.3devel on aarch64-linux-gnu-gcc-4.7 (Ubuntu/Linaro 4.7.2-21ubuntu3) 4.7.2, compiled by x86_64-pc-linux-gnu, 64-bit Sorry, that is clearly wrong. I'll come up with a better patch soon (and actually check that it makes sense! :) 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] [PATCH] Version info mixes up host and target platform when cross-compiling
On Sun, Mar 17, 2013 at 3:38 PM, Marti Raudsepp ma...@juffo.org wrote: Sorry, that is clearly wrong. I'll come up with a better patch soon (and actually check that it makes sense! :) Sorry about the noise, I just misunderstood the configure --target and --host arguments. If I set them correctly then the version string ends up right, too. 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] Strange Windows problem, lock_timeout test request
Boszormenyi Zoltan z...@cybertec.at writes: 2013-03-17 04:48 keltezéssel, Tom Lane írta: [ worries about stray SIGALRM events ] Your reasoning seems to be correct. However, if we take it to the extreme, enable_timeout_at/enable_timeout_after/enable_timeouts should also disable the interrupt handler at the beginning of the function and enabled at the end with pqsignal(). An evil soul may send SIGALRM externally to the backend processes at the proper moment and create a race condition while enable_timeout*() is in progress and the itimer is about to trigger at the same time (but doesn't since it was disabled). Well, a malefactor with the ability to send signals to a backend process could also send SIGKILL, or any number of other signals that would mess things up. I'm not too concerned about that scenario. I *am* concerned about leftover timer events, both because this code isn't very well tested, and because third-party code loaded into the backend (think pl/perl for instance) could easily set up timer events we weren't expecting. It suddenly occurs to me though that there's more than one way to skin this cat. We could easily add another static flag variable called sigalrm_allowed or some such, and have the signal handler test that and immediately return without doing anything if it's off. Clearing and setting such a variable would be a lot cheaper than an extra setitimer call, as well as more robust since it would protect us from all sources of SIGALRM not just ITIMER_REAL. 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] Re: Proposal for Allow postgresql.conf values to be changed via SQL [review]
On 3/14/13 4:48 PM, Boszormenyi Zoltan wrote: The attached patch makes SET PERSISTENT transactional and uses one setting per file. It uses the currently existing parsing and validating code and because of this, the patch is half the size of v12 from Amit. That's not a completely fair comparison, because you lost all the regression testing code too. This does look like a usefully tighter implementation in a few ways, so good progress on that. I still can't see any reason to prefer this one setting per file idea. As I see it, that is pushing the complexity toward the user in a bad way, seemingly just so it's easier to implement. Most of my customers now use tools like Puppet to manage their PostgreSQL configuration. I do not want to have this conversation: Greg: You can use SET PERSISTENT to modify settings instead of changing the postgresql.conf User: That's useful. How do we adjust Puppet to make sure it picks up the changes? Greg: You just scan this config directory and add every file that appears in there! Each setting will be in its own file. User: shocked look It creates new files? That isn't going to fit into our version control approach easily. Can we disable this feature so no one accidentally uses it? I'm not exaggerating here--one setting per file makes this feature less than useless to me. It becomes something where I will have to waste time defending against people using it. I'd prefer to not have this at all than to do it that way. That we're breaking these settings off into their own file, instead of trying to edit the postgresql.conf, to me is a pragmatic trade-off to keep the implementation from being really complicated. It's also a step forward in a larger series for how to improve configuration management. Just because that change introduces an entire directory being scanned, I don't see that as an excuse to clutter it with a long list of files too. -- 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] [COMMITTERS] pgsql: Move pqsignal() to libpgport.
On Sun, 2013-03-17 at 16:06 +, Tom Lane wrote: Move pqsignal() to libpgport. We had two copies of this function in the backend and libpq, which was already pretty bogus, but it turns out that we need it in some other programs that don't use libpq (such as pg_test_fsync). So put it where it probably should have been all along. The signal-mask-initialization support in src/backend/libpq/pqsignal.c stays where it is, though, since we only need that in the backend. Hi, When I try to compile HEAD right after this commit, I have this issue with pg_receivexlog: pg_receivexlog.c: In function ‘main’: pg_receivexlog.c:425:11: error: ‘SIGINT’ undeclared (first use in this function) pg_receivexlog.c:425:11: note: each undeclared identifier is reported only once for each function it appears in The attached patch fixes this. Not sure it's the right fix though... -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com diff --git a/src/bin/pg_basebackup/pg_receivexlog.c b/src/bin/pg_basebackup/pg_receivexlog.c index e65f127..91caf66 100644 --- a/src/bin/pg_basebackup/pg_receivexlog.c +++ b/src/bin/pg_basebackup/pg_receivexlog.c @@ -20,6 +20,7 @@ #include streamutil.h #include dirent.h +#include signal.h #include sys/stat.h #include sys/types.h #include unistd.h -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Move pqsignal() to libpgport.
Guillaume Lelarge guilla...@lelarge.info writes: On Sun, 2013-03-17 at 16:06 +, Tom Lane wrote: Move pqsignal() to libpgport. When I try to compile HEAD right after this commit, I have this issue with pg_receivexlog: Oddly, I didn't see that on the machine I was testing on --- it must have something else pulling in signal.h there. Fixed. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Move pqsignal() to libpgport.
On Sun, 2013-03-17 at 14:13 -0400, Tom Lane wrote: Guillaume Lelarge guilla...@lelarge.info writes: On Sun, 2013-03-17 at 16:06 +, Tom Lane wrote: Move pqsignal() to libpgport. When I try to compile HEAD right after this commit, I have this issue with pg_receivexlog: Oddly, I didn't see that on the machine I was testing on --- it must have something else pulling in signal.h there. Fixed. Thanks. -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Continue to export pqsignal() from libpq.so?
I just noticed that libpq no longer builds on my OS X machine: Undefined symbols for architecture x86_64: _pqsignal, referenced from: -exported_symbol[s_list] command line option ld: symbol(s) not found for architecture x86_64 collect2: ld returned 1 exit status make[3]: *** [libpq.5.6.dylib] Error 1 It appears that (1) pqsignal() is not needed in libpq itself if you're building with ENABLE_THREAD_SAFETY on, and (2) this causes OS X's linker to not bind it into the shlib from libpgport.a, even though it's needed according to the exported-symbols list. (It kinda looks like this is happening on my Linux box too, but the Linux linker doesn't complain about the unsatisfied reference :-(.) The easiest fix for this would be to remove pqsignal from the exports.txt list for libpq. Now that we have it in libpgport, there's no need so far as our own code goes for libpq to provide it, and it was never an official part of the API for libpq.so. However that fix is potentially an ABI break if anything else is (perhaps unintentionally) depending on pqsignal. If we don't do that then we need some kluge to force it to be bound into the shlib even though it's not used therein. Thoughts? 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 Windows problem, lock_timeout test request
2013-03-17 16:07 keltezéssel, Tom Lane írta:
Boszormenyi Zoltan z...@cybertec.at writes:
2013-03-17 04:48 keltezéssel, Tom Lane írta:
[ worries about stray SIGALRM events ]
Your reasoning seems to be correct. However, if we take it to the
extreme, enable_timeout_at/enable_timeout_after/enable_timeouts
should also disable the interrupt handler at the beginning of the
function and enabled at the end with pqsignal(). An evil soul may
send SIGALRM externally to the backend processes at the proper
moment and create a race condition while enable_timeout*() is in
progress and the itimer is about to trigger at the same time (but
doesn't since it was disabled).
Well, a malefactor with the ability to send signals to a backend
process could also send SIGKILL, or any number of other signals that
would mess things up. I'm not too concerned about that scenario.
I *am* concerned about leftover timer events, both because this code
isn't very well tested, and because third-party code loaded into the
backend (think pl/perl for instance) could easily set up timer events
we weren't expecting.
I see.
It suddenly occurs to me though that there's more than one way to skin
this cat. We could easily add another static flag variable called
sigalrm_allowed or some such, and have the signal handler test that
and immediately return without doing anything if it's off. Clearing
and setting such a variable would be a lot cheaper than an extra
setitimer call, as well as more robust since it would protect us from
all sources of SIGALRM not just ITIMER_REAL.
Something like the attached patch?
Best regards,
Zoltán Böszörményi
--
--
Zoltán Böszörményi
Cybertec Schönig Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
diff --git a/src/backend/utils/misc/timeout.c b/src/backend/utils/misc/timeout.c
index b5a3c8f..314a601 100644
--- a/src/backend/utils/misc/timeout.c
+++ b/src/backend/utils/misc/timeout.c
@@ -49,52 +49,29 @@ static bool all_timeouts_initialized = false;
static volatile int num_active_timeouts = 0;
static timeout_params *volatile active_timeouts[MAX_TIMEOUTS];
+/*
+ * Safeguard for the signal handler.
+ */
+static volatile bool alarm_enabled = false;
/*
* Internal helper functions
*
* For all of these, it is caller's responsibility to protect them from
- * interruption by the signal handler. Generally, call disable_alarm()
- * first to prevent interruption, then update state, and last call
+ * interruption by the signal handler. Generally, set alarm_enabled to false
+ * first to prevent interrupt to do anything, then update state, and last call
* schedule_alarm(), which will re-enable the interrupt if needed.
- */
-
-/*
- * Disable alarm interrupts
*
- * multi_insert must be true if the caller intends to activate multiple new
- * timeouts. Otherwise it should be false.
- */
-static void
-disable_alarm(bool multi_insert)
-{
- struct itimerval timeval;
+ * There may be a pending interrupt during enable_timeout_at(),
+ * enable_timeout_after() or enablwe_timeouts(). But since alarm_enabled is
+ * false, it is harmless when it triggers. We will reschedule the interrupt
+ * that just fired. In the worst case, it will be late by one unit of the
+ * the timer resolution in the OS.
+ *
+ */
- /*
- * If num_active_timeouts is zero, and multi_insert is false, we don't
- * have to call setitimer. There should not be any pending interrupt, and
- * even if there is, the worst possible case is that the signal handler
- * fires during schedule_alarm. (If it fires at any point before
- * insert_timeout has incremented num_active_timeouts, it will do nothing,
- * since it sees no active timeouts.) In that case we could end up
- * scheduling a useless interrupt ... but when the extra interrupt does
- * happen, the signal handler will do nothing, so it's all good.
- *
- * However, if the caller intends to do anything more after first calling
- * insert_timeout, the above argument breaks down, since the signal
- * handler could interrupt the subsequent operations leading to corrupt
- * state. Out of an abundance of caution, we forcibly disable the timer
- * even though it should be off already, just to be sure. Even though
- * this setitimer call is probably useless, we're still ahead of the game
- * compared to scheduling two or more timeouts independently.
- */
- if (multi_insert || num_active_timeouts 0)
- {
- MemSet(timeval, 0, sizeof(struct itimerval));
- if (setitimer(ITIMER_REAL, timeval, NULL) != 0)
- elog(FATAL, could not disable SIGALRM timer: %m);
- }
-}
+#define disable_alarm() \
+ do { alarm_enabled = false; } while (0)
/*
* Find
Re: [HACKERS] Re: Proposal for Allow postgresql.conf values to be changed via SQL [review]
2013-03-17 17:05 keltezéssel, Greg Smith írta: On 3/14/13 4:48 PM, Boszormenyi Zoltan wrote: The attached patch makes SET PERSISTENT transactional and uses one setting per file. It uses the currently existing parsing and validating code and because of this, the patch is half the size of v12 from Amit. That's not a completely fair comparison, because you lost all the regression testing code too. True. This does look like a usefully tighter implementation in a few ways, so good progress on that. I still can't see any reason to prefer this one setting per file idea. As I see it, that is pushing the complexity toward the user in a bad way, seemingly just so it's easier to implement. Most of my customers now use tools like Puppet to manage their PostgreSQL configuration. I do not want to have this conversation: Greg: You can use SET PERSISTENT to modify settings instead of changing the postgresql.conf User: That's useful. How do we adjust Puppet to make sure it picks up the changes? Greg: You just scan this config directory and add every file that appears in there! Each setting will be in its own file. User: shocked look It creates new files? That isn't going to fit into our version control approach easily. Can we disable this feature so no one accidentally uses it? I'm not exaggerating here--one setting per file makes this feature less than useless to me. It becomes something where I will have to waste time defending against people using it. I'd prefer to not have this at all than to do it that way. That we're breaking these settings off into their own file, instead of trying to edit the postgresql.conf, to me is a pragmatic trade-off to keep the implementation from being really complicated. It's also a step forward in a larger series for how to improve configuration management. Just because that change introduces an entire directory being scanned, I don't see that as an excuse to clutter it with a long list of files too. OK. I just wanted to show an alternative implementation. I admit, I haven't read all mails from this thread so I don't know how important for this feature to be non-transactional, or whether it would be better to be transactional. The one-file-to-rule-them-all approach can also be added to this patch as well, but synchronizing transactions over parsing and rewriting the extra file would decrease the relaxed effects of synchronous_commit. On the other hand, writing out one file per setting, although atomic to the level of one file, cannot be guaranteed to be atomic as a whole for all settings changed in the transaction without some synchronization. As far as I can see, AtEOXact_GUC() is called outside the critical section and is not synchronized any other way. (Currently, there's no need to.) Best regards, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ -- 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] Enabling Checksums
On 17 March 2013 00:41, Tom Lane t...@sss.pgh.pa.us wrote: Simon Riggs si...@2ndquadrant.com writes: On 15 March 2013 13:08, Andres Freund and...@2ndquadrant.com wrote: I commented on this before, I personally think this property makes fletcher a not so good fit for this. Its not uncommon for parts of a block being all-zero and many disk corruptions actually change whole runs of bytes. I think you're right to pick up on this point, and Ants has done a great job of explaining the issue more clearly. My perspective, after some thought, is that this doesn't matter to the overall effectiveness of this feature. PG blocks do have large runs of 0x00 in them, though that is in the hole in the centre of the block. If we don't detect problems there, its not such a big deal. Most other data we store doesn't consist of large runs of 0x00 or 0xFF as data. Most data is more complex than that, so any runs of 0s or 1s written to the block will be detected. Meh. I don't think that argument holds a lot of water. The point of having checksums is not so much to notice corruption as to be able to point the finger at flaky hardware. If we have an 8K page with only 1K of data in it, and we fail to notice that the hardware dropped a lot of bits in the other 7K, we're not doing our job; and that's not really something to write off, because it would be a lot better if we complain *before* the hardware manages to corrupt something valuable. So I think we'd be best off to pick an algorithm whose failure modes don't line up so nicely with probable hardware failure modes. It's worth noting that one of the reasons that CRCs are so popular is precisely that they were designed to detect burst errors with high probability. I think that's a reasonable refutation of my argument, so I will relent, especially since nobody's +1'd me. What I think we could do here is to allow people to set their checksum algorithm with a plugin. Please, no. What happens when their plugin goes missing? Or they install the wrong one on their multi-terabyte database? This feature is already on the hairy edge of being impossible to manage; we do *not* need to add still more complication. Agreed. (And thanks for saying please!) So I'm now moving towards commit using a CRC algorithm. I'll put in a feature to allow algorithm be selected at initdb time, though that is mainly a convenience to allow us to more easily do further testing on speedups and whether there are any platform specific regressions there. -- 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] Enabling Checksums
On 13 March 2013 06:33, Jeff Davis pg...@j-davis.com wrote: On Thu, 2013-03-07 at 13:45 -0800, Jeff Davis wrote: I need to do another self-review after these changes and some more extensive testing, so I might have missed a couple things. New patch attached. Aside from rebasing, I also found a problem with temp tables. At first I was going to fix it by continuing to exclude temp tables from checksums entirely. But then I re-thought it and decided to just checksum temp tables, too. Excluding temp tables from checksums means more special cases in the code, and more documentation. After thinking about it, there is no huge benefit to excluding temp tables: * small temp tables will be in memory only, and never checksummed * no WAL for temp tables, so the biggest cost of checksums is non-existent * there are good reasons to want to checksum temp tables, because they can be used to stage data for permanent tables However, I'm willing to be convinced to exclude temp tables again. I'm convinced we must include temp tables. No point putting a lock on the front door if there's a back door still open. -- 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] Enabling Checksums
On 3/15/13 5:32 AM, Ants Aasma wrote: Best case using the CRC32 instruction would be 6.8 bytes/cycle [1]. But this got me thinking about how to do this faster... [1] http://www.drdobbs.com/parallel/fast-parallelized-crc-computation-using/229401411 The optimization work you went through here looked very nice. Unfortunately, a few things seem pushing toward using a CRC16 instead of the Fletcher approach. It seems possible to execute a CRC16 in a reasonable enough time, in the same neighborhood as the Fletcher one. And there is some hope that hardware acceleration for CRCs will be available in a system API/compiler feature one day, making them even cheaper. Ants, do you think you could take a similar look at optimizing a CRC16 calculation? I'm back to where I can do a full performance comparison run again starting tomorrow, with the latest version of this patch, and I'd like to do that with a CRC16 implementation or two. I'm not sure if it's possible to get a quicker implementation because the target is a CRC16, or whether it's useful to consider truncating a CRC32 into a CRC16. -- 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] Enabling Checksums
On 3/17/13 1:41 PM, Simon Riggs wrote: So I'm now moving towards commit using a CRC algorithm. I'll put in a feature to allow algorithm be selected at initdb time, though that is mainly a convenience to allow us to more easily do further testing on speedups and whether there are any platform specific regressions there. That sounds reasonable. As I just posted, I'm hoping Ants can help make a pass over a CRC16 version, since his one on the Fletcher one seemed very productive. If you're spending time looking at this, I know I'd prefer to see you poking at the WAL related aspects instead. There are more of us who are capable of crunching CRC code than the list of people who have practice at WAL changes like you do. I see the situation with checksums right now as being similar to the commit/postpone situation for Hot Standby in 9.0. The code is uglier and surely buggier than we'd like, but it has been getting beat on regularly for over a year now to knock problems out. There are surely more bugs left to find. The improved testing that comes only from something being committed is probably necessary to really advance the testing coverage though. But with adopting the feature being a strict opt-in, the bug rate for non-adopters isn't that broad. All the TLI rearrangements is a lot of the patch, but that's pretty mechanical work that doesn't seem that risky. There was one question that kepts coming up in person this week (Simon, Jeff, Daniel, Josh Berkus, and myself were all in the same place for a few days) that I wanted to address with some thoughts on-list. Given that the current overhead is right on the edge of being acceptable, the concern is whether committing this will lock the project into a permanent problem that can't be improved later. I think it's manageable, though. Here's how I interpret the data we have: -The checksum has to change from Fletcher 16 to CRC-16. The hairy parts of the feature don't change very much from that though. I see exactly which checksum is produced is a pretty small detail, from a code correctness perspective. It's not like this will be starting over the testing cycle completely. The performance change should be quantified though. -Some common workloads will show no performance drop, like things that fit into shared_buffers and don't write hint bits. -Some common workloads that write things seem to hit about a 2% drop, presumably because they hit one of the slower situations around 10% of the time. -There are a decent number of hard to deal with workloads that have shared_buffers - OS cache thrashing, and any approach here will regularly hit them with around a 20% drop. There's some hope that this will improve later, especially if a CRC is used and later versions can pick up the Intel i7 CRC32 hardware acceleration. The magnitude of this overhead doesn't seem too negotiable though. We've heard enough comparisons with other people's implementations now to see that's near the best anyone does here. If the weird slowdowns some people report with very large values of shared_buffers is fixed, that will make this situation better. That's on my hit list of things I really want to see sorted in the next release. -The worst of the worst case behavior is Jeff's SELECTs now write a WAL logged hint bit now test, which can easily exceed a 20% drop. There have been lots of features submitted in the last two releases that try to improve hint bit operations. Some of those didn't show enough of a win to be worth the trouble. It may be the case, though, that in a checksummed environment those wins are suddenly big enough to matter. If any of those go in later, the worst case for checksums could then improve too. Having to test both ways, with and without checksums, complicates the performance testing. But the project has to start adopting a better approach to that in the next year regardless IMHO, and I'm scheduling time to help as much as I can with it. (That's a whole other discussion) -Having COPY FREEZE available now is a useful tool to eliminate a lot of the load/expensive hint bit write scenarios I know exist in the real world. I think the docs for checksumming should even highlight that synergy. As long as the feature is off by default, so that people have to turn it on to hit the biggest changed code paths, the exposure to potential bugs doesn't seem too bad. New WAL data is no fun, but it's not like this hasn't happened before. For version 9.3+1, there's a decent sized list of potential performance improvements that seem possible. I don't see any reason to believe committing a CRC16 based version of this will lock the implementation into a bad form that can't be optimized later. The comparison with Hot Standby again seems apt again here. There was a decent list of rough edges that were hit by early 9.0 adopters only when they turned the feature on. Then many were
Re: [HACKERS] Trust intermediate CA for client certificates
On 03/09/2013 04:52 PM, Ian Pilcher wrote: 3. Once the root CA certificate is trusted, however, the bad client can also connect by using a certificate chain that includes the Server CA certificate --cat bad-client.crt server-ca.crt ~/.postgresql/postgresql.crt. After looking at be-secure.c and investigating the way that OpenSSL validates certificates, I do not believe that there is any way of achieving the desired behavior with the current codebase. I'm testing this and looking into it now. At first glance this looks like a genuine problem. We need to be storing the certs used for validating client cert auth separately from the certificate chain that links those certs to trusted self-signed CA roots. I was under the strong impression that OpenSSL would do this if the client validation certs were in root.crt and the certificate chain was in OpenSSL's certificate search path and am testing that now. Even if that's the case we need to at least document this issue and preferably detect the case where root.crt contains a certificate chain. If this tests out as expected you need to consider the effects it'd have on people who're not using self-signed CAs, but are instead using certs signed by big CAs. *Any other customer of the same CA could potentially connect to your server with a genuine, valid client cert issued to them by the CA*. Ouch. I'm going through and reproducing the problem now and will also test OpenSSL certificate chain lookup path configurations to see if there's a way to set things up correctly with the current backend code. I'll report back shortly. -- Craig Ringer 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] Strange Windows problem, lock_timeout test request
Boszormenyi Zoltan z...@cybertec.at writes: 2013-03-17 16:07 keltezéssel, Tom Lane írta: It suddenly occurs to me though that there's more than one way to skin this cat. We could easily add another static flag variable called sigalrm_allowed or some such, and have the signal handler test that and immediately return without doing anything if it's off. Clearing and setting such a variable would be a lot cheaper than an extra setitimer call, as well as more robust since it would protect us from all sources of SIGALRM not just ITIMER_REAL. Something like the attached patch? Well, something like that, but it still needed some improvements. In particular I thought it best to leave the signal handler still releasing the procLatch unconditionally --- that behavior is independent of what this module is doing. Also you seem to have some odd ideas about what do-while will accomplish. AFAIK, in this usage it's purely a syntactic trick without much semantic content. It's the marking of the variable as volatile that counts for telling the compiler not to re-order operations. Updated and committed. 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] [v9.3] OAT_POST_ALTER object access hooks
On Tue, Mar 12, 2013 at 11:53 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: The attached patch is rebased one towards the latest master. It newly added a hook being missed in the previous revision at ALTER EVENT TRIGGER ENABLE / DISABLE, and adjusted argument of finish_heap_swap() on REFRESH MATERIALIZED VIEW to handle it as internal operations. Thanks. I committed the backend portions of this, but not the sepgsql and related documentation changes yet. I made some improvements to the comments along the way. I am not entirely happy with the grotty hack around ALTER COLUMN SET/DROP DEFAULT. Is there a better alternative? Do we really need a hook there at all? The one non-cosmetic change I made was to adjust slightly the firing point in swap_relation_files. It doesn't make any sense to me to exclude pg_class here, rather arbitrarily, out of all objects in the system. This does to some degree raise the question more broadly: is the point just after CatalogUpdateIndexes() the right rule for where to put this hook? But I've left it the way you have it for now. Part of me thinks that what you really want here is a pre-alter hook rather than a post-alter hook... -- 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] pg_test_fsync crashes on systems with POSIX signal handling
Bruce Momjian br...@momjian.us writes: On Fri, Mar 15, 2013 at 03:05:54PM -0400, Tom Lane wrote: The quick-and-dirty fix for this is to just copy pqsignal() into pg_test_fsync, and use that instead of calling signal() directly. I wonder though if we shouldn't move that function into libpgport. Thoughts? Well, the Win32 signal handler is already in port, so moving the Unix one seems to make sense, i.e. the comment above pgsignal says: /* Win32 signal handling is in backend/port/win32/signal.c */ Done, though it was a bit more painful than I expected --- I seem to have guessed completely wrong about where the portability hazards were. Good thing we have a buildfarm. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Patch to add regression tests for SCHEMA
Hi, Please find attached a patch to take 'make check' code-coverage of SCHEMA from 33% to 98%. Any feedback is more than welcome. p.s.: I am currently working on more regression tests (USER / VIEW / DISCARD etc). Please let me know if I need to post these as one large patch, instead of submitting one patch at a time. -- Robins Tharakan regress-schema.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] Patch to add regression tests for SCHEMA
robins escribió: Hi, Please find attached a patch to take 'make check' code-coverage of SCHEMA from 33% to 98%. Any feedback is more than welcome. I think you should use more explicit names for shared objects such as roles -- i.e. not r1 but regression_user_1 and so on. (But be careful about role names used by other tests). The issue is that these tests might be run in a database that contains other stuff; certainly we don't want to drop or otherwise affect previously existing roles. p.s.: I am currently working on more regression tests (USER / VIEW / DISCARD etc). Please let me know if I need to post these as one large patch, instead of submitting one patch at a time. I think separate patches is better. Are you adding these patches to the upcoming commitfest, for evaluation? https://commitfest.postgresql.org -- Á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] Trust intermediate CA for client certificates
On 03/09/2013 04:52 PM, Ian Pilcher wrote: After looking at be-secure.c and investigating the way that OpenSSL validates certificates, I do not believe that there is any way of achieving the desired behavior with the current codebase. Test process: SET UP SERVER VERIFIED SSL (NO CLIENT CERTS) -- Edited postgresql.conf and set: ssl=on ssl_cert_file = 'server.crt' ssl_key_file = 'server.key' ssl_ca_file='root.crt' Copied your samples: # cp $CERTS/postgres.crt server.crt # cp $CERTS/postgres.key server.key # cp $CERTS/client-ca.crt root.crt # chown postgres:postgres root.crt server.crt server.key # chmod 0600 server.crt server.key root.crt # systemctl restart postgresql-9.2.service $ psql postgresql://localhost/?sslmode=require SSL connection (cipher: DHE-RSA-AES256-SHA, bits: 256) (connects OK; expected since we aren't requiring client certs yet and we aren't validating the server cert). $ psql postgresql://localhost/?sslmode=verify-ca psql: root certificate file /home/craig/.postgresql/root.crt does not exist Either provide the file or change sslmode to disable server certificate verification. Again expected, though we really should be using the system SSL cert database. Anyway: $ mkdir .postgresql $ cp $CERTS/root-ca.crt ~/.postgresql/root.crt (This should be the trusted root, not server-ca or client-ca since we shouldn't have to keep copies of either to verify server trust). Now, test we can verify the server's identity: $ psql postgresql://localhost/?sslmode=require psql: SSL error: certificate verify failed Plonk, we can't. The reason for this is that the server has sent us its cert and we have the root cert, but we don't have the intermediate server-ca.crt . Append that to server.crt: # cat $CERTS/postgres.crt $CERTS/server-ca.crt server.crt # systemctl restart postgresql-9.2.service $ psql postgresql://localhost/?sslmode=require SSL connection (cipher: DHE-RSA-AES256-SHA, bits: 256) OK, we're good now, the server is sending us the intermediate cert we require. Regular non-client-cert verified SSL is fine. Examination of the protocol chat shows that the server is sending a Server Hello with a Certificate message containing the server and intermdediate certificate DNs: id-at-commonName=postgres.example.com,id-at-organizationName=Ian Pilcher,id-at-localityName=Carrollton,id-at-stateOrProvinceName=Texas,id-at-countryName=US id-at-commonName=Server CA,id-at-organizationName=Ian Pilcher,id-at-localityName=Carrollton,id-at-stateOrProvinceName=Texas,id-at-countryName=US as expected. ENABLE CLIENT CERTIFICATE VERIFICATION - Now lets install our client certificates in the client. $ cp $CERTS/good-client.key ~/.postgresql/postgresql.key $ # Note that the order is important here, the client cert must appear first, followed by the chain cert(s) $ cat $CERTS/good-client.crt $CERTS/client-ca.crt ~/.postgresql/postgresql.crt $ chmod 0600 .postgresql/postgresql.key $ psql postgresql://localhost/?sslmode=verify-ca psql: SSL error: tlsv1 alert unknown ca Examination of the handshake shows that the server is sending a request for client certificates signed by: Distinguished Name: (id-at-commonName=Client CA,id-at-organizationName=Ian Pilcher,id-at-localityName=Carrollton,id-at-stateOrProvinceName=Texas,id-at-countryName=US) and the client is sending in response: Certificate (id-at-commonName=Good Client,id-at-organizationName=Ian Pilcher,id-at-localityName=Carrollton,id-at-stateOrProvinceName=Texas,id-at-countryName=US) Certificate (id-at-commonName=Client CA,id-at-organizationName=Ian Pilcher,id-at-localityName=Carrollton,id-at-stateOrProvinceName=Texas,id-at-countryName=US) as expected, and good-client.crt is indeed signed by client-ca.crt . Again the issue appears to be that Pg can't find the root of trust, which is fair enough given that it is not present in Pg's root.crt or server.crt and it isn't installed system-wide either. We could add it to the server's root.crt but that'd cause the issues that started this thread: # cat $CERTS/client-ca.crt $CERTS/root-ca.crt root.crt # systemctl restart postgresql-9.2.service the good client cert works now: $ psql postgresql://localhost/?sslmode=verify-ca SSL connection (cipher: DHE-RSA-AES256-SHA, bits: 256) PROBLEM VERIFIED -- ... but so does the one we don't want to trust, as per the problem report: $ cat $CERTS/bad-client.crt $CERTS/server-ca.crt postgresql.crt $ cp $CERTS/bad-client.key postgresql.key $ chmod 600 postgresql.key $ psql postgresql://localhost/?sslmode=verify-ca SSL connection (cipher: DHE-RSA-AES256-SHA, bits: 256) So this problem is verified. THE SOLUTION What we need to happen instead is for root.crt to contain only the trusted certificates and have a *separate* file or directory for intermediate certificates that OpenSSL can look
Re: [HACKERS] Strange Windows problem, lock_timeout test request
2013-03-18 03:52 keltezéssel, Tom Lane írta: Boszormenyi Zoltan z...@cybertec.at writes: 2013-03-17 16:07 keltezéssel, Tom Lane írta: It suddenly occurs to me though that there's more than one way to skin this cat. We could easily add another static flag variable called sigalrm_allowed or some such, and have the signal handler test that and immediately return without doing anything if it's off. Clearing and setting such a variable would be a lot cheaper than an extra setitimer call, as well as more robust since it would protect us from all sources of SIGALRM not just ITIMER_REAL. Something like the attached patch? Well, something like that, but it still needed some improvements. In particular I thought it best to leave the signal handler still releasing the procLatch unconditionally --- that behavior is independent of what this module is doing. Also you seem to have some odd ideas about what do-while will accomplish. AFAIK, in this usage it's purely a syntactic trick without much semantic content. It's the marking of the variable as volatile that counts for telling the compiler not to re-order operations. The volatile marking shouldn't even be necessary there. The signal handler doesn't writes to it, only the main code. I just put it there saying why not? to myself. IIRC, volatile is needed if both the signal handler and the main code changes the same variable. I got the reordering idea from here: http://yarchive.net/comp/linux/compiler_barriers.html Thanks for committing, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ -- 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] Enabling Checksums
On Sun, Mar 17, 2013 at 5:50 PM, Greg Smith g...@2ndquadrant.com wrote: On the testing front, we've seen on-list interest in this feature from companies like Heroku and Enova, who both have some resources and practice to help testing too. Heroku can spin up test instances with workloads any number of ways. Enova can make a Londiste standby with checksums turned on to hit it with a logical replicated workload, while the master stays un-checksummed. I was thinking about turning checksums on for all new databases as long as I am able to turn them off easily, per my message prior: http://www.postgresql.org/message-id/caazkufzza+aw8zl4f_5c8t8zhrtjo3cm1ajqddglqcpez_3...@mail.gmail.com. An unstated assumption here was that I could apply the patch to 9.2 with some work. It seems the revitalized interest in the patch has raised a couple of issues on inspection that have yet to be resolved, so before moving I'd prefer to wait for a quiescence in the patch's evolution, as was the case for some time even after review. However, if we want to just hit 9.3dev with a bunch of synthetic traffic, that's probably doable also, and in some ways easier (or at least less risky). -- fdr -- 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 Windows problem, lock_timeout test request
2013-03-18 06:22 keltezéssel, Boszormenyi Zoltan írta: 2013-03-18 03:52 keltezéssel, Tom Lane írta: Boszormenyi Zoltan z...@cybertec.at writes: 2013-03-17 16:07 keltezéssel, Tom Lane írta: It suddenly occurs to me though that there's more than one way to skin this cat. We could easily add another static flag variable called sigalrm_allowed or some such, and have the signal handler test that and immediately return without doing anything if it's off. Clearing and setting such a variable would be a lot cheaper than an extra setitimer call, as well as more robust since it would protect us from all sources of SIGALRM not just ITIMER_REAL. Something like the attached patch? Well, something like that, but it still needed some improvements. In particular I thought it best to leave the signal handler still releasing the procLatch unconditionally --- that behavior is independent of what this module is doing. Also you seem to have some odd ideas about what do-while will accomplish. AFAIK, in this usage it's purely a syntactic trick without much semantic content. It's the marking of the variable as volatile that counts for telling the compiler not to re-order operations. The volatile marking shouldn't even be necessary there. The signal handler doesn't writes to it, only the main code. I just put it there saying why not? to myself. IIRC, volatile is needed if both the signal handler and the main code changes the same variable. Also, since the the variable assignment doesn't depend on other code in the same function (or vice-versa) the compiler is still free to reorder it. Volatile is about not caching the variable in a CPU register since it's volatile... I got the reordering idea from here: http://yarchive.net/comp/linux/compiler_barriers.html Thanks for committing, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ -- 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 Windows problem, lock_timeout test request
Boszormenyi Zoltan z...@cybertec.at writes: The volatile marking shouldn't even be necessary there. The signal handler doesn't writes to it, only the main code. Well, (a) that's not the case in the patch as committed, and (b) even if it were true, the volatile marking is still *necessary*, because that's what tells the compiler it can't optimize away changes to the variable, say on the grounds of there being another store with no intervening fetches in the main-line code. Without that, a compiler that had aggressively inlined the smaller functions could well deduce that the disable_alarm() assignment was useless. Also, since the the variable assignment doesn't depend on other code in the same function (or vice-versa) the compiler is still free to reorder it. Volatile is about not caching the variable in a CPU register since it's volatile... I don't believe you understand what volatile is about at all. Go read the C standard: it's about requiring objects' values to actually match the nominal program-specified values at sequence points. A more accurate heuristic is that volatile tells the compiler there may be other forces reading or writing the variable besides the ones it can see in the current function's source code, and so it can't drop or reorder accesses to the variable. 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
