Re: [HACKERS] pg_basebackup from cascading standby after timeline switch

2012-12-27 Thread Heikki Linnakangas

On 23.12.2012 15:33, Fujii Masao wrote:

On Fri, Dec 21, 2012 at 9:54 PM, Heikki Linnakangas
hlinnakan...@vmware.com  wrote:

Yes, this should be backpatched to 9.2. I came up with the attached.


In this patch, if '-X stream' is specified in pg_basebackup, the timeline
history files are not backed up.


Good point.


We should change pg_backup background
process and walsender so that they stream also timeline history files,
for example, by using 'TIMELINE_HISTORY' replication command?
Or basebackup.c should send all timeline history files at the end of backup
even if '-X stream' is specified?


Perhaps. We should enhance pg_receivexlog to follow timeline switches, 
anyway. I was thinking of leaving that as a todo item, but pg_basebackup 
-X stream shares the code, so we should implement that now to get that 
support into both.


In the problem you reported on the other thread 
(http://archives.postgresql.org/message-id/50db5ea9.7010...@vmware.com), 
you also need the timeline history files, but that one didn't use -X 
at all. Even if we teach pg_basebackup to fetch the timeline history 
files in -X stream mode, that still leaves the problem on that other 
thread.


The simplest solution would be to always include all timeline history 
files in the backup, even if -X is not used. Currently, however, pg_xlog 
is backed up as an empty directory in that case, but that would no 
longer be the case if we start including timeline history files there. I 
wonder if that would confuse any existing backup scripts people are using.


- Heikki


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Proposal: Store timestamptz of database creation on pg_database

2012-12-27 Thread Stephen Frost
* Josh Berkus (j...@agliodbs.com) wrote:
 This information could be extremely useful for forensics, debugging, ETL
 processes (many of which create tables as part of their processes), etc.
 
 I'd say moderately useful at best.  Quite a number of things could
 make the creation dates misleading or not distinctive (think
 partition replacement, restore from pg_dump, replicas, etc.).
 ALTER dates would be more useful, but as Tom points out, would need
 the user-configurability which can only be delivered by something
 like event triggers.

To be honest, I really just don't find this to be *that* difficult and
an intuitive set of rules which are well documented feels like it'd
cover 99% of the cases.  pg_dump would preserve the times (though it
could be optional), replicas should as well, etc.  We haven't even
started talking about the 'hard' part, which would be a 'modification'
type of field..

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] buffer assertion tripping under repeat pgbench load

2012-12-27 Thread Greg Stark
On Thu, Dec 27, 2012 at 3:17 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 The thing that this theory has a hard time with is that the buffer's
 global refcount is zero.  If you assume that there's a bit that
 sometimes randomly goes to 1 when it should be 0, then what I'd expect
 to typically happen is that UnpinBuffer sees nonzero LocalRefCount and
 hence doesn't drop the session's global pin when it should.  The only
 way that doesn't happen is if decrementing LocalRefCount to zero stores
 a nonzero pattern when it should store zero, but nonetheless the CPU
 thinks it stored zero.

It seems entirely plausible when you factor in the L2 cache. The cpu
could be happily incrementing and decrementing the count entirely
correctly blissfully unaware that the value being stored in the DRAM
has this extra bit set every time. Not until the transaction ends and
it has to refetch the cache line because enough time has passed for it
to age out of L2 cache does it find the corrupt value.

 At the moment I like the other theory you alluded to, that this is a
 wild store from code that thinks it's manipulating some other data
 structure entirely.  The buffer IDs will help confirm or refute that
 perhaps.  No idea ATM how we would find the problem if it's like that

Yeah, though I have to admit the particular data structures I
mentioned are entirely irrelevant. Looking back at the code
LocalRefCount is heap allocated and BufferBlocks is actually a pointer
to shmem.

If it's always the first buffer then it could conceivably still be
some other heap allocated object that always lands before
LocalRefCount. It does seem a bit weird to be storing 130 though --
there are no 130 constants that we might be storing for example.

It occurs to me that it should be possible to walk the list of
resource owners and count up what the correct LocalRefCount should be
every time we increment or decrement it. That should catch the error
sooner -- if it's not a hardware problem that the cache is hiding.

-- 
greg


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] A stab at implementing better password hashing, with mixed results

2012-12-27 Thread Peter Bex
Hello all,

A while ago, on pgsql-general, I raised the issue that the password
storage employed by postgres is a little weak and promised I'd look
into this during the holidays, so here are my findings.

Implementing bcrypt instead of md5 is indeed rather straightforward;
just move the pgcrypto blowfish code into Postgres and tweak a few
things.  However, there seems to be no way to get the md5-based
authentication working with any different kind of storage than
plaintext or the existing md5 storage because it requires access
to the salted MD5 password hash.  Backwards compatibility can
only be guaranteed for passwords using this storage.  Adding bcrypt
means (oddly enough) that the authentication system will become *less*
secure, requiring plaintext password logins, making SSL mandatory.
However, since the existing authentication is not terribly secure
anyway that may not be considered a big loss (except maybe for
passwords popping up in accidental local network data dumps..).

Implementing a more secure challenge-response based algorithm means
a change in the client-server protocol.  Perhaps something like SCRAM
(maybe through SASL) really is the way forward for this, but that
seems like quite a project and it seems to dictate how the passwords are
stored; it requires a hash of the PBKDF2 algorithm to be stored.

One problem with a better algorithm is that for CGI-like web scripts,
connection speed is important.  The CGI model (which is also employed
by Perl and PHP, AFAIK) is in direct opposition to the security
requirement that password hashes should take long to create or verify;
if it takes long, this is a constant extra factor for all web requests
(unless advanced connection pooling is used, perhaps).  The
authentication method may also have an impact if it needs expensive
calculations.

I'm unsure how useful this bcrypt patch is, since it won't be acceptable
as-is, but I've attached it just in case.  If it is decided that this
should be integrated after all, it needs more work:

- There's no way to specify which hashing system to use for new
   passwords; I've ripped out the md5 hashing.  I think this requires
   a change in the user management statements.
