Re: [PATCHES] [HACKERS] Text - C string

2008-04-16 Thread Brendan Jurd
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

- -BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On Sat, Mar 29, 2008 at 5:40 AM, Brendan Jurd  wrote:
 On 29/03/2008, Tom Lane  wrote:
   I intentionally didn't touch xml.c, nor anyplace that is not dealing
in text, even if it happens to be binary-compatible with text.
  

  Hmm, okay.  My original submission did include a few such changes; for
  example, in xml_in and xml_out_internal I saw that the conversion
  between xmltype and cstring was identical to the text conversion, so I
  went ahead and used the text functions.  Feedback upthread suggested
  that it was okay to go ahead with casting in identical cases. [1]

  I saw that these changes made it into the commit, so I assumed that it
  was the right call.

  If we don't want to meddle with xmltype/bytea/VarChar at all, we'll
  have to revert those changes, and I'll have to seriously scale back
  the cleanup patch I was about to post.

Still not sure where we stand on the above.  To cast, or not to cast?

Cheers,
BJ
- -BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.7 (GNU/Linux)
Comment: http://getfiregpg.org

iD8DBQFIBaFy5YBsbHkuyV0RAsMmAKDHaEb7aMudKJgVxcf5RRcOtAJ+bwCgivLI
5B3xJze46NGWjEyOpq/TSaY=
=BObd
- -END PGP SIGNATURE-

-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.7 (GNU/Linux)
Comment: http://getfiregpg.org

iD8DBQFIBaIl5YBsbHkuyV0RArDDAJ0QThLXAhzy+NX+2YsF+q4z/sIy1QCeJPiW
s/rVns3FFQVP5g9DTOshDfQ=
=4Tdh
-END PGP SIGNATURE-

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


Re: [PATCHES] Improve shutdown during online backup

2008-04-16 Thread Simon Riggs
On Tue, 2008-04-08 at 09:16 +0200, Albe Laurenz wrote:

 Heikki Linnakangas wrote:
  Albe Laurenz wrote:
  Moreover, if Shutdown == SmartShutdown, new connections won't be accepted,
  and nobody can connect and call pg_stop_backup().
  So even if I'd add a check for
  (pmState == PM_WAIT_BACKENDS)  !BackupInProgress() somewhere in the
  ServerLoop(), it wouldn't do much good, because the only way for somebody
  to cancel online backup mode would be to manually remove the file.
  
  Good point.
  
  So the only reasonable thing to do on smart shutdown during an online
  backup is to have the shutdown request fail, right? The only alternative 
  being
  that a smart shutdown request should interrupt online backup mode.
  
  Or we can add another state, PM_WAIT_BACKUP, before PM_WAIT_BACKENDS, 
  that allows new connections, and waits until the backup ends.
 
 That's an option. Maybe it is possible to restrict connections to superusers
 (who are the only ones who can call pg_stop_backup() anyway).
 
 Or, we could allow superuser connections in state PM_WAIT_BACKENDS...

That sounds right. 

Completely unrelated to backups, if you issue a smart shutdown and it
doesn't, you probably would like to connect and see what is happening
and why. The reason may not be a backup-in-progress.

Personally, I think smart shutdown could be even smarter. It should
kick off unwanted sessions, such as an idle pgAdmin session - maybe a
rule like anything that has been idle for 30 seconds.

-- 
  Simon Riggs
  2ndQuadrant  http://www.2ndQuadrant.com


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


[PATCHES] options for RAISE statement

2008-04-16 Thread Pavel Stehule
Hello

this patch adds possibility to set additional options (SQLSTATE,
DETAIL, DETAIL_LOG and HINT) for RAISE statement,

Proposal: http://archives.postgresql.org/pgsql-hackers/2008-04/msg00919.php

I changed keyword from WITH to USING, because I don't would to create
new keyword

RAISE level 'format' [, expression [, ...]] [ USING ( option =
expression [, ... ] ) ];

