Re: [HACKERS] texteq/byteaeq: avoid detoast [REVIEW]

2011-01-17 Thread Itagaki Takahiro
2011/1/17 KaiGai Kohei kai...@ak.jp.nec.com:
 Are you talking about an idea to apply toast id as an alternative key?

No, probably. I'm just talking about whether diff -q A.txt B.txt and
diff -q A.gz  B.gz always returns the same result or not.

... I found it depends on version of gzip. So, if we use such logic,
we cannot improve toast compression logic because the data is migrated
by pg_upgrade.

 I did similar idea to represent security label on user tables for row
 level security in the v8.4/9.0 based implementation. Because a small
 number of security labels are shared by massive tuples, it is waste of
 space if we have all the text data being toasted individually, not only
 performance loss in toast/detoast.

It looks the same issue as large object rather than the discussion here.
We have vacuumlo in contrib to solve the issue. So, we could have
vacuumlo-like special sweeping logic for the security label table.

-- 
Itagaki Takahiro

-- 
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_basebackup for streaming base backups

2011-01-17 Thread Fujii Masao
On Sun, Jan 16, 2011 at 11:31 PM, Magnus Hagander mag...@hagander.net wrote:
 Ok. Updated patch that includes this change attached.

I could not apply the patch cleanly against the git master.
Do you know what the cause is?

$ patch -p1 -d.  /hoge/pg_basebackup.patch
patching file doc/src/sgml/backup.sgml
patching file doc/src/sgml/ref/allfiles.sgml
patching file doc/src/sgml/ref/pg_basebackup.sgml
patching file doc/src/sgml/reference.sgml
patching file src/bin/Makefile
patching file src/bin/pg_basebackup/Makefile
patching file src/bin/pg_basebackup/nls.mk
patching file src/bin/pg_basebackup/pg_basebackup.c
patch:  malformed patch at line 1428: diff --git
a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

-- 
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] REVIEW: PL/Python validator function

2011-01-17 Thread Jan Urbański
On 17/01/11 01:02, Hitoshi Harada wrote:
 This is a review for the patch sent as
 https://commitfest.postgresql.org/action/patch_view?id=456

Thanks!

 It includes adequate amount of test. I found regression test failure
 in plpython_error.

 My environment is CentOS release 5.4 (Final) with python 2.4.3
 installed default.

Darn, I would've sworn I tested all of them on older pythons. I've
reproduced it with 2.4 and earlier versions. Will fix, thanks for
spotting it.

 I think catversion update should be included in the patch, since it
 contains pg_pltemplate catalog changes.

I think that's usually left for the committer, otherwise it will almost
always be a source of conflicts.

 It looks fine overall. The only thing that I came up with is trigger
 check logic in PLy_procedure_is_trigger. Although it seems following
 plperl's corresponding function, the check of whether the prorettype
 is pseudo type looks redundant since it checks prorettype is
 TRIGGEROID or OPAQUEOID later. But it is not critical.

Yes, you're right, a check for prorettype only should be sufficient. Wil
fix.

 I mark Waiting on Author for the regression test issue. Other points
 are trivial.

Thank you,
Jan

-- 
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_basebackup for streaming base backups

2011-01-17 Thread Magnus Hagander
On Jan 17, 2011 9:16 AM, Fujii Masao masao.fu...@gmail.com wrote:

 On Sun, Jan 16, 2011 at 11:31 PM, Magnus Hagander mag...@hagander.net
wrote:
  Ok. Updated patch that includes this change attached.

 I could not apply the patch cleanly against the git master.
 Do you know what the cause is?

 $ patch -p1 -d.  /hoge/pg_basebackup.patch
 patching file doc/src/sgml/backup.sgml
 patching file doc/src/sgml/ref/allfiles.sgml
 patching file doc/src/sgml/ref/pg_basebackup.sgml
 patching file doc/src/sgml/reference.sgml
 patching file src/bin/Makefile
 patching file src/bin/pg_basebackup/Makefile
 patching file src/bin/pg_basebackup/nls.mk
 patching file src/bin/pg_basebackup/pg_basebackup.c
 patch:  malformed patch at line 1428: diff --git
 a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm


Weird, no idea. Will have to look into that later - meanwhile you can grab
the branch tip from my github repo if you want to review it.

/Magnus


Re: [HACKERS] pg_basebackup for streaming base backups

2011-01-17 Thread Fujii Masao
On Mon, Jan 17, 2011 at 5:44 PM, Magnus Hagander mag...@hagander.net wrote:
 Weird, no idea. Will have to look into that later - meanwhile you can grab
 the branch tip from my github repo if you want to review it.

Which repo should I grab? You seem to have many repos :)
http://git.postgresql.org/gitweb

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

-- 
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] Wildcard search support for pg_trgm

2011-01-17 Thread Alexander Korotkov
Hi,

Here is updated version of this patch.


With best regards,
Alexander Korotkov.
*** a/contrib/pg_trgm/pg_trgm.sql.in
--- b/contrib/pg_trgm/pg_trgm.sql.in
***
*** 113,118  FOR TYPE text USING gist
--- 113,120 
  AS
  OPERATOR1   % (text, text),
  OPERATOR2   - (text, text) FOR ORDER BY pg_catalog.float_ops,
+ OPERATOR3   ~~ (text, text),
+ OPERATOR4   ~~* (text, text),
  FUNCTION1   gtrgm_consistent (internal, text, int, oid, internal),
  FUNCTION2   gtrgm_union (bytea, internal),
  FUNCTION3   gtrgm_compress (internal),
***
*** 129,140  RETURNS internal
  AS 'MODULE_PATHNAME'
  LANGUAGE C IMMUTABLE STRICT;
  
! CREATE OR REPLACE FUNCTION gin_extract_trgm(text, internal, int2, internal, internal)
  RETURNS internal
  AS 'MODULE_PATHNAME'
  LANGUAGE C IMMUTABLE STRICT;
  
! CREATE OR REPLACE FUNCTION gin_trgm_consistent(internal, int2, text, int4, internal, internal)
  RETURNS bool
  AS 'MODULE_PATHNAME'
  LANGUAGE C IMMUTABLE STRICT;
--- 131,142 
  AS 'MODULE_PATHNAME'
  LANGUAGE C IMMUTABLE STRICT;
  
! CREATE OR REPLACE FUNCTION gin_extract_query_trgm(text, internal, int2, internal, internal, internal, internal)
  RETURNS internal
  AS 'MODULE_PATHNAME'
  LANGUAGE C IMMUTABLE STRICT;
  
! CREATE OR REPLACE FUNCTION gin_trgm_consistent(internal, int2, text, int4, internal, internal, internal, internal)
  RETURNS bool
  AS 'MODULE_PATHNAME'
  LANGUAGE C IMMUTABLE STRICT;
***
*** 144,151  CREATE OPERATOR CLASS gin_trgm_ops
  FOR TYPE text USING gin
  AS
  OPERATOR1   % (text, text),
  FUNCTION1   btint4cmp (int4, int4),
  FUNCTION2   gin_extract_trgm (text, internal),
! FUNCTION3   gin_extract_trgm (text, internal, int2, internal, internal),
! FUNCTION4   gin_trgm_consistent (internal, int2, text, int4, internal, internal),
  STORAGE int4;
--- 146,155 
  FOR TYPE text USING gin
  AS
  OPERATOR1   % (text, text),
+ OPERATOR3   ~~ (text, text),
+ OPERATOR4   ~~* (text, text),
  FUNCTION1   btint4cmp (int4, int4),
  FUNCTION2   gin_extract_trgm (text, internal),
! FUNCTION3   gin_extract_query_trgm (text, internal, int2, internal, internal, internal, internal),
! FUNCTION4   gin_trgm_consistent (internal, int2, text, int4, internal, internal, internal, internal),
  STORAGE int4;
*** a/contrib/pg_trgm/trgm.h
--- b/contrib/pg_trgm/trgm.h
***
*** 19,24 
--- 19,26 
  /* operator strategy numbers */
  #define	SimilarityStrategyNumber	1
  #define	DistanceStrategyNumber		2
+ #define LikeStrategyNumber			3
+ #define ILikeStrategyNumber			4
  
  
  typedef char trgm[3];
***
*** 53,59  typedef struct
  
  /* gist */
  #define BITBYTE 8
! #define SIGLENINT  3			/* 122 = key will toast, so very slow!!! */
  #define SIGLEN	( sizeof(int)*SIGLENINT )
  
  #define SIGLENBIT (SIGLEN*BITBYTE - 1)	/* see makesign */
--- 55,61 
  
  /* gist */
  #define BITBYTE 8
! #define SIGLENINT  15			/* 122 = key will toast, so very slow!!! */
  #define SIGLEN	( sizeof(int)*SIGLENINT )
  
  #define SIGLENBIT (SIGLEN*BITBYTE - 1)	/* see makesign */
***
*** 89,94  typedef char *BITVECP;
--- 91,101 
  extern float4 trgm_limit;
  
  TRGM	   *generate_trgm(char *str, int slen);
+ TRGM	   *generate_wildcard_trgm(char *str, int slen);
  float4		cnt_sml(TRGM *trg1, TRGM *trg2);
+ booltrgm_contain(TRGM *trg1, TRGM *trg2);
+ 
+ #define ISESCAPECHAR(x) (*(x) == '\\')
+ #define ISWILDCARDCHAR(x) (*(x) == '_' || *(x) == '%')
  
  #endif /* __TRGM_H__ */
*** a/contrib/pg_trgm/trgm_gin.c
--- b/contrib/pg_trgm/trgm_gin.c
***
*** 6,11 
--- 6,12 
  #include trgm.h
  
  #include access/gin.h
+ #include access/skey.h
  #include access/itup.h
  #include access/tuptoaster.h
  #include storage/bufpage.h
***
*** 16,21 
--- 17,25 
  PG_FUNCTION_INFO_V1(gin_extract_trgm);
  Datum		gin_extract_trgm(PG_FUNCTION_ARGS);
  
+ PG_FUNCTION_INFO_V1(gin_extract_query_trgm);
+ Datum		gin_extract_query_trgm(PG_FUNCTION_ARGS);
+ 
  PG_FUNCTION_INFO_V1(gin_trgm_consistent);
  Datum		gin_trgm_consistent(PG_FUNCTION_ARGS);
  
***
*** 58,90  gin_extract_trgm(PG_FUNCTION_ARGS)
  }
  
  Datum
  gin_trgm_consistent(PG_FUNCTION_ARGS)
  {
  	bool	   *check = (bool *) PG_GETARG_POINTER(0);
! 	/* StrategyNumber strategy = PG_GETARG_UINT16(1); */
  	/* text*query = PG_GETARG_TEXT_P(2); */
! 	int32		nkeys = PG_GETARG_INT32(3);
! 	/* Pointer*extra_data = (Pointer *) PG_GETARG_POINTER(4); */
  	bool	   *recheck = (bool *) PG_GETARG_POINTER(5);
  	bool		res = FALSE;
  	int32		i,
! 

Re: [HACKERS] walreceiver fallback_application_name

2011-01-17 Thread Bernd Helmle



--On 16. Januar 2011 21:53:47 +0100 Dimitri Fontaine 
dimi...@2ndquadrant.fr wrote:



Is walreceiver something that the average DBA is going to realize
what it is? Perhaps go for something like replication slave?


I think walreceiver is very good here, and the user is already
confronted to such phrasing.


http://www.postgresql.org/docs/9.0/interactive/runtime-config-wal.html#GU
C-MAX-WAL-SENDERS


Hmm, given this link we have mentioned standby multiple times. Wouldn't 
it be better to follow that phrasing?


--
Thanks

Bernd

--
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_basebackup for streaming base backups

2011-01-17 Thread Magnus Hagander
On Mon, Jan 17, 2011 at 09:50, Fujii Masao masao.fu...@gmail.com wrote:
 On Mon, Jan 17, 2011 at 5:44 PM, Magnus Hagander mag...@hagander.net wrote:
 Weird, no idea. Will have to look into that later - meanwhile you can grab
 the branch tip from my github repo if you want to review it.

 Which repo should I grab? You seem to have many repos :)
 http://git.postgresql.org/gitweb

Oh, sorry about that. There is only one that contains postgresql though :P

http://github.com/mhagander/postgres, branch streaming_base.

-- 
 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] walreceiver fallback_application_name

2011-01-17 Thread Magnus Hagander
On Mon, Jan 17, 2011 at 04:05, Fujii Masao masao.fu...@gmail.com wrote:
 On Mon, Jan 17, 2011 at 11:16 AM, Robert Haas robertmh...@gmail.com wrote:
 On Sun, Jan 16, 2011 at 3:53 PM, Dimitri Fontaine
 dimi...@2ndquadrant.fr wrote:
 Magnus Hagander mag...@hagander.net writes:
 Is walreceiver something that the average DBA is going to realize
 what it is? Perhaps go for something like replication slave?

 I think walreceiver is very good here, and the user is already
 confronted to such phrasing.

  http://www.postgresql.org/docs/9.0/interactive/runtime-config-wal.html#GUC-MAX-WAL-SENDERS

 I agree that walreceiver is a reasonable default to supply in this case.

 +1 though I could not find the mention to walreceiver in the doc.

It's on this page:
http://www.postgresql.org/docs/9.0/interactive/warm-standby.html

The word exists a single time in our entire documentation. But I sgree
with Dimitri's comment  - we should document *what* is on the other
side (walreceiver) not what it's doing (standby). Because what it's
doing is already visible through the state column.


 diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
 b/src/backend/replication/libpqwalreceiver/libpqwalreceiv
 index c052df2..962ee04 100644
 --- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
 +++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
 @@ -92,7 +92,7 @@ libpqrcv_connect(char *conninfo, XLogRecPtr startpoint)
     * replication for .pgpass lookup.
     */
    snprintf(conninfo_repl, sizeof(conninfo_repl),
 -            %s dbname=replication replication=true,
 +            %s dbname=replication replication=true
 fallback_application_name=postgres,
             conninfo);

 Also the size of conninfo_repl needs to be enlarged.

Oh, nice catch. Worked perfectly in my testing, but I see why it
should be increased :-)

-- 
 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] Include WAL in base backup

2011-01-17 Thread Magnus Hagander
On Sun, Jan 16, 2011 at 20:13, Dimitri Fontaine dimi...@2ndquadrant.fr wrote:
 Magnus Hagander mag...@hagander.net writes:
 However, it's not quite that simple. just adding a fork() doesn't
 work on all our platforms, and you need to deal with feedback and such
 between them as well.

 I'd think client-side, we have an existing implementation with the
 parallel pg_restore option.  Don't know (yet) how easy it is to reuse
 that code…

True.

But however we do it, it will be significantly more complex than just
including the WAL. And I want to make sure we get *something* done in
time for 9.1, and then improve upon it. If we can get the improvement
into 9.1 that's great, but if not it will have to wait until 9.2 - and
I don't want to leave us without anything for 9.1.

 Oh, and this might be the use-case for integrating the streaming log
 code as well :-) But if we plan to do that, perhaps we should pick a
 different name for the binary? Or maybe just share code with another
 one later..

 You're talking about the pg_streamrecv binary?  Then yes, my idea about
 it is that it should become the default archive client we ship with
 PostgreSQL.  And grow into offering a way to be the default restore
 command too.  I'd see the current way that the standby can switch
 between restoring from archive and from a live stream as a first step
 into that direction.

Right. I did put both the base backup and the wal streaming in the
same binary earlier, and the only shared code was the one to connect
to the db. So I split them apart again. This is the reason to put them
back together perhaps - or just have the ability for pg_basebackup to
fork()+exec() the wal streamer.

-- 
 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] texteq/byteaeq: avoid detoast [REVIEW]

2011-01-17 Thread Magnus Hagander
On Mon, Jan 17, 2011 at 09:13, Itagaki Takahiro
itagaki.takah...@gmail.com wrote:
 2011/1/17 KaiGai Kohei kai...@ak.jp.nec.com:
 Are you talking about an idea to apply toast id as an alternative key?

 No, probably. I'm just talking about whether diff -q A.txt B.txt and
 diff -q A.gz  B.gz always returns the same result or not.

 ... I found it depends on version of gzip. So, if we use such logic,
 we cannot improve toast compression logic because the data is migrated
 by pg_upgrade.

Yeah, that might be a bad tradeoff.

I wonder if we can trust the *equality* test, but not the inequality?
E.g. if compressed(A) == compressed(B) we know they're the same, but
if compressed(A) != compressed(B) we don't know they're not they still
might be.

