Re: [HACKERS] JSON for PG 9.2

2011-12-15 Thread Hitoshi Harada
On Tue, Dec 13, 2011 at 1:13 PM, Merlin Moncure mmonc...@gmail.com wrote:
 Mozilla SpiderMonkey seems like a good fit: it compiles to a
 dependency free .so, has excellent platform support, has a stable C
 API, and while it's C++ internally makes no use of exceptions (in
 fact, it turns them off in the c++ compiler).  ISTM to be a suitable
 foundation for an external module, 'in core' parser, pl, or anything
 really.

When I started to think about PL/js, I compared three of SpiderMonkey,
SquirrelFish, and V8. SpiderMonkey at that time (around 2009) was
not-fast, not-small in memory while what you raise, as well as its
advanced features like JS1.7 (pure yield!), was attractive. Also
SpiderMonkey was a little harder to build in arbitrary platform
(including Windows) than v8. SquirrelFish was fastest of three, but
yet it's sticky with Webkit and also hard to build itself. Dunno how
they've changed since then.

Thanks,
-- 
Hitoshi Harada

-- 
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: CHECK FUNCTION statement

2011-12-15 Thread Albe Laurenz
Pavel Stehule wrote:
 so removed quite option
 and removed multiple check regression tests also - there is missing
 explicit order of function checking, so regress tests can fail :(

There seems to be a problem with the SET clause of CREATE FUNCTION:

ftest=# CREATE OR REPLACE FUNCTION a(integer) RETURNS integer
LANGUAGE plpgsql AS 'BEGIN RETURN 2*$1; END';
CREATE FUNCTION
ftest=# CHECK FUNCTION a(integer);
NOTICE:  checked function a(integer)
CHECK FUNCTION
ftest=# CREATE OR REPLACE FUNCTION a(integer) RETURNS integer
LANGUAGE plpgsql SET search_path=public AS 'BEGIN RETURN 2*$1; END';
CREATE FUNCTION
ftest=# CHECK FUNCTION a(integer);
The connection to the server was lost. Attempting reset: Failed.
!

Yours,
Laurenz Albe

-- 
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: CHECK FUNCTION statement

2011-12-15 Thread Pavel Stehule
Hello

2011/12/15 Albe Laurenz laurenz.a...@wien.gv.at:
 Pavel Stehule wrote:
 so removed quite option
 and removed multiple check regression tests also - there is missing
 explicit order of function checking, so regress tests can fail :(

 There seems to be a problem with the SET clause of CREATE FUNCTION:

 ftest=# CREATE OR REPLACE FUNCTION a(integer) RETURNS integer
        LANGUAGE plpgsql AS 'BEGIN RETURN 2*$1; END';
 CREATE FUNCTION
 ftest=# CHECK FUNCTION a(integer);
 NOTICE:  checked function a(integer)
 CHECK FUNCTION
 ftest=# CREATE OR REPLACE FUNCTION a(integer) RETURNS integer
        LANGUAGE plpgsql SET search_path=public AS 'BEGIN RETURN 2*$1; END';
 CREATE FUNCTION
 ftest=# CHECK FUNCTION a(integer);
 The connection to the server was lost. Attempting reset: Failed.
 !


There was bug - missing detoast call

fixed

Regards

Pavel


 Yours,
 Laurenz Albe


check_function-2011-12-15-1.diff.gz
Description: GNU Zip compressed data

-- 
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] includeifexists in configuration file

2011-12-15 Thread Greg Smith

On 12/12/2011 04:47 PM, Andrew Dunstan wrote:
I have briefly looked at the code (but not tried to apply or build 
it), and modulo the naming issue it looks OK to me.
Unless there is some other issue let's just get it applied. It looks 
like almost a no-brainer to me.


It isn't very fancy, but is does something people that can fit into a 
couple of use-cases.  Attached update has two changes to address the 
suggestions I got, which closes everything I knew about with this one:


-It's now include_if_exists
-Files that are skipped are logged now

So current behavior:

$ tail -n 1 postgresql.conf
include 'missing.conf'
$ start
server starting
$ tail $PGLOG
LOG:  could not open configuration file 
/home/gsmith/pgwork/data/include-exists/missing.conf: No such file or 
directory
FATAL:  configuration file 
/home/gsmith/pgwork/data/include-exists/postgresql.conf contains errors


And new behavior:

$ vi $PGDATA/postgresql.conf
$ tail -n 1 postgresql.conf
include_if_exists 'missing.conf'
$ start
server starting
$ tail $PGLOG
LOG:  skipping missing configuration file 
/home/gsmith/pgwork/data/include-exists/missing.conf

LOG:  database system was shut down at 2011-12-15 06:48:46 EST
LOG:  database system is ready to accept connections

--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index d1e628f..0cc3296 100644
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
*** include 'filename'
*** 91,96 
--- 91,108 
  
 para
  indexterm
+  primaryliteralinclude_if_exists//primary
+  secondaryin configuration file/secondary
+ /indexterm
+ Use the same approach as the literalinclude/ directive, continuing
+ normally if the file does not exist.  A regular literalinclude/
+ will stop with an error if the referenced file is missing, while
+ literalinclude_if_exists/ does not.  A warning about the missing
+ file will be logged.
+/para
+ 
+para
+ indexterm
   primarySIGHUP/primary
  /indexterm
  The configuration file is reread whenever the main server process receives a
diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index a094c7a..6ba130c 100644
*** a/src/backend/utils/misc/guc-file.l
--- b/src/backend/utils/misc/guc-file.l
*** ProcessConfigFile(GucContext context)
*** 129,135 
  	/* Parse the file into a list of option names and values */
  	head = tail = NULL;
  
! 	if (!ParseConfigFile(ConfigFileName, NULL, 0, elevel, head, tail))
  	{
  		/* Syntax error(s) detected in the file, so bail out */
  		error = true;
--- 129,135 
  	/* Parse the file into a list of option names and values */
  	head = tail = NULL;
  
! 	if (!ParseConfigFile(ConfigFileName, NULL, true, 0, elevel, head, tail))
  	{
  		/* Syntax error(s) detected in the file, so bail out */
  		error = true;
*** ProcessConfigFile(GucContext context)
*** 363,369 
   * and absolute-ifying the path name if necessary.
   */
  bool
! ParseConfigFile(const char *config_file, const char *calling_file,
  int depth, int elevel,
  ConfigVariable **head_p,
  ConfigVariable **tail_p)
--- 363,369 
   * and absolute-ifying the path name if necessary.
   */
  bool
! ParseConfigFile(const char *config_file, const char *calling_file, bool strict,
  int depth, int elevel,
  ConfigVariable **head_p,
  ConfigVariable **tail_p)
*** ParseConfigFile(const char *config_file,
*** 414,424 
  	fp = AllocateFile(config_file, r);
  	if (!fp)
  	{
! 		ereport(elevel,
! (errcode_for_file_access(),
!  errmsg(could not open configuration file \%s\: %m,
! 		config_file)));
! 		return false;
  	}
  
  	OK = ParseConfigFp(fp, config_file, depth, elevel, head_p, tail_p);
--- 414,430 
  	fp = AllocateFile(config_file, r);
  	if (!fp)
  	{
! 		if (strict)
! 		{
! 			ereport(elevel,
! 	(errcode_for_file_access(),
! 	 errmsg(could not open configuration file \%s\: %m,
! 			config_file)));
! 			return false;
! 		}
! 
! 		elog(LOG, skipping missing configuration file \%s\, config_file);
! 		return OK;
  	}
  
  	OK = ParseConfigFp(fp, config_file, depth, elevel, head_p, tail_p);
*** ParseConfigFp(FILE *fp, const char *conf
*** 512,518 
  		}
  
  		/* OK, process the option name and value */
! 		if (guc_name_compare(opt_name, include) == 0)
  		{
  			/*
  			 * An include directive isn't a variable and should be processed
--- 518,541 
  		}
  
  		/* OK, process the option name and value */
! 		if (guc_name_compare(opt_name, include_if_exists) == 0)
! 		{
! 			/*
! 			 * An include_if_exists directive isn't a variable and should be
! 			 * processed immediately.
! 			 */
! 			unsigned int save_ConfigFileLineno = ConfigFileLineno;
! 
! 			if (!ParseConfigFile(opt_value, config_file, false,
! 			

Re: [HACKERS] IDLE in transaction introspection

2011-12-15 Thread Greg Smith
This patch seems closing in on being done, but it's surely going to take 
at least one more round of review to make sure all the naming and 
documentation is up right.  I can work on that again whenever Scott gets 
another version necessary, and Magnus is already poking around with an 
eye toward possibly committing the result.  I think we can make this one 
returned with feedback for this CF, but encourage Scott to resubmit as 
early as feasible before the next one.  With some good luck, we might 
even close this out before that one even starts.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us


--
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] Measuring relation free space

2011-12-15 Thread Greg Smith

On 11/28/2011 05:40 AM, Greg Smith wrote:
Ignoring fillfactor seems to have even more downsides as I see it.  
Certainly deserves a doc improvement, as well as fixing the 
description of the value so it's clearly a ratio rather than a true 
percentage.


So:  I'm very clear on what to do here now:

-Make the computation be in units that match it documetnation
-Take a look at other index types, as well as TOAST, at least to get the 
easy ones right.

-Fully confirm the extension upgrade logic works as hoped

That's the must do stuff.  Then there's two more features to consider 
and do something with if sensible:


-Double check whether there is really a useful role in using 
pg_freespace here.  I don't think the numbers will be as good, but maybe 
we don't care.
-Once the above is all sorted, add a second UI that samples some pages 
and extrapolates, ANALYZE-style, rather than hitting everything.


This ones leaves as returned with feedback, feeling pretty good it will 
be whipped into good shape for the last 9.2 CommitFest.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us


--
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 : Allow toast tables to be moved to a different tablespace

2011-12-15 Thread Greg Smith

On 12/13/2011 12:29 PM, Julien Tachoires wrote:

2011/12/13 Robert Haasrobertmh...@gmail.com:
   

On Tue, Dec 13, 2011 at 12:02 PM, Julien Tachoiresjul...@gmail.com  wrote:
 

Right, it seems to happen when the destination tablespace is the same
as the database's tbs, because, in this case, relation's tbs is set to
InvalidOid :
src/backend/commands/tablecmds.c line 8342

+   rd_rel-reltablespace = (newTableSpace == MyDatabaseTableSpace) ?
InvalidOid : newTableSpace;

Why don't just asign newTableSpace value here ?
   

When a relation is stored in the default tablespace, we always record
that in the system catalogs as InvalidOid.  Otherwise, if the
database's default tablespace were changed, things would break.
 

OK, considering that, I don't see any way to handle the case raised by Jaime :(
   


So we have a problem here:  there's a case that's messy to handle.  And 
there's really a large issue hanging over this whole patch, which is 
that it needs a better explanation of what exactly it's going to get 
used for.  Especially if the implementation gets more complicated, we'd 
want to see a clear reason to use this feature.  And that's not really 
clear.


If you can return with an update that perhaps finds a way to work around 
this OID issue, please re-submit that.  And if you can explain some more 
about where you think this feature is useful, more information on that 
would be helpful.  Since this isn't going to get committed soon, I'm 
going to mark it returned with feedback for now.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us


--
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: CHECK FUNCTION statement

2011-12-15 Thread Pavel Stehule
Hello

one small update - better emulation of environment for security
definer functions

Regards

Pavel


check_function-2011-12-15-2.diff.gz
Description: GNU Zip compressed data

-- 
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] Race condition in HEAD, possibly due to PGPROC splitup

2011-12-15 Thread Simon Riggs
On Wed, Dec 14, 2011 at 3:20 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Pavan Deolasee pavan.deola...@gmail.com writes:
 Looking at CommitTransaction(), it seems quite clear to me that we
 call ProcArrayEndTransaction() before releasing the locks held by the
 transaction. So its quite possible that when
 GetRunningTransactionLocks goes through the list of currently held
 locks, the pgxact-xid is already cleared. This seems to a old bug to
 me and not related to PGXACT work.

 Hm.  So maybe the correct fix is to deem the lock already released
 if we get zero when we read the xid?  It's not clear to me what the
 requirements for GetRunningTransactionLocks actually are, but if it's
 okay for it to think a lock is released slightly ahead of when the
 rest of the system thinks so, that would work.

Attached patch closes both race conditions:
* where xid is zero
* where xid is non-zero yet WAL record for the commit of xid wins race
ahead of the WAL record for locks

Patch fixes it in backwards compatible way.

No version increments.

Patch head, 9.1 and 9.0.

Will wait a couple of days for additional testing.

Comments?

-- 
 Simon Riggs   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services
*** a/src/backend/storage/ipc/procarray.c
--- b/src/backend/storage/ipc/procarray.c
***
*** 499,505  ProcArrayApplyRecoveryInfo(RunningTransactions running)
  	 * Remove stale transactions, if any.
  	 */
  	ExpireOldKnownAssignedTransactionIds(running-oldestRunningXid);
! 	StandbyReleaseOldLocks(running-oldestRunningXid);
  
  	/*
  	 * If our snapshot is already valid, nothing else to do...
--- 499,505 
  	 * Remove stale transactions, if any.
  	 */
  	ExpireOldKnownAssignedTransactionIds(running-oldestRunningXid);
! 	StandbyReleaseOldLocks(running-xcnt, running-xids);
  
  	/*
  	 * If our snapshot is already valid, nothing else to do...
***
*** 554,565  ProcArrayApplyRecoveryInfo(RunningTransactions running)
  	 */
  
  	/*
- 	 * Release any locks belonging to old transactions that are not running
- 	 * according to the running-xacts record.
- 	 */
- 	StandbyReleaseOldLocks(running-nextXid);
- 
- 	/*
  	 * Nobody else is running yet, but take locks anyhow
  	 */
  	LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
--- 554,559 
*** a/src/backend/storage/ipc/standby.c
--- b/src/backend/storage/ipc/standby.c
***
*** 525,531  StandbyAcquireAccessExclusiveLock(TransactionId xid, Oid dbOid, Oid relOid)
  	LOCKTAG		locktag;
  
  	/* Already processed? */
! 	if (TransactionIdDidCommit(xid) || TransactionIdDidAbort(xid))
  		return;
  
  	elog(trace_recovery(DEBUG4),
--- 525,533 
  	LOCKTAG		locktag;
  
  	/* Already processed? */
! 	if (!TransactionIdIsValid(xid) ||
! 		TransactionIdDidCommit(xid) ||
! 		TransactionIdDidAbort(xid))
  		return;
  
  	elog(trace_recovery(DEBUG4),
***
*** 607,640  StandbyReleaseLockTree(TransactionId xid, int nsubxids, TransactionId *subxids)
  }
  
  /*
!  * StandbyReleaseLocksMany
!  *		Release standby locks held by XIDs  removeXid
!  *
!  * If keepPreparedXacts is true, keep prepared transactions even if
!  * they're older than removeXid
   */
! static void
! StandbyReleaseLocksMany(TransactionId removeXid, bool keepPreparedXacts)
  {
  	ListCell   *cell,
  			   *prev,
  			   *next;
  	LOCKTAG		locktag;
  
- 	/*
- 	 * Release all matching locks.
- 	 */
  	prev = NULL;
  	for (cell = list_head(RecoveryLockList); cell; cell = next)
  	{
  		xl_standby_lock *lock = (xl_standby_lock *) lfirst(cell);
  
  		next = lnext(cell);
  
! 		if (!TransactionIdIsValid(removeXid) || TransactionIdPrecedes(lock-xid, removeXid))
  		{
- 			if (keepPreparedXacts  StandbyTransactionIdIsPrepared(lock-xid))
- continue;
  			elog(trace_recovery(DEBUG4),
   releasing recovery lock: xid %u db %u rel %u,
   lock-xid, lock-dbOid, lock-relOid);
--- 609,691 
  }
  
  /*
!  * Called at end of recovery and when we see a shutdown checkpoint.
   */
! void
! StandbyReleaseAllLocks(void)
! {
! 	ListCell   *cell,
! 			   *prev,
! 			   *next;
! 	LOCKTAG		locktag;
! 
! 	elog(trace_recovery(DEBUG2), release all standby locks);
! 
! 	prev = NULL;
! 	for (cell = list_head(RecoveryLockList); cell; cell = next)
! 	{
! 		xl_standby_lock *lock = (xl_standby_lock *) lfirst(cell);
! 
! 		next = lnext(cell);
! 
! 		elog(trace_recovery(DEBUG4),
! 			 releasing recovery lock: xid %u db %u rel %u,
! 			 lock-xid, lock-dbOid, lock-relOid);
! 		SET_LOCKTAG_RELATION(locktag, lock-dbOid, lock-relOid);
! 		if (!LockRelease(locktag, AccessExclusiveLock, true))
! 			elog(LOG,
!  RecoveryLockList contains entry for lock no longer recorded by lock manager: xid %u database %u relation %u,
!  lock-xid, lock-dbOid, lock-relOid);
! 		RecoveryLockList = list_delete_cell(RecoveryLockList, cell, prev);
! 		pfree(lock);
! 	}
! }
! 
! /*
!  * StandbyReleaseOldLocks
!  *		Release standby locks held by XIDs that aren't 

[HACKERS] why do we need create tuplestore for each fetch?

2011-12-15 Thread 高增琦
Hi everyone,

I found this several days ago when I try to debug a fetch of cursor.
And I have sent a mail to this list, but no one reply...
Maybe this is a very simple problem, please help me, thanks a lot...

Here is the example:
create table t (a int);
insert into t values (1),(3),(5),(7),(9);
insert into t select a+1 from t;
begin;
declare c cursor for select * from t order by a;
fetch 3 in c;
fetch 3 in c;
fetch 3 in c;

In 'PortalRun', a fetch stmt will be treated with PORTAL_UTIL_SELECT,
and then a tuplestore will be created in 'FillPortalStore' in the
fetch stmt's portal.

In 'FillPortalStore', all result will be store at that tuplestore,
Then, go back to 'PortalRun'; next,  'PortalRunSelect' will send this
results to client...

My problem is: why do we need create that tuplestore as an
middle storeage? why do not we just send these result to clent
at the first time?

Thank you very much.

-- 
GaoZengqi
pgf...@gmail.com
zengqi...@gmail.com


Re: [HACKERS] patch : Allow toast tables to be moved to a different tablespace

2011-12-15 Thread Alvaro Herrera

Excerpts from Julien Tachoires's message of mar dic 13 14:29:56 -0300 2011:
 2011/12/13 Robert Haas robertmh...@gmail.com:
  On Tue, Dec 13, 2011 at 12:02 PM, Julien Tachoires jul...@gmail.com wrote:
  Right, it seems to happen when the destination tablespace is the same
  as the database's tbs, because, in this case, relation's tbs is set to
  InvalidOid :
  src/backend/commands/tablecmds.c line 8342
 
  +       rd_rel-reltablespace = (newTableSpace == MyDatabaseTableSpace) ?
  InvalidOid : newTableSpace;
 
  Why don't just asign newTableSpace value here ?
 
  When a relation is stored in the default tablespace, we always record
  that in the system catalogs as InvalidOid.  Otherwise, if the
  database's default tablespace were changed, things would break.
 
 OK, considering that, I don't see any way to handle the case raised by Jaime 
 :(

Uhm, surely you could compare the original toast tablespace to the heap
tablespace, and if they differ, handle appropriately when creating the
new toast table?  Just pass down the toast tablespace into
AlterTableCreateToastTable, instead of having it assume that
rel-rd_rel-relnamespace is sufficient.  This should be done in all
cases where a toast tablespace is created, which shouldn't be more than
a handful of them.

-- 
Á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] Moving more work outside WALInsertLock

2011-12-15 Thread Heikki Linnakangas
I've been looking at various ways to make WALInsertLock less of a 
bottleneck on multi-CPU servers. The key is going to be to separate the 
two things that are done while holding the WALInsertLock: a) allocating 
the required space in the WAL, and b) calculating the CRC of the record 
header and copying the data to the WAL page. a) needs to be serialized, 
but b) could be done in parallel.


I've been experimenting with different approaches to do that, but one 
thing is common among all of them: you need to know the total amount of 
WAL space needed for the record, including backup blocks, before you 
take the lock. So, here's a patch to move things around in XLogInsert() 
a bit, to accomplish that.


This patch doesn't seem to have any performance or scalability impact. I 
must admit I expected it to give a tiny gain in scalability by 
shortening the time WALInsertLock is held by a few instructions, but I 
can't measure any. But IMO it makes the code more readable, so this is 
worthwhile for that reason alone.


--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
***
*** 690,695  XLogInsert(RmgrId rmid, uint8 info, XLogRecData *rdata)
--- 690,698 
  	uint32		freespace;
  	int			curridx;
  	XLogRecData *rdt;
+ 	XLogRecData *rdt_cur;
+ 	XLogRecData	*rdt_bkp_first;
+ 	XLogRecData *rdt_bkp_last;
  	Buffer		dtbuf[XLR_MAX_BKP_BLOCKS];
  	bool		dtbuf_bkp[XLR_MAX_BKP_BLOCKS];
  	BkpBlock	dtbuf_xlg[XLR_MAX_BKP_BLOCKS];
***
*** 704,709  XLogInsert(RmgrId rmid, uint8 info, XLogRecData *rdata)
--- 707,713 
  	bool		updrqst;
  	bool		doPageWrites;
  	bool		isLogSwitch = (rmid == RM_XLOG_ID  info == XLOG_SWITCH);
+ 	uint8		info_final;
  
  	/* cross-check on whether we should be here or not */
  	if (!XLogInsertAllowed())
***
*** 727,738  XLogInsert(RmgrId rmid, uint8 info, XLogRecData *rdata)
  	}
  
  	/*
! 	 * Here we scan the rdata chain, determine which buffers must be backed
! 	 * up, and compute the CRC values for the data.  Note that the record
! 	 * header isn't added into the CRC initially since we don't know the final
! 	 * length or info bits quite yet.  Thus, the CRC will represent the CRC of
! 	 * the whole record in the order rdata, then backup blocks, then record
! 	 * header.
  	 *
  	 * We may have to loop back to here if a race condition is detected below.
  	 * We could prevent the race by doing all this work while holding the
--- 731,738 
  	}
  
  	/*
! 	 * Here we scan the rdata chain to determine which buffers must be backed
! 	 * up.
  	 *
  	 * We may have to loop back to here if a race condition is detected below.
  	 * We could prevent the race by doing all this work while holding the
***
*** 746,751  XLogInsert(RmgrId rmid, uint8 info, XLogRecData *rdata)
--- 746,752 
  	 * over the chain later.
  	 */
  begin:;
+ 	info_final = info;
  	for (i = 0; i  XLR_MAX_BKP_BLOCKS; i++)
  	{
  		dtbuf[i] = InvalidBuffer;
***
*** 760,766  begin:;
  	 */
  	doPageWrites = fullPageWrites || Insert-forcePageWrites;
  
- 	INIT_CRC32(rdata_crc);
  	len = 0;
  	for (rdt = rdata;;)
  	{
--- 761,766 
***
*** 768,774  begin:;
  		{
  			/* Simple data, just include it */
  			len += rdt-len;
- 			COMP_CRC32(rdata_crc, rdt-data, rdt-len);
  		}
  		else
  		{
--- 768,773 
***
*** 779,790  begin:;
  {
  	/* Buffer already referenced by earlier chain item */
  	if (dtbuf_bkp[i])
  		rdt-data = NULL;
  	else if (rdt-data)
- 	{
  		len += rdt-len;
- 		COMP_CRC32(rdata_crc, rdt-data, rdt-len);
- 	}
  	break;
  }
  if (dtbuf[i] == InvalidBuffer)
--- 778,789 
  {
  	/* Buffer already referenced by earlier chain item */
  	if (dtbuf_bkp[i])
+ 	{
  		rdt-data = NULL;
+ 		rdt-len = 0;
+ 	}
  	else if (rdt-data)
  		len += rdt-len;
  	break;
  }
  if (dtbuf[i] == InvalidBuffer)
***
*** 796,807  begin:;
  	{
  		dtbuf_bkp[i] = true;
  		rdt-data = NULL;
  	}
  	else if (rdt-data)
- 	{
  		len += rdt-len;
- 		COMP_CRC32(rdata_crc, rdt-data, rdt-len);
- 	}
  	break;
  }
  			}
--- 795,804 
  	{
  		dtbuf_bkp[i] = true;
  		rdt-data = NULL;
+ 		rdt-len = 0;
  	}
  	else if (rdt-data)
  		len += rdt-len;
  	break;
  }
  			}
***
*** 816,854  begin:;
  	}
  
  	/*
! 	 * Now add the backup block headers and data into the CRC
  	 */
  	for (i = 0; i  XLR_MAX_BKP_BLOCKS; i++)
  	{
! 		if (dtbuf_bkp[i])
  		{
! 			BkpBlock   *bkpb = (dtbuf_xlg[i]);
! 			char	   *page;
! 
! 			COMP_CRC32(rdata_crc,
! 	   (char *) bkpb,
! 	   sizeof(BkpBlock));
! 			page = (char *) BufferGetBlock(dtbuf[i]);
! 			if (bkpb-hole_length == 0)
! 			{
! COMP_CRC32(rdata_crc,
! 		   page,
! 		   

Re: [HACKERS] Race condition in HEAD, possibly due to PGPROC splitup

2011-12-15 Thread Robert Haas
On Thu, Dec 15, 2011 at 8:19 AM, Simon Riggs si...@2ndquadrant.com wrote:
 Comments?

I think there is a small bug here:

+   TransactionId xid = pgxact-xid;
+
+   /*
+* Don't record locks for transactions if we know they 
have already
+* issued their WAL record for commit but not yet 
released lock.
+* It is still possible that we see locks held by 
already complete
+* transactions, if they haven't yet zeroed their xids.
+*/
+   if (!TransactionIdIsValid(xid))
+   continue;

accessExclusiveLocks[index].xid = pgxact-xid;
accessExclusiveLocks[index].dbOid = 
lock-tag.locktag_field1;

...because you're fetching pgxact-xid, testing whether it's valid,
and then fetching it again.  It could change in the meantime.  You
probably want to change the assignment to read:

accessExclusiveLocks[index].xid = xid;

Also, we should probably change this pointer to be declared volatile:

PGXACT *pgxact = 
ProcGlobal-allPgXact[proc-pgprocno];

Otherwise, I think the compiler might get cute and decide to fetch the
xid twice anyway, effectively undoing our attempt to pull it into a
local variable.

I think this comment could be clarified in some way, to make it more
clear that we had a bug at one point, and it was fixed - the from a
time when they were possible language wouldn't be entirely clear to
me after the fact:

+  * Zero xids should no longer be possible, but we may be replaying WAL
+  * from a time when they were possible.

It would probably make sense to break out of this loop if a match is found:

!   for (i = 0; i  nxids; i++)
!   {
!   if (lock-xid == xids[i])
!   found = true;
!   }

I'm not sure I fully understand the rest of this, but those are the
only things I noticed on a read-through.

-- 
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 more work outside WALInsertLock

2011-12-15 Thread Andres Freund
On Thursday, December 15, 2011 02:51:33 PM Heikki Linnakangas wrote:
 I've been looking at various ways to make WALInsertLock less of a
 bottleneck on multi-CPU servers. The key is going to be to separate the
 two things that are done while holding the WALInsertLock: a) allocating
 the required space in the WAL, and b) calculating the CRC of the record
 header and copying the data to the WAL page. a) needs to be serialized,
 but b) could be done in parallel.
 
 I've been experimenting with different approaches to do that, but one
 thing is common among all of them: you need to know the total amount of
 WAL space needed for the record, including backup blocks, before you
 take the lock. So, here's a patch to move things around in XLogInsert()
 a bit, to accomplish that.
 
 This patch doesn't seem to have any performance or scalability impact. I
 must admit I expected it to give a tiny gain in scalability by
 shortening the time WALInsertLock is held by a few instructions, but I
 can't measure any. But IMO it makes the code more readable, so this is
 worthwhile for that reason alone.
Thats great! I did (or at least tried) something similar when I was playing 
around with another crc32 implementation (which I plan to finish sometime). My 
changes where totally whacky but I got rather big improvements when changing 
the crc computation from incremental to one big swoop.
I started to hack up an api which buffered xlog data in statically sized buffer 
in each backend and only submitted that every now and then. Never got that to 
actually work correctly in more than the simplest cases though ;). In many 
cases were taking the wal insert lock way to often during a single insert... 
(you obviously know that...)

Andres


-- 
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] Command Triggers

2011-12-15 Thread Robert Haas
On Wed, Dec 14, 2011 at 5:44 PM, Dimitri Fontaine
dimi...@2ndquadrant.fr wrote:
 Robert Haas robertmh...@gmail.com writes:
 I can get behind this argument: force all stuff through ProcessUtility
 for regularity, and not necessarily in the first patch for this feature.

 That seems like a pretty heavy dependency on an implementation detail
 that we might want to change at some point.

 Given ProcessUtility_hook, how much of an implementation detail rather
 than a public API are we talking about?

I think that a hook and an SQL command are not on the same level.
Hooks are not documented; SQL commands are.  You may, of course,
disagree.

But the basic point is that it seems pretty weird to say, on the one
hand, these are command triggers.  They fire when you execute a
command.  So the CREATE TABLE trigger will fire when someone says
CREATE TABLE.  But then we say, oh, well if you use CREATE SCHEMA foo
CREATE TABLE blah ... we will fire the trigger anyway.  Now it's not a
command trigger any more; it's a trigger that fires when you perform a
certain operation - e.g. create a table.  Unless, of course, you
create the table using CREATE TABLE AS SELECT or SELECT .. INTO.  Then
it doesn't fire.  Unless we decide to make those utility commands,
which I think was just recently under discussion, in which case it
will suddenly start firing for those operations.  So now something
that is, right now, an essentially an implementation detail which we
can rearrange as we like turns into a user-visible behavior change.
And what if some day we want to change it back?

I think it would be a much better idea to decree from the beginning
that we're trapping the *operation* of creating a table, rather than
the *command* create table.  Then, the behavior is clear and
well-defined from day one, and it doesn't depend on how we happen to
implement things internally right at the moment.  If there's some way
of creating a table without firing the trigger, it's a bug.  If
there's some way of firing the trigger without attempting to create a
table, it's a bug.  That might require some more thought about what
information to pass to the trigger function (e.g. if it's a SELECT ..
INTO, you're not going to have pregenerated SQL that starts with the
words CREATE TABLE) but the fact that gives much more clear
definition to the core feature seems like a big plus in my book.

-- 
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] unite recovery.conf and postgresql.conf

2011-12-15 Thread Robert Haas
On Wed, Dec 14, 2011 at 8:56 PM, Josh Berkus j...@agliodbs.com wrote:
 So for streaming replication, will I need to have a standby.enabled
 file, or will there be a parameter in postgresql.conf (or included
 files) which controls whether or not that server is a standby, available?

 In the best of all possible worlds, I'd really like to have a GUC which
 100% controls whether or not the current server is a standby.

I think that would be a bad idea, because then how would pg_ctl
promote work?  It'd have to permanently change the value of that GUC,
which means rewriting a postgresql.conf file with an arbitrary forest
of comments and include files, and we can't do that now or, probably,
ever.  At least not reliably enough for this kind of use case.  I
think what Greg is going for is this:

1. Get rid of recovery.conf - error out if it is seen
2. For each parameter that was previously a recovery.conf parameter,
make it a GUC
3. For the parameter that was does recovery.conf exist?, replace it
with does standby.enabled exist?.

IMHO, as Greg says, that's as much change as we can cope with in one release.

-- 
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] Race condition in HEAD, possibly due to PGPROC splitup

2011-12-15 Thread Simon Riggs
On Thu, Dec 15, 2011 at 2:13 PM, Robert Haas robertmh...@gmail.com wrote:
 On Thu, Dec 15, 2011 at 8:19 AM, Simon Riggs si...@2ndquadrant.com wrote:
 Comments?

 I think there is a small bug here:

 +                       TransactionId xid = pgxact-xid;
 +
 +                       /*
 +                        * Don't record locks for transactions if we know 
 they have already
 +                        * issued their WAL record for commit but not yet 
 released lock.
 +                        * It is still possible that we see locks held by 
 already complete
 +                        * transactions, if they haven't yet zeroed their 
 xids.
 +                        */
 +                       if (!TransactionIdIsValid(xid))
 +                               continue;

                        accessExclusiveLocks[index].xid = pgxact-xid;
                        accessExclusiveLocks[index].dbOid = 
 lock-tag.locktag_field1;

 ...because you're fetching pgxact-xid, testing whether it's valid,
 and then fetching it again.  It could change in the meantime.  You
 probably want to change the assignment to read:

                        accessExclusiveLocks[index].xid = xid;

 Also, we should probably change this pointer to be declared volatile:

                        PGXACT     *pgxact = 
 ProcGlobal-allPgXact[proc-pgprocno];

 Otherwise, I think the compiler might get cute and decide to fetch the
 xid twice anyway, effectively undoing our attempt to pull it into a
 local variable.

 I think this comment could be clarified in some way, to make it more
 clear that we had a bug at one point, and it was fixed - the from a
 time when they were possible language wouldn't be entirely clear to
 me after the fact:

 +  * Zero xids should no longer be possible, but we may be replaying WAL
 +  * from a time when they were possible.

 It would probably make sense to break out of this loop if a match is found:

 !                       for (i = 0; i  nxids; i++)
 !                       {
 !                               if (lock-xid == xids[i])
 !                                       found = true;
 !                       }

 I'm not sure I fully understand the rest of this, but those are the
 only things I noticed on a read-through.

Thanks, useful changes.

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

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


Re: [HACKERS] Command Triggers

2011-12-15 Thread Andres Freund
On Thursday, December 15, 2011 03:36:25 PM Robert Haas wrote:
 On Wed, Dec 14, 2011 at 5:44 PM, Dimitri Fontaine
 
 dimi...@2ndquadrant.fr wrote:
  Robert Haas robertmh...@gmail.com writes:
  I can get behind this argument: force all stuff through ProcessUtility
  for regularity, and not necessarily in the first patch for this
  feature.
  That seems like a pretty heavy dependency on an implementation detail
  that we might want to change at some point.
  Given ProcessUtility_hook, how much of an implementation detail rather
  than a public API are we talking about?
 I think that a hook and an SQL command are not on the same level.
 Hooks are not documented; SQL commands are.  You may, of course,
 disagree.
I am not disagreeing.

 But the basic point is that it seems pretty weird to say, on the one
 hand, these are command triggers.  They fire when you execute a
 command.  So the CREATE TABLE trigger will fire when someone says
 CREATE TABLE.  But then we say, oh, well if you use CREATE SCHEMA foo
 CREATE TABLE blah ... we will fire the trigger anyway.  Now it's not a
 command trigger any more; it's a trigger that fires when you perform a
 certain operation - e.g. create a table.  Unless, of course, you
 create the table using CREATE TABLE AS SELECT or SELECT .. INTO.  Then
 it doesn't fire.  Unless we decide to make those utility commands,
 which I think was just recently under discussion, in which case it
 will suddenly start firing for those operations. 
Command triggers were the initial reason for me doing that conversion.

 So now something
 that is, right now, an essentially an implementation detail which we
 can rearrange as we like turns into a user-visible behavior change.
 And what if some day we want to change it back?

 I think it would be a much better idea to decree from the beginning
 that we're trapping the *operation* of creating a table, rather than
 the *command* create table.  Then, the behavior is clear and
 well-defined from day one, and it doesn't depend on how we happen to
 implement things internally right at the moment.
I am with you there.

 If there's some way
 of creating a table without firing the trigger, it's a bug.  If
 there's some way of firing the trigger without attempting to create a
 table, it's a bug.
Again.

 That might require some more thought about what
 information to pass to the trigger function (e.g. if it's a SELECT ..
 INTO, you're not going to have pregenerated SQL that starts with the
 words CREATE TABLE) but the fact that gives much more clear
 definition to the core feature seems like a big plus in my book.
Not sure what youre getting at here? The command tag in Dim's patch is alread 
independent of the form of actual statement that was run.

A big +1 on the general direction of this email. I dislike reducing the scope 
of command triggers to top level commands run by the actual user because imo 
that would reduce the possible scope of the patch rather much.

On the other hand I think delivering a complete patch covering just about 
anything triggered anywhere is not really realistic. Not sure whats the best 
way to continue is.


Andres

-- 
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] splitting plpython into smaller parts

2011-12-15 Thread Peter Eisentraut
How to people feel about naming the files (as proposed)

! OBJS = plpython.o plpython_io.o plpython_procedure.o plpython_exec.o \
!plpython_plpy.o plpython_spi.o plpython_result.o plpython_cursor.o \
!plpython_plan.o plpython_subtransaction.o plpython_functions.o \
!plpython_elog.o

vs. say

! OBJS = main.o io.o procedure.o exec.o plpy.o spi.o result.o cursor.o \
!plan.o subtransaction.o functions.o elog.o

?



-- 
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] splitting plpython into smaller parts