RAISE EXCEPTION 'Nonexistent ID -- %', user_id
  USING (hint = 'Please, check your user id');

Regards
Pavel Stehule
*** ./doc/src/sgml/plpgsql.sgml.orig	2008-04-16 11:17:51.0 +0200
--- ./doc/src/sgml/plpgsql.sgml	2008-04-16 12:59:44.0 +0200
***
*** 2742,2748 
  raise errors.
  
  synopsis
! RAISE replaceable class=parameterlevel/replaceable 'replaceable class=parameterformat/replaceable' optional, replaceable class=parameterexpression/replaceable optional, .../optional/optional;
  /synopsis
  
  Possible levels are literalDEBUG/literal,
--- 2742,2748 
  raise errors.
  
  synopsis
! RAISE replaceable class=parameterlevel/replaceable 'replaceable class=parameterformat/replaceable' optional, replaceable class=parameterexpression/replaceable optional, .../optional/optional optional USING ( replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional ) /optional;
  /synopsis
  
  Possible levels are literalDEBUG/literal,
***
*** 2785,2801 
 para
  This example will abort the transaction with the given error message:
  programlisting
! RAISE EXCEPTION 'Nonexistent ID -- %', user_id;
  /programlisting
 /para
  
  para
!  commandRAISE EXCEPTION/command presently always generates
!  the same varnameSQLSTATE/varname code, literalP0001/, no matter what message
   it is invoked with.  It is possible to trap this exception with
   literalEXCEPTION ... WHEN RAISE_EXCEPTION THEN .../ but there
   is no way to tell one commandRAISE/ from another.
  /para
   /sect1
  
   sect1 id=plpgsql-trigger
--- 2785,2808 
 para
  This example will abort the transaction with the given error message:
  programlisting
! RAISE EXCEPTION 'Nonexistent ID -- %', user_id USING (hint = 'Please, check your user id');
  /programlisting
 /para
  
  para
!  commandRAISE EXCEPTION/command presently generates
!  the same varnameSQLSTATE/varname code, literalP0001/ , no matter what message
   it is invoked with.  It is possible to trap this exception with
   literalEXCEPTION ... WHEN RAISE_EXCEPTION THEN .../ but there
   is no way to tell one commandRAISE/ from another.
  /para
+ 
+ para
+   With additional options is possible set some log informaition related to 
+   raised exception. Possible options are literalSQLSTATE/literal,
+   literalDETAIL/literal, literalDETAIL_LOG/literal and literalHINT/literal.
+ /para 
+ 
   /sect1
  
   sect1 id=plpgsql-trigger
*** ./src/pl/plpgsql/src/gram.y.orig	2008-04-15 07:37:03.0 +0200
--- ./src/pl/plpgsql/src/gram.y	2008-04-15 13:42:36.0 +0200
***
*** 52,57 
--- 52,58 
  	  const char *end_label);
  static PLpgSQL_expr 	*read_cursor_args(PLpgSQL_var *cursor,
  		  int until, const char *expected);
+ static List *read_exception_additional_options();
  
  %}
  
***
*** 1258,1263 
--- 1259,1265 
  		new-elog_level = $3;
  		new-message	= $4;
  		new-params		= NIL;
+ 		new-options		= NIL;
  
  		tok = yylex();
  
***
*** 1266,1272 
  		 * indicates no parameters, or a comma that
  		 * begins the list of parameter expressions
  		 */
! 		if (tok != ','  tok != ';')
  			yyerror(syntax error);
  
  		if (tok == ',')
--- 1268,1274 
  		 * indicates no parameters, or a comma that
  		 * begins the list of parameter expressions
  		 */
! 		if (tok != ','  tok != ';'  tok != K_USING)
  			yyerror(syntax error);
  
  		if (tok == ',')
