Re: [HACKERS] Re: [COMMITTERS] pgsql: Force strings passed to and from plperl to be in UTF8 encoding.

[Robert Haas]

On Wed, Oct 5, 2011 at 5:03 PM, Alex Hunsaker  wrote:
> On Wed, Oct 5, 2011 at 08:18, Robert Haas  wrote:
>> On Wed, Oct 5, 2011 at 3:58 AM, Amit Khandekar
>>  wrote:
>>> I have no more issues with the patch.
>>> Thanks!
>>
>> I think this patch needs to be added to the open CommitFest, with
>> links to the reviews, and marked Ready for Committer.
>
> The open commitfest? Even if its an "important" bug fix that should be
> backpatched?

Considering that the issue appears to have been ignored from
mid-February until early October, I don't see why it should now get to
jump to the head of the queue.  Other people may have different
opinions, of course.

-- 
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] alter table only ... drop constraint broken in HEAD

[Robert Haas]

On Wed, Oct 5, 2011 at 5:53 PM, Alex Hunsaker  wrote:
> tldr:
>
> Seems to be broken by
> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=4da99ea4231e3d8bbf28b666748c1028e7b7d665
> :
> commit 4da99ea4231e3d8bbf28b666748c1028e7b7d665
> Author: Robert Haas 
> Date:   Mon Jun 27 10:27:17 2011 -0400
>
>    Avoid having two copies of the HOT-chain search logic.

Hmm... that's pretty terrible.  Yikes.  That commit wasn't intended to
change any behavior, just to clean up the code, so I think the right
thing to do here is figure out how I changed the behavior without
meaning too, rather than to start patching up all the places that
might have been affected by whatever the behavior change was.  I'm too
tired to figure this out right now, but I'll spend some time staring
at it tomorrow.

-- 
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] Bug in walsender when calling out to do_pg_stop_backup (and others?)

[Fujii Masao]

On Wed, Oct 5, 2011 at 10:30 PM, Magnus Hagander  wrote:
> When walsender calls out to do_pg_stop_backup() (during base backups),
> it is not possible to terminate the process with a SIGTERM - it
> requires a SIGKILL. This can leave unkillable backends for example if
> archive_mode is on and archive_command is failing (or not set). A
> similar thing would happen in other cases if walsender calls out to
> something that would block (do_pg_start_backup() for example), but the
> stop one is easy to provoke.

Good catch!

> ISTM one way to fix it is the attached, which is to have walsender set
> the "global" flags saying that we have received sigterm, which in turn
> causes the CHECK_FOR_INTERRUPTS() calls in the routines to properly
> exit the process. AFAICT it works fine. Any holes in this approach?

Very simple patch. Looks fine.

> Second, I wonder if we should add a SIGINT handler as well, that would
> make it possible to send a cancel signal. Given that the end result
> would be the same (at least if we want to keep with the "walsender is
> simple" path), I'm not sure it's necessary - but it would at least
> help those doing pg_cancel_backend()... thoughts?

I don't think that's necessary because, as you suggested, there is no
use case for *now*. We can add that handler when someone proposes
the feature which requires that.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
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] Bug with pg_ctl -w/wait and config-only directories

[Mr. Aaron W. Swenson]

On Wed, Oct 05, 2011 at 07:59:07PM -0400, Bruce Momjian wrote:
> Mr. Aaron W. Swenson wrote:
> > > With the patch I am going to commit, you will not need to use one of the
> > > -D flags because pg_ctl will find the data directory location;  you will
> > > just specify the config-only directory with one -D, and the
> > > --data-directory.
> > 
> > So, you're saying that my passing the config directory to pg_ctl through
> > '-D' will eliminate the need -- as far as config and data directories are
> > concerned -- to pass options directly to the postmaster?
> 
> Yes, in PG 9.2.

\o/

-- 
Mr. Aaron W. Swenson
Gentoo Linux Developer
Email: titanof...@gentoo.org
GnuPG FP : 2C00 7719 4F85 FB07 A49C  0E31 5713 AA03 D1BB FDA0
GnuPG ID : D1BBFDA0


pgpedIo5z4j4c.pgp
Description: PGP signature

[HACKERS] [PATCH] Log crashed backend's query v3

[Marti Raudsepp]

Hi,

My apologies for the last grumpy message I wrote; I sent it out of
surprise that my patch was marked "closed" on the current commitfest.
I hope I haven't discouraged you from reviewing this updated version.

I think you intended to use the "Waiting on Author" status -- that
leaves the commitfest entry open. I will re-open the commitfest entry
myself, I hope that's OK.

Here is version 3 of the patch.

On Wed, Oct 5, 2011 at 02:36, gabrielle  wrote:
> - The doc comment 'pgstat_get_backend_current_activity' doesn't match
> the function name 'pgstat_get_crashed_backend_activity'.

This is now fixed in the patch.

> Project coding guidelines:
> - There are some formatting problems, such as all newlines at the same
> indent level need to line up.
> - Wayward tabs, line 2725 in pgstat.c specifically