- There's no way to specify (the log 2 of) the number of rounds to
   run the blowfish algorithm.  This is extremely important as it
   directly impacts the time it takes to set up a connection, like
   I said above, and some admins might like to increase the work
   factor for increased password protection.
   I think this might be a run-time server variable and/or possibly
   an option in the user management statement.
- Ideally, pgcrypto could re-use this, so that there is no duplicate
   copy of the bcrypt code.  The salt generation is less strong than
   pgcrypto's, which uses openssl AFAICT, so that might need some extra
   hooks.  The crypt code could also be exposed as an SQL function.
- Currently, if a client uses md5 auth to connect as a user who has
   a bcrypted password it simply means the correct password will be
   rejected.  There's no clean way to handle this, as far as I can see;
   giving more feedback will probably require a change in the protocol
   and will give away more information than we want: you'll know a user
   exists and even that the password isn't stored in plaintext or md5.

The conclusion is that switching to bcrypt requires a handful of extra
changes that I don't know how to make, and has the consequence of
*requiring* plaintext password authentication or a change in the
protocol.  If it's decided to go forward with this anyway, I'm willing
to help out, but implementing a completely new authentication system
goes a little over my head, I'm afraid.

Cheers,
Peter
-- 
http://sjamaan.ath.cx
--
The process of preparing programs for a digital computer
 is especially attractive, not only because it can be economically
 and scientifically rewarding, but also because it can be an aesthetic
 experience much like composing poetry or music.
-- Donald Knuth
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 569385c..c50268c 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -28,6 +28,7 @@
 #include commands/seclabel.h
 #include commands/user.h
 #include libpq/md5.h
+#include libpq/crypt-blowfish.h
 #include miscadmin.h
 #include storage/lmgr.h
 #include utils/acl.h
@@ -79,8 +80,10 @@ CreateRole(CreateRoleStmt *stmt)
ListCell   *item;
ListCell   *option;
char   *password = NULL;/* user password */
-   boolencrypt_password = Password_encryption; /* encrypt 
password? */
-   charencrypted_password[MD5_PASSWD_LEN + 1];
+   boolencrypt_password = Password_encryption; /* encrypt 
(hash) password? */
+   charsalt[16];
+   charbcrypt_setting[BCRYPT_SALT_LEN + 1];
+   char

Re: [HACKERS] pgcrypto seeding problem when ssl=on

2012-12-27 Thread Marko Kreen
On Sun, Dec 23, 2012 at 9:45 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Marko Kreen mark...@gmail.com writes:
 On Sat, Dec 22, 2012 at 9:20 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  I'm not thrilled with the suggestion to do RAND_cleanup() after forking
  though, as that seems like it'll guarantee that each session starts out
  with only a minimal amount of entropy.

  No, that's actually the right fix - that forces OpenSSL to do new reseed
  with system randomness, thus making backend (including SSL_accept)
  maximally disconnected from static pool in postmaster.

 I don't think that maximal disconnectedness is the deciding factor
 here, or even a very important factor.  If we do RAND_cleanup() then
 each new session will be forced to suck entropy from /dev/urandom
 (immediately, if an SSL connection is attempted).  This opens the door
 to quasi-denial-of-service attacks that bleed all the entropy out of the
 system, forcing long delays for new PRNG-using processes, Postgres or
 otherwise.  And long delays are the *best case* scenario --- worst case,
 if the system lacks decent /dev/random support, is that new processes
 are starting up with highly correlated PRNG states.

You cannot realistically drain /dev/urandom - it's just a CSPRNG, periodically
seeded from /dev/random.  And it cannot cause long-delays.  Sessions keys
are actually it's main use-case.

And anyway - if best way to attack Postgres SSL session is to drain and
cryptoanalyze /dev/urandom via SSL attempts, then we are in pretty good
shape.  When depening only on properly-used OpenSSL PRNG, we are still
good, but bit worse situation - it's gets minimal amount of entropy
after initial seeding, thus successful cryptoanalyze is more probable
than on /dev/urandom.  And when depending on incorrectly used OpenSSL PRNG
(current situation) then the situation is bad - we seem to be depending
on security of single hash.

 If such an approach were a good thing, the OpenSSL guys would have
 implemented it internally --- it'd be easy enough to do, just by forcing
 RAND_cleanup anytime getpid() reads differently than it did when the
 pool was set up.

I thought about it and I realized they cannot do it - the libcrypto has
no idea of application lifetime, so doing such cleanup without application
knowledge is wrong.

Eg. imagine a daemon that loads config, sets up SSL, goes background into
empty chroot.

So the current idea of hashing getpid() on each call is best they can
do to help badly written applications - like Postgres.

 gettimeofday() might not be the very best way of changing the inherited
 PRNG state from child to child, but I think it's a more attractive
 solution than RAND_cleanup.

1. It optimizes CPU for unrealistic attack scenario.

2. It spends more CPU in single-threaded postmaster when SSL is not used -
   as postmaster does not know whether incoming connection is SSL or not
   it needs to do the PRNG feeding anyway.  This might be noticeable
   in realistic short-connections scenario, where SSL is used only
   for admin or replication connections.

  This also makes behaviour equal to current ssl=off and exec-backend
  mode, which already do initial seeding in backend.

 No, it's not equal, because when ssl=off seeding attempts won't happen
 immediately after fork and thus an attacker doesn't have such an easy
 way of draining entropy.  If we do what you're suggesting, the attacker
 doesn't even need a valid database login to drain entropy --- he can
 just fire-and-forget NEGOTIATE_SSL startup packets.

You can't just fire-and-forget them - you need to do TCP handshake
plus Postgres SSL handshake; this means each request takes noticeable
amount of setup time from attacker side and on server side the sucker's
IP is visible in logs.

And /dev/urandom seeding is prettly inexpensive compared to SSL pubkey
crypto.  It does not look like a scenario we need to design against.

 (The exec-backend
 case will have such issues anyway, but who thought Windows was a secure
 OS?)

ATM the Windows is pretty good shape compared to our Unix situation...

  If you decide against RAND_cleanup in backend and instead
  do workarounds in backend or postmaster, then please
  also to periodic RAND_cleanup in postmaster.  This should
  make harder to exploit weaknesses in reused slowly moving state.

 We could consider that, but it's not apparent to me that it has any
 real value.

