date:20140128

Re: [HACKERS] Patch: Show process IDs of processes holding a lock; show relation and tuple infos of a lock to acquire

2014-01-28 Thread Christian Kruse

Hi,

On 27/01/14 11:44, Rajeev rastogi wrote:
 I have checked the revised patch. It looks fine to me except one minor code 
 formatting issue.
 In elog.c, two tabs are missing in the definition of function 
 errdetail_log_plural.
 Please run pgindent tool to check the same.

I did, but this reformats various other locations in the file,
too. Nevertheless I now ran pg_indent against it and removed the other
parts. Attached you will find the corrected patch version.

 Also I would like to highlight one behavior here is that process ID of 
 process trying to
 acquire lock is also listed in the list of Request queue. E.g.

   session 1 with process id X: BEGIN; LOCK TABLE foo IN SHARE MODE;
   session 2 with process id Y: BEGIN; LOCK TABLE foo IN EXCLUSIVE MODE;

 On execution of LOCK in session-2, as part of log it will display as:
   DETAIL:  Process holding the lock: X. Request queue: Y.

   Where Y is the process ID of same process, which was trying to acquire 
 lock.

This is on purpose due to the rewording of the Message. In the first
version the PID of the backend was missing.

Thanks for the review!

Best regards,

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

diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index ee6c24c..6c648cf 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -1195,13 +1195,23 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
 		 */
 		if (log_lock_waits  deadlock_state != DS_NOT_YET_CHECKED)
 		{
-			StringInfoData buf;
+			StringInfoData buf,
+		lock_waiters_sbuf,
+		lock_holders_sbuf;
 			const char *modename;
 			long		secs;
 			int			usecs;
 			long		msecs;
+			SHM_QUEUE  *procLocks;
+			PROCLOCK   *proclock;
+			bool		first_holder = true,
+		first_waiter = true;
+			int			lockHoldersNum = 0;
 
 			initStringInfo(buf);
+			initStringInfo(lock_waiters_sbuf);
+			initStringInfo(lock_holders_sbuf);
+
 			DescribeLockTag(buf, locallock-tag.lock);
 			modename = GetLockmodeName(locallock-tag.lock.locktag_lockmethodid,
 	   lockmode);
@@ -1211,10 +1221,67 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
 			msecs = secs * 1000 + usecs / 1000;
 			usecs = usecs % 1000;
 
+			/*
+			 * we loop over the lock's procLocks to gather a list of all
+			 * holders and waiters. Thus we will be able to provide more
+			 * detailed information for lock debugging purposes.
+			 *
+			 * lock-procLocks contains all processes which hold or wait for
+			 * this lock.
+			 */
+
+			LWLockAcquire(partitionLock, LW_SHARED);
+
+			procLocks = (lock-procLocks);
+			proclock = (PROCLOCK *) SHMQueueNext(procLocks, procLocks,
+			   offsetof(PROCLOCK, lockLink));
+
+			while (proclock)
+			{
+/*
+ * we are a waiter if myProc-waitProcLock == proclock; we are
+ * a holder if it is NULL or something different
+ */
+if (proclock-tag.myProc-waitProcLock == proclock)
+{
+	if (first_waiter)
+	{
+		appendStringInfo(lock_waiters_sbuf, %d,
+		 proclock-tag.myProc-pid);
+		first_waiter = false;
+	}
+	else
+		appendStringInfo(lock_waiters_sbuf, , %d,
+		 proclock-tag.myProc-pid);
+}
+else
+{
+	if (first_holder)
+	{
+		appendStringInfo(lock_holders_sbuf, %d,
+		 proclock-tag.myProc-pid);
+		first_holder = false;
+	}
+	else
+		appendStringInfo(lock_holders_sbuf, , %d,
+		 proclock-tag.myProc-pid);
+
+	lockHoldersNum++;
+}
+
+proclock = (PROCLOCK *) SHMQueueNext(procLocks, proclock-lockLink,
+			   offsetof(PROCLOCK, lockLink));
+			}
+
+			LWLockRelease(partitionLock);
+
 			if (deadlock_state == DS_SOFT_DEADLOCK)
 ereport(LOG,
 		(errmsg(process %d avoided deadlock for %s on %s by rearranging queue order after %ld.%03d ms,
-			  MyProcPid, modename, buf.data, msecs, usecs)));
+MyProcPid, modename, buf.data, msecs, usecs),
+		 (errdetail_log_plural(Process holding the lock: %s. Request queue: %s.,
+		Processes holding the lock: %s. Request queue: %s.,
+			   lockHoldersNum, lock_holders_sbuf.data, lock_waiters_sbuf.data;
 			else if (deadlock_state == DS_HARD_DEADLOCK)
 			{
 /*
@@ -1226,13 +1293,19 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
  */
 ereport(LOG,
 		(errmsg(process %d detected deadlock while waiting for %s on %s after %ld.%03d ms,
-			  MyProcPid, modename, buf.data, msecs, usecs)));
+MyProcPid, modename, buf.data, msecs, usecs),
+		 (errdetail_log_plural(Process holding the lock: %s. Request queue: %s.,
+			Processes holding lock: %s. Request queue: %s.,
+			   lockHoldersNum, lock_holders_sbuf.data, lock_waiters_sbuf.data;
 			}
 
 			if (myWaitStatus == STATUS_WAITING)
 ereport(LOG,
 		(errmsg(process %d still waiting for %s on %s after

Re: [HACKERS] Infinite recursion in row-security based on updatable s.b. views

2014-01-28 Thread Craig Ringer

On 01/24/2014 07:16 PM, Dean Rasheed wrote:
 think recursively calling the rewriter
 to expand view references in the new RLS qual, and
 expand_security_qual() to expand any additional RLS quals in the
 securityQuals list

With this, it'd be helpful if expand_security_qual(...) took a
RangeTblEntry instead of an rt_index.

That'd also be much more efficient with large rtables if we can arrange
a scan through the rtable when looking for security quals.

Like other places that operate on the rangetable while it's being
modified, we can walk the rangetable list up until the final entry that
existed when we started walking. This approach saves the series of
rt_fetch calls, which are something like O(n log n) for n relations.

It's safe because the operation will only append rangetable entries.

(I can't help wonder how much we'd gain by making the rtable an array
that gets doubled in size and copied whenever it overflows, rather than
a linked list, given all the walking of it that gets done, and how dead
entries to get flagged as dead rather than actually removed.)

I'm looking for where I found the code that already does this so I can
point and say I'm not crazy, we already do it here. Will follow up
with a patch.

-- 
 Craig Ringer   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] Storing pg_stat_statements query texts externally, pg_stat_statements in core

2014-01-28 Thread Peter Geoghegan

I noticed a minor omission in the patch as committed. Attached patch
corrects this.


-- 
Peter Geoghegan
*** a/contrib/pg_stat_statements/pg_stat_statements.c
--- b/contrib/pg_stat_statements/pg_stat_statements.c
*** generate_normalized_query(pgssJumbleStat
*** 2726,2732 
  		if (tok_len  0)
  			continue;			/* ignore any duplicates */
  
! 		/* Copy next chunk, or as much as will fit */
  		len_to_wrt = off - last_off;
  		len_to_wrt -= last_tok_len;
  
--- 2726,2732 
  		if (tok_len  0)
  			continue;			/* ignore any duplicates */
  
! 		/* Copy next chunk */
  		len_to_wrt = off - last_off;
  		len_to_wrt -= last_tok_len;
  

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

Re: [HACKERS] pg_basebackup and pg_stat_tmp directory

2014-01-28 Thread Magnus Hagander

On Tue, Jan 28, 2014 at 6:11 AM, Amit Kapila amit.kapil...@gmail.comwrote:

 On Tue, Jan 28, 2014 at 9:26 AM, Fujii Masao masao.fu...@gmail.com
 wrote:
  Hi,
 
  The files in pg_stat_tmp directory don't need to be backed up because
 they are
  basically reset at the archive recovery. So I think it's worth
  changing pg_basebackup
  so that it skips any files in pg_stat_tmp directory. Thought?

 I think this is good idea, but can't it also avoid
 PGSTAT_STAT_PERMANENT_TMPFILE along with temp files in
 pg_stat_tmp


All stats files should be excluded. IIRC the PGSTAT_STAT_PERMANENT_TMPFILE
refers to just the global one. You want to exclude based
on PGSTAT_STAT_PERMANENT_DIRECTORY (and of course based on the
guc stats_temp_directory if it's in PGDATA.

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

[HACKERS] Observed Compilation warning in WIN32 build

2014-01-28 Thread Rajeev rastogi

I observed below WIN32 compilation warnings in postmaster.c (seems introduced 
by commit ea9df812d8502fff74e7bc37d61bdc7d66d77a7f Relax the requirement that 
all lwlocks be stored in a single array.).

1.\src\backend\postmaster\postmaster.c(5625) : warning C4133: 
'=' : incompatible types - from 'LWLockPadded *' to 'LWLock *'
1.\src\backend\postmaster\postmaster.c(5856) : warning C4133: '=' : 
incompatible types - from 'LWLock *' to 'LWLockPadded *'

Attached is the patch with the fix.

Thanks and Regards,
Kumar Rajeev Rastogi



compile_issue_lwlock.patch
Description: compile_issue_lwlock.patch

-- 
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: Show process IDs of processes holding a lock; show relation and tuple infos of a lock to acquire

2014-01-28 Thread Rajeev rastogi

On 28/01/14, Christian Kruse wrote:
  I have checked the revised patch. It looks fine to me except one
 minor code formatting issue.
  In elog.c, two tabs are missing in the definition of function
 errdetail_log_plural.
  Please run pgindent tool to check the same.
 
 I did, but this reformats various other locations in the file, too.
 Nevertheless I now ran pg_indent against it and removed the other parts.
 Attached you will find the corrected patch version.
 
  Also I would like to highlight one behavior here is that process ID
 of
  process trying to acquire lock is also listed in the list of Request
 queue. E.g.
 
session 1 with process id X: BEGIN; LOCK TABLE foo IN SHARE
 MODE;
session 2 with process id Y: BEGIN; LOCK TABLE foo IN EXCLUSIVE
  MODE;
 
  On execution of LOCK in session-2, as part of log it will display as:
DETAIL:  Process holding the lock: X. Request queue: Y.
 
Where Y is the process ID of same process, which was trying to
 acquire lock.
 
 This is on purpose due to the rewording of the Message. In the first
 version the PID of the backend was missing.
 
 Thanks for the review!
 

Now patch looks fine to me. I am marking this as Ready for Committer.

Thanks and Regards,
Kumar Rajeev Rastogi


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

[HACKERS] Function definition removed but prototype still there

2014-01-28 Thread Rajeev rastogi

As part of the below commit

36a35c550ac114caa423bcbe339d3515db0cd957 (Compress GIN posting 
lists, for smaller index size.)

Function GinDataPageAddItemPointer definition was removed but corresponding 
prototype was still there.

Attached is the patch to fix the same.

Thanks and Regards,
Kumar Rajeev Rastogi



unwanted_prototype.patch
Description: unwanted_prototype.patch

-- 
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] UNION ALL on partitioned tables won't use indices.

2014-01-28 Thread Kyotaro HORIGUCHI

Hello, Thank you, and I' sorry to have kept you waiting.

 Let's focus on type 3; Tom and I both found it most promising.

Agreed.

  The attached two patches are rebased to current 9.4dev HEAD and
  make check at the topmost directory and src/test/isolation are
  passed without error. One bug was found and fixed on the way. It
  was an assertion failure caused by probably unexpected type
  conversion introduced by collapse_appendrels() which leads
  implicit whole-row cast from some valid reltype to invalid
  reltype (0) into adjust_appendrel_attrs_mutator().
 
 What query demonstrated this bug in the previous type 2/3 patches?
 
   - unionall_inh_idx_typ3_v4_20140114.patch
 
 You have not addressed my review comments from November:
 http://www.postgresql.org/message-id/20131123073913.ga1008...@tornado.leadboat.com

mmm. Sorry, I've overlooked most of it..

em_is_child is no more a issue, and the rest seem to cited below, thanks.

 Specifically, these:
 
 [transvar_merge_mutator()] has roughly the same purpose as
 adjust_appendrel_attrs_mutator(), but it propagates the change to far fewer
 node types.  Why this case so much simpler?  The parent translated_vars of
 parent_appinfo may bear mostly-arbitrary expressions.

There are only two places where AppendRelInfo node is generated,
expand_inherited_rtentry and
pull_up_union_leaf_queries.(_copyAppendrelInfo is irrelevant to
this discussion.) The core part generating translated_vars for
them are make_inh_translation_list and
make_setop_translation_list respectively. That's all. And they
are essentially works on the same way, making a Var for every
referred target entry of children like following.

| makeVar(varno,
| tle-resno,
| exprType((Node *) tle-expr),
| exprTypmod((Node *) tle-expr),
| exprCollation((Node *) tle-expr),
| 0);

So all we should do to collapse nested appendrels is simplly
connecting each RTEs directly to the root (most ancient?) RTE in
the relationship, resolving Vars like above, as seen in patched
expand_inherited_rtentry.

# If translated_vars are generated always in the way shown above,
# using tree walker might be no use..

This can be done apart from all other stuffs compensating
translation skew done in adjust_appendrel_attrs. I believe they
are in orthogonal relationship.


 Approaches (2) and (3) leave the inheritance parent with rte-inh == true
 despite no AppendRelInfo pointing to it as their parent.  Until now,
 expand_inherited_rtentry() has been careful to clear rte-inh in such cases.

Thank you. I missed that point. rte-inh at first works as a
trigger to try expand inheritance and create append_rel_list
entries, and later works to say dig me through appinfos. From
that point of view, inh of the RTEs whose children took away
should be 0. The two meanings of inh are now become different
from each other so I suppose it'd better rename it, but I don't
come up with good alternatives..

Anyway this is corrected in the patch attached and works as
follows,

BEFORE:
 rte[1] Subquery SELECT*1, inh = 1
+- appinfo[0] - rte[4] Relation p1, inh = 1
|   +- appinfo[2] - rte[6]  Relation p1, inh = 0
|   +- appinfo[3] - rte[7]  Relation c11, inh = 0
|   +- appinfo[4] - rte[8]  Relation c12, inh = 0
+- appinfo[1] - rte[5] Relation p2, inh = 1
+- appinfo[5] - rte[9]  Relation p1, inh = 0
+- appinfo[6] - rte[10] Relation c11, inh = 0
+- appinfo[7] - rte[11] Relation c12, inh = 0

COLLAPSED:
 rte[1] Subquery SELECT*1, inh = 1
+- appinfo[0] - rte[4]  Relation p1,  inh = 1 = 0
+- appinfo[2] - rte[6]  Relation p1,  inh = 0
+- appinfo[3] - rte[7]  Relation c11, inh = 0
+- appinfo[4] - rte[8]  Relation c12, inh = 0
+- appinfo[1] - rte[5]  Relation p2,  inh = 1 = 0
+- appinfo[5] - rte[9]  Relation p1,  inh = 0
+- appinfo[6] - rte[10] Relation c11, inh = 0
+- appinfo[7] - rte[11] Relation c12, inh = 0


 I get this warning:
 
 prepunion.c: In function `expand_inherited_rtentry':
 prepunion.c:1450: warning: passing argument 1 of `expression_tree_mutator' 
 from incompatible pointer type

Sorry, I forgot to put a casting to generic type. It is fixed in
the attached version.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c
index 52dcc72..6ef82d7 100644
--- a/src/backend/optimizer/prep/prepunion.c
+++ b/src/backend/optimizer/prep/prepunion.c
@@ -57,6 +57,11 @@ typedef struct
 	AppendRelInfo *appinfo;
 } adjust_appendrel_attrs_context;
 
+typedef struct {
+	AppendRelInfo	*child_appinfo;
+	Index			 target_rti;
+} transvars_merge_context;
+
 static Plan *recurse_set_operations(Node *setOp, PlannerInfo *root,
 	   double tuple_fraction,
 	   List *colTypes, List *colCollations,
@@ -98,6 +103,8 @@ static List *generate_append_tlist(List *colTypes, List

Re: [HACKERS] WIP patch (v2) for updatable security barrier views

2014-01-28 Thread Simon Riggs

On 28 January 2014 04:10, Kouhei Kaigai kai...@ak.jp.nec.com wrote:
  AFAICS the only area of objection is the handling of inherited
  relations, which occurs within the planner in the current patch. I can
  see that would be a cause for concern since the planner is pluggable
  and it would then be possible to bypass security checks. Obviously
  installing a new planner isn't trivial, but doing so shouldn't cause
  collateral damage.

 FWIW, I don't see any way _not_ to do that in RLS. If there are security
 quals on a child table, they must be added, and that can only happen once
 inheritance expansion happens. That's in the planner.

 I don't see it as acceptable to ignore security quals on child tables, and
 if we can't, we've got to do some work in the planner.

 (I'm starting to really loathe inheritance).

 Let me ask an elemental question. What is the reason why inheritance
 expansion logic should be located on the planner stage, not on the tail
 of rewriter?
 Reference to a relation with children is very similar to reference of
 multiple tables using UNION ALL. Isn't it a crappy idea to move the
 logic into rewriter stage (if we have no technical reason here)?

I agree that this is being seen the wrong way around. The planner
contains things it should not do, and as a result we are now
discussing enhancing the code that is in the wrong place, which of
course brings objections.

I think we would be best served by stopping inheritance in its tracks
and killing it off. It keeps getting in the way. What we need is real
partitioning. Other uses are pretty obscure and we should re-examine
things.

In the absence of that, releasing this updateable-security views
without suppport for inheritance is a good move. It gives us most of
what we want now and continuing to have some form of restriction is
better than having a much greater restriction of it not working at
all.

-- 
 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] Add min and max execute statement time in pg_stat_statement

2014-01-28 Thread KONDO Mitsumasa


(2014/01/28 15:17), Rajeev rastogi wrote:



On 27th January, Mitsumasa KONDO wrote:

2014-01-26 Simon Riggs si...@2ndquadrant.com
mailto:si...@2ndquadrant.com

 On 21 January 2014 19:48, Simon Riggs si...@2ndquadrant.com
 mailto:si...@2ndquadrant.com wrote:
   On 21 January 2014 12:54, KONDO Mitsumasa

kondo.mitsum...@lab.ntt.co.jp

 mailto:kondo.mitsum...@lab.ntt.co.jp wrote:
   Rebased patch is attached.
  
   Does this fix the Windows bug reported by Kumar on 20/11/2013 ?

Sorry, I was misunderstanding. First name of Mr. Rajeev Rastogi is
Kumar! I searched only e-mail address and title by his name...

I don't have windows compiler enviroment, but attached patch might be
fixed.
Could I ask Mr. Rajeev Rastogi to test my patch again?


I tried to test this but I could not apply the patch on latest git HEAD.
This may be because of recent patch (related to pg_stat_statement only
pg_stat_statements external query text storage ), which got committed
on 27th January.
Thank you for trying to test my patch. As you say, recently commit changes 
pg_stat_statements.c a lot. So I have to revise my patch. Please wait for a while.


By the way, latest pg_stat_statement might affect performance in Windows system. 
Because it uses fflush() system call every creating new entry in 
pg_stat_statements, and it calls many fread() to warm file cache. It works well 
in Linux system, but I'm not sure in Windows system. If you have time, could you 
test it on your Windows system? If it affects perfomance a lot, we can still 
change it.


Regards,
--
Mitsumasa KONDO
NTT Open Source Software Center


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

Re: [HACKERS] Infinite recursion in row-security based on updatable s.b. views

2014-01-28 Thread Craig Ringer

On 01/28/2014 04:39 PM, Craig Ringer wrote:
 I'm looking for where I found the code that already does this so I can
 point and say I'm not crazy, we already do it here. Will follow up
 with a patch.

I spoke to soon. The code I'm talking about is
expand_inherited_tables(...) and it still uses rt_fetch, it just avoids
foreach(...) in favour of stopping scanning at the end of the original
rtable.

So I get to be crazy after all.

I really don't like how many places we're rt_fetch'ing the same RTE from
in updatable s.b. views and its interaction with row-security, but that
can be a later problem. For now I'll stick with RTIs.

-- 
 Craig Ringer   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] A better way than tweaking NTUP_PER_BUCKET

2014-01-28 Thread Jeremy Harris


On 27/01/14 18:00, Simon Riggs wrote:

On 27 January 2014 17:44, Pavel Stehule pavel.steh...@gmail.com wrote:


This topic is interesting - we found very bad performance with hashing large
tables with high work_mem. MergeJoin with quicksort was significantly
faster.


I've seen this also.


I didn't deeper research - there is a possibility of virtualization
overhead.


I took measurements and the effect was repeatable and happened for all
sizes of work_mem, but nothing more to add.


FWIW my current list-based internal merge seems to perform worse at
larger work-mem, compared to quicksort.   I've been starting to wonder
if the rate of new ram-chip page opens is an issue (along with the
more-usually considered cache effects).  Any random-access workload
would be affected by this, if it really exists.
--
Cheers,
   Jeremy



--
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] PoC: Partial sort

2014-01-28 Thread Marti Raudsepp

On Tue, Jan 28, 2014 at 7:51 AM, Alexander Korotkov
aekorot...@gmail.com wrote:
 I didn't test it, but I worry that overhead might be high.
 If it's true then it could be like constraint_exclusion option which id off
 by default because of planning overhead.

I see, that makes sense.

I will try to find the time to run some benchmarks in the coming few days.

Regards,
Marti


-- 
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] alternative back-end block formats

2014-01-28 Thread Cédric Villemain

Le lundi 27 janvier 2014 13:42:29 Christian Convey a écrit :
 On Sun, Jan 26, 2014 at 5:47 AM, Craig Ringer cr...@2ndquadrant.com 
wrote:
  On 01/21/2014 07:43 PM, Christian Convey wrote:
   Does anyone know if this has been done before with Postgres?  I
   would
   have assumed yes, but I'm not finding anything in Google about
   people
   having done this.
  
  AFAIK (and I don't know much in this area) the storage manager isn't
  very pluggable compared to the rest of Pg.
 
 Thanks for the warning.  Duly noted.

As written in the meeting notes, Tom Lane revealed an interest in 
pluggable storage. So it might be interesting to check that.

https://wiki.postgresql.org/wiki/PgCon_2013_Developer_Meeting


-- 
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation

signature.asc
Description: This is a digitally signed message part.

Re: [HACKERS] Review: Patch FORCE_NULL option for copy COPY in CSV mode

2014-01-28 Thread Ian Lawrence Barwick

2013-11-01 Payal Singh pa...@omniti.com:
 The post was made before I subscribed to the mailing list, so posting my
 review separately. The link to the original patch mail is
 http://www.postgresql.org/message-id/CAB8KJ=jS-Um4TGwenS5wLUfJK6K4rNOm_V6GRUj+tcKekL2=g...@mail.gmail.com


 Hi,

 This patch implements the following TODO item:

   Allow COPY in CSV mode to control whether a quoted zero-length
   string is treated as NULL

   Currently this is always treated as a zero-length string,
   which generates an error when loading into an integer column

 Re: [PATCHES] allow CSV quote in NULL
 http://archives.postgresql.org/pgsql-hackers/2007-07/msg00905.php


   http://wiki.postgresql.org/wiki/Todo#COPY


 I had a very definite use-case for this functionality recently while
 importing
 CSV files generated by Oracle, and was somewhat frustrated by the
 existence
 of a FORCE_NOT_NULL option for specific columns, but not one for
 FORCE_NULL.

 I'll add this to the November commitfest.


 Regards

 Ian Barwick


 ==
 Contents  Purpose
 ==

 This patch introduces a new 'FORCE_NULL' option which has the opposite
 functionality of the already present 'FORCE_NOT_NULL' option for the COPY
 command. Prior to this option there was no way to convert a zeroed-length
 quoted value to a NULL value when COPY FROM is used to import data from CSV
 formatted files.

 ==
 Submission Review
 ==

 The patch is in the correct context diff format. It includes changes to the
 documentation as well as additional regression tests. The description has
 been discussed and defined in the previous mails leading to this patch.

 =
 Functionality Review
 =

 CORRECTION NEEDED - Due to a minor error in code (details in 'Code Review'
 section below), force_null option is not limited to COPY FROM, and works
 even when COPY TO is used. This should instead give an error message.

 The updated documentation describes the added functionality clearly.

 All regression tests passed successfully.

 Code compilation after including patch was successful. No warnings either.

 Manually tested COPY FROM with FORCE_NULL for a number of scenarios, all
 with expected outputs. No issues.

 Been testing the patch for a few days, no crashes or weird behavior
 observed.

 =
 Code Formatting Review (Needs Improvement)
 =

 Looks good, the tabs between variable declaration and accompanying comment
 can be improved.

 =
 Code Review (Needs Improvement)
 =

 1. There is a  NOTE: force_not_null option are not applied to the returned
 fields. before COPY FROM block. Similar note should be added for force_null
 option too.

 2. One of the conditions to check and give an error if force_null is true
 and copy from is false is wrong, cstate-force_null should be checked
 instead of cstate-force_notnull:

 /* Check force_notnull */
 if (!cstate-csv_mode  cstate-force_notnull != NIL)
 ereport(ERROR,
 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
  errmsg(COPY force not null available only
 in CSV mode)));
 if (cstate-force_notnull != NIL  !is_from)
 ereport(ERROR,
 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
   errmsg(COPY force not null only available using
 COPY FROM)));

 /* Check force_null */
 if (!cstate-csv_mode  cstate-force_null != NIL)
 ereport(ERROR,
 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
  errmsg(COPY force null available only in
 CSV mode)));

 ==  if (cstate-force_notnull != NIL  !is_from)
 ereport(ERROR,
 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
   errmsg(COPY force null only available using COPY
 FROM)));

 ===
 Suggested Changes  Conclusion
 ===

 The Above mentioned error condition should be corrected. Minor comments and
 tab changes are upto the author.

 In all, suggested modifications aside, the patch works well and in my
 opinion, would be a useful addition to the COPY command.

Hi Payal

Many thanks for the review, and my apologies for not getting back to
you earlier.

Updated version of the patch attached with suggested corrections. I'm not
sure about the tabs in the variable declarations - the whole section
seems to be all over the place (regardless of whether tabs are set to 4 or
8 spaces) and could do with tidying up, however I didn't want to expand the
scope of the patch too much.

Quick recap of the reasons behind this patch - we had a bunch of CSV files
(and by bunch I mean

Re: [HACKERS] Retain dynamic shared memory segments for postmaster lifetime

2014-01-28 Thread Kyotaro HORIGUCHI

Hello,

Currently there is no way user can keep the dsm
segments if he wants for postmaster lifetime, so I
have exposed a new API dsm_keep_segment()
to implement the same.

I had a short look on this patch.

- DSM implimentation seems divided into generic part (dsm.c) and
platform dependent part(dsm_impl.c). This dsm_keep_segment
puts WIN32 specific part directly into dms.c. I suppose it'd
be better defining DSM_OP_KEEP_SEGMENT and calling dms_impl_op
from dms_keep_segment, or something.

- Repeated calling of dsm_keep_segment even from different
backends creates new (orphan) handles as many as it is called.
Simplly invoking this function in some of extensions intending
to stick segments might results in so many orphan
handles. Something to curb that situation would be needed.

- Global/PostgreSQL.%u is the same literal constant with that
occurred in dsm_impl_windows. It should be defined as a
constant (or a macro).

- dms_impl_windows uses errcode_for_dynamic_shared_memory() for
ereport and it finally falls down to
errcode_for_file_access(). I think it is preferable, maybe.

The specs and need for this API is already discussed
in thread:
http://www.postgresql.org/message-id/ca+tgmoakogujqbedgeykysxud9eaidqx77j2_hxzrgfo3hr...@mail.gmail.com

I had used dsm_demo (hacked it a bit) module used
during initial tests for dsm API's to verify the working on
Windows. So one idea could be that I can extend
that module to use this new API, so that it can be tested
by others as well or if you have any other better way, please
do let me know.

I'll run on windows sooner:-)

As the discussion about its specs and need is already
discussed in above mentioned thread, so I will upload
this patch to CF unless there is any Objection.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

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

Re: [HACKERS] Observed Compilation warning in WIN32 build

2014-01-28 Thread Andres Freund

On 2014-01-28 09:13:15 +, Rajeev rastogi wrote:
 I observed below WIN32 compilation warnings in postmaster.c (seems introduced 
 by commit ea9df812d8502fff74e7bc37d61bdc7d66d77a7f Relax the requirement 
 that all lwlocks be stored in a single array.).
 
 1.\src\backend\postmaster\postmaster.c(5625) : warning 
 C4133: '=' : incompatible types - from 'LWLockPadded *' to 'LWLock *'
 1.\src\backend\postmaster\postmaster.c(5856) : warning C4133: '=' : 
 incompatible types - from 'LWLock *' to 'LWLockPadded *'
 
 Attached is the patch with the fix.
 
 Thanks and Regards,
 Kumar Rajeev Rastogi
 

 *** a/src/backend/postmaster/postmaster.c
 --- b/src/backend/postmaster/postmaster.c
 ***
 *** 5622,5628  save_backend_variables(BackendParameters *param, Port 
 *port,
   #ifndef HAVE_SPINLOCKS
   param-SpinlockSemaArray = SpinlockSemaArray;
   #endif
 ! param-MainLWLockArray = MainLWLockArray;
   param-ProcStructLock = ProcStructLock;
   param-ProcGlobal = ProcGlobal;
   param-AuxiliaryProcs = AuxiliaryProcs;
 --- 5622,5628 
   #ifndef HAVE_SPINLOCKS
   param-SpinlockSemaArray = SpinlockSemaArray;
   #endif
 ! param-MainLWLockArray = (LWLock*)MainLWLockArray;
   param-ProcStructLock = ProcStructLock;
   param-ProcGlobal = ProcGlobal;
   param-AuxiliaryProcs = AuxiliaryProcs;
 ***
 *** 5853,5859  restore_backend_variables(BackendParameters *param, Port 
 *port)
   #ifndef HAVE_SPINLOCKS
   SpinlockSemaArray = param-SpinlockSemaArray;
   #endif
 ! MainLWLockArray = param-MainLWLockArray;
   ProcStructLock = param-ProcStructLock;
   ProcGlobal = param-ProcGlobal;
   AuxiliaryProcs = param-AuxiliaryProcs;
 --- 5853,5859 
   #ifndef HAVE_SPINLOCKS
   SpinlockSemaArray = param-SpinlockSemaArray;
   #endif
 ! MainLWLockArray = (LWLockPadded*)param-MainLWLockArray;
   ProcStructLock = param-ProcStructLock;
   ProcGlobal = param-ProcGlobal;
   AuxiliaryProcs = param-AuxiliaryProcs;

This strikes me as the wrong fix, the types in BackendParams should be
changed instead.

Greetings,

Andres Freund

-- 
 Andres Freund 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] [PATCH] Use MAP_HUGETLB where supported (v3)

2014-01-28 Thread Heikki Linnakangas


On 01/27/2014 09:20 PM, Alvaro Herrera wrote:

Heikki Linnakangas wrote:


I spent some time whacking this around, new patch version attached.
I moved the mmap() code into a new function, that leaves the
PGSharedMemoryCreate more readable.


Did this patch go anywhere?


Oh darn, I remembered we had already committed this, but clearly not. 
I'd love to still get this into 9.4. The latest patch 
(hugepages-v5.patch) was pretty much ready for commit, except for 
documentation.


- Heikki


--
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] Use MAP_HUGETLB where supported (v3)

2014-01-28 Thread Christian Kruse

Hi,

On 28/01/14 13:51, Heikki Linnakangas wrote:
 Oh darn, I remembered we had already committed this, but clearly not. I'd
 love to still get this into 9.4. The latest patch (hugepages-v5.patch) was
 pretty much ready for commit, except for documentation.

I'm working on it. I ported it to HEAD and currently doing some
benchmarks. Next will be documentation.

Best regards,

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



pgpHMKmOD_jGn.pgp
Description: PGP signature

Re: [HACKERS] alternative back-end block formats

2014-01-28 Thread Christian Convey

On Tue, Jan 28, 2014 at 5:42 AM, Cédric Villemain ced...@2ndquadrant.comwrote:
...

 As written in the meeting notes, Tom Lane revealed an interest in
 pluggable storage. So it might be interesting to check that.

 https://wiki.postgresql.org/wiki/PgCon_2013_Developer_Meeting


Thanks.  I just read those meeting notes, and also Josh Berkus' blog on the
topic:
http://www.databasesoup.com/2013/05/postgresql-new-development-priorities-2.html

I was only thinking to enable pluggable operations on a single, specified
heap page, probably as a function of which table owned the page.  Josh's
blog seems to describe something a little broader in scope, although I
can't tell from that post exactly what functionality that entails.

Either way, this sounds like something I'd enjoy pitching in on, to
whatever extent I could be useful.  Has anyone started work on this yet?

Re: [HACKERS] [Review] inherit support for foreign tables

2014-01-28 Thread Etsuro Fujita


(2014/01/27 21:49), Shigeru Hanada wrote:

2014-01-27 Etsuro Fujita fujita.ets...@lab.ntt.co.jp:

(2014/01/25 11:27), Shigeru Hanada wrote:
Yeah, the consistency is essential for its ease of use.  But I'm not sure
that inherited stats ignoring foreign tables is actually useful for query
optimization.  What I think about the consistency is a) the ANALYZE command
with no table names skips ANALYZEing inheritance trees that include at least
one foreign table as a child, but b) the ANALYZE command with a table name
indicating an inheritance tree that includes any foreign tables does compute
the inherited stats in the same way as an inheritance tree consiting of only
ordinary tables by acquiring the sample rows from each foreign table on the
far side.