I guess with two different versions or even completely different
algorithms we could end up with exactly the same compressed value for
different plaintexts (it's not a cryptographic hash after all), so
that's probably not an acceptable comparison either.

-- 
 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_basebackup for streaming base backups

2011-01-17 Thread Fujii Masao
On Mon, Jan 17, 2011 at 6:53 PM, Magnus Hagander mag...@hagander.net wrote:
 On Mon, Jan 17, 2011 at 09:50, Fujii Masao masao.fu...@gmail.com wrote:
 On Mon, Jan 17, 2011 at 5:44 PM, Magnus Hagander mag...@hagander.net wrote:
 Weird, no idea. Will have to look into that later - meanwhile you can grab
 the branch tip from my github repo if you want to review it.

 Which repo should I grab? You seem to have many repos :)
 http://git.postgresql.org/gitweb

 Oh, sorry about that. There is only one that contains postgresql though :P

 http://github.com/mhagander/postgres, branch streaming_base.

Thanks!

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

-- 
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 in walreceiver

2011-01-17 Thread Heikki Linnakangas

On 13.01.2011 12:35, Fujii Masao wrote:

On Thu, Jan 13, 2011 at 5:59 PM, Heikki Linnakangas
heikki.linnakan...@enterprisedb.com  wrote:

On 13.01.2011 10:28, Fujii Masao wrote:


When the master shuts down or crashes, there seems to be
the case where walreceiver exits without flushing WAL which
has already been written. This might lead startup process to
replay un-flushed WAL and break a Write-Ahead-Logging rule.


Hmm, that can happen at a crash even with no replication involved. If you
kill -9 postmaster, and some WAL had been written but not fsync'd, on
crash recovery we will happily recover the unsynced WAL.


Right. If postmaster restarts immediately after kill -9, WAL which has not
reached to the disk might be replayed. Then if the server crashes when
min recovery point indicates such an unsynced WAL, the database would
get corrupted.

As you say, that is not just about replication. But that is more likely to
happen in the standby because unsynced WAL appears while recovery
is in progress. This is one of reasons why walreceiver doesn't let the
startup process know that new WAL has arrived before flushing it, I think.

So I believe that the patch is somewhat worth applying.


Agreed, Committed.

--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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


Re: [HACKERS] walreceiver fallback_application_name

2011-01-17 Thread Magnus Hagander
On Mon, Jan 17, 2011 at 10:57, Magnus Hagander mag...@hagander.net wrote:
 On Mon, Jan 17, 2011 at 04:05, Fujii Masao masao.fu...@gmail.com wrote:
 diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
 b/src/backend/replication/libpqwalreceiver/libpqwalreceiv
 index c052df2..962ee04 100644
 --- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
 +++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
 @@ -92,7 +92,7 @@ libpqrcv_connect(char *conninfo, XLogRecPtr startpoint)
     * replication for .pgpass lookup.
     */
    snprintf(conninfo_repl, sizeof(conninfo_repl),
 -            %s dbname=replication replication=true,
 +            %s dbname=replication replication=true
 fallback_application_name=postgres,
             conninfo);

 Also the size of conninfo_repl needs to be enlarged.

 Oh, nice catch. Worked perfectly in my testing, but I see why it
 should be increased :-)

Applied with change name and the extension of the buffer.


-- 
 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_stat_replication security

2011-01-17 Thread Magnus Hagander
On Sun, Jan 16, 2011 at 21:57, Josh Berkus j...@agliodbs.com wrote:

 I suggest instead either superuser or replication permissions.

 That's another idea.

 Oh, wait.  I take that back ... we're trying to encourage users NOT to
 use the replication user as a login, yes?

yeah.

Here's a patch that limits it to superuser only. We can't easily match
it to the user of the session given the way the walsender data is
returned - it doesn't contain the user information. But limiting it to
superuser only seems perfectly reasonable and in line with the
encouragement not to use the replication user for login.

Objections?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 241131c..306af4e 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -299,7 +299,9 @@ postgres: replaceableuser/ replaceabledatabase/ replaceablehost/ re
   entryOne row per WAL sender process, showing process acronymID/,
   user OID, user name, application name, client's address and port number,
   time at which the server process began execution, current WAL sender
-  state and transaction log location.
+  state and transaction log location. The columns detailing what exactly
+  the connection is doing are only visible if the user examining the view
+  is a superuser.
  /entry
  /row
 
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 0ad6804..dba475d 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1141,8 +1141,20 @@ pg_stat_get_wal_senders(PG_FUNCTION_ARGS)
 
 		memset(nulls, 0, sizeof(nulls));
 		values[0] = Int32GetDatum(walsnd-pid);
-		values[1] = CStringGetTextDatum(WalSndGetStateString(state));
-		values[2] = CStringGetTextDatum(sent_location);
+		if (!superuser())
+		{
+			/*
+			 * Only superusers can see details. Other users only get
+			 * the pid value to know it's a walsender, but no details.
+			 */
+			nulls[1] = true;
+			nulls[2] = true;
+		}
+		else
+		{
+			values[1] = CStringGetTextDatum(WalSndGetStateString(state));
+			values[2] = CStringGetTextDatum(sent_location);
+		}
 
 		tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 	}

-- 
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 in pg_describe_object, patch v2

2011-01-17 Thread Joel Jacobson
2011/1/16 Tom Lane t...@sss.pgh.pa.us:
 Comments?

I think it's great you undertook the challenge of solving this problem
the proper way.
I think your desire to achieve perfection in every little detail is admirable.

Your patch is according to me, not far from perfect, but could be
improved in a faw ways:

a) pg_describe_object should always include the schema in the name,
even for object in public and pg_catalog. Otherwise it's not
explicitly stated whether an object relies in pg_catalog or public. If
you would create a function in the public schema in a 8.3 database,
with a name which does not exist in pg_catalog, when converting to
version 9 in the future, which provides a few more functions in the
pg_catalog schema than 8.3, there is a risk your function would
conflict with the new function with the same name in version 9. The
pg_catalog function would then be selected by default, unless
explicitly calling it with the fully qualified name. This might or
might not be what you want. Let's say you are unfortunate and unaware
it's bad to name functions pg_* in the public schema, since it's
possible, it could possibly lead to unexpected results. Now, let's go
back to the discussion on what pg_describe_object(oid,oid,int) should
return. I strongly believe it's wiser to include the schema in the
description, even though it's not important nor interesting in most
cases. It's good to explicitly inform the end-user (who's
understanding of SQL or PostgreSQL might be limited) there is a
distinct difference between public.myfunc and pg_catalog.myfunc, and
it's better to always include the schema, than to auto-detect if
there are two functions with the same name, because it's highly
confusing the description depends on your SET search_path TO
setting.

Conclusions in summary:
*) It would be a lot more user-friendly if pg_describe_object always
returned the same text string, independent of your search_path
setting.
*) It would be a lot more programmer-friendly if pg_describe_object
returned the same text string, independent of your search_path
setting and at the same time always included all the unique columns of
the object (including the schema would fix it in 99% of the cases,
and your patch would fix the remaining 1% of the cases).
*) Alternatively, it could possibly be better to introduce a new
function, only returning a text string composed using only the unique
columns (and some constants to do the formatting of the nice text
string, resolved using the unique columns), if it's not feasible to
append the schema name to the description, considering people might
(unlikely, but not impossibly) programtically rely on
pg_describe_object already and parse the text string, which would
break a lot of things.

Just some thoughts...


-- 
Best regards,

Joel Jacobson
Glue Finance

-- 
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] Warning compiling pg_dump (MinGW, Windows XP)

2011-01-17 Thread Pavel Golub
Hello, Robert.

You wrote:

RH 2011/1/13 Pavel Golub pa...@microolap.com:
 Hello, Pgsql-hackers.

 I'm getting such warnings:

 pg_dump.c: In function 'dumpSequence':
 pg_dump.c:11449:2: warning: unknown conversion type character 'l' in format
 pg_dump.c:11449:2: warning: too many arguments for format
 pg_dump.c:11450:2: warning: unknown conversion type character 'l' in format
 pg_dump.c:11450:2: warning: too many arguments for format

 Line numbers my not be the same in the official sources, because I've
 made some changes. But the lines are:

        snprintf(bufm, sizeof(bufm), INT64_FORMAT, SEQ_MINVALUE);
        snprintf(bufx, sizeof(bufx), INT64_FORMAT, SEQ_MAXVALUE);

 In my oppinion configure failed for MinGw+Windows in this case. Am I
 right? Can someone give me a hint how to avoid this?

RH It seems like PGAC_FUNC_SNPRINTF_LONG_LONG_INT_FORMAT is getting the
RH wrong answer on your machine, though I'm not sure why.  The easiest
RH workaround is probably to run configure and then edit
RH src/include/pg_config.h before compiling.

Thanks Robert. What value should I enter for this option?




-- 
With best wishes,
 Pavel  mailto:pa...@gf.microolap.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_stat_replication security

2011-01-17 Thread Itagaki Takahiro
On Mon, Jan 17, 2011 at 19:51, Magnus Hagander mag...@hagander.net wrote:
 Here's a patch that limits it to superuser only. We can't easily match
 it to the user of the session given the way the walsender data is
 returned - it doesn't contain the user information. But limiting it to
 superuser only seems perfectly reasonable and in line with the
 encouragement not to use the replication user for login.

 Objections?

It hides all fields in pg_stat_wal_senders(). Instead, can we just
revoke usage of the function and view?  Or, do we have some plans
to add fields which normal users can see?

-- 
Itagaki Takahiro

-- 
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] Warning compiling pg_dump (MinGW, Windows XP)

2011-01-17 Thread Andrew Dunstan



On 01/17/2011 05:54 AM, Pavel Golub wrote:

Hello, Robert.

You wrote:

RH  2011/1/13 Pavel Golubpa...@microolap.com:

Hello, Pgsql-hackers.

I'm getting such warnings:

pg_dump.c: In function 'dumpSequence':
pg_dump.c:11449:2: warning: unknown conversion type character 'l' in format
pg_dump.c:11449:2: warning: too many arguments for format
pg_dump.c:11450:2: warning: unknown conversion type character 'l' in format
pg_dump.c:11450:2: warning: too many arguments for format

Line numbers my not be the same in the official sources, because I've
made some changes. But the lines are:

snprintf(bufm, sizeof(bufm), INT64_FORMAT, SEQ_MINVALUE);
snprintf(bufx, sizeof(bufx), INT64_FORMAT, SEQ_MAXVALUE);

In my oppinion configure failed for MinGw+Windows in this case. Am I
right? Can someone give me a hint how to avoid this?

RH  It seems like PGAC_FUNC_SNPRINTF_LONG_LONG_INT_FORMAT is getting the
RH  wrong answer on your machine, though I'm not sure why.  The easiest
RH  workaround is probably to run configure and then edit
RH  src/include/pg_config.h before compiling.

Thanks Robert. What value should I enter for this option?




Mingw has always had a huge number of format warnings. See for example 
http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=frogmouthdt=2011-01-17%2007%3A30%3A00stg=make


If someone wants to fix them that would be good, but I'm not sure it's a 
simple task. There's probably some discussion of it in the archives back 
when we first did the Windows port.


cheers

andrew

--
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] replication and pg_hba.conf

2011-01-17 Thread Robert Haas
On Jan 17, 2011, at 1:44 AM, Heikki Linnakangas 
heikki.linnakan...@enterprisedb.com wrote:
 But I wonder if we should add lines in the default pg_hba.conf to trust 
 replication connections from loopback, like we do for normal connections?

Seems sorta pointless.

...Robert
-- 
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_stat_replication security

2011-01-17 Thread Magnus Hagander
On Mon, Jan 17, 2011 at 12:11, Itagaki Takahiro
itagaki.takah...@gmail.com wrote:
 On Mon, Jan 17, 2011 at 19:51, Magnus Hagander mag...@hagander.net wrote:
 Here's a patch that limits it to superuser only. We can't easily match
 it to the user of the session given the way the walsender data is
 returned - it doesn't contain the user information. But limiting it to
 superuser only seems perfectly reasonable and in line with the
 encouragement not to use the replication user for login.

 Objections?

 It hides all fields in pg_stat_wal_senders(). Instead, can we just
 revoke usage of the function and view?  Or, do we have some plans
 to add fields which normal users can see?

Yes, for consistency with pg_stat_activity. We let all users see which
other sessions are there, but not what they're doing - seems
reasonable to have the same definitions for replication sessions as
other sessions.

-- 
 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] Warning compiling pg_dump (MinGW, Windows XP)

2011-01-17 Thread Pavel Golub
Hello, Andrew.

You wrote:



AD On 01/17/2011 05:54 AM, Pavel Golub wrote:
 Hello, Robert.

 You wrote:

 RH  2011/1/13 Pavel Golubpa...@microolap.com:
 Hello, Pgsql-hackers.

 I'm getting such warnings:

 pg_dump.c: In function 'dumpSequence':
 pg_dump.c:11449:2: warning: unknown conversion type character 'l' in format
 pg_dump.c:11449:2: warning: too many arguments for format
 pg_dump.c:11450:2: warning: unknown conversion type character 'l' in format
 pg_dump.c:11450:2: warning: too many arguments for format

 Line numbers my not be the same in the official sources, because I've
 made some changes. But the lines are:

 snprintf(bufm, sizeof(bufm), INT64_FORMAT, SEQ_MINVALUE);
 snprintf(bufx, sizeof(bufx), INT64_FORMAT, SEQ_MAXVALUE);

 In my oppinion configure failed for MinGw+Windows in this case. Am I
 right? Can someone give me a hint how to avoid this?
 RH  It seems like PGAC_FUNC_SNPRINTF_LONG_LONG_INT_FORMAT is getting the
 RH  wrong answer on your machine, though I'm not sure why.  The easiest
 RH  workaround is probably to run configure and then edit
 RH  src/include/pg_config.h before compiling.

 Thanks Robert. What value should I enter for this option?



AD Mingw has always had a huge number of format warnings. See for example
AD 
http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=frogmouthdt=2011-01-17%2007%3A30%3A00stg=make

So you think I should just ignore these warnings? Because I can't
remember the same behaviour on 8.x branches...

AD If someone wants to fix them that would be good, but I'm not sure it's a
AD simple task. There's probably some discussion of it in the archives back
AD when we first did the Windows port.

AD cheers

AD andrew



-- 
With best wishes,
 Pavel  mailto:pa...@gf.microolap.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_basebackup for streaming base backups

2011-01-17 Thread Peter Eisentraut
On lör, 2011-01-15 at 19:10 +0100, Magnus Hagander wrote:
 This patch creates pg_basebackup in bin/, being a client program for
 the streaming base backup feature.
 
 I think it's more or less done now. I've again split it out of
 pg_streamrecv, because it had very little shared code with that
 (basically just the PQconnectdb() wrapper).
 
 One thing I'm thinking about - right now the tool just takes -c
 conninfo to connect to the database. Should it instead be taught to
 take the connection parameters that for example pg_dump does - one for
 each of host, port, user, password? (shouldn't be hard to do..)

Probably yes, for consistency.  I have been thinking for a while,
however, that it would be very good if the tools also supported a
conninfo string, so you don't have to invent a new option for every new
connection option.  psql already supports that.

Some other comments:

I had trouble at first interpreting the documentation.  In particular,
where does the data come from, and where does it go to?  -d speaks of
restoring, but I was just looking for making a backup, not restoring it.
Needs some clarification, and some complete examples.  Also what happens
if -c, or -d and -t are omitted.

Downthread you say that this tool is also useful for making base backups
independent of replication functionality.  Sounds good.  But then the
documentation says that the connection must be with a user that has the
replication permission.  Something is conceptually wrong here: why would
I have to grant replication permission just to take a base backup for
the purpose of making a backup?



-- 
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_basebackup for streaming base backups

2011-01-17 Thread Magnus Hagander
On Mon, Jan 17, 2011 at 13:38, Peter Eisentraut pete...@gmx.net wrote:
 On lör, 2011-01-15 at 19:10 +0100, Magnus Hagander wrote:
 This patch creates pg_basebackup in bin/, being a client program for
 the streaming base backup feature.

 I think it's more or less done now. I've again split it out of
 pg_streamrecv, because it had very little shared code with that
 (basically just the PQconnectdb() wrapper).

 One thing I'm thinking about - right now the tool just takes -c
 conninfo to connect to the database. Should it instead be taught to
 take the connection parameters that for example pg_dump does - one for
 each of host, port, user, password? (shouldn't be hard to do..)

 Probably yes, for consistency.  I have been thinking for a while,
 however, that it would be very good if the tools also supported a
 conninfo string, so you don't have to invent a new option for every new
 connection option.  psql already supports that.

libpq has an option to expand a connection string if it's passed in
the database name, it seems. The problem is that this is done on the
dbname parameter - won't work in pg_dump for example, without special
treatment, since the db name is used in the client there.


 Some other comments:

 I had trouble at first interpreting the documentation.  In particular,
 where does the data come from, and where does it go to?  -d speaks of
 restoring, but I was just looking for making a backup, not restoring it.
 Needs some clarification, and some complete examples.  Also what happens
 if -c, or -d and -t are omitted.

You get an error. (not with -c anymore)

I'll look at adding some further clarifications to it. Concrete
suggestions from you or others are of course also appreciated :-)


 Downthread you say that this tool is also useful for making base backups
 independent of replication functionality.  Sounds good.  But then the
 documentation says that the connection must be with a user that has the
 replication permission.  Something is conceptually wrong here: why would
 I have to grant replication permission just to take a base backup for
 the purpose of making a backup?

It uses the replication features for it. You also have to set
max_walsenders  0, which is in the replication section of the
postgresql.conf file.

The point I wanted to make downthread was that it's useful without
having a replication *slave*. But yes, you need the master.

-- 
 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] psql: Add \dL to show languages