Properly-designed and properly-used CSPRNG feeding and extraction
should tolerate minor breaks in low-level hashes/ciphers.  If we are
not using it as intended, then we lose that guarantee and need to
limit the damage on our side.

-- 
marko


-- 
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] A stab at implementing better password hashing, with mixed results

2012-12-27 Thread Claudio Freire
On Thu, Dec 27, 2012 at 11:46 AM, Peter Bex peter@xs4all.nl wrote:

 Implementing a more secure challenge-response based algorithm means
 a change in the client-server protocol.  Perhaps something like SCRAM
 (maybe through SASL) really is the way forward for this, but that
 seems like quite a project and it seems to dictate how the passwords are
 stored; it requires a hash of the PBKDF2 algorithm to be stored.

It would be nonsense to do it in any other way... protecting the
password store and not the exchange would just shift the weak spot.


-- 
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] A stab at implementing better password hashing, with mixed results

2012-12-27 Thread Peter Bex
On Thu, Dec 27, 2012 at 12:31:08PM -0300, Claudio Freire wrote:
 On Thu, Dec 27, 2012 at 11:46 AM, Peter Bex peter@xs4all.nl wrote:
 
  Implementing a more secure challenge-response based algorithm means
  a change in the client-server protocol.  Perhaps something like SCRAM
  (maybe through SASL) really is the way forward for this, but that
  seems like quite a project and it seems to dictate how the passwords are
  stored; it requires a hash of the PBKDF2 algorithm to be stored.
 
 It would be nonsense to do it in any other way... protecting the
 password store and not the exchange would just shift the weak spot.

Yeah, that's why I was being rather pessimistic about the patch I posted.
However, SCRAM will only protect the password; SSL is still required
to protect against connection hijacking.

Cheers,
Peter
-- 
http://sjamaan.ath.cx
--
The process of preparing programs for a digital computer
 is especially attractive, not only because it can be economically
 and scientifically rewarding, but also because it can be an aesthetic
 experience much like composing poetry or music.
-- Donald Knuth


-- 
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] Proposal: Store timestamptz of database creation on pg_database

2012-12-27 Thread Dimitri Fontaine
Hi,

Tom Lane t...@sss.pgh.pa.us writes:
 This proposal is about add a column datcreated on pg_database to store
 the timestamp of the database creation.

 I'm inclined to think that anyone who really needs this should be
 pointed at event triggers.  That feature (if it gets in) will allow
 people to track creation/DDL-change times with exactly the behavior
 they want.

Agreed.

Stephen Frost sfr...@snowman.net writes:
 To be honest, I really just don't find this to be *that* difficult and
 an intuitive set of rules which are well documented feels like it'd
 cover 99% of the cases.  pg_dump would preserve the times (though it
 could be optional), replicas should as well, etc.  We haven't even
 started talking about the 'hard' part, which would be a 'modification'
 type of field..

Here's a complete test case that works with my current branch, with a
tricky test while at it, of course:

create table public.tracking
(
relation regclass primary key,
relname  name not null,  -- in case it changes later
relnamespace name not null,  -- same reason
created  timestamptz default now(),
altered  timestamptz,
dropped  timestamptz
);

create or replace function public.track_table_activity() returns 
event_trigger
  language plpgsql
as $$
begin
  raise notice 'track table activity: % %', tg_tag, tg_objectid::regclass;
  if tg_operation = 'CREATE'
  then
insert into public.tracking(relation, relname, relnamespace)
 select tg_objectid, tg_objectname, tg_schemaname;

  elsif tg_operation = 'ALTER'
  then
update public.tracking set altered = now() where relation = tg_objectid;

  elsif tg_operation = 'DROP'
  then
update public.tracking set dropped = now() where relation = tg_objectid;

  else
raise notice 'unknown operation';
  end if;
end;
$$;

drop event trigger if exists track_table;

create event trigger track_table
  on ddl_command_trace
 when tag in ('create table', 'alter table', 'drop table')
  and context in ('toplevel', 'generated', 'subcommand')
   execute procedure public.track_table_activity();

drop schema if exists test cascade;

create schema test
   create table foo(id serial primary key, f1 text);

alter table test.foo add column f2 text;

select relation::regclass, * from public.tracking;

drop table test.foo;

select * from public.tracking;

select * from public.tracking;
-[ RECORD 1 ]+--
relation | tracking
relname  | tracking
relnamespace | public
created  | 2012-12-27 17:02:13.567979+01
altered  | 
dropped  | 
-[ RECORD 2 ]+--
relation | 25139
relname  | foo
relnamespace | test
created  | 2012-12-27 17:02:26.696039+01
altered  | 2012-12-27 17:02:29.105241+01
dropped  | 2012-12-27 17:02:37.834997+01


Maybe the best way to reconciliate both your views would be to provide
the previous example in the event trigger docs?

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] buffer assertion tripping under repeat pgbench load

2012-12-27 Thread Tom Lane
Greg Stark st...@mit.edu writes:
 On Thu, Dec 27, 2012 at 3:17 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 The thing that this theory has a hard time with is that the buffer's
 global refcount is zero.  If you assume that there's a bit that
 sometimes randomly goes to 1 when it should be 0, then what I'd expect
 to typically happen is that UnpinBuffer sees nonzero LocalRefCount and
 hence doesn't drop the session's global pin when it should.  The only
 way that doesn't happen is if decrementing LocalRefCount to zero stores
 a nonzero pattern when it should store zero, but nonetheless the CPU
 thinks it stored zero.

 It seems entirely plausible when you factor in the L2 cache. The cpu
 could be happily incrementing and decrementing the count entirely
 correctly blissfully unaware that the value being stored in the DRAM
 has this extra bit set every time. Not until the transaction ends and
 it has to refetch the cache line because enough time has passed for it
 to age out of L2 cache does it find the corrupt value.

Hmm ... that could be plausible.  It would be good if we could reproduce
this (or fail to) on some other machine.  Greg mentioned running some
memory diagnostics as well.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] fix bgworkers in EXEC_BACKEND