I wasn't entirely sure which line you're referring to (what's on line
2725 in the current git master wasn't touched by me)
But I presume it's this statement:
if (activity < BackendActivityBuffer || ...

I have rearranged this if statement using a temporary variable so that
there's no need to right-align long expressions anymore.

I also tweaked the wording of comments here and there.

For the points not addressed in this version, see my response here:
http://archives.postgresql.org/pgsql-hackers/2011-10/msg00202.php

Regards,
Marti
From 748c5fcaf1dc7518a06da276574a565098cde628 Mon Sep 17 00:00:00 2001
From: Marti Raudsepp 
Date: Wed, 28 Sep 2011 00:46:48 +0300
Subject: [PATCH] Log crashed backend's query (activity string)

The crashing query is often a good starting point in debugging the
cause, and much more easily accessible than core dumps.

We're extra-paranoid in reading the activity buffer since it might be
corrupt. All non-ASCII characters are replaced with '?'

Example output:
LOG:  server process (PID 31451) was terminated by signal 9: Killed
DETAIL:  Running query: DO LANGUAGE plpythonu 'import os;os.kill(os.getpid(),9)'
---
 src/backend/postmaster/pgstat.c |   66 +++
 src/backend/postmaster/postmaster.c |   17 ++---
 src/backend/utils/adt/ascii.c   |   31 
 src/include/pgstat.h|1 +
 src/include/utils/ascii.h   |1 +
 5 files changed, 111 insertions(+), 5 deletions(-)

diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 9132db7..220507f 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -58,6 +58,7 @@
 #include "storage/pg_shmem.h"
 #include "storage/pmsignal.h"
 #include "storage/procsignal.h"
+#include "utils/ascii.h"
 #include "utils/guc.h"
 #include "utils/memutils.h"
 #include "utils/ps_status.h"
@@ -2751,6 +2752,71 @@ pgstat_get_backend_current_activity(int pid, bool checkUser)
 	return "";
 }
 
+/* --
+ * pgstat_get_crashed_backend_activity() -
+ *
+ *	Return a string representing the current activity of the backend with
+ *	the specified PID.	Like the function above, but reads shared memory with
+ *	the expectation that it may be corrupt.
+ *
+ *	This function is only intended to be used by postmaster to report the
+ *	query that crashed the backend. In particular, no attempt is made to
+ *	follow the correct concurrency protocol when accessing the
+ *	BackendStatusArray. But that's OK, in the worst case we get a corrupted
+ *	message. We also must take care not to trip on ereport(ERROR).
+ *
+ *	Note: return strings for special cases match pg_stat_get_backend_activity.
+ * --
+ */
+const char *
+pgstat_get_crashed_backend_activity(int pid)
+{
+	volatile PgBackendStatus *beentry;
+	int			i;
+
+	beentry = BackendStatusArray;
+	for (i = 1; i <= MaxBackends; i++)
+	{
+		if (beentry->st_procpid == pid)
+		{
+			/* Read pointer just once, so it can't change after validation */
+			const char *activity = beentry->st_activity;
+			char	   *buffer;
+			const char *activity_last;
+
+			/*
+			 * We can't access activity pointer before we verify that it
+			 * falls into BackendActivityBuffer. To make sure that the entire
+			 * string including its ending is contained within the buffer,
+			 * the pointer can start at offset (MaxBackends - 1) at most.
+			 */
+			activity_last = BackendActivityBuffer +
+((MaxBackends - 1) * pgstat_track_activity_query_size);
+
+			if (activity < BackendActivityBuffer ||
+activity > activity_last)
+return "";
+
+			if (*(activity) == '\0')
+return "";
+
+			buffer = (char *) palloc(pgstat_track_activity_query_size);
+			/*
+			 * Copy only ASCII-safe characters so we don't run into encoding
+			 * problems when reporting the message.
+			 */
+			ascii_safe_strncpy(buffer, activity,
+			   pgstat_track_activity_query_size);
+
+			return buffer;
+		}
+
+		beentry++;
+	}
+
+	/* PID not found */
+	return "";
+}
 
 /* 
  * Local support functions follow
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postm

Re: [HACKERS] Bug with pg_ctl -w/wait and config-only directories

[Bruce Momjian]

Mr. Aaron W. Swenson wrote:
> > With the patch I am going to commit, you will not need to use one of the
> > -D flags because pg_ctl will find the data directory location;  you will
> > just specify the config-only directory with one -D, and the
> > --data-directory.
> 
> So, you're saying that my passing the config directory to pg_ctl through
> '-D' will eliminate the need -- as far as config and data directories are
> concerned -- to pass options directly to the postmaster?

Yes, in PG 9.2.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

-- 
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] Bug with pg_ctl -w/wait and config-only directories

[Mr. Aaron W. Swenson]

On Wed, Oct 05, 2011 at 07:20:16PM -0400, Bruce Momjian wrote:
> Mr. Aaron W. Swenson wrote:
> -- Start of PGP signed section.
> > On Wed, Oct 05, 2011 at 10:44:38AM -0400, Bruce Momjian wrote:
> > > Peter Eisentraut wrote:
> > > > On m?n, 2011-10-03 at 15:09 -0400, Bruce Momjian wrote:
> > > > > Why were people not using pg_ctl?
> > > > 
> > > > Actually, a slight correction/addition here: The Debian init script does
> > > > use pg_ctl to start the service.  Seems to work fine.
> > > 
> > > Yes.  The script authors discovered a working behavior, which matches my
> > > research:
> > > 
> > > pg_ctl startspecify config directory
> > > pg_ctl -w start impossible
> > > ...
> > 
> > 
> > Maybe I'm misunderstanding what you've written, but for 'pg_ctl -w' it is
> > possible. The following command does work (I've replaced the variables
> > with their default values to make it easier to read):
> > 
> >   su -l postgres \
> > -c "env PGPORT=\"5432\" /usr/lib/postgresql-9.1/bin/pg_ctl start -w \
> > -t 60 -s -D /var/lib/postgresql/9.1/data/ \
> >  -o '-D /etc/postgresql-9.1/ \
> > --data-directory=/var/lib/postgresql/9.1/data/ \
> > --silent-mode=true'"
> 
> Wow.  So you are telling pg_ctl to use the real data directory, but are
> passing flags to the postmaster to start with the config directory and
> also setting data-directory to the real data directory.  I am impressed.
> 
> I can see how that would work.  Did you look at the C code to figure
> this out or did you just test until it worked, or did it just seem
> logical to you?

No, I didn't look into the C code. (Though, I have poked around in there a
bit.)

Being a Gentoo user has taught me a few things like one should always
'read the friendly manual.' It just seemed logical after reading the
pg_ctl documentation that passing options to the postmaster would do the
trick, which spurred my testing several variations until it worked. But, I
only knew that I could give 'postgres'/'postmaster' the configuration
directory and it would do the right thing.
 
> With the patch I am going to commit, you will not need to use one of the
> -D flags because pg_ctl will find the data directory location;  you will
> just specify the config-only directory with one -D, and the
> --data-directory.

So, you're saying that my passing the config directory to pg_ctl through
'-D' will eliminate the need -- as far as config and data directories are
concerned -- to pass options directly to the postmaster?

-- 
Mr. Aaron W. Swenson
Gentoo Linux Developer
Email: titanof...@gentoo.org
GnuPG FP : 2C00 7719 4F85 FB07 A49C  0E31 5713 AA03 D1BB FDA0
GnuPG ID : D1BBFDA0


pgpZN3TMQo6L7.pgp
Description: PGP signature

Re: [HACKERS] Bug with pg_ctl -w/wait and config-only directories

[Bruce Momjian]

Mr. Aaron W. Swenson wrote:
-- Start of PGP signed section.
> On Wed, Oct 05, 2011 at 10:44:38AM -0400, Bruce Momjian wrote:
> > Peter Eisentraut wrote:
> > > On m?n, 2011-10-03 at 15:09 -0400, Bruce Momjian wrote:
> > > > Why were people not using pg_ctl?
> > > 
> > > Actually, a slight correction/addition here: The Debian init script does
> > > use pg_ctl to start the service.  Seems to work fine.
> > 
> > Yes.  The script authors discovered a working behavior, which matches my
> > research:
> > 
> > pg_ctl startspecify config directory
> > pg_ctl -w start impossible
> > ...
> 
> 
> Maybe I'm misunderstanding what you've written, but for 'pg_ctl -w' it is
> possible. The following command does work (I've replaced the variables
> with their default values to make it easier to read):
> 
>   su -l postgres \
> -c "env PGPORT=\"5432\" /usr/lib/postgresql-9.1/bin/pg_ctl start -w \
> -t 60 -s -D /var/lib/postgresql/9.1/data/ \
>  -o '-D /etc/postgresql-9.1/ \
> --data-directory=/var/lib/postgresql/9.1/data/ \
> --silent-mode=true'"

Wow.  So you are telling pg_ctl to use the real data directory, but are
passing flags to the postmaster to start with the config directory and
also setting data-directory to the real data directory.  I am impressed.

I can see how that would work.  Did you look at the C code to figure
this out or did you just test until it worked, or did it just seem
logical to you?

With the patch I am going to commit, you will not need to use one of the
-D flags because pg_ctl will find the data directory location;  you will
just specify the config-only directory with one -D, and the
--data-directory.

pg_upgrade could use this method if it had -o options for old and new
clusters.  Right now it has -p and -P and those could be removed and use
-o/-O instead, but it would require parsing the -o string, with is
problematic.  The right solution for the port number would be to use the
new postmaster -C option, pass the -o into that, and read the port
number with 'postmaster -C port'.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

-- 
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] Log crashed backend's query v2

[Marti Raudsepp]

On Wed, Oct 5, 2011 at 02:36, gabrielle  wrote:
> This review was compiled from a PDXPUG group review (Dan Colish, Mark
> Wong, Brent Dombrowski, and Gabrielle Roth).

Whaat, you marked the patch as "Returned with Feedback" based on this review?

The only obvious change I need to make in response to your feedback is
the function name fix in a comment. Most points are incorrect: there's
no regression test in this patch and no requirement of plpythonu. I
didn't introduce any new messages with the text "unknown". The
behavior of ascii_safe_strncpy is deliberate and was implemented from
feedback on the first patch version.

What's left is the indentation alignment in the if() statement. No way
is that a a reason to delay the patch to the next CommitFest!

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

[HACKERS] alter table only ... drop constraint broken in HEAD

[Alex Hunsaker]

tldr:

Seems to be broken by
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=4da99ea4231e3d8bbf28b666748c1028e7b7d665
:
commit 4da99ea4231e3d8bbf28b666748c1028e7b7d665
Author: Robert Haas 
Date:   Mon Jun 27 10:27:17 2011 -0400

Avoid having two copies of the HOT-chain search logic.

Full story:

While playing with the non inheritable constraints patch
(https://commitfest.postgresql.org/action/patch_view?id=611) I noticed
the following sequence was broken:
create table colors (c text check (not null));
create table colors_i () inherits (colors);
alter table only colors drop constraint colors_check;
ERROR:  relation 16412 has non-inherited constraint "colors_check"

Naturally I assumed it was the patches fault, but after further
digging turns out it was not. I bisected it down to the above commit
(for those that have not tried git bisect and git bisect run its great
for this kind of thing).

The basic problem seems to be after we update pg_constraint to
decrement inhcount for a child table we call
CommandCounterIncrement(); then we fetch the next row from
pg_constraint... which happens to be the row we just updated. So we
try to decrement it again, only now its at 0 which shouldn't happen so
we error out.

The simple fix seemed to be to move the CommandCounterIncrement()
outside of the while(... systable_getnext()) loop. Im not sure if
that's the right thing to-do or if there is some other bug here...
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***
*** 6734,6739  ATExecDropConstraint(Relation rel, const char *constrName,
--- 6734,6740 
  	{
  		Oid			childrelid = lfirst_oid(child);
  		Relation	childrel;
+ 		bool		updated = false;
  
  		/* find_inheritance_children already got lock */
  		childrel = heap_open(childrelid, NoLock);
***
*** 6790,6798  ATExecDropConstraint(Relation rel, const char *constrName,
  	con->coninhcount--;
  	simple_heap_update(conrel, ©_tuple->t_self, copy_tuple);
  	CatalogUpdateIndexes(conrel, copy_tuple);
! 
! 	/* Make update visible */
! 	CommandCounterIncrement();
  }
  			}
  			else
--- 6791,6798 
  	con->coninhcount--;
  	simple_heap_update(conrel, ©_tuple->t_self, copy_tuple);
  	CatalogUpdateIndexes(conrel, copy_tuple);
! 	/* need to call CommandCounterIncrement() */
! 	updated = true;
  }
  			}
  			else
***
*** 6807,6815  ATExecDropConstraint(Relation rel, const char *constrName,
  
  simple_heap_update(conrel, ©_tuple->t_self, copy_tuple);
  CatalogUpdateIndexes(conrel, copy_tuple);
! 
! /* Make update visible */
! CommandCounterIncrement();
  			}
  
  			heap_freetuple(copy_tuple);
--- 6807,6814 
  
  simple_heap_update(conrel, ©_tuple->t_self, copy_tuple);
  CatalogUpdateIndexes(conrel, copy_tuple);
! /* need to call CommandCounterIncrement() */
! updated = true;
  			}
  
  			heap_freetuple(copy_tuple);
***
*** 6824,6829  ATExecDropConstraint(Relation rel, const char *constrName,
--- 6823,6832 
  	   constrName,
  	   RelationGetRelationName(childrel;
  
+ 		/* Make update visible */
+ 		if (updated)
+ 			CommandCounterIncrement();
+ 
  		heap_close(childrel, NoLock);
  	}
  
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
***
*** 2103,2105  ALTER TABLE tt7 NOT OF;
--- 2103,2112 
   x  | integer  | 
   y  | numeric(8,2) | 
  
+ -- make sure we can drop a constraint on the parent but it remains on the child
+ create table test_drop_constr_parent (c text check (c is not null));
+ create table test_drop_constr_child () inherits (test_drop_constr_parent);
+ alter table only test_drop_constr_parent drop constraint "test_drop_constr_parent_c_check";
+ -- should fail
+ insert into test_drop_constr_child (c) values (NULL);
+ ERROR:  new row for relation "test_drop_constr_child" violates check constraint "test_drop_constr_parent_c_check"
*** a/src/test/regress/sql/alter_table.sql
--- b/src/test/regress/sql/alter_table.sql
***
*** 1456,1458  CREATE TYPE tt_t1 AS (x int, y numeric(8,2));
--- 1456,1465 
  ALTER TABLE tt7 OF tt_t1;			-- reassign an already-typed table
  ALTER TABLE tt7 NOT OF;
  \d tt7
+ 
+ -- make sure we can drop a constraint on the parent but it remains on the child
+ create table test_drop_constr_parent (c text check (c is not null));
+ create table test_drop_constr_child () inherits (test_drop_constr_parent);
+ alter table only test_drop_constr_parent drop constraint "test_drop_constr_parent_c_check";
+ -- should fail
+ insert into test_drop_constr_child (c) values (NULL);

-- 
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] Bug with pg_ctl -w/wait and config-only directories

[Mr. Aaron W. Swenson]

On Wed, Oct 05, 2011 at 10:44:38AM -0400, Bruce Momjian wrote:
> Peter Eisentraut wrote:
> > On m?n, 2011-10-03 at 15:09 -0400, Bruce Momjian wrote:
> > > Why were people not using pg_ctl?
> > 
> > Actually, a slight correction/addition here: The Debian init script does
> > use pg_ctl to start the service.  Seems to work fine.
> 
> Yes.  The script authors discovered a working behavior, which matches my
> research:
> 
> pg_ctl startspecify config directory
> pg_ctl -w start impossible
> ...


Maybe I'm misunderstanding what you've written, but for 'pg_ctl -w' it is
possible. The following command does work (I've replaced the variables
with their default values to make it easier to read):

  su -l postgres \
-c "env PGPORT=\"5432\" /usr/lib/postgresql-9.1/bin/pg_ctl start -w \
-t 60 -s -D /var/lib/postgresql/9.1/data/ \
 -o '-D /etc/postgresql-9.1/ \
--data-directory=/var/lib/postgresql/9.1/data/ \
--silent-mode=true'"