2011-01-17 Thread Peter Eisentraut
On mån, 2011-01-17 at 07:37 +0100, Magnus Hagander wrote:
  which, as Magnus points out, includes non-procedural languages (SQL).
 
  I think that list languages could be confusing to newcomers -- the
  very people who might be reading through the help output of psql for
  the first time -- who might suppose that languages has something to
  do with the character sets supported by PostgreSQL, and might not even
  be aware that a variety of procedural languages can be used inside the
  database.
 
  Fair point.
 
 Yeah. Procedural langauges may strictly be wrong, but people aren't
 likely to misunderstand it.

The term procedural in this context originated with Oracle's PL/SQL,
which is a procedural language extension to the non-procedural SQL
language.  From this came PostgreSQL's PL/pgSQL, and that naming was
then continued with PL/Tcl, at which point PL/$X lost its original
meaning of procedural extension to the non-procedural language $X and
meant more or less handler for writing PostgreSQL functions in language
$X.

Otherwise PL/Scheme will blow your mind. :)

Think of procedural language as language for writing [PostgreSQL]
procedures.  As was pointed out, it's also a useful convention for
distinguishing this from other languages, such as message
translations.



-- 
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] Warning compiling pg_dump (MinGW, Windows XP)

2011-01-17 Thread Robert Haas
On Mon, Jan 17, 2011 at 5:54 AM, Pavel Golub pa...@microolap.com wrote:
 RH It seems like PGAC_FUNC_SNPRINTF_LONG_LONG_INT_FORMAT is getting the
 RH wrong answer on your machine, though I'm not sure why.  The easiest
 RH workaround is probably to run configure and then edit
 RH src/include/pg_config.h before compiling.

 Thanks Robert. What value should I enter for this option?

Not sure.  I notice that the configure test has this comment:

# PGAC_FUNC_SNPRINTF_LONG_LONG_INT_FORMAT
# ---
# Determine which format snprintf uses for long long int.  We handle
# %lld, %qd, %I64d.  The result is in shell variable
# LONG_LONG_INT_FORMAT.
#
# MinGW uses '%I64d', though gcc throws an warning with -Wall,
# while '%lld' doesn't generate a warning, but doesn't work.
#

...and the values the test actually tries are:

%lld
%qd
%I64d

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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


Re: [HACKERS] texteq/byteaeq: avoid detoast [REVIEW]

2011-01-17 Thread Robert Haas
On Mon, Jan 17, 2011 at 2:56 AM, Peter Eisentraut pete...@gmx.net wrote:
 On mån, 2011-01-17 at 07:35 +0100, Magnus Hagander wrote:
 For text, I think locales may make that impossible. Aren't there
 locale rules where two different characters can behave the same when
 comparing them? I know in Swedish at least w and v behave the same
 when sorting (but not when comparing) in some variants of the locale.

 In fact, aren't there cases where the *length test* also fails? I
 don't know this for sure, but unless we know for certain that two
 different length strings can never be the same *independent of
 locale*, this whole patch has a big problem...

 Currently, two text values are only equal of strcoll() considers them
 equal and the bits are the same.  So this patch is safe in that regard.

 There is, however, some desire to loosen this.  Possible applications
 are case-insensitive comparison and Unicode normalization.  It's not
 going to happen soon, but it may be worth considering not putting in an
 optimization that we'll end up having to rip out again in a year
 perhaps.

Hmm.  I hate to give up on this - it's a nice optimization for the
cases to which it applies.   Would it be possible to jigger things so
that we can still do it byte-for-byte when case-insensitive comparison
or Unicode normalization AREN'T in use?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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


Re: [HACKERS] Determining client_encoding from client locale

2011-01-17 Thread Susanne Ebrecht

On 14.01.2011 20:12, Peter Eisentraut wrote:

I have adjusted your old patch for the current tree, and it seems to
work.  I think it was just forgotten last time because the move to
PQconnectdbParams had to happen first.  But I'll throw it back into the
ring now.


Hello,

maybe i missed pre-discussion but ...

I miss considering auto-detect of file encoding.

A simple example:
$ psql -f dump.sql db

What happens when dump.sql is written by using
another encoding?

I just would expect that when client encoding is detected
by automatism that it also will be detected when I use
files.

My own experience from the others is that users
more often forget to think about encoding when they
deal with files as when they type in the client.

Anyway, the code itself seems ok.
I just would recommend to document that the client encoding
should be checked from the user anyway. Just to make sure that
it is not our fault, when there is a libc bug.

Just my 2ct,

Susanne

--
Susanne Ebrecht - 2ndQuadrant
PostgreSQL Development, 24x7 Support, Training and Services
www.2ndQuadrant.com


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


Re: [HACKERS] Replication logging

2011-01-17 Thread Robert Haas
On Mon, Jan 17, 2011 at 1:53 AM, Magnus Hagander mag...@hagander.net wrote:
 On Mon, Jan 17, 2011 at 03:06, Robert Haas robertmh...@gmail.com wrote:
 On Sun, Jan 16, 2011 at 9:19 AM, Magnus Hagander mag...@hagander.net wrote:
 Currently, replication connections *always* logs something like:
 LOG:  replication connection authorized: user=mha host=[local]

 There's no way to turn that off.

 I can't find the reasoning behind this - why is this one not
 controlled by log_connections like normal ones? There's a comment in
 the code that says this is intentional, but I can't figure out why...

 Because it's reasonably likely that you'd want to log replication
 connections but not regular ones?  On the theory that replication is
 more important than an ordinary login?

 Well, a superuser connection is even worse, but we don't hard-code
 logging of those.

From a security perspective, perhaps; but not from an oh crap my
replication slave can't connect I'm hosed if the server crashes
perspective.

 What do you have in mind?

 Either having it controlled by log_connections, or perhaps have a
 log_highpriv_connections that controls replication *and* superuser, to
 be somewhat consistent.

-1.  We could provide an option to turn this on and off, but I
wouldn't want it merged with log_connections or logging of superuser
connections.

Incidentally, I think ClientAuthentication_hook is sufficiently
powerful to allow logging of superuser connections but no others, if
someone wanted to write a contrib module.  That doesn't necessarily
mean an in-core facility wouldn't be useful too, but it's at least
worth thinking about using the hook.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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


Re: [HACKERS] review: FDW API

2011-01-17 Thread Shigeru HANADA
On Sun, 16 Jan 2011 01:55:19 +0100
Jan Urbański wulc...@wulczer.org wrote:
 what follows is a review of the FDW API patch from
 http://archives.postgresql.org/message-id/20110114212358.82c7.69899...@metrosystems.co.jp

Thanks for the comments!
For now, I answer to the first half of your comments. I'll answer to
the rest soon.

 All three patches apply cleanly and compile without warnings. Regression
 tests pass.
 
 Let me go patch by patch, starting with the first one that adds the
 HANDLER option.

Sure.

 It adds one useless hunk in src/backend/commands/foreigncmds.c (changes
 order if #includes).
 
 There's a typo in a C commnent (determin which validator to be used).
 
 Other than that, it looks OK.

Fixed in attached patch.

 The third patch just adds a GetForeignTable helper function and it looks OK.

Thanks.  This patch might be able to committed separately because it
would be small enough, and similar to existing lookup functions such
as GetForeignDataWrapper() and GetForeignServer().

 The second patch actually adds the API. First of all, I'd like say that
 it's a really cool piece of code, allowing all kinds of awesome
 funcionality to be added. I'm already excited by the things that this
 will make possible. Congratulations!
 
 To get a feel of the API I wrote a simple FDW wrapper that presents data
 from the commitfest RSS feed, based heavily on twitter_fdw by Hitoshi
 Harada. Please treat my thoughts as someone's who doesn't really know
 *why* the API looks like it does, but has some observations about what
 was missing or what felt strange when using it. I guess that's the
 position a typical FDW wrapper writer will be in.

Sure, I think your point of view is very important.

 First of all, the C comments mention that PlanRelScan should put a tuple
 descriptor in FdwPlan, but there's no such field in it. Also, comments
 for PlanRelScan talk about the 'attnos' argument, which is not in the
 function's signature. I guess the comments are just obsolete and should
 be updated. I think it's actually a good thing you don't have to put a
 TupleDesc in FdwPlan.

Removed comments about 'attnos' and tuple descriptor.

 There are two API methods, PlanNative and PlanQuery that are ifdef'd out
 using IN_THE_FUTURE. Isn't the standard symbol we use NOT_USED? Also,
 the comments say you can implement either PlanRelScan or PlanNative, and
 only the former is available for now. If we keep these methods
 commented, they should be moved to the end of the struct, so that
 uncommenting them will not break compatibility with existing FDWs.

Agreed.  Moved ifdef'd part to the end of struct.

 The only documentation a FDW writer has is fdwapi.h, so comments there
 need to be top-notch. We might contemplate writing a documentation
 chapter explaining how FDW handlers should be written, like we do for C
 SRFs and libpq programs, but I guess for now just the headers file would
 be enough.

file_fdw and postgresql_fdw would be good samples  for wrapper
developer if we could ship them as contrib modules.

 FdwExecutionState is just a struct around a void pointer, can we imagine
 adding more fields there? If not, maybe we could just remove the
 structure and pass void pointers around? OTOH that gives us some
 compiler checking and possibility of extending the struct, so I guess we
 could also just leave it like that.

ISTM that using a struct as a interface is better than void*, as you
mentioned.

 The Iterate method gets passed a TupleTableSlot. Do we really need such
 a low-level structure? It makes returning the result easy, because you
 just form your tuple and call ExecStoreTuple, but perhaps we could
 abstract that away a bit and add a helper method that will take a tuple
 and call ExecStoreTuple for us, passing InvalidBuffer and false as the
 remaining arguments. Or maybe make Iterate return the tuple and call
 ExecStoreTuple internally? I don't have strong opinions, but
 TupleTableSlot feels a bit too gutty - I'm having a hard time imagining
 what fields from it would be useful for a FDW writer, and so perhaps you
 don't need to expose it.

This would be debatable issue.  Currently Iterate() is expected to
return materialized HeapTuple through TupleTableSlot.

I think an advantage to use TupleTableSlot is that FDW can set shoudFree
flag for the tuple.

 Why does BeginScan accept a ParamListInfo argument? First of all, it
 feels like a parsing thing, not a relation scan thing, so perhaps it
 should be available at some other, earlier stage. Second of all, what
 would it be useful for anyway? Neither file_fdw nor my commitfest_fdw
 does anything with it.

ParamListInfo is added to pass parameters of PREPARE/EXECUTE statement
to FDWs.

Plan for a prepared query is generated at PREPARE with placeholders,
and executed at EXECUTE with actual values.  With  ParamListInfo
parameter for BeginScan(), FDWs would be able to optimize their remote
query with actual parameter values.

Regard,
--
Shigeru Hanada



-- 
Sent via 

Re: [HACKERS] Fwd: [JDBC] Weird issues when reading UDT from stored function

2011-01-17 Thread Robert Haas
On Mon, Jan 17, 2011 at 12:00 AM, Oliver Jowett oli...@opencloud.com wrote:
 However, doing the same via a plpgsql function with an OUT parameter
 produces something completely mangled:

 test_udt=# CREATE FUNCTION p_enhance_address2 (address OUT u_address_type)
 AS $$ BEGIN SELECT t_author.address INTO address FROM t_author WHERE
 first_name = 'George'; END; $$ LANGUAGE plpgsql;
 CREATE FUNCTION

 test_udt=# SELECT * FROM p_enhance_address2();
               street                | zip | city | country | since | code

 -+-+--+-+---+--
  ((Parliament Hill,77),NW31A9) |     |      |         |       |
 (1 row)

 Here, we've somehow got the first two fields of u_address_type - street and
 zip - squashed together into one column named 'street', and all the other
 columns nulled out.

I think this is the old problem of PL/pgsql having two forms of SELECT
INTO.  You can either say:

SELECT col1, col2, col3, ... INTO recordvar FROM ...

Or you can say:

SELECT col1, col2, col3, ... INTO nonrecordvar1, nonrecordvar2,
nonrecordvar3, ... FROM ...

In this case, since address is a recordvar, it's expecting the first
form - thus the first select-list item gets matched to the first
column of the address, rather than to address as a whole.  It's not
smart enough to consider the types of the items involved - only
whether they are records.  :-(

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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


Re: [HACKERS] review: FDW API

2011-01-17 Thread Shigeru HANADA
On Mon, 17 Jan 2011 22:13:19 +0900
Shigeru HANADA han...@metrosystems.co.jp wrote:
 Fixed in attached patch.

Sorry, I have not attached patch to last message.
I'll post revised patch in next message soon.

Regards,
--
Shigeru Hanada



-- 
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] Determining client_encoding from client locale

2011-01-17 Thread Peter Geoghegan
On 17 January 2011 12:58, Susanne Ebrecht susa...@2ndquadrant.com wrote:
 Hello,

 maybe i missed pre-discussion but ...

 I miss considering auto-detect of file encoding.

 A simple example:
 $ psql -f dump.sql db

 What happens when dump.sql is written by using
 another encoding?

That doesn't tend to be much of a problem in practice because pg_dump
will have the dump SET client_encoding as appropriate from the DB
encoding.

-- 
Regards,
Peter Geoghegan

-- 
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_basebackup for streaming base backups

2011-01-17 Thread Fujii Masao
On Mon, Jan 17, 2011 at 7:14 PM, Fujii Masao masao.fu...@gmail.com wrote:
 Oh, sorry about that. There is only one that contains postgresql though :P

 http://github.com/mhagander/postgres, branch streaming_base.

 Thanks!

Though I haven't seen the core part of the patch (i.e.,
ReceiveTarFile, etc..) yet,
here is the comments against others.


+   if (strcmp(argv[1], -h) == 0 || strcmp(argv[1], --help) == 
0 ||
+   strcmp(argv[1], -?) == 0)

strcmp(argv[1], -h) should be removed


+   printf(_(  -p, --progressshow progress information\n));

-p needs to be changed to -P


+   printf(_(  -D, --pgdata=directory   receive base backup into 
directory\n));
+   printf(_(  -T, --tardir=directoryreceive base backup into tar 
files\n
+stored in specified 
directory\n));
+   printf(_(  -Z, --compress=0-9compress tar output\n));
+   printf(_(  -l, --label=label set backup label\n));
+   printf(_(  -p, --progressshow progress information\n));
+   printf(_(  -v, --verbose output verbose messages\n));

Can we list those options in alphabetical order as other tools do?

Like -f and -F option of pg_dump, it's more intuitive to provide one option for
output directory and that for format. Something like

-d directory
--dir=directory

-F format
--format=format

p
plain

t
tar


+   case 'p':
+   if (atoi(optarg) == 0)
+   {
+   fprintf(stderr, _(%s: invalid port 
number \%s\),
+   progname, optarg);
+   exit(1);
+   }

Shouldn't we emit an error message when the result of atoi is *less than* or
equal to 0? \n should be in the tail of the error message. Is this error check
really required here? IIRC libpq does. If it's required, atoi for compresslevel
should also be error-checked.


+   case 'v':
+   verbose++;

Why is the verbose defined as integer?