2012-12-27 Thread Alvaro Herrera
I committed background workers three weeks ago, claiming it worked on
EXEC_BACKEND, and shortly thereafter I discovered that it didn't.  I
noticed that the problem is the kludge to cause postmaster and children
to recompute MaxBackends after shared_preload_libraries is processed; so
the minimal fix is to duplicate this bit, from PostmasterMain() into
SubPostmasterMain():

@@ -4443,6 +4443,17 @@ SubPostmasterMain(int argc, char *argv[])
 */
process_shared_preload_libraries();
 
+   /*
+* If loadable modules have added background workers, MaxBackends needs to
+* be updated.  Do so now by forcing a no-op update of max_connections.
+* XXX This is a pretty ugly way to do it, but it doesn't seem worth
+* introducing a new entry point in guc.c to do it in a cleaner fashion.
+*/
+   if (GetNumShmemAttachedBgworkers()  0)
+   SetConfigOption(max_connections,
+   GetConfigOption(max_connections, false, false),
+   PGC_POSTMASTER, PGC_S_OVERRIDE);

I considered this pretty ugly when I first wrote it, and as the comment
says I tried to add something to guc.c to make it cleaner, but it was
even uglier.

So I now came up with a completely different idea: how about making
MaxBackends a macro, i.e.

+#define MaxBackends (MaxConnections + autovacuum_max_workers + 1 + \
+GetNumShmemAttachedBgworkers())

so that instead of having guc.c recompute it, each caller that needs to
value obtains it up to date all the time?  This additionally means that
assign_maxconnections and assign_autovacuum_max_workers go away (only
the check routines remain).  Patch attached.

The one problem I see as serious with this approach is that it'd be
moderately expensive (i.e. not just fetch a value from memory) to
compute the value because it requires a walk of the registered workers
list.  For most callers this wouldn't be a problem because it's just
during shmem sizing/creation; but there are places such as multixact.c
and async.c that use it routinely, so it's likely that we need to cache
the value somehow.  It seems relatively straightforward though.

I'd like to hear opinions on just staying with the IMO ugly minimal fix,
or pursue instead making MaxBackends a macro plus some sort of cache to
avoid repeated computation.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services
*** a/src/backend/access/nbtree/nbtutils.c
--- b/src/backend/access/nbtree/nbtutils.c
***
*** 21,26 
--- 21,27 
  #include access/reloptions.h
  #include access/relscan.h
  #include miscadmin.h
+ #include storage/proc.h
  #include utils/array.h
  #include utils/lsyscache.h
  #include utils/memutils.h
*** a/src/backend/access/transam/multixact.c
--- b/src/backend/access/transam/multixact.c
***
*** 57,62 
--- 57,63 
  #include miscadmin.h
  #include pg_trace.h
  #include storage/lmgr.h
+ #include storage/proc.h
  #include storage/procarray.h
  #include utils/builtins.h
  #include utils/memutils.h
*** a/src/backend/commands/async.c
--- b/src/backend/commands/async.c
***
*** 126,131 
--- 126,132 
  #include miscadmin.h
  #include storage/ipc.h
  #include storage/lmgr.h
+ #include storage/proc.h
  #include storage/procsignal.h
  #include storage/sinval.h
  #include tcop/tcopprot.h
*** a/src/backend/libpq/pqcomm.c
--- b/src/backend/libpq/pqcomm.c
***
*** 93,98 
--- 93,99 
  #include libpq/libpq.h
  #include miscadmin.h
  #include storage/ipc.h
+ #include storage/proc.h
  #include utils/guc.h
  #include utils/memutils.h
  
*** a/src/backend/postmaster/autovacuum.c
--- b/src/backend/postmaster/autovacuum.c
***
*** 108,114 
   * GUC parameters
   */
  bool		autovacuum_start_daemon = false;
! int			autovacuum_max_workers;
  int			autovacuum_naptime;
  int			autovacuum_vac_thresh;
  double		autovacuum_vac_scale;
--- 108,114 
   * GUC parameters
   */
  bool		autovacuum_start_daemon = false;
! /* autovacuum_max_workers is elsewhere */
  int			autovacuum_naptime;
  int			autovacuum_vac_thresh;
  double		autovacuum_vac_scale;
*** a/src/backend/postmaster/pgstat.c
--- b/src/backend/postmaster/pgstat.c
***
*** 52,57 
--- 52,58 
  #include storage/ipc.h
  #include storage/latch.h
  #include storage/pg_shmem.h
+ #include storage/proc.h
  #include storage/procsignal.h
  #include utils/ascii.h
  #include utils/guc.h
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
***
*** 897,913  PostmasterMain(int argc, char *argv[])
  	process_shared_preload_libraries();
  
  	/*
- 	 * If loadable modules have added background workers, MaxBackends needs to
- 	 * be updated.	Do so now by forcing a no-op update of max_connections.
- 	 * XXX This is a pretty ugly way to do it, but it doesn't seem worth
- 	 * introducing a new entry point in guc.c to do it in a cleaner fashion.
- 	 */
- 	

Re: [HACKERS] fix bgworkers in EXEC_BACKEND

2012-12-27 Thread Magnus Hagander
On Thu, Dec 27, 2012 at 6:15 PM, Alvaro Herrera
alvhe...@2ndquadrant.com wrote:
 I committed background workers three weeks ago, claiming it worked on
 EXEC_BACKEND, and shortly thereafter I discovered that it didn't.  I
 noticed that the problem is the kludge to cause postmaster and children
 to recompute MaxBackends after shared_preload_libraries is processed; so
 the minimal fix is to duplicate this bit, from PostmasterMain() into
 SubPostmasterMain():

 @@ -4443,6 +4443,17 @@ SubPostmasterMain(int argc, char *argv[])
  */
 process_shared_preload_libraries();

 +   /*
 +* If loadable modules have added background workers, MaxBackends needs to
 +* be updated.  Do so now by forcing a no-op update of max_connections.
 +* XXX This is a pretty ugly way to do it, but it doesn't seem worth
 +* introducing a new entry point in guc.c to do it in a cleaner fashion.
 +*/
 +   if (GetNumShmemAttachedBgworkers()  0)
 +   SetConfigOption(max_connections,
 +   GetConfigOption(max_connections, false, false),
 +   PGC_POSTMASTER, PGC_S_OVERRIDE);

 I considered this pretty ugly when I first wrote it, and as the comment
 says I tried to add something to guc.c to make it cleaner, but it was
 even uglier.