***
*** 1274,1286 
  			do
  			{
  PLpgSQL_expr *expr;
! 
! expr = read_sql_expression2(',', ';',
! 			, or ;,
! 			tok);
  new-params = lappend(new-params, expr);
  			} while (tok == ',');
  		}
  
  		$$ = (PLpgSQL_stmt *)new;
  	}
--- 1276,1294 
  			do
  			{
  PLpgSQL_expr *expr;
! 
! expr = read_sql_construct(',', ';', K_USING, , or ; or USING,
! SELECT , true, true, tok);
  new-params = lappend(new-params, expr);
  			} while (tok == ',');
  		}
+ 		
+ 		if (tok == K_USING)
+ 		{
+ 			new-options = read_exception_additional_options();
+ 			if (yylex() != ';')
+ yyerror(syntax error);
+ 		}
  
  		$$ = (PLpgSQL_stmt *)new;
  	}
***
*** 2633,2638 
--- 2641,2704 
  }
  
  
+ /*
+  * Procedure read RAISE 

Re: [PATCHES] Reference by output in : \d table_name

2008-04-16 Thread kenneth d'souza

Hi Brendan,
 
I thought you were referring to the spaces sourrounding the word FOREIGN KEY 
on the last line and hence my explaination was out of place.I am glad that you 
have corrected the indentation to 4 spaces. Those were unintentional at 2 
spaces from myside.
However,Why does the word FOREIGN KEY appear in the last line of your output. 
My original patch had the output like this.
Referenced by:  bar_foo_fkey IN public.bar(foo) REFERENCES foo(a)
The keyword FOREIGN KEY was removed by me as it would further cause a 
confusion.
Secondly, since the table foo is altered with an addition of a new column 
bar, it doesn't display in your output. Please double check.
My output is looking like this: 
testdb=# \d foo  Table public.foo Column |  Type   | 
Modifiers+-+--- a  | integer | not null bar| 
integer | not null/* Brendan--this line is missing in your 
output */Indexes:foo_pkey PRIMARY KEY, btree (a)Foreign-key constraints:  
  foo_bar_fkey FOREIGN KEY (bar) REFERENCES bar(a)Referenced by:  
bar_foo_fkey IN public.bar(foo) REFERENCES foo(a)
/* please ignore the 2 space indent, I am still using my orignal patch. I will 
correct it later */
Thanks,Kenneth Date: Mon, 14 Apr 2008 11:04:35 -0400 From: [EMAIL PROTECTED] 
To: [EMAIL PROTECTED] CC: [EMAIL PROTECTED]; [EMAIL PROTECTED]; 
pgsql-patches@postgresql.org Subject: Re: [PATCHES] Reference by output in : 
\d table_name  Brendan Jurd escribió:   Yeah, that's what I figured. The 
patch I attached to my previous  email should fix it up.  Applied, thanks. 
 --  Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, 
Consulting, Custom Development, 24x7 support
_
Fashion Channel : Want to know what’s the latest in the fashion world ? You 
have it all here on MSN Fashion.
http://lifestyle.in.msn.com/

Re: [PATCHES] Improve shutdown during online backup

2008-04-16 Thread Gregory Stark
Simon Riggs [EMAIL PROTECTED] writes:

 Personally, I think smart shutdown could be even smarter. It should
 kick off unwanted sessions, such as an idle pgAdmin session - maybe a
 rule like anything that has been idle for 30 seconds.

That's not a bad idea in itself but I don't think it's something the server
should be in the business of doing. One big reason is that the server
shouldn't be imposing arbitrary policy. That should be something the person
running the shutdown is in control over.

What you could do is have a separate program (I would write a client but a
server-side function would work too) to kick off users based on various
criteria you can specify.

Then you can put in your backup scripts two commands, one to kick off idle
users and then do a smart shutdown.

-- 
  Gregory Stark
  EnterpriseDB  http://www.enterprisedb.com
  Ask me about EnterpriseDB's PostGIS support!

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


Re: [PATCHES] options for RAISE statement