+   if (optind  argc)
+   {
+   fprintf(stderr,
+   _(%s: too many command-line arguments (first 
is \%s\)\n),
+   progname, argv[optind + 1]);

You need to reference to argv[optind] instead.

What about using PGDATA environment variable when no target directory is
specified?


+ * Verify that the given directory exists and is empty. If it does not
+ * exist, it is created. If it exists but is not empty, an error will
+ * be give and the process ended.
+ */
+static void
+verify_dir_is_empty_or_create(char *dirname)

Since verify_dir_is_empty_or_create can be called after the connection has
been established, it should call PQfinish before exit(1).


+   keywords[2] = fallback_application_name;
+   values[2] = pg_basebackup;

Using the progname variable seems better rather than the fixed word
pg_basebackup.


+   if (dbgetpassword == 1)
+   {
+   /* Prompt for a password */
+   password = simple_prompt(_(Password: ), 100, false);

PQfinish should be called for the password retry case.


+   if (PQstatus(conn) != CONNECTION_OK)
+   {
+   fprintf(stderr, _(%s: could not connect to server: 
%s\n),
+   progname, PQerrorMessage(conn));
+   exit(1);
+   }

PQfinish seems required before exit(1).


+   if (PQsendQuery(conn, current_path) == 0)
+   {
+   fprintf(stderr, _(%s: coult not start base backup: %s\n),

Typo: s/coult/could


+   /*
+* Get the header
+*/
+   res = PQgetResult(conn);

After this, PQclear seems required before each exit(1) call.


+   if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
+   {
+   fprintf(stderr, _(%s: final receive failed: %s\n),
+   progname, PQerrorMessage(conn));
+   exit(1);
+   }

PQfinish seems required before exit(1).

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

-- 
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_basebackup for streaming base backups

2011-01-17 Thread Fujii Masao
On Mon, Jan 17, 2011 at 9:43 PM, Magnus Hagander mag...@hagander.net wrote:
 Probably yes, for consistency.  I have been thinking for a while,
 however, that it would be very good if the tools also supported a
 conninfo string, so you don't have to invent a new option for every new
 connection option.  psql already supports that.

 libpq has an option to expand a connection string if it's passed in
 the database name, it seems. The problem is that this is done on the
 dbname parameter - won't work in pg_dump for example, without special
 treatment, since the db name is used in the client there.

If conninfo is specified, you can just append the dbname=replication
into it and pass it to libpq as dbname. I don't think that supporting
conninfo is difficult.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

-- 
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_basebackup for streaming base backups

2011-01-17 Thread Simon Riggs
On Mon, 2011-01-17 at 13:43 +0100, Magnus Hagander wrote:
 Downthread you say that this tool is also useful for making base
 backups
  independent of replication functionality.  Sounds good.  But then
 the
  documentation says that the connection must be with a user that has
 the
  replication permission.  Something is conceptually wrong here: why
 would
  I have to grant replication permission just to take a base backup
 for
  the purpose of making a backup?
 
 It uses the replication features for it. You also have to set
 max_walsenders  0, which is in the replication section of the
 postgresql.conf file.
 
 The point I wanted to make downthread was that it's useful without
 having a replication *slave*. But yes, you need the master.

Suggest calling this utility pg_replication_backup
and the other utility pg_replication_stream

It will be easier to explain the connection with replication.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and 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] pg_basebackup for streaming base backups

2011-01-17 Thread Magnus Hagander
On Mon, Jan 17, 2011 at 14:30, Fujii Masao masao.fu...@gmail.com wrote:
 On Mon, Jan 17, 2011 at 7:14 PM, Fujii Masao masao.fu...@gmail.com wrote:
 Oh, sorry about that. There is only one that contains postgresql though :P

 http://github.com/mhagander/postgres, branch streaming_base.

 Thanks!

 Though I haven't seen the core part of the patch (i.e.,
 ReceiveTarFile, etc..) yet,
 here is the comments against others.


 +               if (strcmp(argv[1], -h) == 0 || strcmp(argv[1], --help) 
 == 0 ||
 +                       strcmp(argv[1], -?) == 0)

 strcmp(argv[1], -h) should be removed

Oh, crap. From the addition of -h for host. oopsie.

 +       printf(_(  -p, --progress            show progress information\n));

 -p needs to be changed to -P

Indeed.

 +       printf(_(  -D, --pgdata=directory   receive base backup into 
 directory\n));
 +       printf(_(  -T, --tardir=directory    receive base backup into tar 
 files\n
 +                                                    stored in specified 
 directory\n));
 +       printf(_(  -Z, --compress=0-9        compress tar output\n));
 +       printf(_(  -l, --label=label         set backup label\n));
 +       printf(_(  -p, --progress            show progress information\n));
 +       printf(_(  -v, --verbose             output verbose messages\n));

 Can we list those options in alphabetical order as other tools do?

Certainly. But it makes more sense to have -D and -T next to each
other, I think - they'd end up way elsewhere. Perhaps we need a group
taht says target?


 Like -f and -F option of pg_dump, it's more intuitive to provide one option 
 for
 output directory and that for format. Something like

    -d directory
    --dir=directory

    -F format
    --format=format

    p
    plain

    t
    tar

That's another option. It would certainly make for more consistency -
probably a better idea.

 +                       case 'p':
 +                               if (atoi(optarg) == 0)
 +                               {
 +                                       fprintf(stderr, _(%s: invalid port 
 number \%s\),
 +                                                       progname, optarg);
 +                                       exit(1);
 +                               }

 Shouldn't we emit an error message when the result of atoi is *less than* or
 equal to 0? \n should be in the tail of the error message. Is this error check
 really required here? IIRC libpq does. If it's required, atoi for 
 compresslevel
 should also be error-checked.

Yes on all.


 +                       case 'v':
 +                               verbose++;

 Why is the verbose defined as integer?

I envisioned multiple level of verbosity (which I had in
pg_streamrecv), where multiple -v's would add things.

 +       if (optind  argc)
 +       {
 +               fprintf(stderr,
 +                               _(%s: too many command-line arguments (first 
 is \%s\)\n),
 +                               progname, argv[optind + 1]);

 You need to reference to argv[optind] instead.

Check. Copy/paste:o.


 What about using PGDATA environment variable when no target directory is
 specified?

Hmm. I don't really like that. I prefer requiring it to be specified.


 + * Verify that the given directory exists and is empty. If it does not
 + * exist, it is created. If it exists but is not empty, an error will
 + * be give and the process ended.
 + */
 +static void
 +verify_dir_is_empty_or_create(char *dirname)

 Since verify_dir_is_empty_or_create can be called after the connection has
 been established, it should call PQfinish before exit(1).

Yeah, that's a general thing - do we need to actually bother doing
that? At most exit() places I haven't bothered free:ing memory or
closing the connection.

It's not just the PQclear() that you refer to further down - it's also
all the xstrdup()ed strings for example. Do we really need to care
about those before we do exit(1)? I think not.

Requiring PQfinish() might be more reasonable since it will give you a
log on the server if you don't, but I'm not convinced that's necessary
either?


snip similar requirements


 +       keywords[2] = fallback_application_name;
 +       values[2] = pg_basebackup;

 Using the progname variable seems better rather than the fixed word
 pg_basebackup.

I don't think so - that turns into argv[0], which means that if you
use the full path of the program (/usr/local/pgsql/bin/pg_basebackup
for example), the entire path goes into fallback_application_name -
not just the program name.

-- 
 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_basebackup for streaming base backups

2011-01-17 Thread Magnus Hagander
On Mon, Jan 17, 2011 at 14:43, Fujii Masao masao.fu...@gmail.com wrote:
 On Mon, Jan 17, 2011 at 9:43 PM, Magnus Hagander mag...@hagander.net wrote:
 Probably yes, for consistency.  I have been thinking for a while,
 however, that it would be very good if the tools also supported a
 conninfo string, so you don't have to invent a new option for every new
 connection option.  psql already supports that.

 libpq has an option to expand a connection string if it's passed in
 the database name, it seems. The problem is that this is done on the
 dbname parameter - won't work in pg_dump for example, without special
 treatment, since the db name is used in the client there.

 If conninfo is specified, you can just append the dbname=replication
 into it and pass it to libpq as dbname. I don't think that supporting
 conninfo is difficult.

Yeah, it's easy enough for pg_basebackup. But not for example for
pg_dump. (I meant for being able to use it more or less with zero
modification to the current code - it can certainly be adapted to be
able to deal with it)

-- 
 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_basebackup for streaming base backups

2011-01-17 Thread Magnus Hagander
On Mon, Jan 17, 2011 at 14:49, Simon Riggs si...@2ndquadrant.com wrote:
 On Mon, 2011-01-17 at 13:43 +0100, Magnus Hagander wrote:
 Downthread you say that this tool is also useful for making base
 backups
  independent of replication functionality.  Sounds good.  But then
 the
  documentation says that the connection must be with a user that has
 the
  replication permission.  Something is conceptually wrong here: why
 would
  I have to grant replication permission just to take a base backup
 for
  the purpose of making a backup?

 It uses the replication features for it. You also have to set
 max_walsenders  0, which is in the replication section of the
 postgresql.conf file.

 The point I wanted to make downthread was that it's useful without
 having a replication *slave*. But yes, you need the master.

 Suggest calling this utility pg_replication_backup
 and the other utility pg_replication_stream

 It will be easier to explain the connection with replication.

Hmm. I don't like those names at all :(

But that's just me - and I don't totally hate them. So I'm opening the
floor for other peoples votes :-)

-- 
 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] Replication logging

2011-01-17 Thread Magnus Hagander
On Mon, Jan 17, 2011 at 14:00, Robert Haas robertmh...@gmail.com wrote:
 On Mon, Jan 17, 2011 at 1:53 AM, Magnus Hagander mag...@hagander.net wrote:
 On Mon, Jan 17, 2011 at 03:06, Robert Haas robertmh...@gmail.com wrote:
 On Sun, Jan 16, 2011 at 9:19 AM, Magnus Hagander mag...@hagander.net 
 wrote:
 Currently, replication connections *always* logs something like:
 LOG:  replication connection authorized: user=mha host=[local]

 There's no way to turn that off.

 I can't find the reasoning behind this - why is this one not
 controlled by log_connections like normal ones? There's a comment in
 the code that says this is intentional, but I can't figure out why...

 Because it's reasonably likely that you'd want to log replication
 connections but not regular ones?  On the theory that replication is
 more important than an ordinary login?

 Well, a superuser connection is even worse, but we don't hard-code
 logging of those.

 From a security perspective, perhaps; but not from an oh crap my
 replication slave can't connect I'm hosed if the server crashes
 perspective.

True, there are more than one ways to look at them.

That doesn't mean one is more important than the other though, so they
should be equally configurable, imho.


 What do you have in mind?

 Either having it controlled by log_connections, or perhaps have a
 log_highpriv_connections that controls replication *and* superuser, to
 be somewhat consistent.

 -1.  We could provide an option to turn this on and off, but I
 wouldn't want it merged with log_connections or logging of superuser
 connections.

Fair enough, we could have a log_replication_connections as a separate
one then? Or having one log_connections, one
log_replication_connections and one log_superuser_connections?


 Incidentally, I think ClientAuthentication_hook is sufficiently
 powerful to allow logging of superuser connections but no others, if
 someone wanted to write a contrib module.  That doesn't necessarily
 mean an in-core facility wouldn't be useful too, but it's at least
 worth thinking about using the hook.

Do we have an example of this hook somewhere already? If not, it could
be made into a useful example of that, perhaps?

-- 
 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] Determining client_encoding from client locale

2011-01-17 Thread Susanne Ebrecht

On 17.01.2011 14:26, Peter Geoghegan wrote:

On 17 January 2011 12:58, Susanne Ebrechtsusa...@2ndquadrant.com  wrote:

Hello,

maybe i missed pre-discussion but ...

I miss considering auto-detect of file encoding.

A simple example:
$ psql -f dump.sql db

What happens when dump.sql is written by using
another encoding?

That doesn't tend to be much of a problem in practice because pg_dump
will have the dump SET client_encoding as appropriate from the DB
encoding.


Ok, naming it dump.sql was confusing. My fault.
I didn't thought about pg_dump dump files here.
I more thought about files that came out of editors using mad encoding
and maybe then also were created on Windows and then copied to
Unix for import.

Written on little endian, copied to big endian and so on.

Susanne

--
Susanne Ebrecht - 2ndQuadrant
PostgreSQL Development, 24x7 Support, Training and Services
www.2ndQuadrant.com


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


Re: [HACKERS] pg_basebackup for streaming base backups

2011-01-17 Thread Simon Riggs
On Mon, 2011-01-17 at 14:55 +0100, Magnus Hagander wrote:
 
  It uses the replication features for it. You also have to set
  max_walsenders  0, which is in the replication section of the
  postgresql.conf file.
 
  The point I wanted to make downthread was that it's useful without
  having a replication *slave*. But yes, you need the master.
 
  Suggest calling this utility pg_replication_backup
  and the other utility pg_replication_stream
 
  It will be easier to explain the connection with replication.
 
 Hmm. I don't like those names at all :(
 
 But that's just me - and I don't totally hate them. So I'm opening the
 floor for other peoples votes :-)

No problem. My point is that we should look for a name that illustrates
the function more clearly. If Peter was confused, others will be also.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and 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] SSPI client authentication in non-Windows builds

2011-01-17 Thread Magnus Hagander
On Mon, Jan 3, 2011 at 14:11, Christian Ullrich ch...@chrullrich.net wrote:
 Hello all,

 this patch adds support for connecting to servers running on Windows
 and requesting SSPI authentication. It does this by treating
 AUTH_REQ_SSPI the same as AUTH_REQ_GSS if no native SSPI support is
 available.

 In addition to being generally useful, this is a workaround to a
 problem with MIT KfW that I encountered back in September 2010 [1].

 This change has been tested and works correctly on FreeBSD 8.1, using
 the Kerberos and GSSAPI libraries from Heimdal 1.4. The server is
 running PostgreSQL 9.0.2 on Windows 2008.

Does this require some certain minimum version of the kerberos
libraries? Do you know if it works with just Heimdal or both Heimdal
and MIT?

What I'm after is: do we need a autoconf check, or a #ifdef, or
something to make sure we don't enable it in a scenario where it won't
work?

-- 
 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] Warning compiling pg_dump (MinGW, Windows XP)

2011-01-17 Thread Andrew Dunstan



On 01/17/2011 07:18 AM, Pavel Golub wrote:


AD  Mingw has always had a huge number of format warnings. See for example
AD  
http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=frogmouthdt=2011-01-17%2007%3A30%3A00stg=make

So you think I should just ignore these warnings? Because I can't
remember the same behaviour on 8.x branches...




We've had them all along, as I said. See 
http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=frogmouthdt=2011-01-04%2023%3A54%3A16stg=make 
for the same thing in an 8.2 build.


cheers

andre

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


[HACKERS] Add getopt() support to test_fsync

2011-01-17 Thread Bruce Momjian
The attached, applied patch adds getopt() support to test_fsync.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/src/tools/fsync/README b/src/tools/fsync/README
index 5bf6f1f..fee9c62 100644
*** /tmp/pgdiff.11631/0CF5hb_README	Mon Jan 17 09:34:35 2011
--- src/tools/fsync/README	Mon Jan 17 09:28:15 2011
*** test_fsync
*** 3,9 
  
  This program tests fsync.  The tests are described as part of the program output.
  
! 	Usage:	test_fsync [-f filename] [ops_per_test]
  	
  test_fsync is intended to give you a reasonable idea of what the fastest
  fsync_method is on your specific system, as well as supplying diagnostic
--- 3,13 
  
  This program tests fsync.  The tests are described as part of the program output.
  
! 	Usage: test_fsync [option...]
! 
! Options:
! 	-f, --filename		specify filename for test
! 	-o, --ops-per-test	operations per test
  	
  test_fsync is intended to give you a reasonable idea of what the fastest
  fsync_method is on your specific system, as well as supplying diagnostic
*** The output filename defaults to test_fsy
*** 16,21 
  test_fsync should be run in the same filesystem as your transaction log
  directory (pg_xlog).
  
! Ops-per-test defaults to 2000.  Increase this to get more accurate
  measurements.
  
--- 20,25 
  test_fsync should be run in the same filesystem as your transaction log
  directory (pg_xlog).
  
! Operations per test defaults to 2000.  Increase this to get more accurate
  measurements.
  
diff --git a/src/tools/fsync/test_fsync.c b/src/tools/fsync/test_fsync.c
index c0c58f6..b1cec74 100644
*** /tmp/pgdiff.11631/YRJFLe_test_fsync.c	Mon Jan 17 09:34:35 2011
--- src/tools/fsync/test_fsync.c	Mon Jan 17 09:27:44 2011
***
*** 8,13 
--- 8,14 
  
  #include postgres.h
  