Isn't that version going to make the source field in pg_settings for
max_connections show override whenever you are using background
workers? Thus breaking things like the fields to tell you which config
file and line it's on?


 So I now came up with a completely different idea: how about making
 MaxBackends a macro, i.e.

 +#define MaxBackends (MaxConnections + autovacuum_max_workers + 1 + \
 +GetNumShmemAttachedBgworkers())

 so that instead of having guc.c recompute it, each caller that needs to
 value obtains it up to date all the time?  This additionally means that
 assign_maxconnections and assign_autovacuum_max_workers go away (only
 the check routines remain).  Patch attached.

That seems neater.


 The one problem I see as serious with this approach is that it'd be
 moderately expensive (i.e. not just fetch a value from memory) to
 compute the value because it requires a walk of the registered workers
 list.  For most callers this wouldn't be a problem because it's just
 during shmem sizing/creation; but there are places such as multixact.c
 and async.c that use it routinely, so it's likely that we need to cache
 the value somehow.  It seems relatively straightforward though.

 I'd like to hear opinions on just staying with the IMO ugly minimal fix,
 or pursue instead making MaxBackends a macro plus some sort of cache to
 avoid repeated computation.

If my understanding per above is correct, then here's a +1 for the
non-ugly non-minimal fix.


--
 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] fix bgworkers in EXEC_BACKEND

2012-12-27 Thread Heikki Linnakangas

On 27.12.2012 19:15, Alvaro Herrera wrote:

I committed background workers three weeks ago, claiming it worked on
EXEC_BACKEND, and shortly thereafter I discovered that it didn't.  I
noticed that the problem is the kludge to cause postmaster and children
to recompute MaxBackends after shared_preload_libraries is processed; so
the minimal fix is to duplicate this bit, from PostmasterMain() into
SubPostmasterMain():

@@ -4443,6 +4443,17 @@ SubPostmasterMain(int argc, char *argv[])
  */
 process_shared_preload_libraries();

+   /*
+* If loadable modules have added background workers, MaxBackends needs to
+* be updated.  Do so now by forcing a no-op update of max_connections.
+* XXX This is a pretty ugly way to do it, but it doesn't seem worth
+* introducing a new entry point in guc.c to do it in a cleaner fashion.
+*/
+   if (GetNumShmemAttachedBgworkers()  0)
+   SetConfigOption(max_connections,
+   GetConfigOption(max_connections, false, false),
+   PGC_POSTMASTER, PGC_S_OVERRIDE);

I considered this pretty ugly when I first wrote it, and as the comment
says I tried to add something to guc.c to make it cleaner, but it was
even uglier.


Might be cleaner to directly assign the correct value to MaxBackends 
above, ie. MaxBackends =  MaxConnections + newval + 1 + 
GetNumShmemAttachedBgworkers(). With a comment to remind that it needs 
to be kept in sync with the other places where that calculation is done, 
in guc.c. Or put that calculation in a new function and call it above 
and in guc.c.


Thinking about this some more, it might be cleaner to move the 
responsibility of setting MaxBackends out of guc.c, into postmaster.c. 
The guc machinery would set max_connections and autovacuum_max_workers 
as usual, but not try to set MaxBackends. After reading the config file 
in postmaster.c, calculate MaxBackends.


This would have the advantage that MaxBackends would be kept set at 
zero, until we know the final value. That way it's obvious that you 
cannot trust the value of MaxBackends in a contrib module 
preload-function, for example, which would reduce the chance of 
programmer mistakes.



So I now came up with a completely different idea: how about making
MaxBackends a macro, i.e.

+#define MaxBackends (MaxConnections + autovacuum_max_workers + 1 + \
+GetNumShmemAttachedBgworkers())

so that instead of having guc.c recompute it, each caller that needs to
value obtains it up to date all the time?  This additionally means that
assign_maxconnections and assign_autovacuum_max_workers go away (only
the check routines remain).  Patch attached.

The one problem I see as serious with this approach is that it'd be
moderately expensive (i.e. not just fetch a value from memory) to
compute the value because it requires a walk of the registered workers
list.  For most callers this wouldn't be a problem because it's just
during shmem sizing/creation; but there are places such as multixact.c
and async.c that use it routinely, so it's likely that we need to cache
the value somehow.  It seems relatively straightforward though.


I don't like that. The result of GetNumShmemAttachedBgWorkers() doesn't 
change after postmaster startup, so it seems silly to call it 
repeatedly. And from a readability point of view, it makes you think 
that it might change, because it's recalculated every time.


If I'm reading the code correctly, GetNumShmemAttachedBgWorkers() works 
by walking through a backend-local list. What happens if a background 
worker fails to register itself when preloaded in one backend? That 
backend would calculate a different value of MaxBackends, with 
interesting consequences. That would be a clear case of don't do that, 
but nevertheless, I think it would be better if we didn't rely on that. 
I'd suggest adding MaxBackends to the list of variables passed from 
postmaster to backends via BackendParameters.


All in all, I propose the attached. Not tested on Windows.

- Heikki
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 8f39aec..9dc8bd3 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -499,6 +499,7 @@ typedef struct
 	bool		redirection_done;
 	bool		IsBinaryUpgrade;
 	int			max_safe_fds;
+	int			MaxBackends;
 #ifdef WIN32
 	HANDLE		PostmasterHandle;
 	HANDLE		initial_signal_pipe;
@@ -897,15 +898,11 @@ PostmasterMain(int argc, char *argv[])
 	process_shared_preload_libraries();
 
 	/*
-	 * If loadable modules have added background workers, MaxBackends needs to
-	 * be updated.	Do so now by forcing a no-op update of max_connections.
-	 * XXX This is a pretty ugly way to do it, but it doesn't seem worth
-	 * introducing a new entry point in guc.c to do it in a cleaner fashion.
+	 * Now that loadable modules have had their chance to register background
+	 * workers, calculate MaxBackends.
 	 */