-- 
Mr. Aaron W. Swenson
Gentoo Linux Developer
Email: titanof...@gentoo.org
GnuPG FP : 2C00 7719 4F85 FB07 A49C  0E31 5713 AA03 D1BB FDA0
GnuPG ID : D1BBFDA0


pgpW11PKX3kCL.pgp
Description: PGP signature

Re: [HACKERS] Re: [COMMITTERS] pgsql: Force strings passed to and from plperl to be in UTF8 encoding.

[Alex Hunsaker]

On Wed, Oct 5, 2011 at 08:18, Robert Haas  wrote:
> On Wed, Oct 5, 2011 at 3:58 AM, Amit Khandekar
>  wrote:
>> I have no more issues with the patch.
>> Thanks!
>
> I think this patch needs to be added to the open CommitFest, with
> links to the reviews, and marked Ready for Committer.

The open commitfest? Even if its an "important" bug fix that should be
backpatched?

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

[HACKERS] Review: Non-inheritable check constraints

[Alex Hunsaker]

Hi! *Waves*

First off, it all seems to work as described:
- regressions pass
- domains work
- tried various inherit options (merging constraints, alter table no
inherit etc)
- pg_dump/restore

I didn't care for the changes to gram.y so I reworked it a bit so we
now pass is_only to AddRelationNewConstraint() (like we do with
is_local). Seemed simpler but maybe I missed something. Comments?

I also moved the is_only check in AtAddCheckConstraint() to before we
grab and loop through any children. Seemed a bit wasteful to loop
through the new constraints just to set a flag so that we could bail
out while looping through the children.

You also forgot to bump Natts_pg_constraint.

PFA the above changes as well as being "rebased" against master.


non_inh_check_v2.patch.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] Action requested - Application Softblock implemented | Issue report ID341057

[Dave Page]

On Wed, Oct 5, 2011 at 7:58 PM, Seiko Ishida (MP Tech Consulting LLC) <
v-sei...@microsoft.com> wrote:

>
> Would this download site be a good URL for that?
> *http://www.enterprisedb.com/products-services-training/pgdownload*
>
>
No, please use http://www.postgresql.org/download/

That's the stable URL that will always point to our downloads.



-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] Action requested - Application Softblock implemented | Issue report ID341057

[Robert Haas]

On Wed, Oct 5, 2011 at 2:58 PM, Seiko Ishida (MP Tech Consulting LLC) <
v-sei...@microsoft.com> wrote:

>  Hello all,
>
> Thank you for all your responses to my inquiry.
> We are aware that this application version 8.2.x is pretty old and
> PostgreSQL will stop releasing updates for the 8.2.X in December 2011.
> *http://www.postgresql.org/docs/8.2/interactive/release-8-2-22.html*
>
> We also confirmed this installation issue was addressed in the latest
> version of the installer. (8.3.x and 8.4.x) However, we would like to warn
> endusers who might be affected by this issue trying to install 8.2.x
> versions on Windows8, and to direct them to install the latest version. You
> provided the following URL, but better yet is there a download site for the
> newer version we can direct endusers to?
>
> *http://wiki.postgresql.org/wiki/PostgreSQL_Release_Support_Policy*
>
>
> The dialog box presented to users will look something like this.
>
>
> Would this download site be a good URL for that?
> *http://www.enterprisedb.com/products-services-training/pgdownload*
>

That's not a PostgreSQL community web site, so it wouldn't be appropriate to
send people there, I think.  However, you could direct them to the
PostgreSQL download page:

http://www.postgresql.org/download/

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
<>

Re: [HACKERS] Multixact truncation for FK locks patch

[Alvaro Herrera]

Excerpts from Alvaro Herrera's message of lun sep 26 13:16:24 -0300 2011:

> So I had to look for something else -- and I think I have it: have
> multixact itself track its truncation position relative to Xid.  Each
> pg_multixact/offset segment would store ReadNewTransactionId at the
> time it is created.  Whenever vacuum attempts to run pg_clog
> truncation, it would also pass that Xid to multixact truncation; this
> would scan existing segments and delete those that come before the one
> marked with the maximum Xid previous to the pg_clog truncation point.

So here's how it would really work in detail.

The first MultiXactId in each segment, i.e. one pg_multixact/offset
entry every 32 pages, stores RecentGlobalXmin at the time the segment is
created, that is, when we get to ZeroOffsetPage the first page of the
segment.  Such positions are skipped when handing out MultiXactIds.  (We
abuse the system a bit by storing a TransactionId in a place that
normally holds an offset.  It's not like we don't do exactly this in
MultiXactId themselves.)  The semantics of this value is "segments prior
to this one can be removed as soon as we freeze Xids behind this one".
(However, note that the value is actually capped to RecentGlobalXmin.)

The reason that this is correct is that any MultiXactId that we might be
interested in knowing must be after the freeze point: any tuple before
the freeze point must have already been visited by vacuum; so either the
mxact contained only locks (in which case the interesting lifetime is
RecentGlobalXmin, which we already covered above), or it contained at
least one update; and since we're freezing, that update must be either
committed (so the tuple is invisible to everyone and would have been
removed by vacuum), or aborted (in which case the tuple would have been
relabeled HEAP_XMAX_INVALID).  /* FIXME what if the update is gone but
was locked by a later transaction? */

pg_control contains a new field, TransactionId mxactFrozenXid.  This
value is bootstrapped to InvalidTransactionId,

On CHECKPOINT, we call TruncateMultiXact(oldestXid).  (This is the most
recent value we know from VACUUM).

In TruncateMultiXact, if we see that pg_control.mxactFrozenXid is older
than oldestXid, we know we have segments to remove.  We scan the whole
directory to remove them, and then update mxactFrozenXid to the value
from the oldest remaining segment.  (Both things can be done in a single
scan).