+ #include getopt_long.h
  #include access/xlog_internal.h
  #include access/xlog.h
  #include access/xlogdefs.h
*** main(int argc, char *argv[])
*** 80,105 
  void
  handle_args(int argc, char *argv[])
  {
! 	if (argc  1  strcmp(argv[1], -h) == 0)
  	{
! 		fprintf(stderr, test_fsync [-f filename] [ops-per-test]\n);
! 		exit(1);
  	}
! 	
! 	/* 
! 	 * arguments: ops_per_test and filename (optional) 
! 	 */
! 	if (argc  2  strcmp(argv[1], -f) == 0)
  	{
! 		filename = argv[2];
! 		argv += 2;
! 		argc -= 2;
! 	}
  
! 	if (argc  1) 
! 		ops_per_test = atoi(argv[1]);
  
! 	printf(Ops-per-test = %d\n\n, ops_per_test);
  }
  
  void
--- 81,132 
  void
  handle_args(int argc, char *argv[])
  {
! 	static struct option long_options[] = {
! 		{filename, required_argument, NULL, 'f'},
! 		{ops-per-test, required_argument, NULL, 'o'},
! 		{NULL, 0, NULL, 0}
! 	};
! 	int			option;			/* Command line option */
! 	int			optindex = 0;	/* used by getopt_long */
! 
! 	if (argc  1)
  	{
! 		if (strcmp(argv[1], --help) == 0 || strcmp(argv[1], -h) == 0 ||
! 			strcmp(argv[1], -?) == 0)
! 		{
! 			fprintf(stderr, test_fsync [-f filename] [ops-per-test]\n);
! 			exit(0);
! 		}
! 		if (strcmp(argv[1], --version) == 0 || strcmp(argv[1], -V) == 0)
! 		{
! 			fprintf(stderr,test_fsync  PG_VERSION \n);
! 			exit(0);
! 		}
  	}
! 
! 	while ((option = getopt_long(argc, argv, f:o:,
!  long_options, optindex)) != -1)
  	{
! 		switch (option)
! 		{
! 			case 'f':
! filename = strdup(optarg);
! break;
  
! 			case 'o':
! ops_per_test = atoi(optarg);
! break;
! 
! 			default:
! fprintf(stderr,
! 	   Try \%s --help\ for more information.\n,
! 	   test_fsync);
! exit(1);
! break;
! 		}
! 	}
  
! 	printf(%d operations per test\n\n, ops_per_test);
  }
  
  void
*** test_open_syncs(void)
*** 448,454 
  	}
  	
  	if ((tmpfile = open(filename, O_RDWR | OPEN_SYNC_FLAG | PG_O_DIRECT, 0)) == -1)
! 		printf(NA_FORMAT, n/a**\n);
  	else
  	{
  		printf(LABEL_FORMAT, 2 open_sync 8k writes);
--- 475,481 
  	}
  	
  	if ((tmpfile = open(filename, O_RDWR | OPEN_SYNC_FLAG | PG_O_DIRECT, 0)) == -1)
! 		printf(NA_FORMAT, o_direct, n/a**\n);
  	else
  	{
  		printf(LABEL_FORMAT, 2 open_sync 8k writes);

-- 
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] psql: Add \dL to show languages

2011-01-17 Thread David Fetter
On Mon, Jan 17, 2011 at 02:48:43PM +0200, Peter Eisentraut wrote:
 On mån, 2011-01-17 at 07:37 +0100, Magnus Hagander wrote:
   which, as Magnus points out, includes non-procedural languages
   (SQL).
  
   I think that list languages could be confusing to newcomers
   -- the very people who might be reading through the help output
   of psql for the first time -- who might suppose that
   languages has something to do with the character sets
   supported by PostgreSQL, and might not even be aware that a
   variety of procedural languages can be used inside the
   database.
  
   Fair point.
  
  Yeah. Procedural langauges may strictly be wrong, but people
  aren't likely to misunderstand it.
 
 The term procedural in this context originated with Oracle's
 PL/SQL, which is a procedural language extension to the
 non-procedural SQL language.

We can repurpose 'P' to mean, Programming or PostgreSQL, and have done :)

Cheers,
David.
-- 
David Fetter da...@fetter.org http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

-- 
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] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.

2011-01-17 Thread Peter Eisentraut
On fre, 2011-01-14 at 18:45 -0500, Robert Haas wrote:
 On Fri, Jan 14, 2011 at 3:45 PM, Marti Raudsepp ma...@juffo.org
 wrote:
  There's a similar case with CREATE TABLE IF NOT EXISTS, maybe this
 is
  worth covering in an updated patch too?
  And if I change that, people might expect the same from DROP X IF
 EXISTS too?
 
 It's far less clear what you'd change those cases to say, and they
 already emit a NOTICE, so it seems unnecessary.

Maybe instead of the proposed patch, a notice could be added:

NOTICE: existing object was replaced



-- 
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] Replication logging

2011-01-17 Thread Robert Haas
On Mon, Jan 17, 2011 at 8:58 AM, Magnus Hagander mag...@hagander.net wrote:
 What do you have in mind?

 Either having it controlled by log_connections, or perhaps have a
 log_highpriv_connections that controls replication *and* superuser, to
 be somewhat consistent.

 -1.  We could provide an option to turn this on and off, but I
 wouldn't want it merged with log_connections or logging of superuser
 connections.

 Fair enough, we could have a log_replication_connections as a separate
 one then? Or having one log_connections, one
 log_replication_connections and one log_superuser_connections?

log_replication_connections seems reasonable.  Not sure about
log_superuser_connections.

 Incidentally, I think ClientAuthentication_hook is sufficiently
 powerful to allow logging of superuser connections but no others, if
 someone wanted to write a contrib module.  That doesn't necessarily
 mean an in-core facility wouldn't be useful too, but it's at least
 worth thinking about using the hook.

 Do we have an example of this hook somewhere already? If not, it could
 be made into a useful example of that, perhaps?

contrib/auth_delay

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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


Re: [HACKERS] SSI patch version 12

2011-01-17 Thread David Fetter
On Mon, Jan 17, 2011 at 09:58:35AM +0200, Anssi Kääriäinen wrote:
 One thing I have been thinking about is how does predicate locking
 indexes work when using functional indexes and functions marked as
 immutable but which really aren't.  I don't know how predicate
 locking indexes works, so it might be that this is a non-issue.  I
 haven't been able to break anything in this way, and even if I
 could, this is probably something that doesn't need anything else
 than a warning that if you label your index functions immutable when
 they aren't, serializable transactions might not work.

When you tell the system an untruth about the state of the world, such
as alleging immutability when it's not actually there, it will get its
revenge in ways more drastic than this.

Cheers,
David.
-- 
David Fetter da...@fetter.org http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

-- 
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] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.

2011-01-17 Thread Robert Haas
On Mon, Jan 17, 2011 at 9:41 AM, Peter Eisentraut pete...@gmx.net wrote:
 On fre, 2011-01-14 at 18:45 -0500, Robert Haas wrote:
 On Fri, Jan 14, 2011 at 3:45 PM, Marti Raudsepp ma...@juffo.org
 wrote:
  There's a similar case with CREATE TABLE IF NOT EXISTS, maybe this
 is
  worth covering in an updated patch too?
  And if I change that, people might expect the same from DROP X IF
 EXISTS too?

 It's far less clear what you'd change those cases to say, and they
 already emit a NOTICE, so it seems unnecessary.

 Maybe instead of the proposed patch, a notice could be added:

 NOTICE: existing object was replaced

Well, that would eliminate the backward-compatibility hazard, pretty
much, but it seems noisy.  I already find some of these notices to be
unduly informative.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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


Re: [HACKERS] Replication logging

2011-01-17 Thread Magnus Hagander
On Mon, Jan 17, 2011 at 16:03, Robert Haas robertmh...@gmail.com wrote:
 On Mon, Jan 17, 2011 at 8:58 AM, Magnus Hagander mag...@hagander.net wrote:
 What do you have in mind?

 Either having it controlled by log_connections, or perhaps have a
 log_highpriv_connections that controls replication *and* superuser, to
 be somewhat consistent.

 -1.  We could provide an option to turn this on and off, but I
 wouldn't want it merged with log_connections or logging of superuser
 connections.

 Fair enough, we could have a log_replication_connections as a separate
 one then? Or having one log_connections, one
 log_replication_connections and one log_superuser_connections?

 log_replication_connections seems reasonable.  Not sure about
 log_superuser_connections.

So basically like this (see attachment).


 Incidentally, I think ClientAuthentication_hook is sufficiently
 powerful to allow logging of superuser connections but no others, if
 someone wanted to write a contrib module.  That doesn't necessarily
 mean an in-core facility wouldn't be useful too, but it's at least
 worth thinking about using the hook.

 Do we have an example of this hook somewhere already? If not, it could
 be made into a useful example of that, perhaps?

 contrib/auth_delay

Hmm, ok, so not that then :-)


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 8e2a2c5..5f83adf 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3380,6 +3380,21 @@ local0.*/var/log/postgresql
   /listitem
  /varlistentry
 
+ varlistentry id=guc-log-replication-connections xreflabel=log_replication_connections
+  termvarnamelog_replication_connections/varname (typeboolean/type)/term
+  indexterm
+   primaryvarnamelog_replication_connections/ configuration parameter/primary
+  /indexterm
+  listitem
+   para
+Causes each successful replication connection to the server to be
+logged. This parameter can only be set in the
+filenamepostgresql.conf/ file or on the server command line.
+The default is on.
+   /para
+  /listitem
+ /varlistentry
+
  varlistentry id=guc-log-disconnections xreflabel=log_disconnections
   termvarnamelog_disconnections/varname (typeboolean/type)/term
   indexterm
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 179048f..a128e21 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -199,6 +199,7 @@ int			AuthenticationTimeout = 60;
 
 bool		log_hostname;		/* for ps display and logging */
 bool		Log_connections = false;
+bool		Log_replication_connections = false;
 bool		Db_user_namespace = false;
 
 bool		enable_bonjour = false;
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index b227e6c..3c941b1 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3137,6 +3137,7 @@ set_debug_options(int debug_flag, GucContext context, GucSource source)
 	if (debug_flag = 1  context == PGC_POSTMASTER)
 	{
 		SetConfigOption(log_connections, true, context, source);
+		SetConfigOption(log_replication_connections, true, context, source);
 		SetConfigOption(log_disconnections, true, context, source);
 	}
 	if (debug_flag = 2)
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 76890f2..d09633e 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -219,22 +219,25 @@ PerformAuthentication(Port *port)
 		elog(FATAL, could not disable timer for authorization timeout);
 
 	/*
-	 * Log connection for streaming replication even if Log_connections
-	 * disabled.
+	 * Log connection for streaming replication if Log_replication_connections
+	 * is enabled.
 	 */
 	if (am_walsender)
 	{
-		if (port-remote_port[0])
-			ereport(LOG,
-	(errmsg(replication connection authorized: user=%s host=%s port=%s,
-			port-user_name,
-			port-remote_host,
-			port-remote_port)));
-		else
-			ereport(LOG,
-(errmsg(replication connection authorized: user=%s host=%s,
-		port-user_name,
-		port-remote_host)));
+		if (Log_replication_connections)
+		{
+			if (port-remote_port[0])
+ereport(LOG,
+		(errmsg(replication connection authorized: user=%s host=%s port=%s,
+port-user_name,
+port-remote_host,
+port-remote_port)));
+			else
+ereport(LOG,
+		(errmsg(replication connection authorized: user=%s host=%s,
+port-user_name,
+port-remote_host)));
+		}
 	}
 	else if (Log_connections)
 		ereport(LOG,
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index e4dea31..94c58a4 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -803,6 +803,14 @@ static struct config_bool ConfigureNamesBool[] =
 		false, NULL, NULL
 	},
 	{
+		

Re: [HACKERS] pg_basebackup for streaming base backups

2011-01-17 Thread Robert Haas
On Mon, Jan 17, 2011 at 8:55 AM, Magnus Hagander mag...@hagander.net wrote:
 Hmm. I don't like those names at all :(

I agree.  I don't think your original names are bad, as long as
they're well-documented.  I sympathize with Simon's desire to make it
clear that these use the replication framework, but I really don't
want the command names to be that long.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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


Re: [HACKERS] pg_basebackup for streaming base backups

2011-01-17 Thread Magnus Hagander
On Mon, Jan 17, 2011 at 16:18, Robert Haas robertmh...@gmail.com wrote:
 On Mon, Jan 17, 2011 at 8:55 AM, Magnus Hagander mag...@hagander.net wrote:
 Hmm. I don't like those names at all :(

 I agree.  I don't think your original names are bad, as long as
 they're well-documented.  I sympathize with Simon's desire to make it
 clear that these use the replication framework, but I really don't
 want the command names to be that long.

Actually, after some IM chats, I think pg_streamrecv should be
renamed, probably to pg_walstream (or pg_logstream, but pg_walstream
is a lot more specific than 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] REVIEW: Extensions support for pg_dump

2011-01-17 Thread Anssi Kääriäinen
I used the patch from CommitFest application and applied the following 
commit to fix a known issue:


http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=commitdiff;h=d4991d35283ae0ceeb7f9e4203cf6a9dfb5d128d

Is the patch in context diff format?
Yes.

Does the patch apply cleanly?
No:
patching file src/include/commands/defrem.h
Hunk #2 succeeded at 107 with fuzz 2 (offset 27 lines).
Hunk #3 FAILED at 105.
1 out of 5 hunks FAILED -- saving rejects to file 
src/include/commands/defrem.h.rej


I have used the git head of

http://git.postgresql.org/gitweb?p=postgresql-extension.git branch extensions

to do the rest of reviewing. There is one compiler warning:

gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith

-Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing
-fwrapv -g -I../../../src/interfaces/libpq -I../../../src/include -D_GNU_
SOURCE   -c -o pg_dump.o pg_dump.c
pg_dump.c: In function ‘getTables’:
pg_dump.c:3748: warning: too many arguments for format


And, make check gives me the following errors:
test sanity_check ... FAILED
test rules... FAILED

Does it include reasonable tests, necessary doc patches, etc?

There is enough documentation, but I think the documentation needs some 
polishing. I am not a native English speaker, so it might be it is my 
English that is failing. The data is there, but the representation might 
be a little off.


I didn't spot any tests. This could be that I don't know what to look for...

Usability review:

The patch implements a way to create extensions. While the patch is 
labeled extensions support for pg_dump, it actually implements more. It 
implements a new way to package and install extension, and changes 
contrib extensions to use the new way.


I do think we want these features, and that we do not have those 
already. As far as I understand, there is nothing in the standard 
regarding this feature.


I wonder if the structure of having all the extensions in share/contrib/ 
is a good idea. It might be better if one could separate the contrib 
extensions in one place, and user created extensions in another place. 
This could make it easy to see what user created extensions is installed 
in (or installable to) the cluster. I think this might be helpful to 
DBAs upgrading PostgreSQL.


Overall, the system seems intuitive to use. It is relatively easy to 
create extension using this feature, and loading contrib extensions is 
really easy. I haven't tested how easy it is to create C-language 
extensions using this. If I am not mistaken it just adds the compile  
install the C-functions before installing the extension.


Feature test:

The packaging feature works as advertised, expect for the following bugs 
and inconsistencies.


When using the plain CREATE EXTENSION foobar command without schema 
qualifier, the extension is created in schema public (by name) without 
regard to the current search path. This is inconsistent with create 
table, and if the public schema is renamed, the command gives error:


ERROR: schema public does not exist

This makes the name public have a special meaning, and I suspect that 
is not wanted.


When issuing CREATE EXTENSION foo SCHEMA bar, and the extension foo is 
not relocatable and it's control file uses the SET search_path TO 
@extschema@, the search_path is set to bar for the session. That is, it 
is not changed to the original search_path after the command has completed.


When trying to load extensions which contain identical signatured 
functions, if the loading will succeed is dependent on the search path. 
If there is a conflicting function in search path (first extension 
loaded to schema public), then the second extension load will fail. But 
if the order is reversed (first extension loaded to schema foo which is 
not in search path), then the second extension can be loaded to the 
public schema.


While it is not possible to drop functions in extensions, it is possible 
to rename a function, and also to CREATE OR REPLACE a function in an 
extension. After renaming or CORing a function, it is possible to drop 
the function. I also wonder if alter function ... set parameter should 
be allowed?


There is no validation for the extension names in share/contrib/. It is 
possible to have extensions control files named .control, 
.name.control, name''.control etc. While it is stupid to name them 
so, it might be better to give an explicit warning / error in these 
cases. It is of course possible to use these extensions.


If there is no comment for a given extension in the .control file, then 
\dx will not show the extension installed even if it is installed.


I was able to make it crash:

postgres=# alter extension foo.bar set schema baz;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

Log contains this:
TRAP: 

Re: [HACKERS] pg_basebackup for streaming base backups

2011-01-17 Thread Simon Riggs
On Mon, 2011-01-17 at 16:20 +0100, Magnus Hagander wrote:
 On Mon, Jan 17, 2011 at 16:18, Robert Haas robertmh...@gmail.com wrote:
  On Mon, Jan 17, 2011 at 8:55 AM, Magnus Hagander mag...@hagander.net 
  wrote:
  Hmm. I don't like those names at all :(
 
  I agree.  I don't think your original names are bad, as long as
  they're well-documented.  I sympathize with Simon's desire to make it
  clear that these use the replication framework, but I really don't
  want the command names to be that long.
 
 Actually, after some IM chats, I think pg_streamrecv should be
 renamed, probably to pg_walstream (or pg_logstream, but pg_walstream
 is a lot more specific than that)

pg_stream_log
pg_stream_backup
?

-- 
 Simon Riggs   http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and 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] texteq/byteaeq: avoid detoast [REVIEW]