2011-12-15 Thread Alvaro Herrera

Excerpts from Peter Eisentraut's message of jue dic 15 12:00:13 -0300 2011:
 How to people feel about naming the files (as proposed)
 
 ! OBJS = plpython.o plpython_io.o plpython_procedure.o plpython_exec.o \
 !plpython_plpy.o plpython_spi.o plpython_result.o plpython_cursor.o \
 !plpython_plan.o plpython_subtransaction.o plpython_functions.o \
 !plpython_elog.o
 
 vs. say
 
 ! OBJS = main.o io.o procedure.o exec.o plpy.o spi.o result.o cursor.o \
 !plan.o subtransaction.o functions.o elog.o
 
 ?

I find the extra prefix unnecessary and ugly; if we had to had a
prefix, I'd choose a shorter one (maybe py instead of plpython_).

-- 
Á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] splitting plpython into smaller parts

2011-12-15 Thread Tom Lane
Alvaro Herrera alvhe...@commandprompt.com writes:
 Excerpts from Peter Eisentraut's message of jue dic 15 12:00:13 -0300 2011:
 How to people feel about naming the files (as proposed)
 
 ! OBJS = plpython.o plpython_io.o plpython_procedure.o plpython_exec.o \
 !plpython_plpy.o plpython_spi.o plpython_result.o plpython_cursor.o \
 !plpython_plan.o plpython_subtransaction.o plpython_functions.o \
 !plpython_elog.o
 
 vs. say
 
 ! OBJS = main.o io.o procedure.o exec.o plpy.o spi.o result.o cursor.o \
 !plan.o subtransaction.o functions.o elog.o
 
 ?

 I find the extra prefix unnecessary and ugly; if we had to had a
 prefix, I'd choose a shorter one (maybe py instead of plpython_).