b) sounds little complex to understand or explain.

Is it too big change that making ANALYZE command to handle foreign
tables too even if no table name was specified?  IIRC, performance
issue was the reason to exclude foreign tables from auto-analyze
processing.  ANALYZEing large database contains local huge data also
takes long time.  One idea to avoid unexpected long processing is to
add option to foreign tables to mark it as not-auto-analyzable.


Maybe I didn't express my idea clearly.  Sorry for that.

I don't think that we now allow the ANALYZE command to handle foreign 
tables when no table name is specified with the command.  I think that 
we allow the ANALYZE command to handle an inheritance tree that includes 
foreign tables when the name of the parent table is specified, without 
ignoring such foreign tables in the caluculation.  ISTM it would be 
possible to do so if we introduce a new parameter, say, vac_mode, which 
indicates wether vacuum() is called with a specific table or not.


I'll try to modify the ANALYZE command to do so on top of you patch.

Thanks,

Best regards,
Etsuro Fujita


--
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] Use MAP_HUGETLB where supported (v3)

2014-01-28 Thread Christian Kruse

Hi,

On 15/11/13 15:17, Heikki Linnakangas wrote:
 I spent some time whacking this around, new patch version attached. I moved
 the mmap() code into a new function, that leaves the PGSharedMemoryCreate
 more readable.