2011-01-17 Thread Noah Misch
On Mon, Jan 17, 2011 at 07:35:52AM +0100, Magnus Hagander wrote:
 On Mon, Jan 17, 2011 at 06:51, Itagaki Takahiro
 itagaki.takah...@gmail.com wrote:
  On Mon, Jan 17, 2011 at 04:05, Andy Colson a...@squeakycode.net wrote:
  This is a review of:
  https://commitfest.postgresql.org/action/patch_view?id=468
 
  Purpose:
  
  Equal and not-equal _may_ be quickly determined if their lengths are
  different. ? This _may_ be a huge speed up if we don't have to detoast.
 
  We can skip detoast to compare lengths of two text/bytea values
  with the patch, but we still need detoast to compare the contents
  of the values.
 
  If we always generate same toasted byte sequences from the same raw
  values, we don't need to detoast at all to compare the contents.
  Is it possible or not?
 
 For bytea, it seems it would be possible.
 
 For text, I think locales may make that impossible. Aren't there
 locale rules where two different characters can behave the same when
 comparing them? I know in Swedish at least w and v behave the same
 when sorting (but not when comparing) in some variants of the locale.
 
 In fact, aren't there cases where the *length test* also fails? I
 don't know this for sure, but unless we know for certain that two
 different length strings can never be the same *independent of
 locale*, this whole patch has a big problem...

Just to be clear, the code already has these length tests today.  This patch
just moves them before the detoast.

-- 
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] texteq/byteaeq: avoid detoast [REVIEW]

2011-01-17 Thread Noah Misch
On Mon, Jan 17, 2011 at 11:05:09AM +0100, Magnus Hagander wrote:
 On Mon, Jan 17, 2011 at 09:13, Itagaki Takahiro
 itagaki.takah...@gmail.com wrote:
  2011/1/17 KaiGai Kohei kai...@ak.jp.nec.com:
  Are you talking about an idea to apply toast id as an alternative key?
 
  No, probably. I'm just talking about whether diff -q A.txt B.txt and
  diff -q A.gz ?B.gz always returns the same result or not.

Interesting.

  ... I found it depends on version of gzip. So, if we use such logic,
  we cannot improve toast compression logic because the data is migrated
  by pg_upgrade.
 
 Yeah, that might be a bad tradeoff.
 
 I wonder if we can trust the *equality* test, but not the inequality?
 E.g. if compressed(A) == compressed(B) we know they're the same, but
 if compressed(A) != compressed(B) we don't know they're not they still
 might be.

Exactly.

 I guess with two different versions or even completely different
 algorithms we could end up with exactly the same compressed value for
 different plaintexts (it's not a cryptographic hash after all), so
 that's probably not an acceptable comparison either.

It's safe to assume that will never happen.  If compressed(A) == compressed(B)
when A != B, we would have a lossy compression algorithm.

As you say, though, _inequality_ implies nothing for an arbitrary decompressor.
One can trivially construct many inputs to the zlib decompressor that yield the
same output.  gzip -1 ... gzip -9 do this, for example.  So the main win
here would come if we tightly controlled the compressor, such that we could
infer something from compressed(A) != compressed(B).  That would be an
intriguing path to explore.

nm

-- 
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] Replication logging

2011-01-17 Thread Robert Haas
On Mon, Jan 17, 2011 at 10:12 AM, Magnus Hagander mag...@hagander.net wrote:
 On Mon, Jan 17, 2011 at 16:03, Robert Haas robertmh...@gmail.com wrote:
 On Mon, Jan 17, 2011 at 8:58 AM, Magnus Hagander mag...@hagander.net wrote:
 What do you have in mind?

 Either having it controlled by log_connections, or perhaps have a
 log_highpriv_connections that controls replication *and* superuser, to
 be somewhat consistent.

 -1.  We could provide an option to turn this on and off, but I
 wouldn't want it merged with log_connections or logging of superuser
 connections.

 Fair enough, we could have a log_replication_connections as a separate
 one then? Or having one log_connections, one
 log_replication_connections and one log_superuser_connections?

 log_replication_connections seems reasonable.  Not sure about
 log_superuser_connections.

 So basically like this (see attachment).

Yeah.  Although maybe we should take this opportunity to eliminate the
funky capitalization of Log_connections.

 Do we have an example of this hook somewhere already? If not, it could
 be made into a useful example of that, perhaps?

 contrib/auth_delay

 Hmm, ok, so not that then :-)

Doesn't preclude this.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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


Re: [HACKERS] Replication logging

2011-01-17 Thread Magnus Hagander
On Mon, Jan 17, 2011 at 16:31, Robert Haas robertmh...@gmail.com wrote:
 On Mon, Jan 17, 2011 at 10:12 AM, Magnus Hagander mag...@hagander.net wrote:
 On Mon, Jan 17, 2011 at 16:03, Robert Haas robertmh...@gmail.com wrote:
 On Mon, Jan 17, 2011 at 8:58 AM, Magnus Hagander mag...@hagander.net 
 wrote:
 What do you have in mind?

 Either having it controlled by log_connections, or perhaps have a
 log_highpriv_connections that controls replication *and* superuser, to
 be somewhat consistent.

 -1.  We could provide an option to turn this on and off, but I
 wouldn't want it merged with log_connections or logging of superuser
 connections.

 Fair enough, we could have a log_replication_connections as a separate
 one then? Or having one log_connections, one
 log_replication_connections and one log_superuser_connections?

 log_replication_connections seems reasonable.  Not sure about
 log_superuser_connections.

 So basically like this (see attachment).

 Yeah.  Although maybe we should take this opportunity to eliminate the
 funky capitalization of Log_connections.

We have that on several other variables as well, I'd rather see that
as a separate thing to change. But is it worth it, wrt it breaking
back-patchability?

Before I go ahead and commit the part that adds
log_replication_connections, anybody else want to object to the idea?
;)


 Do we have an example of this hook somewhere already? If not, it could
 be made into a useful example of that, perhaps?

 contrib/auth_delay

 Hmm, ok, so not that then :-)

 Doesn't preclude this.

Nope, but also doesn't make it the second reason to do it :-) I don't
personally have the itch to go write it, but if somebody does I can
always volunteer to review it...

-- 
 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] REVIEW: Extensions support for pg_dump

2011-01-17 Thread Alvaro Herrera
Excerpts from Anssi Kääriäinen's message of lun ene 17 12:41:25 -0300 2011:

 While it is not possible to drop functions in extensions, it is possible 
 to rename a function, and also to CREATE OR REPLACE a function in an 
 extension. After renaming or CORing a function, it is possible to drop 
 the function.

Hmm, this seems a serious problem.  I imagine that what's going on is
that the function cannot be dropped because the extension depends on it;
but after the rename, the dependencies of the function are dropped and
recreated, but the dependency that relates it to the extension is
forgotten.

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


[HACKERS] test_fsync open_sync test

2011-01-17 Thread Bruce Momjian
Is there a value to this test_fsync test?

Compare open_sync with different sizes:
(This is designed to compare the cost of one large
sync'ed write and two smaller sync'ed writes.)
open_sync 16k write   242.563 ops/sec
2 open_sync 8k writes 752.752 ops/sec

It compares the cost of doing larger vs. two smaller open_sync writes.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

-- 
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] REVIEW: Extensions support for pg_dump

2011-01-17 Thread Robert Haas
On Mon, Jan 17, 2011 at 10:41 AM, Anssi Kääriäinen
anssi.kaariai...@thl.fi wrote:
 I haven't had time to review the pg_dump part of the patch yet, will do that
 next (tomorrow). I hope it is OK to post a partial review...

It is, and this is a very good and detailed review!

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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


[HACKERS] Moving test_fsync to /contrib?

2011-01-17 Thread Bruce Momjian
Is there value in moving test_fsync to /contrib?

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

-- 
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] Moving test_fsync to /contrib?

2011-01-17 Thread Robert Haas
On Mon, Jan 17, 2011 at 11:02 AM, Bruce Momjian br...@momjian.us wrote:
 Is there value in moving test_fsync to /contrib?

Why would we want to do that?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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


Re: [HACKERS] Moving test_fsync to /contrib?

2011-01-17 Thread Bruce Momjian
Robert Haas wrote:
 On Mon, Jan 17, 2011 at 11:02 AM, Bruce Momjian br...@momjian.us wrote:
  Is there value in moving test_fsync to /contrib?
 
 Why would we want to do that?

If we expect users to run the tool to best choose the best
wal_sync_method.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

-- 
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] Spread checkpoint sync

2011-01-17 Thread Bruce Momjian
Greg Smith wrote:
 One of the components to the write queue is some notion that writes that 
 have been waiting longest should eventually be flushed out.  Linux has 
 this number called dirty_expire_centiseconds which suggests it enforces 
 just that, set to a default of 30 seconds.  This is why some 5-minute 
 interval checkpoints with default parameters, effectively spreading the 
 checkpoint over 2.5 minutes, can work under the current design.  
 Anything you wrote at T+0 to T+2:00 *should* have been written out 
 already when you reach T+2:30 and sync.  Unfortunately, when the system 
 gets busy, there is this congestion control logic that basically 
 throws out any guarantee of writes starting shortly after the expiration 
 time.

Should we be writing until 2:30 then sleep 30 seconds and fsync at 3:00?

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

-- 
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] Moving test_fsync to /contrib?

2011-01-17 Thread Robert Haas
On Mon, Jan 17, 2011 at 11:16 AM, Bruce Momjian br...@momjian.us wrote:
 Robert Haas wrote:
 On Mon, Jan 17, 2011 at 11:02 AM, Bruce Momjian br...@momjian.us wrote:
  Is there value in moving test_fsync to /contrib?

 Why would we want to do that?

 If we expect users to run the tool to best choose the best
 wal_sync_method.

I don't see how moving it from src/tools to contrib is going to make
that happen more often.  src/tools to src/bin might have that effect.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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


Re: [HACKERS] Spread checkpoint sync

2011-01-17 Thread Jeff Janes
On Sun, Jan 16, 2011 at 7:13 PM, Greg Smith g...@2ndquadrant.com wrote:
 I have finished a first run of benchmarking the current 9.1 code at various
 sizes.  See http://www.2ndquadrant.us/pgbench-results/index.htm for many
 details.  The interesting stuff is in Test Set 3, near the bottom.  That's
 the first one that includes buffer_backend_fsync data.  This iall on ext3 so
 far, but is using a newer 2.6.32 kernel, the one from Ubuntu 10.04.

 The results are classic Linux in 2010:  latency pauses from checkpoint sync
 will easily leave the system at a dead halt for a minute, with the worst one
 observed this time dropping still for 108 seconds.  That one is weird, but
 these two are completely averge cases:

 http://www.2ndquadrant.us/pgbench-results/210/index.html
 http://www.2ndquadrant.us/pgbench-results/215/index.html

 I think a helpful next step here would be to put Robert's fsync compaction
 patch into here and see if that helps.  There are enough backend syncs
 showing up in the difficult workloads (scale=1000, clients =32) that its
 impact should be obvious.

Have you ever tested Robert's other idea of having a metronome process
do a periodic fsync on a dummy file which is located on the same ext3fs
as the table files?  I think that that would be interesting to see.

Cheers,

Jeff

-- 
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] Moving test_fsync to /contrib?

2011-01-17 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Mon, Jan 17, 2011 at 11:02 AM, Bruce Momjian br...@momjian.us wrote:
 Is there value in moving test_fsync to /contrib?

 Why would we want to do that?

So it would be built by default, installed under reasonable conditions,
and there would be a place to document it.  Where it is, it's not a
user-facing thing at all.

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] Add support for logging the current role

2011-01-17 Thread Alvaro Herrera
Excerpts from Tom Lane's message of sáb ene 15 00:34:40 -0300 2011:
 Stephen Frost sfr...@snowman.net writes:

  What about something other than
  version_x_y?  I could maybe see having a 'default' and an 'all'
  instead..  Then have the default be what we have currently and 'all' be
  the full list I'm thinking about.
 
 If default always means what it means today, I can live with that.
 But if the meaning of all changes from version to version, that seems
 like a royal mess.  Again, I'm concerned that an external tool like
 pgfouine be able to make sense of the value without too much context.
 If it doesn't know what some of the columns are, it can just ignore them
 --- but a magic summary identifier that it doesn't understand at all is
 a problem.

Maybe if we offered a way for the utility to find out the field list
from the magic identifier, it would be enough.

(It would be neat to have magic identifiers for terse, verbose,
etc, that mimicked the behavior of client processing)

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 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] Replication logging

2011-01-17 Thread Tom Lane
Magnus Hagander mag...@hagander.net writes:
 Before I go ahead and commit the part that adds
 log_replication_connections, anybody else want to object to the idea?

I think it'd make more sense just to say that replication connections
are subject to the same log_connections rule as others.  An extra GUC
for this is surely overkill.

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] SSPI client authentication in non-Windows builds

2011-01-17 Thread Christian Ullrich

Magnus Hagander wrote:

On Mon, Jan 3, 2011 at 14:11, Christian Ullrichch...@chrullrich.net  wrote:

Hello all,

this patch adds support for connecting to servers running on Windows
and requesting SSPI authentication. It does this by treating
AUTH_REQ_SSPI the same as AUTH_REQ_GSS if no native SSPI support is
available.

In addition to being generally useful, this is a workaround to a
problem with MIT KfW that I encountered back in September 2010 [1].

This change has been tested and works correctly on FreeBSD 8.1, using
the Kerberos and GSSAPI libraries from Heimdal 1.4. The server is
running PostgreSQL 9.0.2 on Windows 2008.

Does this require some certain minimum version of the kerberos
libraries? Do you know if it works with just Heimdal or both Heimdal
and MIT?
All it does ist GSSAPI auth, which means that it should work in any 
environment where GSSAPI auth against a GSSAPI implementation that calls 
itself that would work (because that part of SSPI is actually designed 
for interoperability). As for reality, I'm afraid I don't know whether 
it works with anything but the configuration I mentioned. I will do some 
more testing this week, but I'm limited in the number of combinations I 
can try; some randomly chosen Linux distributions with whatever Kerberos 
they ship and the Heimdal from the FreeBSD 8 base system instead of the 
port (if I can get PostgreSQL to build with that) against Windows 2003 
and 2008 is probably going to be all I can offer. Expect a report early 
next week.


You can find some more information at 
http://msdn.microsoft.com/en-us/library/aa380496(v=VS.85).aspx 
http://msdn.microsoft.com/en-us/library/aa380496%28v=VS.85%29.aspx.

What I'm after is: do we need a autoconf check, or a #ifdef, or
something to make sure we don't enable it in a scenario where it won't
work?

Enabling it unconditionally (that is, conditional on --with-gssapi) 
would mean that, instead of SSPI authentication unsupported, the user 
would get either success or authentication failure. Some may consider 
the latter a regression in terms of user experience; I don't really agree.


The patch does not add any additional risk of build failure, because the 
GSSAPI client code will always be compiled if enabled, and all the patch 
really does is move a case label.


--
Christian


--
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] Moving test_fsync to /contrib?