+1 for a prefix, mainly because the shorter names duplicate some
names already in use elsewhere in our tree.  But I agree with Alvaro
that py would be sufficient.

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 more work outside WALInsertLock

2011-12-15 Thread Tom Lane
Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes:
 I've been experimenting with different approaches to do that, but one 
 thing is common among all of them: you need to know the total amount of 
 WAL space needed for the record, including backup blocks, before you 
 take the lock. So, here's a patch to move things around in XLogInsert() 
 a bit, to accomplish that.

This patch may or may not be useful, but this description of it is utter
nonsense, because we already do compute that before taking the lock.
Please try again to explain what you're doing?

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] Command Triggers

2011-12-15 Thread Dimitri Fontaine
Hi,

Thank your Robert for this continued review, I think we're making good
progress in a community discussion that needs to happen!

Robert Haas robertmh...@gmail.com writes:
 Given ProcessUtility_hook, how much of an implementation detail rather
 than a public API are we talking about?

 I think that a hook and an SQL command are not on the same level.
 Hooks are not documented; SQL commands are.  You may, of course,
 disagree.

Mmmm, yes.  I think that we should document hooks, and I'm going to
propose soon a pg_extension_module() SRF that lists all currently loaded
modules and which extension implements them, and I've begun thinking
about what it would take to be able to list what hooks each module is
implementing.