2008-04-16 Thread Alvaro Herrera
Pavel Stehule escribió:
 Hello
 
 this patch adds possibility to set additional options (SQLSTATE,
 DETAIL, DETAIL_LOG and HINT) for RAISE statement,

Added to May commitfest page, thanks.


-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] [HACKERS] Text - C string

2008-04-16 Thread Tom Lane
Brendan Jurd [EMAIL PROTECTED] writes:
 If we don't want to meddle with xmltype/bytea/VarChar at all, we'll
 have to revert those changes, and I'll have to seriously scale back
 the cleanup patch I was about to post.

 Still not sure where we stand on the above.  To cast, or not to cast?

I dunno.  I know there was previously some handwaving about representing
XML values in some more intelligent fashion than a plain text string,
but I have no idea if anyone is likely to do anything about it in the
foreseeable future.

regards, tom lane

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


Re: [PATCHES] [HACKERS] Text - C string

2008-04-16 Thread Andrew Dunstan



Tom Lane wrote:

Brendan Jurd [EMAIL PROTECTED] writes:
  

If we don't want to meddle with xmltype/bytea/VarChar at all, we'll
have to revert those changes, and I'll have to seriously scale back
the cleanup patch I was about to post.
  


  

Still not sure where we stand on the above.  To cast, or not to cast?



I dunno.  I know there was previously some handwaving about representing
XML values in some more intelligent fashion than a plain text string,
but I have no idea if anyone is likely to do anything about it in the
foreseeable future.


  


It seems very unlikely to me.

cheers

andrew

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


Re: [PATCHES] Reference by output in : \d table_name

2008-04-16 Thread Brendan Jurd
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On Wed, Apr 16, 2008 at 10:00 PM, kenneth d'souza  wrote:
  However,Why does the word FOREIGN KEY appear in the last line of your
 output. My original patch had the output like this.
  Referenced by:
   bar_foo_fkey IN public.bar(foo) REFERENCES foo(a)
  The keyword FOREIGN KEY was removed by me as it would further cause a
 confusion.


Hi Kenneth,

Tom reinstated the FOREIGN KEY part of the definition when he
committed your patch.  I think it's fine with FOREIGN KEY left in
there; I don't find it confusing.  I actually think it makes the line
more descriptive and obvious.

 Secondly, since the table foo is altered with an addition of a new column
 bar, it doesn't display in your output. Please double check.


You're right; it was just a copy-paste error I made when I was
composing my email.  The actual output from psql shows all columns as
expected.

Cheers,
BJ
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.7 (GNU/Linux)
Comment: http://getfiregpg.org

iD8DBQFIBixv5YBsbHkuyV0RArJSAJ0eDes2V0nwlgQuNE0GjJxwW4Ey8gCgiWTw
UjSjF8EJaTDSTQnkTfgSasY=
=xdOl
-END PGP SIGNATURE-

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


Re: [PATCHES] [HACKERS] Text - C string

2008-04-16 Thread Brendan Jurd
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On Thu, Apr 17, 2008 at 1:16 AM, Tom Lane  wrote:
 Brendan Jurd  writes:

  If we don't want to meddle with xmltype/bytea/VarChar at all, we'll
   have to revert those changes, and I'll have to seriously scale back
   the cleanup patch I was about to post.

   Still not sure where we stand on the above.  To cast, or not to cast?

  I dunno.  I know there was previously some handwaving about representing
  XML values in some more intelligent fashion than a plain text string,
  but I have no idea if anyone is likely to do anything about it in the
  foreseeable future.


Well ... if somebody does want to change the representation of xml
down the road, he's going to have to touch all the sites where the
code converts to and from cstring anyway, right?

So the only real difference this will make is that, instead of having
to replace four lines of VARDATA/memcpy per site, he'll have to
replace a single function call.  That doesn't seem like a negative.

With that in mind, please find attached my followup patch.  It cleans
up another 21 sites manually copying between cstring and varlena, for
a net reduction of 115 lines of code.