I think there's a bug in this version of the patch. Have a look at
this:

+   if (huge_tlb_pages == HUGE_TLB_ON || huge_tlb_pages == HUGE_TLB_TRY)
+   {
[…]
+   ptr = mmap(NULL, *size, PROT_READ | PROT_WRITE,
+  PG_MMAP_FLAGS | MAP_HUGETLB, -1, 0);
[…]
+   }
+#endif
+
+   if (huge_tlb_pages == HUGE_TLB_OFF || huge_tlb_pages == HUGE_TLB_TRY)
+   {
+   allocsize = *size;
+   ptr = mmap(NULL, *size, PROT_READ | PROT_WRITE, PG_MMAP_FLAGS, 
-1, 0);
+   }

This will lead to a duplicate mmap() if hugepages work and
huge_tlb_pages == HUGE_TLB_TRY, or am I missing something?
I think it should be like this:

if (huge_tlb_pages == HUGE_TLB_OFF ||
(huge_tlb_pages == HUGE_TLB_TRY  ptr == MAP_FAILED))

Best regards,

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



pgpG9E74KaJDV.pgp
Description: PGP signature

Re: [HACKERS] KNN-GiST with recheck

2014-01-28 Thread Heikki Linnakangas


On 01/13/2014 07:17 PM, Alexander Korotkov wrote:

Here goes a desription of this patch same as in original thread.

KNN-GiST provides ability to get ordered results from index, but this order
is based only on index information. For instance, GiST index contains
bounding rectangles for polygons, and we can't get exact distance to
polygon from index (similar situation is in PostGIS). In attached patch,
GiST distance method can set recheck flag (similar to consistent method).
This flag means that distance method returned lower bound of distance and
we should recheck it from heap.

See an example.

create table test as (select id, polygon(3+(random()*10)::int,
circle(point(random(), random()), 0.0003 + random()*0.001)) as p from
generate_series(1,100) id);
create index test_idx on test using gist (p);

We can get results ordered by distance from polygon to point.

postgres=# select id, p - point(0.5,0.5) from test order by p -
point(0.5,0.5) limit 10;
id   |   ?column?
+--
  755611 | 0.000405855808916853
  807562 | 0.000464123777564343
  437778 | 0.000738524708741959
  947860 |  0.00076250998760724
  389843 | 0.000886362723569568
   17586 | 0.000981960100555216
  411329 |  0.00145338112316853
  894191 |  0.00149399559703506
  391907 |   0.0016647896049741
  235381 |  0.00167554614889509
(10 rows)

It's fast using just index scan.

 QUERY PLAN

--
  Limit  (cost=0.29..1.86 rows=10 width=36) (actual time=0.180..0.230
rows=10 loops=1)
-  Index Scan using test_idx on test  (cost=0.29..157672.29
rows=100 width=36) (actual time=0.179..0.228 rows=10 loops=1)
  Order By: (p - '(0.5,0.5)'::point)
  Total runtime: 0.305 ms