2011-01-17 Thread Bruce Momjian
Tom Lane wrote:
 Robert Haas robertmh...@gmail.com writes:
  On Mon, Jan 17, 2011 at 11:02 AM, Bruce Momjian br...@momjian.us wrote:
  Is there value in moving test_fsync to /contrib?
 
  Why would we want to do that?
 
 So it would be built by default, installed under reasonable conditions,
 and there would be a place to document it.  Where it is, it's not a
 user-facing thing at all.

I have cleaned up the code so it is reasonable to ship and use by
end-users.  It is documented already where we mention setting
wal_sync_method, but having it in src/tools really is a hinderance.

It seems like /contrib would be more natural, no?  /bin seems like
overkill because most people will not want to run it.  Most of /contrib
is installed already by installers, I think.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

-- 
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] Moving test_fsync to /contrib?

2011-01-17 Thread Robert Haas
On Mon, Jan 17, 2011 at 11:47 AM, Bruce Momjian br...@momjian.us wrote:
 Tom Lane wrote:
 Robert Haas robertmh...@gmail.com writes:
  On Mon, Jan 17, 2011 at 11:02 AM, Bruce Momjian br...@momjian.us wrote:
  Is there value in moving test_fsync to /contrib?

  Why would we want to do that?

 So it would be built by default, installed under reasonable conditions,
 and there would be a place to document it.  Where it is, it's not a
 user-facing thing at all.

 I have cleaned up the code so it is reasonable to ship and use by
 end-users.  It is documented already where we mention setting
 wal_sync_method, but having it in src/tools really is a hinderance.

 It seems like /contrib would be more natural, no?  /bin seems like
 overkill because most people will not want to run it.  Most of /contrib
 is installed already by installers, I think.

At least on Red Hat, it is packaged separately.  So if you install
postgresql-server and postgresql-client you will not get things that
are only in contrib.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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


Re: [HACKERS] Add support for logging the current role

2011-01-17 Thread Andrew Dunstan



On 01/17/2011 11:44 AM, Alvaro Herrera wrote:

Excerpts from Tom Lane's message of sáb ene 15 00:34:40 -0300 2011:

Stephen Frostsfr...@snowman.net  writes:

What about something other than
version_x_y?  I could maybe see having a 'default' and an 'all'
instead..  Then have the default be what we have currently and 'all' be
the full list I'm thinking about.

If default always means what it means today, I can live with that.
But if the meaning of all changes from version to version, that seems
like a royal mess.  Again, I'm concerned that an external tool like
pgfouine be able to make sense of the value without too much context.
If it doesn't know what some of the columns are, it can just ignore them
--- but a magic summary identifier that it doesn't understand at all is
a problem.

Maybe if we offered a way for the utility to find out the field list
from the magic identifier, it would be enough.

(It would be neat to have magic identifiers for terse, verbose,
etc, that mimicked the behavior of client processing)



Just output a header line with the column names. We've long been able to 
import such files. If the list of columns changes we should rotate log 
files before outputting the new format. That might get a little tricky 
to coordinate between backends.


cheers

andrew


--
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] Replication logging

2011-01-17 Thread Magnus Hagander
On Mon, Jan 17, 2011 at 17:46, Tom Lane t...@sss.pgh.pa.us wrote:
 Magnus Hagander mag...@hagander.net writes:
 Before I go ahead and commit the part that adds
 log_replication_connections, anybody else want to object to the idea?

 I think it'd make more sense just to say that replication connections
 are subject to the same log_connections rule as others.  An extra GUC
 for this is surely overkill.

I thought so, but Robert didn't agree. And given that things are the
way they are, clearly somebody else didn't agree as well - though I've
been unable to locate the original discussion if there was one.


-- 
 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] Moving test_fsync to /contrib?

2011-01-17 Thread Alvaro Herrera
Excerpts from Bruce Momjian's message of lun ene 17 13:47:40 -0300 2011:

 It seems like /contrib would be more natural, no?  /bin seems like
 overkill because most people will not want to run it.  Most of /contrib
 is installed already by installers, I think.

I don't understand why it would be overkill.  Are you saying people
would complain because you installed a 25 kB executable that they might
not want to use?  Just for fun I checked /usr/bin and noticed that I
have a pandoc executable, weighing 17 MB, that I have never used and I
have no idea what is it for.

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 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] Moving test_fsync to /contrib?

2011-01-17 Thread Cédric Villemain
2011/1/17 Bruce Momjian br...@momjian.us:
 Robert Haas wrote:
 On Mon, Jan 17, 2011 at 11:02 AM, Bruce Momjian br...@momjian.us wrote:
  Is there value in moving test_fsync to /contrib?

 Why would we want to do that?

 If we expect users to run the tool to best choose the best
 wal_sync_method.

+1  to move it to contrib/


 --
  Bruce Momjian  br...@momjian.us        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +

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




-- 
Cédric Villemain               2ndQuadrant
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] Moving test_fsync to /contrib?

2011-01-17 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Mon, Jan 17, 2011 at 11:47 AM, Bruce Momjian br...@momjian.us wrote:
 It seems like /contrib would be more natural, no?  /bin seems like
 overkill because most people will not want to run it.  Most of /contrib
 is installed already by installers, I think.

 At least on Red Hat, it is packaged separately.

On Red Hat, it is not packaged at all (at least not by me), and won't
be unless it goes into contrib.  I don't believe it belongs in the
base package.

Also, it's not going to get packaged at all unless it gets renamed to
something less generic, maybe pg_test_fsync; I'm not going to risk the
oppobrium of sticking something named test_fsync into /usr/bin.
Moving to contrib would be a good opportunity to fix the name.

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] Moving test_fsync to /contrib?

2011-01-17 Thread Robert Haas
On Mon, Jan 17, 2011 at 12:05 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Mon, Jan 17, 2011 at 11:47 AM, Bruce Momjian br...@momjian.us wrote:
 It seems like /contrib would be more natural, no?  /bin seems like
 overkill because most people will not want to run it.  Most of /contrib
 is installed already by installers, I think.

 At least on Red Hat, it is packaged separately.

 On Red Hat, it is not packaged at all (at least not by me), and won't
 be unless it goes into contrib.  I don't believe it belongs in the
 base package.

I confess to some confusion about what things belong where.  Is
contrib the right place for this because we think it's half-baked, or
because we think most people won't use it, or just because we're
violently allergic to adding stuff to src/bin, or what?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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


Re: [HACKERS] pg_basebackup for streaming base backups

2011-01-17 Thread Dimitri Fontaine
Magnus Hagander mag...@hagander.net writes:
 The walsender can't read pg_class for example, so it can't generate
 that mapping file.

I don't see any way out here.  So let's call oid.tar good enough for now…

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] REVIEW: Extensions support for pg_dump

2011-01-17 Thread Dimitri Fontaine
Hi,

Thanks for your review!

Anssi Kääriäinen anssi.kaariai...@thl.fi writes:
 Does the patch apply cleanly?
 No:

That was some bitrot, has been fixed, thanks you for working from the
git repository meanwhile.

 pg_dump.c:3748: warning: too many arguments for format

Fixed in v25 already sent this morning.

 And, make check gives me the following errors:
 test sanity_check ... FAILED
 test rules... FAILED

Fixed too.  Was due to git conflict solving where it adds a new line in
the tests and keep the embedded rowcount the same.  I think.

 Does it include reasonable tests, necessary doc patches, etc?

 There is enough documentation, but I think the documentation needs some
 polishing. I am not a native English speaker, so it might be it is my
 English that is failing. The data is there, but the representation might be
 a little off.

We already made lots of improvements there thanks to David Wheeler
reviews in the previous commitfest (which shows up already), and I'll be
happy to continue improving as much as I can.  If all it needs is
english native review, I guess that'll be part of the usual marathon
Bruce runs every year there?

 I didn't spot any tests. This could be that I don't know what to look for...

make -C contrib installcheck will exercise CREATE EXTENSION for each
contrib module.

 Usability review:

 The patch implements a way to create extensions. While the patch is labeled
 extensions support for pg_dump, it actually implements more. It implements a
 new way to package and install extension, and changes contrib extensions to
 use the new way.

Well, all there's in the patch is infrastructure to be able to issue
those single lines in your dump :)

 I do think we want these features, and that we do not have those already. As
 far as I understand, there is nothing in the standard regarding this
 feature.

 I wonder if the structure of having all the extensions in share/contrib/ is
 a good idea. It might be better if one could separate the contrib extensions
 in one place, and user created extensions in another place. This could make
 it easy to see what user created extensions is installed in (or installable
 to) the cluster. I think this might be helpful to DBAs upgrading PostgreSQL.

It's always been this way and I though it wouldn't be in this patch
scope to re-organise things.  Also I think we should include the UPGRADE
needs when we discuss file system level layout.

 Overall, the system seems intuitive to use. It is relatively easy to create
 extension using this feature, and loading contrib extensions is really
 easy. I haven't tested how easy it is to create C-language extensions using
 this. If I am not mistaken it just adds the compile  install the
 C-functions before installing the extension.

It's using PGXS which existed before, all you have to do that's new is
care about the Makefile EXTENSION variable and the control file.  Even
when doing C coded extension work.

 When using the plain CREATE EXTENSION foobar command without schema
 qualifier, the extension is created in schema public (by name) without
 regard to the current search path. This is inconsistent with create table,
 and if the public schema is renamed, the command gives error:

 ERROR: schema public does not exist

 This makes the name public have a special meaning, and I suspect that is
 not wanted.

Fixed in git, thanks for reporting!

~:5490=# set client_min_messages to debug1;
SET
~:5490=# set search_path to utils;
SET
~:5490=# create extension unaccent;
DEBUG:  parse_extension_control_file(unaccent, 
'/home/dfontaine/pgsql/exts/share/contrib/unaccent.control')
DEBUG:  CreateExtensionStmt: with user data, schema = 'utils', encoding = ''
DEBUG:  Installing extension 'unaccent' from 
'/home/dfontaine/pgsql/exts/share/contrib/unaccent.sql', in schema utils, with 
user data
CREATE EXTENSION
~:5490=# \dx
 List of extensions
 Schema | Name  | Version  |   Description  
 
+---+--+-
 utils  | citext| 9.1devel | case-insensitive character string type
 utils  | hstore| 9.1devel | storing sets of key/value pairs
 utils  | int_aggregate | 9.1devel | integer aggregator and an enumerator 
(obsolete)
 utils  | lo| 9.1devel | managing Large Objects
 utils  | unaccent  | 9.1devel | text search dictionary that removes accents
(5 rows)

 When issuing CREATE EXTENSION foo SCHEMA bar, and the extension foo is not
 relocatable and it's control file uses the SET search_path TO @extschema@,
 the search_path is set to bar for the session. That is, it is not changed to
 the original search_path after the command has completed.

It used to work this way with \i, obviously.  Should the extension patch
care about that and how?  Do we want to RESET search_path or to manually
manage it like we do for the client_min_messages and 

Re: [HACKERS] REVIEW: Extensions support for pg_dump

2011-01-17 Thread Dimitri Fontaine
Alvaro Herrera alvhe...@commandprompt.com writes:

 Excerpts from Anssi Kääriäinen's message of lun ene 17 12:41:25 -0300 2011:

 While it is not possible to drop functions in extensions, it is possible 
 to rename a function, and also to CREATE OR REPLACE a function in an 
 extension. After renaming or CORing a function, it is possible to drop 
 the function.

 Hmm, this seems a serious problem.  I imagine that what's going on is
 that the function cannot be dropped because the extension depends on it;
 but after the rename, the dependencies of the function are dropped and
 recreated, but the dependency that relates it to the extension is
 forgotten.

Well I'm not seeing that here:

~:5490=# drop function utils.lo_manage_d();
ERROR:  cannot drop function utils.lo_manage_d() because extension lo requires 
it
HINT:  You can drop extension lo instead.

src/backend/commands/functioncmds.c

/* rename */
namestrcpy((procForm-proname), newname);
simple_heap_update(rel, tup-t_self, tup);
CatalogUpdateIndexes(rel, tup);

But here:

src/backend/catalog/pg_proc.c

/*
 * Create dependencies for the new function.  If we are updating an
 * existing function, first delete any existing pg_depend entries.
 * (However, since we are not changing ownership or permissions, the
 * shared dependencies do *not* need to change, and we leave them 
alone.)
 */
if (is_update)
deleteDependencyRecordsFor(ProcedureRelationId, retval);

[ ... adding all dependencies back ... ]

/* dependency on extension */
if (create_extension)
{
recordDependencyOn(myself, CreateExtensionAddress, 
DEPENDENCY_INTERNAL);
}

Will investigate some more later.
-- 
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] Include WAL in base backup

2011-01-17 Thread Dimitri Fontaine
Magnus Hagander mag...@hagander.net writes:
 But however we do it, it will be significantly more complex than just
 including the WAL. And I want to make sure we get *something* done in
 time for 9.1, and then improve upon it. If we can get the improvement
 into 9.1 that's great, but if not it will have to wait until 9.2 - and
 I don't want to leave us without anything for 9.1.

+1.  The only point I'm not clear on is the complexity, and I trust you
to cut off at the right point here… meanwhile, I'm still asking for this
little more until you say your plate's full :)

 Right. I did put both the base backup and the wal streaming in the
 same binary earlier, and the only shared code was the one to connect
 to the db. So I split them apart again. This is the reason to put them
 back together perhaps - or just have the ability for pg_basebackup to
 fork()+exec() the wal streamer.

That would be awesome.

Then pg_streamrecv could somehow accept options that make it suitable
for use as an archive command, connecting to your (still?) third-party
daemon?  At this point it'd be pg_walsender :)

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] Streaming base backups

2011-01-17 Thread Dimitri Fontaine
Magnus Hagander mag...@hagander.net writes:
 With pg_basebackup, you can set up streaming replication in what's
 basically a single command (run the base backup, copy i na
 recovery.conf file). In my first version I even had a switch that
 would create the recovery.conf file for you - should we bring that
 back?

+1.  Well, make it optional maybe?

 It does require you to set a reasonable wal_keep_segments, though,
 but that's really all you need to do on the master side.

Until we get integrated WAL streaming while the base backup is ongoing.
We don't know when that is (9.1 or future), but that's what we're aiming
to now, right?

 What Fujii-san unsuccessfully proposed was to have the master restore
 segments from the archive and stream them to clients, on request.  It
 was deemed better to have the slave obtain them from the archive
 directly.

 Did Fuji-san agreed on the conclusion?

 I can see the point of the mastering being able to do this, but it
 seems like a pretty narrow usecase, really. I think we invented
 wal_keep_segments partially to solve this problem in a neater way?

Well I still think that the easier setup we can offer here is to ship
with integrated libpq based archive and restore commands.  Those could
be bin/pg_walsender and bin/pg_walreceiver.  They would have some
switches to make them suitable for running in subprocesses of either the
base backup utility or the default libpq based archive daemon.

Again, all of that is not forcibly material for 9.1, despite having all
the pieces already coded and tested, mainly in Magnus hands.  But could
we get agreement about going this route?

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] walreceiver fallback_application_name

2011-01-17 Thread Dimitri Fontaine
Fujii Masao masao.fu...@gmail.com writes:
  http://www.postgresql.org/docs/9.0/interactive/runtime-config-wal.html#GUC-MAX-WAL-SENDERS

 +1 though I could not find the mention to walreceiver in the doc.

True, we already use wal sender, I should have said similar phrasing.

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] SSI patch version 12

2011-01-17 Thread Kevin Grittner
Anssi Kääriäinen wrote:
 
 I tried to break the version 11 of the patch (some of the work was
 against earlier versions). In total I have used a full work day
 just trying to break things, but haven't been able to find anything
 after version 8. I can verify that the partial index issue is
 fixed, and the count(*) performance is a lot better now.
 
Thanks for all your testing!
 
For the record, Dan found one more issue with the placement of our LW
locking statements which provided a very small window for one process
to see an uninitialized structure from another.  He found it by
uncommenting the '#define TEST_OLDSERXID' line (thereby forcing any
terminated transaction immediately into the SLRU logic) and letting
DBT-2 pound on it for a long time.  If anyone else is going to be
doing heavy stress testing, you might want to apply this fix:
 
http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=036eefe05ba58da6dff9e1cae766e565182f37be
 
The issue is sufficiently hard to hit that I didn't want to post a
whole new patch for it.  I've been considering it now that doc
changes are complete, but am waiting to see how the 2PC work is
going.
 
 One thing I have been thinking about is how does predicate locking
 indexes work when using functional indexes and functions marked as
 immutable but which really aren't. I don't know how predicate
 locking indexes works, so it might be that this is a non-issue.
 