I didn't attempt to work through every reference to VARDATA, but I did
look at every hit from a  `grep -Rn VARDATA . | grep memcpy`.

All regression tests passed (contrib tests included) on gentoo.

Patch added to commitfest queue.

Cheers,
BJ
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.7 (GNU/Linux)
Comment: http://getfiregpg.org

iD8DBQFIBm105YBsbHkuyV0RAmqfAKCfyNyFdciqX4QV81sG9MhPt+KXuACfe694
3d/ICZF6yqV6K20X3TVX+So=
=CvRM
-END PGP SIGNATURE-


text-cstring-followup.diff.bz2
Description: BZip2 compressed data

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


Re: [PATCHES] printTable API (was: Show INHERIT in \du)

2008-04-16 Thread Alvaro Herrera
Brendan Jurd escribió:

 The second version of the patch is attached.

Thanks.  I looked the patch over and did some minor changes.  Modified
version attached.

One thing I'm not entirely happy about is the fact that we need a
pointer to the last footer/cell/header, so two pointers for each element
kind.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.


print-table-3.patch.bz2
Description: Binary data

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


Re: [PATCHES] Sorting writes during checkpoint

2008-04-16 Thread Greg Smith

On Wed, 16 Apr 2008, ITAGAKI Takahiro wrote:


Dirty region of database was probably larger than disk controller's cache.


Might be worthwhile to run with log_checkpoints on and collect some 
statistics there next time you're running these tests.  It's a good habit 
to get other testers into regardless; it's nice to be able to say 
something like during the 15 checkpoints encountered during this test, 
the largest dirty area was 516MB while the median was 175MB.



Hmm, but I think we need to copy buffer tags into bgwriter's local memory
in order to avoid locking taga many times in the sorting. Is it better to
allocate sorting buffers at the first time and keep and reuse it from then on?


That what I was thinking:  allocate the memory when the background writer 
starts and just always have it there, the allocation you're doing is 
always the same size.  If it's in use 50% of the time anyway (which it is 
if you have checkpoint_completion_target at its default), why introduce 
the risk that an allocation will fail at checkpoint time?  Just allocate 
it once and keep it around.



It is 0.25% of shared buffers; when shared_buffers is set to 10GB,
it takes 25MB of process local memory.


Your numbers are probably closer to correct.  I was being pessimistic 
about the size of all the integers just to demonstrate that it's not 
really a significant amount of memory even if they're large.


If we want to consume less memory for it, RelFileNode in BufferTag could 
be hashed and packed into an integer


I personally don't feel it's worth making the code any more complicated 
than it needs to be just to save a fraction of a percent of the total 
memory used by the buffer pool.


--
* Greg Smith [EMAIL PROTECTED] http://www.gregsmith.com Baltimore, MD

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


Re: [PATCHES] Suppress compiler warnings on mingw

2008-04-16 Thread Andrew Dunstan



Applied, Thanks.

wiki updated.

cheers

andrew

ITAGAKI Takahiro wrote:

Peter Eisentraut [EMAIL PROTECTED] wrote:

  
Then try using %lu and no casts.  That should get rid of the warnings the 
proper way.



Ok, I rewrote it to use %lu for format strings.


Jeremy Drake [EMAIL PROTECTED] wrote:
  

sizeof(DWORD) is always 4, even on 64-bit windows.  sizeof(long) is also
always 4.



I got it. This change will work on 64-bit windows, because DWORD is
defined as 'unsigned long' there, too. We need to support LLP64
compliers in advance, though.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center


  




  


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


Re: [PATCHES] [HACKERS] Patch for Prevent pg_dump/pg_restore from being affected by statement_timeout

2008-04-16 Thread Alex Hunsaker
On Wed, Apr 16, 2008 at 4:54 PM, Alvaro Herrera
[EMAIL PROTECTED] wrote:
 Joshua D. Drake escribió:

  That is an interesting idea. Something like:
  
   pg_restore -E SET STATEMENT_TIMEOUT=0; SET MAINTENANCE_WORK_MEM=1G ?

  We already have it -- it's called PGOPTIONS.