That would require an hook registration API, and would allow a much
easier security auditing of a setup you don't know beforehand.

So I think that hooks not being documented is an implementation detail
here :)

 But the basic point is that it seems pretty weird to say, on the one
 hand, these are command triggers.  They fire when you execute a
 command.  So the CREATE TABLE trigger will fire when someone says
 CREATE TABLE.  But then we say, oh, well if you use CREATE SCHEMA foo
 CREATE TABLE blah ... we will fire the trigger anyway.  Now it's not a

Yes, this CREATE SCHEMA any create command is weird, and unique, so
it's not that difficult to document, I think.  And if we fix things by
having all subcommands go through ProcessUtility and command triggers,
then it's easy to document as the new rule.

My documentation idea would then be explaining what is a command and
what is a subcommand, and then the current rule (command triggers do not
support subcommands) and then the exception to that rule (CREATE SCHEMA
subcommands do, in fact, support command triggers).

If we decide that we should in fact support (nested) subcommands in
command triggers, then we will be in a position to prepare a patch
implementing that with a documentation change that the rule is now that
command triggers do in fact support subcommands.

When the command trigger is called on a subcommand, I think you will
want both the main command string and the subcommand string, and we have
to research if what you need isn't a whole stack of commands, because
I'm not sure you can only have 1 level deep nesting here.

That would be done with a special API for the trigger functions, and
with a specific syntax, because it seems to me that having a trigger
code that is only ever called on a top-level command is a good thing to
have.

  create trigger foo after command CREATE TABLE … 
  create trigger foo after top level command CREATE TABLE … 
  create trigger foo after nested command CREATE TABLE … 

Either we add support for that kind of syntax now (and not implementing
it in 9.3 would seem weird as hell) or we instruct pg_dump and
pg_upgrade to switch from current syntax to the new one (add in the “top
level” keywords) when we do implement the feature down the road.

 command trigger any more; it's a trigger that fires when you perform a
 certain operation - e.g. create a table.  Unless, of course, you
 create the table using CREATE TABLE AS SELECT or SELECT .. INTO.  Then
 it doesn't fire.  Unless we decide to make those utility commands,
 which I think was just recently under discussion, in which case it
 will suddenly start firing for those operations.  So now something

Andres has been sending a complete patch to fix that old oddity of the
parser.  And again, that's a very narrow exception to the usual state of
things, and as such, easy to document in the list of things that don't
fall into the general rule.

Even when fixed, though, you have 3 different command tags to deal with,
and we identify the command triggers on command tags, so that a command
trigger on CREATE TABLE will not be called when doing SELECT INTO, nor
when doing CREATE TABLE AS.

Also, I think that the command triggers in 9.2 will not address all and
any command that PostgreSQL offers (think “alter operator family”), so
that we will need to maintain an exhaustive list of supported commands,
identified by command tags.

 that is, right now, an essentially an implementation detail which we
 can rearrange as we like turns into a user-visible behavior change.
 And what if some day we want to change it back?

Yes, we need to make a decision about that now. Do we want any
“operation” to go through ProcessUtility so that hooks and command
triggers can get called?

I think it's a good idea in the long run, and Alvaro seems to be
thinking it is too. That entails changing the backend code to build a
Statement then call ProcessUtility on it rather than calling the
internal functions directly, and that also means some more DDL are
subject to being logged on the server logs. Removing those
“cross-modules” #include might be a good think in the long run though.

And we don't have to do that unless we want to make subcommands
available to ProcessUtility_hook and 

Re: [HACKERS] Command Triggers

2011-12-15 Thread Andres Freund
On Thursday, December 15, 2011 04:53:15 PM Dimitri Fontaine wrote:
 Hi,
 
 Thank your Robert for this continued review, I think we're making good
 progress in a community discussion that needs to happen!
 
 Robert Haas robertmh...@gmail.com writes:
  Given ProcessUtility_hook, how much of an implementation detail rather
  than a public API are we talking about?
  
  I think that a hook and an SQL command are not on the same level.
  Hooks are not documented; SQL commands are.  You may, of course,
  disagree.

 Mmmm, yes.  I think that we should document hooks, and I'm going to
 propose soon a pg_extension_module() SRF that lists all currently loaded
 modules and which extension implements them, and I've begun thinking
 about what it would take to be able to list what hooks each module is
 implementing.
I don't really see that as possible/realistic.

  But the basic point is that it seems pretty weird to say, on the one
  hand, these are command triggers.  They fire when you execute a
  command.  So the CREATE TABLE trigger will fire when someone says
  CREATE TABLE.  But then we say, oh, well if you use CREATE SCHEMA foo
  CREATE TABLE blah ... we will fire the trigger anyway.  Now it's not a
 
 Yes, this CREATE SCHEMA any create command is weird, and unique, so
 it's not that difficult to document, I think.  And if we fix things by
 having all subcommands go through ProcessUtility and command triggers,
 then it's easy to document as the new rule.
I don't think I ever saw that one in real worldddl scripts ;)

 My documentation idea would then be explaining what is a command and
 what is a subcommand, and then the current rule (command triggers do not
 support subcommands) and then the exception to that rule (CREATE SCHEMA
 subcommands do, in fact, support command triggers).

 If we decide that we should in fact support (nested) subcommands in
 command triggers, then we will be in a position to prepare a patch
 implementing that with a documentation change that the rule is now that
 command triggers do in fact support subcommands.

 When the command trigger is called on a subcommand, I think you will
 want both the main command string and the subcommand string, and we have
 to research if what you need isn't a whole stack of commands, because
 I'm not sure you can only have 1 level deep nesting here.
I don't think you need that. If needed you will have to build the data 
structure in $pl.

 That would be done with a special API for the trigger functions, and
 with a specific syntax, because it seems to me that having a trigger
 code that is only ever called on a top-level command is a good thing to
 have.
 
   create trigger foo after command CREATE TABLE …
   create trigger foo after top level command CREATE TABLE …
   create trigger foo after nested command CREATE TABLE …
 
 Either we add support for that kind of syntax now (and not implementing
 it in 9.3 would seem weird as hell) or we instruct pg_dump and
 pg_upgrade to switch from current syntax to the new one (add in the “top
 level” keywords) when we do implement the feature down the road.
I personally think there should only be one variant which is always called. 
With a parameter that indicates whether its a subcommand or not.

Why would you ever only want top level commands?

  that is, right now, an essentially an implementation detail which we
  can rearrange as we like turns into a user-visible behavior change.
  And what if some day we want to change it back?
 Yes, we need to make a decision about that now. Do we want any
 “operation” to go through ProcessUtility so that hooks and command
 triggers can get called?
I personally think thats a good idea for most stuff.

I don't see that for alter table subcommands and such though.

 I think it's a good idea in the long run, and Alvaro seems to be
 thinking it is too. That entails changing the backend code to build a
 Statement then call ProcessUtility on it rather than calling the
 internal functions directly, and that also means some more DDL are
 subject to being logged on the server logs. Removing those
 “cross-modules” #include might be a good think in the long run though.
Uhm. I don't think building strings is the way to go here. I think building 
*Stmt nodes is better.


Andres

-- 
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 more work outside WALInsertLock

2011-12-15 Thread Jeff Janes
On Thu, Dec 15, 2011 at 7:34 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes:
 I've been experimenting with different approaches to do that, but one
 thing is common among all of them: you need to know the total amount of
 WAL space needed for the record, including backup blocks, before you
 take the lock. So, here's a patch to move things around in XLogInsert()
 a bit, to accomplish that.

 This patch may or may not be useful, but this description of it is utter
 nonsense, because we already do compute that before taking the lock.
 Please try again to explain what you're doing?

Currently the CRC of all the data minus the header is computed outside the lock,
but then the header's computation is added and the CRC is finalized
inside the lock.


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] Command Triggers

2011-12-15 Thread Dimitri Fontaine
Andres Freund and...@anarazel.de writes:
   create trigger foo after command CREATE TABLE …
   create trigger foo after top level command CREATE TABLE …
   create trigger foo after nested command CREATE TABLE …
 
 Either we add support for that kind of syntax now (and not implementing
 it in 9.3 would seem weird as hell) or we instruct pg_dump and
 pg_upgrade to switch from current syntax to the new one (add in the “top
 level” keywords) when we do implement the feature down the road.
 I personally think there should only be one variant which is always called. 
 With a parameter that indicates whether its a subcommand or not.

 Why would you ever only want top level commands?

Because the command create table foo(id primary key) could then fire
your index and constraint command triggers twice and if all you do
is storing the command string in a table for auditing purposes, maybe
you just don't care.

 Yes, we need to make a decision about that now. Do we want any
 “operation” to go through ProcessUtility so that hooks and command
 triggers can get called?
 I personally think thats a good idea for most stuff.

 I don't see that for alter table subcommands and such though.

By subcommand, I mean any operation that a main command do for you and
that you could have been doing manually with another command, such as
serial and sequences, primary key and its alter table form, embedded
checks or not null and its alter table form, etc.

I don't remember that ALTER TABLE implement facilities that are in turn
calling another command for you?

 I think it's a good idea in the long run, and Alvaro seems to be
 thinking it is too. That entails changing the backend code to build a
 Statement then call ProcessUtility on it rather than calling the
 internal functions directly, and that also means some more DDL are
 subject to being logged on the server logs. Removing those
 “cross-modules” #include might be a good think in the long run though.
 Uhm. I don't think building strings is the way to go here. I think building 
 *Stmt nodes is better.

Agreed, I meant that exactly.

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] Moving more work outside WALInsertLock

2011-12-15 Thread Tom Lane
Jeff Janes jeff.ja...@gmail.com writes:
 On Thu, Dec 15, 2011 at 7:34 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 This patch may or may not be useful, but this description of it is utter
 nonsense, because we already do compute that before taking the lock.
 Please try again to explain what you're doing?

 Currently the CRC of all the data minus the header is computed outside the 
 lock,
 but then the header's computation is added and the CRC is finalized
 inside the lock.

Quite.  AFAICS that is not optional, unless you are proposing to remove
the prev_link from the scope of the CRC, which is not exactly a
penalty-free change.

regards, tom lane

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


[HACKERS] pgstat wait timeout

2011-12-15 Thread pratikchirania
Hi,

I am having a scenario where I get consistent warnings in the pglog folder:

2011-12-11 00:00:03 JST WARNING:  pgstat wait timeout
2011-12-11 00:00:14 JST WARNING:  pgstat wait timeout
2011-12-11 00:00:24 JST WARNING:  pgstat wait timeout
2011-12-11 00:00:31 JST WARNING:  pgstat wait timeout
2011-12-11 00:00:44 JST WARNING:  pgstat wait timeout
2011-12-11 00:00:52 JST WARNING:  pgstat wait timeout
2011-12-11 00:01:03 JST WARNING:  pgstat wait timeout
2011-12-11 00:01:11 JST WARNING:  pgstat wait timeout
.
.
.

This is impacting database performance.

The issue persists even when I use the database minimally.

I have tried fine-tuning the Auto-vacuum parameters:

Change the parameter autovacuum_vacuum_cost_delay to 40ms ::: Issue is
reproduced 4 hours after this change

Change the parameter autovacuum_max_workers to 20 ::: I got the warning
message 2 times at times 17:20:12 and 17:20:20. After this, no warnings for
5 hours. Then I tried:

Change the parameter autovacuum_vacuum_cost_delay to: 60ms, Change the
parameter autovacuum_max_workers to: 10::: I got the warning message 2 times
at times 17:20:12 and 17:20:20. After this, no warnings for 5 hours

Any Ideas?

--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/pgstat-wait-timeout-tp5078125p5078125.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.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] IDLE in transaction introspection

2011-12-15 Thread Magnus Hagander
On Thu, Dec 15, 2011 at 13:18, Greg Smith g...@2ndquadrant.com wrote:
 This patch seems closing in on being done, but it's surely going to take at
 least one more round of review to make sure all the naming and documentation
 is up right.  I can work on that again whenever Scott gets another version
 necessary, and Magnus is already poking around with an eye toward possibly
 committing the result.  I think we can make this one returned with
 feedback for this CF, but encourage Scott to resubmit as early as feasible
 before the next one.  With some good luck, we might even close this out
 before that one even starts.

My hope was to get a quick update from Scott and then apply it. But
feel free to set it to returned, but Scott - don't delay until the
next CF, just post it when you're ready and I'll pick it up in between
the two CFs when I find a moment.


-- 
 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] pgstat wait timeout

2011-12-15 Thread Tomas Vondra
On 15 Prosinec 2011, 17:55, pratikchirania wrote:
 Hi,

 I am having a scenario where I get consistent warnings in the pglog
 folder:

 2011-12-11 00:00:03 JST WARNING:  pgstat wait timeout
 2011-12-11 00:00:14 JST WARNING:  pgstat wait timeout
 2011-12-11 00:00:24 JST WARNING:  pgstat wait timeout
 2011-12-11 00:00:31 JST WARNING:  pgstat wait timeout
 2011-12-11 00:00:44 JST WARNING:  pgstat wait timeout
 2011-12-11 00:00:52 JST WARNING:  pgstat wait timeout
 2011-12-11 00:01:03 JST WARNING:  pgstat wait timeout
 2011-12-11 00:01:11 JST WARNING:  pgstat wait timeout

 This is impacting database performance.

It's rather a sign that the I/O is overloaded, although in some cases it
may actually be the cause.

 The issue persists even when I use the database minimally.

Yes, because the file is written periodically - twice a second IIRC. If
the file is large, this may be an issue. What is the pgstat.stat size
(should be in data/global).

 I have tried fine-tuning the Auto-vacuum parameters:

Autovacuum has nothing to do with this.

 Any Ideas?

Move the file to a RAM drive - there's even a config parameter
'stats_temp_directory' to do that. See
http://www.postgresql.org/docs/9.1/static/runtime-config-statistics.html

Tomas


-- 
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] pgstat wait timeout

2011-12-15 Thread Magnus Hagander
On Thu, Dec 15, 2011 at 18:13, Tomas Vondra t...@fuzzy.cz wrote:
 On 15 Prosinec 2011, 17:55, pratikchirania wrote:
 Hi,

 I am having a scenario where I get consistent warnings in the pglog
 folder:

 2011-12-11 00:00:03 JST WARNING:  pgstat wait timeout
 2011-12-11 00:00:14 JST WARNING:  pgstat wait timeout
 2011-12-11 00:00:24 JST WARNING:  pgstat wait timeout
 2011-12-11 00:00:31 JST WARNING:  pgstat wait timeout
 2011-12-11 00:00:44 JST WARNING:  pgstat wait timeout
 2011-12-11 00:00:52 JST WARNING:  pgstat wait timeout
 2011-12-11 00:01:03 JST WARNING:  pgstat wait timeout
 2011-12-11 00:01:11 JST WARNING:  pgstat wait timeout

 This is impacting database performance.

 It's rather a sign that the I/O is overloaded, although in some cases it
 may actually be the cause.

 The issue persists even when I use the database minimally.

 Yes, because the file is written periodically - twice a second IIRC. If
 the file is large, this may be an issue. What is the pgstat.stat size
 (should be in data/global).

That was only true prior to 8.4. As of 8.4 it's only written when
necessary, which is usually a lot less than twice / second.

-- 
 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] pgstat wait timeout

2011-12-15 Thread Tomas Vondra
On 15 Prosinec 2011, 18:19, Magnus Hagander wrote:
 On Thu, Dec 15, 2011 at 18:13, Tomas Vondra t...@fuzzy.cz wrote:
 On 15 Prosinec 2011, 17:55, pratikchirania wrote:
 Hi,

 I am having a scenario where I get consistent warnings in the pglog
 folder:

 2011-12-11 00:00:03 JST WARNING:  pgstat wait timeout
 2011-12-11 00:00:14 JST WARNING:  pgstat wait timeout
 2011-12-11 00:00:24 JST WARNING:  pgstat wait timeout
 2011-12-11 00:00:31 JST WARNING:  pgstat wait timeout
 2011-12-11 00:00:44 JST WARNING:  pgstat wait timeout
 2011-12-11 00:00:52 JST WARNING:  pgstat wait timeout
 2011-12-11 00:01:03 JST WARNING:  pgstat wait timeout
 2011-12-11 00:01:11 JST WARNING:  pgstat wait timeout

 This is impacting database performance.

 It's rather a sign that the I/O is overloaded, although in some cases it
 may actually be the cause.

 The issue persists even when I use the database minimally.

 Yes, because the file is written periodically - twice a second IIRC. If
 the file is large, this may be an issue. What is the pgstat.stat size
 (should be in data/global).

 That was only true prior to 8.4. As of 8.4 it's only written when
 necessary, which is usually a lot less than twice / second.

Thanks for the correction. Nevertheless, it would be useful to know what
is the size of the file and what is the I/O usage.

Pratik, can you post the size of the pgstat.stat file and post a few lines
of iostat -x 1 collected when the pgstat wait timeout happens?

Tomas


-- 
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] JSON for PG 9.2

2011-12-15 Thread Simon Riggs
On Tue, Dec 13, 2011 at 8:15 AM, Joey Adams joeyadams3.14...@gmail.com wrote:

 Issues we've encountered include:

  * Should JSON be stored as binary or as text?

Text

  * How do we deal with Unicode escapes and characters if the server or
 client encoding is not UTF-8?  Some (common!) character encodings have
 code points that don't map to Unicode.  Also, the charset conversion
 modules do not provide fast entry points for converting individual
 characters; each conversion involves a funcapi call.

Make JSON datatypes only selectable if client encoding is UTF-8.

Lets JFDI

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

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


Re: [HACKERS] pgstat wait timeout

2011-12-15 Thread pratikchirania
Size of pgstat.stat file: 86KB

I did not understand the second part. Where do I get iostat -x 1 message?
(Its not present in any file in the pg_log folder)


I am using postgres 9.0.1

--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/pgstat-wait-timeout-tp5078125p5078391.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.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 more work outside WALInsertLock

2011-12-15 Thread Heikki Linnakangas

On 15.12.2011 18:48, Tom Lane wrote:

Jeff Janesjeff.ja...@gmail.com  writes:

On Thu, Dec 15, 2011 at 7:34 AM, Tom Lanet...@sss.pgh.pa.us  wrote:

This patch may or may not be useful, but this description of it is utter
nonsense, because we already do compute that before taking the lock.
Please try again to explain what you're doing?



Currently the CRC of all the data minus the header is computed outside the lock,
but then the header's computation is added and the CRC is finalized
inside the lock.


Quite.  AFAICS that is not optional,


Right, my patch did not change that.


unless you are proposing to remove
the prev_link from the scope of the CRC, which is not exactly a
penalty-free change.


We could CRC the rest of the record header before getting the lock, 
though, and only include the prev-link while holding the lock. I 
micro-benchmarked that a little bit, but didn't see much benefit from 
doing just that. Once you do more drastic changes so that the lock 
doesn't need to be held while copying the data and calculating the CRC 
of the record header, so that those things can be done in parallel, it 
matters even less.


--
  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] pgstat wait timeout

2011-12-15 Thread Tomas Vondra
On 15 Prosinec 2011, 19:42, pratikchirania wrote:
 Size of pgstat.stat file: 86KB

That's pretty small.

 I did not understand the second part. Where do I get iostat -x 1
 message?
 (Its not present in any file in the pg_log folder)

iostat is not part of PostgreSQL, it's a tool used to display various I/O
metrics in Linux (and Unix in general). What OS are you using?

It seems the I/O subsystem is so busy it can't write the pgstat.stat on
time, so a warning is printed. You need to find out why the I/O is so
overutilized.

 I am using postgres 9.0.1

That's way too old. Upgrade to 9.0.6.

Tomas


-- 
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 more work outside WALInsertLock

2011-12-15 Thread Heikki Linnakangas

On 15.12.2011 17:34, Tom Lane wrote:

Heikki Linnakangasheikki.linnakan...@enterprisedb.com  writes:

I've been experimenting with different approaches to do that, but one
thing is common among all of them: you need to know the total amount of
WAL space needed for the record, including backup blocks, before you
take the lock. So, here's a patch to move things around in XLogInsert()
a bit, to accomplish that.


This patch may or may not be useful, but this description of it is utter
nonsense, because we already do compute that before taking the lock.


Nope. Without this patch, the total length including the backup blocks, 
write_len, is added up in the loop that creates the rdata entries for 
backup blocks. That is done while holding the lock (see code beginning 
with comment Make additional rdata chain entries for the backup blocks).


Admittedly you could sum up the total length quite easily in the earlier 
loop, before we grab the lock, where we calculate the CRC of the backup 
blocks (Now add the backup block headers and data into the CRC). That 
would be a smaller patch.



Please try again to explain what you're doing?


Ok: I'm moving the creation of rdata entries for backup blocks outside 
the critical section, so that it's done before grabbing the lock. I'm 
also moving the CRC calculation so that it's done after all the rdata 
entries have been created, including the ones for backup blocks. It's 
more readable to do it that way, as a separate step, instead of 
sprinkling the COMP_CRC macros in many places.


--
  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] CommitFest 2011-11 Update

2011-12-15 Thread Robert Haas
On Sat, Dec 10, 2011 at 1:07 PM, Greg Smith g...@2ndquadrant.com wrote:
 There's a normal sized plate lined up for Robert since he's been keeping up
 better; in no particular order:

 -More review on the always a new surprise Fix Leaky Views Problem, again
 -Extra fun with Tuplesort comparison overhead reduction
 -His own avoid taking two snapshots per query and FlexLocks improvements

Greg,

Thanks for working through this.  I see I still need to look more at
the leaky views stuff.  I think that the tuplesort comparison overhead
reduction is in Peter G's hands to rebase at the moment; I will look
at that again when it reemerges.  I think avoid two snapshots per
query is pretty much ready to go, though I'm vaguely waiting for any
further input from Dimitri.  I believe I'm going to punt on FlexLocks
for now, for the reasons set out in the email I just sent on that
topic, and accordingly am marking that patch with feedback.

One more time to make sure I made my main point: thanks for working on
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] IDLE in transaction introspection

2011-12-15 Thread Scott Mead
On Thu, Dec 15, 2011 at 12:10 PM, Magnus Hagander mag...@hagander.netwrote:

 On Thu, Dec 15, 2011 at 13:18, Greg Smith g...@2ndquadrant.com wrote:
  This patch seems closing in on being done, but it's surely going to take
 at
  least one more round of review to make sure all the naming and
 documentation
  is up right.  I can work on that again whenever Scott gets another
 version
  necessary, and Magnus is already poking around with an eye toward
 possibly
  committing the result.  I think we can make this one returned with
  feedback for this CF, but encourage Scott to resubmit as early as
 feasible
  before the next one.  With some good luck, we might even close this out
  before that one even starts.

 My hope was to get a quick update from Scott and then apply it. But
 feel free to set it to returned, but Scott - don't delay until the
 next CF, just post it when you're ready and I'll pick it up in between
 the two CFs when I find a moment.


Thanks guys, I should have something by this weekend.  Sorry for the delay.

--Scott



 --
  Magnus Hagander
  Me: http://www.hagander.net/
  Work: http://www.redpill-linpro.com/



Re: [HACKERS] Command Triggers

2011-12-15 Thread Noah Misch
On Thu, Dec 15, 2011 at 05:46:05PM +0100, Dimitri Fontaine wrote:
 Andres Freund and...@anarazel.de writes:
  Yes, we need to make a decision about that now. Do we want any
  ???operation??? to go through ProcessUtility so that hooks and command
  triggers can get called?
  I personally think thats a good idea for most stuff.
 
  I don't see that for alter table subcommands and such though.
 
 By subcommand, I mean any operation that a main command do for you and
 that you could have been doing manually with another command, such as
 serial and sequences, primary key and its alter table form, embedded
 checks or not null and its alter table form, etc.
 
 I don't remember that ALTER TABLE implement facilities that are in turn
 calling another command for you?

When ALTER TABLE ALTER TYPE changes an indexed column, it updates the index by
extracting its SQL definition, dropping it, and running that SQL.  (Not sure
whether it passes through the code paths to hit your trigger.)  We ought not to
promise that ALTER TABLE will always do so.  By comparison, we could implement
ALTER TABLE SET NOT NULL on an inheritance tree as several ALTER TABLE ONLY, but
we don't implement it that way.  Triggering on top-level commands can be quite
well-defined; triggers on subcommands (as you have defined the term) will risk
firing at different times across major versions.  That may just be something to
document.

I think we'll need a way to tell SPI whether a command should be considered a
subcommand or not.  A PL/pgSQL function that calls CREATE TABLE had better
fire a trigger, but some SPI usage will qualify as subcommands.

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] Command Triggers

2011-12-15 Thread Robert Haas
On Thu, Dec 15, 2011 at 10:53 AM, Dimitri Fontaine
dimi...@2ndquadrant.fr wrote:
 Yes, we need to make a decision about that now. Do we want any
 “operation” to go through ProcessUtility so that hooks and command
 triggers can get called?

No.  That's 100% backwards.  We should first decide what functionality
we want, and then decide how we're going to implement it.  If we
arbitrarily decide to force everything that someone might want to
write a trigger on through ProcessUtility_hook, then we're going to
end up being unable to add triggers for some things because they can't
be conveniently forced through ProcessUtility, or else we're going to
end up contorting the code in bizarre ways because we've drawn some
line in the sand that ProcessUtility is the place where triggers have
to get called.

In doing this project, I think we should pay a lot of attention to the
lessons that have been learned developing sepgsql.  I can certainly
understand if your eyes roll back in your head when you hear that,
because that project has been exceedingly long and difficult and shows
no sign of reaching its full potential for at least another few
release cycles.  But I think it's really worth the effort to avoid
pounding our heads against the brick wall twice.  Two things that leap
out at me in this regard are:

(1) It's probably a mistake to assume that we only need one interface.
 sepgsql has several hooks, and will need more before the dust
settles.  We have one hook for checking permissions on table names
that appear in DML queries, a second for applying security labels just
after a new SQL object is created, and a third for adjusting the
security context when an sepgsql trusted procedure is invoked.  In a
similar way, I think it's probably futile to think that we can come up
with a one-size-fits-all interface where every command (or operation)
trigger can accept the same arguments.  CREATE TABLE is going to want
to know different stuff than LOCK TABLE or ALTER OPERATOR FAMILY, and
trigger writers are going to want a *convenient* API to that
information, not just the raw query text.

(2) It's almost certainly a mistake to assume that everything you want
to trigger on is a command.  For example, somebody might want to get
control whenever a table gets added to a column, either at table
create time or later.  I don't think most of us would consider CREATE
TABLE bob (a int, b int) to be a create-a-table-with-no-columns
operation plus two add-a-column-to-a-table operations.  But being able
to enforce a column naming policy is no less useful than being able to
enforce a table naming policy, and there are other things you might
want to do there as well (logging, updating metadata, prohibiting use
of certain types, etc.).

-- 
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] Command Triggers

2011-12-15 Thread Dimitri Fontaine
Robert Haas robertmh...@gmail.com writes:
 No.  That's 100% backwards.  We should first decide what functionality
 we want, and then decide how we're going to implement it.  If we
 arbitrarily decide to force everything that someone might want to
 write a trigger on through ProcessUtility_hook, then we're going to
 end up being unable to add triggers for some things because they can't
 be conveniently forced through ProcessUtility, or else we're going to
 end up contorting the code in bizarre ways because we've drawn some
 line in the sand that ProcessUtility is the place where triggers have
 to get called.

In theory you're right.  In practice, we're talking about utility
command triggers, that fires on a top-level command.

We're now enlarging the talk about what to do with sub-commands, that is
things done by a command as part of its implementation but that you
could have done separately with another finer grained dedicated
top-level command.

I'm not wanting to implement a general ”event” trigger mechanism where
anyone can come and help define the set of events, and I think what
you're talking about now amounts to be doing that.

Here again, trying to generalize before we have anything useful is a
recipe for failure. I concur that “Process Utility Top-Level Only
Command Triggers” is a pretty limited feature in scope, yet that's what
I want to obtain here, and I think it's useful enough on its own.

If you disagree, please propose a user level scheme where we can fit the
work I'm doing so that I can adapt my patch and implement a part of your
scheme in a future proof way.  I'm ready to do that even when I have no
need for what you're talking about, if that's what it takes.

 (1) It's probably a mistake to assume that we only need one interface.
[... useful sepgsql history ...]
 trigger can accept the same arguments.  CREATE TABLE is going to want
 to know different stuff than LOCK TABLE or ALTER OPERATOR FAMILY, and
 trigger writers are going to want a *convenient* API to that
 information, not just the raw query text.

Are you familiar with the nodeToString() output?  That's close to a full
blown XML, JSON or YAML document that you can easily use from a program.
Command triggers functions are not given just a raw text.

When trying to define a more complex API in the line of what you're
referencing here, back at the Cluster Hackers Developer Meeting, it was
devised that the nodeToString() output is all you need. For having
written code that produces it and code that consumes it, Jan has been
very clear that you hardly can do better than that while still being
generic enough.

 to enforce a column naming policy is no less useful than being able to
 enforce a table naming policy, and there are other things you might
 want to do there as well (logging, updating metadata, prohibiting use
 of certain types, etc.).

Are you familiar with the nodeToString() output?  What makes you think
that the uses cases you propose here are hard to implement in a command
trigger when given this parse tree string?

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] Measuring relation free space

2011-12-15 Thread Noah Misch
On Sun, Nov 06, 2011 at 10:20:49PM +0100, Bernd Helmle wrote:
 --On 6. November 2011 01:08:11 -0200 Greg Smith g...@2ndquadrant.com wrote:

 Attached patch adds a new function to the pageinspect extension for measuring
 total free space, in either tables or indexes.

 I wonder if that should be done in the pgstattuple module, which output 
 some similar numbers.

Indeed, pgstattuple already claims to show precisely the same measure.  Its
reckoning is right in line for heaps, but the proposed pageinspect function
finds more free space in indexes:

[local] test=# SELECT t.free_percent, relation_free_space('pg_proc'), 
i.free_percent, relation_free_space('pg_proc_proname_args_nsp_index') FROM 
pgstattuple('pg_proc') t, pgstattuple('pg_proc_proname_args_nsp_index') i;
 free_percent | relation_free_space | free_percent | relation_free_space 
--+-+--+-
 2.53 |   0.0253346 | 8.61 |0.128041
(1 row)

Is one of those index figures simply wrong, or do they measure two senses of
free space, both of which are interesting to DBAs?

Thanks,
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] JSON for PG 9.2

2011-12-15 Thread Andrew Dunstan



On 12/15/2011 01:34 PM, Simon Riggs wrote:

On Tue, Dec 13, 2011 at 8:15 AM, Joey Adamsjoeyadams3.14...@gmail.com  wrote:


Issues we've encountered include:

  * Should JSON be stored as binary or as text?

Text


Works for me.


  * How do we deal with Unicode escapes and characters if the server or
client encoding is not UTF-8?  Some (common!) character encodings have
code points that don't map to Unicode.  Also, the charset conversion
modules do not provide fast entry points for converting individual
characters; each conversion involves a funcapi call.

Make JSON datatypes only selectable if client encoding is UTF-8.


Yuck. Do we have this sort of restriction for any other data type?

ISTM that the encoding problem is at least as likely to be the reverse 
of what's above - i.e. that there's a code point in the stored JSON 
that's not represented in the client encoding.


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] Moving more work outside WALInsertLock

2011-12-15 Thread Simon Riggs
On Thu, Dec 15, 2011 at 7:06 PM, Heikki Linnakangas
heikki.linnakan...@enterprisedb.com wrote:

 Please try again to explain what you're doing?


 Ok: I'm moving the creation of rdata entries for backup blocks outside the
 critical section, so that it's done before grabbing the lock. I'm also
 moving the CRC calculation so that it's done after all the rdata entries
 have been created, including the ones for backup blocks. It's more readable
 to do it that way, as a separate step, instead of sprinkling the COMP_CRC
 macros in many places.

There's a comment that says we can't undo the linking of the rdata
chains, but it looks like a reversible process to me.

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

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


Re: [HACKERS] Moving more work outside WALInsertLock

2011-12-15 Thread Simon Riggs
On Thu, Dec 15, 2011 at 6:50 PM, Heikki Linnakangas
heikki.linnakan...@enterprisedb.com wrote:

 unless you are proposing to remove
 the prev_link from the scope of the CRC, which is not exactly a
 penalty-free change.


 We could CRC the rest of the record header before getting the lock, though,
 and only include the prev-link while holding the lock. I micro-benchmarked
 that a little bit, but didn't see much benefit from doing just that. Once
 you do more drastic changes so that the lock doesn't need to be held while
 copying the data and calculating the CRC of the record header, so that those
 things can be done in parallel, it matters even less.

You missed your cue to discuss leaving the prev link out of the CRC altogether.

On its own that sounds dangerous, but its not. When we need to confirm
the prev link we already know what we expect it to be, so CRC-ing it
is overkill. That isn't true of any other part of the WAL record, so
the prev link is the only thing we can relax, but thats OK because we
can CRC check everything else outside of the locked section.

That isn't my idea, but I'm happy to put it on the table since I'm not shy.

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

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


Re: [HACKERS] RangeVarGetRelid()

2011-12-15 Thread Robert Haas
On Fri, Dec 9, 2011 at 5:41 PM, Noah Misch n...@leadboat.com wrote:
 It also seems my last explanation didn't convey the point.  Yes, nearly every
 command has a different set of permissions checks.  However, we don't benefit
 equally from performing each of those checks before acquiring a lock.
 Consider renameatt(), which checks three things: you must own the relation,
 the relation must be of a supported relkind, and the relation must not be a
 typed table.  To limit opportunities for denial of service, let's definitely
 perform the ownership check before taking a lock.  The other two checks can
 wait until we hold that lock.  The benefit of checking them early is to avoid
 making a careless relation owner wait for a lock before discovering the
 invalidity of his command.  That's nice as far as it goes, but let's not
 proliferate callbacks for such a third-order benefit.

I agree, but my point is that so far we have no callbacks that differ
only in that detail.  I accept that we'd probably want to avoid 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] Patch to allow users to kill their own queries

2011-12-15 Thread Josh Kupershmidt
On Tue, Dec 13, 2011 at 5:59 AM, Greg Smith g...@2ndquadrant.com wrote:
 Same-user cancels, but not termination.  Only this, and nothing more.

+1 from me on this approach. I think enough people have clamored for
this simple approach which solves the common-case.

 There's one obvious and questionable design decision I made to highlight.
  Right now the only consumers of pg_signal_backend are the cancel and
 terminate calls.  What I did was make pg_signal_backend more permissive,
 adding the idea that role equivalence = allowed, and therefore granting that
 to anything else that might call it.  And then I put a stricter check on
 termination.  This results in a redundant check of superuser on the
 termination check, and the potential for mis-using pg_signal_backend. . .

I think that's OK, it should be easy enough to reorganize if more
callers to pg_signal_backend() happen to come along.

The only niggling concern I have about this patch (and, I think, all
similar ones proposed) is the possible race condition between the
permissions checking and the actual call of kill() inside
pg_signal_backend(). I fooled around with this by using gdb to attach
to Backend #1, stuck a breakpoint right before the call to:

if (kill(-pid, sig))

and ran a pg_cancel_backend() of a same-role Backend #2. Then, with
the permissions checking passed, I exited Backend #2 and used a script
to call fork() in a loop until my system PIDs had wrapped around to a
few PIDs before the one Backend #2 had held. Then, I opened a few
superuser connections until I had one with the same PID which Backend
#2 previously had.

After I released the breakpoint in gdb on Backend #1, it happily sent
a SIGINT to my superuser backend.

I notice that BackendPidGetProc() has the comment:

  ...  Note that it is up to the caller to be
  sure that the question remains meaningful for long enough for the
  answer to be used ...

So someone has mulled this over before. It took my script maybe 15-20
minutes to cause a wraparound by running fork() in a loop, and that
wraparound would somehow have to occur within the short execution of
pg_signal_backend(). I'm not super worried about the patch from a
security perspective, just thought I'd point this out.

Josh

-- 
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] includeifexists in configuration file

2011-12-15 Thread Andrew Dunstan



On 12/15/2011 06:54 AM, Greg Smith wrote:

On 12/12/2011 04:47 PM, Andrew Dunstan wrote:
I have briefly looked at the code (but not tried to apply or build 
it), and modulo the naming issue it looks OK to me.
Unless there is some other issue let's just get it applied. It looks 
like almost a no-brainer to me.


It isn't very fancy, but is does something people that can fit into a 
couple of use-cases.  Attached update has two changes to address the 
suggestions I got, which closes everything I knew about with this one:


-It's now include_if_exists
-Files that are skipped are logged now

So current behavior:

$ tail -n 1 postgresql.conf
include 'missing.conf'
$ start
server starting
$ tail $PGLOG
LOG:  could not open configuration file 
/home/gsmith/pgwork/data/include-exists/missing.conf: No such file 
or directory
FATAL:  configuration file 
/home/gsmith/pgwork/data/include-exists/postgresql.conf contains errors