I considered the idea that Xids might advance faster than we consume
multixact segments, making the offset's freezeXid wrapped around.  After
thinking about this, my conclusion is that there isn't really a problem
here (but I'm very open to be mistaken).

One thing of note is that the first page of the first segment is zeroed
twice: first when it is created by bootstrap, and second when the first
multixactid is created.  This is a bit annoying, so I'm going to have
bootstrap set mxact 2 as the first one to be created, not 1.  This means
we no longer zero that page twice; it also means we don't set a
freezeXid value uselessly for that page, which causes all sorts of
issues.  (After wraparound, mxact 1 would be assigned normally and
segment zero would behave just like any other segment).

In a directory scan to remove segments, we need to open the first page
of each segment to fetch its freezeXid.  Therefore it would be nice if
we could skip doing this if possible.  I think the way to do this is to
have the callback keep track of "earliest segment that we have to
keep" and "oldest segment that we can remove".  That way, any segment in
between needn't be opened.  In reality, I doubt this is going to save
much, because removing segments is not all that frequent anyway (unless
you're eating tons of MultiXactIds), so maybe we shouldn't implement
this bit.


Note that this is all about truncating pg_multixact/offset only.  To
truncate pg_multixact/members, we need to check the earliest kept
offsets segment, to know what's the earliest members segment we need to
keep.  This is the same we do now, IIRC.


Thoughts?  Does anybody see any serious flaw?

-- 
Álvaro Herrera 
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] [v9.2] DROP statement reworks

[Robert Haas]

On Wed, Oct 5, 2011 at 12:16 PM, Kohei KaiGai  wrote:
> * The logic of check_object_validation() got included within
>  get_relation_address(), and rewritten more smartly, as:
>
> +   relkind = RelationGetForm(relation)->relkind;
> +   if ((objtype == OBJECT_INDEX         && relkind != RELKIND_INDEX) ||
> +       (objtype == OBJECT_SEQUENCE      && relkind != RELKIND_SEQUENCE) ||
> +       (objtype == OBJECT_TABLE         && relkind != RELKIND_RELATION) ||
> +       (objtype == OBJECT_VIEW          && relkind != RELKIND_VIEW) ||
> +       (objtype == OBJECT_FOREIGN_TABLE && relkind != RELKIND_FOREIGN_TABLE))
> +       ereport(ERROR,
> +               (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> +                errmsg("\"%s\" is not a %s",
> +                       NameListToString(objname),
> +                       get_object_property_typetext(objtype;
> +

That's no good.  We've discussed it before.  It breaks translatability.

-- 
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] Query regarding postgres lock contention - Followup

[Robert Haas]

On Wed, Oct 5, 2011 at 1:48 PM, Kevin Grittner
 wrote:
> Hamza Bin Sohail  wrote:
>
>> My postgres version is 8.3.7
>
> Why such an old version?  Why exclude the available bug fixes?
>
> http://www.postgresql.org/support/versioning
>
>>> I am aware that lock contention can be checked with lockstat (and
>>> with pg_locks ? ) but I wanted to know if someone can tell me how
>>> much contention there would be for this database in a 16-core
>>> system vs a 4-core system. I just need a rough idea.
>
> How many database connections will be used?  If more than about
> twice the number of cores, you should probably be going through a
> transaction-based connection pool.
>
> With 16 cores, even with a properly configured connection pool, you
> will probably be on the edge of where spinlock contention starts
> eating significant CPU time.  With enough system RAM and proper
> tuning the hit should be fairly minor, I think.  It really gets bad
> at 32 cores, although that is being improved for next year's 9.2
> release.

I think that on write-heavy workloads (like pgbench) we bottleneck on
lightweight lock contention around 8 cores.  :-(

-- 
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] Query regarding postgres lock contention - Followup

[Kevin Grittner]

Hamza Bin Sohail  wrote:
 
> My postgres version is 8.3.7
 
Why such an old version?  Why exclude the available bug fixes?
 
http://www.postgresql.org/support/versioning
 
>> I am aware that lock contention can be checked with lockstat (and
>> with pg_locks ? ) but I wanted to know if someone can tell me how
>> much contention there would be for this database in a 16-core
>> system vs a 4-core system. I just need a rough idea.
 
How many database connections will be used?  If more than about
twice the number of cores, you should probably be going through a
transaction-based connection pool.
 
With 16 cores, even with a properly configured connection pool, you
will probably be on the edge of where spinlock contention starts
eating significant CPU time.  With enough system RAM and proper
tuning the hit should be fairly minor, I think.  It really gets bad
at 32 cores, although that is being improved for next year's 9.2
release.
 
-Kevin

-- 
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] Inlining comparators as a performance optimisation

[Bruce Momjian]

Robert Haas wrote:
> Markus Wanner took a crack at generalizing the autovacuum machinery
> that we have now into something that could be used to fire up
> general-purpose worker processes, but it fell down mostly because I
> (and, I think, others) weren't convinced that imessages were something
> we wanted to suck into core, and Markus reasonably enough wasn't
> interested in rewriting it to do something that wouldn't really help
> his work with Postgres-R.  I'm not sure where Bruce is getting his
> timeline from, but I think the limiting factor is not so much that we
> don't have people who can write the code as that those people are
> busy, and this is a big project.  But you can bet that if it gets to
> the top of Tom's priority list (just for example) we'll see some
> motion...!

I was thinking of setting up a team to map out some strategies and get
community buy-in, and then we could attack each issue.  I got the 2-3
years from the Win32 timeline.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

-- 
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] Bug with pg_ctl -w/wait and config-only directories

[Bruce Momjian]

Bruce Momjian wrote:
> Bruce Momjian wrote:
> > Robert Haas wrote:
> > > On Mon, Oct 3, 2011 at 11:04 PM, Bruce Momjian  wrote:
> > > > OK, here is a patch that adds a -C option to the postmaster so any
> > > > config variable can be dumped, even while the server is running (there
> > > > is no security check because we don't have a user name at this point),
> 
> OK, here is an updated patch, with documentation, that I consider ready
> for commit to git head.

And with a --help documentation patch.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +
commit c599ef7e91d4a28fe59b7ab4d84666bb01376570
Author: Bruce Momjian 
Date:   Wed Oct 5 11:18:26 2011 -0400

Add --help output.

diff --git a/src/backend/main/main.c b/src/backend/main/main.c
new file mode 100644
index 8d9cb94..b14c731
*** a/src/backend/main/main.c
--- b/src/backend/main/main.c
*** help(const char *progname)
*** 283,288 
--- 283,289 
  #endif
printf(_("  -B NBUFFERS number of shared buffers\n"));
printf(_("  -c NAME=VALUE   set run-time parameter\n"));
+   printf(_("  -C NAME return run-time parameter\n"));
printf(_("  -d 1-5  debugging level\n"));
printf(_("  -D DATADIR  database directory\n"));
printf(_("  -e  use European date input format (DMY)\n"));

-- 
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] Bug with pg_ctl -w/wait and config-only directories

[Bruce Momjian]

Bruce Momjian wrote:
> Robert Haas wrote:
> > On Mon, Oct 3, 2011 at 11:04 PM, Bruce Momjian  wrote:
> > > OK, here is a patch that adds a -C option to the postmaster so any
> > > config variable can be dumped, even while the server is running (there
> > > is no security check because we don't have a user name at this point),

OK, here is an updated patch, with documentation, that I consider ready
for commit to git head.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/doc/src/sgml/ref/postgres-ref.sgml b/doc/src/sgml/ref/postgres-ref.sgml
new file mode 100644
index b16bbdf..3807f40
*** a/doc/src/sgml/ref/postgres-ref.sgml
--- b/doc/src/sgml/ref/postgres-ref.sgml
*** PostgreSQL documentation
*** 141,146 
--- 141,160 
   
  
   
+   -C name
+   
+
+ Returns the value of a named run-time parameter, and exits.
+ (See the -c option above for details.)  This can
+ be used on a running server, and returns values from
+ postgresql.conf, modified by any parameters
+ supplied in this invocation.  It does not reflect parameters
+ supplied when the cluster was started.
+
+   
+  
+ 
+  
-d debug-level

 
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
new file mode 100644
index 0a84d97..dd7493c
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
*** bool		enable_bonjour = false;
*** 203,208 
--- 203,210 
  char	   *bonjour_name;
  bool		restart_after_crash = true;
  
+ char 		*output_config_variable = NULL;
+ 
  /* PIDs of special child processes; 0 when not running */
  static pid_t StartupPID = 0,
  			BgWriterPID = 0,
*** PostmasterMain(int argc, char *argv[])
*** 537,543 
  	 * tcop/postgres.c (the option sets should not conflict) and with the
  	 * common help() function in main/main.c.
  	 */
! 	while ((opt = getopt(argc, argv, "A:B:bc:D:d:EeFf:h:ijk:lN:nOo:Pp:r:S:sTt:W:-:")) != -1)
  	{
  		switch (opt)
  		{
--- 539,545 
  	 * tcop/postgres.c (the option sets should not conflict) and with the
  	 * common help() function in main/main.c.
  	 */
! 	while ((opt = getopt(argc, argv, "A:B:bc:C:D:d:EeFf:h:ijk:lN:nOo:Pp:r:S:sTt:W:-:")) != -1)
  	{
  		switch (opt)
  		{
*** PostmasterMain(int argc, char *argv[])
*** 554,559 
--- 556,565 
  IsBinaryUpgrade = true;
  break;
  
+ 			case 'C':
+ output_config_variable = optarg;
+ break;
+ 
  			case 'D':
  userDoption = optarg;
  break;
*** PostmasterMain(int argc, char *argv[])
*** 728,733 
--- 734,746 
  	if (!SelectConfigFiles(userDoption, progname))
  		ExitPostmaster(2);
  
+ 	if (output_config_variable != NULL)
+ 	{
+ 		/* permission is handled because the user is reading inside the data dir */
+ 		puts(GetConfigOption(output_config_variable, false, false));
+ 		ExitPostmaster(0);
+ 	}
+ 	
  	/* Verify that DataDir looks reasonable */
  	checkDataDir();
  
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
new file mode 100644
index c7eac71..19d94b2
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
*** process_postgres_switches(int argc, char
*** 3170,3176 
  	 * postmaster/postmaster.c (the option sets should not conflict) and with
  	 * the common help() function in main/main.c.
  	 */
! 	while ((flag = getopt(argc, argv, "A:B:bc:D:d:EeFf:h:ijk:lN:nOo:Pp:r:S:sTt:v:W:-:")) != -1)
  	{
  		switch (flag)
  		{
--- 3170,3176 
  	 * postmaster/postmaster.c (the option sets should not conflict) and with
  	 * the common help() function in main/main.c.
  	 */
! 	while ((flag = getopt(argc, argv, "A:B:bc:C:D:d:EeFf:h:ijk:lN:nOo:Pp:r:S:sTt:v:W:-:")) != -1)
  	{
  		switch (flag)
  		{
*** process_postgres_switches(int argc, char
*** 3187,3192 
--- 3187,3196 
  IsBinaryUpgrade = true;
  break;
  
+ 			case 'C':
+ /* ignored for consistency with the postmaster */
+ break;
+ 
  			case 'D':
  if (secure)
  	userDoption = strdup(optarg);
*** process_postgres_switches(int argc, char
*** 3272,3278 
  break;
  
  			case 'T':
! /* ignored for consistency with postmaster */
  break;
  
  			case 't':
--- 3276,3282 
  break;
  
  			case 'T':
! /* ignored for consistency with the postmaster */
  break;
  
  			case 't':
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
new file mode 100644
index 0dbdfe7..e633d0c
*** a/src/bin/pg_ctl/pg_ctl.c
--- b/src/bin/pg_ctl/pg_ctl.c
*** static ShutdownMode shutdown_mode = SMAR
*** 81,86 
--- 81,87 
  static int	sig = SIGTERM;		/* default */
  static CtlCommand ctl_command = NO_COMMAND;
  static char *pg_data = NULL;
+ static char *pg_

Re: [HACKERS] Bug with pg_ctl -w/wait and config-only directories

[Bruce Momjian]

Robert Haas wrote:
> On Tue, Oct 4, 2011 at 8:32 PM, Tom Lane  wrote:
> > I still think this is a matter for HEAD only. ?We haven't supported
> > these cases in back branches and so there is little argument for
> > back-patching.
> 
> According to Bruce's original post, there is at least one 9.1
> regression here relative to 9.0:
> 
> >> What is even worse is that pre-9.1, pg_ctl start would read ports from
> >> the pg_ctl -o command line, but in 9.1 we changed this to force reading
> >> the postmaster.pid file to find the port number and socket directory
> >> location --- meaning, new in PG 9.1, 'pg_ctl -w start' doesn't work for
> >> config-only directories either.

Yes, PG 9.1 pg_ctl -w does this:

$ pg_ctl -w -D tmp start
waiting for server to startLOG:  could not open usermap file

"/usr/var/local/pgdev/pgfoundry/pg_migrator/pg_migrator/tmp/pg_ident.conf":
No such file or directory
LOG:  database system was shut down at 2011-10-05 10:53:09 EDT
LOG:  database system is ready to accept connections
LOG:  autovacuum launcher started
 stopped waiting
pg_ctl: could not start server
Examine the log output.

This used to work.  Of course, there are many non-standard setting that
didn't work in pre-9.1.

I think the real question is whether this issue or allowing pg_upgrade
to work with and old pre-9.2 is worth the backpatching risk.  I am
unsure myself.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

-- 
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] Bug with pg_ctl -w/wait and config-only directories

[Tom Lane]

Peter Eisentraut  writes:
> On tis, 2011-10-04 at 17:49 -0400, Tom Lane wrote:
>> As of fairly recently, the Fedora package also uses pg_ctl for both
>> starting and stopping.  We've fixed all the reasons that formerly
>> existed to avoid use of pg_ctl, and it's a real PITA to try to
>> implement the waiting logic at shell level.

> Well, it's debatable whether an init script should actually do any
> waiting.  I'm not saying that what you are doing is wrong, but it
> depends on local policy and conventions.  I maintain some unrelated init
> scripts in Debian and have gotten occasional hell from users for holding
> up the boot process even a bit while waiting for the service to become
> fully operational.  A restart of a failing PostgreSQL server can take
> minutes; I don't want to think about how that would be received. :-/
> Considering how much work people are putting into speeding up the boot
> process in Linux distributions at the moment, with upstart, systemd
> etc., it's not clear to me that the waiting feature is a required
> behavior.

Well, actually, it wasn't until Fedora went to systemd that I could
sanely use "pg_ctl start -w".  In SysV initscripts, you're right,
waiting indefinitely for the DB server to come up is not tenable.
But in systemd, there is no serialization of services and it's better
if systemd is aware that the service isn't fully started yet.
In particular, with this implementation, somebody can put "After:
postgresql.service" in their unit file and be sure that the DB will be
ready when their service starts.

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] Bug with pg_ctl -w/wait and config-only directories

[Bruce Momjian]

Peter Eisentraut wrote:
> On m?n, 2011-10-03 at 15:09 -0400, Bruce Momjian wrote:
> > Why were people not using pg_ctl?
> 
> Actually, a slight correction/addition here: The Debian init script does
> use pg_ctl to start the service.  Seems to work fine.

Yes.  The script authors discovered a working behavior, which matches my
research:

pg_ctl startspecify config directory
pg_ctl -w start impossible
pg_ctl restart  impossible
pg_ctl stop specify real data dir
pg_ctl -w stop  specify real data dir
pg_ctl reload   specify real data dir

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

-- 
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] Bug with pg_ctl -w/wait and config-only directories

[Bruce Momjian]

Tom Lane wrote:
> Bruce Momjian  writes:
> > Tom Lane wrote:
> >> As of fairly recently, the Fedora package also uses pg_ctl for both
> >> starting and stopping.  We've fixed all the reasons that formerly
> >> existed to avoid use of pg_ctl, and it's a real PITA to try to
> >> implement the waiting logic at shell level.
> 
> > OK, that's a good use-case to warrant barrelling ahead with improving
> > pg_ctl for config-only installs.
> 
> Did I say that?  The Fedora packages don't support config-only
> arrangements (if you tried, SELinux would likely deny access to whatever
> directory you chose ...)
> 
> > What releases do we want to apply that
> > patch to allow postgres to dump config values and have pg_ctl use them? 
> > This patch is required for old/new installs for pg_upgrade to work.
> 
> I still think this is a matter for HEAD only.  We haven't supported
> these cases in back branches and so there is little argument for
> back-patching.

OK.  Just be aware that limits pg_upgrade to only upgrading old
config-only installs of PG 9.2+, e.g. old PG 9.2 to new PG 9.3.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

-- 
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] Error building v9.1.1 (git) with python 3.2.2

[Jan Urbański]

On 05/10/11 15:24, Lou Picciano wrote:
> Hackers, 
> 
> Have been following with some interest the dialog recently about getting 
> pl/python built within the v9 tree; most recently, I've been trying v9.1.1 
> (git) and python 3.2.2. Configure and make complete without error, but 'make 
> install' does not produce any plpython.so 
> 
> I have since added a symlink to /usr/lib/config-3.2mu, as suggested in that 
> dialog - No Joy. 
> 
> Can anyone point me in the right direction? 

Could you attach the configure and build logs? It's hard to diagnose
with so little info.

Cheers,
Jan

-- 
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] Re: [COMMITTERS] pgsql: Force strings passed to and from plperl to be in UTF8 encoding.

[Robert Haas]

On Wed, Oct 5, 2011 at 3:58 AM, Amit Khandekar
 wrote:
> I have no more issues with the patch.
> Thanks!

I think this patch needs to be added to the open CommitFest, with
links to the reviews, and marked Ready for Committer.

-- 
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] Action requested - Application Softblock implemented | Issue report ID341057

[Robert Haas]

On Wed, Oct 5, 2011 at 10:13 AM, Dave Page  wrote:
> On Wed, Oct 5, 2011 at 3:04 PM, Robert Haas  wrote:
>>
>> That seems strange, since the MSFT guy is reporting that whatever he
>> tested did work on Windows 7, but now that you've pointed out that
>> Windows 8 won't be released until after we stop supporting 8.2, I am
>> filled with uncaring.
>
> It might have appeared to successfully install, but that doesn't mean
> it's setup the way we want it. For example, none of the administrative
> menu shortcuts in that version will do privilege escalation which is
> more or less required in Windows 7+.

OK.  So... we don't care.  Got it.  :-)

-- 
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] Bug with pg_ctl -w/wait and config-only directories

[Robert Haas]

On Tue, Oct 4, 2011 at 8:32 PM, Tom Lane  wrote:
> I still think this is a matter for HEAD only.  We haven't supported
> these cases in back branches and so there is little argument for
> back-patching.

According to Bruce's original post, there is at least one 9.1
regression here relative to 9.0:

>> What is even worse is that pre-9.1, pg_ctl start would read ports from
>> the pg_ctl -o command line, but in 9.1 we changed this to force reading
>> the postmaster.pid file to find the port number and socket directory
>> location --- meaning, new in PG 9.1, 'pg_ctl -w start' doesn't work for
>> config-only directories either.

-- 
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] Action requested - Application Softblock implemented | Issue report ID341057

[Dave Page]

On Wed, Oct 5, 2011 at 3:04 PM, Robert Haas  wrote:
>
> That seems strange, since the MSFT guy is reporting that whatever he
> tested did work on Windows 7, but now that you've pointed out that
> Windows 8 won't be released until after we stop supporting 8.2, I am
> filled with uncaring.

It might have appeared to successfully install, but that doesn't mean
it's setup the way we want it. For example, none of the administrative
menu shortcuts in that version will do privilege escalation which is
more or less required in Windows 7+.

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: 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] [v9.2] DROP statement reworks

[Robert Haas]

On Wed, Oct 5, 2011 at 4:47 AM, Kohei KaiGai  wrote:
> The get_relation_address() follows the logic in RemoveRelations() to be
> eliminated by this patch, so it is not a code duplication.
> The reason why we didn't consolidate this routine with get_object_address()
> was that remove-index requires locks on the table which owns the index to
> be removed, and it was ugly to add an ad-hoc if-block on the routine.

Yeah, that's a problem that's been in the back of my mind for a bit
now, but I haven't come up with a good solution.  I don't think
RemoveRelations() is the only place we have this problem, either.

-- 
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] Action requested - Application Softblock implemented | Issue report ID341057

[Robert Haas]

On Wed, Oct 5, 2011 at 9:43 AM, Dave Page  wrote:
> On Wed, Oct 5, 2011 at 2:40 PM, Robert Haas  wrote:
>> On Wed, Oct 5, 2011 at 2:27 AM, Heikki Linnakangas
>>  wrote:
>>> On 04.10.2011 22:46, Seiko Ishida (MP Tech Consulting LLC) wrote:

 Our team drives the bug notification activity with our valued Windows
 partners. This email is to notify you that PostgreSQL's application/driver
 experienced compatibility issue(s) during internal Microsoft testing and 
 has
 been blocked. Please note that this block may already be in the latest
 Windows Developer Preview build so your prompt attention to the issue is
 much appreciated.

 ...
 Here are the details of the Softblock implementations:
 Compatibility Issue:
 Product name:  PostgreSQL 8.2
>>>
>>> Version 8.2 is quite old, and the community support for it will end in
>>> December. I don't think anyone cares if it works on Windows 8. If you could
>>> test with PostgreSQL 9.1, that would be great.
>>
>> On the other hand, it seems as though they've identified the offending
>> code, so maybe it wouldn't be that much work to fix:
>
> This is in the old 8.2 MSI installer, which doesn't even support
> Windows 7 (and would require more work than just this to do so). The
> last ever release of 8.2 will happen long before Windows 8 is going to
> go GA - it's safe to say this version is never going to be supported
> on Windows 8.

That seems strange, since the MSFT guy is reporting that whatever he
tested did work on Windows 7, but now that you've pointed out that
Windows 8 won't be released until after we stop supporting 8.2, I am
filled with uncaring.

Perhaps if the MSFT guys want a link to direct people to, we could
suggest http://wiki.postgresql.org/wiki/PostgreSQL_Release_Support_Policy

-- 
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] Inlining comparators as a performance optimisation

[Robert Haas]

On Tue, Oct 4, 2011 at 10:55 PM, Greg Stark  wrote:
> I agree that if we wanted to farm out entire plan nodes we would
> probably end up generating new partial plans which would be handed to
> other backend processes. That's not unlike what Oracle did with
> Parallel Query. But i'm a bit skeptical that we'll get much of that
> done in 2-3 years. The main use case for Parallel Query in Oracle is
> for partitioned tables -- and we haven't really polished that after
> how many years?

Partitioning hasn't been completely neglected; 9.1 adds support for
something called Merge Append.  You may have heard of (or, err,
authored) that particular bit of functionality.  :-)

Of course, it would be nice to have a better syntax, but I don't think
the lack of it should discourage us from working on parallel query,
which is a muti-part problem.  You need to:

- have planner support to decide when to parallelize
- have a mechanism for firing up worker processes and synchronizing
the relevant bits of state between the master and the workers
- have an IPC mechanism for streaming data between the master process
and the workers
- figure out how to structure the executor so that the workers neither
stall nor run too far ahead of the master (which might have LIMIT 10
or something)

Markus Wanner took a crack at generalizing the autovacuum machinery
that we have now into something that could be used to fire up
general-purpose worker processes, but it fell down mostly because I
(and, I think, others) weren't convinced that imessages were something
we wanted to suck into core, and Markus reasonably enough wasn't
interested in rewriting it to do something that wouldn't really help
his work with Postgres-R.  I'm not sure where Bruce is getting his
timeline from, but I think the limiting factor is not so much that we
don't have people who can write the code as that those people are
busy, and this is a big project.  But you can bet that if it gets to
the top of Tom's priority list (just for example) we'll see some
motion...!

-- 
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] Action requested - Application Softblock implemented | Issue report ID341057

[Dave Page]

On Wed, Oct 5, 2011 at 2:40 PM, Robert Haas  wrote:
> On Wed, Oct 5, 2011 at 2:27 AM, Heikki Linnakangas
>  wrote:
>> On 04.10.2011 22:46, Seiko Ishida (MP Tech Consulting LLC) wrote:
>>>
>>> Our team drives the bug notification activity with our valued Windows
>>> partners. This email is to notify you that PostgreSQL's application/driver
>>> experienced compatibility issue(s) during internal Microsoft testing and has
>>> been blocked. Please note that this block may already be in the latest
>>> Windows Developer Preview build so your prompt attention to the issue is
>>> much appreciated.
>>>
>>> ...
>>> Here are the details of the Softblock implementations:
>>> Compatibility Issue:
>>> Product name:  PostgreSQL 8.2
>>
>> Version 8.2 is quite old, and the community support for it will end in
>> December. I don't think anyone cares if it works on Windows 8. If you could
>> test with PostgreSQL 9.1, that would be great.
>
> On the other hand, it seems as though they've identified the offending
> code, so maybe it wouldn't be that much work to fix:

This is in the old 8.2 MSI installer, which doesn't even support
Windows 7 (and would require more work than just this to do so). The
last ever release of 8.2 will happen long before Windows 8 is going to
go GA - it's safe to say this version is never going to be supported
on Windows 8.

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: 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] Action requested - Application Softblock implemented | Issue report ID341057