Ok but is not the purpose of the patch to turn off statement_timeout
by *default* in pg_restore/pg_dump?

Here is an updated patch for I posted above (with the command line
option --use-statement-timeout) for pg_dump and pg_restore.

(sorry If I hijacked your patch Josh :) )


pg_dump_restore_statement_timeout.diff
Description: Binary data

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


Re: [PATCHES] printTable API (was: Show INHERIT in \du)

2008-04-16 Thread Brendan Jurd
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On Thu, Apr 17, 2008 at 7:27 AM, Alvaro Herrera  wrote:
 Brendan Jurd escribió:

   The second version of the patch is attached.

  Thanks.  I looked the patch over and did some minor changes.  Modified
  version attached.


Cool, I had a look through your changes and they all seemed fine to
me.  In particular, moving the header comments to the ... header ...
file seemed like a smart move. =)

  One thing I'm not entirely happy about is the fact that we need a
  pointer to the last footer/cell/header, so two pointers for each element
  kind.


Well, the alternative is iterating through the array each time you
want to add something until you hit a NULL pointer, and adding the new
item at that point.  Considering we're only chewing up an extra 4 *
sizeof(pointer) = 16 bytes in the struct, it seems like a reasonable
price to pay for the convenience.

What is it about the extra fields that makes you unhappy?

Cheers,
BJ
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.7 (GNU/Linux)
Comment: http://getfiregpg.org

iD8DBQFIBo2y5YBsbHkuyV0RAmLhAJ9CP/9L1Nv7WbCtrCYu6tyoGhQItQCeMRm0
HegSSBq8tXw43Kj2xeQ2RCs=
=RvzP
-END PGP SIGNATURE-

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


Re: [PATCHES] printTable API (was: Show INHERIT in \du)

2008-04-16 Thread Alvaro Herrera
Brendan Jurd escribió:

 On Thu, Apr 17, 2008 at 7:27 AM, Alvaro Herrera  wrote:

   One thing I'm not entirely happy about is the fact that we need a
   pointer to the last footer/cell/header, so two pointers for each
   element kind.
 
 Well, the alternative is iterating through the array each time you
 want to add something until you hit a NULL pointer, and adding the new
 item at that point.  Considering we're only chewing up an extra 4 *
 sizeof(pointer) = 16 bytes in the struct, it seems like a reasonable
 price to pay for the convenience.

Well, consider that there are going to be 1 or 2 entries in the arrays
in most cases anyway :-)  Well, as far as footers go anyway ... I just
realized that in all other cases it will certainly be the wrong thing to
do :-)  Still, perhaps a integer count is better?

 What is it about the extra fields that makes you unhappy?

I don't know if unnecessarity is a word, but I hope you get what I
mean :-)

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] Proposed patch - psql wraps at window width

2008-04-16 Thread Bruce Momjian
Bryce Nesbitt wrote:
 I've attached a patch, against current 8.4 cvs, which optionally sets a 
 maximum width for psql output:
  
 # \pset format aligned-wrapped
 # \pset border 2
 # select * from distributors order by did;
 +--++-+---+
 | did  |name|descr| long_col_name |
 +--++-+---+
 |1 | Food fish and wine | default |   |
 |2 | Cat Food Heaven 2  | abcdefghijklmnopqrs !   |
 |  || tuvwxyz |   |
 |3 | Cat Food Heaven 3  | default |   |
 |   10 | Lah| default |   |
 |   12 | name   | line one|   |
 | 2892 ! short name | short   |   |
 | 8732 || |   |
 +--++-+---+
 (8 rows)
 
 The interactive terminal column width comes from
 char * temp = getenv(COLUMNS);
 Which has the strong advantage of great simplicity and portability.  But 
 it may not be 1000% universal.  If $COLUMNS is not defined, the code 
 bails to assuming an infinitely wide terminal.
 
 I will also backport this to Postgres 8.1, for my own use.  Though the 
 code is almost totally different in structure.

