Re: [HACKERS] Pretty printed trigger in psql

2010-01-18 Thread Brad T. Sliger 
On Tuesday 12 January 2010 01:06:22 Takahiro Itagaki wrote:
 Psql shows too many parentheses when it prints triggers with WHEN clause.

 postgres=# \d t1
   Table public.t1
  Column |  Type   | Modifiers
 +-+---
  c1 | integer |
 Triggers:
 mytrig AFTER UPDATE ON t1 FOR EACH ROW
 WHEN ((old.c1  new.c1)) EXECUTE PROCEDURE myfunc()
   ^^

 The attached patch eliminates unneeded parentheses by using
 pg_get_triggerdef(pretty = true) in psql.

 Triggers:
 mytrig AFTER UPDATE ON t1 FOR EACH ROW
 WHEN (old.c1  new.c1) EXECUTE PROCEDURE myfunc()

 snip

Greetings,

I tried to apply this patch to the latest version of PostgreSQL in git 
(bbfc96e).  Some of the patch did not apply.  Please 
find attached the output from patch.  The full path of the ruleutils.c.rej is 
src/backend/utils/adt/ruleutils.c.rej

Thanks,

--bts
Hmm...  Looks like a new-style context diff to me...
The text leading up to this was:
--
|diff -cprN head/src/backend/utils/adt/ruleutils.c 
work/src/backend/utils/adt/ruleutils.c
|*** head/src/backend/utils/adt/ruleutils.c 2010-01-04 09:10:26.638773000 
+0900
|--- work/src/backend/utils/adt/ruleutils.c 2010-01-12 17:51:27.595666819 
+0900
--
Patching file src/backend/utils/adt/ruleutils.c using Plan A...
Hunk #1 failed at 518.
Hunk #2 failed at 572.
Hunk #3 succeeded at 636.
2 out of 3 hunks failed--saving rejects to src/backend/utils/adt/ruleutils.c.rej
Hmm...  The next patch looks like a new-style context diff to me...
The text leading up to this was:
--
|diff -cprN head/src/bin/psql/describe.c work/src/bin/psql/describe.c
|*** head/src/bin/psql/describe.c   2010-01-04 09:10:26.638773000 +0900
|--- work/src/bin/psql/describe.c   2010-01-12 17:51:27.597646243 +0900
--
Patching file src/bin/psql/describe.c using Plan A...
Hunk #1 succeeded at 1854 with fuzz 2.
Hmm...  The next patch looks like a new-style context diff to me...
The text leading up to this was:
--
|diff -cprN head/src/test/regress/expected/triggers.out 
work/src/test/regress/expected/triggers.out
|*** head/src/test/regress/expected/triggers.out2009-11-24 
10:04:57.883822000 +0900
|--- work/src/test/regress/expected/triggers.out2010-01-12 
17:53:21.142635393 +0900
--
Patching file src/test/regress/expected/triggers.out using Plan A...
Hunk #1 succeeded at 375.
Hunk #2 succeeded at 387.
Hunk #3 succeeded at 416.
Hmm...  The next patch looks like a new-style context diff to me...
The text leading up to this was:
--
|diff -cprN head/src/test/regress/sql/triggers.sql 
work/src/test/regress/sql/triggers.sql
|*** head/src/test/regress/sql/triggers.sql 2009-11-24 10:04:57.883822000 
+0900
|--- work/src/test/regress/sql/triggers.sql 2010-01-12 17:51:27.597646243 
+0900
--
Patching file src/test/regress/sql/triggers.sql using Plan A...
Hunk #1 succeeded at 304.
done
***
*** 518,527 
  	initStringInfo(buf);
  
  	tgname = NameStr(trigrec-tgname);
! 	appendStringInfo(buf, CREATE %sTRIGGER %s,
! 	 trigrec-tgisconstraint ? CONSTRAINT  : ,
  	 quote_identifier(tgname));
- 	appendStringInfoString(buf, pretty ? \n :  );
  
  	if (TRIGGER_FOR_BEFORE(trigrec-tgtype))
  		appendStringInfo(buf, BEFORE);
--- 518,526 
  	initStringInfo(buf);
  
  	tgname = NameStr(trigrec-tgname);
! 	appendStringInfo(buf, CREATE %sTRIGGER %s ,
! 	 trigrec-tgisconstraint ? CONSTRAINT : ,
  	 quote_identifier(tgname));
  
  	if (TRIGGER_FOR_BEFORE(trigrec-tgtype))
  		appendStringInfo(buf, BEFORE);
***
*** 573,605 
  			appendStringInfo(buf,  TRUNCATE);
  		findx++;
  	}
! 	appendStringInfo(buf,  ON %s,
  	 generate_relation_name(trigrec-tgrelid, NIL));
- 	appendStringInfoString(buf, pretty ? \n :  );
  
  	if (trigrec-tgisconstraint)
  	{
  		if (OidIsValid(trigrec-tgconstrrelid))
! 		{
! 			appendStringInfo(buf, FROM %s,
  			 generate_relation_name(trigrec-tgconstrrelid, NIL));
- 			appendStringInfoString(buf, pretty ? \n :  );
- 		}
  		if (!trigrec-tgdeferrable)
  			appendStringInfo(buf, NOT );
  		appendStringInfo(buf, DEFERRABLE INITIALLY );
  		if (trigrec-tginitdeferred)
! 			appendStringInfo(buf, DEFERRED);
  		else
! 			appendStringInfo(buf, IMMEDIATE);
! 		appendStringInfoString(buf, pretty ? \n :  );
  	}
  
  	if (TRIGGER_FOR_ROW(trigrec-tgtype))
! 		appendStringInfo(buf, FOR EACH ROW);
  	else
! 		appendStringInfo(buf, FOR EACH STATEMENT);
! 	appendStringInfoString(buf, pretty ? \n :  );
  
  	/* If the trigger has a WHEN qualification, add that */
  	value = fastgetattr(ht_trig, Anum_pg_trigger_tgqual,
--- 572,598 
  			appendStringInfo(buf,  TRUNCATE);
  		findx++;
  	}
! 	appendStringInfo(buf,  ON %s ,

Re: [HACKERS] Pretty printed trigger in psql

2010-01-18 Thread Brad T. Sliger 
On Monday 18 January 2010 16:40:07 Takahiro Itagaki wrote:
 Brad T. Sliger b...@sliger.org wrote:
  I tried to apply this patch to the latest version of PostgreSQL in git
  (bbfc96e).  Some of the patch did not apply.  Please find attached the
  output from patch.  The full path of the ruleutils.c.rej is
  src/backend/utils/adt/ruleutils.c.rej

 The attached patch is rebased to current CVS.

That patch applies, builds and installs.  `gmake check`  and `gmake 
distcheck` pass.  The code style looks fine and the 
patch doesn't seem to add additional lint.

The patch does remove extra ()'s from trigger descriptions with \d in 
psql.

As far as I can tell, everything looks reasonable.

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

Thanks,

--bts

-- 
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] Unicode UTF-8 table formatting for psql text output

2009-10-07 Thread Brad T. Sliger 
On Tuesday 06 October 2009 11:35:03 Roger Leigh wrote:
 On Tue, Oct 06, 2009 at 10:44:27AM +0100, Roger Leigh wrote:
  On Mon, Oct 05, 2009 at 04:32:08PM -0400, Tom Lane wrote:
   Roger Leigh rle...@codelibre.net writes:
On Sun, Oct 04, 2009 at 11:22:27PM +0300, Peter Eisentraut wrote:
Elsewhere in the psql code, notably in mbprint.c, we make the
decision on whether to apply certain Unicode-aware processing based
on whether the client encoding is UTF8.  The same should be done
here.
   
There is a patch somewhere in the pipeline that would automatically
set the psql client encoding to whatever the locale says, but until
that is done, the client encoding should be the sole setting that
rules what kind of character set processing is done on the client
side.
   
OK, that makes sense to a certain extent.  However, the characters
used to draw the table lines are not really that related to the
client encoding for data sent from the database (IMHO).
  
   Huh?  The data *in* the table is going to be in the client_encoding,
   and psql contains no mechanisms that would translate it to something
   else. Surrounding it with decoration in a different encoding is just a
   recipe for breakage.
 
  Ah, I was under the mistaken assumption that this was iconv()ed or
  otherwise translated for correct display.  In that case, I'll leave
  the patch as is (using the client encoding for table lines).
 
  I've attached an updated copy of the patch (it just removes the
  now unneeded langinfo.h header).

 This patch included a bit of code not intended for inclusion
 (setting of client encoding based on locale), which the attached
 (and hopefully final!) revision of the patch excludes.


 Regards,
 Roger

I looked at psql-utf8-table-9.patch.

It applies, builds and installs.  `gmake check` passes and is not 
affected by values of LANG or LC_ALL in the 
environment.  HTML and man documentation builds and looks good.  The patch 
doesn't introduce new lint.

The psql utility displays UTF8 line art when the client encoding is 
UTF8, and ASCII line art is displayed otherwise.  
This can be overridden with `\pset linestyle ASCII` or `\pset linestyle UTF8`.  
The psql line art is no longer affected by 
the values of LANG or LC_ALL in the environment.

As far as I can tell, everything looks reasonable.

Thanks,

--bts

-- 
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] Unicode UTF-8 table formatting for psql text output

2009-10-02 Thread Brad T. Sliger 
On Friday 02 October 2009 04:21:35 Roger Leigh wrote:
 On Wed, Sep 30, 2009 at 06:50:46PM -0400, Tom Lane wrote:
  Roger Leigh rle...@codelibre.net writes:
   On Wed, 2009-09-30 at 11:03 -0400, Andrew Dunstan wrote:
   Thinking about this some more, ISTM a much better way of approaching
   it would be to provide a flag for psql to turn off the fancy
   formatting, and have pg_regress use that flag.
  
   The attached patch implements this feature.  It adds a
   --no-pretty-formatting/-G option to psql (naming isn't my fort�,
   so feel free to change it!).  This is also documented in the
   SGML docs, and help text.  Lastly, this option is used when invoking
   psql by pg_regress, which results in a working testsuite in a UTF-8
   locale.
 
  It would be a good idea to tie this to a psql magic variable (like
  ON_ERROR_STOP) so that it could conveniently be set in ~/.psqlrc.
  I'm not actually sure that we need a dedicated command line switch
  for it, since you could use -v varname instead.

 I have attached a patch which implements the feature as a pset
 variable.  This also slightly simplifies some of the patch since
 the table style is passed to functions directly in printTableContent
 rather than separately.  The psql option '-P tablestyle=ascii' is
 passed to psql in pg_regress_main.c which means the testsuite doesn't
 fail any more.  The option is documented in the psql docs, and is
 also tab-completed.  Users can just put '\pset tablestyle ascii' in
 their .psqlrc if they want the old format in a UTF-8 locale.

 To follow up on the comments about the problems of defaulting to
 UTF-8.  There are just two potential problems with defaulting, both of
 which are problems with the user's mis-configuration of their system
 and (IMHO) not something that postgresql needs to care about.
 1) The user lacks a font containing the line-drawing characters.
It's very rare for a fixed-width terminal font to not contain
these characters, and the patch as provided sticks to the most
basic range from the VT100 set which are most commonly provided.
 2) The user's terminal emulator is not configured to accept UTF-8
input.  If you're using a UTF-8 locale, then this is necessary
to display non-ASCII characters, and is done automatically by
almost every terminal emulator out there (on Linux, they default
to using nl_langinfo(CODESET) unless configured otherwise, which
is a very simple change if required).  On any current GNU/Linux
system, you have to go out of your way to break the defaults.

 The patch currently switches to UTF-8 automatically /when available/.
 IMO this is the correct behaviour since it will work for all but the
 handful of users who misconfigured their system, and provides an
 immediate benefit.  We spent years making UTF-8 work out of the box on
 Linux and Unix systems, and it seems a trifle unfair to penalise all
 users for the sake of the few who just didn't set up their terminal
 emulator correctly (their setup is already broken, since non-ASCII
 characters returned by queries are /already/ going to be displayed
 incorrectly).


 Regards,
 Roger

I looked at psql-utf8-table-5.patch.

Lint(1) says there is an extra trailing ',' in src/bin/psql/print.h. in 
'typedef enum printTextRule'.  The addition to 
src/bin/psql/command.c could use a comment, like adjacent code.

'ASCII' and 'UTF8' may need acronym/acronym tags in 
doc/src/sgml/ref/psql-ref.sgml, like adjacent 
code.  I'm not sure someone who hasn't seen this patch in action would 
immediately know what it does from the 
documentation.  `gmake html` works without the patch, but fails with the patch:

openjade:ref/psql-ref.sgml:1692:15:E: document type does not allow 
element TERM here; assuming 
missing VARLISTENTRY start-tag

After the patch, `\pset format wrapped` produces '\pset: unknown 
option: format'.  I saw this in interactive psql 
and from .psqlrc.  I think this can be fixed by changing the addition to 
src/bin/psql/command.c from an 'if' clause to 
an 'else if' clause.

Otherwise, the patch applied, built and installed.  The `gmake check` 
tests all passed with LANG and/or LC_ALL 
set.  The various tablestyle options seem to work.  The default behavior with 
respect to various LANG and LC_ALL 
values seems reasonable and can be overridden.

Thanks,

--bts

-- 
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] Unicode UTF-8 table formatting for psql text output

2009-09-28 Thread Brad T. Sliger 
On Sunday 27 September 2009 19:03:33 Robert Haas wrote:
 On Sun, Sep 27, 2009 at 9:24 PM, Selena Deckelmann

 selenama...@gmail.com wrote:
  Hi!
 
  On Wed, Sep 23, 2009 at 2:16 AM, Roger Leigh rle...@codelibre.net wrote:
  On Fri, Sep 18, 2009 at 11:30:05AM -0700, Selena Deckelmann wrote:
  Brad says:
 
         The patched code compiles without any additional warnings.
  Lint gripes about a trailing ',' in 'typedef enum printTextRule' in
  print.h.  Other additional lint seem to be false positives.  The
  regression tests pass against the new patch.
 
  I've attached a new patch which tidies up those extra commas, plus
  a patch showing the changes from the previous patch.
 
  Great!  Thank you.
 
  Brad -- can you review this patch? Is it ready for a committer?

 Brad already marked it that way on the CommitFest application, so I
 think that probably means yes.  :-)

 ...Robert

I looked at the new (psql-utf8-table-4.patch) patch.  I think the style 
issues are fixed.

During this review I found that `gmake check` will fail when 
LANG=en_US.UTF-8 in the environment.  In this case 
the patched psql produces UTF8 line art and the tests expect ASCII line art.

pg_regress clears LC_ALL by default, but does not clear LANG by 
default.  Please find attached a patch that 
causes pg_regress to also clear LANG by default.

Thanks,

--bts

diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index f2f9603..68ba30b 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -697,11 +697,6 @@ initialize_environment(void)
 		unsetenv(LC_MONETARY);
 		unsetenv(LC_NUMERIC);
 		unsetenv(LC_TIME);
-		unsetenv(LANG);
-		/* On Windows the default locale cannot be English, so force it */
-#if defined(WIN32) || defined(__CYGWIN__)
-		putenv(LANG=en);
-#endif
 	}
 
 	/*
@@ -712,8 +707,14 @@ initialize_environment(void)
 	 */
 	unsetenv(LANGUAGE);
 	unsetenv(LC_ALL);
+	unsetenv(LANG);
+	/* On Windows the default locale cannot be English, so force it */
+#if defined(WIN32) || defined(__CYGWIN__)
+	putenv(LANG=en);
+#endif
 	putenv(LC_MESSAGES=C);
 
+
 	/*
 	 * Set multibyte as requested
 	 */

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