-	if (GetNumShmemAttachedBgworkers()  0)

Re: [HACKERS] fix bgworkers in EXEC_BACKEND

2012-12-27 Thread Stephen Frost
* Heikki Linnakangas (hlinnakan...@vmware.com) wrote:
 Thinking about this some more, it might be cleaner to move the
 responsibility of setting MaxBackends out of guc.c, into
 postmaster.c. The guc machinery would set max_connections and
 autovacuum_max_workers as usual, but not try to set MaxBackends.
 After reading the config file in postmaster.c, calculate
 MaxBackends.

I tend to prefer Heikki's solution wrt supporting what we do currently.
I do wonder if, perhaps, the reason the assign_XXX() functions were put
in place and used from GUC was a hope that some day we'd actually
support online changing of max_connections (and friends).  I realize
that's pretty pie-in-the-sky, but it sure would be nice to reduce the
number of parameters that require a full restart.

All that said, putting those functions back and changing guc.c would
certainly be pretty trivially done, should some new patch come along
that would allow online changing of max_connections.

So, +1 on Heikki's approach.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] fix bgworkers in EXEC_BACKEND

2012-12-27 Thread Simon Riggs
On 27 December 2012 18:06, Heikki Linnakangas hlinnakan...@vmware.com wrote:

 This would have the advantage that MaxBackends would be kept set at zero,
 until we know the final value. That way it's obvious that you cannot trust
 the value of MaxBackends in a contrib module preload-function, for example,
 which would reduce the chance of programmer mistakes.

I admire your forward thinking on that; yes, that could cause
problems. But even then, we would be admitting that nobody now gets a
valid value of MaxBackends, which sounds like it might be a problem in
itself.

Perhaps we should try to solve that a different way? Can we ask for
reservations of bgworkers ahead of running their _init functions,
using an additional API call?

That way we'd know the final value and everybody would have the
correct value at init time.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] fix bgworkers in EXEC_BACKEND

2012-12-27 Thread Stephen Frost
Simon,

* Simon Riggs (si...@2ndquadrant.com) wrote:
 I admire your forward thinking on that; yes, that could cause
 problems. But even then, we would be admitting that nobody now gets a
 valid value of MaxBackends, which sounds like it might be a problem in
 itself.

I agree that the current implementation could lead to problems/confusion
for contrib module authors, if they're doing something with MaxBackends.

 Perhaps we should try to solve that a different way? Can we ask for
 reservations of bgworkers ahead of running their _init functions,
 using an additional API call?

Before we go there, I'm honestly curious to hear what the use case is
for a contrib module to need that information, particularly at _init
time..?  Also, would we need every contrib module to add in that call?
What if they don't create any additional bgworkers?  What if they do but
don't provide the API call?  I wonder if max_connections, which is well
defined and not subject to other modules or other things happening,
might be a better value to encourage authors to use, if they're looking
for some heuristic for how many possible backends there could be?

 That way we'd know the final value and everybody would have the
 correct value at init time.

This sounds like an opportunity for people to add another backend worker
and then forget to update what they tell the postmaster and everyone
else about how many backend workers they're going to create..  Or maybe
having a larger value be returned than they actually use.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] Event Triggers: adding information

2012-12-27 Thread Simon Riggs
On 21 December 2012 14:48, Robert Haas robertmh...@gmail.com wrote:

 I predict that if we commit this, it will
 becomes a never-ending and irrecoverable font of trouble.

Event triggers are a new feature with many powerful uses: replication,
online upgrade, data transport.

New features require extra code. To decide how big a maintenance
burden, we need to look at whether we want the feature, and if we have
it, how much code it would be to maintain it.

pg_dump needs to dump what is there, so dumping current state
information is required. That can be accessed from catalog, but
requires significant extra code to bring that all back together into
DDL text. pg_dump is useful for schema backups and pg_upgrade. This
code already exists in core.

Replication needs to see delta information, so we can track changes
as they happen. Trying to deduce exact deltas by diffing two current
state dumps is close to impossible. The only way is a delta stream.
This would be used by replication and on-line upgrade. This code can
be added to core by the patches under discussion.

The code is broadly comparable between current state and delta.
It's basically just assembling the text of DDL statements from
available information. In one case from the catalog, in the other case
from the command parse/execution trees. It's not especially hard to
code, nor will it be fiddly or difficult to understand - just
developing normalised text versions of the change in progress.
Normalisation is important and useful; we spent time in 9.2 doing
things the right way for pg_stat_statements.

In both cases, the code really should live in core. So we can tie the
code directly to each release. Realistically, doing things outside
core means there are often holes or problems that cannot be easily
solved. That seems almost certain to be the case here.

pg_dump maintenance is required but its not a huge burden. pg_dump
hasn't slowed down the addition of new DDL features. It just requires
a little extra code, mostly very simple.

The PostgreSQL project needs rewrite support for DDL, so it will come
to exist inside, or outside core. Knowing it is there, being able to
rely on that, means we'll make faster progress towards the new
features we are building. The approach proposed here has been
discussed over many years, minuted in detail at cluster hackers
meetings: DDL Triggers.

My thoughts are that the approach here has been well thought through
and is exactly what we need. It *is* extra code, but not complex code,
and it is code designed to bring us closer to our goals, not to hamper
the development of new DDL statements. With the right tests, the
changes required would be easy to spot and add code for them. It looks
to me that we already have a good suggestion on exactly how to do that
(from Robert).

So overall, it certainly is worth thinking hard about whether we
should include the rewrite code. Balancing the costs and the benefits,
I think we should include.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] fix bgworkers in EXEC_BACKEND

2012-12-27 Thread Simon Riggs
On 27 December 2012 18:36, Stephen Frost sfr...@snowman.net wrote:
 Simon,

 * Simon Riggs (si...@2ndquadrant.com) wrote:
 I admire your forward thinking on that; yes, that could cause
 problems. But even then, we would be admitting that nobody now gets a
 valid value of MaxBackends, which sounds like it might be a problem in
 itself.

 I agree that the current implementation could lead to problems/confusion
 for contrib module authors, if they're doing something with MaxBackends.