[Robert Haas]

On Wed, Oct 5, 2011 at 2:27 AM, Heikki Linnakangas
 wrote:
> On 04.10.2011 22:46, Seiko Ishida (MP Tech Consulting LLC) wrote:
>>
>> Our team drives the bug notification activity with our valued Windows
>> partners. This email is to notify you that PostgreSQL's application/driver
>> experienced compatibility issue(s) during internal Microsoft testing and has
>> been blocked. Please note that this block may already be in the latest
>> Windows Developer Preview build so your prompt attention to the issue is
>> much appreciated.
>>
>> ...
>> Here are the details of the Softblock implementations:
>> Compatibility Issue:
>> Product name:  PostgreSQL 8.2
>
> Version 8.2 is quite old, and the community support for it will end in
> December. I don't think anyone cares if it works on Windows 8. If you could
> test with PostgreSQL 9.1, that would be great.

On the other hand, it seems as though they've identified the offending
code, so maybe it wouldn't be that much work to fix:

MSFT> In the RunInitdb function, you can see that char *datadir is
never initialized but it is used erroneously to check for null. This
bug was fixed in the 8.3.x and 8.4.x branch. On Windows7, 8.2.x works
even though datadir is pointing to garbage, the garbage just happens
to be non-null. On Win8, somehow [ebp-14h]/datadir is NULL. Either
way, this is what's breaking the installation.