(4 rows)


Nice! Some thoughts:

1. This patch introduces a new polygon - point operator. That seems 
useful on its own, with or without this patch.


2. I wonder how useful it really is to allow mixing exact and non-exact 
return values from the distance function. The distance function included 
in the patch always returns recheck=true. I have a feeling that all 
other distance function will also always return either true or false.


3. A binary heap would be a better data structure to buffer the 
rechecked values. A Red-Black tree allows random insertions and 
deletions, but in this case you need to insert arbitrary values but only 
remove the minimum item. That's exactly what a binary heap excels at. We 
have a nice binary heap implementation in the backend that you can use, 
see src/backend/lib/binaryheap.c.


4. (as you mentioned in the other thread: ) It's a modularity violation 
that you peek into the heap tuple from gist. I think the proper way to 
do this would be to extend the IndexScan executor node to perform the 
re-shuffling of tuples that come from the index in wrong order, or 
perhaps add a new node type for it.


Of course that's exactly what your partial sort patch does :-). I 
haven't looked at that in detail, but I don't think the approach the 
partial sort patch takes will work here as is. In the KNN-GiST case, the 
index is returning tuples roughly in the right order, but a tuple that 
it returns might in reality belong somewhere later in the ordering. In 
the partial sort patch, the input stream of tuples is divided into 
non-overlapping groups, so that the tuples within the group are not 
sorted, but the groups are. I think the partial sort case is a special 
case of the KNN-GiST case, if you consider the lower bound of each tuple 
to be the leading keys that you don't need to sort.