I can't see any problems myself and am happy with Heikki's proposal to
accept that restriction, since other workarounds are possible.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] fix bgworkers in EXEC_BACKEND

2012-12-27 Thread Alvaro Herrera
Stephen Frost wrote:
 * Heikki Linnakangas (hlinnakan...@vmware.com) wrote:
  Thinking about this some more, it might be cleaner to move the
  responsibility of setting MaxBackends out of guc.c, into
  postmaster.c. The guc machinery would set max_connections and
  autovacuum_max_workers as usual, but not try to set MaxBackends.
  After reading the config file in postmaster.c, calculate
  MaxBackends.
 
 I tend to prefer Heikki's solution wrt supporting what we do currently.
 I do wonder if, perhaps, the reason the assign_XXX() functions were put
 in place and used from GUC was a hope that some day we'd actually
 support online changing of max_connections (and friends).

No, that's not the reason.  The reason is that the check hooks did not
exist at all, so both the check and the assignment were done in the
assign hook.  Now we're getting rid of the assignment hooks because
they're useless, but the check hooks must, of course, remain.  We put
the hooks in because it was the simplest thing we could think of to set
MaxBackends when either max_connections or autovacuum_max_workers were
tweaked.  My guess is if we had thought of propagating MaxBackends via
BackendParameters back then, we'd probably gone that route as well.
But certainly we had no intention to make max_connections online changeable.

I too like Heikki's proposed patch much better than mine.  Thanks.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] fix bgworkers in EXEC_BACKEND

2012-12-27 Thread Tom Lane
Stephen Frost sfr...@snowman.net writes:
 Simon,
 * Simon Riggs (si...@2ndquadrant.com) wrote:
 I admire your forward thinking on that; yes, that could cause
 problems. But even then, we would be admitting that nobody now gets a
 valid value of MaxBackends, which sounds like it might be a problem in
 itself.

 I agree that the current implementation could lead to problems/confusion
 for contrib module authors, if they're doing something with MaxBackends.

This is more or less a necessary consequence of the fact that _init
functions are now allowed to add background workers.  If there is any
code today that expects MaxBackends to be correct at
preload_shared_libraries time, it's already been broken irretrievably
by the bgworkers patch; and we'd be well advised to make that breakage
obvious not subtle.

So I'm +1 for Heikki's proposal as well.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] fix bgworkers in EXEC_BACKEND

2012-12-27 Thread Alvaro Herrera
Heikki Linnakangas wrote:

 Might be cleaner to directly assign the correct value to MaxBackends
 above, ie. MaxBackends =  MaxConnections + newval + 1 +
 GetNumShmemAttachedBgworkers(). With a comment to remind that it
 needs to be kept in sync with the other places where that
 calculation is done, in guc.c. Or put that calculation in a new
 function and call it above and in guc.c.
 
 Thinking about this some more, it might be cleaner to move the
 responsibility of setting MaxBackends out of guc.c, into
 postmaster.c. The guc machinery would set max_connections and
 autovacuum_max_workers as usual, but not try to set MaxBackends.
 After reading the config file in postmaster.c, calculate
 MaxBackends.

Here's a small patch that applies on top of yours.  Do you intend to
commit this?  If not, let me know and I'll do it.

Thanks.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 8dd2b4b..0d808d0 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -103,13 +103,14 @@ int			work_mem = 1024;
 int			maintenance_work_mem = 16384;
 
 /*
- * Primary determinants of sizes of shared-memory structures.  MaxBackends is
- * MaxConnections + autovacuum_max_workers + 1 (it is computed by the GUC
- * assign hooks for those variables):
+ * Primary determinants of sizes of shared-memory structures.
+ *
+ * MaxBackends is computed by PostmasterMain after modules have had a chance to
+ * register background workers.
  */
 int			NBuffers = 1000;
-int			MaxBackends = 100;
 int			MaxConnections = 90;
+int			MaxBackends = 0;
 
 int			VacuumCostPageHit = 1;		/* GUC parameters for vacuum */
 int			VacuumCostPageMiss = 10;

-- 
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] Proposal: Store timestamptz of database creation on pg_database

2012-12-27 Thread Fabrízio de Royes Mello
On Thu, Dec 27, 2012 at 2:13 AM, Tom Lane t...@sss.pgh.pa.us wrote:

 This has been debated, and rejected, before.


I know this discussion...

 To mention just one problem, are we going to add nonstandard,
 non-backwards-compatible syntax to every single kind of CREATE to allow
 pg_dump to preserve the creation dates?  Another interesting question is
 whether we should likewise track the last ALTER time, or perhaps whether
 a sufficiently major ALTER redefinition should update the creation time.


I agree with you because now we have Event Triggers...

 I'm inclined to think that anyone who really needs this should be
 pointed at event triggers.  That feature (if it gets in) will allow
 people to track creation/DDL-change times with exactly the behavior
 they want.


Exactly, but Event Triggers [1] don't cover CREATE DATABASE statement,
and for this reason I propose the patch to add a single column datcreated
on shared catalog pg_database.

[1] http://www.postgresql.org/docs/devel/static/event-trigger-matrix.html


Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Blog sobre TI: http://fabriziomello.blogspot.com
 Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello


Re: [HACKERS] Proposal: Store timestamptz of database creation on pg_database

2012-12-27 Thread Fabrízio de Royes Mello
On Thu, Dec 27, 2012 at 2:04 PM, Dimitri Fontaine dimi...@2ndquadrant.fr
wrote:


 Tom Lane t...@sss.pgh.pa.us writes:
  This proposal is about add a column datcreated on pg_database to
store
  the timestamp of the database creation.
 
  I'm inclined to think that anyone who really needs this should be
  pointed at event triggers.  That feature (if it gets in) will allow
  people to track creation/DDL-change times with exactly the behavior
  they want.

 Agreed.


+1


 Maybe the best way to reconciliate both your views would be to provide
 the previous example in the event trigger docs?


+1

If all of you agree I can improve the event trigger docs...

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Blog sobre TI: http://fabriziomello.blogspot.com
 Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello


Re: [HACKERS] fix bgworkers in EXEC_BACKEND