I assume this must be the installer code, though, not PG itself, since
I don't see anything called RunInitdb in our sources.

-- 
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] Unremovable tuple monitoring (was: Addition of some trivial auto vacuum logging)

[Robert Haas]

On Tue, Oct 4, 2011 at 6:45 PM, Royce Ausburn  wrote:
> I'm not sure what my next step should be.  I've added this patch to the open 
> commit fest -- is that all for now until the commit fest begins review?

Yep, except that it might be nice if you could volunteer to review
someone else's patch in turn.  :-)

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

[HACKERS] Error building v9.1.1 (git) with python 3.2.2

[Lou Picciano]

Hackers, 

Have been following with some interest the dialog recently about getting 
pl/python built within the v9 tree; most recently, I've been trying v9.1.1 
(git) and python 3.2.2. Configure and make complete without error, but 'make 
install' does not produce any plpython.so 

I have since added a symlink to /usr/lib/config-3.2mu, as suggested in that 
dialog - No Joy. 

Can anyone point me in the right direction? 

Lou Picciano 

[HACKERS] Bug in walsender when calling out to do_pg_stop_backup (and others?)

[Magnus Hagander]

When walsender calls out to do_pg_stop_backup() (during base backups),
it is not possible to terminate the process with a SIGTERM - it
requires a SIGKILL. This can leave unkillable backends for example if
archive_mode is on and archive_command is failing (or not set). A
similar thing would happen in other cases if walsender calls out to
something that would block (do_pg_start_backup() for example), but the
stop one is easy to provoke.

ISTM one way to fix it is the attached, which is to have walsender set
the "global" flags saying that we have received sigterm, which in turn
causes the CHECK_FOR_INTERRUPTS() calls in the routines to properly
exit the process. AFAICT it works fine. Any holes in this approach?

Second, I wonder if we should add a SIGINT handler as well, that would
make it possible to send a cancel signal. Given that the end result
would be the same (at least if we want to keep with the "walsender is
simple" path), I'm not sure it's necessary - but it would at least
help those doing pg_cancel_backend()... thoughts?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 5d1c518..c8fd165 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1302,6 +1302,13 @@ WalSndShutdownHandler(SIGNAL_ARGS)
 	if (MyWalSnd)
 		SetLatch(&MyWalSnd->latch);
 
+	/*
+	 * Set the standard (non-walsender) state as well, so that we can
+	 * abort things like do_pg_stop_backup().
+	 */
+	InterruptPending = true;
+	ProcDiePending = true;
+
 	errno = save_errno;
 }
 

-- 
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] Double sorting split patch

[Alexander Korotkov]

Path without allocating extra bytes is attached.
I run some more detailed tests on geonames and two smaller datasets from
rtreeportal.org.
Test tables are following:
1) test1 - geonames
2) test2 - California Roads, containing the MBRs of 2,249,727 streets
3) test3 - Tiger Streams, containing the MBRs of 194,971 streams
Scripts for test queries generation and execution are attached
(scripts.tar.gz).

Build times are given in the following table:
  New linear Double sorting
test1 277.889630 273.766355
test2  73.262561  75.114079
test3   4.251563   4.425139
As we can see, build times differ insignificantly.

Index sizes are given in the following table:
  New linear Double sorting
test1 572383232  578977792
test2 179929088  178569216
test3  15409152   15532032
As we can see, index sizes differ insignificantly.

Data about index quality testing are in the table test_results of the
following structure:
tablename - name of the queried table
count - actual count of rows matching query
nominal_count - count of rows matching query which test generator tried to
provide (test generator not always can create test query which return
exactly same amount of rows as it was expected)
strategy - split strategy: 1 - new liner split(current), 2 - double
sorting(this patch)
hits - number of pages hits for query execution.

Raw data are in the attachment (test_result.sql.gz). Report is below.
test=# select tablename, nominal_count, round(avg(count),2) as avg_count,
strategy, round(avg(hits),2) as avg_hits from test_results group by
tablename, nominal_count, strategy order by tablename, nominal_count,
strategy;
 tablename | nominal_count | avg_count | strategy | avg_hits
---+---+---+--+--
 test1 | 1 |  4.87 |1 |19.94
 test1 | 1 |  4.87 |2 | 6.46
 test1 |10 | 11.07 |1 |23.95
 test1 |10 | 11.07 |2 | 7.36
 test1 |   100 |101.36 |1 |43.30
 test1 |   100 |101.36 |2 |10.19
 test1 |  1000 |998.70 |1 |86.48
 test1 |  1000 |998.70 |2 |24.21
 test2 | 1 |  1.32 |1 | 8.67
 test2 | 1 |  1.32 |2 | 5.99
 test2 |10 | 11.32 |1 | 9.40
 test2 |10 | 11.32 |2 | 6.71
 test2 |   100 |102.93 |1 |13.10
 test2 |   100 |102.93 |2 | 9.02
 test2 |  1000 |999.67 |1 |32.16
 test2 |  1000 |999.67 |2 |23.51
 test3 | 1 |  0.99 |1 | 6.03
 test3 | 1 |  0.99 |2 | 4.32
 test3 |10 |  9.95 |1 | 7.52
 test3 |10 |  9.95 |2 | 5.09
 test3 |   100 | 99.92 |1 |10.73
 test3 |   100 | 99.92 |2 | 7.73
 test3 |  1000 |999.75 |1 |27.47
 test3 |  1000 |999.75 |2 |22.44
(24 rows)

As we can see, double sorting split outperfoms new linear split in all the
presented cases. Difference in test2 and test3 is not so big as it is in
test1. It can be explained by fact that test2 and test3 are smaller than
test1.

--
With best regards,
Alexander Korotkov.


scripts.tar.gz
Description: GNU Zip compressed data
*** a/src/backend/access/gist/gistproc.c
--- b/src/backend/access/gist/gistproc.c
***
*** 27,32  static double size_box(Datum dbox);
--- 27,35 
  static bool rtree_internal_consistent(BOX *key, BOX *query,
  		  StrategyNumber strategy);
  