And new behavior:

$ vi $PGDATA/postgresql.conf
$ tail -n 1 postgresql.conf
include_if_exists 'missing.conf'
$ start
server starting
$ tail $PGLOG
LOG:  skipping missing configuration file 
/home/gsmith/pgwork/data/include-exists/missing.conf

LOG:  database system was shut down at 2011-12-15 06:48:46 EST
LOG:  database system is ready to accept connections


Committed. I changed the elog() call to use ereport(): you're not 
supposed to use elog() for things we expect might well happen and cause 
log entries - see bottom of 
http://www.postgresql.org/docs/current/static/error-message-reporting.html. 
I've probably been guilty of this in the past, it's a bit too easy to 
forget.


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] Moving more work outside WALInsertLock

2011-12-15 Thread Tom Lane
Simon Riggs si...@2ndquadrant.com writes:
 You missed your cue to discuss leaving the prev link out of the CRC 
 altogether.

 On its own that sounds dangerous, but its not. When we need to confirm
 the prev link we already know what we expect it to be, so CRC-ing it
 is overkill. That isn't true of any other part of the WAL record, so
 the prev link is the only thing we can relax, but thats OK because we
 can CRC check everything else outside of the locked section.

 That isn't my idea, but I'm happy to put it on the table since I'm not shy.

I'm glad it's not your idea, because it's a bad one.  A large part of
the point of CRC'ing WAL records is to guard against torn-page problems
in the WAL files, and doing things like that would give up a significant
part of that protection, because there would no longer be any assurance
that the body of a WAL record had anything to do with its prev_link.

Consider a scenario like this:

* We write a WAL record that starts 8 bytes before a sector boundary,
so that the prev_link is in one sector and the rest of the record in
the next one(s).

* Time passes, and we recycle that WAL file.

* We write another WAL record that starts 8 bytes before the same sector
boundary, so that the prev_link is in one sector and the rest of the
record in the next one(s).

* System crashes, after having written out the earlier sector but not
the later one(s).

On restart, the replay code will see a prev_link that matches what it
expects.  If the CRC for the remainder of the record is not dependent
on the prev_link, then the remainder of the old record will look good
too, and we'll attempt to replay it, n*16MB too late.

Including the prev_link in the CRC adds a significant amount of
protection against such problems.  We should not remove this protection
in the name of shaving a few cycles.

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] Storing hot members of PGPROC out of the band

2011-12-15 Thread Robert Haas
On Mon, Nov 7, 2011 at 7:28 AM, Robert Haas robertmh...@gmail.com wrote:
 On Mon, Nov 7, 2011 at 6:45 AM, Heikki Linnakangas
 heikki.linnakan...@enterprisedb.com wrote:
 While looking at GetSnapshotData(), it occurred to me that when a PGPROC
 entry does not have its xid set, ie. xid == InvalidTransactionId, it's
 pointless to check the subxid array for that proc. If a transaction has no
 xid, none of its subtransactions can have an xid either. That's a trivial
 optimization, see attached. I tested this with pgbench -S -M prepared -c
 500 on the 8-core box, and it seemed to make a 1-2% difference (without the
 other patch). So, almost in the noise, but it also doesn't cost us anything
 in terms of readability or otherwise.

 Oh, that's a good idea.  +1 for doing that now, and then we can keep
 working on the rest of it.

I spent some time looking at (and benchmarking) this idea today.  I
rearranged that section of code a little more into what seemed like
the optimal order to avoid doing more work than necessary, but I
wasn't getting much of a gain out of it even on unlogged tables and on
permanent tables it was looking like a small loss, though I didn't
bother letting the benchmarks run for long enough to nail that down
because it didn't seem to amount to much one way or the other.  I
added then added one more change that helped quite a lot: I introduced
a macro NormalTransactionIdPrecedes() which works like
TransactionIdPrecdes() but (a) is only guaranteed to work if the
arguments are known to be normal transaction IDs (which it also
asserts, for safety) and (b) is a macro rather than a function call.
I found three places to use that inside GetSnapshotData(), and the
results with that change look fairly promising.

Nate Boley's box, m = master, s = with the attached patch, median of
three 5-minute runs at scale factor 100, config same as my other
recent tests:

Permanent Tables

m01 tps = 617.734793 (including connections establishing)
s01 tps = 620.330099 (including connections establishing)
m08 tps = 4589.566969 (including connections establishing)
s08 tps = 4545.942588 (including connections establishing)
m16 tps = 7618.842377 (including connections establishing)
s16 tps = 7596.759619 (including connections establishing)
m24 tps = 11770.534809 (including connections establishing)
s24 tps = 11789.424152 (including connections establishing)
m32 tps = 10776.660575 (including connections establishing)
s32 tps = 10643.361817 (including connections establishing)
m80 tps = 11057.353339 (including connections establishing)
s80 tps = 10598.254713 (including connections establishing)

Unlogged Tables

m01 tps = 668.145495 (including connections establishing)
s01 tps = 676.793337 (including connections establishing)
m08 tps = 4715.214745 (including connections establishing)
s08 tps = 4737.833913 (including connections establishing)
m16 tps = 8110.607784 (including connections establishing)
s16 tps = 8192.013414 (including connections establishing)
m24 tps = 14120.753841 (including connections establishing)
s24 tps = 14302.915040 (including connections establishing)
m32 tps = 17886.032656 (including connections establishing)
s32 tps = 18735.319468 (including connections establishing)
m80 tps = 12711.930739 (including connections establishing)
s80 tps = 17592.715471 (including connections establishing)

Now, you might not initially find those numbers all that encouraging.
Of course, the unlogged tables numbers are quite good, especially at
32 and 80 clients, where the gains are quite marked.  But the
permanent table numbers are at best a wash, and maybe a small loss.
Interestingly enough, some recent benchmarking of the FlexLocks patch
showed a similar (though more pronounced) effect:

http://archives.postgresql.org/pgsql-hackers/2011-12/msg00095.php

Now, both the FlexLocks patch and this patch aim to mitigate
contention problems around ProcArrayLock.  But they do it in
completely different ways.  When I got a small regression on permanent
tables with the FlexLocks patch, I thought the problem was with the
patch itself, which is believable, since it was tinkering with the
LWLock machinery, a fairly global sort of change that you can well
imagine might break something.  But it's hard to imagine how that
could possibly be the case here, especially given the speedups on
unlogged tables, because this patch is dead simple and entirely
isolated to GetSnapshotData().  So I have a new theory: on permanent
tables, *anything* that reduces ProcArrayLock contention causes an
approximately equal increase in WALInsertLock contention (or maybe
some other lock), and in some cases that increase in contention
elsewhere can cost more than the amount we're saving here.

On that theory, I'm inclined to think that's not really a problem.
We'll go nuts if we refuse to commit anything until it shows a
meaningful win on every imaginable workload, and it seems like this
can't really be worse than the status quo; any case where it is must

Re: [HACKERS] includeifexists in configuration file

2011-12-15 Thread Greg Smith

On 12/15/2011 08:16 PM, Andrew Dunstan wrote:
 I changed the elog() call to use ereport(): you're not supposed to 
use elog() for things we expect might well happen and cause log 
entries - see bottom of 
http://www.postgresql.org/docs/current/static/error-message-reporting.html. 
I've probably been guilty of this in the past, it's a bit too easy to 
forget.


Quite, I both knew this once and forgot it last night.  There was some 
nagging in the back of my head that I was doing something wrong, but I 
couldn't place what.  Happy this is committed, given that I've suggested 
relying upon it in the recovery.conf thread.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us


--
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 to allow users to kill their own queries

2011-12-15 Thread Greg Smith

On 12/15/2011 07:36 PM, Josh Kupershmidt wrote:

The only niggling concern I have about this patch (and, I think, all
similar ones proposed) is the possible race condition between the
permissions checking and the actual call of kill() inside
pg_signal_backend().


This is a problem with the existing code though, and the proposed 
changes don't materially alter that; there's just another quick check in 
one path through.  Right now we check if someone is superuser, then if 
it's a backend PID, then we send the signal.  If you assume someone can 
run through all the PIDs between those checks and the kill, the system 
is already broken that way.



I notice that BackendPidGetProc() has the comment:

   ...  Note that it is up to the caller to be
   sure that the question remains meaningful for long enough for the
   answer to be used ...

So someone has mulled this over before. It took my script maybe 15-20
minutes to cause a wraparound by running fork() in a loop, and that
wraparound would somehow have to occur within the short execution of
pg_signal_backend().


Right, the the possibility you're raising is the obvious flaw with this 
approach.  Currently the only consumers of BackendPidGetProc or 
IsBackendPid are these cancel/terminate functions.  As you measured, 
running through the PIDs fast enough to outrace any of that code is 
unlikely.  I'm sure a lot of other UNIX apps would break, too, if that 
ever became untrue.  The sort of thing that comment is warning is that 
you wouldn't want to do something like grab a PID, vacuum a table, and 
then assume that PID was still valid.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us


--
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] Measuring relation free space

2011-12-15 Thread Greg Smith

On 12/15/2011 04:11 PM, Noah Misch wrote:

Is one of those index figures simply wrong, or do they measure two senses of
free space, both of which are interesting to DBAs?
   


I think the bigger one--the one I was aiming to measure--also includes 
fill-factor space.  It should be possible to isolate whether that's true 
by running the function against a fresh index, or by trying tests with a 
table where there's no useful fill.  I need to add some of those to the 
test example suite.


While in theory both measures of free space might be interesting to 
DBAs, I'd prefer to have the one that reflects the lost space to 
fill-factor if I'm checking an index.  But as Robert Treat was pointing 
out, even the very rough estimates being made with existing user-space 
tools not using the contrib module features are helpful enough for a lot 
of users.  So long as it's easy and accuracy is good enough to find 
bloated indexes, either implementation is probably good enough.


Shaking out the alternate implementation ideas was really my goal for 
this CF here.  The major goal of the next revision is to present the 
options with a measure of their respective accuracy and runtime.  If I 
have to give up just a of bit of accuracy and make it much faster, 
that's probably what most people want as an option.  When Jaime and I 
come back with an update, it really needs to have benchmarks and 
accuracy numbers for each option.  That may be complicated a bit 
depending on how much of the table or index is cached, so isolating that 
out will be a pain.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us


--
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] Caching for stable expressions with constant arguments v3

2011-12-15 Thread Greg Smith
I think what would be helpful here next is a self-contained benchmarking 
script.  You posted an outline of what you did for the original version 
at 
http://archives.postgresql.org/message-id/cabrt9rc-1wgxzc_z5mwkdk70fgy2drx3slxzdp4vobkukpz...@mail.gmail.com


It doesn't sound like it would be hard to turn that into a little shell 
script.  I'd rather see one from you mainly so I don't have to worry 
about whether I duplicated your procedure correctly or not.  Don't even 
worry about best of 3, just run it three times, grep out the line that 
shows the TPS number, and show them all.  One of the first questions I 
have is whether the performance changed between there and your v5.  A 
self-contained and fully automated performance test case would ease 
review, and give some performance regression testing for other suggested 
changes to use as a reference.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us


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