Re: [HACKERS] Changeset Extraction v7.6
On Thu, February 13, 2014 17:12, Andres Freund wrote: Hi, On 2014-02-11 11:22:24 -0500, Robert Haas wrote: [loads of comments] I tried to address all the points you mentioned. 0001-wal_decoding-Introduce-logical-changeset-extraction.patch.gz 159 k 0002-wal_decoding-logical-changeset-extraction-walsender-.patch.gz 16 k 0003-wal_decoding-pg_recvlogical-Introduce-pg_receivexlog.patch.gz 15 k 0004-wal_decoding-Documentation-for-replication-slots-and.patch.gz 14 k 0005-wal_decoding-Temporarily-add-logical-decoding-regres.patch.gz 1.8 k These don't apply... -- 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] HBA files w/include support?
Bruce, * Bruce Momjian (br...@momjian.us) wrote: On Thu, Feb 13, 2014 at 08:24:27PM -0600, Jerry Sievers wrote: I'm aware of how a pg_hba.conf file can refer to other files for including @lists of users, etc. But there is currently no support for being able to pull in entire file segments as can be done for postgresql.conf via the include directive. In the environment that I'm managing, we are using a makefile to stick together a common header with a custom section for any of several clusters and may extend this further to permit additional includes for hba rules common to groupings of clusters. Anyway, please advise. I don't recall hearing anything like this discussed. Has been proposed, discussed and voted down? Or never mentioned? I have never heard of anyone request this. I've brought it up on various threads before, including both the ALTER SYSTEM thread and the postgresql.conf 'includes' thread, though I don't feel like going back and hunting down the specific emails right now. Having @include and directory.d-style capabilities for pg_hba.conf *and* pg_ident.conf would make managing larger environments much better. There has been some talk about providing those capabilities via tables in the catalog, but I'm not aware of anyone working on it and it'd certainly be quite a bit more work than adding include/dir.d options. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] [BUG] Archive recovery failure on 9.3+.
Hello, Before taking up the topic.. At Thu, 13 Feb 2014 19:45:38 +0200, Heikki Linnakangas wrote On 02/13/2014 06:47 PM, Heikki Linnakangas wrote: On 02/13/2014 02:42 PM, Heikki Linnakangas wrote: The behavior where we prefer a segment from archive with lower TLI over a file with higher TLI in pg_xlog actually changed in commit a068c391ab0. Arguably changing it wasn't a good idea, but the problem your test script demonstrates can be fixed by not archiving the partial segment, with no change to the preference of archive/pg_xlog. As discussed, archiving a partial segment seems like a bad idea anyway, so let's just stop doing that. It surely makes things simple and I rather like the idea but as long as the final and possiblly partial segment of the lower TLI is actually created and the recovery mechanism allows users to command recovery operation requires such segments (recovery_target_timeline does this), a perfect archive - which means an archive which can cover all sorts of restore operatoins - necessarily may have such duplicate segments, I believe. Besides, I suppose that policy makes operations around archive/restore way difficult. DBAs should get stuck with tensive work of picking only actually needed segments for the recovery undertaken out of the haystack. It sounds somewhat gloomy.. # However I also doubt the appropriateness of stockpiling archive # segments spanning over so many timelines, two generations are # enough to cause this issue. Anyway, returning to the topic, After some further thought, while not archiving the partial segment fixes your test script, it's not enough to fix all variants of the problem. Even if archive recovery doesn't archive the last, partial, segment, if the original master server is still running, it's entirely possible that it fills the segment and archives it. In that case, archive recovery will again prefer the archived segment with lower TLI over the segment with newer TLI in pg_xlog. Yes, it is the generalized description of the case I've mentioned. (Though I've not reached that thought :) So I agree we should commit the patch you posted (or something to that effect). The change to not archive the last segment still seems like a good idea, but perhaps we should only do that in master. My opinion on duplicate segments on older timelines is as decribed above. To draw this to conclusion, barring any further insights to this, I'm going to commit the attached patch to master and REL9_3_STABLE. Please have a look at the patch, to see if I'm missing something. I modified the state machine to skip over XLOG_FROM_XLOG state, if reading in XLOG_FROM_ARCHIVE failed; otherwise you first scan the archive and pg_xlog together, and then pg_xlog alone, which is pointless. In master, I'm also going to remove the archive last segment on old timeline code. Thank you for finishing the patch. I didn't think of the behavior after XLOG_FROM_ARCHIVE failure. It seems that the state machine will go round getting rid of extra round with it. Recovery process becomes able to grab the segment on highest (expected) TLI among those with the same segment id regardless of their locations. I think the recovery process will cope with perfect archives described above for all types of recovery operation. The state machine loop considering fallback from archive to pg_xlog now seems somewhat too complicated than needed but it's also no harm. Though, here which was in my original patch, readFile = XLogFileReadAnyTLI(readSegNo, DEBUG2, currentSource == XLOG_FROM_ARCHIVE ? XLOG_FROM_ANY : currentSource); is sticking far out the line wrapping boundary and seems somewhat dirty:( And what the conditional operator seems to make the meaning of the XLOG_FROM_ARCHIVE and _ANY a bit confused. But I failed to unify them to any side so it is left as is.. Finally, the patch you will find attached is fixed only in styling mentioned above from your last patch. This patch applies current HEAD and I confirmed that it fixes this issue but I have not checked the lastSourceFailed section. Simple file removal could not lead to there. regards, -- Kyotaro Horiguchi NTT Open Source Software Center diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 508970a..85a0ce9 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -11006,17 +11006,15 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess, /*--- * Standby mode is implemented by a state machine: * - * 1. Read from archive (XLOG_FROM_ARCHIVE) - * 2. Read from pg_xlog (XLOG_FROM_PG_XLOG) - * 3. Check trigger file - * 4. Read from primary server via walreceiver (XLOG_FROM_STREAM) - * 5. Rescan timelines - * 6. Sleep 5 seconds, and loop back to 1. + * 1. Read from either archive or pg_xlog (XLOG_FROM_ARCHIVE), or just + *pg_xlog (XLOG_FROM_XLOG) + *
Re: [HACKERS] narwhal and PGDLLIMPORT
(2014/02/12 8:30), Tom Lane wrote: I wrote: Hiroshi Inoue in...@tpf.co.jp writes: I tried MINGW port with the attached change and successfully built src and contrib and all pararell regression tests were OK. I cleaned this up a bit (the if-nesting in Makefile.shlib was making my head hurt, not to mention that it left a bunch of dead code) and committed it. Hm ... according to buildfarm member narwhal, this doesn't work so well for plperl: gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -g -Wno-comment -shared -o plperl.dll plperl.o SPI.o Util.o -L../../../src/port -L../../../src/common -Wl,--allow-multiple-definition -L/mingw/lib -Wl,--as-needed -LC:/Perl/lib/CORE -lperl58 -L../../../src/backend -lpostgres -lpgcommon -lpgport -lintl -lxslt -lxml2 -lssleay32 -leay32 -lz -lm -lws2_32 -lshfolder -Wl,--export-all-symbols -Wl,--out-implib=libplperl.a Cannot export .idata$4: symbol not found Cannot export .idata$5: symbol not found Cannot export .idata$6: symbol not found Cannot export .text: symbol not found Cannot export perl58_NULL_THUNK_DATA: symbol not found Creating library file: libplperl.a collect2: ld returned 1 exit status make[3]: *** [plperl.dll] Error 1 Not very clear what's going on there; could this be a problem in narwhal's admittedly-ancient toolchain? The error doesn't occur here (un)fortunately. One thing I'm wondering about is that plperl is linking perlxx.lib not libperlxx.a. I made a patch following plpython and it also works here. Is it worth trying? regards, Hiroshi Inoue diff --git a/src/pl/plperl/GNUmakefile b/src/pl/plperl/GNUmakefile index e0e31ec..065c5d3 100644 --- a/src/pl/plperl/GNUmakefile +++ b/src/pl/plperl/GNUmakefile @@ -16,6 +16,8 @@ endif ifeq ($(shared_libperl),yes) ifeq ($(PORTNAME), win32) +generate_perl_implib = yes + override CPPFLAGS += -DPLPERL_HAVE_UID_GID # Perl on win32 contains /* within comment all over the header file, # so disable this warning. @@ -36,7 +38,21 @@ DATA = plperl.control plperl--1.0.sql plperl--unpackaged--1.0.sql \ PERLCHUNKS = plc_perlboot.pl plc_trusted.pl +# Perl on win32 ships with import libraries only for Microsoft Visual C++, +# which may not be handled with mingw gcc. Therefore we choose to build a +# new import library to link with. +ifeq ($(generate_perl_implib), yes) + perlwithver = $(subst -l,,$(filter -l%, $(perl_embed_ldflags))) + OBJS += lib$(perlwithver).a +lib$(perlwithver).a: $(perlwithver).def + dlltool --dllname $(perlwithver).dll --def $(perlwithver).def --output-lib lib$(perlwithver).a +$(perlwithver).def: + pexports $(dir $(subst ',,$(PERL)))$(perlwithver).dll $(perlwithver).def + +SHLIB_LINK = +else SHLIB_LINK = $(perl_embed_ldflags) +endif REGRESS_OPTS = --dbname=$(PL_TESTDB) --load-extension=plperl --load-extension=plperlu REGRESS = plperl plperl_lc plperl_trigger plperl_shared plperl_elog plperl_util plperl_init plperlu plperl_array @@ -105,6 +121,9 @@ submake: clean distclean maintainer-clean: clean-lib rm -f SPI.c Util.c $(OBJS) perlchunks.h plperl_opmask.h rm -rf $(pg_regress_clean_files) +ifeq (yes, $(generate_perl_implib)) + rm -rf $(perlwithver).def +endif else # can't build -- 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] Changeset Extraction v7.6
Hi, On 2014-02-14 09:23:45 +0100, Erik Rijkers wrote: 0001-wal_decoding-Introduce-logical-changeset-extraction.patch.gz159 k 0002-wal_decoding-logical-changeset-extraction-walsender-.patch.gz 16 k 0003-wal_decoding-pg_recvlogical-Introduce-pg_receivexlog.patch.gz 15 k 0004-wal_decoding-Documentation-for-replication-slots-and.patch.gz 14 k 0005-wal_decoding-Temporarily-add-logical-decoding-regres.patch.gz 1.8 k These don't apply... Works here, could you give a bit more details about the problem you have applying them? Note that they are compressed, so need to be gunzipped first... 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] New hook after raw parsing, before analyze
Thanks for the reply. There are two things I think I’ve been misunderstood: 1, the point is to do the rewrite without and before catalog access 2, I do want to push the join to the source and equally important pushing the where conditions there Best regards, David 2014.02.13. dátummal, 21:22 időpontban Tom Lane t...@sss.pgh.pa.us írta: David Beck db...@starschema.net writes: I have table like data structures in the source system for the FDW I work on. These tables are sometimes too big and the source system is able to filter and join them with limitations, thus it is not optimal to transfer the data to Postgres. At the same time I want the users to think in terms of the original tables. The idea is to rewrite the SQL queries like this: “SELECT * FROM tableA a, tableB b WHERE a.id=b.id AND a.col1=1234 AND b.col2=987” to: “SELECT * FROM fdw_tableA_tableB ab WHERE ab.col1=1234 AND ab.col2=987” TBH this sounds like a spectacularly bad idea, especially in the place and way you propose to do it. You can't even do catalog access safely where you've put that hook, not to mention that there are many other places where queries can be submitted. But more generally, an FDW should not operate in the way you're describing. We do lack support for pushing joins to the foreign server, and that needs to be addressed; but we need to do it in the planner, not by kluging the query somewhere upstream of that. 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] Changeset Extraction v7.6
On Fri, February 14, 2014 10:13, Andres Freund wrote: Hi, On 2014-02-14 09:23:45 +0100, Erik Rijkers wrote: 0001-wal_decoding-Introduce-logical-changeset-extraction.patch.gz 159 k 0002-wal_decoding-logical-changeset-extraction-walsender-.patch.gz 16 k 0003-wal_decoding-pg_recvlogical-Introduce-pg_receivexlog.patch.gz 15 k 0004-wal_decoding-Documentation-for-replication-slots-and.patch.gz 14 k 0005-wal_decoding-Temporarily-add-logical-decoding-regres.patch.gz 1.8 k These don't apply... Works here, could you give a bit more details about the problem you have applying them? Note that they are compressed, so need to be gunzipped first... yeah, unzipping -- I thought of that :) (no offense taken :)) I just gave it into my standard patch-applying-compiling shell script which does something like, as always : patch --dry-run -b -l -F 25 -p 1 /home/aardvark/download/pgpatches/0094/lcsg/20140213/0001-wal_decoding-Introduce-logical-changeset-extraction.patch patch.1.dry-run.1.out (it loops to try out several -p values, none acceptable) etc -- 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] Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease
On 2014-02-13 15:34:09 +0100, Florian Pflug wrote: On Feb10, 2014, at 17:38 , Andres Freund and...@2ndquadrant.com wrote: On 2014-02-10 11:11:28 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: So what we need to do is to acquire a write barrier between the assignments to lwWaitLink and lwWaiting, i.e. proc-lwWaitLink = NULL; pg_write_barrier(); proc-lwWaiting = false; You didn't really explain why you think that ordering is necessary? Each proc being awoken will surely see both fields updated, and other procs won't be examining these fields at all, since we already delinked all these procs from the LWLock's queue. The problem is that one the released backends could wake up concurrently because of a unrelated, or previous PGSemaphoreUnlock(). It could see lwWaiting = false, and thus wakeup and acquire the lock, even if the store for lwWaitLink hasn't arrived (or performed, there's no guaranteed ordering here) yet. Now, it may well be that there's no practical consequence of that, but I am not prepared to bet on it. AFAICS there is a potential problem if three backends are involved, since by the time the waiting backend's lwWaitLink is set to NULL after the original lock holder released the lock, the waiting backend might already have acquired the lock (due to a spurious wakeup) *and* a third backend might have already enqueued behind it. The exact sequence for backends A,B,C that corrupts the wait queue is: A: Release lock, set B's lwWaiting to false B: Wakes up spuriously, takes the lock C: Enqueues behind B A: Sets B' lwWaitLink back to NULL, thereby truncating the queue and causing C and anyone behind it to block indefinitely. I don't think that can actually happen because the head of the wait list isn't the lock holder's lwWaitLink, but LWLock-head. I thought the same for a while... So, right now I don't see problems without either the compiler reordering stores or heavily out of order machines with speculative execution. I wonder whether LWLockRelease really needs to update lwWaitLink at all. We take the backends we awake off the queue by updating the queue's head and tail, so the contents of lwWaitLink should only matter once the backend is re-inserted into some wait queue. But when doing that, we reset lwWaitLink back to NULL anway. I don't like that, this stuff is hard to debug already, having stale pointers around doesn't help. I think we should just add the barrier in the releases with barrier.h and locally use a volatile var in the branches before that. 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
[HACKERS] walsender can ignore send failures in WalSndLoop
Hi, There's a small issue in abfd192b, namely one of the error cases wasn't changed when WalSndLoop was changed to be able to return. I don't think this is likely to have any grave consequences, we'll likely error out soon afterwards again. Patch attached. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c index 06b22e2..8750dd2 100644 --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -1287,7 +1287,7 @@ WalSndLoop(void) ping_sent = true; /* Try to flush pending output to the client */ if (pq_flush_if_writable() != 0) - break; + goto send_failure; } } -- 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] gaussian distribution pgbench
I add exponential distribution random generator (and little bit refactoring:) ). I use inverse transform method to create its distribution. It's very simple method that is created by - log (rand()). We can control slope of distribution using threshold parameter. It is same as gaussian threshold. usage example pgbench --exponential=NUM -S Attached graph is created with exponential threshold = 5. We can see exponential distribution in the graphs. It supports -S, -N options and custom script. So we set ¥setexponential [var] [min] [max] [threshold] in a transaction pattern file, it appear distribution we want. We have no time to fix its very much... But I think almost part of patch have been completed. Regards, -- Mitsumasa KONDO NTT Open Source Software Center gaussian_and_exponential_pgbench_v6.patch Description: Binary data attachment: exponential=5.png gnuplot.sh Description: Bourne shell script -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] walsender doesn't send keepalives when writes are pending
Hi,
In WalSndLoop() we do:
wakeEvents = WL_LATCH_SET | WL_POSTMASTER_DEATH | WL_TIMEOUT |
WL_SOCKET_READABLE;
if (pq_is_send_pending())
wakeEvents |= WL_SOCKET_WRITEABLE;
else if (wal_sender_timeout 0 !ping_sent)
{
...
if (GetCurrentTimestamp() = timeout)
WalSndKeepalive(true);
...
I think those two if's should simply be separate. There's no reason not
to ask for a ping when we're writing. On a busy server that might be the
case most of the time.
The if (pq_is_send_pending()) should also be *after* sending the
keepalive, after all, it might not immediately have been flushed as
unlikely as that is.
The attached patch is unsurprisingly simple.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 06b22e2..41db03d 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1270,9 +1270,7 @@ WalSndLoop(void)
wakeEvents = WL_LATCH_SET | WL_POSTMASTER_DEATH | WL_TIMEOUT |
WL_SOCKET_READABLE;
- if (pq_is_send_pending())
-wakeEvents |= WL_SOCKET_WRITEABLE;
- else if (wal_sender_timeout 0 !ping_sent)
+ if (wal_sender_timeout 0 !ping_sent)
{
/*
* If half of wal_sender_timeout has lapsed without receiving
@@ -1291,6 +1289,9 @@ WalSndLoop(void)
}
}
+ if (pq_is_send_pending())
+wakeEvents |= WL_SOCKET_WRITEABLE;
+
/* Determine time until replication timeout */
if (wal_sender_timeout 0)
{
--
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] Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease
On Feb14, 2014, at 11:45 , Andres Freund and...@2ndquadrant.com wrote: On 2014-02-13 15:34:09 +0100, Florian Pflug wrote: On Feb10, 2014, at 17:38 , Andres Freund and...@2ndquadrant.com wrote: On 2014-02-10 11:11:28 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: So what we need to do is to acquire a write barrier between the assignments to lwWaitLink and lwWaiting, i.e. proc-lwWaitLink = NULL; pg_write_barrier(); proc-lwWaiting = false; You didn't really explain why you think that ordering is necessary? Each proc being awoken will surely see both fields updated, and other procs won't be examining these fields at all, since we already delinked all these procs from the LWLock's queue. The problem is that one the released backends could wake up concurrently because of a unrelated, or previous PGSemaphoreUnlock(). It could see lwWaiting = false, and thus wakeup and acquire the lock, even if the store for lwWaitLink hasn't arrived (or performed, there's no guaranteed ordering here) yet. Now, it may well be that there's no practical consequence of that, but I am not prepared to bet on it. AFAICS there is a potential problem if three backends are involved, since by the time the waiting backend's lwWaitLink is set to NULL after the original lock holder released the lock, the waiting backend might already have acquired the lock (due to a spurious wakeup) *and* a third backend might have already enqueued behind it. The exact sequence for backends A,B,C that corrupts the wait queue is: A: Release lock, set B's lwWaiting to false B: Wakes up spuriously, takes the lock C: Enqueues behind B A: Sets B' lwWaitLink back to NULL, thereby truncating the queue and causing C and anyone behind it to block indefinitely. I don't think that can actually happen because the head of the wait list isn't the lock holder's lwWaitLink, but LWLock-head. I thought the same for a while... Hm, true, but does that protect us under all circumstances? If another backend grabs the lock before B gets a chance to do so, B will become the wait queue's head, and anyone who enqueues behind B will do so by updating B's lwWaitLink. That is then undone by the delayed reset of B's lwWaitLink by the original lock holder. The specific sequence I have in mind is: A: Take lock B: Enqueue A: Release lock, set B's lwWaiting to false D: Acquire lock B: Enqueue after spurious wakeup (lock-head := B) C: Enqueue behind B (B-lwWaitLink := C, lock-tail := C) A: Set B's wWaitLink back to NULL, thereby corrupting the queue (B-lwWaitLink := NULL) D: Release lock, dequeue and wakeup B (lock-head := B-lwWaitLink, i.e. lock-head := NULL) B: Take and release lock, queue appears empty! C blocks indefinitely. I wonder whether LWLockRelease really needs to update lwWaitLink at all. We take the backends we awake off the queue by updating the queue's head and tail, so the contents of lwWaitLink should only matter once the backend is re-inserted into some wait queue. But when doing that, we reset lwWaitLink back to NULL anway. I don't like that, this stuff is hard to debug already, having stale pointers around doesn't help. I think we should just add the barrier in the releases with barrier.h and locally use a volatile var in the branches before that. I like the idea of updating lwWaiting and lwWaitLink while still holding the spinlock better. The costs are probably negligible, and it'd make the code much easier to reason about. best regards, Florian Pflug -- 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] Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease
On 2014-02-14 13:28:47 +0100, Florian Pflug wrote: I don't think that can actually happen because the head of the wait list isn't the lock holder's lwWaitLink, but LWLock-head. I thought the same for a while... Hm, true, but does that protect us under all circumstances? If another backend grabs the lock before B gets a chance to do so, B will become the wait queue's head, and anyone who enqueues behind B will do so by updating B's lwWaitLink. That is then undone by the delayed reset of B's lwWaitLink by the original lock holder. The specific sequence I have in mind is: A: Take lock B: Enqueue A: Release lock, set B's lwWaiting to false D: Acquire lock B: Enqueue after spurious wakeup (lock-head := B) C: Enqueue behind B (B-lwWaitLink := C, lock-tail := C) A: Set B's wWaitLink back to NULL, thereby corrupting the queue (B-lwWaitLink := NULL) D: Release lock, dequeue and wakeup B (lock-head := B-lwWaitLink, i.e. lock-head := NULL) B: Take and release lock, queue appears empty! C blocks indefinitely. That's assuming either reordering by the compiler or an out-of-order store architecture, right? I wonder whether LWLockRelease really needs to update lwWaitLink at all. We take the backends we awake off the queue by updating the queue's head and tail, so the contents of lwWaitLink should only matter once the backend is re-inserted into some wait queue. But when doing that, we reset lwWaitLink back to NULL anway. I don't like that, this stuff is hard to debug already, having stale pointers around doesn't help. I think we should just add the barrier in the releases with barrier.h and locally use a volatile var in the branches before that. I like the idea of updating lwWaiting and lwWaitLink while still holding the spinlock better. The costs are probably negligible, and it'd make the code much easier to reason about. I agree we should do that, but imo not in the backbranches. Anything more than than the minimal fix in that code should be avoided in the stable branches, this stuff is friggin performance sensitive, and the spinlock already is a *major* contention point in many workloads. 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] issue with gininsert under very high load
On 2014-02-14 08:06:40 +0100, Jesper Krogh wrote: On 14/02/14 00:49, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2014-02-13 16:15:42 -0500, Tom Lane wrote: Something like the attached? Can somebody who's seen this problem confirm this improves matters? Hm. Won't that possiby lead to the fast tuple list growing unboundedly? I think we would need to at least need to stop using the fast tuple mechanism during gininsert() if it's already too big and do plain inserts. No, because we've already got a process working on cleaning it out. In any case, this needs some testing to see if it's an improvement or not. Having some real-world experience with the fastupdate mechanism. Under concurrent load it behaves really bad. Random processes waiting for cleanup (or competing with cleanup) is going to see latency-spikes, because they magically hit that corner, thus reverting to plain inserts if it cannot add to the pending list, will not remove this problem, but will make it only hit the process actually doing the cleanup. Yea, this is only a part of fixing fastupdate. Limiting the size of the fastupdate list to something more reasonable is pretty important as well. Not competing around cleanup will make cleanup much faster though, so I am not that concerned about the latency spikes it causes once it's limited in size and done non-concurrently. The build in mechanism, that cleanup is i cost paid by the process who happened to fill the pendinglist, is really hard to deal with in production. More control is appreciated, perhaps even an explicit flush-mechanism.. I'd like to batch up inserts during one transaction only and flush on commit. That doesn't seem likely to work with a reasonable amount of effort. The fastupdate list is shared across all processes, so one backend will always pay the price for several others. 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] Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease
On Feb14, 2014, at 13:36 , Andres Freund and...@2ndquadrant.com wrote: On 2014-02-14 13:28:47 +0100, Florian Pflug wrote: I don't think that can actually happen because the head of the wait list isn't the lock holder's lwWaitLink, but LWLock-head. I thought the same for a while... Hm, true, but does that protect us under all circumstances? If another backend grabs the lock before B gets a chance to do so, B will become the wait queue's head, and anyone who enqueues behind B will do so by updating B's lwWaitLink. That is then undone by the delayed reset of B's lwWaitLink by the original lock holder. The specific sequence I have in mind is: A: Take lock B: Enqueue A: Release lock, set B's lwWaiting to false D: Acquire lock B: Enqueue after spurious wakeup (lock-head := B) C: Enqueue behind B (B-lwWaitLink := C, lock-tail := C) A: Set B's wWaitLink back to NULL, thereby corrupting the queue (B-lwWaitLink := NULL) D: Release lock, dequeue and wakeup B (lock-head := B-lwWaitLink, i.e. lock-head := NULL) B: Take and release lock, queue appears empty! C blocks indefinitely. That's assuming either reordering by the compiler or an out-of-order store architecture, right? Yes, it requires that a backend exits out of the PGSemaphoreLock loop in LWLockAcquire before lwWaitLink has been reset to NULL by the previous lock holder's LWLockRelease. Only if that happens there is a risk of the PGPROC being on a wait queue by the time lwWaitLink is finally reset to NULL. I wonder whether LWLockRelease really needs to update lwWaitLink at all. We take the backends we awake off the queue by updating the queue's head and tail, so the contents of lwWaitLink should only matter once the backend is re-inserted into some wait queue. But when doing that, we reset lwWaitLink back to NULL anway. I don't like that, this stuff is hard to debug already, having stale pointers around doesn't help. I think we should just add the barrier in the releases with barrier.h and locally use a volatile var in the branches before that. I like the idea of updating lwWaiting and lwWaitLink while still holding the spinlock better. The costs are probably negligible, and it'd make the code much easier to reason about. I agree we should do that, but imo not in the backbranches. Anything more than than the minimal fix in that code should be avoided in the stable branches, this stuff is friggin performance sensitive, and the spinlock already is a *major* contention point in many workloads. No argument there. But unless I missed something, there actually is a bug there, and using volatile won't fix it. A barrier would, but what about the back branches that don't have barrier.h? AFAICS the only other choices we have on these branches are to either ignore the bug - it's probably *extremely* unlikely - or to use a spinlock acquire/release cycle to create a barrier. The former leaves me with a bit of an uneasy feeling, and the latter quite certainly has a larger performance impact than moving the PGPROC updates within the section protected by the spinlock and using an array to remember the backends to wakeup. best regards, Florian Pflug -- 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] walsender doesn't send keepalives when writes are pending
On Fri, Feb 14, 2014 at 12:05 PM, Andres Freund and...@2ndquadrant.com wrote: There's no reason not to ask for a ping when we're writing. Is there a reason to ask for a ping? The point of keepalives is to ensure there's some traffic on idle connections so that if the connection is dead it doesn't linger forever and so that any on-demand links (or more recently NAT routers or stateful firewalls) don't time out and disconnect and have to reconnect (or more recently just fail outright). By analogy TCP doesn't send any keepalives if there is any traffic on the link. On the other hand I happen to know (the hard way) that typical PPPoE routers *do* send LCP pings even when the link is busy so there's precedent for either behaviour. I'm guess it comes down to why you want the keepalives. -- 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] HBA files w/include support?
Jerry Sievers wrote: The other thing that comes to mind, is that as opposed to postgresql.conf and the include scenario there... one can do show all or query from pg_stat_activity just to see what setting they ended up with. I'm not aware of any way to probe what hba rules are loaded at runtime and thus, debugging hba config changes not really possible. Well, getting includes in postgresql.conf in an acceptable shape required lots more work than just implementing the include directive; one of the very first requirements was that the file path and line number that each setting got its value from was available in pg_settings. I'm pretty sure that a patch that only added includes without equivalent functionality would not get very far. -- Á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] walsender doesn't send keepalives when writes are pending
On 2014-02-14 12:55:06 +, Greg Stark wrote: On Fri, Feb 14, 2014 at 12:05 PM, Andres Freund and...@2ndquadrant.com wrote: There's no reason not to ask for a ping when we're writing. Is there a reason to ask for a ping? The point of keepalives is to ensure there's some traffic on idle connections so that if the connection is dead it doesn't linger forever and so that any on-demand links (or more recently NAT routers or stateful firewalls) don't time out and disconnect and have to reconnect (or more recently just fail outright). This ain't TCP keepalives. The reason is that we want to kill walsenders if they haven't responded to a ping inside wal_sender_timeout. That's rather important e.g. for sychronous replication, so we can quickly fall over to the next standby. In such scenarios you'll usually want a timeout *far* below anything TCP provides. 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] issue with gininsert under very high load
Andres Freund wrote: On 2014-02-14 08:06:40 +0100, Jesper Krogh wrote: The build in mechanism, that cleanup is i cost paid by the process who happened to fill the pendinglist, is really hard to deal with in production. More control is appreciated, perhaps even an explicit flush-mechanism.. I'd like to batch up inserts during one transaction only and flush on commit. That doesn't seem likely to work with a reasonable amount of effort. The fastupdate list is shared across all processes, so one backend will always pay the price for several others. Unless some other process does it, such as autovacuum. -- Á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] Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease
On 2014-02-14 13:52:45 +0100, Florian Pflug wrote: I agree we should do that, but imo not in the backbranches. Anything more than than the minimal fix in that code should be avoided in the stable branches, this stuff is friggin performance sensitive, and the spinlock already is a *major* contention point in many workloads. No argument there. But unless I missed something, there actually is a bug there, and using volatile won't fix it. A barrier would, but what about the back branches that don't have barrier.h? Yea, but I don't see a better alternative. Realistically the likelihood of a problem without the compiler reordering stuff is miniscule on any relevant platform that's supported by the !barrier.h branches. Newer ARMs are the only realistic suspect, and the support for in older releases wasn't so good... The former leaves me with a bit of an uneasy feeling, and the latter quite certainly has a larger performance impact than moving the PGPROC updates within the section protected by the spinlock and using an array to remember the backends to wakeup. I am not so sure, it adds a host of new cacheline references in a piece of code that's already heavily affected by pipeline stalls, that can influence performance. I am not saying it's super likely, just more than I want to do for a theoretical problem in the back branches. 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] [BUG] Archive recovery failure on 9.3+.
On 02/14/2014 10:38 AM, Kyotaro HORIGUCHI wrote: Finally, the patch you will find attached is fixed only in styling mentioned above from your last patch. This patch applies current HEAD and I confirmed that it fixes this issue but I have not checked the lastSourceFailed section. Simple file removal could not lead to there. Ok, applied. Thanks! - Heikki -- 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] walsender doesn't send keepalives when writes are pending
On 2014-02-14 13:58:59 +0100, Andres Freund wrote: On 2014-02-14 12:55:06 +, Greg Stark wrote: On Fri, Feb 14, 2014 at 12:05 PM, Andres Freund and...@2ndquadrant.com wrote: There's no reason not to ask for a ping when we're writing. Is there a reason to ask for a ping? The point of keepalives is to ensure there's some traffic on idle connections so that if the connection is dead it doesn't linger forever and so that any on-demand links (or more recently NAT routers or stateful firewalls) don't time out and disconnect and have to reconnect (or more recently just fail outright). This ain't TCP keepalives. The reason is that we want to kill walsenders if they haven't responded to a ping inside wal_sender_timeout. That's rather important e.g. for sychronous replication, so we can quickly fall over to the next standby. In such scenarios you'll usually want a timeout *far* below anything TCP provides. walreceiver sends pings everytime it receives a 'w' message, so it's probably not an issue there, but pg_receivexlog/basebackup don't; they use their own configured intervarl. So this might be an explanation of the latter two being disconnected too early. I've seen reports of that... 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] Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease
On Feb14, 2014, at 14:07 , Andres Freund and...@2ndquadrant.com wrote: On 2014-02-14 13:52:45 +0100, Florian Pflug wrote: I agree we should do that, but imo not in the backbranches. Anything more than than the minimal fix in that code should be avoided in the stable branches, this stuff is friggin performance sensitive, and the spinlock already is a *major* contention point in many workloads. No argument there. But unless I missed something, there actually is a bug there, and using volatile won't fix it. A barrier would, but what about the back branches that don't have barrier.h? Yea, but I don't see a better alternative. Realistically the likelihood of a problem without the compiler reordering stuff is miniscule on any relevant platform that's supported by the !barrier.h branches. Newer ARMs are the only realistic suspect, and the support for in older releases wasn't so good... Isn't POWER/PowerPC potentially affected by this? Also, there is still the alternative of not resetting lwWaitLink in LWLockRelease. I can understand why you oppose that - it's certainly nicer to not have stray pointer lying around. But since (as least as far as we know) a) resetting lwWaitLink is not strictly necessary b) resetting lwWaitLink introduces a race condition (however unlikely) we'll trade correctness for cleanliness if we continue to reset lwWaitLink without protecting against the race. That's a bit of a weird trade-off to make. Another idea for a fix would be to conflate lwWaiting and lwWaitLink into one field. We could replace lwWaiting by lwWaitLink != NULL everywhere it's tested, and set lwWaitLink to some special non-NULL value (say 0x1) when we enqueue a PGPROC, instead of setting it to NULL and setting lwWaiting to true. We'd then depend on pointer-sized stores being atomic, which I think we depend on in other places already. best regards, Florian Pflug -- 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] New hook after raw parsing, before analyze
Let me rephrase this: Let’s remove my motivations and use cases from this conversation…. Why is that a bad idea of rewriting the query before it reaches transform/analyze (without ever accessing the catalog)? If that flexibility is acceptable to you, where would be the best place to put it in? Thanks, David 2014.02.14. dátummal, 10:30 időpontban David Beck db...@starschema.net írta: Thanks for the reply. There are two things I think I’ve been misunderstood: 1, the point is to do the rewrite without and before catalog access 2, I do want to push the join to the source and equally important pushing the where conditions there Best regards, David 2014.02.13. dátummal, 21:22 időpontban Tom Lane t...@sss.pgh.pa.us írta: David Beck db...@starschema.net writes: I have table like data structures in the source system for the FDW I work on. These tables are sometimes too big and the source system is able to filter and join them with limitations, thus it is not optimal to transfer the data to Postgres. At the same time I want the users to think in terms of the original tables. The idea is to rewrite the SQL queries like this: “SELECT * FROM tableA a, tableB b WHERE a.id=b.id AND a.col1=1234 AND b.col2=987” to: “SELECT * FROM fdw_tableA_tableB ab WHERE ab.col1=1234 AND ab.col2=987” TBH this sounds like a spectacularly bad idea, especially in the place and way you propose to do it. You can't even do catalog access safely where you've put that hook, not to mention that there are many other places where queries can be submitted. But more generally, an FDW should not operate in the way you're describing. We do lack support for pushing joins to the foreign server, and that needs to be addressed; but we need to do it in the planner, not by kluging the query somewhere upstream of that. 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] HBA files w/include support?
On Thu, Feb 13, 2014 at 11:28:45PM -0600, Jerry Sievers wrote: One issue with this is that pg_hba.conf is order sensitive, which could become a trap for the unwary if includes are used carelessly. Indeed. The other thing that comes to mind, is that as opposed to postgresql.conf and the include scenario there... one can do show all or query from pg_stat_activity just to see what setting they ended up with. I'm not aware of any way to probe what hba rules are loaded at runtime and thus, debugging hba config changes not really possible. In an ideal world we would have a tool where you could plug in a username, database, IP address, and test pg_hba.conf file and it would report what line is matched. I presume that a simple scenario involving just 1 level of includes not too difficult to grok but nested includes sure might be a foot gun unless there was a way to dump the resulting configs somehow. Thus pasting hba files together externally a more reliable approach. You certainly would not have a visual idea of what line is matched _first_. We have the same problem with postgresql.conf includes, though the last match wins there --- not sure if that makes it any easier. I think one concern is that pg_hba.conf is more security-oriented than postgresql.conf. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] HBA files w/include support?
On Fri, Feb 14, 2014 at 03:28:23AM -0500, Stephen Frost wrote: Bruce, * Bruce Momjian (br...@momjian.us) wrote: On Thu, Feb 13, 2014 at 08:24:27PM -0600, Jerry Sievers wrote: I'm aware of how a pg_hba.conf file can refer to other files for including @lists of users, etc. But there is currently no support for being able to pull in entire file segments as can be done for postgresql.conf via the include directive. In the environment that I'm managing, we are using a makefile to stick together a common header with a custom section for any of several clusters and may extend this further to permit additional includes for hba rules common to groupings of clusters. Anyway, please advise. I don't recall hearing anything like this discussed. Has been proposed, discussed and voted down? Or never mentioned? I have never heard of anyone request this. I've brought it up on various threads before, including both the ALTER SYSTEM thread and the postgresql.conf 'includes' thread, though I don't feel like going back and hunting down the specific emails right now. Having @include and directory.d-style capabilities for pg_hba.conf *and* pg_ident.conf would make managing larger environments much better. There has been some talk about providing those capabilities via tables in the catalog, but I'm not aware of anyone working on it and it'd certainly be quite a bit more work than adding include/dir.d options. Do we want a TODO for this? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] HBA files w/include support?
* Bruce Momjian (br...@momjian.us) wrote: On Fri, Feb 14, 2014 at 03:28:23AM -0500, Stephen Frost wrote: Having @include and directory.d-style capabilities for pg_hba.conf *and* pg_ident.conf would make managing larger environments much better. There has been some talk about providing those capabilities via tables in the catalog, but I'm not aware of anyone working on it and it'd certainly be quite a bit more work than adding include/dir.d options. Do we want a TODO for this? Sure, and it should be a candidate for GSoC, imv... Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] HBA files w/include support?
On Fri, Feb 14, 2014 at 3:32 PM, Bruce Momjian br...@momjian.us wrote: On Thu, Feb 13, 2014 at 11:28:45PM -0600, Jerry Sievers wrote: One issue with this is that pg_hba.conf is order sensitive, which could become a trap for the unwary if includes are used carelessly. Indeed. The other thing that comes to mind, is that as opposed to postgresql.conf and the include scenario there... one can do show all or query from pg_stat_activity just to see what setting they ended up with. I'm not aware of any way to probe what hba rules are loaded at runtime and thus, debugging hba config changes not really possible. In an ideal world we would have a tool where you could plug in a username, database, IP address, and test pg_hba.conf file and it would report what line is matched. I almost wrote a function you could call to do that a while back. I never finished it though :) It's not all that hard to do, but requires some minor refactoring of how the HBA code works. What would also be useful would be to be able to use such a function/tool against a different file than the current HBA one, to verify *before* you reload... -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] HBA files w/include support?
* Bruce Momjian (br...@momjian.us) wrote: In an ideal world we would have a tool where you could plug in a username, database, IP address, and test pg_hba.conf file and it would report what line is matched. That's not a bad idea, but we don't expose the logic that figures that out today.. It would, perhaps, not be horrible to duplicate it, but then we'd need to make sure that we update both places if it ever changes (not that it's changed much in oh-so-many-years). Perhaps another candidate to be a GSoC project. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] HBA files w/include support?
On Fri, Feb 14, 2014 at 09:34:59AM -0500, Stephen Frost wrote: * Bruce Momjian (br...@momjian.us) wrote: On Fri, Feb 14, 2014 at 03:28:23AM -0500, Stephen Frost wrote: Having @include and directory.d-style capabilities for pg_hba.conf *and* pg_ident.conf would make managing larger environments much better. There has been some talk about providing those capabilities via tables in the catalog, but I'm not aware of anyone working on it and it'd certainly be quite a bit more work than adding include/dir.d options. Do we want a TODO for this? Sure, and it should be a candidate for GSoC, imv... OK, added. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] HBA files w/include support?
On Fri, Feb 14, 2014 at 12:36 PM, Stephen Frost sfr...@snowman.net wrote: * Bruce Momjian (br...@momjian.us) wrote: In an ideal world we would have a tool where you could plug in a username, database, IP address, and test pg_hba.conf file and it would report what line is matched. That's not a bad idea, but we don't expose the logic that figures that out today.. It would, perhaps, not be horrible to duplicate it, but then we'd need to make sure that we update both places if it ever changes (not that it's changed much in oh-so-many-years). Perhaps another candidate to be a GSoC project. I would like to participate the GSoC this year as a student. Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello
Re: [HACKERS] HBA files w/include support?
Stephen Frost sfr...@snowman.net writes: Having @include and directory.d-style capabilities for pg_hba.conf *and* pg_ident.conf would make managing larger environments much better. I'm a little suspicious of this, mainly because pg_hba searching is necessarily linear (and none too cheap per-entry). I think anyone who tries to use a set of entries large enough to really need multiple files is going to have pain. We already have various methods for making one pg_hba entry do the work of many; for instance, IP-subnet entries, wildcards, and role references. And you can use database CONNECT privilege grants as another substitute for fine-grained pg_hba entries. I'd be interested to see a real use-case where those things aren't an adequate substitute for a pg_hba rule set that's too large to fit conveniently in one file. Maybe we could identify another pg_hba abstraction technique we need to support. In short: I suspect this approach may be fixing the wrong thing. 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] Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease
Florian Pflug f...@phlo.org writes: Another idea for a fix would be to conflate lwWaiting and lwWaitLink into one field. We could replace lwWaiting by lwWaitLink != NULL everywhere it's tested, and set lwWaitLink to some special non-NULL value (say 0x1) when we enqueue a PGPROC, instead of setting it to NULL and setting lwWaiting to true. We'd then depend on pointer-sized stores being atomic, which I think we depend on in other places already. I don't believe that's true; neither that we depend on it now, nor that it would be safe to do so. 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] HBA files w/include support?
On 2014-02-14 10:19:30 -0500, Tom Lane wrote: Stephen Frost sfr...@snowman.net writes: Having @include and directory.d-style capabilities for pg_hba.conf *and* pg_ident.conf would make managing larger environments much better. I'm a little suspicious of this, mainly because pg_hba searching is necessarily linear (and none too cheap per-entry). I think anyone who tries to use a set of entries large enough to really need multiple files is going to have pain. I don't think big hba files should be the primary motivator for a feature like this. What I have seen is that people want a couple entries that grant local access to maintenance scripts and root/postgres. Additionally they want to have a *few* for every application that runs on the same cluster and another additional couple of entries allowing access to a limited number of replicas. It's not too hard to script this, simply have a pg_bha.d directory whose contents are concatenated in an alphabetic order resulting in a combined file. But that's clearly inferior to builtin support for that because the error reporting obviously won't be as good and it's not possible to nicely hook this into reloads and such. I wonder if the linear search could be improved a bit by separating database specific entries from database independent ones as the setups with large number of entries mostly had lots of database specific rules. It would probably be nontrivial to guarantee the sequential lookup order, but it should be possible. I'd be interested to see a real use-case where those things aren't an adequate substitute for a pg_hba rule set that's too large to fit conveniently in one file. Maybe we could identify another pg_hba abstraction technique we need to support. That additionally very well might be. The biggest thing I have seen is a) the inability to restrict access based on outside group membership when using ldap, b) the inability to grant membership in groups from outside of pg. Greetings, Andres Freund -- 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] Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease
On 2014-02-14 10:26:07 -0500, Tom Lane wrote: Florian Pflug f...@phlo.org writes: Another idea for a fix would be to conflate lwWaiting and lwWaitLink into one field. We could replace lwWaiting by lwWaitLink != NULL everywhere it's tested, and set lwWaitLink to some special non-NULL value (say 0x1) when we enqueue a PGPROC, instead of setting it to NULL and setting lwWaiting to true. We'd then depend on pointer-sized stores being atomic, which I think we depend on in other places already. I don't believe that's true; neither that we depend on it now, nor that it would be safe to do so. Yea, we currently rely on 4 byte stores being atomic (most notably for xids), but we don't rely on anything bigger. I am not sure if there are architectures with 64bit pointers that aren't accessed atomically when aligned? Even if, that's certainly nothing that should be introduced when backpatching. 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] Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease
On Feb14, 2014, at 16:32 , Andres Freund and...@2ndquadrant.com wrote: On 2014-02-14 10:26:07 -0500, Tom Lane wrote: Florian Pflug f...@phlo.org writes: Another idea for a fix would be to conflate lwWaiting and lwWaitLink into one field. We could replace lwWaiting by lwWaitLink != NULL everywhere it's tested, and set lwWaitLink to some special non-NULL value (say 0x1) when we enqueue a PGPROC, instead of setting it to NULL and setting lwWaiting to true. We'd then depend on pointer-sized stores being atomic, which I think we depend on in other places already. I don't believe that's true; neither that we depend on it now, nor that it would be safe to do so. Yea, we currently rely on 4 byte stores being atomic (most notably for xids), but we don't rely on anything bigger. I am not sure if there are architectures with 64bit pointers that aren't accessed atomically when aligned? Even if, that's certainly nothing that should be introduced when backpatching. Hm, we could use 4-byte stores instead of 8-byte stores if we turned lwWaitLink into an index into the proc array instead of a pointer to the PGPROC struct. We could then use 0x instead place of NULL to indicate not waiting, and PROCARRAY_MAXPROCS to indicate waiting, but no successor in the queue. best regards, Florian Pflug -- 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] narwhal and PGDLLIMPORT
Hiroshi Inoue in...@tpf.co.jp writes: (2014/02/12 8:30), Tom Lane wrote: Not very clear what's going on there; could this be a problem in narwhal's admittedly-ancient toolchain? The error doesn't occur here (un)fortunately. One thing I'm wondering about is that plperl is linking perlxx.lib not libperlxx.a. I made a patch following plpython and it also works here. Is it worth trying? I hadn't noticed that part of plpython's Makefile before. Man, that's an ugly technique :-(. Still, there's little about this platform that isn't ugly. Let's try it and see what happens. 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] Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease
On 2014-02-14 15:03:16 +0100, Florian Pflug wrote: On Feb14, 2014, at 14:07 , Andres Freund and...@2ndquadrant.com wrote: On 2014-02-14 13:52:45 +0100, Florian Pflug wrote: I agree we should do that, but imo not in the backbranches. Anything more than than the minimal fix in that code should be avoided in the stable branches, this stuff is friggin performance sensitive, and the spinlock already is a *major* contention point in many workloads. No argument there. But unless I missed something, there actually is a bug there, and using volatile won't fix it. A barrier would, but what about the back branches that don't have barrier.h? Yea, but I don't see a better alternative. Realistically the likelihood of a problem without the compiler reordering stuff is miniscule on any relevant platform that's supported by the !barrier.h branches. Newer ARMs are the only realistic suspect, and the support for in older releases wasn't so good... Isn't POWER/PowerPC potentially affected by this? I wouldn't consider it a major architecture... And I am not sure how much out of order a CPU has to be to be affected by this, the read side uses spinlocks, which in most of the spinlock implementations implies a full memory barrier which in many cache coherency designs will cause other CPUs to flush writes. And I think the control dependency in the PGSemaphoreUnlock() loop will actually cause a flush on ppc... Also, there is still the alternative of not resetting lwWaitLink in LWLockRelease. I can understand why you oppose that - it's certainly nicer to not have stray pointer lying around. But since (as least as far as we know) a) resetting lwWaitLink is not strictly necessary I don't want to rely on that in the backbranches, that's an entirely new assumption. I think anything more than minimal changes are hard to justify for a theoretical issue that hasn't been observed in the field. I think the biggest danger here is that writes are reordered by the compiler, that we definitely need to protect against. Making a variable volatile or introducing a memory barrier is pretty simple to analyze. b) resetting lwWaitLink introduces a race condition (however unlikely) we'll trade correctness for cleanliness if we continue to reset lwWaitLink without protecting against the race. That's a bit of a weird trade-off to make. It's not just cleanliness, it's being able to actually debug crashes. 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] HBA files w/include support?
* Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@snowman.net writes: Having @include and directory.d-style capabilities for pg_hba.conf *and* pg_ident.conf would make managing larger environments much better. I'm a little suspicious of this, mainly because pg_hba searching is necessarily linear (and none too cheap per-entry). I think anyone who tries to use a set of entries large enough to really need multiple files is going to have pain. It would depend on the connection rate- some of these environments have low connection rate but quite a few users (eg: corporate database server). As expressed up-thread, and in my own experience, people are already doing this by building up the files themselves, so I don't agree that performance would be so bad that this isn't worth it. Not everyone is using PG to back their website. Improving performance would always be good too, or perhaps providing another mechanism which performs better when there are more entries. We already have various methods for making one pg_hba entry do the work of many; for instance, IP-subnet entries, wildcards, and role references. And you can use database CONNECT privilege grants as another substitute for fine-grained pg_hba entries. Yes, but those entries all have to agree on the exact authentication method, including any mappings or other options which are passed through pg_hba.conf. Also, CONNECT is only true/false, it doesn't have any ability to consider source-IP, SSL, etc. I'd be interested to see a real use-case where those things aren't an adequate substitute for a pg_hba rule set that's too large to fit conveniently in one file. Maybe we could identify another pg_hba abstraction technique we need to support. Being able to 'fit' inside of one file is wholly in the eye of the beholder and isn't terribly relevant either. Larger installations will be using puppet, chef, or one of the other configuration management systems and will want to use even small/independent files rather than having to write code which fools around with the one file approach. The files I have in /etc/cron.d/ aren't particularly large, but it's really nice to be able to independently manage and deploy them without templating them. Also, all of the above ignores the pg_ident side of the house, which is even worse as you need an entry for every user, period, if you're using client-side SSL certificates or Kerberos/GSSAPI-based authentication with full princ names. In short: I suspect this approach may be fixing the wrong thing. I'm curious what you're thinking would be the right thing to fix here? Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] HBA files w/include support?
On 2014-02-14 11:03:19 -0500, Stephen Frost wrote: Also, all of the above ignores the pg_ident side of the house, which is even worse as you need an entry for every user, period, if you're using client-side SSL certificates or Kerberos/GSSAPI-based authentication with full princ names. Well, there's regexes for mappings, that can often enough take care of most of that? 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] HBA files w/include support?
* Andres Freund (and...@2ndquadrant.com) wrote: On 2014-02-14 11:03:19 -0500, Stephen Frost wrote: Also, all of the above ignores the pg_ident side of the house, which is even worse as you need an entry for every user, period, if you're using client-side SSL certificates or Kerberos/GSSAPI-based authentication with full princ names. Well, there's regexes for mappings, that can often enough take care of most of that? In some cases, yes, but certainly not all. Apologies for over-stating the case, but I came from an environment where the Kerberos princs were 'm##', while the database users were all first-initial || last-name. Also, the CN in an SSL certificate isn't likely to be what you want for a username either, and a regexp isn't likely to help that either. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] HBA files w/include support?
Stephen Frost sfr...@snowman.net writes: * Tom Lane (t...@sss.pgh.pa.us) wrote: In short: I suspect this approach may be fixing the wrong thing. I'm curious what you're thinking would be the right thing to fix here? I was asking for use-cases so we could figure out what's the right thing ;-) The argument about wanting to assemble a pg_hba file from separately managed configuration pieces seems to have some merit, but the weak spot there is how do you define the search order? Or are you planning to just cross your fingers and hope it doesn't matter too much? 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] HBA files w/include support?
On 2014-02-14 11:10:48 -0500, Tom Lane wrote: Stephen Frost sfr...@snowman.net writes: * Tom Lane (t...@sss.pgh.pa.us) wrote: In short: I suspect this approach may be fixing the wrong thing. I'm curious what you're thinking would be the right thing to fix here? I was asking for use-cases so we could figure out what's the right thing ;-) The argument about wanting to assemble a pg_hba file from separately managed configuration pieces seems to have some merit, but the weak spot there is how do you define the search order? Or are you planning to just cross your fingers and hope it doesn't matter too much? The usual solution is to prepend a numeric prefix guaranteeing the search order. 00 is sysadmin stuff, 10 replication, 20 database specific or somesuch. I think most admins using automated tools to manage bigger configuration files by using some .d config directory already know how to deal with that problem. 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] Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease
On 02/14/2014 07:51 PM, Andres Freund wrote: On 2014-02-14 15:03:16 +0100, Florian Pflug wrote: On Feb14, 2014, at 14:07 , Andres Freund and...@2ndquadrant.com wrote: On 2014-02-14 13:52:45 +0100, Florian Pflug wrote: I agree we should do that, but imo not in the backbranches. Anything more than than the minimal fix in that code should be avoided in the stable branches, this stuff is friggin performance sensitive, and the spinlock already is a *major* contention point in many workloads. No argument there. But unless I missed something, there actually is a bug there, and using volatile won't fix it. A barrier would, but what about the back branches that don't have barrier.h? Yea, but I don't see a better alternative. Realistically the likelihood of a problem without the compiler reordering stuff is miniscule on any relevant platform that's supported by the !barrier.h branches. Newer ARMs are the only realistic suspect, and the support for in older releases wasn't so good... Isn't POWER/PowerPC potentially affected by this? I wouldn't consider it a major architecture... And I am not sure how much out of order a CPU has to be to be affected by this, the read side uses spinlocks, which in most of the spinlock implementations implies a full memory barrier which in many cache coherency designs will cause other CPUs to flush writes. And I think the control dependency in the PGSemaphoreUnlock() loop will actually cause a flush on ppc... PGSemaphoreUnlock() should really introduce memory barrier, but the problem can happen before PGSemaphoreUnlock() is called. So presence of PGSemaphoreUnlock() just reduces probability of race condition on non-TSO architectures (PPC, ARM, IA64,...) but doesn't completely eliminate it. Also, there is still the alternative of not resetting lwWaitLink in LWLockRelease. I can understand why you oppose that - it's certainly nicer to not have stray pointer lying around. But since (as least as far as we know) a) resetting lwWaitLink is not strictly necessary I don't want to rely on that in the backbranches, that's an entirely new assumption. I think anything more than minimal changes are hard to justify for a theoretical issue that hasn't been observed in the field. I think the biggest danger here is that writes are reordered by the compiler, that we definitely need to protect against. Making a variable volatile or introducing a memory barrier is pretty simple to analyze. b) resetting lwWaitLink introduces a race condition (however unlikely) we'll trade correctness for cleanliness if we continue to reset lwWaitLink without protecting against the race. That's a bit of a weird trade-off to make. It's not just cleanliness, it's being able to actually debug crashes. Frankly speaking I do not understand why elimination of resetting of lwWaitLink was considered to be bad idea. Resetting this pointer to NULL will not help much to analyze crash dumps, because right now it is possible that lwWaitLink==NULL but process in waiting for a lock and linked in the list (if it is the last element of the list). So the fact that lwWaitLink==NULL actually gives not so much useful information. Greetings, Andres Freund -- 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] Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease
On 2014-02-14 20:23:32 +0400, knizhnik wrote: we'll trade correctness for cleanliness if we continue to reset lwWaitLink without protecting against the race. That's a bit of a weird trade-off to make. It's not just cleanliness, it's being able to actually debug crashes. Frankly speaking I do not understand why elimination of resetting of lwWaitLink was considered to be bad idea. Resetting this pointer to NULL will not help much to analyze crash dumps, because right now it is possible that lwWaitLink==NULL but process in waiting for a lock and linked in the list (if it is the last element of the list). So the fact that lwWaitLink==NULL actually gives not so much useful information. At the moment if you connect to a live pg or a crash dump, investigating the wait links allows you to somewhat sensibly determine which backends are waiting for a lock and whether they are part of a queue. If they aren't reset anymore that will tell you nothing, so you'll need to connect to all pg instances to debug issues. 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] Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease
On 02/14/2014 08:28 PM, Andres Freund wrote:
On 2014-02-14 20:23:32 +0400, knizhnik wrote:
we'll trade correctness for cleanliness if we continue to reset lwWaitLink
without protecting against the race. That's a bit of a weird trade-off to make.
It's not just cleanliness, it's being able to actually debug crashes.
Frankly speaking I do not understand why elimination of resetting of lwWaitLink
was considered to be bad idea.
Resetting this pointer to NULL will not help much to analyze crash dumps,
because right now it is possible that lwWaitLink==NULL but process in waiting
for a lock and linked in the list
(if it is the last element of the list). So the fact that lwWaitLink==NULL
actually gives not so much useful information.
At the moment if you connect to a live pg or a crash dump, investigating
the wait links allows you to somewhat sensibly determine which backends
are waiting for a lock and whether they are part of a queue. If they
aren't reset anymore that will tell you nothing, so you'll need to
connect to all pg instances to debug issues.
Why it is not enough to inspect lwWaiting flag?
In any case, resetting lwWaitLink can be safely done in awakened process:
if (!proc-lwWaiting) {
proc-lwWaitLink = NULL;
break;
}
Greetings,
Andres Freund
--
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] New hook after raw parsing, before analyze
On Fri, Feb 14, 2014 at 2:28 PM, David Beck db...@starschema.net wrote: Why is that a bad idea of rewriting the query before it reaches transform/analyze (without ever accessing the catalog)? If that flexibility is acceptable to you, where would be the best place to put it in? Well if there are two foreign tables and the planner could push the join work down to the fdw then the planner should be able to accurately represent that plan and cost it without having the user have to create any catalog structures. That's what the planner does for every other type of plan node. What you're describing would still be useful for materialized views. In that case the user is creating the materialized view and it is a real thing in the catalogs that won't disappear on the planner. Even then it would be ideal if the planner could decide to use the materialized view late enough that it can actually determine if it's superior rather than rewriting the query before it gets to that point. That would be much more flexible for users too who might not write the query in a way that exactly matches the materialized view. -- 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] HBA files w/include support?
* Andres Freund (and...@2ndquadrant.com) wrote: On 2014-02-14 11:10:48 -0500, Tom Lane wrote: Stephen Frost sfr...@snowman.net writes: * Tom Lane (t...@sss.pgh.pa.us) wrote: In short: I suspect this approach may be fixing the wrong thing. I'm curious what you're thinking would be the right thing to fix here? I was asking for use-cases so we could figure out what's the right thing ;-) The specific use-cases that I've seen are primairly around managing the sets of users who are allowed to connect from specific subnets. As for how the files might be independently managed, as Andres pointed out, all systems would get a 'default' set which includes access for the local 'postgres' unix user, and then certain sets of systems would get appropriate combinations of: system-specific admin users (from anywhere) normal users for that environment (from the desktop network) application users for app servers (from the app servers) ETL users for batch systems (from the ETL servers) sales users for the *replica* (whose CONNECT privs can't be different..) etc.. Would it be possible to have those all in one pg_hba file? Sure, in most cases, and you'd trust that the accounts simply wouldn't exist or that they wouldn't have CONNECT privs on the database, but this isn't how I run my systems (entries in pg_hba for users that don't exist?) and I doubt I'm alone in that. Being able to enforce based on source-IP is *very* nice and is only available in pg_hba today. Also, as noted above, if you're running a replica that should have different access rights (in particular, more 'open') then you can't depend on CONNECT privs or account authentication (username/password differences) to control access since that's all in the catalog and therefore must be identical to the primary. The argument about wanting to assemble a pg_hba file from separately managed configuration pieces seems to have some merit, but the weak spot there is how do you define the search order? Or are you planning to just cross your fingers and hope it doesn't matter too much? The usual solution is to prepend a numeric prefix guaranteeing the search order. 00 is sysadmin stuff, 10 replication, 20 database specific or somesuch. I think most admins using automated tools to manage bigger configuration files by using some .d config directory already know how to deal with that problem. Indeed, and run-parts codified it as 'lexical sort order (according to the C/POSIX locale)', under at least Debian and Ubuntu systems. I'm pretty confident that most admins are going to be familiar with handling this issue as it's exactly how /etc/cron.daily and anything else called through run-parts works. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] narwhal and PGDLLIMPORT
I wrote: Hiroshi Inoue in...@tpf.co.jp writes: One thing I'm wondering about is that plperl is linking perlxx.lib not libperlxx.a. I made a patch following plpython and it also works here. Is it worth trying? I hadn't noticed that part of plpython's Makefile before. Man, that's an ugly technique :-(. Still, there's little about this platform that isn't ugly. Let's try it and see what happens. And what happens is this: http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=narwhaldt=2014-02-14%2017%3A00%3A02 namely, it gets through plperl now and then chokes with the same symptoms on pltcl. So I guess we need the same hack in pltcl. The fun never stops ... (BTW, narwhal is evidently not trying to build plpython. I wonder why not?) 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] HBA files w/include support?
Tom Lane t...@sss.pgh.pa.us writes: Stephen Frost sfr...@snowman.net writes: * Tom Lane (t...@sss.pgh.pa.us) wrote: In short: I suspect this approach may be fixing the wrong thing. I'm curious what you're thinking would be the right thing to fix here? I was asking for use-cases so we could figure out what's the right thing ;-) No doubt this environment I took over managing, of 90 clusters ranging from tiny to ~3.5TB, some with hundreds of user accounts... could stand some massive rejiggering in regards to users/rols and such to go towards doing it the right way. That said, trying to roll up the hba files some with over 300 entries and lots of cases of high duplication among clusters belonging to somne subset, would be daunting and perhaps invasive. I realize that gathering up those duplicates into a file common to whatever group and then having that pulled in as an include is going to result in some reordering of the rules since they are not in a totally predictable order presently And my Jr. DBAs are still handling requests daily to add more hba rules quite often to more than a single machine but still along the same groupings mentioned. Even without retrofitting a good cleanup here, being able to include a global section at top of all files and at least one group specific file next then followed by legacy entries and/or items specific to a single cluster, might save a lot of work. Thus my idea of using a make file for this and then inspiring the question about includes. Great just getting the ball rolling and I respect whatever the concensus opinion that emerges. Thx The argument about wanting to assemble a pg_hba file from separately managed configuration pieces seems to have some merit, but the weak spot there is how do you define the search order? Or are you planning to just cross your fingers and hope it doesn't matter too much? regards, tom lane -- Jerry Sievers e: jerry.siev...@comcast.net p: 312.241.7800 -- 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] Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease
On Feb14, 2014, at 16:51 , Andres Freund and...@2ndquadrant.com wrote: On 2014-02-14 15:03:16 +0100, Florian Pflug wrote: On Feb14, 2014, at 14:07 , Andres Freund and...@2ndquadrant.com wrote: On 2014-02-14 13:52:45 +0100, Florian Pflug wrote: I agree we should do that, but imo not in the backbranches. Anything more than than the minimal fix in that code should be avoided in the stable branches, this stuff is friggin performance sensitive, and the spinlock already is a *major* contention point in many workloads. No argument there. But unless I missed something, there actually is a bug there, and using volatile won't fix it. A barrier would, but what about the back branches that don't have barrier.h? Yea, but I don't see a better alternative. Realistically the likelihood of a problem without the compiler reordering stuff is miniscule on any relevant platform that's supported by the !barrier.h branches. Newer ARMs are the only realistic suspect, and the support for in older releases wasn't so good... Isn't POWER/PowerPC potentially affected by this? I wouldn't consider it a major architecture... And I am not sure how much out of order a CPU has to be to be affected by this, the read side uses spinlocks, which in most of the spinlock implementations implies a full memory barrier which in many cache coherency designs will cause other CPUs to flush writes. And I think the control dependency in the PGSemaphoreUnlock() loop will actually cause a flush on ppc... I guess it's sufficient for the store to lwWaitLink to be delayed. As long as the CPU on which that store is executing hasn't managed to gain exclusive access to the relevant cache line, memory barriers on the read side won't help because the store won't be visible to other CPUs. Also, there is still the alternative of not resetting lwWaitLink in LWLockRelease. I can understand why you oppose that - it's certainly nicer to not have stray pointer lying around. But since (as least as far as we know) a) resetting lwWaitLink is not strictly necessary I don't want to rely on that in the backbranches, that's an entirely new assumption. I think anything more than minimal changes are hard to justify for a theoretical issue that hasn't been observed in the field. I think the biggest danger here is that writes are reordered by the compiler, that we definitely need to protect against. Making a variable volatile or introducing a memory barrier is pretty simple to analyze. Well, the assumption isn't all that new. We already have the situation that a PGPROC may be not on any wait queue, yet its lwWaitLink may be non-NULL. Currently, the process who took it off the queue is responsible for rectifying that eventually, with the changed proposed below it'll be the backend who owns the PGPROC. From the point of view of other backends, nothing really changes. b) resetting lwWaitLink introduces a race condition (however unlikely) we'll trade correctness for cleanliness if we continue to reset lwWaitLink without protecting against the race. That's a bit of a weird trade-off to make. It's not just cleanliness, it's being able to actually debug crashes. We could still arrange for the stray lwWaitLink from being visible only momentarily. If a backend is woken up and detects that lwWaiting is false, it knows that it cannot be on any wait queue - it was just removed, and hasn't added itself again yet. At that point, it's safe to reset lwWaitLink back to NULL. The stray value would thus only be visible while a backend executes the PGSemaphoreLock() loop, and whether or not this is the case can be deduced from a stack trace. So I'm not convinced that this makes debugging any harder. (knizhnik has just suggested the same) It's interesting, BTW, that updating lwWaitLink after lwWaiting is OK here - the crucial property is not that it's updated before lwWaiting, but rather that it is reset before enqueuing the backend again. Which is something that backend itself can guarantee far more easily than whoever happens to de-queue it due to spurious wakeups. best regards, Florian Pflug -- 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] git-review: linking commits to review discussion in git
On 2014-01-28 07:10:45 Heikki Linnakangas hlinnakan...@vmware.com wrote: On 01/27/2014 11:36 PM, Murtuza Mukadam wrote: Hello All, We have linked peer review discussions on 'pgsql-hackers' to their respective commits within the main postgresql.git repository. You can view the linked reviews from 2012 until present in the GitHub repo at https://github.com/mmukadam/postgres/tree/review [snip] I don't understand what this does. The repository at https://github.com/mmukadam/postgres looks like just a clone of the main PostgreSQL repository, with no extra links anywhere. And the repository at https://github.com/mmukadam/postgres/tree/review looks like a mailing list archive turned into a git repository, but I don't see any links to the commits in the main repository there. Am I missing something? If you want to search for the reviews related to a commit, you can type in the full hash code here: http://users.encs.concordia.ca/~m_mukada/git-review-tracker.html or append the commit hash to the following url: https://github.com/mmukadam/postgres/tree/review/commit_hash It's a bit hard to view the reviews on GitHub because the 'review' branch was designed to be viewed on the command line using a set of simple scripts that sit on top of the standard git commands. For example, to view all reviews Heikki is involved in you can do: git review --log-reviewer heikki to view the reviews of particular commit: git review --log 1a1832eb085e5bca198735e5d0e766a3cb61b8fc You can also create new reviews on a commit and mail them as well as import reviews from email. The tool is beta, but it just creates a detached review branch, so it won't affect master. To remove it all you do is delete the scripts and the 'review' branch. More info available here: Online man page: http://users.encs.concordia.ca/~m_mukada/git-review.html Tutorial: http://users.encs.concordia.ca/~m_mukada/git-review-tutorial.html Installation: http://users.encs.concordia.ca/~m_mukada/git-review-tutorial.html#Install Cheers, Peter PS this is part of Murtuza's thesis, so any feedback is appreciated. If you'd like us to extract reviews from another mailing list or repo, please let us know Example output: git review --log-reviewer heikki Review: adfe85d4dd1844c1f09401d2d970f564d0dac61d Commit Reviewed: 593a9631a7947ab95903e87e24786d7e469cc988 Author: Tom Lane t...@sss.pgh.pa.us AuthorDate: Tue Feb 21 15:03:36 2012 -0500 Reviewer: Heikki Linnakangas hlinnakan...@vmware.com hlinnakan...@vmware.com ReviewDate: Mon Jan 27 16:34:44 2014 +0200 Re: Race condition in b-tree page deletion Review: e7bfe4323d2b83605df83174f11785c117ad3a53 Commit Reviewed: 56a57473a999b0497e63bde3e303beda5a3c0ff3 Author: Tom Lane t...@sss.pgh.pa.us AuthorDate: Sat Jan 8 14:47:13 2011 -0500 Reviewer: Heikki Linnakangas hlinnakan...@vmware.com hlinnakan...@vmware.com ReviewDate: Sat Jan 25 23:21:20 2014 +0200 Re: GIN improvements part2: fast scan -- http://users.encs.concordia.ca/~pcr/ -- 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] Release schedule for 9.3.3?
All, Do the 9.3.3 replication fixes mean that users should reclone their replicas, like 9.3.2 did? Or not? -- 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] Release schedule for 9.3.3?
Hi, On 2014-02-14 13:08:34 -0500, Josh Berkus wrote: Do the 9.3.3 replication fixes mean that users should reclone their replicas, like 9.3.2 did? Or not? Which replication replication fixes are you referring to? http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=ebde6c40148c9f811f7c6d35f67e7ea3ce2d9b34 ? If so, no, that doesn't require a reclone. 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] Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease
On 2014-02-14 18:49:33 +0100, Florian Pflug wrote: I wouldn't consider it a major architecture... And I am not sure how much out of order a CPU has to be to be affected by this, the read side uses spinlocks, which in most of the spinlock implementations implies a full memory barrier which in many cache coherency designs will cause other CPUs to flush writes. And I think the control dependency in the PGSemaphoreUnlock() loop will actually cause a flush on ppc... I guess it's sufficient for the store to lwWaitLink to be delayed. As long as the CPU on which that store is executing hasn't managed to gain exclusive access to the relevant cache line, memory barriers on the read side won't help because the store won't be visible to other CPUs. The whole lwlock actually should be on the same cacheline on anything with cachelines = 32. As the woken up side is doing an atomic op on it *before* modifying lwWaitLink I think we are actually guaranteed that any incomplete store on the writer will have completed unless the compiler reordered. So we are safe against out of order stores, but not out of order execution. That might have been what prevented the issue from being noticable on some platforms. Well, the assumption isn't all that new. We already have the situation that a PGPROC may be not on any wait queue, yet its lwWaitLink may be non-NULL. Currently, the process who took it off the queue is responsible for rectifying that eventually, with the changed proposed below it'll be the backend who owns the PGPROC. From the point of view of other backends, nothing really changes. That window is absolutely tiny tho. b) resetting lwWaitLink introduces a race condition (however unlikely) we'll trade correctness for cleanliness if we continue to reset lwWaitLink without protecting against the race. That's a bit of a weird trade-off to make. It's not just cleanliness, it's being able to actually debug crashes. We could still arrange for the stray lwWaitLink from being visible only momentarily. If a backend is woken up and detects that lwWaiting is false, it knows that it cannot be on any wait queue - it was just removed, and hasn't added itself again yet. At that point, it's safe to reset lwWaitLink back to NULL. The stray value would thus only be visible while a backend executes the PGSemaphoreLock() loop, and whether or not this is the case can be deduced from a stack trace. So I'm not convinced that this makes debugging any harder. That's not actually safe on an out of order architecture afaics. Without barriers the store to lwWaitLink in the woken up backend can preempt the read for the next element in the PGSemaphoreUnlock() loop. 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] Release schedule for 9.3.3?
On 02/14/2014 01:11 PM, Andres Freund wrote: Hi, On 2014-02-14 13:08:34 -0500, Josh Berkus wrote: Do the 9.3.3 replication fixes mean that users should reclone their replicas, like 9.3.2 did? Or not? Which replication replication fixes are you referring to? http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=ebde6c40148c9f811f7c6d35f67e7ea3ce2d9b34 ? If so, no, that doesn't require a reclone. Hmmm. I thought there were also 9.3-only replication fixes in this release? No? -- 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] Release schedule for 9.3.3?
On 2014-02-14 13:33:46 -0500, Josh Berkus wrote: On 02/14/2014 01:11 PM, Andres Freund wrote: Hi, On 2014-02-14 13:08:34 -0500, Josh Berkus wrote: Do the 9.3.3 replication fixes mean that users should reclone their replicas, like 9.3.2 did? Or not? Which replication replication fixes are you referring to? http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=ebde6c40148c9f811f7c6d35f67e7ea3ce2d9b34 ? If so, no, that doesn't require a reclone. Hmmm. I thought there were also 9.3-only replication fixes in this release? No? I don't know any. There's further multixact fixes but AFAIK there's nothing replication specific, and they shouldn't cause problems but lost row locks in some edge cases. 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] HBA files w/include support?
On Fri, Feb 14, 2014 at 6:33 AM, Bruce Momjian br...@momjian.us wrote: On Fri, Feb 14, 2014 at 03:28:23AM -0500, Stephen Frost wrote: Bruce, Having @include and directory.d-style capabilities for pg_hba.conf *and* pg_ident.conf would make managing larger environments much better. There has been some talk about providing those capabilities via tables in the catalog, but I'm not aware of anyone working on it and it'd certainly be quite a bit more work than adding include/dir.d options. Do we want a TODO for this? If we are assembling a wish-list, I've often wanted the opposite of an include. I want the ability to encapsulate the contents of pg_hba.conf directly into postgresql.conf. So instead of giving a filename to hba_file, optionally give a multi-lined string with some kind of here-doc like mechanism, or something like that. When I set up a forked dev environment and then eventually want to compare the diverged dev setup back to production, I often forget to compare the pg_hba.conf file. Cheers, Jeff
Re: [HACKERS] Release schedule for 9.3.3?
Andres Freund wrote: On 2014-02-14 13:33:46 -0500, Josh Berkus wrote: On 02/14/2014 01:11 PM, Andres Freund wrote: Hi, On 2014-02-14 13:08:34 -0500, Josh Berkus wrote: Do the 9.3.3 replication fixes mean that users should reclone their replicas, like 9.3.2 did? Or not? Which replication replication fixes are you referring to? http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=ebde6c40148c9f811f7c6d35f67e7ea3ce2d9b34 ? If so, no, that doesn't require a reclone. Hmmm. I thought there were also 9.3-only replication fixes in this release? No? I don't know any. There's further multixact fixes but AFAIK there's nothing replication specific, and they shouldn't cause problems but lost row locks in some edge cases. There is one issue that might cause foreign keys to go unchecked. In cases where applications update referenced tuples and then delete them in the same transaction, it might be wise to recheck foreign keys. This is the relevant commit: commit db1014bc46de21a6de1751b807ea084e607104ad Author: Alvaro Herrera alvhe...@alvh.no-ip.org Date: Wed Dec 18 13:31:27 2013 -0300 Don't ignore tuple locks propagated by our updates If a tuple was locked by transaction A, and transaction B updated it, the new version of the tuple created by B would be locked by A, yet visible only to B; due to an oversight in HeapTupleSatisfiesUpdate, the lock held by A wouldn't get checked if transaction B later deleted (or key-updated) the new version of the tuple. This might cause referential integrity checks to give false positives (that is, allow deletes that should have been rejected). This is an easy oversight to have made, because prior to improved tuple locks in commit 0ac5ad5134f it wasn't possible to have tuples created by our own transaction that were also locked by remote transactions, and so locks weren't even considered in that code path. It is recommended that foreign keys be rechecked manually in bulk after installing this update, in case some referenced rows are missing with some referencing row remaining. Per bug reported by Daniel Wood in CAPweHKe5QQ1747X2c0tA=5zf4yns2xcvgf13opd-1mq24rf...@mail.gmail.com -- Á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] Recovery inconsistencies, standby much larger than primary
Going over this I think this is still a potential issue: On 31 Jan 2014 15:56, Andres Freund and...@2ndquadrant.com wrote: I am not sure that explains the issue, but I think the redo action for truncation is not safe across crashes. A XLOG_SMGR_TRUNCATE will just do a smgrtruncate() (and then mdtruncate) which will iterate over the segments starting at 0 till mdnblocks()/segment_size and *truncate* but not delete individual segment files that are not needed anymore, right? If we crash in the midst of that a new mdtruncate() will be issued, but it will get a shorter value back from mdnblocks(). Am I missing something? I'm not too familiar with md.c but my reading of the code is that we truncate the files in reverse order? In which case I think the code is safe *iff* the filesystem guarantees ordered meta data writes which I tihnk ext3 does (I think in all the journal modes). Most filesystems meta data writes are synchronous so the truncates are safe for them too. But we don't generally rely on meta data writes being ordered. I think the correct thing to do is to record the nblocks prior to the truncate and then have md.c expose a new function that takes that parameter and pokes around looking for any segments it might need to clean up. But that would involve lots of abstraction violations in md.c. I think using nblocks would keep the violations within md.c but that still seems like a pain.
Re: [HACKERS] New hook after raw parsing, before analyze
I think I’m gonna need to dig into the planner to fully understand your points. Thank you for the insights. I was more into putting the knowledge of the legacy system into the an extension and my codebase. Now I see better use of the planner would help. Thank you. What inspired me is the scriptable query rewrite in http://dev.mysql.com/downloads/mysql-proxy/ The hook I proposed would be a lot nicer in Postgres because the raw parsing is already done at this point while in mysql-proxy that has to be done manually. Another point I liked in mysql is the possibility to write info schema plugins: http://dev.mysql.com/doc/refman/5.1/en/writing-information-schema-plugins.html Like a virtual catalog. Is there anything similar in Postgres? Thank you, David 2014.02.14. dátummal, 18:06 időpontban Greg Stark st...@mit.edu írta: On Fri, Feb 14, 2014 at 2:28 PM, David Beck db...@starschema.net wrote: Why is that a bad idea of rewriting the query before it reaches transform/analyze (without ever accessing the catalog)? If that flexibility is acceptable to you, where would be the best place to put it in? Well if there are two foreign tables and the planner could push the join work down to the fdw then the planner should be able to accurately represent that plan and cost it without having the user have to create any catalog structures. That's what the planner does for every other type of plan node. What you're describing would still be useful for materialized views. In that case the user is creating the materialized view and it is a real thing in the catalogs that won't disappear on the planner. Even then it would be ideal if the planner could decide to use the materialized view late enough that it can actually determine if it's superior rather than rewriting the query before it gets to that point. That would be much more flexible for users too who might not write the query in a way that exactly matches the materialized view. -- 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] [SQL] Comparison semantics of CHAR data type
On Thu, Feb 13, 2014 at 09:47:01PM -0500, Bruce Momjian wrote: On Wed, Oct 16, 2013 at 02:17:11PM -0400, Bruce Momjian wrote: You can see the UTF8 case is fine because \n is considered greater than space, but in the C locale, where \n is less than space, the false return value shows the problem with internal_bpchar_pattern_compare() trimming the string and first comparing on lengths. This is exactly the problem you outline, where space trimming assumes everything is less than a space. For collations other than C some of those issues that have to do with string comparisons might simply be hidden, depending on how strcoll() handles inputs off different lengths: If strcoll() applies implicit space padding to the shorter value, there won't be any visible difference in ordering between bpchar and varchar values. If strcoll() does not apply such space padding, the right-trimming of bpchar values causes very similar issues even in a en_US collation. I have added the attached C comment to explain the problem, and added a TODO item to fix it if we ever break binary upgrading. Does anyone think this warrants a doc mention? I have done some more thinking on this and I found a way to document this, which reduces our need to actually fix it some day. I am afraid the behavioral change needed to fix this might break so many applications that the fix will never be done, though I will keep the TODO item until I get more feedback on that. Patch attached. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml new file mode 100644 index 30fd9bb..9635004 *** a/doc/src/sgml/datatype.sgml --- b/doc/src/sgml/datatype.sgml *** SELECT '52093.89'::money::numeric::float *** 1072,1081 para Values of type typecharacter/type are physically padded with spaces to the specified width replaceablen/, and are ! stored and displayed that way. However, the padding spaces are ! treated as semantically insignificant. Trailing spaces are ! disregarded when comparing two values of type typecharacter/type, ! and they will be removed when converting a typecharacter/type value to one of the other string types. Note that trailing spaces emphasisare/ semantically significant in typecharacter varying/type and typetext/type values, and --- 1072,1084 para Values of type typecharacter/type are physically padded with spaces to the specified width replaceablen/, and are ! stored and displayed that way. However, trailing spaces are treated as ! semantically insignificant and disregarded when comparing two values ! of type typecharacter/type. In collations where whitespace ! is significant, this behavior can produce unexpected results, ! e.g. commandSELECT 'a '::CHAR(2) collate C 'a\n'::CHAR(2) ! returns true. ! Trailing spaces are removed when converting a typecharacter/type value to one of the other string types. Note that trailing spaces emphasisare/ semantically significant in typecharacter varying/type and typetext/type values, and diff --git a/src/backend/utils/adt/varchar.c b/src/backend/utils/adt/varchar.c new file mode 100644 index 284b5d1..502ca44 *** a/src/backend/utils/adt/varchar.c --- b/src/backend/utils/adt/varchar.c *** bpcharcmp(PG_FUNCTION_ARGS) *** 846,863 len2; int cmp; - /* - * Trimming trailing spaces off of both strings can cause a string - * with a character less than a space to compare greater than a - * space-extended string, e.g. this returns false: - * SELECT E'ab\n'::CHAR(10) E'ab '::CHAR(10); - * even though '\n' is less than the space if CHAR(10) was - * space-extended. The correct solution would be to trim only - * the longer string to be the same length of the shorter, if - * possible, then do the comparison. However, changing this - * might break existing indexes, breaking binary upgrades. - * For details, see http://www.postgresql.org/message-id/CAK+WP1xdmyswEehMuetNztM4H199Z1w9KWRHVMKzyyFM+hV=z...@mail.gmail.com - */ len1 = bcTruelen(arg1); len2 = bcTruelen(arg2); --- 846,851 -- 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] Per table autovacuum vacuum cost limit behaviour strange
Haribabu Kommi escribió: I changed the balance cost calculations a little bit to give priority to the user provided per table autovacuum parameters. If any user specified per table vacuum parameters exists and those are different with guc vacuum parameters then the balance cost calculations will not include that worker in calculation. Only the cost is distributed between other workers with specified guc vacuum cost parameter. The problem in this calculation is if the user provides same guc values to the per table values also then it doesn't consider them in calculation. I think this is a strange approach to the problem, because if you configure the backends just so, they are completely ignored instead of being adjusted. And this would have action-at-a-distance consequences because if you change the defaults in postgresql.conf you end up with completely different behavior on the tables for which you have carefully tuned the delay so that they are ignored in rebalance calculations. I think that rather than ignoring some backends completely, we should be looking at how to weight the balancing calculations among all the backends in some smart way that doesn't mean they end up with the default values of limit, which AFAIU is what happens now -- which is stupid. Not real sure how to do that, perhaps base it on the globally-configured ratio of delay/limit vs. the table-specific ratio. What I mean is that perhaps the current approach is all wrong and we need to find a better algorithm to suit this case and more generally. Of course, I don't mean to say that it should behave completely differently than now in normal cases, only that it shouldn't give completely stupid results in corner cases such as this one. As an example, suppose that global limit=200 and global delay=20 (the defaults). Then we have a global ratio of 5. If all three tables being vacuumed currently are using the default values, then they all have ratio=5 and therefore all should have the same limit and delay settings applied after rebalance. Now, if two tables have ratio=5 and one table has been configured to have a very fast vacuum, that is limit=1, then ratio for that table is 1/20=500. Therefore that table should be configured, after rebalance, to have a limit and delay that are 100 times faster than the settings for the other two tables. (And there is a further constraint that the total delay per limit unit should be so-and-so to accomodate getting the correct total delay per limit unit.) I haven't thought about how to code that, but I don't think it should be too difficult. Want to give it a try? I think it makes sense to modify both the running delay and the running limit to achieve whatever ratio we come up with, except that delay should probably not go below 10ms because, apparently, some platforms have that much of sleep granularity and it wouldn't really work to have a smaller delay. Am I making sense? -- Á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
[HACKERS] Ctrl+C from sh can shut down daemonized PostgreSQL cluster
We have had a case where a production cluster was accidentally shut down by a customer who used Ctrl+C in the same sh session in which they had (long before) run pg_ctl start. We have only seen this in sh on Solaris. Other shells on Solaris don't behave this way, nor does sh on tested versions of Linux. Nevertheless, the problem is seen on the default shell for a supported OS. Analysis suggests that this is because the postmaster retains the process group ID of the original parent (in this case pg_ctl). If pg_ctl is run through the setpgrp command a subsequent Ctrl+C in the sh session does not shut down the PostgreSQL cluster. On my development Linux machine: $ ps axfopid,ppid,pgid,command PID PPID PGID COMMAND [ ... ] 8416 1 8412 /home/kgrittn/pg/master/Debug/bin/postgres -D Debug/data 8418 8416 8418 \_ postgres: checkpointer process 8419 8416 8419 \_ postgres: writer process 8420 8416 8420 \_ postgres: wal writer process 8421 8416 8421 \_ postgres: autovacuum launcher process 8422 8416 8422 \_ postgres: stats collector process 8427 8416 8427 \_ postgres: kgrittn test [local] idle All of the PPID values seem correct, and while the PGID values for backends might initially seem surprising, the commit notes and C comments here explain why each backend sets up its own process group: http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=3ad0728 What is surprising is that the postmaster doesn't set up its own process group when it is running as a daemon. We probably don't want to change that when postgres is run directly from a command line for development or diagnostic purposes, but Noah suggested perhaps we should add a --daemonize option which pg_ctl should use when launching the postmaster, which would cause it to create its own session group. Although this is arguably a bug, it seems like it is very rarely hit and has several workarounds, and any fix would either change things in a way which might break existing user scripts or would require a new command-line option; so back-patching a fix to stable branches doesn't seem appropriate. I would argue for including a fix in 9.4 on the basis of it being a bug fix and there being time to mention it in the release change notes; but I understand the counter-arguments and realize this is a judgment call. Thoughts? If the new option seems reasonable, I can draft a patch to implement that. -- Kevin Grittner EDB: 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] Ctrl+C from sh can shut down daemonized PostgreSQL cluster
Kevin Grittner kgri...@ymail.com writes: What is surprising is that the postmaster doesn't set up its own process group when it is running as a daemon. We probably don't want to change that when postgres is run directly from a command line for development or diagnostic purposes, but Noah suggested perhaps we should add a --daemonize option which pg_ctl should use when launching the postmaster, which would cause it to create its own session group. We intentionally removed the daemonization support that used to be there; see commit f7ea6beaf4ca02b8e6dc576255e35a5b86035cb9. One of the things it did was exactly this. I'm a bit disinclined to put that back. If this is, as it sounds to be, a Solaris shell bug, doesn't it affect other daemons too? 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] New hook after raw parsing, before analyze
On Fri, Feb 14, 2014 at 9:16 PM, David Beck db...@starschema.net wrote: Another point I liked in mysql is the possibility to write info schema plugins: http://dev.mysql.com/doc/refman/5.1/en/writing-information-schema-plugins.html Like a virtual catalog. Is there anything similar in Postgres? The documentation you linked to describes how to provide information_schema plugins but not why you would want to do such a thing. I'm not seeing why this would be useful. The information_schema schema is described by the standard so creating new views in it isn't needed very often and the schema for the existing views doesn't change very often. I can see why a plugin might want to add rows to the views but that doesn't seem to be what this feature is about. -- 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] Per table autovacuum vacuum cost limit behaviour strange
On Feb 15, 2014 9:19 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Haribabu Kommi escribió: I changed the balance cost calculations a little bit to give priority to the user provided per table autovacuum parameters. If any user specified per table vacuum parameters exists and those are different with guc vacuum parameters then the balance cost calculations will not include that worker in calculation. Only the cost is distributed between other workers with specified guc vacuum cost parameter. The problem in this calculation is if the user provides same guc values to the per table values also then it doesn't consider them in calculation. I think this is a strange approach to the problem, because if you configure the backends just so, they are completely ignored instead of being adjusted. And this would have action-at-a-distance consequences because if you change the defaults in postgresql.conf you end up with completely different behavior on the tables for which you have carefully tuned the delay so that they are ignored in rebalance calculations. I think that rather than ignoring some backends completely, we should be looking at how to weight the balancing calculations among all the backends in some smart way that doesn't mean they end up with the default values of limit, which AFAIU is what happens now -- which is stupid. Not real sure how to do that, perhaps base it on the globally-configured ratio of delay/limit vs. the table-specific ratio. What I mean is that perhaps the current approach is all wrong and we need to find a better algorithm to suit this case and more generally. Of course, I don't mean to say that it should behave completely differently than now in normal cases, only that it shouldn't give completely stupid results in corner cases such as this one. As an example, suppose that global limit=200 and global delay=20 (the defaults). Then we have a global ratio of 5. If all three tables being vacuumed currently are using the default values, then they all have ratio=5 and therefore all should have the same limit and delay settings applied after rebalance. Now, if two tables have ratio=5 and one table has been configured to have a very fast vacuum, that is limit=1, then ratio for that table is 1/20=500. Therefore that table should be configured, after rebalance, to have a limit and delay that are 100 times faster than the settings for the other two tables. (And there is a further constraint that the total delay per limit unit should be so-and-so to accomodate getting the correct total delay per limit unit.) I haven't thought about how to code that, but I don't think it should be too difficult. Want to give it a try? I think it makes sense to modify both the running delay and the running limit to achieve whatever ratio we come up with, except that delay should probably not go below 10ms because, apparently, some platforms have that much of sleep granularity and it wouldn't really work to have a smaller delay. Am I making sense? Yes makes sense and it's a good approach also not leaving the delay parameter as is. Thanks I will give a try. Regards, Hari Babu Fujitsu Australia
Re: [HACKERS] New hook after raw parsing, before analyze
Greg Stark st...@mit.edu writes: On Fri, Feb 14, 2014 at 9:16 PM, David Beck db...@starschema.net wrote: Another point I liked in mysql is the possibility to write info schema plugins: http://dev.mysql.com/doc/refman/5.1/en/writing-information-schema-plugins.html Like a virtual catalog. Is there anything similar in Postgres? The documentation you linked to describes how to provide information_schema plugins but not why you would want to do such a thing. I'm not seeing why this would be useful. The information_schema schema is described by the standard so creating new views in it isn't needed very often and the schema for the existing views doesn't change very often. IIRC, mysql has a liberal view about what they can do to the information_schema, so for them this isn't as insane as it sounds to us. 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] New hook after raw parsing, before analyze
On Fri, Feb 14, 2014 at 9:16 PM, David Beck db...@starschema.net wrote: What inspired me is the scriptable query rewrite in http://dev.mysql.com/downloads/mysql-proxy/ The hook I proposed would be a lot nicer in Postgres because the raw parsing is already done at this point while in mysql-proxy that has to be done manually. There are a few similar things in the Postgres world such as pgbouncer and plproxy but as you note they are limited in how powerful they can be by the complexity of parsing and analyzing SQL. I think Postgres tends to take a different tack regarding extensibility than MySQL. Rather than have one system then hooks to allow external code to modify or replace it, in Postgres modules usually are treated similarly to the internal code. This is a pretty broad generalization and there are certainly exceptions. But for example new data types, functions, even whole new index types are all treated almost identically to the internal data types, functions, and index types. The planner then considers them all more or less on equal basis. Obviously there are limits. Btree indexes are kind of special because they represent Postgres's basic concept of ordering. And we don't have a pluggable recovery system which makes any externally provided index types non-crash-safe. And we don't have a generalized concept of table storage -- the closest thing we have is FDWs which is more like MySQL's style of extensibility where the extension is a special case. But this is why our instinct is that if you want to be able to push down joins the way to do it is to extend the FDW api so that the planner can make those decisions just like it makes the decisions about internal joins rather than have an external module that takes over part of the planner's work. -- 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] Recovery inconsistencies, standby much larger than primary
On 2014-02-14 20:46:01 +, Greg Stark wrote:
Going over this I think this is still a potential issue:
On 31 Jan 2014 15:56, Andres Freund and...@2ndquadrant.com wrote:
I am not sure that explains the issue, but I think the redo action for
truncation is not safe across crashes. A XLOG_SMGR_TRUNCATE will just
do a smgrtruncate() (and then mdtruncate) which will iterate over the
segments starting at 0 till mdnblocks()/segment_size and *truncate* but
not delete individual segment files that are not needed anymore, right?
If we crash in the midst of that a new mdtruncate() will be issued, but
it will get a shorter value back from mdnblocks().
Am I missing something?
I'm not too familiar with md.c but my reading of the code is that we
truncate the files in reverse order?
That's what I had assumed as well, but it doesn't look that way:
v = mdopen(reln, forknum, EXTENSION_FAIL);
priorblocks = 0; /* - initial value */
while (v != NULL)
{
MdfdVec*ov = v;
if (priorblocks nblocks)
{
/* truncate entire segment */
}
else if (priorblocks + ((BlockNumber) RELSEG_SIZE) nblocks)
{
/* truncate entire segment */
}
else
{
/* nothing to do, all needed */
}
priorblocks += RELSEG_SIZE;
}
So, according to that we start at segment 0, right?
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] narwhal and PGDLLIMPORT
(2014/02/15 2:32), Tom Lane wrote: I wrote: Hiroshi Inoue in...@tpf.co.jp writes: One thing I'm wondering about is that plperl is linking perlxx.lib not libperlxx.a. I made a patch following plpython and it also works here. Is it worth trying? I hadn't noticed that part of plpython's Makefile before. Man, that's an ugly technique :-(. Still, there's little about this platform that isn't ugly. Let's try it and see what happens. And what happens is this: http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=narwhaldt=2014-02-14%2017%3A00%3A02 namely, it gets through plperl now and then chokes with the same symptoms on pltcl. So I guess we need the same hack in pltcl. The fun never stops ... There seems some risk to link msvc import library. Linking perlxx.lib or tcl.lib worls here but linking pythonxx.lib doesn't work even in newer machine. (BTW, narwhal is evidently not trying to build plpython. I wonder why not?) Due to *initializer element is not constant* error which also can be see on my old machine. http://www.postgresql.org/message-id/CA+OCxozr=0wkQDF7kfd2n-bJQOwdSUs0Myohg29pA_U5=2p...@mail.gmail.com regards, Hiroshi Inoue -- 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] Ctrl+C from sh can shut down daemonized PostgreSQL cluster
On 14 Feb 2014 23:07, Tom Lane t...@sss.pgh.pa.us wrote: If this is, as it sounds to be, a Solaris shell bug, doesn't it affect other daemons too? This is simmering i never exactly followed but i think if the shell doesn't support job control it's expected behaviour, not a bug. Only shells that support job control create new process groups for every backgrounded command. I would have expected if I run postgres myself that it be attached to the terminal and die when I C-c it but if it's started by pg_ctl I would have thought it was running independently of my terminal and shell.
Re: [HACKERS] should we add a XLogRecPtr/LSN SQL type?
On Thu, Feb 6, 2014 at 11:26 AM, Michael Paquier michael.paqu...@gmail.com wrote: Here are updated patches to use pg_lsn instead of pglsn... Should I register this patch somewhere to avoid having it lost in the void? Regards, -- 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] narwhal and PGDLLIMPORT
Hiroshi Inoue in...@tpf.co.jp writes: (2014/02/15 2:32), Tom Lane wrote: (BTW, narwhal is evidently not trying to build plpython. I wonder why not?) Due to *initializer element is not constant* error which also can be see on my old machine. Hmm, isn't that one of the symptoms of inadequacies in --enable-auto-import? Wonder if the disable change fixed it. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] BUG #9210: PostgreSQL string store bug? not enforce check with correct characterSET/encoding
HI,
Thanks very much.
We use dblink or foreign table migrate datas instead pg_dump now resolve
the error data load problem.
--
公益是一辈子的事,I'm Digoal,Just Do It.
At 2014-02-14 04:49:08,Tom Lane t...@sss.pgh.pa.us wrote:
dig...@126.com writes:
select t, t::bytea from convert_from('\xeec1', 'sql_ascii') as g(t);
[ fails to check that string is valid in database encoding ]
Hm, yeah. Normal input to the database goes through pg_any_to_server(),
which will apply a validation step if the source encoding is SQL_ASCII
and the destination encoding is something else. However, pg_convert and
some other places call pg_do_encoding_conversion() directly, and that
function will just quietly do nothing if either encoding is SQL_ASCII.
The minimum-refactoring solution to this would be to tweak
pg_do_encoding_conversion() so that if the src_encoding is SQL_ASCII but
the dest_encoding isn't, it does pg_verify_mbstr() rather than nothing.
I'm not sure if this would break anything we need to have work,
though. Thoughts? Do we want to back-patch such a change?
regards, tom lane
Re: [HACKERS] Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease
On Feb14, 2014, at 19:21 , Andres Freund and...@2ndquadrant.com wrote: On 2014-02-14 18:49:33 +0100, Florian Pflug wrote: Well, the assumption isn't all that new. We already have the situation that a PGPROC may be not on any wait queue, yet its lwWaitLink may be non-NULL. Currently, the process who took it off the queue is responsible for rectifying that eventually, with the changed proposed below it'll be the backend who owns the PGPROC. From the point of view of other backends, nothing really changes. That window is absolutely tiny tho. True, but it's there, so if anything counts on that never being the case, it's still already broken. b) resetting lwWaitLink introduces a race condition (however unlikely) we'll trade correctness for cleanliness if we continue to reset lwWaitLink without protecting against the race. That's a bit of a weird trade-off to make. It's not just cleanliness, it's being able to actually debug crashes. We could still arrange for the stray lwWaitLink from being visible only momentarily. If a backend is woken up and detects that lwWaiting is false, it knows that it cannot be on any wait queue - it was just removed, and hasn't added itself again yet. At that point, it's safe to reset lwWaitLink back to NULL. The stray value would thus only be visible while a backend executes the PGSemaphoreLock() loop, and whether or not this is the case can be deduced from a stack trace. So I'm not convinced that this makes debugging any harder. That's not actually safe on an out of order architecture afaics. Without barriers the store to lwWaitLink in the woken up backend can preempt the read for the next element in the PGSemaphoreUnlock() loop. Hm, true. I guess we could prevent that by introducing an artificial dependency between the read and the write - something like proc = head; head = proc-lwWaitLink /* * We don't ever expect to actually PANIC here, but the test forces the * load to execute before the store to lwWaiting. This prevents a race * between reading lwWaitLink here and resetting it back to zero in the * awoken backend (Note that backends can wake up spuriously, so just * reading it before doing PGSemaphoreUnlock is insufficient) */ if (head != MyProc) proc-lwWaiting = false; else elog(PANIC, ...) PGSemaphoreUnlock(proc-sem); (This assumes that proc is a volatile pointer) Another idea would be to do as you suggest and only mark the PGPROC pointers volatile, but to additionally add a check for queue corruption somewhere. We should be able to detect that - if we ever hit this issue, LWLockRelease should find a PGPROC while traversing the queue whose lwWaitLink is NULL but which isn't equal to lock-tail. If that ever happens, we'd simply PANIC. best regards, Florian Pflug -- 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] Recovery inconsistencies, standby much larger than primary
Andres Freund and...@2ndquadrant.com writes: On 2014-02-14 20:46:01 +, Greg Stark wrote: Going over this I think this is still a potential issue: On 31 Jan 2014 15:56, Andres Freund and...@2ndquadrant.com wrote: I am not sure that explains the issue, but I think the redo action for truncation is not safe across crashes. A XLOG_SMGR_TRUNCATE will just do a smgrtruncate() (and then mdtruncate) which will iterate over the segments starting at 0 till mdnblocks()/segment_size and *truncate* but not delete individual segment files that are not needed anymore, right? If we crash in the midst of that a new mdtruncate() will be issued, but it will get a shorter value back from mdnblocks(). I'm not too familiar with md.c but my reading of the code is that we truncate the files in reverse order? That's what I had assumed as well, but it doesn't look that way: No, it's deleting forward. We could probably fix things so it deleted backwards; it'd be a tad tedious because the list structure isn't organized that way, but we could do it. Not sure if that's good enough though. If you don't want to assume the filesystem metadata is coherent after a crash, we might have nonzero-size segments after zero-size ones, even if the truncate calls had been issued in the right order. Another possibility is to keep opening and truncating files until we don't find the next segment in sequence, looking directly at the filesystem not at the mdfd chain. I don't think this would be appropriate in normal operation, but we could do it if InRecovery (and maybe even only if we don't think the database is consistent?) 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] Small psql memory fix
The attached tiny patch fixes a small leak in psql's \gset command and
simplifies memory freeing in two places.
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
new file mode 100644
index 764534a..e80528d
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
*** exec_command(const char *cmd,
*** 746,753
{
expand_tilde(fname);
pset.gfname = pg_strdup(fname);
}
- free(fname);
status = PSQL_CMD_SEND;
}
--- 746,753
{
expand_tilde(fname);
pset.gfname = pg_strdup(fname);
+ free(fname);
}
status = PSQL_CMD_SEND;
}
*** exec_command(const char *cmd,
*** 757,762
--- 757,764
char *prefix = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, false);
+ if (pset.gset_prefix)
+ free(pset.gset_prefix);
if (prefix)
pset.gset_prefix = prefix;
else
*** exec_command(const char *cmd,
*** 1152,1159
success = false;
}
free(newval);
}
- free(opt0);
}
--- 1154,1161
success = false;
}
free(newval);
+ free(opt0);
}
}
--
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: show xid and xmin in pg_stat_activity and pg_stat_replication
On Wed, Feb 12, 2014 at 8:00 AM, Christian Kruse christ...@2ndquadrant.com wrote: On Wednesday 12 February 2014 11:14:56 Andres Freund wrote: But they do take up shared memory without really needing to. I personally don't find that too bad, it's not much memory. If we want to avoid it we could have a LocalPgBackendStatus that includes the normal PgBackendStatus. Since pgstat_read_current_status() copies all the data locally, that'd be a sensible point to fill it. While that will cause a bit of churn, I'd guess we can use the infrastructure in the not too far away future for other parts. That's a good idea. Attached you will find a patch implementing it that way; is this how you pictured it? Although I'm not sure if this shouldn't be done in two patches, one for the changes needed for LocalPgBackendStatus and one for the xid/xmin changes. Well, this version of the patch reveals a mighty interesting point: a lot of the people who are calling pgstat_fetch_stat_beentry() don't need this additional information and might prefer not to pay the cost of fetching it. None of pg_stat_get_backend_pid, pg_stat_get_backend_dbid, pg_stat_get_backend_userid, pg_stat_get_backend_activity, pg_stat_get_backend_activity, pg_stat_get_backend_waiting, pg_stat_get_backend_activity_start, pg_stat_get_backend_xact_start, pg_stat_get_backend_start, pg_stat_get_backend_client_addr, pg_stat_get_backend_client_port, pg_stat_get_backend_client_port, and pg_stat_get_db_numbackends actually need this new information; it's only ever used in one place. So it seems like it might be wise to have pgstat_fetch_stat_beentry continue to return the PgBackendStatus * and add a new function pgstat_fetch_stat_local_beentry to fetch the LocalPgBackendStatus *; then most of these call sites wouldn't need to change. It would still be the case that pgstat_read_current_status() pays the price of fetching this information even if pg_stat_get_activity is never called. But since that's probably by far the most commonly-used API for this information, that's probably OK. -- 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] Small psql memory fix
Bruce Momjian br...@momjian.us writes: The attached tiny patch fixes a small leak in psql's \gset command and simplifies memory freeing in two places. The first and third hunks look to me like they introduce memory leaks, not fix them. The second hunk is probably safe enough, although I'm not sure the case can actually occur --- gset should free the prefix before any new backslash command can be executed. 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] Small psql memory fix
On Fri, Feb 14, 2014 at 11:28:49PM -0500, Tom Lane wrote:
Bruce Momjian br...@momjian.us writes:
The attached tiny patch fixes a small leak in psql's \gset command and
simplifies memory freeing in two places.
The first and third hunks look to me like they introduce memory
leaks, not fix them. The second hunk is probably safe enough,
The first and third just move the free into blocks where we have already
tested the value is not null. It just more clearly matches the
surrounding code. Do you see something different?
although I'm not sure the case can actually occur --- gset should
free the prefix before any new backslash command can be executed.
Oh, interesting idea. Updated patch attached.
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
new file mode 100644
index 764534a..8225159
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
*** exec_command(const char *cmd,
*** 746,762
{
expand_tilde(fname);
pset.gfname = pg_strdup(fname);
}
- free(fname);
status = PSQL_CMD_SEND;
}
/* \gset [prefix] -- send query and store result into variables */
else if (strcmp(cmd, gset) == 0)
{
! char *prefix = psql_scan_slash_option(scan_state,
! OT_NORMAL, NULL, false);
if (prefix)
pset.gset_prefix = prefix;
else
--- 746,766
{
expand_tilde(fname);
pset.gfname = pg_strdup(fname);
+ free(fname);
}
status = PSQL_CMD_SEND;
}
/* \gset [prefix] -- send query and store result into variables */
else if (strcmp(cmd, gset) == 0)
{
! char *prefix;
!
! if (pset.gset_prefix)
! free(pset.gset_prefix);
+ prefix = psql_scan_slash_option(scan_state,
+ OT_NORMAL, NULL, false);
if (prefix)
pset.gset_prefix = prefix;
else
*** exec_command(const char *cmd,
*** 1152,1159
success = false;
}
free(newval);
}
- free(opt0);
}
--- 1156,1163
success = false;
}
free(newval);
+ free(opt0);
}
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Small psql memory fix
Bruce Momjian br...@momjian.us writes: On Fri, Feb 14, 2014 at 11:28:49PM -0500, Tom Lane wrote: The first and third hunks look to me like they introduce memory leaks, not fix them. The second hunk is probably safe enough, The first and third just move the free into blocks where we have already tested the value is not null. It just more clearly matches the surrounding code. Do you see something different? [ looks closer... ] OK, they're not actually broken, but I question whether they're improvements. The existing coding is clear enough that the result of psql_scan_slash_option will be freed. What you're doing is conflating that requirement with some other processing. although I'm not sure the case can actually occur --- gset should free the prefix before any new backslash command can be executed. Oh, interesting idea. Updated patch attached. I don't think that's an improvement at all. My point was that I didn't think gset_prefix would ever be set at entry to this code, because the setting should never survive for more than one backslash command execution cycle. If you want to add probably-useless code to free it, go ahead, but this isn't a clearer way to do that than your previous version. 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] Small psql memory fix
On Fri, Feb 14, 2014 at 11:52:42PM -0500, Tom Lane wrote: Bruce Momjian br...@momjian.us writes: On Fri, Feb 14, 2014 at 11:28:49PM -0500, Tom Lane wrote: The first and third hunks look to me like they introduce memory leaks, not fix them. The second hunk is probably safe enough, The first and third just move the free into blocks where we have already tested the value is not null. It just more clearly matches the surrounding code. Do you see something different? [ looks closer... ] OK, they're not actually broken, but I question whether they're improvements. The existing coding is clear enough that the result of psql_scan_slash_option will be freed. What you're doing is conflating that requirement with some other processing. although I'm not sure the case can actually occur --- gset should free the prefix before any new backslash command can be executed. Oh, interesting idea. Updated patch attached. I don't think that's an improvement at all. My point was that I didn't think gset_prefix would ever be set at entry to this code, because the setting should never survive for more than one backslash command execution cycle. If you want to add probably-useless code to free it, go ahead, but this isn't a clearer way to do that than your previous version. If you think this code isn't helping, I will just discard it. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] Long paths for tablespace leads to uninterruptible hang in Windows
On Fri, Feb 14, 2014 at 8:27 AM, Bruce Momjian br...@momjian.us wrote: On Tue, Jan 7, 2014 at 12:33:33PM +0530, Amit Kapila wrote: On Thu, Oct 31, 2013 at 8:58 AM, Amit Kapila amit.kapil...@gmail.com wrote: On Wed, Oct 16, 2013 at 1:44 AM, Amit Kapila amit.kapil...@gmail.com wrote: On Tue, Oct 15, 2013 at 6:28 PM, Magnus Hagander mag...@hagander.net wrote: I agree we'll probably want to work around it in the end, but I still think it should be put to Microsoft PSS if we can. The usual - have we actually produced a self-contained example that does just this (and doesn't include the full postgres support) and submitted it to *microsoft* for comments? Further update on this issue: Microsoft has suggested a workaround for stat API. Their suggestion is to use 'GetFileAttributesEx' instead of stat, when I tried their suggestion, it also gives me same problem as stat. Still they have not told anything about other API's (rmdir, RemoveDirectory) which has same problem. Where are we on this? Till now we didn't received any workaround which can fix this problem from Microsoft. From the discussion over support ticket with them, it seems this problem is in their kernel and changing the code for it might not be straight forward for them, neither they have any clear alternative. Is there a check we should add in our code? We can possibly solve this problem in one of the below ways: 1. Resolve symbolic link to actual path in code whenever we tries to access it. 2. Another way is to check in code (initdb and create tablespace) to not allow path of length more than ~120 for Windows. Approach-1 has benefit that it can support the actual MAX_PATH and even if MS doesn't resolve the problem, PostgreSQL will not face it. Approach-2 is straightforward to fix. If we want to go with Approach-2, then we might change the limit of MaxPath for windows in future whenever there is a fix for it. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