+ /* Minimal possible ratio of split */
+ #define LIMIT_RATIO 0.3
+ 
  
  /**
   * Box ops
***
*** 49,78  rt_box_union(PG_FUNCTION_ARGS)
  	PG_RETURN_BOX_P(n);
  }
  
- static Datum
- rt_box_inter(PG_FUNCTION_ARGS)
- {
- 	BOX		   *a = PG_GETARG_BOX_P(0);
- 	BOX		   *b = PG_GETARG_BOX_P(1);
- 	BOX		   *n;
- 
- 	n = (BOX *) palloc(sizeof(BOX));
- 
- 	n->high.x = Min(a->high.x, b->high.x);
- 	n->high.y = Min(a->high.y, b->high.y);
- 	n->low.x = Max(a->low.x, b->low.x);
- 	n->low.y = Max(a->low.y, b->low.y);
- 
- 	if (n->high.x < n->low.x || n->high.y < n->low.y)
- 	{
- 		pfree(n);
- 		/* Indicate "no intersection" by returning NULL pointer */
- 		n = NULL;
- 	}
- 
- 	PG_RETURN_BOX_P(n);
- }
- 
  /*
   * The GiST Consistent method for boxes
   *
--- 52,57 
***
*** 194,279  gist_box_penalty(PG_FUNCTION_ARGS)
  	PG_RETURN_POINTER(result);
  }
  
- static void
- chooseLR(GIST_SPLITVEC *v,
- 		 OffsetNumber *list1, int nlist1, BOX *union1,
- 		 OffsetNumber *list2, int nlist2, BOX *union2)
- {
- 	bool		firstToLeft = true;
- 
- 	if (v->spl_ldatum_exists || v->spl_rdatum_exists)
- 	{
- 		if (

Re: [HACKERS] Displaying accumulated autovacuum cost

[Alvaro Herrera]

Excerpts from Greg Smith's message of mié oct 05 04:02:12 -0300 2011:
> 
> On 09/29/2011 10:40 AM, Alvaro Herrera wrote:
> > I reviewed this patch.  My question for you is: does it make sense to
> > enable to reporting of write rate even when vacuum cost accounting is
> > enabled?  In my opinion it would be useful to do so.  If you agree,
> > please submit an updated patch.
> 
> Presumably you meant to ask if this makes sense to show when cost 
> accounting isn't enabled, because the code doesn't do that right now.  
> No cost accounting, no buffer usage/write rate data as this was submitted.

Yes, sorry, that's what I meant.

> Looks like making this work even in cases where cost accounting isn't on 
> will make the patch a bit larger obtrusive, but it's not unreasonable.  
> Now that you mention it, people who do a manual, full-speed VACUUM would 
> certainly appreciate some feedback on the rate it ran at.  I'll include 
> that in the next update.

Thanks.

-- 
Álvaro Herrera 
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] Query regarding postgres lock contention - Followup

[Hamza Bin Sohail]

In addition to the previous post,

My postgres version is 8.3.7

>Hi there,
>
>Just to let you know, I'm not a database expert by any means. 
>I have configured dbt-2  with postgres and created a database with 4000 
>warehouses, 
>150 customers etc. The database size is over 8G. I am aware that lock 
>contention
>can be checked with lockstat (and with pg_locks ? ) but I wanted to know if 
>someone can tell me how much contention there would be for this database
>in a 16-core system vs a 4-core system. I just need a rough idea.
>
>Any response would be very helpful
>
>Thanks
>
>~Hamza

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

[HACKERS] Query regarding postgres lock contention

[Hamza Bin Sohail]

Hi there,

Just to let you know, I'm not a database expert by any means. 
I have configured dbt-2  with postgres and created a database with 4000 
warehouses, 
150 customers etc. The database size is over 8G. I am aware that lock contention
can be checked with lockstat (and with pg_locks ? ) but I wanted to know if 
someone can tell me how much contention there would be for this database
in a 16-core system vs a 4-core system. I just need a rough idea.

Any response would be very helpful

Thanks

~Hamza

-- 
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] [v9.2] DROP statement reworks

[Kohei KaiGai]

Thanks for your reviewing.

2011/10/4 Robert Haas :
> On Wed, Sep 28, 2011 at 11:51 AM, Kohei KaiGai  wrote:
>> I rebased the patches towards the latest git master, so I believe these
>> are available to apply.
>
> Reviewing away...
>
> I don't see why we need one struct called ObjectProperty and another
> called CatalogProperty.  Just fold CatalogProperty into ObjectProperty
> and make it one big flat structure.
>
The reason of this separation is some of ObjectType shares same catalogs;
such as pg_class for OBJECT_(TABLE|VIEW|INDEX|SEQUENCE),
so I thought it is better to reference CatalogProperty by several ObjectProperty
entries to avoid accidental inconsistence.
However, it is just a matter of my preference. I'll modify it.
The big flat structure provides a simple structure on the other hand.

> The get_object_property_waddawadda functions appear to be designed
> under the assumption that each object type will be in the
> ObjectProperty structure at the offset corresponding to (int) objtype.
>  Are these functions going to get called from any place that's
> performance-critical enough to justify that optimization?  I'd rather
> just have these functions use linear search, so that if someone adds a
> new OBJECT_* constant and doesn't add it to the array it doesn't
> immediately break everything.
>
OK, I'll fix it.
I assume these functions are used in DDL operations; mostly not
a performance critical path.

> get_object_namespace() looks like it ought to live in objectaddress.c,
> not dropcmds.c.
OK, I'll move it.

> check_object_validation() doesn't seem like a very
> good description of what that code actually does -- and that code
> looks like it's copy-and-pasted from elsewhere.  Can we avoid that?
>
I noticed that get_object_address() already applied same checks on
pg_type object, and pg_class objects also.
Due to the below reason, it does not invoke get_object_address() for
the pg_class objects, so it seems to me reasonable to move whole
of the logic in check_object_validation() to get_relation_address().

> get_relation_address() is surely a copy of some other bit of code; how
> can we avoid duplication?
>
The get_relation_address() follows the logic in RemoveRelations() to be
eliminated by this patch, so it is not a code duplication.
The reason why we didn't consolidate this routine with get_object_address()
was that remove-index requires locks on the table which owns the index to
be removed, and it was ugly to add an ad-hoc if-block on the routine.

I'll try to work on this. Please wait for a while...

Thanks,
-- 
KaiGai Kohei 

-- 
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] Bug with pg_ctl -w/wait and config-only directories

[Peter Eisentraut]

On mån, 2011-10-03 at 15:09 -0400, Bruce Momjian wrote:
> Why were people not using pg_ctl?

Actually, a slight correction/addition here: The Debian init script does
use pg_ctl to start the service.  Seems to work fine.


-- 
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] Bug with pg_ctl -w/wait and config-only directories

[Peter Eisentraut]

On tis, 2011-10-04 at 17:49 -0400, Tom Lane wrote:
> Bruce Momjian  writes:
> > Greg Stark wrote:
> >> An interactive tool can dwim automatically but that isn't appropriate
> >> for a startup script. A startupt script should always do the same
> >> thing exactly and do that based on the OS policy, not based on
> >> inspecting what programs are actually running on the machine.
> 
> > I agree, except the Gentoo script does exactly that --- wait for
> > completion using pg_ctl -w.
> 
> As of fairly recently, the Fedora package also uses pg_ctl for both
> starting and stopping.  We've fixed all the reasons that formerly
> existed to avoid use of pg_ctl, and it's a real PITA to try to
> implement the waiting logic at shell level.

Well, it's debatable whether an init script should actually do any
waiting.  I'm not saying that what you are doing is wrong, but it
depends on local policy and conventions.  I maintain some unrelated init
scripts in Debian and have gotten occasional hell from users for holding
up the boot process even a bit while waiting for the service to become
fully operational.  A restart of a failing PostgreSQL server can take
minutes; I don't want to think about how that would be received. :-/
Considering how much work people are putting into speeding up the boot
process in Linux distributions at the moment, with upstart, systemd
etc., it's not clear to me that the waiting feature is a required
behavior.



-- 
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] Double sorting split patch

[Alexander Korotkov]

On Wed, Oct 5, 2011 at 11:37 AM, Heikki Linnakangas <
heikki.linnakan...@enterprisedb.com> wrote:

> On 04.10.2011 15:10, Alexander Korotkov wrote:
>
>> On Tue, Oct 4, 2011 at 1:46 PM, Heikki Linnakangas<
>> heikki.linnakangas@**enterprisedb.com>
>>  wrote:
>>
>>  Ok. Could you phrase that as a code comment?
>>>
>>> Here's a version of the patch I've been working on. There's no functional
>>> changes, just a lot of moving things around, comment changes, etc. to
>>> hopefully make it more readable.
>>>
>>
>> Thanks for your work on this patch. Patch with comment is attached.
>>
>
> Thanks, I incorporated that, and did a lot of other comment changes. I
> included the example you gave earlier on how the first phase of the
> algorithm works, in a comment. Please review, and if you have some test
> cases at hand, run them. I think this is ready for commit now.
>
Comments looks good, thanks. I'm going to try also some datasets from
rtreeportal.org 


> One more thing:
>
>>/* Allocate vectors for results */
>>nbytes = (maxoff + 2) * sizeof(OffsetNumber);
>>v->spl_left = (OffsetNumber *) palloc(nbytes);
>>v->spl_right = (OffsetNumber *) palloc(nbytes);
>>
>
> Why "maxoff + 2" ? Allocating a few extra bytes is obviously harmless, but
> I wonder if it was just a leftover from something.

It was nested from old code. This extra bytes are useless in modern versions
of PostgreSQL as we found while seg picksplit patch discussion. Modern
version of seg picksplit doesn't contain them:
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=2a6ebe70fb2f7ec97a08dc07214fe2ca571d2780;hp=290f1603b4208ca6a13776f744b586a958e98d74

--
With best regards,
Alexander Korotkov.

Re: [HACKERS] Re: [COMMITTERS] pgsql: Force strings passed to and from plperl to be in UTF8 encoding.

[Amit Khandekar]

On 5 October 2011 12:29, Alex Hunsaker  wrote:
> On Wed, Oct 5, 2011 at 00:30, Alex Hunsaker  wrote:
>> On Tue, Oct 4, 2011 at 23:46, Amit Khandekar
>>  wrote:
>
>>> You mean the final changes in plperl_helpers.h would look like
>>> something like this right? :
>>>
>>>  static inline char *
>>>  utf_u2e(const char *utf8_str, size_t len)
>>>  {
>>>        char       *ret = (char *) pg_do_encoding_conversion((unsigned
>>> char *) utf8_str, len, PG_UTF8, GetDatabaseEncoding());
>>>
>>>        if (ret == utf8_str)
>>> +       {
>>> +               if (GetDatabaseEncoding() == PG_UTF8 ||
>>> +                       GetDatabaseEncoding() == PG_SQL_ASCII)
>>> +               {
>>> +                       pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
>>> +               }
>>> +
>>>                ret = pstrdup(ret);
>>> +       }
>>>        return ret;
>>>  }
>>
>
>>> Yeah I am ok with that. It's just an additional check besides (ret ==
>>> utf8_str) to know if we really require validation.
>
> Find it attached. [ Note I didn't put the check inside the if (ret ==
> utf8_str) as it seemed a bit cleaner (indentation wise) to have it
> outside ]

I have no more issues with the patch.
Thanks!
-Amit

>

-- 
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] Double sorting split patch

[Heikki Linnakangas]

On 04.10.2011 15:10, Alexander Korotkov wrote:

On Tue, Oct 4, 2011 at 1:46 PM, Heikki Linnakangas<
heikki.linnakan...@enterprisedb.com>  wrote:


Ok. Could you phrase that as a code comment?

Here's a version of the patch I've been working on. There's no functional
changes, just a lot of moving things around, comment changes, etc. to
hopefully make it more readable.


Thanks for your work on this patch. Patch with comment is attached.


Thanks, I incorporated that, and did a lot of other comment changes. I 
included the example you gave earlier on how the first phase of the 
algorithm works, in a comment. Please review, and if you have some test 
cases at hand, run them. I think this is ready for commit now.


One more thing:

/* Allocate vectors for results */
nbytes = (maxoff + 2) * sizeof(OffsetNumber);
v->spl_left = (OffsetNumber *) palloc(nbytes);
v->spl_right = (OffsetNumber *) palloc(nbytes);


Why "maxoff + 2" ? Allocating a few extra bytes is obviously harmless, 
but I wonder if it was just a leftover from something.


--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com
*** a/src/backend/access/gist/gistproc.c
--- b/src/backend/access/gist/gistproc.c
***
*** 27,32  static double size_box(Datum dbox);
--- 27,35 
  static bool rtree_internal_consistent(BOX *key, BOX *query,
  		  StrategyNumber strategy);
  