BTW, this capability might also be highly useful for the min/max indexes 
as well. A min/max index cannot return an exact ordering of tuples, but 
it can also give a lower bound for a group of tuples.


- Heikki


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

Re: [HACKERS] Add force option to dropdb

2014-01-28 Thread salah jubeh

Hello Sawada,

- This patch is not patched to master branch 

Sorry, My mistake
//new connections are not allowed
Corrected.

I hope now the patch in better state, if somthing left, I will be glad to fix 
it 

Regards




On Tuesday, January 28, 2014 4:17 AM, Sawada Masahiko sawada.m...@gmail.com 
wrote:
 
On 2014年1月17日 0:56, salah jubeh s_ju...@yahoo.com wrote:

If the user owns objects, that will prevent this from working also.  I
have the feeling that adding DROP OWNED BY and/or REASSIGNED OWNED BY
calls to this utility would be a bit excessive, but who knows.

 Please find attached the first attempt to drop drop the client connections.
 I have added an option -k, --kill instead of force since killing client
 connection does not guarantee -drop force-.
 Regards


 On Tuesday, January 14, 2014 8:06 PM, Alvaro Herrera
 alvhe...@2ndquadrant.com wrote:
 salah jubeh wrote:

 For the sake of completeness:
 1. I think also, I need also to temporary disallow conecting to the
 database, is that right?
 2. Is there other factors can hinder dropping database?

 If the user owns objects, that will prevent this from working also.  I
 have the feeling that adding DROP OWNED BY and/or REASSIGNED OWNED BY
 calls to this utility would be a bit excessive, but who knows.


 3. Should I write two patches one for pg_version=9.2 and one for
 pg_version9.2


 No point -- nothing gets applied to branches older than current
 development anyway.