2012-12-27 Thread Andres Freund
On 2012-12-27 20:06:07 +0200, Heikki Linnakangas wrote:
 On 27.12.2012 19:15, Alvaro Herrera wrote:
 I committed background workers three weeks ago, claiming it worked on
 EXEC_BACKEND, and shortly thereafter I discovered that it didn't.  I
 noticed that the problem is the kludge to cause postmaster and children
 to recompute MaxBackends after shared_preload_libraries is processed; so
 the minimal fix is to duplicate this bit, from PostmasterMain() into
 SubPostmasterMain():
 
 If I'm reading the code correctly, GetNumShmemAttachedBgWorkers() works by
 walking through a backend-local list. What happens if a background worker
 fails to register itself when preloaded in one backend? That backend would
 calculate a different value of MaxBackends, with interesting consequences.
 That would be a clear case of don't do that, but nevertheless, I think it
 would be better if we didn't rely on that. I'd suggest adding MaxBackends to
 the list of variables passed from postmaster to backends via
 BackendParameters.

I am still worried about the following scenario in the EXEC_BACKEND case:

1) postmaster starts
2) reads config
3) executes _PG_init for shared_preload_libraries
4) library 'abc' gets config value 'abc.num_workers = 2' and registers as many 
workers
5) some time goes by, workers, backends start
6) abc.num_workers gets changed to 3, SIGHUP
7) some worker dies and gets restarted
8) _PG_init in the new worker does one more RegisterBackgroundWorker than before
9) ???

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] fix bgworkers in EXEC_BACKEND

2012-12-27 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes:
 I am still worried about the following scenario in the EXEC_BACKEND case:

 1) postmaster starts
 2) reads config
 3) executes _PG_init for shared_preload_libraries
 4) library 'abc' gets config value 'abc.num_workers = 2' and registers as 
 many workers
 5) some time goes by, workers, backends start
 6) abc.num_workers gets changed to 3, SIGHUP

This is broken whether it's EXEC_BACKEND or not: you don't get to change
anything that determines the number of workers post-startup.
num_workers should have been declared PGC_POSTMASTER.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] fix bgworkers in EXEC_BACKEND

2012-12-27 Thread Andres Freund
On 2012-12-27 18:44:57 -0500, Tom Lane wrote:
 Andres Freund and...@2ndquadrant.com writes:
  I am still worried about the following scenario in the EXEC_BACKEND case:

  1) postmaster starts
  2) reads config
  3) executes _PG_init for shared_preload_libraries
  4) library 'abc' gets config value 'abc.num_workers = 2' and registers as 
  many workers
  5) some time goes by, workers, backends start
  6) abc.num_workers gets changed to 3, SIGHUP

 This is broken whether it's EXEC_BACKEND or not: you don't get to change
 anything that determines the number of workers post-startup.
 num_workers should have been declared PGC_POSTMASTER.

Well, the problem is, a shared library can't do that
afaics. abc.num_workers would be using custom_variable_classes (well,
whatever its called now, that it doesn't need to be configured).

There is no predefined 'num_workers' variable or anything like it in the
patch, but I guess some of the module authors will come of up with
configuration variables like that.

I personally want e.g. 'bdr.databases = 'a, b, c' which obviously has
the same problem...

Greetings,

Andres Freund

--
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] fix bgworkers in EXEC_BACKEND

2012-12-27 Thread anara...@anarazel.de


Tom Lane t...@sss.pgh.pa.us schrieb:

Andres Freund and...@2ndquadrant.com writes:
 I am still worried about the following scenario in the EXEC_BACKEND
case:

 1) postmaster starts
 2) reads config
 3) executes _PG_init for shared_preload_libraries
 4) library 'abc' gets config value 'abc.num_workers = 2' and
registers as many workers
 5) some time goes by, workers, backends start
 6) abc.num_workers gets changed to 3, SIGHUP

This is broken whether it's EXEC_BACKEND or not: you don't get to
change
anything that determines the number of workers post-startup.
num_workers should have been declared PGC_POSTMASTER.

BTW, I think it happens not to be broken in the non EXEC_BACKEND case because 
the registration point for bgworkers is _PG_init which will only be called once 
without EXEC_BACKEND, so config changes to SIGHUP'able variables won't change a 
thing dangerous as the bgworker stuff is all set up.

Andres


--- 
Please excuse the brevity and formatting - I am writing this on my mobile phone.


-- 
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] Proposal: Store timestamptz of database creation on pg_database

2012-12-27 Thread Stephen Frost
Dimitri,

* Dimitri Fontaine (dimi...@2ndquadrant.fr) wrote:
 Here's a complete test case that works with my current branch, with a
 tricky test while at it, of course:

Apparently I've managed to miss the tricky case..?

Sure, dropping tables, schemas, etc, would have an impact on the values.
Dropping a table and then recreating it would be akin to deleteing a row
and then inserting a new one- the create value would be set to the time
of the new table being created and information about the dropped table
would be lost.

I'm not thinking of this as audit tracking where every action is logged.

I agree that what I was suggesting would be possible to implement with
event triggers, but I see that as a rather advanced feature that most
users aren't going to understand or implement.  At the same time, those
more novice users are likely to be looking for this kind of information-
being told oh, well, you *could* have been collecting it all along if
you knew about event triggers isn't a particularly satisfying answer.

That's my 2c on it.

I agree that having the example in the docs would be nice- examples are
always good things to include.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] fix bgworkers in EXEC_BACKEND

2012-12-27 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes:
 On 2012-12-27 18:44:57 -0500, Tom Lane wrote:
 This is broken whether it's EXEC_BACKEND or not: you don't get to change
 anything that determines the number of workers post-startup.
 num_workers should have been declared PGC_POSTMASTER.

 Well, the problem is, a shared library can't do that
 afaics. abc.num_workers would be using custom_variable_classes (well,
 whatever its called now, that it doesn't need to be configured).

We fixed that a few years ago, no?

http://git.postgresql.org/gitweb/?p=postgresql.gita=commitdiffh=4605d1c98

If that's broken, we certainly need to fix it again.  This issue exists
for any parameter that feeds into shared memory sizing, which is exactly
why we changed it back then.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers