Re: [PATCHES] Exposing keywords to clients

2008-05-03 Thread Dave Page
On Sat, May 3, 2008 at 12:24 AM, Alvaro Herrera
[EMAIL PROTECTED] wrote:
 Dave Page wrote:
  Hi,
 
  The attached patch implements a new function, pg_get_keywords(), which
  returns a set of records describing the keywords recognised by the
  server. This allows clients such as pgAdmin to get quoting rules
  correct, and helps with other tasks such as syntax highlighting where
  we need to support multiple server versions.

 FWIW pg_dump has fmtId() which does something related.

 I think it's a bit bogus to be using the list as compiled client-side,
 precisely due to the theoretical chance that it could change from one
 server version to the next, but it's probably not very likely that we
 ever remove a keyword from the server grammar.  And highlighting a
 keyword that's not really a keyword is unlikely to be a problem in
 practice -- in fact it makes it obvious that the user is likely to be in
 trouble later when they upgrade.

Yeah, we currently lift a copy of keywords.c into the pgAdmin source
and use that in our own qtIdent() function, but it's clearly only
correct for the version of Postgres we pull it from, whilst pgAdmin
supports back to 7.3 in current releases. It's also a pain because we
try to support some of the derivative servers like those offered by
Greenplum and EnterpriseDB which may have additional keywords (though
that obviously is not a problem for this community).

  postgres=# select * from pg_get_keywords();
 word|   category
  ---+---
   all   | Reserved
   binary| Type or function name
   xmlserialize  | Column name
   zone  | Unreserved
  (372 rows)
 
  I wasn't sure about the best way to describe the categories -
  obviously they need to be non-translatable (for client software to
  interpret), but human readable is also nice. I'm happy to hear
  alternate suggestions.

 Perhaps use a separate string for machine parse (say R, T, C, U), and
 let the string be translatable.

I considered that, but wasn't sure if folks would like the redundancy.
It's trivial to do of course - any objections?

-- 
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com

-- 
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] plpgsql CASE statement - last version

2008-05-03 Thread Pavel Stehule
Hello

2008/5/3 Tom Lane [EMAIL PROTECTED]:
 Pavel Stehule [EMAIL PROTECTED] writes:
 2008/5/2 Heikki Linnakangas [EMAIL PROTECTED]:
 How about taking a completely different strategy, and implement the
 CASE-WHEN construct fully natively in plpgsql, instead of trying to convert
 it to a single SQL CASE-WHEN expression? It's not a very good match anyway;

 It was first variant. It's  simpler for parsing and slower for
 execution :(. It means more than once expression evaluation and for
 simple case value casting and comparation.

 I agree with Heikki: this patch is seriously ugly, and slower for
 execution isn't a good enough reason for saddling us with having
 to maintain such a kluge in the parser.

 I don't really see why you should need to have multiple expression
 evaluations, anyhow.  Can't you evaluate the test expression once
 and inject its value into the comparisons using CaseTestExpr,
 the same way the core CASE-expression code works?



I have to look on this way.

Regards
Pavel Stehule

   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] [COMMITTERS] pgsql: Sigh ...

2008-05-03 Thread Peter Eisentraut
Andrew Dunstan wrote:
 This patch should fix things for both sets of changes. And it
 demonstrates pretty much what you need to do for config options for MSVC.

Btw., it is quite easily possible to use the autom4te tracing facility to 
parse the configure.ac file, in case you are interested in generating some of 
the Windows build code automatically.

For example:

$ autoconf -t 'AC_ARG_ENABLE:$1' configure.in
integer-datetimes
nls
shared
rpath
spinlocks
debug
profiling
dtrace
segmented-files
depend
cassert
thread-safety
thread-safety
thread-safety-force
largefile

Let me know if you want to do something with that.

-- 
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] Exposing keywords to clients

2008-05-03 Thread Peter Eisentraut
Dave Page wrote:
  Perhaps use a separate string for machine parse (say R, T, C, U), and
  let the string be translatable.

 I considered that, but wasn't sure if folks would like the redundancy.
 It's trivial to do of course - any objections?

Is there anything useful you would do with this information?  Or would you 
just quote all listed words anyway?

-- 
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] configure option for XLOG_BLCKSZ

2008-05-03 Thread Simon Riggs
On Fri, 2008-05-02 at 12:28 -0400, Greg Smith wrote:

 As PostgreSQL makes its way into higher throughput environments, it 
 wouldn't surprise me to discover more of these situations where switching 
 WAL segments every 16MB turns into a bottleneck. 

We already hit that issue and fixed it early in the 8.3 cycle. It was
more of a problem than the checkpoint issue because it caused hard
lock-outs while the file switches occurred. It didn't show up unless you
looked at the very detailed transaction result data because on fast
systems we are file switching every few seconds.

Not seen any gains from varying the WAL file size since then... 

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


Re: [PATCHES] [COMMITTERS] pgsql: Sigh ...

2008-05-03 Thread Andrew Dunstan



Peter Eisentraut wrote:

Andrew Dunstan wrote:
  

This patch should fix things for both sets of changes. And it
demonstrates pretty much what you need to do for config options for MSVC.



Btw., it is quite easily possible to use the autom4te tracing facility to 
parse the configure.ac file, in case you are interested in generating some of 
the Windows build code automatically.


For example:

$ autoconf -t 'AC_ARG_ENABLE:$1' configure.in
integer-datetimes
nls
shared
rpath
spinlocks
debug
profiling
dtrace
segmented-files
depend
cassert
thread-safety
thread-safety
thread-safety-force
largefile

Let me know if you want to do something with that.

  


Yeah, maybe. Let's fix the issue pg_config.h.win32 that Tom raised first.

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] create or replace language

2008-05-03 Thread Andreas 'ads' Scherbaum
On Sat, 29 Mar 2008 22:35:21 -0400 Tom Lane wrote:

 The key argument seems to be that it's quite unclear what the state
 following CREATE IF NOT EXISTS (CINE) should be, if the object does
 exist but not with the same properties specified in the CINE command.
 CREATE OR REPLACE resolves that by making it clear that it's gonna be
 what the command says.  Perhaps there is a use-case for the alternate
 behavior where the pre-existing object doesn't get modified, but I'm
 not too sure what it would be.

Attached is a first version for the CREATE OR REPLACE LANGUAGE patch.
It's still missing some functionality (especially the update part is
far away from being complete) and it's also missing documentation.

I just want to know if i'm heading in the right direction or if
something is totally broken in my basic approach:


In case a language is already in pg_pltemplate, the (possibly changed)
values from this table are used to update the pg_languages entry. This
gives the ability to change the owner, trust status, the language or
validator handler.

In case the language is not in pg_pltemplate, the values from the
commandline are used, just like create language.



Thanks  kind regards

-- 
Andreas 'ads' Scherbaum
German PostgreSQL User Group
diff -x CVS -ruN pgsql.orig/src/backend/commands/proclang.c pgsql/src/backend/commands/proclang.c
--- pgsql.orig/src/backend/commands/proclang.c	2008-04-29 23:59:02.0 +0200
+++ pgsql/src/backend/commands/proclang.c	2008-05-03 18:28:50.0 +0200
@@ -48,7 +48,7 @@
 } PLTemplate;
 
 static void create_proc_lang(const char *languageName,
- Oid languageOwner, Oid handlerOid, Oid valOid, bool trusted);
+ Oid languageOwner, Oid handlerOid, Oid valOid, bool trusted, int replace);
 static PLTemplate *find_language_template(const char *languageName);
 static void AlterLanguageOwner_internal(HeapTuple tup, Relation rel,
 			Oid newOwnerId);
@@ -67,6 +67,7 @@
 valOid;
 	Oid			funcrettype;
 	Oid			funcargtypes[1];
+	int			replace; /* store info about replace */
 
 	/*
 	 * Translate the language name and check that this language doesn't
@@ -74,12 +75,24 @@
 	 */
 	languageName = case_translate_language_name(stmt-plname);
 
+
+	replace = 0;
 	if (SearchSysCacheExists(LANGNAME,
 			 PointerGetDatum(languageName),
 			 0, 0, 0))
-		ereport(ERROR,
-(errcode(ERRCODE_DUPLICATE_OBJECT),
- errmsg(language \%s\ already exists, languageName)));
+	{
+		if (stmt-replace)
+		{
+			replace = 1;
+		}
+		else
+		{
+			ereport(ERROR,
+	(errcode(ERRCODE_DUPLICATE_OBJECT),
+	 errmsg(language \%s\ already exists, languageName)));
+		}
+	}
+
 
 	/*
 	 * If we have template information for the language, ignore the supplied
@@ -131,7 +144,7 @@
 		{
 			handlerOid = ProcedureCreate(pltemplate-tmplhandler,
 		 PG_CATALOG_NAMESPACE,
-		 false, /* replace */
+		 stmt-replace, /* replace */
 		 false, /* returnsSet */
 		 LANGUAGE_HANDLEROID,
 		 ClanguageId,
@@ -164,7 +177,7 @@
 			{
 valOid = ProcedureCreate(pltemplate-tmplvalidator,
 		 PG_CATALOG_NAMESPACE,
-		 false, /* replace */
+		 stmt-replace, /* replace */
 		 false, /* returnsSet */
 		 VOIDOID,
 		 ClanguageId,
@@ -189,7 +202,7 @@
 
 		/* ok, create it */
 		create_proc_lang(languageName, GetUserId(), handlerOid, valOid,
-		 pltemplate-tmpltrusted);
+		 pltemplate-tmpltrusted, replace);
 	}
 	else
 	{
@@ -253,7 +266,7 @@
 
 		/* ok, create it */
 		create_proc_lang(languageName, GetUserId(), handlerOid, valOid,
-		 stmt-pltrusted);
+		 stmt-pltrusted, replace);
 	}
 }
 
@@ -262,67 +275,125 @@
  */
 static void
 create_proc_lang(const char *languageName,
- Oid languageOwner, Oid handlerOid, Oid valOid, bool trusted)
+ Oid languageOwner, Oid handlerOid, Oid valOid, bool trusted, int replace)
 {
 	Relation	rel;
 	TupleDesc	tupDesc;
 	Datum		values[Natts_pg_language];
 	char		nulls[Natts_pg_language];
+	char		replaces[Natts_pg_language];
 	NameData	langname;
 	HeapTuple	tup;
 	ObjectAddress myself,
 referenced;
 
-	/*
-	 * Insert the new language into pg_language
-	 */
-	rel = heap_open(LanguageRelationId, RowExclusiveLock);
-	tupDesc = rel-rd_att;
+	if (replace == 0)
+	{
+		/*
+		 * Insert the new language into pg_language
+		 */
+		rel = heap_open(LanguageRelationId, RowExclusiveLock);
+		tupDesc = rel-rd_att;
 
-	memset(values, 0, sizeof(values));
-	memset(nulls, ' ', sizeof(nulls));
+		memset(values, 0, sizeof(values));
+		memset(nulls, ' ', sizeof(nulls));
 
-	namestrcpy(langname, languageName);
-	values[Anum_pg_language_lanname - 1] = NameGetDatum(langname);
-	values[Anum_pg_language_lanowner - 1] = ObjectIdGetDatum(languageOwner);
-	values[Anum_pg_language_lanispl - 1] = BoolGetDatum(true);
-	values[Anum_pg_language_lanpltrusted - 1] = BoolGetDatum(trusted);
-	values[Anum_pg_language_lanplcallfoid - 1] = ObjectIdGetDatum(handlerOid);
-	

Re: [PATCHES] Exposing keywords to clients

2008-05-03 Thread Tom Lane
Peter Eisentraut [EMAIL PROTECTED] writes:
 Dave Page wrote:
 Perhaps use a separate string for machine parse (say R, T, C, U), and
 let the string be translatable.
 
 I considered that, but wasn't sure if folks would like the redundancy.
 It's trivial to do of course - any objections?

 Is there anything useful you would do with this information?  Or would you 
 just quote all listed words anyway?

I think the practical application would be to avoid quoting unreserved
keywords, as pg_dump for instance already does.  I doubt anyone would
bother distinguishing the different types of partially/wholly reserved
words.  So maybe a boolean would be sufficient --- but I have nothing
against the R/T/C/U suggestion.

A more radical alternative is just to omit unreserved words from the
view altogether.

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] configure option for XLOG_BLCKSZ

2008-05-03 Thread Tom Lane
Simon Riggs [EMAIL PROTECTED] writes:
 We already hit that issue and fixed it early in the 8.3 cycle. It was
 more of a problem than the checkpoint issue because it caused hard
 lock-outs while the file switches occurred. It didn't show up unless you
 looked at the very detailed transaction result data because on fast
 systems we are file switching every few seconds.

 Not seen any gains from varying the WAL file size since then... 

I think the use-case for varying the WAL segment size is unrelated to
performance of the master server, but would instead be concerned with
adjusting the granularity of WAL log shipping.

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] [COMMITTERS] pgsql: Sigh ...

2008-05-03 Thread Tom Lane
Andrew Dunstan [EMAIL PROTECTED] writes:
 Peter Eisentraut wrote:
 Btw., it is quite easily possible to use the autom4te tracing facility to 
 parse the configure.ac file, in case you are interested in generating some 
 of 
 the Windows build code automatically.

 Yeah, maybe. Let's fix the issue pg_config.h.win32 that Tom raised first.

+1 for both.  We really need to eliminate as much redundancy as we can
between the two build systems, because it'll keep biting us until we do.

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] configure option for XLOG_BLCKSZ

2008-05-03 Thread Andreas 'ads' Scherbaum
On Sat, 03 May 2008 13:14:35 -0400 Tom Lane wrote:

 Simon Riggs [EMAIL PROTECTED] writes:
 
  Not seen any gains from varying the WAL file size since then... 
 
 I think the use-case for varying the WAL segment size is unrelated to
 performance of the master server, but would instead be concerned with
 adjusting the granularity of WAL log shipping.

*nod* I heard this argument several times. Simon: there was a discussion
about this topic in Prato last year. Since WAL logfiles are usually
binary stuff, the files can't be compressed much so a smaller logfile
size on a not-so-much-used system would save a noticeable amount of
bandwith (and cpu cycles for compression).


Kind regards

-- 
Andreas 'ads' Scherbaum
German PostgreSQL User Group

-- 
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] create or replace language

2008-05-03 Thread Tom Lane
Andreas 'ads' Scherbaum [EMAIL PROTECTED] writes:
 Attached is a first version for the CREATE OR REPLACE LANGUAGE patch.
 It's still missing some functionality (especially the update part is
 far away from being complete) and it's also missing documentation.

It strikes me that if there are any existing functions in the language,
we might want to restrict what can be changed by CREATE OR REPLACE.
For instance switching to a completely different language handler
doesn't seem like a great idea.

The equivalent problem for views and functions is handled by restricting
CREATE OR REPLACE to not change the output column set of a view or the
type signature of a function, independently of whether there are any
actual references to the object.  So maybe the right thing is that
CREATE OR REPLACE LANGUAGE can change inessential properties of an
existing language, but not the core properties --- which might only be
the handler function, though you could make a case for the validator and
the trusted flag as well.

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] create or replace language

2008-05-03 Thread Andreas 'ads' Scherbaum

Hello,

On Sat, 03 May 2008 13:34:05 -0400 Tom Lane wrote:

 So maybe the right thing is that CREATE OR REPLACE LANGUAGE can change
 inessential properties of an existing language, but not the core
 properties --- which might only be the handler function, though you
 could make a case for the validator and the trusted flag as well.

Already thought about that: exchanging the handler function or the
libbrary might only be useful in a developing environment, i don't see
other use cases here. The same is true for the validator (but a missing
validator could be added afterwards) and in my opinion i would prefer
not to change the trust flag - some functions may depend on this.

The name cannot be changed at all so only the owner and maybe the
validator is left ...

Did i miss something?


Kind regards

-- 
Andreas 'ads' Scherbaum
German PostgreSQL User Group

-- 
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] [COMMITTERS] pgsql: Sigh ...

2008-05-03 Thread Magnus Hagander

Tom Lane wrote:

Andrew Dunstan [EMAIL PROTECTED] writes:

Peter Eisentraut wrote:
Btw., it is quite easily possible to use the autom4te tracing facility to 
parse the configure.ac file, in case you are interested in generating some of 
the Windows build code automatically.



Yeah, maybe. Let's fix the issue pg_config.h.win32 that Tom raised first.


+1 for both.  We really need to eliminate as much redundancy as we can
between the two build systems, because it'll keep biting us until we do.


+1 from here as well, if that wasn' obvious :-)

(will get back to the actual issues at hand when I get back in a more 
connected mode in a couple of days, unless they are already solved by then)


//Magnus

--
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] Exposing keywords to clients

2008-05-03 Thread Dave Page
On Sat, May 3, 2008 at 6:12 PM, Tom Lane [EMAIL PROTECTED] wrote:

 Peter Eisentraut [EMAIL PROTECTED] writes:
  Is there anything useful you would do with this information?  Or would you
  just quote all listed words anyway?

Currently, yes, we just quote all listed words.

 I think the practical application would be to avoid quoting unreserved
 keywords, as pg_dump for instance already does.  I doubt anyone would
 bother distinguishing the different types of partially/wholly reserved
 words.  So maybe a boolean would be sufficient --- but I have nothing
 against the R/T/C/U suggestion.

 A more radical alternative is just to omit unreserved words from the
 view altogether.

Well my thinking is that it costs nothing extra bar a dozen lines of
code to include the info now in case it's useful in the future, if not
for pgAdmin, then maybe for Lightning Admin or one of the other tools.
If a need for it arises in the future and we haven't included it now
we'll either want to add a second function or break compatibility both
of which strike me as a lot more objectionable than doing it all now.

Attached is an updated patch, giving the following output. The catdesc
column can be translated.

postgres=# select * from pg_get_keywords();
   word| catcode |catdesc
---+-+---
 abort | U   | Unreserved
 all   | R   | Reserved
 bigint| C   | Column name
 binary| T   | Type or function name

-- 
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
Index: doc/src/sgml/func.sgml
===
RCS file: /projects/cvsroot/pgsql/doc/src/sgml/func.sgml,v
retrieving revision 1.434
diff -c -r1.434 func.sgml
*** doc/src/sgml/func.sgml  28 Apr 2008 14:48:57 -  1.434
--- doc/src/sgml/func.sgml  3 May 2008 19:10:03 -
***
*** 10874,10879 
--- 10874,10885 
/row

row
+
entryliteralfunctionpg_get_keywords/function()/literal/entry
+entrytypesetof record/type/entry
+entrylist of keywords and their categories/entry
+   /row
+ 
+   row
 
entryliteralfunctionpg_my_temp_schema/function()/literal/entry
 entrytypeoid/type/entry
 entryOID of session's temporary schema, or 0 if none/entry
***
*** 11009,11014 
--- 11015,11033 
 /para

 indexterm
+ primarypg_get_keywords/primary
+/indexterm
+
+para
+ functionpg_get_keywords/function returns a set of records describing
+ the keywords recognized by the server. The structfieldword/ column
+ contains the keyword, the structfieldcatcode/ column contains a
+ category code of 'U' for unknown, 'C' for column name, 'T' for type or
+ function name or 'U' for unreserved. The structfieldcatdesc/
+ column contains a localised stribg describing the category.
+/para
+
+indexterm
  primarypg_my_temp_schema/primary
 /indexterm

Index: src/backend/parser/keywords.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/parser/keywords.c,v
retrieving revision 1.195
diff -c -r1.195 keywords.c
*** src/backend/parser/keywords.c   27 Mar 2008 03:57:33 -  1.195
--- src/backend/parser/keywords.c   2 May 2008 19:32:40 -
***
*** 38,44 
   * !!WARNING!!: This list must be sorted by ASCII name, because binary
   * search is used to locate entries.
   */
