Re: [HACKERS] [bug fix] pg_ctl always uses the same event source
On Sat, May 10, 2014 at 4:10 PM, MauMau wrote: > From: "Amit Kapila" > >> I think it's bit late for this patch for 9.4, you might want to move it to >> next CF. > > > Thanks, I've moved it. It's a regret that this very small patch wasn't put > in 9.4. i took a look at the latest version of this patch, since it's marked as ready for committer. A few comments: Is there a reason for there still being changes in guc.c, pgevent.c etc? Shouldn't it all be confined to pg_ctl now? That's my understanding from the thread that that's the only part we care about. More importantly, isn't it wrong to claim it will only be used for register and unregister? If we get an early failure in start, for example, there are numerous codepaths that will end up calling write_stderr(), which will use the eventlog when running as a service. Shouldn't the "-e" parameter be moved under "common options"? I also think we should have the documentation specifically note that regular postgres output still goes to whatever the backend is configured to do. (and of course, any failure within the backend *before* we have parsed the config file for example will still go to the default source, and not the pg_ctl source - so we need to make it really clear that this is *only* for output from pg_ctl). -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Removing dependency to wsock32.lib when compiling code on WIndows
On Thu, Jun 19, 2014 at 4:13 PM, MauMau wrote:
> From: "Michael Paquier"
>
>> You are right, I only though about the MS scripts when working on this
>> patch. Updated patch for a complete cleanup is attached (note I
>> updated manually ./configure for test purposes and did not re-run
>> autoconf).
>
>
> I marked this patch as ready for committer. I confirmed:
I'm pretty sure this is not ready for commiter due to:
> * The binaries can be built with MSVC 2008 Express. I didn't try to build
> on MinGW.
Somebody needs to test it on mingw first :)
That should be an easy test for someone who has a mingw install
around, so better leave it at "needs review" so more people see it and
can run those tests.
> * The regression tests pass ("vcregress check").
>
> However, I wonder if some DLL entries in dlls[] in
> src/interfaces/libpq/win32.c should be removed. winsock.dll should
> definitely be removed, because it is for 16-bit applications. I don't know
> about the rest. Especially, wsock32n.dll and mswsock.dll may also be
> obsolete libraries from their names. Could you look into this and revise
> the patch if possible? But I don't mind if you do so.
I'm with michael about the "scaryness" of touching this. Also, it is
definitely a separate patch if we do...
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL information view
On Sun, Jul 13, 2014 at 10:32 PM, Stefan Kaltenbrunner wrote: > On 07/12/2014 03:08 PM, Magnus Hagander wrote: >> As an administrator, I find that you fairly often want to know what >> your current connections are actually using as SSL parameters, and >> there is currently no other way than gdb to find that out - something >> we definitely should fix. > > Yeah that would be handy - however I often wish to be able to figure > that out based on the logfile as well, any chance of getting these into > connection-logging/log_line_prefix as well? We do already log some of it if you have enabled log_connections - protocol and cipher. Anything else in particular you'd be looking for - compression info? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extending MSVC scripts to support --with-extra-version
On Mon, Jun 30, 2014 at 1:43 PM, Michael Paquier wrote: > > > > On Mon, Jun 30, 2014 at 7:05 PM, Asif Naeem wrote: >> Patch looks good to me. I think it is ready for committer. Thanks. > Thanks for the extra checks and for taking the time to review it. Applied, thanks! -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL information view
On Sat, Jul 12, 2014 at 4:36 PM, Tom Lane wrote: > Magnus Hagander writes: >> As an administrator, I find that you fairly often want to know what >> your current connections are actually using as SSL parameters, and >> there is currently no other way than gdb to find that out - something >> we definitely should fix. > > I'm wondering whether it's such a great idea that everybody can see > everybody else's client DN. Other than that, no objection to the > concept. I was thinking that's mostly the equivalent of a username, which we do let everybody see in pg_stat_activity. >> Second, I was planning to implement it by adding fields to >> PgBackendStatus and thus to BackendStatusArray, booleans directly in >> the struct and strings similar to how we track for example hostnames. >> Anybody see a problem with that? > > Space in that array is at a premium, and again the client DN seems > problematic, in that it's not short and has no clear upper bound. > > If you were to drop the DN from the proposed view then I'd be fine > with this. The text fields, like hostname, are tracked in separate parts of shared memory with just a pointer in the main array - I assume that's why, and was planning to do the same. We'd have to cap the length oft he DN at something though (and document as such), to make it fixed length. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] SSL information view
As an administrator, I find that you fairly often want to know what your current connections are actually using as SSL parameters, and there is currently no other way than gdb to find that out - something we definitely should fix. You can find it out today through libpq (the PQgetssl functions), the psql banner, or contrib/sslinfo. All of them are client side (the sslinfo module runs on the server, but it's just SQL functions that can show information about the current connection, nothing that can be used to inspect other connections). I recently put together a quick hack (https://github.com/mhagander/pg_sslstatus) that exposes a view with this information, but it's definitely hacky, and it really is functionality that we should include in core. Thus, I'll provide a version of that hack for 9.5. Before doing that, however, I'd like to ask for opinions :) The hack currently exposes a separate view that you can join to pg_stat_activity (or pg_stat_replication) on the pid -- this is sort of the same way that pg_stat_replication works in the first place. Do we want something similar to that for a builtin SSL view as well, or do we want to include the fields directly in pg_stat_activity and pg_stat_replication? Second, I was planning to implement it by adding fields to PgBackendStatus and thus to BackendStatusArray, booleans directly in the struct and strings similar to how we track for example hostnames. Anybody see a problem with that? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] SSL compression info in psql header
It's today really hard to figure out if your SSL connection is
actually *using* SSL compression. This got extra hard when we the
default value started getting influenced by environment variables at
least on many platforms after the crime attacks. ISTM we should be
making this easier for the user.
Attached patch adds compression info at least to the header of the
psql banner, as that's very non-intrusive. I think this is a small
enough change, yet very useful, that we should squeeze it into 9.4
before the next beta. Not sure if it can be qualified enough of a bug
to backpatch further than that though.
As far as my research shows, the function
SSL_get_current_compression() which it uses was added in OpenSSL
0.9.6, which is a long time ago (stopped being maintained in 2004).
AFAICT even RHEL *3* shipped with 0.9.7. So I think we can safely rely
on it, especially since we only check for whether it returns NULL or
not.
Comments?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index cede72a..b8a8e35 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -1800,8 +1800,9 @@ printSSLInfo(void)
return; /* no SSL */
SSL_get_cipher_bits(ssl, &sslbits);
- printf(_("SSL connection (protocol: %s, cipher: %s, bits: %d)\n"),
- SSL_get_version(ssl), SSL_get_cipher(ssl), sslbits);
+ printf(_("SSL connection (protocol: %s, cipher: %s, bits: %d, compression: %s)\n"),
+ SSL_get_version(ssl), SSL_get_cipher(ssl), sslbits,
+ SSL_get_current_compression(ssl) ? gettext_noop("yes") : gettext_noop("no"));
#else
/*
--
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] Missing autocomplete for CREATE DATABASE
On Fri, Jul 11, 2014 at 1:10 PM, Magnus Hagander wrote: > On Fri, Jul 11, 2014 at 12:01 AM, Vik Fearing wrote: >> On 07/10/2014 09:32 PM, Magnus Hagander wrote: >>> It seems psql is missing autocomplete entries for LC_COLLATE and >>> LC_CTYPE for the CREATE DATABASE command. Attached patch adds that. >>> >>> I doubt this is important enough to backpatch - thoughts? >> >> It won't apply to current head, but otherwise I see no problem with it. > > Meh, thanks for pointing that out. I git-fetch:ed from the wrong repository :) Applied and backpatched. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Missing autocomplete for CREATE DATABASE
On Fri, Jul 11, 2014 at 12:01 AM, Vik Fearing wrote: > On 07/10/2014 09:32 PM, Magnus Hagander wrote: >> It seems psql is missing autocomplete entries for LC_COLLATE and >> LC_CTYPE for the CREATE DATABASE command. Attached patch adds that. >> >> I doubt this is important enough to backpatch - thoughts? > > It won't apply to current head, but otherwise I see no problem with it. Meh, thanks for pointing that out. I git-fetch:ed from the wrong repository :) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_receivexlog and replication slots
On Fri, Jul 11, 2014 at 11:14 AM, Andres Freund wrote: > On 2014-07-11 11:08:48 +0200, Magnus Hagander wrote: >> Is there a particular reason why pg_receivexlog only supports using >> manually created slots but pg_recvlogical supports creating and >> dropping them? Wouldn't it be good for consistency there? > > I've added it to recvlogical because logical decoding isn't usable > without slots. I'm not sure what you'd want to create/drop a slot from > receivexlog for, but I'm not adverse to adding the capability. I was mostly thinking for consistency, really. Using slots with pg_receivexlog makes quite a bit of sense, even though it's useful without it. But having the ability to create and drop (with compatible commandline arguments of course) them directly when you set it up would certainly be more convenient. >> I'm guessing it's related to not being exposed over the replication >> protocol? > > It's exposed: > create_replication_slot: > /* CREATE_REPLICATION_SLOT slot PHYSICAL */ > K_CREATE_REPLICATION_SLOT IDENT K_PHYSICAL Hmm. You know, it would help if I actually looked at a 9.4 version of the file when I check for functions of this kind :) Thanks! -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pg_receivexlog and replication slots
Is there a particular reason why pg_receivexlog only supports using manually created slots but pg_recvlogical supports creating and dropping them? Wouldn't it be good for consistency there? I'm guessing it's related to not being exposed over the replication protocol? We had a discussion earlier that I remember about being able to use an "auto-drop" slot in pg_basebackup, but this would be different - this would be about creating and dropping a regular physical replication slot... -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Missing autocomplete for CREATE DATABASE
It seems psql is missing autocomplete entries for LC_COLLATE and
LC_CTYPE for the CREATE DATABASE command. Attached patch adds that.
I doubt this is important enough to backpatch - thoughts?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 3bb727f..f746ddc 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2046,7 +2046,7 @@ psql_completion(const char *text, int start, int end)
{
static const char *const list_DATABASE[] =
{"OWNER", "TEMPLATE", "ENCODING", "TABLESPACE", "CONNECTION LIMIT",
- NULL};
+ "LC_COLLATE", "LC_CTYPE", NULL};
COMPLETE_WITH_LIST(list_DATABASE);
}
--
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 #10680 - ldapbindpasswd leaks to postgresql log
On Wed, Jun 18, 2014 at 4:50 AM, Tom Lane wrote: > Steven Siebert writes: > > Attached is a proposed patch for BUG #10680. > > > It's a simple fix to the problem of the ldapbindpasswd leaking in > > clear text to the postgresql log. The patch simply removes the raw > > pg_hba.conf line from the log message, but retains the log line number > > to assist admins in troubleshooting. > > You haven't exactly explained why this is a problem. The proposed patch > would impede diagnosing of many other problems, so it's not going to get > committed without a thoroughly compelling rationale. > Yes, properly logging that was intentional, in commit 7f49a67f954db3e92fd96963169fb8302959576e. Hint: "I don't store my postmaster log securely" is not compelling. > We've been over that ground before; there are far too many reasons > why access to the postmaster log is a potential security hazard > to justify concluding that this particular one is worse. > Yeah, and the password is already in cleartext in a file next to it. If we actually feel the need to get rid of it, we should do a better job. Such as actively blanking it out with something else. Since we know the password (we parsed it out), it shouldn't be impossible to actually blank out *just the password*, without ruining all the other diagnostics usage of it. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] 9.5 CF1
On Tue, Jun 17, 2014 at 6:54 PM, Tom Lane wrote: > Alvaro Herrera writes: > > Robert Haas wrote: > >> On Mon, Jun 16, 2014 at 2:36 AM, Abhijit Menon-Sen > wrote: > >>> P.S. If you tag your reviews with [REVIEW] in the Subject, it'll be > >>> easier to keep track of them. > > >> I and, I believe, various other people hate that style, because at > >> least in Gmail, it breaks the threading. It is much easier to find > >> things if they are all posted on one thread. > > > Yes, please don't do that. A simple, normal reply to the message that > > submits the patch is much better from my point of view as a subsequent > > reviewer and committer. > > Worth noting also is that Magnus is working on a new version of the > commitfest app that will be able to automatically keep track of threads > about patches --- so long as they *are* threads according to our mailing > list archives. I'm not sure if the archives recognize replies with a > changed Subject: as being the same thread or not. > The archives code does threading based on the headers (in-reply-to and references, in priority order). It completely ignores the subject when it comes to the threading. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] replication commands and log_statements
On Wed, Jun 11, 2014 at 2:35 PM, Andres Freund wrote: > On 2014-06-11 14:22:43 +0200, Magnus Hagander wrote: > > Yes, but how would you specify for example "i want DDL and all > replication > > commands" (which is quite a reasonable thing to log, I believe). If you > > actually require it to be set to "all", most people won't have any use at > > all for it... > > That's a reasonable feature request - but imo it's a separate > one. It seems pretty noncontroversial and simple to make > log_statement=all log replication commands. What you want - and you're > sure not alone with that - is something considerably more complex... > > I'm just saying if we're going to do it, let's do it right from the beginning. Or are you suggesting this would be something we'd backpatch? Given the lack of complaints about it, I'd rather suggest we don't backpatch anything, and instead make the correct feature in the first pace for the next release. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] replication commands and log_statements
On Wed, Jun 11, 2014 at 2:17 PM, Andres Freund wrote: > On 2014-06-11 13:42:58 +0200, Magnus Hagander wrote: > > On Wed, Jun 11, 2014 at 12:34 PM, Fujii Masao > wrote: > > > > > Hi, > > > > > > Replication commands like IDENTIFY_COMMAND are not logged even when > > > log_statements is set to all. Some users who use log_statements to > > > audit *all* statements might dislike this current situation. So I'm > > > thinking to change log_statements or add something like log_replication > > > so that we can log replication commands. Thought? > > > > > > > +1. I think adding a separate parameter is the way to go. > > Why? I can't really see a case where the log volume by replication > connections is going to be significant in comparison to 'all'? > > Yes, but how would you specify for example "i want DDL and all replication commands" (which is quite a reasonable thing to log, I believe). If you actually require it to be set to "all", most people won't have any use at all for it... -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] replication commands and log_statements
On Wed, Jun 11, 2014 at 12:34 PM, Fujii Masao wrote: > Hi, > > Replication commands like IDENTIFY_COMMAND are not logged even when > log_statements is set to all. Some users who use log_statements to > audit *all* statements might dislike this current situation. So I'm > thinking to change log_statements or add something like log_replication > so that we can log replication commands. Thought? > +1. I think adding a separate parameter is the way to go. The other option would be to turn log_statements into a parameter that you specify multiple ones - so instead of "all" today it would be "ddl,dml,all" or something like that, and then you'd also add "replication" as an option. But that would cause all sorts of backwards compatibility annoyances.. And do you really want to be able to say things like "ddl,all" meanin you'd get ddl and select but not dml? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] /proc/self/oom_adj is deprecated in newer Linux kernels
On Jun 10, 2014 7:05 PM, "Joshua D. Drake" wrote: > > > On 06/10/2014 07:02 AM, Tom Lane wrote: >> >> Gurjeet Singh writes: >>> >>> Startup scripts are not solely in the domain of packagers. End users >>> can also be expected to develop/edit their own startup scripts. >> >> >>> Providing it as GUC would have given end users both the peices, but >>> with a compile-time option they have only one half of the solution; >>> except if they go compile their own binaries, which forces them into >>> being packagers. >> >> >> I don't find that this argument holds any water at all. Anyone who's >> developing their own start script can be expected to manage recompiling >> Postgres. > > > This is certainly not true in any fashion. Production systems require a degree of flexibility and configuration that does not require the maintaining or compiling of source code. > As JD surely understands, it pains me a lot but I have to agree with him here. There are also a non-trivial number of organizations that just don't allow compilers on production boxes, adding another hurdle for those. We may think that's ridiculous, and it may be, but it's reality. That said, I agree that a guc is probably a bad idea. I like the idea of an environment variable if we need it to be changeable. Or perhaps what we need is just better documentation of what's there already. Perhaps the documentation should have a section specifically aimed at packagers? /Magnus
Re: [HACKERS] Supporting Windows SChannel as OpenSSL replacement
On Mon, Jun 9, 2014 at 7:45 PM, Heikki Linnakangas wrote: > On 06/09/2014 06:03 PM, Magnus Hagander wrote: > >> One tricky part is that programs like to use libpq for the >> >>> >authentication, and then they hijack the connection using PGgetssl(). >>> > >>> >> Is there*anybody* other than odbc that does that? Do we actually need a >> >> published API for that, or just a hack for pgodbc? >> > > I wish psqlODBC would stop doing that. It's kind of cool that it supports > compiling without libpq, but it's really quite a mess. I think we should > modify psqlODBC to use the libpq API like most people do. This was, I believe, done at one point, and then reverted. I think that was because libpq didn't actually have all the features required either for the current or for planned featues of the ODBC driver. That situation might be very different now though, there's more functionality available in libpq.. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Supporting Windows SChannel as OpenSSL replacement
On Mon, Jun 9, 2014 at 4:39 PM, Martijn van Oosterhout wrote: > On Mon, Jun 09, 2014 at 03:35:23PM +0200, Magnus Hagander wrote: > > On Mon, Jun 9, 2014 at 3:19 PM, Andreas Karlsson > wrote: > > > > > On 06/09/2014 01:45 PM, Heikki Linnakangas wrote: > > > There was a patch set for this from Martijn van Oosterhout which was > quite > > > complete. > > > > > > http://www.postgresql.org/message-id/20060504134807.gk4...@svana.org > > Wow, blast from the past. > That's fun, itsn't it? :) > A lot has, unfortunately, changed since 2006. It might be a good > > startingpoint. But also actively starting from the point of "let's try to > > support multiple libraries" rather than "let's try to support gnutls" is > > probably also important. > > The patch did provide an API. The idea was that there were a number of > functions which would need to be defined to support an SSL library. > Each library would then have a wrapper which wrapped the library and > based on the results of configure it compiled the right file into the > backend. > > These functions were: > > extern void pgtls_initialize(void); > extern void pgtls_destroy(void); > extern int pgtls_open_server(Port *); > extern void pgtls_close(Port *); > extern ssize_t pgtls_read(Port *port, void *ptr, size_t len); > extern ssize_t pgtls_write(Port *port, void *ptr, size_t len); > > Which should be easy enough to support for any library. These days > you'd need to add support for verifying certificates, but I don't think > that that would be difficult (unless the actual certificate formats are > different). > The two difficult points I think are the async support (libpq) and the windows socket emulation support (backend). Do those really work there? In particular the win32 stuff - though I guess that's less critical since we can actually do hackish things there, unlike in libpq. But the example there is that we can't have the library use recv()/send(), instead having it work through our own functions. > No switching after compile time, that would just lead to useless > overhead. > Yes, I absolutely think we don't need to support >1 library at runtime. > At some point we should design a new API, so that we can deprecate the old > > one. Even if we don't hve the code ready, we need to get rid of > PQgetssl(), > > and replace it with something else. I'm thinking probably a functoin that > > returns both a void pointer and an enum that tells you which library is > > actually in use. And a boolean just saying "ssl on/off", because that's > > what a lot of clients are interested in and they don't care aobut more > than > > that. > > > > Obviously, we also have to do something about PQinitOpenSSL(). > > Yeah, I think this was one of the more controversial parts. Support in > the backend was primarily moving code around and renaming functions, > fairly straightforward. Even error handling was not so hard (I found > the gnutls handling of errors much easier than openssl). > Yeah, the backend is easier, but also less important from the original reason. For the patching reason, it's of course just as important. One tricky part is that programs like to use libpq for the > authentication, and then they hijack the connection using PGgetssl(). > Is there *anybody* other than odbc that does that? Do we actually need a published API for that, or just a hack for pgodbc? > The way I dealt with this is defining a new state "passthrough" where > the caller would get a few function pointers to read/write/check the > connection. Then the callers would not need to know what library libpq > was compiled with. And libpq would know the connection was hijacked > and refuse to act anymore. I don't think everyone was pleased with > this, but no real alternative was presented (other than requiring > people hijacking the connection to do the hard work). > > For information about which library was in use there was PQgettlsinfo() > which returned a PGresult with information about the library and > connection. I beleive since then new functions have been added to > libpq to retrive info about certificates, so that might need a rethink > also. > Not really, no. we return the OpenSSL structure and have the client use that one directly. That's quite horrible already :) Basically, I think these last two points are the hard parts to get > agreement (assuming there's agreement to do anything at all about the > problem) and without nailing those down first whoever picks this up > will be in for a lot of work. > Agreed. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Supporting Windows SChannel as OpenSSL replacement
On Mon, Jun 9, 2014 at 3:02 PM, Marko Kreen wrote: > On Mon, Jun 09, 2014 at 02:45:08PM +0300, Heikki Linnakangas wrote: > > Thoughts? While we're at it, we'll probably want to refactor things > > so that it's easy to support other SSL implementations too, like > > gnutls. > > One project that is proud to support several SSL implementations > is curl: http://curl.haxx.se/ > > Git: https://github.com/bagder/curl.git > Implementations: https://github.com/bagder/curl/tree/master/lib/vtls > > List from vtls.c: > > - OpenSSL > - GnuTLS > - NSS > - QsoSSL > - GSKit > - PolarSSL > - CyaSSL > - Schannel SSPI > - SecureTransport (Darwin) > > We cannot reuse the code directly, but seems it's usable for > reference for various gotchas that need to be solved. > I did actually talk to Daniel at some point about turning that into a generalized library, and/or getting him interested in helping out with it. I can't remember where that ended up - I'll see if I can poke his interest :) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Supporting Windows SChannel as OpenSSL replacement
On Mon, Jun 9, 2014 at 3:19 PM, Andreas Karlsson wrote: > On 06/09/2014 01:45 PM, Heikki Linnakangas wrote: > >> Thoughts? While we're at it, we'll probably want to refactor things so >> that it's easy to support other SSL implementations too, like gnutls. >> > > There was a patch set for this from Martijn van Oosterhout which was quite > complete. > > http://www.postgresql.org/message-id/20060504134807.gk4...@svana.org A lot has, unfortunately, changed since 2006. It might be a good startingpoint. But also actively starting from the point of "let's try to support multiple libraries" rather than "let's try to support gnutls" is probably also important. I am interested in dropping the dependency on OpenSSL, if only to fix the > situation with Debian, libreadline and OpenSSL[1]. > That's one of the many reasons, yes :) At some point we should design a new API, so that we can deprecate the old one. Even if we don't hve the code ready, we need to get rid of PQgetssl(), and replace it with something else. I'm thinking probably a functoin that returns both a void pointer and an enum that tells you which library is actually in use. And a boolean just saying "ssl on/off", because that's what a lot of clients are interested in and they don't care aobut more than that. Obviously, we also have to do something about PQinitOpenSSL(). Unfortunately, I think it's too late to do that for 9.4 - otherwise it would've been good to have a whole cycle of deprecation on it... -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Supporting Windows SChannel as OpenSSL replacement
On Monday, June 9, 2014, Heikki Linnakangas wrote: > Hi, > > I've been looking at Windows' native SSL implementatation, the SChannel > API. It would be nice to support that as a replacement for OpenSSL on > Windows. Currently, we bundle the OpenSSL library in the PostgreSQL, > installers, which is annoying because whenever OpenSSL puts out a new > release that fixes vulnerabilities, we need to do a security release of > PostgreSQL on Windows. I was reminded of this recently wrt. psqlODBC, which > bundles libpq and openssl as well. It's particularly annoying for psqlODBC > and other client applications, as people typically update it less > diligently than their servers. > > I think that we should keep the user-visible behavior the same, i.e. the > libpq connection options, locations of the certificate files etc. would all > be the same regardless of which SSL implementation is used. Using Windows > SChannel API might make it possible to integrate better with Windows' own > certificate store etc. but I don't really know much about that stuff, so > for starters I'd like to just use it as a drop-in replacement for OpenSSL. > > Thoughts? While we're at it, we'll probably want to refactor things so > that it's easy to support other SSL implementations too, like gnutls. > > It's a project that many have started, and nobody has finished :) I'm definitely interested in working on such a things, but I've been unable to carve out enough time recently. One problem is as you say, that we're exposing openssl too much. For one thing, we *cannot* keep the current interface, because it returns OpenSSL internal datastructures. Those functions will need to be deprecated and replaced with something else. Also, my memory says that SChannel doesn't support the key file format that we use now, which makes a much bigger break with the supported platforms. That may have changed of course - have you researched that part? The main other entries I've been looking at are NSS and gnutls, both of which can speak our current file formats. I think the right thing is to start with those and thereby make it more pluggable, and only after that tackle schannel. But I do think it would be good to have them all. It's also a question of if we can accept supporting a different set of libraries on the server vs on the client. It's really on the client that it's a bigger problem, but in the end I think we want to have "symmetrical support". But it might be worth doing just the client side initially, and then move to the server. I think in general, the client side is actually likely to be *harder* than the server side.. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] pg_basebackup failed to back up large file
On Wednesday, June 4, 2014, Tom Lane wrote: > Magnus Hagander > writes: > > On Tue, Jun 3, 2014 at 6:38 PM, Tom Lane > wrote: > >> Another thought is we could make pg_basebackup simply skip any files > that > >> exceed RELSEG_SIZE, on the principle that you don't really need/want > >> enormous log files to get copied anyhow. We'd still need the pax > >> extension if the user had configured large RELSEG_SIZE, but having a > >> compatible tar could be documented as a requirement of doing that. > > > I think going all the way to pax is the proper long-term thing to do, at > > least if we can confirm it works in the main tar implementations. > > > For backpatchable that seems more reasonable. It doesn't work today, and > we > > just need to document that it doesn't, with larger RELSEG_SIZE. And then > > fix it properly for the future. > > Agreed, this would be a reasonable quick fix that we could replace in > 9.5 or later. > > Fujii, are you going to be able to work on this with the now expanded scope? :) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] slotname vs slot_name
On Thu, Jun 5, 2014 at 12:57 PM, Andres Freund wrote: > On 2014-06-05 10:57:58 +0900, Fujii Masao wrote: > > On Thu, Jun 5, 2014 at 8:24 AM, Andres Freund > wrote: > > > Hi, > > > > > > Due to the opened window of the pg_control/catalog version bump a > chance > > > has opened to fix a inconsistency I've recently been pointed > > > towards: > > > Namely that replication slots are named 'slot_name' in one half of the > > > cases and 'slotname' in the other. That's in views, SRF columns, > > > function parameters and the primary_slotname recovery.conf parameter. > > > > > > My personal tendency would be to make it slot_name everywhere except > the > > > primary_slotname recovery.conf parameter. There we already have > > > precedent for shortening names. > > > > > > Other opinions? > > > > I like using "slot_name" everywhere, i.e, even in recovery.conf. > > primary_slot_name seems not so long name. > > It also has the advantage that we can add a couple more slot_* options > later. Will do that. > > > BTW, what about also renaming pg_llog directory? I'm afraid that > > a user can confuse pg_log with pg_llog. > > We have: > * pg_ldecoding (Heikki) > * pg_lcse or pg_lcset (Petr) > * pg_logical (Andres) > > I like, what a surprise, my own suggestion best. The name seems more > versatile because it's not restricted to decoding. > > I don't care too much really, either one is find - but if I should vote, I'll split my vote between pg_locical and pg_ldecoding, I don't like lcse and lcset very much. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] slotname vs slot_name
On Jun 5, 2014 10:22 AM, "Devrim Gündüz" wrote: > > > Hi, > > On Thu, 2014-06-05 at 10:57 +0900, Fujii Masao wrote: > > BTW, what about also renaming pg_llog directory? I'm afraid that > > a user can confuse pg_log with pg_llog. > > +1. I hit this while testing 9.4 this week. Should be confusing for many > end-users. > +500. We already have two directories that users remove to get free space thinking it's safe because it's just log files. We don't need one more :-) /Magnus
Re: [HACKERS] Sigh, we need an initdb
On Jun 4, 2014 8:52 PM, "Tom Lane" wrote: > > I just noticed that we had not one, but two commits in 9.4 that added > fields to pg_control. And neither one changed PG_CONTROL_VERSION. > This is inexcusable sloppiness on the part of the committers involved, > but the question is what do we do now? > > Quick experimentation says that you don't really get to the point of > noticing this if you try to start a 9.4 postmaster against 9.3 database or > vice versa, because the postmaster will look at the PG_VERSION file first. > However, pg_resetxlog and pg_controldata don't do that, so you could get > misleading results from the wrong pg_controldata version or potentially > screw yourself entirely with the wrong pg_resetxlog, if you failed to > interpret their warnings about wrong CRC values correctly. > > I think we could possibly ship 9.4 without fixing this, but it would be > imprudent. Anyone think differently? Shipping it without fixing that seems like a horrible idea. We should either force an initdb or revert those changes. I'd vote for forcing initdb. If we push it off we'll be paying for it for the coming 5 years. > Of course, if we do fix this then the door opens for pushing other > initdb-forcing fixes into 9.4beta2, such as the LOBLKSIZE addition > that I was looking at when I noticed this, or the pg_lsn catalog > additions that were being discussed a couple weeks ago. +1. We should of course evaluate those patches individually but the initdb argument against them isn't valid, so if they are otherwise good, we should accept them. /Magnus
Re: [HACKERS] pg_basebackup failed to back up large file
On Tue, Jun 3, 2014 at 6:38 PM, Tom Lane wrote: > Magnus Hagander writes: > > Yeah, that is a clear advantage of that method. Didn't read up on pax > > format backwards compatibility, does it have some trick to achieve > > something similar? > > I didn't read the fine print but it sounded like the extended header > would look like a separate file entry to a non-aware tar implementation, > which would write it out as a file and then get totally confused when > the length specified in the overlength file's entry didn't match the > amount of data following. So it's a nice solution for some properties > but doesn't fail-soft for file length. Not clear that there's any way > to achieve that though. > Well, there is no way to make it fail completely soft on the client side I think. But at least we should make sure that our implementation doesn't go do something really bad if you were to upgrade your server but not the client. And make sure that we test with at least GNU and BSD tar to see they don't break things badly either. Another thought is we could make pg_basebackup simply skip any files that > exceed RELSEG_SIZE, on the principle that you don't really need/want > enormous log files to get copied anyhow. We'd still need the pax > extension if the user had configured large RELSEG_SIZE, but having a > compatible tar could be documented as a requirement of doing that. > I think going all the way to pax is the proper long-term thing to do, at least if we can confirm it works in the main tar implementations. For backpatchable that seems more reasonable. It doesn't work today, and we just need to document that it doesn't, with larger RELSEG_SIZE. And then fix it properly for the future. Not sure we want to go change the file format even for 9.4 at this time, it seems we're quite overdue for that - so I think that's a 9.5 feature. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] pg_basebackup failed to back up large file
On Jun 3, 2014 6:17 PM, "Andres Freund" wrote: > > On 2014-06-03 17:57:52 +0200, Magnus Hagander wrote: > > On Tue, Jun 3, 2014 at 5:42 PM, Tom Lane wrote: > > > What we had better do, IMO, is fix things so that we don't have a filesize > > > limit in the basebackup format. After a bit of googling, I found out that > > > recent POSIX specs for tar format include "extended headers" that among > > > other things support member files of unlimited size [1]. Rather than > > > fooling with partial fixes, we should make the basebackup logic use an > > > extended header when the file size is over INT_MAX. > > > Yeah, pax seems to be the way to go. It's at least supported by GNU tar - > > is it also supported on say BSD, or other popular platforms? (The size > > extension in the general ustar format seems to be, so it would be a shame > > if this one is less portable) > > PG's tar.c already uses the ustar format and the referenced extension is > an extension to ustar as far as I understand it. So at least tarballs > with files < 8GB would still continue to be readable with all currently > working implementations. Yeah, that is a clear advantage of that method. Didn't read up on pax format backwards compatibility, does it have some trick to achieve something similar? /Magnus
Re: [HACKERS] pg_basebackup failed to back up large file
On Tue, Jun 3, 2014 at 5:42 PM, Tom Lane wrote: > Andres Freund writes: > > On 2014-06-03 11:04:58 -0400, Tom Lane wrote: > >> My point is that having backups crash on an overflow doesn't really seem > >> acceptable. IMO we need to reconsider the basebackup protocol and make > >> sure we don't *need* to put values over 4GB into this field. Where's > the > >> requirement coming from anyway --- surely all files in PGDATA ought to > be > >> 1GB max? > > > Fujii's example was logfiles in pg_log. But we allow to change the > > segment size via a configure flag, so we should support that or remove > > the ability to change the segment size... > > What we had better do, IMO, is fix things so that we don't have a filesize > limit in the basebackup format. After a bit of googling, I found out that > recent POSIX specs for tar format include "extended headers" that among > other things support member files of unlimited size [1]. Rather than > fooling with partial fixes, we should make the basebackup logic use an > extended header when the file size is over INT_MAX. > Yeah, pax seems to be the way to go. It's at least supported by GNU tar - is it also supported on say BSD, or other popular platforms? (The size extension in the general ustar format seems to be, so it would be a shame if this one is less portable) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] pg_basebackup failed to back up large file
On Tue, Jun 3, 2014 at 5:12 PM, Andres Freund wrote: > On 2014-06-03 11:04:58 -0400, Tom Lane wrote: > > Magnus Hagander writes: > > > On Tue, Jun 3, 2014 at 4:40 PM, Tom Lane wrote: > > >> There's a far bigger problem there, which is if we're afraid that > > >> current_len_left might exceed 4GB then what is it exactly that > guarantees > > >> it'll fit in an 11-digit field? > > > > > Well, we will only write 11 digits in there, that's when we read it. > But > > > print_val() on the server side should probably have an overflow check > > > there, which it doesn't. It's going to write some strange values int > here > > > if it overflows.. > > > > My point is that having backups crash on an overflow doesn't really seem > > acceptable. IMO we need to reconsider the basebackup protocol and make > > sure we don't *need* to put values over 4GB into this field. Where's the > > requirement coming from anyway --- surely all files in PGDATA ought to be > > 1GB max? > > Fujii's example was logfiles in pg_log. But we allow to change the > segment size via a configure flag, so we should support that or remove > the ability to change the segment size... > > With Fujii's fix, the new limit is 8GB, per the tar standard. To get around that we could use the "star extension" or whatever it's actually called (implemented by both gnu and bsd tar) , to maintain compatibility. Of course, a quick fix would be to just reject files >8GB - that may be what we have to do backpatchable. IIRC we discussed this at the original tmie and said it would not be needed because such files shouldnt' be there - clearly we forgot to consider logfiles.. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] pg_basebackup failed to back up large file
On Tue, Jun 3, 2014 at 4:40 PM, Tom Lane wrote: > Andres Freund writes: > > On 2014-06-03 23:19:37 +0900, Fujii Masao wrote: > >> -if (sscanf(copybuf + 124, "%11o", > ¤t_len_left) != 1) > >> +if (sscanf(copybuf + 124, "%11lo", > ¤t_len_left) != 1) > > > That's probably not going to work on 32bit platforms or windows where > > you might need to use ll instead of l as a prefix. Also notice that > > apparently (c.f. 9d7ded0f4277f5c0063eca8e871a34e2355a8371) sscanf can't > > reliably be used for 64bit input :(. That pretty much sucks... > > There's a far bigger problem there, which is if we're afraid that > current_len_left might exceed 4GB then what is it exactly that guarantees > it'll fit in an 11-digit field? Well, we will only write 11 digits in there, that's when we read it. But print_val() on the server side should probably have an overflow check there, which it doesn't. It's going to write some strange values int here if it overflows.. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] pg_stat directory and pg_stat_statements
On Mon, Jun 2, 2014 at 4:07 PM, Andres Freund wrote: > On 2014-06-02 22:59:55 +0900, Fujii Masao wrote: > > On Thu, May 29, 2014 at 3:02 PM, Peter Geoghegan wrote: > > > On Wed, May 28, 2014 at 10:49 PM, Fujii Masao > wrote: > > >> You're concerned about the scenario using pg_upgrade? I'm not sure > the detail > > >> of pg_upgrade. But if it doesn't work properly, we should have gotten > > >> the trouble > > > > > > I'm not worried about pg_upgrade, because by design pg_stat_statements > > > will discard stats files that originated in earlier versions. However, > > > I don't see a need to change pg_stat_statements to serialize its > > > statistics to disk in the pg_stat directory before we branch off 9.4. > > > As you mentioned, it's harmless. > > > > Yeah, that's an idea. OTOH, there is no *strong* reason to postpone > > the fix to 9.5. So I just feel inclined to apply the fix now... > > +1 for fixing it now. > > +1 for fixing it for 9.4 before the next beta, but *not* backpatching it to 9.3 - it *is* a behaviour change after all.. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Odd uuid-ossp behavior on smew and shearwater
On Fri, May 30, 2014 at 12:45 AM, Andrew Dunstan wrote: > > On 05/29/2014 05:41 PM, Josh Kupershmidt wrote: > >> On Thu, May 29, 2014 at 4:06 PM, Andrew Dunstan >> wrote: >> >>> Almost all my critters run in VMs (all but jacana and bowerbird). >>> >> They're not running on OpenVZ, are they? `ifconfig` on shearwater says: >> >> [...] > > and it seems this all-zeros MAC address is a common >> (mis-?)configuration on OpenVZ: >> >> https://github.com/robertdavidgraham/masscan/issues/43 >> http://stackoverflow.com/questions/5838225/how-do-i- >> get-a-default-gridgain-node-in-openvz-discover-other-nodes-on-the-same >> http://forum.openvz.org/index.php?t=msg&goto=8117 >> >> > > > Yeah, that's a bit ugly. Mine are on qemu, one (sitella) is on Xen. > That's likely the difference between real virtualization and containerization. It'd be interesting to see how it behaves on FreeBSD jails or LXC-containers. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] pg_basebackup: could not get transaction log end position from server: FATAL: could not open file "./pg_hba.conf~": Permission denied
On Fri, May 16, 2014 at 6:25 PM, Andres Freund wrote: > On 2014-05-16 18:20:35 +0200, Magnus Hagander wrote: > > On Fri, May 16, 2014 at 5:46 PM, Joshua D. Drake >wrote: > > > > > At a minimum: > > > > > > Check to see if there is going to be a permission error BEFORE the base > > > backup begins: > > > > > > starting basebackup: > > > checking perms: ERROR no access to pg_hba.conf~ base backup will fail > > > > > > That's pretty much what it does if you enable progress meter. I realize > you > > don't necessarily want that one, but we could have a switch that still > > tells the server to measure the size, but not actually print the output? > > While it costs a bit of overhead to do that, that's certainly something > > that's a lot more safe than ignoring errors. > > Don't think it'll show you that error - that mode only stats() files, > right? So you'd need to add access() or open()s. > > You're right, we don't. I thought we did, but was clearly remembering wrong. I guess we could add an access() call to that codepath though. Not sure if that's going to cause any real overhead compared to the rest of what we're doing anyway? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] pg_basebackup: could not get transaction log end position from server: FATAL: could not open file "./pg_hba.conf~": Permission denied
On Fri, May 16, 2014 at 5:46 PM, Joshua D. Drake wrote: > At a minimum: > > Check to see if there is going to be a permission error BEFORE the base > backup begins: > > starting basebackup: > checking perms: ERROR no access to pg_hba.conf~ base backup will fail That's pretty much what it does if you enable progress meter. I realize you don't necessarily want that one, but we could have a switch that still tells the server to measure the size, but not actually print the output? While it costs a bit of overhead to do that, that's certainly something that's a lot more safe than ignoring errors. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] imprecise pg_basebackup documentation about excluded files
On Sat, May 10, 2014 at 3:33 AM, Peter Eisentraut wrote: > The pg_basebackup documentation says that only regular files and > directories are "allowed" in the data directory. But it is more correct > that any other files are skipped. Attached is a patch to correct that. > I also added a link to the protocol documentation and added more > details there, also about pg_replslot handling. Not sure exactly how > much detail we want to document, but it seems reasonable to make it > complete if we provide a list at all. > +1. Seems good to me. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Sun, May 11, 2014 at 12:27 AM, Andres Freund wrote: > On 2014-05-10 19:19:22 -0300, Fabrízio de Royes Mello wrote: > > On Sat, May 10, 2014 at 6:52 PM, Tom Lane wrote: > > > > > > Andres Freund writes: > > > > I don't even understand why it's questionable whether this should be > > > > fixed. > > > > > > Sigh. We have some debate isomorphic to this one every year, it seems > > > like. The argument why it shouldn't be fixed now is: ITS. TOO. LATE. > > > Which part of that isn't clear to you? > > > > > > > Sorry but I don't understand why it's too late. The 9.4 branch not been > > created yet. > > The problem is that once the beta is in progress (starting tomorrow), > the projects tries to avoid changes which require a dump and restore (or > pg_upgrade). Since the patch changes the catalog it'd require that. > > It would be pg_upgrade'able though, wouldn't it? Don't we have precedents for requiring pg_upgrade during beta? At least that's a smaller problem than requiring a complete dump/reload. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] proposal: Set effective_cache_size to greater of .conf value, shared_buffers
On Wed, May 7, 2014 at 4:12 PM, Andres Freund wrote: > On 2014-05-07 10:07:07 -0400, Tom Lane wrote: > > In the meantime, it seems like there is an emerging consensus that nobody > > much likes the existing auto-tuning behavior for effective_cache_size, > > and that we should revert that in favor of just increasing the fixed > > default value significantly. I see no problem with a value of say 4GB; > > that's very unlikely to be worse than the pre-9.4 default (128MB) on any > > modern machine. > > > > Votes for or against? > > +1 for increasing it to 4GB and remove the autotuning. I don't like the > current integration into guc.c much and a new static default doesn't > seem to be worse than the current autotuning. > +1. If we can't make the autotuning better than that, we're better off holding off on that one until we can actually figure out something better. (At which point perhaps we can reach the level where we can just remove it.. But that's all handwaving about the future of course). -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Wanted: jsonb on-disk representation documentation
On Wed, May 7, 2014 at 3:04 PM, Andres Freund wrote: > On 2014-05-07 15:00:01 +0200, Magnus Hagander wrote: > > On Wed, May 7, 2014 at 2:56 PM, Andres Freund >wrote: > > > > > On 2014-05-07 08:50:33 -0400, Robert Haas wrote: > > > > On Wed, May 7, 2014 at 7:40 AM, Andres Freund > > > wrote: > > > > > I don't think it's likely that beta1 will be binary compatible > with the > > > > > final version at this point. > > > > > > > > I rather think we're not ready for beta1 at this point (but I expect > > > > to lose that argument). > > > > > > Well, I guess it depends on what we define 'beta1' to be. Imo > evaluating > > > problematic pieces of new code, locating unfinished pieces is part of > > > that. I don't see much point in forbidding incompatible changes in > beta1 > > > personally. That robs th the development cycle of the only period where > > > users can actually test the new version in a halfway sane manner and > > > report back with things that apparently broken. > > > > > > > > We need to be very careful to tell people about it though. Preferrably if > > we *know* a dump/reload will be needed to go beta1->beta2, we should > > actually document that in the releasenotes of beta1 already. So people > can > > make proper plans.. > > Yes, I think it actually makes sense to add that to *all* beta release > notes. Even in beta2, although slightly weakened. > That's not a new thing btw. E.g. 9.3 has had a catversion bump between > beta1/2: > git diff 09bd2acbe5ac866ce9..817a89423f429a6a8b -- > src/include/catalog/catversion.h > > The more interesting note probably is that there quite possibly won't be > pg_upgrade'ability... > > Yeah, that's the big thing really. Requiring pg_upgrade between betas might even be "good" in the sense that then we get more testing of pg_upgrade :) But requiring a dump/reload is going to hurt people more. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Wanted: jsonb on-disk representation documentation
On Wed, May 7, 2014 at 2:56 PM, Andres Freund wrote: > On 2014-05-07 08:50:33 -0400, Robert Haas wrote: > > On Wed, May 7, 2014 at 7:40 AM, Andres Freund > wrote: > > > I don't think it's likely that beta1 will be binary compatible with the > > > final version at this point. > > > > I rather think we're not ready for beta1 at this point (but I expect > > to lose that argument). > > Well, I guess it depends on what we define 'beta1' to be. Imo evaluating > problematic pieces of new code, locating unfinished pieces is part of > that. I don't see much point in forbidding incompatible changes in beta1 > personally. That robs th the development cycle of the only period where > users can actually test the new version in a halfway sane manner and > report back with things that apparently broken. > > We need to be very careful to tell people about it though. Preferrably if we *know* a dump/reload will be needed to go beta1->beta2, we should actually document that in the releasenotes of beta1 already. So people can make proper plans.. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Wanted: jsonb on-disk representation documentation
On Wed, May 7, 2014 at 1:20 PM, Heikki Linnakangas wrote: > So, apart from cleaning up the code, we really need to take a close look > at the on-disk format now. The code can be cleaned up later, too, but we're > going to be stuck with the on-disk format forever, so it's critical to get > that right. > > First, a few observations: > > * JENTRY_ISFIRST is redundant. Whenever you deal with the Jentry struct, > you know from the context which element in the array it is. > > * JENTRY_ISNEST is set but never used. > > * JENTRY_ISBOOL is defined as (JENTRY_ISNUMERIC | JENTRY_ISNEST), which > seems confusing. > > I'm going to proceed refactoring those things, which will change the > on-disk format. It's late in the release cycle - these things really > should've been cleaned up earlier - but it's important to get the on-disk > format right. Shout if you have any objections. +1. It's now or never. If the on-disk format needs changing, not doing it now is going to leave us with a "jsonc" in the future... Better bite the bullet now. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] TABLESPACE and directory for Foreign tables?
On Mon, May 5, 2014 at 8:17 PM, Josh Berkus wrote:
> On 05/05/2014 10:53 AM, Tom Lane wrote:
> > Josh Berkus writes:
> >> I'm working with the cstore_fdw project, which has an interesting
> >> property for an FDW: the FDW itself creates the files which make up the
> >> database. This raises a couple of questions:
> >
> >> 1) Do we want to establish a standard directory for FDWs which create
> >> files, such as $PGDATA/base/{database-oid}/fdw/ ? Or would we want to
> >> leave it up to each FDW to decide?
> >
> > I think we ought to vigorously discourage FDWs from storing any files
> > inside $PGDATA. This cannot lead to anything except grief. Just for
> > starters, what will operations such as pg_basebackup do with them?
>
> That was one advantage to putting them in PGDATA; you get a copy of the
> files with pg_basebackup. Of course, they don't replicate after that,
> but they potentially could, in the future, with Logical Streaming
> Replication.
>
Presumably they'd also be inconsistent? And as such not really useful
unless you actually shut the database down before you back it up (e.g.
don't use pg_basebackup)?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Re: [HACKERS] Sending out a request for more buildfarm animals?
On Sun, May 4, 2014 at 9:35 PM, Josh Berkus wrote: > > Architecture wise there's no coverage for: > > * some ARM architecture varians > > I could run a buildfarm animal on a Raspberry Pi if the Postgres > community will replace my flash cards as they burn out. > Heikki already does that - it's chipmunk. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] 9.4 release notes
On Sun, May 4, 2014 at 4:06 PM, Oleg Bartunov wrote: > Bruce, > > you forgot Alexander Korotkov, who contributed jsonb_hash_ops opclass > for GIN. Something like > "Alexander Korotkov introduced an elegant jsonb_hash ops for GIN, > which competes with MongoDB performance in contains operator". > > Here is a link to discussion - > http://www.postgresql.org/message-id/53304439.8000...@dunslane.net Aexanders work should definitely be credited in the release notes, but please let's not mention MongoDB (or any other product for that matter) in the release notes - they are about the technology, not the advocacy. (That's for things like the press kit..) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Buildfarm "master-next" branch?
On Tue, Apr 29, 2014 at 9:11 PM, Jim Nasby wrote: > On 4/17/14, 9:38 AM, Tom Lane wrote: > >> But the ability to easily spin up temporary branches for testing would >>>> >>also be great. Unfortunately, I suspect that only a minority of the >>>> >>buildfarm owners would choose to participate, which would make it less >>>> >>useful, but if we could solve that problem I'd be all in favor of it. >>>> >>> >... Of course, all this would be done in my copious spare time*cough*. >>> I'm >>> >>> >not sure this would be the best use of it. >>> >> I agree that this would not be worth the effort needed to make it happen. >> > > There's also a sizeable security risk there, of someone putting something > malicious in a branch and then triggering a run from that branch. I suppose > that could be overcome if this was purposefully limited to the main git > repo that only our core committers had access to, but we'd need to be > careful. I would suggest a separate repo to keep the main one "clean", but other than that, yes, it would have to be limited to the same committers as the rest I think. It's reasonably easy to set up build environments in containers/jais on many Unix boxes where that would actually not be a problem (just blow the whole jail away once the build is complete), but one of the main platforms that people would want to use this on I bet is Windows, which has no such facilities AFAIK. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] logical changeset generation v6
On Thu, Apr 24, 2014 at 9:43 AM, Andres Freund wrote: > On 2014-04-24 09:39:21 +0200, Magnus Hagander wrote: > > I can't find that this discussion actually came to a proper consensus, > but > > I may be missing something. Did we go with pg_recvlogical just because we > > couldn't decide on a better name, or did we intentionally decide it was > the > > best? > > I went with pg_recvlogical because that's where the (small) majority > seemed to be. Even if I was unconvinced. There were so many outstanding > big fights at that point that I didn't want to spend my time on this ;) > > I was guessing something like the second part there, which is why I figured this would be a good time to bring this fight back up to the surface ;) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] logical changeset generation v6
On Mon, Sep 23, 2013 at 7:03 PM, Peter Geoghegan wrote: > On Mon, Sep 23, 2013 at 9:54 AM, Andres Freund > wrote: > > I still find it wierd/inconsistent to have: > > * pg_receivexlog > > * pg_recvlogical > > binaries, even from the same source directory. Why once "pg_recv" and > > once "pg_receive"? > > +1 > Digging up a really old thread since I just got annoyed by the inconsistent naming the first time myself :) I can't find that this discussion actually came to a proper consensus, but I may be missing something. Did we go with pg_recvlogical just because we couldn't decide on a better name, or did we intentionally decide it was the best? I definitely think pg_receivelogical would be a better name, for consistency (because it's way too late to rename pg_receivexlog of course - once released that can't really chance. Which is why *if* we want to change the name of pg_recvxlog we have a few more days to make a decision..) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Compilation of pg_recvlogical on Windows
On Thu, Apr 24, 2014 at 8:49 AM, Michael Paquier wrote: > Hi all, > > I noticed that pg_recvlogical is not currently compiled on Windows > when using the msvc scripts. The patch attached corrects that. > Applied, thanks. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Perfomance degradation 9.3 (vs 9.2) for FreeBSD
On Tue, Apr 22, 2014 at 8:26 AM, Mark Kirkwood < mark.kirkw...@catalyst.net.nz> wrote: > On 22/04/14 09:25, Andres Freund wrote: > >> On 2014-04-21 17:21:20 -0400, Bruce Momjian wrote: >> >>> On Mon, Apr 21, 2014 at 02:08:51PM -0700, Joshua Drake wrote: >>> >>>> If the community had more *BSD presence I think it would be great >>>> but it isn't all that viable at this point. I do know however that >>>> no-one in this community would turn down a team of FreeBSD advocates >>>> helping us make PostgreSQL awesome for PostgreSQL. >>>> >>> >>> I don't think we would even implement a run-time control for Linux or >>> Windows for this, so I don't even think it is a FreeBSD issue. >>> >> >> I think some of the arguments in this thread are pretty damn absurd. We >> have just introduced dynamic_shared_memory_type. >> >> > +1 > > I was just thinking the same thing... > > I didn't realize we had a guc for dynamic shared memory, must've missed that in the discussion about that one. I agree that if we have that, it makes perfect sense to have the same setting available for the main shared memory segment. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Perfomance degradation 9.3 (vs 9.2) for FreeBSD
On Mon, Apr 21, 2014 at 4:51 PM, Andres Freund wrote: > On 2014-04-21 10:45:24 -0400, Tom Lane wrote: > > Andres Freund writes: > > > If there are indeed such large regressions on FreeBSD we need to treat > > > them as postgres regressions. It's nicer not to add config options for > > > things that don't need it, but apparently that's not the case here. > > > > > Imo this means we need to add GUC to control wether anon mmap() or sysv > > > shmem is to be used. In 9.3. > > > > I will resist this mightily. One of the main reasons to switch to mmap > > was so we would no longer have to explain about SysV shm configuration. > > It's still explained in the docs and one of the dynshm implementations > is based on sysv shmem. So I don't see this as a convincing reason. > > Regressing installed OSs by 15-20% just to save a couple of lines of > docs and code seems rather unconvincing to me. > > There's also the fact that even if it's changed in FreeBSD, that might be somethign that takes years to trickle out to whatever stable release people are actually using. But do we really want a *guc* for it though? Isn't it enough (and in fact better) with a configure switch to pick the implementation when multiple are available, that could then be set by default for example by the freebsd ports build? That's a lot less "overhead" to keep dragging around... -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Typo fix in src/backend/access/transam/recovery.conf.sample
On Fri, Apr 18, 2014 at 12:24 PM, Amit Langote wrote: > Hi, > > Attached fixes a minor typo in > src/backend/access/transam/recovery.conf.sample. > Applied, except I also rewrapped the line to make it shorter. Thanks! -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] LDAP: bugfix and deprecated OpenLDAP API
On Mon, Oct 21, 2013 at 3:31 PM, Albe Laurenz wrote:
> Peter Eisentraut wrote:
> >> --- 3511,3534
> >> }
> >>
> >> /*
> >> ! * Perform an explicit anonymous bind.
> >> ! * This is not necessary in principle, but we want to set a timeout
> >> ! * of PGLDAP_TIMEOUT seconds and return 2 if the connection fails.
> >> ! * Unfortunately there is no standard conforming way to do that.
> >> */
> >
> > This comment has become a bit confusing. What exactly is nonstandard?
> > Setting a timeout, or returning 2? The code below actually returns 3.
>
> I have improved the comment.
>
> >> + #ifdef HAVE_LIBLDAP
> >> +/* in OpenLDAP, use the LDAP_OPT_NETWORK_TIMEOUT option */
> >
> > We don't use HAVE_LIBLDAP anywhere else to mean OpenLDAP. Existing
> > LDAP-related code uses #ifdef WIN32.
>
> Changed.
>
> > > + #else
> >
> > There should be a comment here indicating what this #else belongs to
> > (#else /* HAVE_LIBLDAP */, or whatever we end up using).
>
> Changed.
>
> >> +/* the nonstandard ldap_connect function performs an anonymous
> bind */
> >> +if (ldap_connect(ld, &time) != LDAP_SUCCESS)
> >> +{
> >> +/* error or timeout in ldap_connect */
> >> +free(url);
> >> +ldap_unbind(ld);
> >> +return 2;
> >> +}
> >> + #endif
> >
> > here too
>
> Changed.
>
> > Bonus: Write a commit message for your patch. (Consider using git
> > format-patch.)
>
> Suggested commit message is included in the patch.
>
Sorry about the huge delay in trying to get around to this one.
For the sake of the archives - I looked at the fact that the windows
codepath always returns 2, whereas the unix codepath separates at least one
case out as a return 3. I can't see a pattern in the windows return codes
that would make it possible to do the same though, without explicitly
listing different error codes mapping to each of them - so I don't think
it's worth doing that.
Thus - applied, and backpatched all the way. Thanks!
(And I just realized I forgot to credit reviewers in the commit message. My
apologies - I blame confusion after havnig to re-setup my windows build
environments all over again..)
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Re: [HACKERS] Tracking replication slot "blockings"
On Wed, Apr 16, 2014 at 6:56 PM, Andres Freund wrote: > Hi, > > On 2014-04-16 18:51:41 +0200, Magnus Hagander wrote: > > I'm thinking it could be interesting to know how many times (or in some > > other useful unit than "times" - how often) a specific replication slot > has > > "blocked" xlog rotation. Since this AFAIK only happens during > checkpoints, > > it seems it should be "reasonably cheap" to track? It would serve as an > > indicator of which slave(s) are having enough trouble keeping up to > > potentially cause issues. > > The xlog removal code just check the "global minimum" required LSN - it > doesn't check the individual slots. So you'd need to add a bit more code > to that location. But it'd be easy. > Do we have statistics there somewhere - how often that global minimum blocks something? That on it's own might be a start :) But I think I'd just monitor/graph the byte difference for all slots > using pg_replication_slots... > Yeah, that would work when monitored continously. I was more looking for the view of "hey, could this be what happened" into a system that did not previously have any monitoring installed and therefor no such history. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
[HACKERS] Tracking replication slot "blockings"
I'm thinking it could be interesting to know how many times (or in some other useful unit than "times" - how often) a specific replication slot has "blocked" xlog rotation. Since this AFAIK only happens during checkpoints, it seems it should be "reasonably cheap" to track? It would serve as an indicator of which slave(s) are having enough trouble keeping up to potentially cause issues. Not having looked at that code at all yet, would this be something that's simple to add? Or is it a silly idea? :) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] New option in pg_basebackup to exclude pg_log files during base backup
On Wed, Apr 9, 2014 at 4:55 PM, Alvaro Herrera wrote: > Magnus Hagander wrote: > > > While pg_log is definitely the most common one being the default on many > > platforms, we'll still be missing other ones. Should we really hardcode > it, > > or should we somehow derive it from the settings for log_directory > instead? > > > > As a more general discussion, is this something we might want to expose > as > > a more general facility rather than hardcode it to the log directory > only? > > And is it perhaps something we'd rather have configured at the server > than > > specified in pg_basebackup - like a guc saying which directories should > > always be excluded from a basebackup? So you don't have to remember it > > every time? > > So it'd be an array, and by default you'd have something like: > basebackup_skip_path = $log_directory > ? > > Maybe use it to skip backup labels by default as well. > basebackup_skip_path = $log_directory, $backup_label_files > I hadn't considered any details, but yes, someting along that line. And then you could also include arbitrary filenames or directories should you want. E.g. if you use the data directory to store your torrents or something. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] New option in pg_basebackup to exclude pg_log files during base backup
On Wed, Apr 9, 2014 at 2:06 AM, Prabakaran, Vaishnavi < vaishna...@fast.au.fujitsu.com> wrote: > Hi all, > > > > Following the discussion in message id - > cahgqgwffmor4ecugwhzpaapyqbsekdg66vmj1rvej6z-ega...@mail.gmail.com , I > have developed the patch which gives option to user to exclude pg_log > directory contents in pg_basebackup. > > > > [Current situation] > > During pg_basebackup, all files in pg_log directory will be copied to new > backup directory. > > > > [Design] > > - Added new non-mandatory option "-S/--skip-log-dir" to pg_basebackup . > > - If "skip-log-dir" is specified in pg_basebackup command, then in > basebackup, exclude copying log files from standard "pg_log" directory and > any other directory specified in Log_directory guc variable. (Still empty > folder "pg_log"/$Log_directory will be created) > > - In case, pg_log/$Log_directory is symbolic link, then an empty folder > will be created > > > > [Advantage] > > It gives an option to user to avoid copying of large log files if they > doesn't wish to and hence can save memory space. > > > > > While pg_log is definitely the most common one being the default on many platforms, we'll still be missing other ones. Should we really hardcode it, or should we somehow derive it from the settings for log_directory instead? As a more general discussion, is this something we might want to expose as a more general facility rather than hardcode it to the log directory only? And is it perhaps something we'd rather have configured at the server than specified in pg_basebackup - like a guc saying which directories should always be excluded from a basebackup? So you don't have to remember it every time? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Including replication slot data in base backups
On Tue, Apr 1, 2014 at 2:24 PM, Michael Paquier wrote: > Hi all, > > As of now, pg_basebackup creates an empty repository for pg_replslot/ > in a base backup, forcing the user to recreate slots on other nodes of > the cluster with pg_create_*_replication_slot, or copy pg_replslot > from another node. This is not really user-friendly especially after a > failover where a given slave may not have the replication slot > information of the master that it is replacing. > > The simple patch attached adds a new option in pg_basebackup, called > --replication-slot, allowing to include replication slot information > in a base backup. This is done by extending the command BASE_BACKUP in > the replication protocol. > --replication-slots would be a better name (plural), or probably --include-replication-slots. (and that comment also goes for the BASE_BACKUP syntax and variables) But. If you want to solve the failover case, doesn't that mean you need to include it in the *replication* stream and not just the base backup? Otherwise, what you're sending over might well be out of date set of slots once the failover happens? What if the set of replication slots change between the time of the basebackup and the failover? As it is too late for 9.4, I would like to add it to the first commit > fest of 9.5. Comments are welcome. > It's not too late to fix omissions in 9.4 features though, if we consider it that. Which I think this could probably be considered as - if we think the solution is the right one. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] datistemplate of pg_database does not behave as per description in documentation
On Mar 27, 2014 12:45 PM, "Tom Lane" wrote:
>
> Magnus Hagander writes:
> > On Thu, Mar 27, 2014 at 9:12 AM, Rajeev rastogi
> > wrote:
> >> But current code does not behave in this manner. Even if dbistemplate
of
> >> database is false, still it allows to be used as template database.
>
> > AFAICT, the *only* thing datistemplate is used is to set parameters in
> > autovacuum.
>
> Huh? The code comment is perfectly clear:
>
> /*
> * Permission check: to copy a DB that's not marked datistemplate, you
> * must be superuser or the owner thereof.
> */
> if (!src_istemplate)
> {
> if (!pg_database_ownercheck(src_dboid, GetUserId()))
> ereport(ERROR,
> (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> errmsg("permission denied to copy database \"%s\"",
> dbtemplate)));
> }
>
> I agree we need to make the docs match the code, but changing behavior
> that's been like that for ten or fifteen years isn't the answer.
Hah, I hadn't done more than git grep for datistemplate :-) that's what I
get for taking shortcuts...
But yes, fixing the documentation is what I advocate as well.
> (Changing code and failing to adjust the adjacent comment is even less
> of an answer.)
I didn't even look at the suggested patch..
/Magnus
Re: [HACKERS] datistemplate of pg_database does not behave as per description in documentation
On Thu, Mar 27, 2014 at 9:12 AM, Rajeev rastogi wrote: > As per the documentation, datistemplate of pg_database is used in > following way: > > > > datistemplate > > Bool > > If true then this database can be used in the TEMPLATE clause of CREATE > DATABASE to create a new database as a clone of this one > > > > But current code does not behave in this manner. Even if dbistemplate of > database is false, still it allows to be used as template database. > > > > postgres=# select datname, datistemplate from pg_database; > > datname | datistemplate > > ---+--- > > template1 | t > > template0 | t > > *postgres | f* > > (3 rows) > > > > *postgres=# create database tempdb template postgres; > ---Actually this should fail.* > > *CREATE DATABASE* > > > > Though I am not sure if we have to modify source code to align the > behavior with documentation or we need to change the documentation itself. > > To me it looks like code change will be better, so I am attaching the > current patch with source code change. After modification, result will be > as follows: > > > > *postgres=# create database newtempdb template postgres;* > > *ERROR: DB name "postgres" given as template is not a template database* > > > > Please provide your feedback. > > > AFAICT, the *only* thing datistemplate is used is to set parameters in autovacuum. So clearly we should do something. Changing the code that way carries the risk of breaking applications (or at least DBA scripts) for no apparent reason. I think it's better to document it. However, that also raises a third option. We could just drop the idea if datistemplate completely, and remove the column. Since clearly it's not actually doing anything, and people seem to have been happy with that for a while, why do we need it in the first place? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Dynamic background workers & docs question
On Mon, Mar 24, 2014 at 12:20 PM, Michael Paquier wrote: > On Mon, Mar 24, 2014 at 5:54 PM, Magnus Hagander > wrote: > > I was looking at > http://www.postgresql.org/docs/devel/static/bgworker.html > > with a client today. > > > > It says: > > "Unlike RegisterBackgroundWorker, which can only be called from within > the > > postmaster,RegisterDynamicBackgroundWorker must be called from a regular > > backend." > > > > Is that the correct restriction? In particular, don't we allow calling > > RegisterDynamicBackgroundWorker from another background worker? (In the > > launcher/worker kind of scenario, like AutoVacuum). > Yes, you can start a dynamic background worker from another background > worker, have a look for example at contrib/worker_spi. Perhaps the > correct wording would be "RegisterDynamicBackgroundWorker must be > called from a regular backend or another background worker". > > That's what I thought. Can a dynamic background worker start *another* dynamic background worker, or can they only be started from "first level" background workers? > > Also: > > "Background workers are expected to be continuously running; if they exit > > cleanly, postgres will restart them immediately. " > > > > This doesn't apply to dynamic ones, which we might want to clarify. Do we > > have a "term" for non-dynamic background workers? "static workers"? > In the code or the documentation, there is no explicit > differentiation, bgworkers are either called plainly "bgworker", or > "dynamic bgworker". Perhaps the solution here is simply to say > "background workers started by the postmaster are expected blabla". > That, or we need to invite a term for it? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
[HACKERS] Dynamic background workers & docs question
I was looking at http://www.postgresql.org/docs/devel/static/bgworker.htmlwith a client today. It says: "Unlike RegisterBackgroundWorker, which can only be called from within the postmaster,RegisterDynamicBackgroundWorker must be called from a regular backend." Is that the correct restriction? In particular, don't we allow calling RegisterDynamicBackgroundWorker from another background worker? (In the launcher/worker kind of scenario, like AutoVacuum). Also: "Background workers are expected to be continuously running; if they exit cleanly, postgres will restart them immediately. " This doesn't apply to dynamic ones, which we might want to clarify. Do we have a "term" for non-dynamic background workers? "static workers"? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] pg_basebackup --slot=SLOTNAME -X stream
On Tue, Mar 18, 2014 at 2:32 PM, Heikki Linnakangas wrote: > On 03/18/2014 03:25 PM, Fujii Masao wrote: > >> Hi, >> >> Is there the explicit reason why --slot option that pg_receivexlog >> already has has not been added into pg_basebackup? If not, >> I'd like to support that option to eliminate the need to increase >> wal_keep_segments for pg_basebackup. Thought? >> > > That seems rather cumbersome. pg_basebackup is a run once, and then it > dies. The slot would have to be created before running pg_basebackup, and > destroyed manually. > > It would make sense to provide a new option that would tell the server to > recycle segments until they've been sent to this client, or until the > client disconnects. But that would look quite different from > pg_receivexlog's --slot option. I started working on that at some point long ago, but never got far enough. Right now, it seems the right way to do that is to somehow reimplement it on top of a "lightweight slot" that gets automatically destroyed when pg_basebackup stops. But as Andres said, there's a little more to it. You'd want the slot to be created in the connection that does BASE_BACKUP, then used by the replication client side, and then destroyed by BASE_BACKUP. But you also want it auto-destroyed if the BASE_BACKUP connection gets terminated for example.. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Fix typo in nbtree.h introduced by efada2b
On Mon, Mar 17, 2014 at 6:06 AM, Michael Paquier wrote: > Hi, > > I found a small typo in nbtree.h, introduced by commit efada2b. Patch > is attached. > Applied, thanks. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Upcoming back branch releases
On Sun, Mar 16, 2014 at 3:20 AM, MauMau wrote: > eFrom: "Tom Lane" > > After some discussion, the core committee has concluded that the >> WAL-replay bug fixed in commit 6bfa88acd3df830a5f7e8677c13512b1b50ae813 >> is indeed bad enough to justify near-term update releases. Since >> there seems no point in being slow about it, tarballs will be wrapped >> Monday (3/17) for public announcement Thursday (3/20). >> > > Regarding: > > Prevent intermittent "could not reserve shared memory region" failures on > recent Windows versions (MauMau) > > Could you include the patch I sent for 9.0/9.1 in response to Magnus san? > > http://www.postgresql.org/message-id/9C709659FE3C4B8DA7135C6F307B83 > F5@maumau > > Hi! I just applied this patch. Apologies for the delay, and thanks for the reminder! -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] [bug fix] postgres.exe fails to start on Windows Server 2012 due to ASLR
On Sat, Feb 22, 2014 at 5:44 AM, MauMau wrote: > From: "Magnus Hagander" > > Does somebody want to look at backpatching this to 9.1 and earlier, or >> should we just say that it's not fully supported on those Windows versions >> unless you apply the registry workaround? >> > > Please use the attached patch. It applies cleanly to both 9.1 and 9.0. > Applied, apologies for the delay. We don't need to consider 8.4, because ASLR became enabled by default in > Visual Studio 2008 and 8.4 doesn't support building with 2008. > Ok, thanks for confirming! -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On Fri, Mar 14, 2014 at 6:30 AM, Prabakaran, Vaishnavi < vaishna...@fast.au.fujitsu.com> wrote: > Hi, > > > > In connection to my previous proposal about "providing catalog view to > pg_hba.conf file contents" , I have developed the attached patch . > > > > [Current situation] > > Currently, to view the pg_hba.conf file contents, DB admin has to access > the file from database server to read the settings. In case of huge and > multiple hba files, finding the appropriate hba rules which are loaded will > be difficult and take some time. > > > > [What this Patch does] > > Functionality of the attached patch is that it will provide a new view > "pg_hba_settings" to admin users. Public access to the view is restricted. > This view will display basic information about HBA setting details of > postgresql cluster. Information to be shown , is taken from parsed hba > lines and not directly read from pg_hba.conf files. Documentation files are > also updated to include details of this new view under "Chapter 47.System > Catalogs". Also , a new note is added in "chapter 19.1 The pg_hba.conf File" > > > > [Advantage] > > Advantage of having this "pg_hba_settings" view is that the admin can > check, what hba rules are loaded in runtime via database connection itself. > And, thereby it will be easy and useful for admin to check all the users > with their privileges in a single view to manage them. > > > This looks like a useful feature, so make sure you register it on https://commitfest.postgresql.org/action/commitfest_view?id=22. I haven't looked at the actual code yet, btu I did notice one thing at a very quick lookover at the docs - it seems to be completely ignoring the key/value parameters given on a row, and stops reporting after the auth method? That seems bad. And also, probably host/mask should be using the inet style datatypes and not text? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] db_user_namespace a "temporary measure"
On Wed, Mar 12, 2014 at 3:52 PM, Tom Lane wrote: > Magnus Hagander writes: > >> Yeah, what we really need is encapsulated per-DB users and local > >> superusers. I think every agrees that this is the goal, but nobody > >> wants to put in the work to implement a generalized solution. > > > Encapsulated would probably be the doable part. But local superuser? > Given > > that a superuser can load and run binaries, how would you propose you > > restrict that superuser from doing anything they want? And if you don't > > need that functionality, then hows it really different from being the > > database owner? > > A local user with the superuser privilege would not be able to log into > another database, because superuser doesn't give you any extra privilege > until you've logged in. > > Yeah, as superuser you could still break things as much as you pleased, > but not through SQL. > You could COPY over the hba file or sometihng like that :) Or just pg_read_binary_file() on the files in another database, which is accessible through SQL as well. I share your doubts as to how useful such a concept actually is, but > it'd work if we had real local users. > It can also do interesting things like ALTER SYSTEM, replication, backups, etc. All of which could be used to escalate privileges beyond the local database. So you'd have to somehow restrict those, at which point what's the point of the property in the first place? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] db_user_namespace a "temporary measure"
On Mar 12, 2014 1:46 AM, "Josh Berkus" wrote: > > On 03/11/2014 06:57 AM, Tom Lane wrote: > > Mind you, I wouldn't be unhappy to see it go away; it's a kluge and always > > has been. I'm just expecting lots of push-back if we try. And it's kind > > of hard to resist push-back when you don't have a substitute to offer. > > Yeah, what we really need is encapsulated per-DB users and local > superusers. I think every agrees that this is the goal, but nobody > wants to put in the work to implement a generalized solution. > Encapsulated would probably be the doable part. But local superuser? Given that a superuser can load and run binaries, how would you propose you restrict that superuser from doing anything they want? And if you don't need that functionality, then hows it really different from being the database owner? /Magnus
Re: [HACKERS] db_user_namespace a "temporary measure"
On Tue, Mar 11, 2014 at 2:40 PM, Tom Lane wrote: > Magnus Hagander writes: > > On Sun, Mar 9, 2014 at 9:00 PM, Thom Brown wrote: > >> It will be 12 years this year since this "temporary measure" was > >> added. I'm just wondering, is there any "complete solution" that > >> anyone had in mind yet? Or should this just be deprecated? > > > I'd say +1 to remove it. That would also make it possible to get id of > > "password" authentication... > > If we remove it without providing a substitute feature, people who are > using it will rightly complain. > > Are you claiming there are no users, and if so, on what evidence? > I am claiming that I don't think anybody is using that, yes. Based on the fact that I have *never* come across it on any system I've come across since, well, forever. Except once I think, many years ago, when someone had enabled it by mistake and needed my help to remove it... But we should absolutely deprecate it first in that place. Preferrably visibly (e.g. with a log message when people use it). That could at least get those people who use it to let us know they do, to that way figure out if they do - and can de-deprecate it. Or if someone wants to fix it properly of course :) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] db_user_namespace a "temporary measure"
On Sun, Mar 9, 2014 at 9:00 PM, Thom Brown wrote: > Hi, > > I've noticed that "db_user_namespace" has had the following note > attached to it since 2002: > > "This feature is intended as a temporary measure until a complete > solution is found. At that time, this option will be removed." > > It will be 12 years this year since this "temporary measure" was > added. I'm just wondering, is there any "complete solution" that > anyone had in mind yet? Or should this just be deprecated? > I'd say +1 to remove it. That would also make it possible to get id of "password" authentication... But we might want to officially deprecate it for 9.4, and then remove it later? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] jsonb and nested hstore
On Thu, Mar 6, 2014 at 4:25 PM, Tom Lane wrote: > Bruce Momjian writes: > > OK, just to summarize: > > > JSONB and everything it shares with hstore will be in core > > hstore-specific code stays in contrib > > hstore contrib will create an hstore type to call contrib and core > code > > 9.4 hstore has some differences from pre-9.4 > > I've got a problem with the last part of that. AFAICS, the value > proposition for hstore2 largely fails if it's not 100% upward compatible > with existing hstore, both as to on-disk storage and as to application- > visible behavior. If you've got to adapt your application anyway, why > not switch to JSONB which is going to offer a lot of benefits in terms > of available code you can work with? > > Although I've not looked at the patch, it was claimed upthread that there > were changes in the I/O format for existing test cases, for example. > IMO, that's an absolute dead no-go. > > > The question is whether we change/improve hstore in 9.4, or create an > > hstore2 that is the improved hstore for 9.4 and keep hstore identical to > > pre-9.4. That last option looks an awful like the dreaded VARCHAR2. > > I think hstore2 as a separate type isn't likely to be a win either. > > The bottom line here is that hstore2 is more or less what we'd agreed to > doing back at the last PGCon, but that decision has now been obsoleted by > events in the JSON area. If jsonb gets in, I think we probably end up > rejecting hstore2 as such. Or at least, that's what we should do IMO. > contrib/hstore is now a legacy type and we shouldn't be putting additional > work into it, especially not work that breaks backwards compatibility. > (not read up on the full details of the thread, sorry if I'm re-iterating something) I think we definitely want/need to maintain hstore compatibility. A completely separate hstore2 type that's not backwards compatible makes very little sense. However, if the new hstore type (compatible with the old one) is the wrapper around jsonb, rather than the other way around, I don't see any problem with it at all. Most future users are almost certainly going to use the json interfaces, but we don't want to leave upgraded users behind. (But of course it has to actually maintain backwards compatibility for that argument to hold) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] commit fest status and release timeline
On Mon, Mar 3, 2014 at 4:34 PM, Tomas Vondra wrote: > On 1.3.2014 18:01, Peter Eisentraut wrote: > > Status Summary. Needs Review: 36, Waiting on Author: 7, Ready for > > Committer: 16, Committed: 43, Returned with Feedback: 8, Rejected: > > 4. Total: 114. > > > > We're still on track to achieve about 50% committed patches, which > > would be similar to the previous few commit fests. So decent job so > > far. > > I'm wondering what is the best way to select a patch to review. I mean, > there are many patches with "needs review" (and often no reviewer) just > one or two comments, but when I checked the email archives there's often > a lot people discussing it. > > Do we have a list of patches that didn't get a proper review yet / badly > need another one? > > What about improving the commitfest page by displaying a number of > related e-mail messages / number of people involved? Shouldn't be > difficult to get this from the mail archives ... > I have some code for that part, that needs a coupe of rounds of final hacking and polish. I've had many targets for it, but right now the target is to be done before pgcon, so we can put it in play for the next set of commitfests. It's not going to happen for *this* one, and we don't want to distrupt the flow even more by making big changes to the tooling in the middle of it. That said, there is definitely a need :) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Securing "make check" (CVE-2014-0067)
On Sun, Mar 2, 2014 at 7:27 PM, Tom Lane wrote: > Noah Misch writes: > > One option that would simplify things is to fix only non-Windows in the > back > > branches, via socket protection, and fix Windows in HEAD only. We could > even > > do so by extending HAVE_UNIX_SOCKETS support to Windows through named > pipes. > > +1 for that solution, if it's not an unreasonable amount of work to add > named-pipe sockets in Windows. That would offer a feature to Windows > users that they didn't have before, ie the ability to restrict connections > based on filesystem permissions; so it seems useful quite aside from any > "make check" considerations. > I think it might be a bigger piece of work than we'd like - and IIRC that's one of the reasons we didn't do it from the start. Named pipes on windows do act as files on Windows, but they do *not* act as sockets. As in, they return HANDLEs, not SOCKETs, and you can't recv() and send() on them. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Securing "make check" (CVE-2014-0067)
On Sun, Mar 2, 2014 at 6:20 AM, Noah Misch wrote: > On Sat, Mar 01, 2014 at 05:51:46PM -0500, Andrew Dunstan wrote: > > On 03/01/2014 05:10 PM, Tom Lane wrote: > > >One other thought here: is it actually reasonable to expend a lot of > effort > > >on the Windows case? I'm not aware that people normally expect a > Windows > > >box to have multiple users at all, let alone non-mutually-trusting > users. > > > > As Stephen said, it's fairly unusual. There are usually quite a few > > roles, but it's rare to have more than one "human" type role > > connected to the machine at a given time. > > I, too, agree it's rare. Rare enough to justify leaving the vulnerability > open on Windows, indefinitely? I'd say not. Windows itself has been > pushing > steadily toward better multi-user support over the past 15 years or so. > Releasing software for Windows as though it were a single-user platform is > backwards-looking. We should be a model in this area, not a straggler. > Terminal Services have definitely become more common over time, but with faster and cheaper virtualization, a lot of people have switched to that instead, which would remove the problem of course. I wonder how common it actually is, though, to *build postgres* on a terminal services machine with other users on it... Not saying we can't ignore it, and I gree that we should not be a straggler on this, so doing a proper fix wwould definitely be the better. > I'd be happy doing nothing in this case, or not very much. e.g. > > provide a password but not with great cryptographic strength. > > One option that would simplify things is to fix only non-Windows in the > back > branches, via socket protection, and fix Windows in HEAD only. We could > even > do so by extending HAVE_UNIX_SOCKETS support to Windows through named > pipes. That could certainly be a useful feature of it's own. But as you say, non-backpatchable. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Securing "make check" (CVE-2014-0067)
On Sat, Mar 1, 2014 at 7:09 PM, Andrew Dunstan wrote: > > On 03/01/2014 12:29 PM, Tom Lane wrote: > > >> In the case of Unix systems, there is a *far* simpler and more portable >> solution technique, which is to tell the test postmaster to put its socket >> in some non-world-accessible directory created by the test scaffolding. >> > > > +1 - I'm all for KISS. > > > >> Of course that doesn't work for Windows, which is why we looked at the >> random-password solution. But I wonder whether we shouldn't use the >> nonstandard-socket-location approach everywhere else, and only use random >> passwords on Windows. That would greatly reduce the number of cases to >> worry about for portability of the password-generation code; and perhaps >> we could also push the crypto issue into reliance on some Windows-supplied >> functionality (though I'm just speculating about that part). >> > > > See for example <http://msdn.microsoft.com/en-us/library/windows/desktop/ > aa379942%28v=vs.85%29.aspx> > For a one-off password used locally only, we could also consider just using a guid, and generate it using http://msdn.microsoft.com/en-us/library/windows/desktop/aa379205(v=vs.85).aspx. Obviously windows only though - do we have *any* Unix platforms that can't do unix sockets? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] [bug fix] postgres.exe fails to start on Windows Server 2012 due to ASLR
On Tue, Feb 18, 2014 at 3:28 PM, Tom Lane wrote: > Magnus Hagander writes: > > I've applied this and backpatched to 9.3 and 9.2, which is as far back as > > it goes cleanly. > > > In 9.1 the build system looked significantly different, which makes it > > strange since the original report in this thread was about 9.1 but the > > patch supplied clearly wasn't for that. > > > Does somebody want to look at backpatching this to 9.1 and earlier, or > > should we just say that it's not fully supported on those Windows > versions > > unless you apply the registry workaround? > > The question I think needs to be asked is not that, but "what about the > mingw and cygwin builds?". > Cygwin has their own implementation of fork(). So the fix there has to be done in cygwin and not us - presumably they have either already fixed it, or will fix it in order to get general compatibility with the new ASLR versions. Mingw is a different story. But some googling indicates that mingw disables ASLR by default. You need to specify --dynamicbase if you want to to be dynamic, which we don't. (Being mingw I've not managed to find any actual docs, but for example http://lists.cypherpunks.ca/pipermail/otr-dev/2012-August/001373.html seems to indicate it has to be enabled). -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] [review] PostgreSQL Service on Windows does not start if data directory given is relative path
On Mon, Feb 3, 2014 at 3:06 PM, MauMau wrote: > From: "Rajeev rastogi" > > I will update commitFest with the latest patch immediately after sending >> the mail. >> > > OK, done setting the status to ready for committer. > > We already have two different versions of make_absolute_path() in the tree - one in src/backend/utils/init/miscinit.c and one in src/test/regress/pg_regress.c. I don't think we need a third one. If we put it in port/ like this patch done, then we should make the other callers use it. We might need one for frontend and one for backend (I haven't looked into that much detail), but definitely the one between pg_regress and pg_ctl should be shared. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] [bug fix] postgres.exe fails to start on Windows Server 2012 due to ASLR
On Tue, Feb 4, 2014 at 12:57 PM, Craig Ringer wrote: > On 02/04/2014 07:28 PM, MauMau wrote: > >> > > > > Please don't mind, I didn't misunderstand your intent. I think we > > should apply this in the next minor release to avoid unnecessary > > confusion -- more new users would use PostgreSQL on Windows 8/2012 and > > hit this problem. > > > > I added this patch to the CommitFest. > > It's really a bugfix suitable for backpatching IMO. > > Unfortunately we missed the releases that have just been wrapped. I've applied this and backpatched to 9.3 and 9.2, which is as far back as it goes cleanly. In 9.1 the build system looked significantly different, which makes it strange since the original report in this thread was about 9.1 but the patch supplied clearly wasn't for that. Does somebody want to look at backpatching this to 9.1 and earlier, or should we just say that it's not fully supported on those Windows versions unless you apply the registry workaround? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] HBA files w/include support?
On Fri, Feb 14, 2014 at 3:32 PM, Bruce Momjian 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] Prevent pg_basebackup -Fp -D -?
On Thu, Feb 13, 2014 at 4:10 AM, Bruce Momjian wrote: > On Thu, Oct 3, 2013 at 06:50:57AM +0200, Magnus Hagander wrote: > > > > On Oct 3, 2013 2:47 AM, "Michael Paquier" > wrote: > > > > > > On Wed, Oct 2, 2013 at 11:31 PM, Magnus Hagander > wrote: > > > > Right now, if you use > > > > > > > > pg_basebackup -Ft -D - > > > > > > > > you get a tarfile, written to stdout, for redirection. > > > > > > > > However, if you use: > > > > > > > > pg_basebackup -Fp -D - > > > > > > > > you get a plaintext (unpackaged) backup, in a directory called "-". > > > > > > > > I can't think of a single usecase where this is a good idea. > Therefor, > > > > I would suggest we simply throw an error in this case, instead of > > > > creating the directory. Only for the specific case of specifying > > > > exactly "-" as a directory. > > > > > > > > Comments? > > > Isn't this a non-problem? This behavior is in line with the > > > documentation, so I would suspected that if directory name is > > > specified as "-" in plain mode, it should create the folder with this > > > name. > > > Do you consider having a folder of this name an annoyance? > > > > Yes, that is exactly the point - i do consider that an annoyance, and i > don't > > see the use case where you'd actually want it. I bet 100% of the users > of that > > have been accidental, thinking they'd get the pipe, not the directory. > > > > > > Also, if we do that, is this something we should consider > > > > backpatchable? It's not strictly speaking a bugfix, but I'd say it > > > > fixes some seriously annoying behavior. > > > This would change the spec of pg_basebackup, so no? Does the current > > > behavior have potential security issues? > > > > No, there are no security issues that I can see. Just annoyance. And > yes, I > > guess it would change the spec, so backpatching might be a bad idea.. > > Has this been fixed? If so, I don't see it. > It has not. I think the thread wasn't entirely clear on if we wanted it or not, which is why I was waiting for more input from others. And then promptly forgot about it since nobody spoke up :) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Terminating pg_basebackup background streamer
On Wed, Feb 12, 2014 at 10:53 PM, Peter Eisentraut wrote: > On 2/12/14, 4:34 PM, Magnus Hagander wrote: > > On Wed, Feb 12, 2014 at 10:28 PM, Peter Eisentraut > <mailto:pete...@gmx.net>> wrote: > > > > On 2/12/14, 12:47 PM, Magnus Hagander wrote: > > > Since there were no other objections, I've applied this patch. > > > > I'm getting a compiler warning: > > > > pg_basebackup.c:105:3: error: implicit declaration of function 'kill' > > [-Werror=implicit-function-declaration] > > > > > > What platform is that? And do you know which header the declaration > > actually lives in? I don't see it here... > > OS X, according to man page > Are you sure you made that test after my fixup patch (the one suggested by Andres)? Because that one was at least supposed to add signal.h... -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Terminating pg_basebackup background streamer
On Wed, Feb 12, 2014 at 10:28 PM, Peter Eisentraut wrote: > On 2/12/14, 12:47 PM, Magnus Hagander wrote: > > Since there were no other objections, I've applied this patch. > > I'm getting a compiler warning: > > pg_basebackup.c:105:3: error: implicit declaration of function 'kill' > [-Werror=implicit-function-declaration] > What platform is that? And do you know which header the declaration actually lives in? I don't see it here... -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Terminating pg_basebackup background streamer
On Mon, Feb 10, 2014 at 7:39 PM, Magnus Hagander wrote: > On Mon, Feb 10, 2014 at 7:29 PM, Heikki Linnakangas < > hlinnakan...@vmware.com> wrote: > >> On 02/09/2014 02:17 PM, Magnus Hagander wrote: >> >>> If an error occurs in the foreground (backup) process of pg_basebackup, >>> and >>> we exit in a controlled way, the background process (streaming xlog >>> process) would stay around and keep streaming. >>> >>> This can happen for example if disk space runs out and there is very low >>> activity on the server. (If there is activity on the server, the >>> background >>> streamer will also run out of disk space and exit) >>> >>> Attached patch kills it off in disconnect_and_exit(), which seems like >>> the >>> right thing to do to me. >>> >>> Any objections to applying and backpatching that for the upcoming minor >>> releases? >>> >> >> Do you get a different error message with this patch than before? Is the >> new one better than the old one? > > > Previously you got double error messages - one from the foreground, and a > second one from the background sometime in the future (whenever it > eventually failed, and for whatever reason - so if it was out of disk > space, it would complain about that once it got enough xlog for it to > happen). > > With the patch you just get the error message from the first process. The > background process doesn't give an error on SIGTERM, it just exists. > > > Since there were no other objections, I've applied this patch. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] narwhal and PGDLLIMPORT
On Feb 12, 2014 4:09 AM, "Craig Ringer" wrote: > > On 02/12/2014 09:45 AM, Tom Lane wrote: > > Andrew Dunstan writes: > >> On 02/11/2014 08:04 PM, Craig Ringer wrote: > >>> Looks like currawong doesn't build postgres_fdw. > > > >> It sure used to: > >> < http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=currawong&dt=2014-02-03%2005%3A30%3A00&stg=make > > > > > Hm, does the MSVC build system do parallel builds? Maybe it > > abandoned the build after noticing the first failure, and > > whether you get to see more than one is timing-dependent. > > I'm pretty certain it does, and that's quite a reasonable explanation. It definitely does. It does it both between individual c files in a project, and between projects (taking care about dependencies). And yes, I've seen that cause it to build modules, particularly smaller ones, in different order at different invocations based on small differences in timing. And yes, it aborts on first failure. So it would surprise me if that is *not* the problem... > That reminds me - in addition to the option to force a serial build, I > really need to post a patch for the msvc build that lets you suppress > VERBOSE mode. That probably wouldn't hurt :-) /Magnus
Re: [HACKERS] Terminating pg_basebackup background streamer
On Mon, Feb 10, 2014 at 7:29 PM, Heikki Linnakangas wrote: > On 02/09/2014 02:17 PM, Magnus Hagander wrote: > >> If an error occurs in the foreground (backup) process of pg_basebackup, >> and >> we exit in a controlled way, the background process (streaming xlog >> process) would stay around and keep streaming. >> >> This can happen for example if disk space runs out and there is very low >> activity on the server. (If there is activity on the server, the >> background >> streamer will also run out of disk space and exit) >> >> Attached patch kills it off in disconnect_and_exit(), which seems like the >> right thing to do to me. >> >> Any objections to applying and backpatching that for the upcoming minor >> releases? >> > > Do you get a different error message with this patch than before? Is the > new one better than the old one? Previously you got double error messages - one from the foreground, and a second one from the background sometime in the future (whenever it eventually failed, and for whatever reason - so if it was out of disk space, it would complain about that once it got enough xlog for it to happen). With the patch you just get the error message from the first process. The background process doesn't give an error on SIGTERM, it just exists. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
[HACKERS] Terminating pg_basebackup background streamer
If an error occurs in the foreground (backup) process of pg_basebackup, and
we exit in a controlled way, the background process (streaming xlog
process) would stay around and keep streaming.
This can happen for example if disk space runs out and there is very low
activity on the server. (If there is activity on the server, the background
streamer will also run out of disk space and exit)
Attached patch kills it off in disconnect_and_exit(), which seems like the
right thing to do to me.
Any objections to applying and backpatching that for the upcoming minor
releases?
Should we perhaps also consider adding a sigterm handler to pg_basebackup,
so if you send it a SIGTERM it kills off the background process as well?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index a6e320c..c27c633 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -76,6 +76,7 @@ static PQExpBuffer recoveryconfcontents = NULL;
/* Function headers */
static void usage(void);
+static void disconnect_and_exit(int code);
static void verify_dir_is_empty_or_create(char *dirname);
static void progress_report(int tablespacenum, const char *filename, bool force);
@@ -88,6 +89,26 @@ static void BaseBackup(void);
static bool reached_end_position(XLogRecPtr segendpos, uint32 timeline,
bool segment_finished);
+
+static void disconnect_and_exit(int code)
+{
+ if (conn != NULL)
+ PQfinish(conn);
+
+#ifndef WIN32
+ /*
+ * On windows, our background thread dies along with the process.
+ * But on Unix, if we have started a subprocess, we want to kill
+ * it off so it doesn't remain running trying to stream data.
+ */
+ if (bgchild> 0)
+ kill(bgchild, SIGTERM);
+#endif
+
+ exit(code);
+}
+
+
#ifdef HAVE_LIBZ
static const char *
get_gz_error(gzFile gzf)
diff --git a/src/bin/pg_basebackup/pg_receivexlog.c b/src/bin/pg_basebackup/pg_receivexlog.c
index 8a702e3..0f191ce 100644
--- a/src/bin/pg_basebackup/pg_receivexlog.c
+++ b/src/bin/pg_basebackup/pg_receivexlog.c
@@ -45,6 +45,13 @@ static void StreamLog();
static bool stop_streaming(XLogRecPtr segendpos, uint32 timeline,
bool segment_finished);
+#define disconnect_and_exit(code)\
+ { \
+ if (conn != NULL) PQfinish(conn); \
+ exit(code); \
+ }
+
+
static void
usage(void)
{
diff --git a/src/bin/pg_basebackup/streamutil.h b/src/bin/pg_basebackup/streamutil.h
index bb3c34d..7c7d022 100644
--- a/src/bin/pg_basebackup/streamutil.h
+++ b/src/bin/pg_basebackup/streamutil.h
@@ -11,10 +11,4 @@ extern char *replication_slot;
/* Connection kept global so we can disconnect easily */
extern PGconn *conn;
-#define disconnect_and_exit(code)\
- { \
- if (conn != NULL) PQfinish(conn); \
- exit(code); \
- }
-
extern PGconn *GetConnection(void);
--
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] pg_basebackup: progress report max once per second
On Mon, Feb 3, 2014 at 4:14 AM, Sawada Masahiko wrote: > On Sat, Feb 1, 2014 at 8:29 PM, Oskari Saarenmaa wrote: > > 31.01.2014 10:59, Sawada Masahiko kirjoitti: > > > > I think the idea in the new progress_report() call (with force == true) > is > > to make sure that there is at least one progress_report call that > actually > > writes the progress report. Otherwise the final report may go missing > if it > > gets suppressed by the time-based check. The force argument as used in > the > > new call skips that check. > > > > I understood. > > I have two concerns as follows. > - I think that there is possible that progress_report() is called > frequently ( less than 1 second). > That is, progress_report() is called with force == true after > progress_report was called with force == false and execute this > function. > - progress_report() is called even if -P option is disabled. I'm > concerned about that is cause of performance degradation. > I looked over the latest version, and the only real problem I see here is your second point, which is the calling with -P not specified. I doubt it's going to be much, but in theory I guess the call to time(NULL) many times could have an effect. I've fixed that by just moving it to after a check for showprogress. As for the first one - I believe that's the point. progress_report should be called with force==true after it was called with it false, that's the intended design. I've applied the patch, with that minor adjustment and an added comment. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Add min and max execute statement time in pg_stat_statement
On Wed, Jan 29, 2014 at 8:58 AM, Peter Geoghegan wrote: > I am not opposed in principle to adding new things to the counters > struct in pg_stat_statements. I just think that the fact that the > overhead of installing the module on a busy production system is > currently so low is of *major* value, and therefore any person that > proposes to expand that struct should be required to very conclusively > demonstrate that there is no appreciably increase in overhead. Having > a standard deviation column would be nice, but it's still not that > important. Maybe when we have portable atomic addition we can afford > to be much more inclusive of that kind of thing. > Not (at this point) commenting on the details, but a big +1 on the fact that the overhead is low is a *very* important property. If the overhead starts growing it will be uninstalled - and that will make things even harder to diagnose. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Add min and max execute statement time in pg_stat_statement
On Wed, Jan 29, 2014 at 9:03 AM, KONDO Mitsumasa < kondo.mitsum...@lab.ntt.co.jp> wrote: > (2014/01/29 15:51), Tom Lane wrote: > > KONDO Mitsumasa writes: > >> By the way, latest pg_stat_statement might affect performance in > Windows system. > >> Because it uses fflush() system call every creating new entry in > >> pg_stat_statements, and it calls many fread() to warm file cache. > > This statement doesn't seem to have much to do with the patch as > > committed. There are no fflush calls, and no notion of warming the > > file cache either. > Oh, all right. > > > We do assume that the OS is smart enough to keep > > a frequently-read file in cache ... is Windows too stupid for that? > I don't know about it. But I think Windows cache feature is stupid. > It seems to always write/read data to/from disk, nevertheless having large > memory... > I'd like to know test result on Windows, if we can... > > I am quite certain the Windows cache is *not* that stupid, and hasn't been since the Windows 3.1 days. If you want to make certain, set FILE_ATTRIBUTE_TEMPORARY when the file is opened, then it will work really hard not to write it to disk - harder than most Linux fs'es do AFAIK. This should of course only be done if we don't mind it going away :) As per port/open.c, pg will set this when O_SHORT_LIVED is specified. But AFAICT, we don't actually use this *anywhere* in the backend? Perhaps we should for this - and also for the pgstat files? (I may have missed a codepath, only had a minute to do some greping) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] pg_basebackup and pg_stat_tmp directory
On Tue, Jan 28, 2014 at 6:11 AM, Amit Kapila wrote: > On Tue, Jan 28, 2014 at 9:26 AM, Fujii Masao > wrote: > > Hi, > > > > The files in pg_stat_tmp directory don't need to be backed up because > they are > > basically reset at the archive recovery. So I think it's worth > > changing pg_basebackup > > so that it skips any files in pg_stat_tmp directory. Thought? > > I think this is good idea, but can't it also avoid > PGSTAT_STAT_PERMANENT_TMPFILE along with temp files in > pg_stat_tmp > > All stats files should be excluded. IIRC the PGSTAT_STAT_PERMANENT_TMPFILE refers to just the global one. You want to exclude based on PGSTAT_STAT_PERMANENT_DIRECTORY (and of course based on the guc stats_temp_directory if it's in PGDATA. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] shouldn't we log permission errors when accessing the configured trigger file?
On Mon, Jan 27, 2014 at 3:43 PM, Robert Haas wrote:
> On Sun, Jan 26, 2014 at 1:03 PM, Andres Freund
> wrote:
> > For some reason CheckForStandbyTrigger() doesn't report permission
> > errors when stat()int the trigger file. Shouldn't we fix that?
> >
> > static bool
> > CheckForStandbyTrigger(void)
> > {
> > ...
> > if (stat(TriggerFile, &stat_buf) == 0)
> > {
> > ereport(LOG,
> > (errmsg("trigger file found: %s",
> TriggerFile)));
> > unlink(TriggerFile);
> > triggered = true;
> > fast_promote = true;
> > return true;
> > }
> >
> > Imo the stat() should warn about all errors but ENOENT?
>
> Seems reasonable. It could lead to quite a bit of log spam, I
> suppose, but the way things are now could be pretty mystifying if
> you've located your trigger file somewhere outside $PGDATA, and a
> parent directory is lacking permissions.
>
+1. Since it actually indicates something that's quite broken (since with
that you can never make the trigger work until you fix it), the log spam
seems like it would be appropriate. (Logspam is never nice, but a single
log line is also very easy to miss - this should log enough that you
wouldn't)
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
[HACKERS] Tablespace options in \db+
Currently, tablespace options (such as random_page_cost) aren't shown in
\db or \db+ in psql - the only way to see them is to directly query
pg_tablespaces. This seems like an oversight from back when the feature was
added.
I realize the CF is closed, but would people be ok with me pushing this
trivial patch into 9.4?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 0d4b151..43f1a1c 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -176,6 +176,11 @@ describeTablespaces(const char *pattern, bool verbose)
",\n pg_catalog.shobj_description(oid, 'pg_tablespace') AS \"%s\"",
gettext_noop("Description"));
+ if (verbose && pset.sversion >= 9)
+ appendPQExpBuffer(&buf,
+ ",\n spcoptions AS \"%s\"",
+ gettext_noop("Options"));
+
appendPQExpBufferStr(&buf,
"\nFROM pg_catalog.pg_tablespace\n");
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] effective_cache_size calculation overflow
To test something unrelated, I set my shared_buffers to 7TB on my laptop today (no, unfortunately I don't have that much RAM). That leads to the startup error: FATAL: -536870912 is outside the valid range for parameter "effective_cache_size" (-1 .. 2147483647) So clearly there is an overflow somewhere in the calculation of effective_cache_size, most likely from the fact that it's now dynamically calculated. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Visual Studio 2013 build
On Sun, Jan 26, 2014 at 1:13 AM, Andrew Dunstan wrote: > > On 12/02/2013 05:12 PM, Brar Piening wrote: > >> Hackers, >> the attached patch enables Microsoft Visual Studio 2013 as additional >> build environment. >> After some tweaking (VS now has got its own rint and a few macro >> definitions that were previously missing) the build runs without errors or >> warnings and the product passes the regression tests. >> I didn't test any special configurations though. >> I'm using full Visual Studio 2013 actually so I can't conform that >> everything still works with Visual Studio Express 2013 for Windows >> Desktop, but there are no documented limitations that make any problems >> foreseeable. >> I will add it to the CommitFest 2014-01 so that there is time for testing >> and tweaking. >> >> > > OK, I have tested this out with the development branch and Visual Studio > Express 2013 for Windows Desktop, on Windows Server 2008 R2-SP1 64 bit. > With a slight adjustment to make the patch apply it works fine. > > How far back should we go? About a year ago when we did this we applied it > for 9.2 (then the latest stable release) and 9.3dev. > Seems reasonable to follow the same pattern, and apply it for 9.3 stable and 9.4dev. The argument being that it's a new build env, and it's not likely that people are going to use that t o build very old versions of postgres. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] extension_control_path
On Sat, Jan 25, 2014 at 6:07 PM, Dimitri Fontaine wrote: > Magnus Hagander writes: > > Using colon as the path separator is going to break on windows. The patch > > notices this and uses semicolon on Windows instead. Do we really want to > go > > down that path - that means that everybody who writes any sorts of > > installation instructions including this will have to make them separate > > for different platforms. Shouldn't we just use semicolon on all > platforms, > > for consistency? > > Well, I've been considering that what I found already in the backend to > solve the same problem was a valid model to build against. > > Pick any reasonnable choice you want to, fix dynamic_library_path along > the new lines or maybe ask me to, and then let's apply the same design > to the new GUC doing about exactly the same thing? > > Ha, I didn't realize dynamic_library_paty had the same problem. In fact, I have to admit I didn't realize I could put more than one path in there - I don't think I've ever used that :D So based on the previous behaviour there, I withdraw my comment - being consistent with the existing behaviour of that parameter makes perfect sense. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] proposal: hide application_name from other users
On Fri, Jan 24, 2014 at 5:21 PM, Harold Giménez wrote: > On Fri, Jan 24, 2014 at 6:46 AM, Magnus Hagander > wrote: > > > > On Thu, Jan 23, 2014 at 2:01 AM, Greg Stark wrote: > >> > >> On Wed, Jan 22, 2014 at 1:03 PM, Josh Berkus wrote: > >> > Probably Heroku has some more specific exploit case to be concerned > >> > about here; if so, might I suggest taking it up with the -security > list? > >> > >> I don't think there's a specific vulnerability that needs to be kept > >> secret here. > >> > >> Here's an example. I just created a new "hobby" database which is on a > >> multi-tenant cluster and ran select * from pg_stat_activity. Here are > >> two of the more interesting examples: > >> > >> 463752 | de5nmf0gbii3u5 | 32250 | 463751 | qspfkgrwgqtbcu | unicorn > >> worker[1] -p 30390 -c ./config/unicorn.rb || > >> | | | > >> | | > >> | || > >> 463752 | de5nmf0gbii3u5 | 32244 | 463751 | qspfkgrwgqtbcu | unicorn > >> worker[0] -p 30390 -c ./config/unicorn.rb || > >> | | | > >> | | > >> | || > >> > >> > >> Note that the contents of the ARGV array are being set by the > >> "unicorn" task queuing library. It knows it's making this information > >> visible to other users with shell access on this machine. But the > >> decision to stuff the ARGV information into the application_name is > >> being made by the Pg driver. Neither is under the control of the > >> application author who may not even be aware this is happening. > >> Neither component has the complete information to make a competent > >> decision about whether this information is safe to be in > >> application_name or not. > >> > >> Note that the query is showing as "" even > >> though it is listed in the ps output -- the same ps output that is > >> listing the unicorn ARGV that is being shown in the > >> application_name > >> > >> You might say that the Pg gem is at fault for making a blanket policy > >> decision for applications that the ARGV is safe to show to other > >> database users but realistically it's so useful to see this > >> information for your own connections that it's probably the right > >> decision. Without it it's awfully hard to tell which worker is on > >> which connection. It would just be nice to be able to treat > >> application_name the same as query. > > > > > > I would say that yes, this is clearly broken in the Pg gem. I can see it > > having such a default, but not allowing an override... > > Uhm, it does allow an override as I said before. > Oops, sorry, I missed that when reading back in the thread. > > The application can of course issue a SET application_name, assuming > there > > is a hook somewhere in the system that will run after the connection has > > been established. I've had customers use that many times in java based > > systems for example, but I don't know enough about the pg gem, or > unicorn, > > to have a clue if anything like it exists there. This is also a good way > to > > track how connections are used throughout a pooled system where the same > > connection might be used for different things at different times. > > > > What actually happens if you set the application_name in the connection > > string in that environment? Does it override it to it's own default? If > so, > > the developers there clearly need to be taught about > > fallback_application_name. > > It can be overridden using any of these methods. It does in fact use > fallback_application_name when it defaults to $0, see > > https://bitbucket.org/ged/ruby-pg/src/6c2444dc63e17eb695363993e8887cc5d67750bc/lib/pg/connection.rb?at=default#cl-46 > > > > > And what happens if you set it in PGAPPNAME? > > It works fine: > ... With that many options of "hiding" it, I would still argue for just picking one of those. For example, of Heroku wants to protect their customers against the behaviour of the pg gem, you can for example set PGAPPNAME in the environment. That will override what the gem sets in fallback_application_name, but those users that actually use it and specify it in their connection string, will override that default. And all of that without removing a valuable debugging/tracing tool from other users. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] proposal: hide application_name from other users
On Sat, Jan 25, 2014 at 10:42 AM, Greg Stark wrote: > On Fri, Jan 24, 2014 at 6:46 AM, Magnus Hagander > wrote: > > What actually happens if you set the application_name in the connection > > string in that environment? Does it override it to it's own default? If > so, > > the developers there clearly need to be taught about > > fallback_application_name. > > > > And what happens if you set it in PGAPPNAME? > > My point wasn't that an application couldn't control this. The point > is that this isn't so easy to manage and users might not realize > there's anything to do. > > And it's not necessarily the case that the library could warn users. > No one of the parts of the code here has the whole picture. In this > case one part of the code is stuffing the information in $0 and > another part is defaulting application_name to $0. > We still show the ip address. And the client port number. and the username. And the database. These may also give away information. No, not as much as if you stick a password into application_name for example, but they still give out information. Perhaps what you really would need is for pg_stat_activity to be *completely* superuser only? Because it *does* tell you about what other users are doing. Now, actually having the ability to do that would be a good thing, because there are certainly environments where it might make sense. But that's back to the "long term solution" of actually making it configurable. Not cherry-picking which features should break for some users and not others. > Long term I agree we should really have some way of controlling these > > permissions more fine grained, but I just blanket hiding application name > > for non-superusers seems like a bad solution that still only fixes a > small > > part of the problem. > > It makes a lot of sense to me to treat it the same way as sql_query. > It's pretty similar (especially in the above given that we put the sql > query in $0 after all) > Except we *don't* put the SQL query in $0. We only put "SELECT" (or other commandtags), not the actual contents of the query. So *we* make sure we don't put the sensitive information there, since the wrong people may see it in argv, because that's our responsibility. Just like it's the responsibility of the client to make sure they don't put security sensitive information in application_name. If we restrict application_name to superusers only this way, we punish those who *don't* do the wrong thing by requiring their monitoring to now use superuser, in favor of those who *do* the wrong thing, which is put security sensitive information in application_name. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] extension_control_path
I haven't actually looked at the patch itself, but I noted this from the other review: On Fri, Jan 24, 2014 at 6:43 PM, Sergey Muraviov < sergey.k.murav...@gmail.com> wrote: > = > > postgresql.conf: >extension_control_path = > '/extensions/postgis-2.0.4:/extensions/postgis-2.1.1' > > Using colon as the path separator is going to break on windows. The patch notices this and uses semicolon on Windows instead. Do we really want to go down that path - that means that everybody who writes any sorts of installation instructions including this will have to make them separate for different platforms. Shouldn't we just use semicolon on all platforms, for consistency? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] proposal: hide application_name from other users
On Thu, Jan 23, 2014 at 2:01 AM, Greg Stark wrote: > On Wed, Jan 22, 2014 at 1:03 PM, Josh Berkus wrote: > > Probably Heroku has some more specific exploit case to be concerned > > about here; if so, might I suggest taking it up with the -security list? > > I don't think there's a specific vulnerability that needs to be kept > secret here. > > Here's an example. I just created a new "hobby" database which is on a > multi-tenant cluster and ran select * from pg_stat_activity. Here are > two of the more interesting examples: > > 463752 | de5nmf0gbii3u5 | 32250 | 463751 | qspfkgrwgqtbcu | unicorn > worker[1] -p 30390 -c ./config/unicorn.rb || > | | | > | | > | || > 463752 | de5nmf0gbii3u5 | 32244 | 463751 | qspfkgrwgqtbcu | unicorn > worker[0] -p 30390 -c ./config/unicorn.rb || > | | | > | | > | || > > > Note that the contents of the ARGV array are being set by the > "unicorn" task queuing library. It knows it's making this information > visible to other users with shell access on this machine. But the > decision to stuff the ARGV information into the application_name is > being made by the Pg driver. Neither is under the control of the > application author who may not even be aware this is happening. > Neither component has the complete information to make a competent > decision about whether this information is safe to be in > application_name or not. > > Note that the query is showing as "" even > though it is listed in the ps output -- the same ps output that is > listing the unicorn ARGV that is being shown in the > application_name > > You might say that the Pg gem is at fault for making a blanket policy > decision for applications that the ARGV is safe to show to other > database users but realistically it's so useful to see this > information for your own connections that it's probably the right > decision. Without it it's awfully hard to tell which worker is on > which connection. It would just be nice to be able to treat > application_name the same as query. I would say that yes, this is clearly broken in the Pg gem. I can see it having such a default, but not allowing an override... The application can of course issue a SET application_name, assuming there is a hook somewhere in the system that will run after the connection has been established. I've had customers use that many times in java based systems for example, but I don't know enough about the pg gem, or unicorn, to have a clue if anything like it exists there. This is also a good way to track how connections are used throughout a pooled system where the same connection might be used for different things at different times. What actually happens if you set the application_name in the connection string in that environment? Does it override it to it's own default? If so, the developers there clearly need to be taught about fallback_application_name. And what happens if you set it in PGAPPNAME? Long term I agree we should really have some way of controlling these permissions more fine grained, but I just blanket hiding application name for non-superusers seems like a bad solution that still only fixes a small part of the problem. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