As David pointed out, doing that is likely to cause you bigger
headaches than this; but yes, you can break serializability by
declaring a non-immutable function as immutable and then using it on
a permanent table.  Like David apparently is, I'm skeptical that this
merits specific mention in the docs; but I'm open to arguments to
the contrary -- particularly if they suggest what language to put
where.
 
-Kevin

-- 
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] Streaming base backups

2011-01-17 Thread Magnus Hagander
On Mon, Jan 17, 2011 at 11:18, Dimitri Fontaine dimi...@2ndquadrant.fr wrote:
 Magnus Hagander mag...@hagander.net writes:
 With pg_basebackup, you can set up streaming replication in what's
 basically a single command (run the base backup, copy i na
 recovery.conf file). In my first version I even had a switch that
 would create the recovery.conf file for you - should we bring that
 back?

 +1.  Well, make it optional maybe?

It has always been optional. Basically it just creates a recovery.conf file with
primary_conninfo=whatever pg_streamrecv was using
standby_mode=on


 It does require you to set a reasonable wal_keep_segments, though,
 but that's really all you need to do on the master side.

 Until we get integrated WAL streaming while the base backup is ongoing.
 We don't know when that is (9.1 or future), but that's what we're aiming
 to now, right?

Yeah, it does sound like a plan. But to still allow both - streaming
it in parallell will eat two connections, and I'm sure some people
might consider that a higher cost.


 What Fujii-san unsuccessfully proposed was to have the master restore
 segments from the archive and stream them to clients, on request.  It
 was deemed better to have the slave obtain them from the archive
 directly.

 Did Fuji-san agreed on the conclusion?

 I can see the point of the mastering being able to do this, but it
 seems like a pretty narrow usecase, really. I think we invented
 wal_keep_segments partially to solve this problem in a neater way?

 Well I still think that the easier setup we can offer here is to ship
 with integrated libpq based archive and restore commands.  Those could
 be bin/pg_walsender and bin/pg_walreceiver.  They would have some
 switches to make them suitable for running in subprocesses of either the
 base backup utility or the default libpq based archive daemon.

Not sure why they'd run as an archive command and not like now as a
replication client - but let's keep that out of this thread and in a
new one :)

-- 
 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] psql: Add \dL to show languages

2011-01-17 Thread Bruce Momjian
Peter Eisentraut wrote:
 On m?n, 2011-01-17 at 07:37 +0100, Magnus Hagander wrote:
   which, as Magnus points out, includes non-procedural languages (SQL).
  
   I think that list languages could be confusing to newcomers -- the
   very people who might be reading through the help output of psql for
   the first time -- who might suppose that languages has something to
   do with the character sets supported by PostgreSQL, and might not even
   be aware that a variety of procedural languages can be used inside the
   database.
  
   Fair point.
  
  Yeah. Procedural langauges may strictly be wrong, but people aren't
  likely to misunderstand it.
 
 The term procedural in this context originated with Oracle's PL/SQL,
 which is a procedural language extension to the non-procedural SQL
 language.  From this came PostgreSQL's PL/pgSQL, and that naming was
 then continued with PL/Tcl, at which point PL/$X lost its original
 meaning of procedural extension to the non-procedural language $X and
 meant more or less handler for writing PostgreSQL functions in language
 $X.
 
 Otherwise PL/Scheme will blow your mind. :)
 
 Think of procedural language as language for writing [PostgreSQL]
 procedures.  As was pointed out, it's also a useful convention for
 distinguishing this from other languages, such as message
 translations.

FYI, I always refer to them as server-side language, to distinguish them
from client-side languages.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

-- 
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] Moving test_fsync to /contrib?

2011-01-17 Thread Bruce Momjian
Tom Lane wrote:
 Robert Haas robertmh...@gmail.com writes:
  On Mon, Jan 17, 2011 at 11:47 AM, Bruce Momjian br...@momjian.us wrote:
  It seems like /contrib would be more natural, no? ?/bin seems like
  overkill because most people will not want to run it. ?Most of /contrib
  is installed already by installers, I think.
 
  At least on Red Hat, it is packaged separately.
 
 On Red Hat, it is not packaged at all (at least not by me), and won't
 be unless it goes into contrib.  I don't believe it belongs in the
 base package.
 
 Also, it's not going to get packaged at all unless it gets renamed to
 something less generic, maybe pg_test_fsync; I'm not going to risk the
 oppobrium of sticking something named test_fsync into /usr/bin.
 Moving to contrib would be a good opportunity to fix the name.

Agreed on the need for a name change.  /contrib or /bin are fine with
me.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

-- 
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] Moving test_fsync to /contrib?

2011-01-17 Thread Bruce Momjian
Robert Haas wrote:
 On Mon, Jan 17, 2011 at 12:05 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  Robert Haas robertmh...@gmail.com writes:
  On Mon, Jan 17, 2011 at 11:47 AM, Bruce Momjian br...@momjian.us wrote:
  It seems like /contrib would be more natural, no? ?/bin seems like
  overkill because most people will not want to run it. ?Most of /contrib
  is installed already by installers, I think.
 
  At least on Red Hat, it is packaged separately.
 
  On Red Hat, it is not packaged at all (at least not by me), and won't
  be unless it goes into contrib. ?I don't believe it belongs in the
  base package.
 
 I confess to some confusion about what things belong where.  Is
 contrib the right place for this because we think it's half-baked, or
 because we think most people won't use it, or just because we're
 violently allergic to adding stuff to src/bin, or what?

I was suggesting /contrib because it seems to be of limited usefulness. 
I assume people want pg_upgrade to stay in /contrib for the same reason.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

-- 
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] Moving test_fsync to /contrib?

2011-01-17 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Mon, Jan 17, 2011 at 12:05 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 On Red Hat, it is not packaged at all (at least not by me), and won't
 be unless it goes into contrib. I don't believe it belongs in the
 base package.

 I confess to some confusion about what things belong where.  Is
 contrib the right place for this because we think it's half-baked, or
 because we think most people won't use it, or just because we're
 violently allergic to adding stuff to src/bin, or what?

The first two, if you ask me.  And there's another point: I disagree
with the assumption that platform-specific packagings will or should
include test_fsync by default.  It'd be better for the packager to make
a platform-specific choice of default for the users.  I don't mind too
much putting it into a secondary subpackage such as postgresql-contrib,
but you won't be seeing it in postgresql-server.

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] READ ONLY fixes

2011-01-17 Thread Kevin Grittner
Jeff Janes  wrote:
 
 A review:
 
Thanks!  Very thorough!
 
 None of the issues I raise above are severe. Does that mean I
 should change the status to ready for committer?
 
I see that notion was endorsed by Robert, so I'll leave it alone for
now.  If a committer asks me to do something about any of those
issues (or others, of course), I'll happily do so.
 
-Kevin

-- 
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] Moving test_fsync to /contrib?

2011-01-17 Thread Tom Lane
Bruce Momjian br...@momjian.us writes:
 Robert Haas wrote:
 I confess to some confusion about what things belong where.

 I was suggesting /contrib because it seems to be of limited usefulness. 
 I assume people want pg_upgrade to stay in /contrib for the same reason.

pg_upgrade is a different issue, really.  I think it's in contrib
because we don't trust it fully and don't want to promise that it will
work in every single future release anyway.  But even if we moved it to
core, it will always be a special case for packagers: not only is it
not appropriate to put it in the base package, it's not useful to
package it at all unless you also provide a copy of a back-rev
postmaster.  So at least for my money it will always be part of its
own special subpackage postgresql-upgrade.  (BTW, I just recently got
round to making that work for Fedora.)

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] Moving test_fsync to /contrib?

2011-01-17 Thread Robert Haas
On Mon, Jan 17, 2011 at 12:48 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Mon, Jan 17, 2011 at 12:05 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 On Red Hat, it is not packaged at all (at least not by me), and won't
 be unless it goes into contrib. I don't believe it belongs in the
 base package.

 I confess to some confusion about what things belong where.  Is
 contrib the right place for this because we think it's half-baked, or
 because we think most people won't use it, or just because we're
 violently allergic to adding stuff to src/bin, or what?

 The first two, if you ask me.  And there's another point: I disagree
 with the assumption that platform-specific packagings will or should
 include test_fsync by default.  It'd be better for the packager to make
 a platform-specific choice of default for the users.  I don't mind too
 much putting it into a secondary subpackage such as postgresql-contrib,
 but you won't be seeing it in postgresql-server.

I see.  Well, that seems reasonable.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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


Re: [HACKERS] REVIEW: Extensions support for pg_dump

2011-01-17 Thread Kääriäinen Anssi
 Well I'm not seeing that here

I am not at work at the moment and I don't have the possibility to compile 
PostgreSQL on this computer, so the example here is from memory.

The issue I saw was this: assume you have an extension foo, containing one 
function, test().

CREATE EXTENSION foo;
DROP FUNCTION test();
-- restricted due to dependency

ALTER FUNCTION test() RENAME TO test2;
DROP FUNCTION test2();
-- not restricted!

The same can be done using CREATE OR REPLACE.

I hope this is not an error on my part. It is possible because I had a lot of 
schemas and my search_path might have been wrong...

 - Anssi
PS: Using web email client, I hope this comes out in somewhat sane format.
-- 
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] What happened to open_sync_without_odirect?

2011-01-17 Thread Bruce Momjian
Josh Berkus wrote:
 On 1/15/11 4:30 PM, Bruce Momjian wrote:
  Josh Berkus wrote:
  Last I remember, we were going to add this as an option.  But I don't
  see a patch in the queue.  Am I missing it?  Was I supposed to write it?
  
  I don't know, but let me add that I am confused how this would look to
  users.  In many cases, kernels don't even support O_DIRECT, so what
  would we do to specify this?  What about just auto-disabling O_DIRECT if
  the filesystem does not support it; maybe issue a log message about it.
 
 Yes, you *are* confused.  The problem isn't auto-disabling, we already
 do that.  The problem is *auto-enabling*; ages ago we made the
 assumption that if o_sync was supported, so was o_direct.  We've now
 found out that's not true on all platforms.
 
 Also, test results show that even when supported, o_direct isn't
 necessarily a win.  Hence, the additional fsync_method options.

I think it would be clear if we did not use o_direct for open_*sync, but
only for open_*sync_direct, so there was no auto-direct anything --- you
had to ask for it, and if we don't support it, you get an error.  Right
now people aren't sure what they are getting.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

-- 
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] Spread checkpoint sync

2011-01-17 Thread Greg Smith

Jeff Janes wrote:

Have you ever tested Robert's other idea of having a metronome process
do a periodic fsync on a dummy file which is located on the same ext3fs
as the table files?  I think that that would be interesting to see.
  


To be frank, I really don't care about fixing this behavior on ext3, 
especially in the context of that sort of hack.  That filesystem is not 
the future, it's not possible to ever really make it work right, and 
every minute spent on pandering to its limitations would be better spent 
elsewhere IMHO.  I'm starting with the ext3 benchmarks just to provide 
some proper context for the worst-case behavior people can see right 
now, and to make sure refactoring here doesn't make things worse on it.  
My target is same or slightly better on ext3, much better on XFS and ext4.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us
PostgreSQL 9.0 High Performance: http://www.2ndQuadrant.com/books


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


[HACKERS] Review: compact fsync request queue on overflow

2011-01-17 Thread Chris Browne
I have been taking a peek at the following commitfest item:
   https://commitfest.postgresql.org/action/patch_view?id=497

Submission:

- I had to trim a little off the end of the patch to apply it, but
  that's likely the fault of how I cut'n'pasted it. It applied cleanly
  against HEAD.

- I observe that it doesn't include any alterations to documentation
  or to regression tests.

Both aspects seem apropos, as the behaviour is entirely internal to
the backend. I wouldn't expect a GUC variable for this, or SQL
commands to control it.

Usability Review:

Does the patch actually implement that?

  - Establishes a hash table
  - Establishes skip slot array
  - Walks through all BGWriter requests
- Adds to hash table.

  (I observe that it wasn't all that obvious that hash_search()
  *adds* elements that are missing.  I got confused and went
  looking for hash_add() or similar.  It's permissible to say dumb
  Chris.)

- If it's a collision, then mark collision in skip slot array, and
  add to count

  - After the walk
- Clean up hash table
- If nothing found, clean up skip slot array, and return

- If collisions found, then clear them out.

  Question: Is there any further cleanup needed for the entries
  that got dropped out of BGWriterShmem-requests?  It seems
  not, but a leak seems conceivable.

Do we want that?

  Eliminating a bunch of fsync() calls that are already being
  induced by other backends seems like a good thing, yep.

Do we already have it?

  Evidently not!

Does it follow SQL spec, or the community-agreed behavior?

  That doesn't seem relevant; this is well outside the scope of
  what SQL spec should have to say.

Does it include pg_dump support (if applicable)?

  Definitely not applicable.

Are there dangers?

  Possibilities...

  - Mentioned in the patch is the possibility of processing the
set of requests in reverse order, which might in principle
reduce work.  But there is some danger of this changing
semantics, so that reversal is not done.

  - Concurrent access...

Is there anything that can write extra elements to
BGWriterShmem-requests while this is running?

I wonder if we need to have any sort of lock surrounding this?

Have all the bases been covered?

  It is a comparatively simple change, so I wouldn't think things
  are missing.

Feature test:

   - Compiled and ran regression test; no problems found.

   Need to do...
- Validate it works as advertised
  - Hook up pgbench
  - Turn on DEBUG1 level
  - Watch that compacted fsync request queue from %d entries to %d 
entries come up

  It was a little troublesome inducing it.  I did so by cutting
  shared memory to minimum (128kB).

  I'd regularly get entries like the following:  (Note that I
  changed the error level to WARNING to induce logging this without
  getting all sorts of other stuff).

CONTEXT:  writing block 1735 of relation base/11933/16396
WARNING:  compacted fsync request queue from 16 entries to 3 entries - lost 
[13] entries
CONTEXT:  writing block 14 of relation base/11933/16387
WARNING:  compacted fsync request queue from 16 entries to 3 entries - lost 
[13] entries
CONTEXT:  writing block 4 of relation base/11933/16387
WARNING:  compacted fsync request queue from 16 entries to 3 entries - lost 
[13] entries
CONTEXT:  writing block 6 of relation base/11933/16387
WARNING:  compacted fsync request queue from 16 entries to 3 entries - lost 
[13] entries
CONTEXT:  writing block 1625 of relation base/11933/16396
WARNING:  compacted fsync request queue from 16 entries to 4 entries - lost 
[12] entries
CONTEXT:  writing block 880 of relation base/11933/16396
WARNING:  compacted fsync request queue from 16 entries to 4 entries - lost 
[12] entries
CONTEXT:  writing block 133 of relation base/11933/16396

  With higher shared memory, I couldn't readily induce compaction,
  which is probably a concurrency matter of not having enough volume
  of concurrent work going on.

- Corner cases?

  It's already a corner case ;-).

- Assertion failures?

  None seen thus far.

Performance test

- Does it slow down simple cases?

  It surely shouldn't; compaction is only considered if the fsync
  queue is larger than the number of shared buffers.  That doesn't
  seem like a simple case to me!

- Does it improve performance?

  I haven't been able to induce it at a level that would make the
  improvement visible.  But a database that is busy enough to have a
  'full' fsync queue should surely be helped by reducing the number
  of fsync requests.

- Does it slow down other things?

  In principle, the only case where it should worsen performance
  is if the amount of time required to:
- Set up a hash table
- Insert an entry for each buffer
- Walk the 

Re: [HACKERS] Replication logging

2011-01-17 Thread Tom Lane
Magnus Hagander mag...@hagander.net writes:
 On Mon, Jan 17, 2011 at 17:46, Tom Lane t...@sss.pgh.pa.us wrote:
 I think it'd make more sense just to say that replication connections
 are subject to the same log_connections rule as others.  An extra GUC
 for this is surely overkill.

 I thought so, but Robert didn't agree. And given that things are the
 way they are, clearly somebody else didn't agree as well - though I've
 been unable to locate the original discussion if there was one.

The existing behavior dates from here:
http://archives.postgresql.org/pgsql-committers/2010-03/msg00245.php

As best I can tell there was no preceding discussion, just Simon
unilaterally deciding that this logging was required for debugging
purposes.  (There is a followup thread in -hackers arguing about the
message wording, but nobody questioned whether it should come out
unconditionally.)

I'm of the opinion that the correct way of lowering in later releases
is to make the messages obey Log_connections.  The needed for debug
argument seems mighty weak to me even for the time, and surely falls
down now.

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


  1   2   >