! static const ScanKeyword ScanKeywords[] = {
/* name, value, category */
{abort, ABORT_P, UNRESERVED_KEYWORD},
{absolute, ABSOLUTE_P, UNRESERVED_KEYWORD},
--- 38,44 
   * !!WARNING!!: This list must be sorted by ASCII name, because binary
   * search is used to locate entries.
   */
! const ScanKeyword ScanKeywords[] = {
/* name, value, category */
{abort, ABORT_P, UNRESERVED_KEYWORD},
{absolute, ABSOLUTE_P, UNRESERVED_KEYWORD},
***
*** 423,428 
--- 423,431 
{zone, ZONE, UNRESERVED_KEYWORD},
  };

+ /* End of ScanKeywords, for use elsewhere */
+ const ScanKeyword *LastScanKeyword = endof(ScanKeywords);
+
  /*
   * ScanKeywordLookup - see if a given word is a keyword
   *
Index: src/backend/utils/adt/misc.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/utils/adt/misc.c,v
retrieving revision 1.62
diff -c -r1.62 misc.c
*** src/backend/utils/adt/misc.c17 Apr 2008 20:56:41 -  1.62
--- src/backend/utils/adt/misc.c3 May 2008 19:27:58 -
***
*** 20,29 
--- 20,31 
  #include math.h

  #include access/xact.h
+ #include catalog/pg_type.h
  #include catalog/pg_tablespace.h
  #include commands/dbcommands.h
  #include funcapi.h
  #include miscadmin.h
+ #include parser/keywords.h
  #include postmaster/syslogger.h
 

Re: [PATCHES] Exposing keywords to clients

2008-05-03 Thread Tom Lane
Dave Page [EMAIL PROTECTED] writes:
 Attached is an updated patch, giving the following output. The catdesc
 column can be translated.

Documentation has got a couple of problems:

 + contains the keyword, the structfieldcatcode/ column contains a
 + category code of 'U' for unknown, 'C' for column name, 'T' for type or
 + function name or 'U' for unreserved. The structfieldcatdesc/
 + column contains a localised stribg describing the category.

I wouldn't document the unknown case at all, much less document it
incorrectly, and you forgot the reserved case, and stribg?

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] Exposing keywords to clients

2008-05-03 Thread Tom Lane
Dave Page [EMAIL PROTECTED] writes:
 Attached is an updated patch, giving the following output.

Oh, one other thing: dropping externs into random modules unrelated to
their source module is completely awful programming style, because there
is nothing preventing incompatible declarations.  Put those externs in
keywords.h instead.  I suspect you have ignored a compiler warning
about not declaring pg_get_keywords itself, too --- it should be
extern'd in builtins.h.

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] Exposing keywords to clients

2008-05-03 Thread Dave Page
On Sat, May 3, 2008 at 9:06 PM, Tom Lane [EMAIL PROTECTED] wrote:
 Dave Page [EMAIL PROTECTED] writes:
  Attached is an updated patch, giving the following output.

 Oh, one other thing: dropping externs into random modules unrelated to
 their source module is completely awful programming style, because there
 is nothing preventing incompatible declarations.  Put those externs in
 keywords.h instead.

OK.

 I suspect you have ignored a compiler warning
 about not declaring pg_get_keywords itself, too --- it should be
 extern'd in builtins.h.

No, no warning (I'm using VC++ today) - but fixed anyway.

Update attached, including corrected docs. Note to self - proof read
docs *after* putting the kids to bed in future.

-- 
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
Index: doc/src/sgml/func.sgml
===
RCS file: /projects/cvsroot/pgsql/doc/src/sgml/func.sgml,v
retrieving revision 1.434
diff -c -r1.434 func.sgml
*** doc/src/sgml/func.sgml  28 Apr 2008 14:48:57 -  1.434
--- doc/src/sgml/func.sgml  3 May 2008 20:17:47 -
***
*** 10874,10879 
--- 10874,10885 
/row
  
row
+
entryliteralfunctionpg_get_keywords/function()/literal/entry
+entrytypesetof record/type/entry
+entrylist of keywords and their categories/entry
+   /row
+ 
+   row
 
entryliteralfunctionpg_my_temp_schema/function()/literal/entry
 entrytypeoid/type/entry
 entryOID of session's temporary schema, or 0 if none/entry
***
*** 11009,11014 
--- 11015,11033 
 /para

 indexterm
+ primarypg_get_keywords/primary
+/indexterm
+
+para
+ functionpg_get_keywords/function returns a set of records describing
+ the keywords recognized by the server. The structfieldword/ column
+ contains the keyword and the structfieldcatcode/ column contains a
+ category code of 'U' for unreserved, 'C' for column name, 'T' for type
+ or function name or 'R' for reserved. The structfieldcatdesc/
+ column contains a localised string describing the category.
+/para
+
+indexterm
  primarypg_my_temp_schema/primary
 /indexterm

Index: src/backend/parser/keywords.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/parser/keywords.c,v
retrieving revision 1.195
diff -c -r1.195 keywords.c
*** src/backend/parser/keywords.c   27 Mar 2008 03:57:33 -  1.195
--- src/backend/parser/keywords.c   2 May 2008 19:32:40 -
***
*** 38,44 
   * !!WARNING!!: This list must be sorted by ASCII name, because binary
   * search is used to locate entries.
   */
! static const ScanKeyword ScanKeywords[] = {
/* name, value, category */
{abort, ABORT_P, UNRESERVED_KEYWORD},
{absolute, ABSOLUTE_P, UNRESERVED_KEYWORD},
--- 38,44 
   * !!WARNING!!: This list must be sorted by ASCII name, because binary
   * search is used to locate entries.
   */
! const ScanKeyword ScanKeywords[] = {
/* name, value, category */
{abort, ABORT_P, UNRESERVED_KEYWORD},
{absolute, ABSOLUTE_P, UNRESERVED_KEYWORD},
***
*** 423,428 
--- 423,431 
{zone, ZONE, UNRESERVED_KEYWORD},
  };

+ /* End of ScanKeywords, for use elsewhere */
+ const ScanKeyword *LastScanKeyword = endof(ScanKeywords);
+
  /*
   * ScanKeywordLookup - see if a given word is a keyword
   *
Index: src/backend/utils/adt/misc.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/utils/adt/misc.c,v
retrieving revision 1.62
diff -c -r1.62 misc.c
*** src/backend/utils/adt/misc.c17 Apr 2008 20:56:41 -  1.62
--- src/backend/utils/adt/misc.c3 May 2008 20:33:45 -
***
*** 20,29 
--- 20,31 
  #include math.h

  #include access/xact.h
+ #include catalog/pg_type.h
  #include catalog/pg_tablespace.h
  #include commands/dbcommands.h
  #include funcapi.h
  #include miscadmin.h
+ #include parser/keywords.h
  #include postmaster/syslogger.h
  #include storage/fd.h
  #include storage/pmsignal.h
***
*** 322,324 
--- 324,393 

PG_RETURN_VOID();
  }
+
+ /* Function to return the keywords list */
+ Datum
+ pg_get_keywords(PG_FUNCTION_ARGS)
+ {
+   FuncCallContext *funcctx;
+
+   if (SRF_IS_FIRSTCALL())
+   {
+   MemoryContext oldcontext;
+   TupleDesc   tupdesc;
+
+   funcctx = SRF_FIRSTCALL_INIT();
+   oldcontext = 
MemoryContextSwitchTo(funcctx-multi_call_memory_ctx);
+
+   tupdesc = CreateTemplateTupleDesc(3, false);
+   TupleDescInitEntry(tupdesc, (AttrNumber) 1, word,
+  TEXTOID, -1, 0);
+   TupleDescInitEntry(tupdesc, (AttrNumber) 2, catcode,
+ 

Re: [PATCHES] [HACKERS] Multiline privileges in \z

2008-05-03 Thread Andrew Dunstan



Brendan Jurd wrote:

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On Fri, Apr 18, 2008 at 2:37 AM, Tom Lane  wrote:
  

 The function names in the patch need schema-qualification in case
 pg_catalog is not first in the search path.




Ah, yes.  I should have seen that.  Thanks Tom.

New version attached with schema-qualification.
  


Wouldn't this expression:


pg_catalog.array_to_string(c.relacl, chr(10))


be better expressed as


pg_catalog.array_to_string(c.relacl, E'\n')

?

Quoted inside a C literal, the backslash would have to be doubled, of course.

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] [HACKERS] Multiline privileges in \z

2008-05-03 Thread Tom Lane
Andrew Dunstan [EMAIL PROTECTED] writes:
 Wouldn't this expression:
   pg_catalog.array_to_string(c.relacl, chr(10))
 be better expressed as
   pg_catalog.array_to_string(c.relacl, E'\n')

+1 ... it's minor, but knowing that ASCII LF is 10 is probably not
wired into too many people's brains anymore.  (Besides, some of us
remember it as octal 12, anyway...)

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] Sorting writes during checkpoint

2008-05-03 Thread Tom Lane
ITAGAKI Takahiro [EMAIL PROTECTED] writes:
 Greg Smith [EMAIL PROTECTED] wrote:
 If shared_buffers(=NBuffers) is set to something big, this could give some 
 memory churn.  And I think it's a bad idea to allocate something this 
 large at checkpoint time, because what happens if that fails?  Really not 
 the time you want to discover there's no RAM left.

 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.

I updated this patch to permanently allocate the working array as Greg
suggests, and to fix a bunch of commenting issues (attached).

However, I am completely unable to measure any performance improvement
from it.  Given the possible risk of out-of-memory failures, I think the
patch should not be applied without some direct proof of performance
benefits, and I don't see any.

regards, tom lane

Index: src/backend/storage/buffer/bufmgr.c
===
RCS file: /cvsroot/pgsql/src/backend/storage/buffer/bufmgr.c,v
retrieving revision 1.228
diff -c -r1.228 bufmgr.c
*** src/backend/storage/buffer/bufmgr.c 1 Jan 2008 19:45:51 -   1.228
--- src/backend/storage/buffer/bufmgr.c 4 May 2008 01:11:08 -
***
*** 56,61 
--- 56,68 
  #define BUF_WRITTEN   0x01
  #define BUF_REUSABLE  0x02
  
+ /* Struct for BufferSync's internal to-do list */
+ typedef struct BufAndTag
+ {
+   int buf_id;
+   BufferTag   tag;
+ } BufAndTag;
+ 
  
  /* GUC variables */
  bool  zero_damaged_pages = false;
***
*** 986,991 
--- 993,1025 
  }
  
  /*
+  * qsort comparator for BufferSync
+  */
+ static int
+ bufandtagcmp(const void *a, const void *b)
+ {
+   const BufAndTag *lhs = (const BufAndTag *) a;
+   const BufAndTag *rhs = (const BufAndTag *) b;
+   int r;
+ 
+   /*
+* We don't much care about the order in which different relations get
+* written, so memcmp is enough for comparing the relfilenodes,
+* even though its behavior will be platform-dependent.
+*/
+   r = memcmp(lhs-tag.rnode, rhs-tag.rnode, sizeof(lhs-tag.rnode));
+   if (r != 0)
+   return r;
+ 
+   /* We do want blocks within a relation to be ordered accurately */
+   if (lhs-tag.blockNum  rhs-tag.blockNum)
+   return -1;
+   if (lhs-tag.blockNum  rhs-tag.blockNum)
+   return 1;
+   return 0;
+ }
+ 
+ /*
   * BufferSync -- Write out all dirty buffers in the pool.
   *
   * This is called at checkpoint time to write out all dirty shared buffers.
***
*** 995,1004 
  static void
  BufferSync(int flags)
  {
int buf_id;
-   int num_to_scan;
int num_to_write;
int num_written;
  
/* Make sure we can handle the pin inside SyncOneBuffer */
ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
--- 1029,1056 
  static void
  BufferSync(int flags)
  {
+   static BufAndTag *bufs_to_write = NULL;
int buf_id;
int num_to_write;
int num_written;
+   int i;
+ 
+   /*
+* We allocate the bufs_to_write[] array on first call and keep it
+* around for the life of the process.  This is okay because in normal
+* operation this function is only called within the bgwriter, so
+* we won't have lots of large arrays floating around.  We prefer this
+* way because we don't want checkpoints to suddenly start failing
+* when the system gets under memory pressure.
+*/
+   if (bufs_to_write == NULL)
+   {
+   bufs_to_write = (BufAndTag *) malloc(NBuffers * 
sizeof(BufAndTag));
+   if (bufs_to_write == NULL)
+   ereport(FATAL,
+   (errcode(ERRCODE_OUT_OF_MEMORY),
+errmsg(out of memory)));
+   }
  
/* Make sure we can handle the pin inside SyncOneBuffer */
ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
***
*** 1033,1038 
--- 1085,1092 
if (bufHdr-flags  BM_DIRTY)
{
bufHdr-flags |= BM_CHECKPOINT_NEEDED;
+   bufs_to_write[num_to_write].buf_id = buf_id;
+   bufs_to_write[num_to_write].tag = bufHdr-tag;
num_to_write++;
}
  
***
*** 1043,1061 
return; /* nothing to do */
  
/*
!* Loop over all buffers again, and write the ones (still) marked with
!* BM_CHECKPOINT_NEEDED.  In this loop, we start at the