+ /* Minimal possible ratio of split */
+ #define LIMIT_RATIO 0.3
+ 
  
  /**
   * Box ops
***
*** 49,78  rt_box_union(PG_FUNCTION_ARGS)
  	PG_RETURN_BOX_P(n);
  }
  
- static Datum
- rt_box_inter(PG_FUNCTION_ARGS)
- {
- 	BOX		   *a = PG_GETARG_BOX_P(0);
- 	BOX		   *b = PG_GETARG_BOX_P(1);
- 	BOX		   *n;
- 
- 	n = (BOX *) palloc(sizeof(BOX));
- 
- 	n->high.x = Min(a->high.x, b->high.x);
- 	n->high.y = Min(a->high.y, b->high.y);
- 	n->low.x = Max(a->low.x, b->low.x);
- 	n->low.y = Max(a->low.y, b->low.y);
- 
- 	if (n->high.x < n->low.x || n->high.y < n->low.y)
- 	{
- 		pfree(n);
- 		/* Indicate "no intersection" by returning NULL pointer */
- 		n = NULL;
- 	}
- 
- 	PG_RETURN_BOX_P(n);
- }
- 
  /*
   * The GiST Consistent method for boxes
   *
--- 52,57 
***
*** 194,279  gist_box_penalty(PG_FUNCTION_ARGS)
  	PG_RETURN_POINTER(result);
  }
  
- static void
- chooseLR(GIST_SPLITVEC *v,
- 		 OffsetNumber *list1, int nlist1, BOX *union1,
- 		 OffsetNumber *list2, int nlist2, BOX *union2)
- {
- 	bool		firstToLeft = true;
- 
- 	if (v->spl_ldatum_exists || v->spl_rdatum_exists)
- 	{
- 		if (v->spl_ldatum_exists && v->spl_rdatum_exists)
- 		{
- 			BOX			LRl = *union1,
- 		LRr = *union2;
- 			BOX			RLl = *union2,
- 		RLr = *union1;
- 			double		sizeLR,
- 		sizeRL;
- 
- 			adjustBox(&LRl, DatumGetBoxP(v->spl_ldatum));
- 			adjustBox(&LRr, DatumGetBoxP(v->spl_rdatum));
- 			adjustBox(&RLl, DatumGetBoxP(v->spl_ldatum));
- 			adjustBox(&RLr, DatumGetBoxP(v->spl_rdatum));
- 
- 			sizeLR = size_box(DirectFunctionCall2(rt_box_inter, BoxPGetDatum(&LRl), BoxPGetDatum(&LRr)));
- 			sizeRL = size_box(DirectFunctionCall2(rt_box_inter, BoxPGetDatum(&RLl), BoxPGetDatum(&RLr)));
- 
- 			if (sizeLR > sizeRL)
- firstToLeft = false;
- 
- 		}
- 		else
- 		{
- 			float		p1,
- 		p2;
- 			GISTENTRY	oldUnion,
- 		addon;
- 
- 			gistentryinit(oldUnion, (v->spl_ldatum_exists) ? v->spl_ldatum : v->spl_rdatum,
- 		  NULL, NULL, InvalidOffsetNumber, FALSE);
- 
- 			gistentryinit(addon, BoxPGetDatum(union1), NULL, NULL, InvalidOffsetNumber, FALSE);
- 			DirectFunctionCall3(gist_box_penalty, PointerGetDatum(&oldUnion), PointerGetDatum(&addon), PointerGetDatum(&p1));
- 			gistentryinit(addon, BoxPGetDatum(union2), NULL, NULL, InvalidOffsetNumber, FALSE);
- 			DirectFunctionCall3(gist_box_penalty, PointerGetDatum(&oldUnion), PointerGetDatum(&addon), PointerGetDatum(&p2));
- 
- 			if ((v->spl_ldatum_exists && p1 > p2) || (v->spl_rdatum_exists && p1 < p2))
- firstToLeft = false;
- 		}
- 	}
- 
- 	if (firstToLeft)
- 	{
- 		v->spl_left = list1;
- 		v->spl_right = list2;
- 		v->spl_nleft = nlist1;
- 		v->spl_nright = nlist2;
- 		if (v->spl_ldatum_exists)
- 			adjustBox(union1, DatumGetBoxP(v->spl_ldatum));
- 		v->spl_ldatum = BoxPGetDatum(union1);
- 		if (v->spl_rdatum_exists)
- 			adjustBox(union2, DatumGetBoxP(v->spl_rdatum));
- 		v->spl_rdatum = BoxPGetDatum(union2);
- 	}
- 	else
- 	{
- 		v->spl_left = list2;
- 		v->spl_right = list1;
- 		v->spl_nleft = nlist2;
- 		v->spl_nright = nlist1;
- 		if (v->spl_ldatum_exists)
- 			adjustBox(union2, DatumGetBoxP(v->spl_ldatum));
- 		v->spl_ldatum = BoxPGetDatum(union2);
- 		if (v->spl_rdatum_exists)
- 			adjustBox(union1, DatumGetBoxP(v->spl_rdatum));
- 		v->spl_rdatum = BoxPGetDatum(union1);
- 	}
- 
- 	v->spl_ldatum_exists = v->spl_rdatum_exists = false;
- }
- 
  /*
   * Trivial split: half of entries will be placed on one page
   * and another half - to another
--- 173,178 
***
*** 338,536 

Re: [HACKERS] Displaying accumulated autovacuum cost

[Greg Smith]

On 09/29/2011 10:40 AM, Alvaro Herrera wrote:

I reviewed this patch.  My question for you is: does it make sense to
enable to reporting of write rate even when vacuum cost accounting is
enabled?  In my opinion it would be useful to do so.  If you agree,
please submit an updated patch.
   


Presumably you meant to ask if this makes sense to show when cost 
accounting isn't enabled, because the code doesn't do that right now.  
No cost accounting, no buffer usage/write rate data as this was submitted.


Looks like making this work even in cases where cost accounting isn't on 
will make the patch a bit larger obtrusive, but it's not unreasonable.  
Now that you mention it, people who do a manual, full-speed VACUUM would 
certainly appreciate some feedback on the rate it ran at.  I'll include 
that in the next update.


--
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] Separating bgwriter and checkpointer

[Simon Riggs]

On Wed, Oct 5, 2011 at 5:10 AM, Dickson S. Guedes  wrote:

> Ah ok! I started reviewing the v4 patch version, this is my comments:

...

> Well, all the tests was running with the default postgresql.conf in my
> laptop but I'll setup a more "real world" environment to test for
> performance regression. Until now I couldn't notice any significant
> difference in TPS before and after patch in a small environment. I'll
> post something soon.

Great testing, thanks. Likely will have no effect in non-I/O swamped
environment, but no regression expected either.

-- 
 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] Re: [COMMITTERS] pgsql: Force strings passed to and from plperl to be in UTF8 encoding.

[Alex Hunsaker]

On Wed, Oct 5, 2011 at 00:30, Alex Hunsaker  wrote:
> On Tue, Oct 4, 2011 at 23:46, Amit Khandekar
>  wrote:

>> You mean the final changes in plperl_helpers.h would look like
>> something like this right? :
>>
>>  static inline char *
>>  utf_u2e(const char *utf8_str, size_t len)
>>  {
>>        char       *ret = (char *) pg_do_encoding_conversion((unsigned
>> char *) utf8_str, len, PG_UTF8, GetDatabaseEncoding());
>>
>>        if (ret == utf8_str)
>> +       {
>> +               if (GetDatabaseEncoding() == PG_UTF8 ||
>> +                       GetDatabaseEncoding() == PG_SQL_ASCII)
>> +               {
>> +                       pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
>> +               }
>> +
>>                ret = pstrdup(ret);
>> +       }
>>        return ret;
>>  }
>

>> Yeah I am ok with that. It's just an additional check besides (ret ==
>> utf8_str) to know if we really require validation.

Find it attached. [ Note I didn't put the check inside the if (ret ==
utf8_str) as it seemed a bit cleaner (indentation wise) to have it
outside ]
*** a/src/pl/plperl/GNUmakefile
--- b/src/pl/plperl/GNUmakefile
***
*** 57,63  PSQLDIR = $(bindir)
  
  include $(top_srcdir)/src/Makefile.shlib
  
! plperl.o: perlchunks.h plperl_opmask.h
  
  plperl_opmask.h: plperl_opmask.pl
  	@if [ x"$(perl_privlibexp)" = x"" ]; then echo "configure switch --with-perl was not specified."; exit 1; fi
--- 57,63 
  
  include $(top_srcdir)/src/Makefile.shlib
  
! plperl.o: perlchunks.h plperl_opmask.h plperl_helpers.h
  
  plperl_opmask.h: plperl_opmask.pl
  	@if [ x"$(perl_privlibexp)" = x"" ]; then echo "configure switch --with-perl was not specified."; exit 1; fi
*** a/src/pl/plperl/expected/plperl.out
--- b/src/pl/plperl/expected/plperl.out
***
*** 639,641  CONTEXT:  PL/Perl anonymous code block
--- 639,651 
  DO $do$ use warnings FATAL => qw(void) ; my @y; my $x = sort @y; 1; $do$ LANGUAGE plperl;
  ERROR:  Useless use of sort in scalar context at line 1.
  CONTEXT:  PL/Perl anonymous code block
+ --
+ -- Make sure strings are validated -- This code may fail in a non-UTF8 database
+ -- if it allows null bytes in strings.
+ --
+ CREATE OR REPLACE FUNCTION perl_zerob() RETURNS TEXT AS $$
+   return "abcd\0efg";
+ $$ LANGUAGE plperlu;
+ SELECT perl_zerob();
+ ERROR:  invalid byte sequence for encoding "UTF8": 0x00
+ CONTEXT:  PL/Perl function "perl_zerob"
*** a/src/pl/plperl/plperl_helpers.h
--- b/src/pl/plperl/plperl_helpers.h
***
*** 7,16 
  static inline char *
  utf_u2e(const char *utf8_str, size_t len)
  {
! 	char	   *ret = (char *) pg_do_encoding_conversion((unsigned char *) utf8_str, len, PG_UTF8, GetDatabaseEncoding());
  
  	if (ret == utf8_str)
  		ret = pstrdup(ret);
  	return ret;
  }
  
--- 7,27 
  static inline char *
  utf_u2e(const char *utf8_str, size_t len)
  {
! 	int 	enc = GetDatabaseEncoding();
! 
! 	char	   *ret = (char *) pg_do_encoding_conversion((unsigned char *) utf8_str, len, PG_UTF8, enc);
! 
! 	/*
! 	* when we are a PG_UTF8 or SQL_ASCII database
! 	* pg_do_encoding_conversion() will not do any conversion or
! 	* verification. we need to do it manually instead.
! 	*/
! 	if (enc == PG_UTF8 || enc == PG_SQL_ASCII)
! 		pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
  
  	if (ret == utf8_str)
  		ret = pstrdup(ret);
+ 
  	return ret;
  }
  
*** a/src/pl/plperl/sql/plperl.sql
--- b/src/pl/plperl/sql/plperl.sql
***
*** 415,417  DO $do$ use strict; my $name = "foo"; my $ref = $$name; $do$ LANGUAGE plperl;
--- 415,426 
  -- check that we can "use warnings" (in this case to turn a warn into an error)
  -- yields "ERROR:  Useless use of sort in scalar context."
  DO $do$ use warnings FATAL => qw(void) ; my @y; my $x = sort @y; 1; $do$ LANGUAGE plperl;
+ 
+ --
+ -- Make sure strings are validated -- This code may fail in a non-UTF8 database
+ -- if it allows null bytes in strings.
+ --
+ CREATE OR REPLACE FUNCTION perl_zerob() RETURNS TEXT AS $$
+   return "abcd\0efg";
+ $$ LANGUAGE plperlu;
+ SELECT perl_zerob();

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