Thank you for the patch.
And sorry for delay in reviewing.

I started to look this patch, So the following is first review comment.

- This patch is not patched to master branch
I tried to patch this patch file to master branch, but I got following error.
$ cd postgresql
$ patch -d. -p1  ../dropdb.patch
can't find fiel to patch at input line 3
Perhaps you used the wrong -p or --strip option?
the text leading up to this was:
--
|--- dropdb_org.c 2014-01-16
|+++ dropdb.c 2014-01-16
--

There is not dropdb_org.c. I think  that you made mistake when the
patch is created.

- This patch is not according the coding rule
For example, line 71 of the patch:
//new connections are not allowed
It should be:
/* new connections are not allowed */
(Comment blocks that need specific line breaks should be formatted as
block comments, where the comment starts as /*--.)
Please refer to coding rule.
http://wiki.postgresql.org/wiki/Developer_FAQ#What.27s_the_formatting_style_used_in_PostgreSQL_source_code.3F


Regards,

---
Sawada Masahikodiff --git a/src/bin/scripts/dropdb.c b/src/bin/scripts/dropdb.c
index fa6ea3e..755abf4 100644
--- a/src/bin/scripts/dropdb.c
+++ b/src/bin/scripts/dropdb.c
@@ -32,6 +32,7 @@ main(int argc, char *argv[])
 		{echo, no_argument, NULL, 'e'},
 		{interactive, no_argument, NULL, 'i'},
 		{if-exists, no_argument, if_exists, 1},
+		{kill, no_argument, NULL, 'k'},
 		{maintenance-db, required_argument, NULL, 2},
 		{NULL, 0, NULL, 0}
 	};
@@ -48,6 +49,8 @@ main(int argc, char *argv[])
 	enum trivalue prompt_password = TRI_DEFAULT;
 	bool		echo = false;
 	bool		interactive = false;
+	bool		kill = false;
+	char *database_conn_limit = -1;
 
 	PQExpBufferData sql;
 
@@ -59,7 +62,7 @@ main(int argc, char *argv[])
 
 	handle_help_version_opts(argc, argv, dropdb, help);
 
-	while ((c = getopt_long(argc, argv, h:p:U:wWei, long_options, optindex)) != -1)
+	while ((c = getopt_long(argc, argv, h:p:U:wWeik, long_options, optindex)) != -1)
 	{
 		switch (c)
 		{
@@ -84,6 +87,9 @@ main(int argc, char *argv[])
 			case 'i':
 interactive = true;
 break;
+			case 'k':
+kill = true;
+break;
 			case 0:
 /* this covers the long options */
 break;
@@ -121,8 +127,6 @@ main(int argc, char *argv[])
 
 	initPQExpBuffer(sql);
 
-	appendPQExpBuffer(sql, DROP DATABASE %s%s;\n,
-	  (if_exists ? IF EXISTS  : ), fmtId(dbname));
 
 	/* Avoid trying to drop postgres db while we are connected to it. */
 	if (maintenance_db == NULL  strcmp(dbname, postgres) == 0)
@@ -131,11 +135,64 @@ main(int argc, char *argv[])
 	conn = connectMaintenanceDatabase(maintenance_db,
 			host, port, username, prompt_password, progname);
 
+	/* Disallow database connections and terminate client connections */
+	if (kill) 
+	{
+		appendPQExpBuffer(sql, SELECT datconnlimit FROM pg_database WHERE datname= '%s';,fmtId(dbname));
+		result = executeQuery(conn, sql.data, progname, echo);
+		/* Get the datconnÄºimit to do a cleanup in case of dropdb fail */
+		if (PQntuples(result) == 1)
+		{
+			database_conn_limit = PQgetvalue(result, 0, 0);
+		} else 
+		{
+			fprintf(stderr, _(%s: database removal failed: %s\n),
+			progname, dbname);
+			PQclear(result);
+			PQfinish(conn);
+			exit(1);
+		}
+		PQclear(result);
+		resetPQExpBuffer(sql);
+		/* New connections are not allowed */
+		appendPQExpBuffer(sql, UPDATE pg_database SET datconnlimit = 0 WHERE datname= '%s';\n,fmtId(dbname));
+		if (echo)
+			printf(%s, sql.data);
+		result = PQexec(conn,

Re: [HACKERS] Planning time in explain/explain analyze

2014-01-28 Thread Robert Haas

On Thu, Jan 9, 2014 at 11:45 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Greg Stark st...@mit.edu writes:
 On Thu, Jan 9, 2014 at 9:14 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 In short then, I think we should just add this to EXPLAIN and be done.
 -1 for sticking the info into PlannedStmt or anything like that.

 I'm confused. I thought I was arguing to support your suggestion that
 the initial planning store the time in the cached plan and explain
 should output the time the original planning took.

 Uh, no, wasn't my suggestion.  Doesn't that design imply measuring *every*
 planning cycle, explain or no?  I was thinking more of just putting the
 timing calls into explain.c.

The problem is that you really can't do it that way.
ExplainOneQuery() is a good place to add the timing calls in general,
but ExplainExecuteQuery() in prepare.c never calls it; it does
GetCachedPlan(), which ultimately calls pg_plan_query() after about
four levels of indirection, and then passes the resulting plan
directly to ExplainOnePlan().  So if you only add timing calls to
explain.c, you can't handle anything that goes through
ExplainExecuteQuery(), which is confusingly in prepare.c rather than
explain.c.

One reasonably principled solution is to just give up on showing the
plan time for prepared queries altogether.  If we don't want to adopt
that approach, then I think the right way to do this is to add a bool
timing argument to pg_plan_query().  When set, pg_plan_query()
measures the time before and after calling planner() and stores it in
the PlannedStmt.  It could alternatively return it via an out
parameter, but that's not very convenient for prepare.c, which is
planning a *list* of queries and therefore presumably needs planning
time for each one.

I'm not bent on this design; I just don't see another way to do this
that has any conceptual integrity.  Having the normal path measure the
time required to call pg_plan_query() and the prepared path measure
the time required to call GetCachedPlan() which may or may not
eventually call pg_plan_query() one or more times doesn't seem like a
good design, particularly if it's motivated solely by a desire to
minimize the code footprint of what's never going to be a very large
patch.  The most recent version of the patch tries to finesse this
issue by determining whether GetCachedPlan() actually did anything; I
think the way it does that is likely an abstraction violation that we
don't want to countenance. But even if we're OK with that, it still
munges the planning times of multiple queries into a single number,
while the normal case separates them.  It just seems like we're going
to a lot of trouble here to avoid the obvious design, and I'm not sure
why.

-- 
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] KNN-GiST with recheck

2014-01-28 Thread Alexander Korotkov

On Tue, Jan 28, 2014 at 5:54 PM, Heikki Linnakangas hlinnakan...@vmware.com
 wrote:

 On 01/13/2014 07:17 PM, Alexander Korotkov wrote:

 Here goes a desription of this patch same as in original thread.

 KNN-GiST provides ability to get ordered results from index, but this
 order
 is based only on index information. For instance, GiST index contains
 bounding rectangles for polygons, and we can't get exact distance to
 polygon from index (similar situation is in PostGIS). In attached patch,
 GiST distance method can set recheck flag (similar to consistent method).
 This flag means that distance method returned lower bound of distance and
 we should recheck it from heap.

 See an example.

 create table test as (select id, polygon(3+(random()*10)::int,
 circle(point(random(), random()), 0.0003 + random()*0.001)) as p from
 generate_series(1,100) id);
 create index test_idx on test using gist (p);

 We can get results ordered by distance from polygon to point.

 postgres=# select id, p - point(0.5,0.5) from test order by p -
 point(0.5,0.5) limit 10;
 id   |   ?column?
 +--
   755611 | 0.000405855808916853
   807562 | 0.000464123777564343
   437778 | 0.000738524708741959
   947860 |  0.00076250998760724
   389843 | 0.000886362723569568
17586 | 0.000981960100555216
   411329 |  0.00145338112316853
   894191 |  0.00149399559703506
   391907 |   0.0016647896049741
   235381 |  0.00167554614889509
 (10 rows)

 It's fast using just index scan.

  QUERY PLAN

 
 --
   Limit  (cost=0.29..1.86 rows=10 width=36) (actual time=0.180..0.230
 rows=10 loops=1)
 -  Index Scan using test_idx on test  (cost=0.29..157672.29
 rows=100 width=36) (actual time=0.179..0.228 rows=10 loops=1)
   Order By: (p - '(0.5,0.5)'::point)
   Total runtime: 0.305 ms
 (4 rows)


 Nice! Some thoughts:

 1. This patch introduces a new polygon - point operator. That seems
 useful on its own, with or without this patch.


Yeah, but exact-knn cant come with no one implementation. But it would
better come in a separate patch.

2. I wonder how useful it really is to allow mixing exact and non-exact
 return values from the distance function. The distance function included in
 the patch always returns recheck=true. I have a feeling that all other
 distance function will also always return either true or false.


For geometrical datatypes recheck variations in consistent methods are also
very rare (I can't remember any). But imagine opclass for arrays where keys
have different representation depending on array length. For such opclass
and knn on similarity recheck flag could be useful.



 3. A binary heap would be a better data structure to buffer the rechecked
 values. A Red-Black tree allows random insertions and deletions, but in
 this case you need to insert arbitrary values but only remove the minimum
 item. That's exactly what a binary heap excels at. We have a nice binary
 heap implementation in the backend that you can use, see
 src/backend/lib/binaryheap.c.


Hmm. For me binary heap would be a better data structure for KNN-GiST at
all :-)


 4. (as you mentioned in the other thread: ) It's a modularity violation
 that you peek into the heap tuple from gist. I think the proper way to do
 this would be to extend the IndexScan executor node to perform the
 re-shuffling of tuples that come from the index in wrong order, or perhaps
 add a new node type for it.

 Of course that's exactly what your partial sort patch does :-). I haven't
 looked at that in detail, but I don't think the approach the partial sort
 patch takes will work here as is. In the KNN-GiST case, the index is
 returning tuples roughly in the right order, but a tuple that it returns
 might in reality belong somewhere later in the ordering. In the partial
 sort patch, the input stream of tuples is divided into non-overlapping
 groups, so that the tuples within the group are not sorted, but the groups
 are. I think the partial sort case is a special case of the KNN-GiST case,
 if you consider the lower bound of each tuple to be the leading keys that
 you don't need to sort.


Yes. But, for instance btree accesses heap for unique checking. Is really
it so crimilal? :-)
This is not only question of a new node or extending existing node. We need
to teach planner/executor access method can return value of some expression
which is lower bound of another expression. AFICS now access method can
return only original indexed datums and TIDs. So, I afraid that enormous
infrastructure changes are required. And I can hardly imagine what they
should look like.

--
With best regards,
Alexander Korotkov.

[HACKERS] bison, flex and ./configure

2014-01-28 Thread salah jubeh

Hello, 


Today, I have noticed that ./configure does not return an error when bison and 
flex are missing.  Is this intended ?


OS: Ubuntu 13.04


Regards

Re: [HACKERS] bison, flex and ./configure

2014-01-28 Thread Heikki Linnakangas


On 01/28/2014 04:14 PM, salah jubeh wrote:

Today, I have noticed that ./configure does not return an error when bison and 
flex are missing.  Is this intended ?


Yes. Bison and flex are not required when building from a source 
tarball, because the tarball includes the generated files. If you're 
building from a git checkout, however, then you need bison and flex. You 
will get an error at make, and IIRC a warning at ./configure.


- Heikki


--
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] bison, flex and ./configure

2014-01-28 Thread salah jubeh

Yes. Bison and flex are not required when building from a source 
tarball, because the tarball includes the generated files. If you're 
building from a git checkout, however, then you need bison and flex. You 
will get an error at make, and IIRC a warning at ./configure

Thanks for the quick reply. For curiosity reasons why the differentiation 
between tar and git.

Regards




On Tuesday, January 28, 2014 3:18 PM, Heikki Linnakangas 
hlinnakan...@vmware.com wrote:
 
On 01/28/2014 04:14 PM, salah jubeh wrote:

 Today, I have noticed that ./configure does not return an error when bison 
 and flex are missing.  Is this intended ?

Yes. Bison and flex are not required when building from a source 
tarball, because the tarball includes the generated files. If you're 
building from a git checkout, however, then you need bison and flex. You 
will get an error at make, and IIRC a warning at ./configure.

- Heikki

1 2 >

1 - 100 of 176 matches

Mail list logo