I spent time reviewing your patch --- quite impressive.  I have attached
and updated version with mostly stylistic changes.

In testing I found the regression tests were failing because of a divide
by zero error (fixed), and a missing case where the column delimiter has
to be :.  In fact I now see all your line continuation cases using :
rather than !.  It actually looks better --- ! was too close to |
to be easily recognized.  (Did you update your patch to use :.  I
didn't see ! in your patch.)

I have added an XXX comment questioning whether the loop to find the
column to wrap is inefficient because it potentially loops over the
length of the longest column and for each character loops over the
number of columns.  Not sure if that is a problem.

I checked the use of COLUMNS and it seems bash updates the environment
variable when a window is resized.  I added ioctl(TIOCGWINSZ) if COLUMNS
isn't set.  We already had a call in print.c for detecting the
number of rows on the screen to determine if the pager should
be used.  Seems COLUMNS should take precedence over ioctl(), right?
I don't think Win32 supports that ioctl(), does it?

I added some comments and clarified some variable names.  I also renamed
the option to a shorter wrapped.  I added documentation too.

For testers compare:

\df

with:

\pset format wrap
\df

Impressive!

-- 
  Bruce Momjian  [EMAIL PROTECTED]http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +
Index: doc/src/sgml/ref/psql-ref.sgml
===
RCS file: /cvsroot/pgsql/doc/src/sgml/ref/psql-ref.sgml,v
retrieving revision 1.199
diff -c -c -r1.199 psql-ref.sgml
*** doc/src/sgml/ref/psql-ref.sgml  30 Mar 2008 18:10:20 -  1.199
--- doc/src/sgml/ref/psql-ref.sgml  17 Apr 2008 02:45:38 -
***
*** 1513,1519 
listitem
para
Sets the output format to one of literalunaligned/literal,
!   literalaligned/literal, literalhtml/literal,
literallatex/literal, or literaltroff-ms/literal.
Unique abbreviations are allowed.  (That would mean one letter
is enough.)
--- 1513,1520 
listitem
para
Sets the output format to one of literalunaligned/literal,
!   literalaligned/literal, literalwrapped/literal, 
!   literalhtml/literal,
literallatex/literal, or literaltroff-ms/literal.
Unique abbreviations are allowed.  (That would mean one letter
is enough.)
***
*** 1525,1531 
is intended to create output that might be intended to be read
in by other programs (tab-separated, comma-separated).
quoteAligned/quote mode is the standard, human-readable,
!   nicely formatted text output that is default. The
quoteacronymHTML/acronym/quote and
quoteLaTeX/quote modes put out tables that are intended to
be included in documents using the respective mark-up
--- 1526,1535 
is intended to create output that might be intended to be read
in by other programs (tab-separated, comma-separated).
quoteAligned/quote mode is the standard, human-readable,
!   nicely formatted text output that is default.
!   quoteWrapped/quote is like literalaligned/ but 

Re: [PATCHES] Proposed patch - psql wraps at window width

2008-04-16 Thread Bryce Nesbitt



Bruce Momjian wrote:

I spent time reviewing your patch --- quite impressive. I have attached
and updated version with mostly stylistic changes.

In testing I found the regression tests were failing because of a divide
by zero error (fixed), and a missing case where the column delimiter has
to be :.  In fact I now see all your line continuation cases using :
rather than !.  It actually looks better --- ! was too close to |
to be easily recognized.  (Did you update your patch to use :.  I
didn't see ! in your patch.)

Nice!  I'll merge with my current version.  As you note I changed to :.

I also found that for hugely wide output it was better to give up (do 
nothing), rather than mangle the output in a futile attempt to squash it 
to the window width.  So there is one more clause in the wrapping if.


I have tested on several unix flavors, but not on Windows or cygwin.

   -Bryce

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