Re: [HACKERS] Fix for pg_upgrade and invalid indexes

2013-03-29 Thread Bruce Momjian
On Fri, Mar 29, 2013 at 07:03:05PM -0400, Tom Lane wrote:
> Andres Freund  writes:
> > Those columns cannot be NULL, so using IS DISTINCT FROM seems a bit
> > clumsy.
> 
> That was what I started to write, too, but actually I think the IS
> DISTINCT is correct and the RIGHT JOIN should be a LEFT JOIN.  Note
> that the query appears to be intended to collect regular tables as
> well as indexes.  (As patched, that's totally broken, so I infer
> Bruce hasn't tested it yet.)

Yes, I only ran my simple tests so far --- I wanted to at least get some
eyes on it.  I was wondering if we ever need to use parentheses for
queries that mix normal and outer joins?  I am unclear on that.

Attached is a fixed patch that uses LEFT JOIN.  I went back and looked
at the patch that added this test and I think the patch is now complete.
I would like to apply it tomorrow/Saturday so it will be ready for
Monday's packaging, and get some buildfarm time on it.

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

  + It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
new file mode 100644
index 65fb548..35783d0
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*** static void check_is_super_user(ClusterI
*** 20,26 
  static void check_for_prepared_transactions(ClusterInfo *cluster);
  static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
  static void check_for_reg_data_type_usage(ClusterInfo *cluster);
- static void check_for_invalid_indexes(ClusterInfo *cluster);
  static void get_bin_version(ClusterInfo *cluster);
  static char *get_canonical_locale_name(int category, const char *locale);
  
--- 20,25 
*** check_and_dump_old_cluster(bool live_che
*** 97,103 
  	check_is_super_user(&old_cluster);
  	check_for_prepared_transactions(&old_cluster);
  	check_for_reg_data_type_usage(&old_cluster);
- 	check_for_invalid_indexes(&old_cluster);
  	check_for_isn_and_int8_passing_mismatch(&old_cluster);
  
  	/* old = PG 8.3 checks? */
--- 96,101 
*** check_for_reg_data_type_usage(ClusterInf
*** 952,1046 
  			   "%s\n\n", output_path);
  	}
  	else
- 		check_ok();
- }
- 
- 
- /*
-  * check_for_invalid_indexes()
-  *
-  *	CREATE INDEX CONCURRENTLY can create invalid indexes if the index build
-  *	fails.  These are dumped as valid indexes by pg_dump, but the
-  *	underlying files are still invalid indexes.  This checks to make sure
-  *	no invalid indexes exist, either failed index builds or concurrent
-  *	indexes in the process of being created.
-  */
- static void
- check_for_invalid_indexes(ClusterInfo *cluster)
- {
- 	int			dbnum;
- 	FILE	   *script = NULL;
- 	bool		found = false;
- 	char		output_path[MAXPGPATH];
- 
- 	prep_status("Checking for invalid indexes from concurrent index builds");
- 
- 	snprintf(output_path, sizeof(output_path), "invalid_indexes.txt");
- 
- 	for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
- 	{
- 		PGresult   *res;
- 		bool		db_used = false;
- 		int			ntups;
- 		int			rowno;
- 		int			i_nspname,
- 	i_relname;
- 		DbInfo	   *active_db = &cluster->dbarr.dbs[dbnum];
- 		PGconn	   *conn = connectToServer(cluster, active_db->db_name);
- 
- 		res = executeQueryOrDie(conn,
- "SELECT n.nspname, c.relname "
- "FROM	pg_catalog.pg_class c, "
- "		pg_catalog.pg_namespace n, "
- "		pg_catalog.pg_index i "
- "WHERE	(i.indisvalid = false OR "
- "		 i.indisready = false) AND "
- "		i.indexrelid = c.oid AND "
- "		c.relnamespace = n.oid AND "
- /* we do not migrate these, so skip them */
- 			" 		n.nspname != 'pg_catalog' AND "
- "		n.nspname != 'information_schema' AND "
- /* indexes do not have toast tables */
- "		n.nspname != 'pg_toast'");
- 
- 		ntups = PQntuples(res);
- 		i_nspname = PQfnumber(res, "nspname");
- 		i_relname = PQfnumber(res, "relname");
- 		for (rowno = 0; rowno < ntups; rowno++)
- 		{
- 			found = true;
- 			if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
- pg_log(PG_FATAL, "Could not open file \"%s\": %s\n",
- 	   output_path, getErrorText(errno));
- 			if (!db_used)
- 			{
- fprintf(script, "Database: %s\n", active_db->db_name);
- db_used = true;
- 			}
- 			fprintf(script, "  %s.%s\n",
- 	PQgetvalue(res, rowno, i_nspname),
- 	PQgetvalue(res, rowno, i_relname));
- 		}
- 
- 		PQclear(res);
- 
- 		PQfinish(conn);
- 	}
- 
- 	if (script)
- 		fclose(script);
- 
- 	if (found)
- 	{
- 		pg_log(PG_REPORT, "fatal\n");
- 		pg_log(PG_FATAL,
- 			   "Your installation contains invalid indexes due to failed or\n"
- 		 	   "currently running CREATE INDEX CONCURRENTLY operations.  You\n"
- 			   "cannot upgrade until these indexes are valid or removed.  A\n"
- 			   "list of the problem indexes is in the file:\n"
- 			   "%s\n\n", output_

Re: [HACKERS] Getting to 9.3 beta

2013-03-29 Thread Amit Kapila
On Friday, March 29, 2013 11:04 PM Andres Freund wrote:
> On 2013-03-29 12:28:59 -0400, Tom Lane wrote:
> > Andres Freund  writes:
> > > On 2013-03-29 10:15:42 -0400, Bruce Momjian wrote:
> > >> What is a reasonable timeframe to target for completion of these
> items?
> >
> > > Here's my take on it:
> >
> > Thanks for annotating these!  I've commented on the ones where I
> > come to a different conclusion:
> >
> > > - replace plugins directory with GUC
> > >   Peter has agreed to boot it to the next fest afaics
> > >   (515357f4.6000...@gmx.net)
> > >   => move (done)
> >
> > It looks to me more like a RWF situation, since Peter has evidently
> lost
> > interest in committing this feature at all.  So I marked it that way.
> 
> Yea, after a second reading of Peter's email I agree.
> 
> > > - Patch to compute Max LSN of Data Pages:
> > >   I *personally* don't really see a point in including it in
> postgres,
> > >   there doesn't really seem to be any real demand met by the tool
> > >   => reject?
> >
> > Not sure.  It certainly hasn't drawn that much enthusiasm on the
> list,
> > but it strikes me as the sort of thing that when you need it you
> really
> > need it.  It would be interesting to hear opinions about it from
> people
> > who deal with data recovery situations on a regular basis.
> 
> I only have done so infrequently, but I guess my real problem is that I
> can't see it being all that helpful in such a situation. If you've lost
> pg_xlog - otherwise there doesn't seem to be much use in this - you can
> just use
> pg_resetxlog to set an insanely high lsn and then dump & restore the
> database. 

This utility can give user value that is okay to proceed.

> The only reason to use something more complex than this is if
> you want to continue using the cluster which seems like a really bad
> idea.

I agree that mostly user doesn't need to use cluster further, 
but incase if he is aware that only some part of the database (particular
tablespace) has problem
and he can take dump of that part of database and redo some of lost
operations on other tablespaces.


With Regards,
Amit Kapila.



-- 
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] Hash Join cost estimates

2013-03-29 Thread Jeff Davis
On Fri, 2013-03-29 at 16:37 -0400, Tom Lane wrote: 
> Jeff Davis  writes:
> > Yes, I have run into this issue (or something very similar). I don't
> > understand why the bucketsize even matters much -- assuming few hash
> > collisions, we are not actually evaluating the quals any more times than
> > necessary. So why all of the hashjoin-specific logic in determining the
> > number of qual evaluations? The only reason I can think of is to model
> > the cost of comparing the hashes themselves.
> 
> I think the point is that there may *not* be few hash collisions ...

In Stephen's case the table was only 41KB, so something still seems off.
Maybe we should model the likelihood of a collision based on the
cardinalities (assuming a reasonably good hash function)?

Also, I think I found an important assumption that seems dubious (in
comment for estimate_hash_bucketsize()):

"If the other relation in the join has a key distribution similar to
this one's, then the most-loaded buckets are exactly those that will be
probed most often.  Therefore, the "average" bucket size for costing
purposes should really be taken as something close to the "worst case"
bucket size.  We try to estimate this by adjusting the fraction if there
are too few distinct data values, and then scaling up by the ratio of
the most common value's frequency to the average frequency."

But the key distribution is not necessarily similar at all... the large
table might have many more distinct values.

Stephen, do you think this could explain your problem?

Regards,
Jeff Davis





-- 
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] [COMMITTERS] pgsql: Add parallel pg_dump option.

2013-03-29 Thread Andrew Dunstan


On 03/29/2013 03:12 PM, David Fetter wrote:

On Sun, Mar 24, 2013 at 03:39:44PM +, Andrew Dunstan wrote:

Add parallel pg_dump option.

This is great!

While testing, I noticed that the only supported -F option when -j is
specified is directory, which is fine as far as it goes, but I think
it would be easier on users if there were some default like ./pg_dump
when -j is specified.  It could of course be overridden by -Fd.

What say?




What you really want is to supply a default for --file in this case. It 
would seem very odd to do so when --jobs is specified but not when it 
isn't. You already need to specify a --file value when 
--format=directory is in use, and the addition of --jobs has not changed 
that; neither has the fact that --jobs requires --format=directory to be 
specified. Given that, this can be seen as a feature request that could 
be considered for 9.4 (although I'd be skeptical, TBH), but it surely 
isn't something that's broken and needs to be fixed, and we're long past 
the point where we should be adding new design for 9.3.


cheers

andrew



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


Re: [HACKERS] [COMMITTERS] pgsql: Add sql_drop event for event triggers

2013-03-29 Thread Tom Lane
Alvaro Herrera  writes:
> Add sql_drop event for event triggers

The buildfarm members that use -DCLOBBER_CACHE_ALWAYS don't like this
patch:

***
*** 760,771 
FROM generate_series(1, 50) a;
  BEGIN;
  SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
  UPDATE serializable_update_tab SET description = 'no no', id = 1 WHERE id = 1;
  COMMIT;
  SELECT description FROM serializable_update_tab WHERE id = 1;
! description 
! 
!  updated in trigger
  (1 row)
  
  DROP TABLE serializable_update_tab;
--- 760,773 
FROM generate_series(1, 50) a;
  BEGIN;
  SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
+ ERROR:  SET TRANSACTION ISOLATION LEVEL must be called before any query
  UPDATE serializable_update_tab SET description = 'no no', id = 1 WHERE id = 1;
+ ERROR:  current transaction is aborted, commands ignored until end of 
transaction block
  COMMIT;
  SELECT description FROM serializable_update_tab WHERE id = 1;
!  description 
! -
!  new
  (1 row)
  
  DROP TABLE serializable_update_tab;

I suspect you have inserted a snapshot-capturing operation into
someplace it mustn't go during transaction startup, but only in a path
that is triggered by an immediately preceding cache flush.

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] Getting to 9.3 beta

2013-03-29 Thread Daniel Farina
On Fri, Mar 29, 2013 at 8:22 AM, Andres Freund  wrote:
> - pg_stat_statements: query, session, and eviction identification:
>   Seems to need at least docs
>   => wait for author, seems to  be easy enough?

I would have responded by now, but recent events have unfortunately
made me put a lot of things that take longer than fifteen minutes on
hold.  Still, I am watching.  Like you say, it's not terribly
invasive.  The under-estimation error propagation is perhaps a bit too
weird to justify (even though it's a simple implementation).

--
fdr


-- 
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] Fix for pg_upgrade and invalid indexes

2013-03-29 Thread Tom Lane
Andres Freund  writes:
> Those columns cannot be NULL, so using IS DISTINCT FROM seems a bit
> clumsy.

That was what I started to write, too, but actually I think the IS
DISTINCT is correct and the RIGHT JOIN should be a LEFT JOIN.  Note
that the query appears to be intended to collect regular tables as
well as indexes.  (As patched, that's totally broken, so I infer
Bruce hasn't tested it yet.)

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] Fix for pg_upgrade and invalid indexes

2013-03-29 Thread Andres Freund
On 2013-03-29 16:57:06 -0400, Bruce Momjian wrote:
> On Thu, Mar 28, 2013 at 05:27:28PM -0400, Tom Lane wrote:
> > Bruce Momjian  writes:
> > > Should I just patch pg_upgrade to remove the "indisvalid", skip
> > > "indisvalid" indexes, and backpatch it?  Users should be using the
> > > version of pg_upgrade to match new pg_dump.  Is there any case where
> > > they don't match?  Do I still need to check for "indisready"?
> > 
> > Yeah, if you can just ignore !indisvalid indexes that should work fine.
> > I see no need to look at indisready if you're doing that.
> 
> Attached is a patch that implements the suggested pg_upgrade changes of
> not copying invalid indexes now that pg_dump doesn't dump them.  This
> should be backpatched back to 8.4 to match pg_dump.  It might require
> release note updates;  not sure.  Previously pg_upgrade threw an error
> if invalid indexes exist, but only since February, when we released the
> pg_upgrade fix to do this.  You can see the majority of this patch is
> removing that check.

> +  "RIGHT OUTER JOIN pg_catalog.pg_index i "
> +  " ON (c.oid = i.indexrelid) "
>"WHERE relkind IN ('r', 'm', 'i'%s) AND "
> + /* pg_dump only dumps valid indexes;  testing 
> indisready is
> +  * necessary in 9.2, and harmless in earlier/later 
> versions. */
> +  " i.indisvalid IS DISTINCT FROM false AND "
> +  " i.indisready IS DISTINCT FROM false AND "
>   /* exclude possible orphaned temp tables */
>"  ((n.nspname !~ '^pg_temp_' AND "
>"n.nspname !~ '^pg_toast_temp_' AND "

Those columns cannot be NULL, so using IS DISTINCT FROM seems a bit
clumsy.

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] Fix for pg_upgrade and invalid indexes

2013-03-29 Thread Tom Lane
Bruce Momjian  writes:
> Attached is a patch that implements the suggested pg_upgrade changes of
> not copying invalid indexes now that pg_dump doesn't dump them.  This
> should be backpatched back to 8.4 to match pg_dump.  It might require
> release note updates;  not sure.  Previously pg_upgrade threw an error
> if invalid indexes exist, but only since February, when we released the
> pg_upgrade fix to do this.  You can see the majority of this patch is
> removing that check.

Surely that should be LEFT JOIN not RIGHT JOIN?

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] Getting to 9.3 beta

2013-03-29 Thread Michael Paquier

On 2013/03/30, at 2:33, Andres Freund  wrote:

> On 2013-03-29 12:28:59 -0400, Tom Lane 
> 
>>> - REINDEX CONCURRENTLY:
>>>  Imo pretty close to being comittable and pretty useful, but it got
>>>  redesigned pretty late and it mostly had review from me and fujii and
>>>  it could use a bit more input
>>>  => unclear
>> 
>> I think this really has no hope of being bulletproof until we have
>> MVCC-based catalog scans.  The work up to now has been good exploratory
>> effort, but it's not going to be committable without that
>> infrastructure, IMHO anyway.
> 
> I think its fair not to commit it in 9.3, the patch came late and has
> undergone significant issues since then. And there are some things in it
> I am not sure about yet.
> But I am not sure why its only acceptable with MVCC scans? Sure, it
> takes an exclusive lock for a very short time when switching the
> relations, but thats still a huge step up over the status quo:
> * toast indexes currently cannot be created/dropped at all. So you have
>  to reindex the table which will need to be locked over the whole time.
> * you currently cannot replace indexes that are referenced by foreign
>  keys without doing manual catalog hackery which is definitely unsafe
> * you cannot create constraints that use an index concurrently. Parts of
>  that is emulatable by creating the constraint with an existing index,
>  but that doesn't help e.g. for exclusion constraints.
> * there's no convenient way to issue CREATE INDEX CONCURRENLTY new_index; DROP
>  INDEX CONCURRENTLY old_index; RENAME new_index old_index; for all indexes.
Thanks for the detailed explanation! Just adding that this patch needs more 
review and attention especially for the toast part where multiple indexes are 
handled.

As it looks that it is too late for 9.3, well let's move it to the next commit 
fest and work on that in the next release cycle.

Thanks,
--
Michael



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


[HACKERS] Fix for pg_upgrade and invalid indexes

2013-03-29 Thread Bruce Momjian
On Thu, Mar 28, 2013 at 05:27:28PM -0400, Tom Lane wrote:
> Bruce Momjian  writes:
> > Should I just patch pg_upgrade to remove the "indisvalid", skip
> > "indisvalid" indexes, and backpatch it?  Users should be using the
> > version of pg_upgrade to match new pg_dump.  Is there any case where
> > they don't match?  Do I still need to check for "indisready"?
> 
> Yeah, if you can just ignore !indisvalid indexes that should work fine.
> I see no need to look at indisready if you're doing that.

Attached is a patch that implements the suggested pg_upgrade changes of
not copying invalid indexes now that pg_dump doesn't dump them.  This
should be backpatched back to 8.4 to match pg_dump.  It might require
release note updates;  not sure.  Previously pg_upgrade threw an error
if invalid indexes exist, but only since February, when we released the
pg_upgrade fix to do this.  You can see the majority of this patch is
removing that check.

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

  + It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
new file mode 100644
index 65fb548..35783d0
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*** static void check_is_super_user(ClusterI
*** 20,26 
  static void check_for_prepared_transactions(ClusterInfo *cluster);
  static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
  static void check_for_reg_data_type_usage(ClusterInfo *cluster);
- static void check_for_invalid_indexes(ClusterInfo *cluster);
  static void get_bin_version(ClusterInfo *cluster);
  static char *get_canonical_locale_name(int category, const char *locale);
  
--- 20,25 
*** check_and_dump_old_cluster(bool live_che
*** 97,103 
  	check_is_super_user(&old_cluster);
  	check_for_prepared_transactions(&old_cluster);
  	check_for_reg_data_type_usage(&old_cluster);
- 	check_for_invalid_indexes(&old_cluster);
  	check_for_isn_and_int8_passing_mismatch(&old_cluster);
  
  	/* old = PG 8.3 checks? */
--- 96,101 
*** check_for_reg_data_type_usage(ClusterInf
*** 952,1046 
  			   "%s\n\n", output_path);
  	}
  	else
- 		check_ok();
- }
- 
- 
- /*
-  * check_for_invalid_indexes()
-  *
-  *	CREATE INDEX CONCURRENTLY can create invalid indexes if the index build
-  *	fails.  These are dumped as valid indexes by pg_dump, but the
-  *	underlying files are still invalid indexes.  This checks to make sure
-  *	no invalid indexes exist, either failed index builds or concurrent
-  *	indexes in the process of being created.
-  */
- static void
- check_for_invalid_indexes(ClusterInfo *cluster)
- {
- 	int			dbnum;
- 	FILE	   *script = NULL;
- 	bool		found = false;
- 	char		output_path[MAXPGPATH];
- 
- 	prep_status("Checking for invalid indexes from concurrent index builds");
- 
- 	snprintf(output_path, sizeof(output_path), "invalid_indexes.txt");
- 
- 	for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
- 	{
- 		PGresult   *res;
- 		bool		db_used = false;
- 		int			ntups;
- 		int			rowno;
- 		int			i_nspname,
- 	i_relname;
- 		DbInfo	   *active_db = &cluster->dbarr.dbs[dbnum];
- 		PGconn	   *conn = connectToServer(cluster, active_db->db_name);
- 
- 		res = executeQueryOrDie(conn,
- "SELECT n.nspname, c.relname "
- "FROM	pg_catalog.pg_class c, "
- "		pg_catalog.pg_namespace n, "
- "		pg_catalog.pg_index i "
- "WHERE	(i.indisvalid = false OR "
- "		 i.indisready = false) AND "
- "		i.indexrelid = c.oid AND "
- "		c.relnamespace = n.oid AND "
- /* we do not migrate these, so skip them */
- 			" 		n.nspname != 'pg_catalog' AND "
- "		n.nspname != 'information_schema' AND "
- /* indexes do not have toast tables */
- "		n.nspname != 'pg_toast'");
- 
- 		ntups = PQntuples(res);
- 		i_nspname = PQfnumber(res, "nspname");
- 		i_relname = PQfnumber(res, "relname");
- 		for (rowno = 0; rowno < ntups; rowno++)
- 		{
- 			found = true;
- 			if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
- pg_log(PG_FATAL, "Could not open file \"%s\": %s\n",
- 	   output_path, getErrorText(errno));
- 			if (!db_used)
- 			{
- fprintf(script, "Database: %s\n", active_db->db_name);
- db_used = true;
- 			}
- 			fprintf(script, "  %s.%s\n",
- 	PQgetvalue(res, rowno, i_nspname),
- 	PQgetvalue(res, rowno, i_relname));
- 		}
- 
- 		PQclear(res);
- 
- 		PQfinish(conn);
- 	}
- 
- 	if (script)
- 		fclose(script);
- 
- 	if (found)
- 	{
- 		pg_log(PG_REPORT, "fatal\n");
- 		pg_log(PG_FATAL,
- 			   "Your installation contains invalid indexes due to failed or\n"
- 		 	   "currently running CREATE INDEX CONCURRENTLY operations.  You\n"
- 			   "cannot upgrade until these indexes are valid or removed.  A\n"
- 			   "list of the problem indexes is in the file:\n"
- 			   "%s\n\n", output_path

Re: [HACKERS] Hash Join cost estimates

2013-03-29 Thread Tom Lane
Jeff Davis  writes:
> Yes, I have run into this issue (or something very similar). I don't
> understand why the bucketsize even matters much -- assuming few hash
> collisions, we are not actually evaluating the quals any more times than
> necessary. So why all of the hashjoin-specific logic in determining the
> number of qual evaluations? The only reason I can think of is to model
> the cost of comparing the hashes themselves.

I think the point is that there may *not* be few hash collisions ...

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] Hash Join cost estimates

2013-03-29 Thread Jeff Davis
On Thu, 2013-03-28 at 19:56 -0400, Stephen Frost wrote:
>   41K hashed, seqscan 4M: 115030.10 + 1229.46 = 116259.56
>   4M hashed, seqscan 41K: 1229.46 + 211156.20 = 212385.66

I think those are backwards -- typo?

>   In the end, I think the problem here is that we are charging far too
>   much for these bucket costs (particularly when we're getting them so
>   far wrong) and not nearly enough for the cost of building the hash
>   table in the first place.
> 
>   Thoughts?  Ideas about how we can 'fix' this?  Have others run into
>   similar issues?

Yes, I have run into this issue (or something very similar). I don't
understand why the bucketsize even matters much -- assuming few hash
collisions, we are not actually evaluating the quals any more times than
necessary. So why all of the hashjoin-specific logic in determining the
number of qual evaluations? The only reason I can think of is to model
the cost of comparing the hashes themselves.

Also, searching the archives turns up at least one other, but I think
I've seen more:

http://www.postgresql.org/message-id/a82128a6-4e3b-43bd-858d-21b129f7b...@richrelevance.com

Regards,
Jeff Davis



-- 
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] Getting to 9.3 beta

2013-03-29 Thread Pavel Stehule
Hello


> - plpgsql_check_function:
>   Tom says (27661.1364267...@sss.pgh.pa.us) that even if the approach
>   can be aggreed uppon it needs quite a bit more work
>   => move
>
>
Can we talk about this patch little bit more before moving to next
commitfest?

I would to have some plan to next commitfest. We have to remove some wanted
functionality - possibility to identify more issues in one run, remove
warnings, ... or we have significantly refactor plpgsql parser (two stages)

Next possibility - introduce some new API and move plpgsql_check_function
to external module, although I am thinking so this is important and very
missing functionality (and should be in core) still.

There is no reply, comments to my last update - where I rewrote output,
what was mayor Tom's objection (well specified).

Regards

Pavel


Re: [HACKERS] [COMMITTERS] pgsql: Add parallel pg_dump option.

2013-03-29 Thread David Fetter
On Sun, Mar 24, 2013 at 03:39:44PM +, Andrew Dunstan wrote:
> Add parallel pg_dump option.

This is great!

While testing, I noticed that the only supported -F option when -j is
specified is directory, which is fine as far as it goes, but I think
it would be easier on users if there were some default like ./pg_dump
when -j is specified.  It could of course be overridden by -Fd.

What say?

Cheers,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
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] Enabling Checksums

2013-03-29 Thread Jim Nasby

On 3/25/13 8:25 AM, Bruce Momjian wrote:

On Fri, Mar 22, 2013 at 11:35:35PM -0500, Jim Nasby wrote:

>On 3/20/13 8:41 AM, Bruce Momjian wrote:

> >Also, if a users uses checksums in 9.3, could they initdb without
> >checksums in 9.4 and use pg_upgrade?  As coded, the pg_controldata
> >checksum settings would not match and pg_upgrade would throw an
> >error, but it might be possible to allow this, i.e. you could go from
> >checksum to no checksum initdb clusters, but not from no checksum to
> >checksum.  I am wondering if the patch should reflect this.

>
>If the docs don't warn about this, they should, but I don't think it's
>the responsibility of this patch to deal with that problem. The reason
>I don't believe this patch should deal with it is because that is a
>known, rather serious, limitation of pg_upgrade. It's something about
>pg_upgrade that just needs to be fixed, regardless of what patches
>might make the situation worse.

Huh?  It wasn't a "serious limitation" of pg_upgrade until this patch.
What limitation does pg_upgrade have regardless of this patch?


The limitation that it depends on binary compatibility.

I suppose it's unfair to say that's a pg_upgrade limitation, but it's a 
certainly a limitation of Postgres upgrade capability. So far we've been able 
to skirt the issue but at some point we need to address this.


--
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] Getting to 9.3 beta

2013-03-29 Thread Andres Freund
On 2013-03-29 12:28:59 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2013-03-29 10:15:42 -0400, Bruce Momjian wrote:
> >> What is a reasonable timeframe to target for completion of these items?
>
> > Here's my take on it:
>
> Thanks for annotating these!  I've commented on the ones where I
> come to a different conclusion:
>
> > - replace plugins directory with GUC
> >   Peter has agreed to boot it to the next fest afaics
> >   (515357f4.6000...@gmx.net)
> >   => move (done)
>
> It looks to me more like a RWF situation, since Peter has evidently lost
> interest in committing this feature at all.  So I marked it that way.

Yea, after a second reading of Peter's email I agree.

> > - Patch to compute Max LSN of Data Pages:
> >   I *personally* don't really see a point in including it in postgres,
> >   there doesn't really seem to be any real demand met by the tool
> >   => reject?
>
> Not sure.  It certainly hasn't drawn that much enthusiasm on the list,
> but it strikes me as the sort of thing that when you need it you really
> need it.  It would be interesting to hear opinions about it from people
> who deal with data recovery situations on a regular basis.

I only have done so infrequently, but I guess my real problem is that I
can't see it being all that helpful in such a situation. If you've lost
pg_xlog - otherwise there doesn't seem to be much use in this - you can just use
pg_resetxlog to set an insanely high lsn and then dump & restore the
database. The only reason to use something more complex than this is if
you want to continue using the cluster which seems like a really bad
idea.
Am I missing a usecase here?

I guess it can be useful as a template utility to start hacking on for
more complex scenarios, but I don't think that warrants /contrib.

> > - REINDEX CONCURRENTLY:
> >   Imo pretty close to being comittable and pretty useful, but it got
> >   redesigned pretty late and it mostly had review from me and fujii and
> >   it could use a bit more input
> >   => unclear
>
> I think this really has no hope of being bulletproof until we have
> MVCC-based catalog scans.  The work up to now has been good exploratory
> effort, but it's not going to be committable without that
> infrastructure, IMHO anyway.

I think its fair not to commit it in 9.3, the patch came late and has
undergone significant issues since then. And there are some things in it
I am not sure about yet.
But I am not sure why its only acceptable with MVCC scans? Sure, it
takes an exclusive lock for a very short time when switching the
relations, but thats still a huge step up over the status quo:
* toast indexes currently cannot be created/dropped at all. So you have
  to reindex the table which will need to be locked over the whole time.
* you currently cannot replace indexes that are referenced by foreign
  keys without doing manual catalog hackery which is definitely unsafe
* you cannot create constraints that use an index concurrently. Parts of
  that is emulatable by creating the constraint with an existing index,
  but that doesn't help e.g. for exclusion constraints.
* there's no convenient way to issue CREATE INDEX CONCURRENLTY new_index; DROP
  INDEX CONCURRENTLY old_index; RENAME new_index old_index; for all indexes.

The last alone imnsho is already a good enough usecase for the patch.

Also, the part that deals with the (locked) switch of the relfilenodes
is a really small part of the patch, improving that later is not going
to throw away much.

Imo having this would have made
e.g. beb850e1d873f8920a78b9b9ee27e9f87c95592f way much less painful. We
(as in 2ndq) had to write scripts for customers to deal with this in a
remotely sane manner. And those were *ugly*.

> > - psql watch
> >   Waiting on author since 4 days
> >   => wait or boot?
>
> If there is agreement on the basic design of the feature, ie
> "\watch [n]" as a repeating version of "\g", it seems to me that
> this could be fixed and committed in a day or less.  I'd be willing
> to look at it sometime next week.

Cool.

> > - transforms:
> >   I am not sure what solution is proposed for the dependency issue
> >   between shared objects.
> >   There also hasn't been code level review for a while...
> >   => unclear
>
> Needs to be moved; no way this is ready in the next week or two.

Good. I haven't looked at it in a long time and nobody else did, so I
just wasn't sure.

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] Getting to 9.3 beta

2013-03-29 Thread Bruce Momjian
On Fri, Mar 29, 2013 at 11:05:46AM -0400, Tom Lane wrote:
> Bruce Momjian  writes:
> > Our final 9.3 commit-fest has has exceeded the two-month mark, so it is
> > time to start targeting a date to close it and get to 9.3 beta.  I see
> > 25 items will needing attention before we can close it:
> 
> > https://commitfest.postgresql.org/action/commitfest_view?id=17
> 
> > What is a reasonable timeframe to target for completion of these items?
> 
> TBH, once Andrew commits the JSON patch, I wouldn't have a problem with
> moving all the rest to Returned With Feedback or the next CF.  None of
> the others seem to me to be close-to-committable (with the possible
> exception of the Max LSN patch, which I've not looked at), and April is
> not the time to be doing development.
> 
> Next week is going to be tied up with the back-branch releases, but
> maybe we could target beta for the week after?  The main gating factor
> at this point really would be how quickly we could write some draft
> release notes, so people know what to test.

I have been clearing my calendar with plans to do the 9.3 release notes,
so I am ready to start.

-- 
  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] Getting to 9.3 beta

2013-03-29 Thread Tom Lane
Andres Freund  writes:
> On 2013-03-29 10:15:42 -0400, Bruce Momjian wrote:
>> What is a reasonable timeframe to target for completion of these items?

> Here's my take on it:

Thanks for annotating these!  I've commented on the ones where I
come to a different conclusion:

> - replace plugins directory with GUC
>   Peter has agreed to boot it to the next fest afaics
>   (515357f4.6000...@gmx.net)
>   => move (done)

It looks to me more like a RWF situation, since Peter has evidently lost
interest in committing this feature at all.  So I marked it that way.

> - Patch to compute Max LSN of Data Pages:
>   I *personally* don't really see a point in including it in postgres,
>   there doesn't really seem to be any real demand met by the tool
>   => reject?

Not sure.  It certainly hasn't drawn that much enthusiasm on the list,
but it strikes me as the sort of thing that when you need it you really
need it.  It would be interesting to hear opinions about it from people
who deal with data recovery situations on a regular basis.

> - REINDEX CONCURRENTLY:
>   Imo pretty close to being comittable and pretty useful, but it got
>   redesigned pretty late and it mostly had review from me and fujii and
>   it could use a bit more input
>   => unclear

I think this really has no hope of being bulletproof until we have
MVCC-based catalog scans.  The work up to now has been good exploratory
effort, but it's not going to be committable without that
infrastructure, IMHO anyway.

> - psql watch
>   Waiting on author since 4 days
>   => wait or boot?

If there is agreement on the basic design of the feature, ie
"\watch [n]" as a repeating version of "\g", it seems to me that
this could be fixed and committed in a day or less.  I'd be willing
to look at it sometime next week.

> - transforms:
>   I am not sure what solution is proposed for the dependency issue
>   between shared objects.
>   There also hasn't been code level review for a while...
>   => unclear

Needs to be moved; no way this is ready in the next week or two.

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] Getting to 9.3 beta

2013-03-29 Thread Tom Lane
Magnus Hagander  writes:
> On Fri, Mar 29, 2013 at 4:05 PM, Tom Lane  wrote:
>> Next week is going to be tied up with the back-branch releases, but
>> maybe we could target beta for the week after?  The main gating factor
>> at this point really would be how quickly we could write some draft
>> release notes, so people know what to test.

> I think we need to give it at least one more week before we can target
> a beta.

Sure, I was just suggesting a best-case scenario.  Probably a more
realistic thing would be to target beta wrap for April 15, which would
leave an extra week or so for documentation polishing and alpha-level
testing.

> Of course, that doesn't prevent from starting work on the release
> notes meanwhile :)

Right ;-)

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] Drastic performance loss in assert-enabled build in HEAD

2013-03-29 Thread Tom Lane
Kevin Grittner  writes:
> Tom Lane  wrote:
>> So maybe I'm nuts to care about the performance of an assert-enabled
>> backend, but I don't really find a 4X runtime degradation acceptable,
>> even for development work.  Does anyone want to fess up to having caused
>> this, or do I need to start tracking down what changed?

> I checked master HEAD for a dump of regression and got about 4
> seconds.  I checked right after my initial push of matview code and
> got 2.5 seconds.  I checked just before that and got 1 second. 
> There was some additional pg_dump work for matviews after the
> initial push which may or may not account for the rest of the time.

I poked at this a bit, and eventually found that the performance
differential goes away if I dike out the pg_relation_is_scannable() call
in getTables()'s table-collection query.  What appears to be happening
is that those calls cause a great many more relcache entries to be made
than used to happen, plus many entries are made earlier in the run than
they used to be.  Many of those entries have subsidiary memory contexts,
which results in an O(N^2) growth in the time spent in AllocSetCheck,
since postgres.c does MemoryContextCheck(TopMemoryContext) once per
received command, and pg_dump will presumably issue O(N) commands in an
N-table database.

So one question that brings up is whether we need to dial back the
amount of work done in memory context checking, but I'm loath to do so
in development builds.  That code has fingered an awful lot of bugs.
OTOH, if the number of tables in the regression DB continues to grow,
we may have little choice.

Anyway, the immediate takeaway is that this represents a horribly
expensive way for pg_dump to find out which matviews aren't scannable.
The cheap way for it to find out would be if we had a boolean flag for
that in pg_class.  Would you review the bidding as to why things were
done the way they are?  Because in general, having to ask the kernel
something is going to suck in any case, so basing it on the size of the
disk file doesn't seem to me to be basically a good thing.

We could alleviate the pain by changing pg_dump's query to something
like

(case when c.relkind = 'm' then pg_relation_is_scannable(c.oid)
 else false end)

but TBH this feels like bandaging a bad design.

Another reason why I don't like this code is that
pg_relation_is_scannable is broken by design:

relid = PG_GETARG_OID(0);
relation = RelationIdGetRelation(relid);
result = relation->rd_isscannable;
RelationClose(relation);

You can't do that: if the relcache entry doesn't already exist, this
will try to construct one while not holding any lock on the relation,
which is subject to all sorts of race conditions.  (In general, direct
calls on RelationIdGetRelation are probably broken.  I see you
introduced more than one such in the matviews patch, and I'm willing to
bet they are all unsafe.)

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] Getting to 9.3 beta

2013-03-29 Thread Andrew Dunstan


On 03/29/2013 11:05 AM, Tom Lane wrote:

Bruce Momjian  writes:

Our final 9.3 commit-fest has has exceeded the two-month mark, so it is
time to start targeting a date to close it and get to 9.3 beta.  I see
25 items will needing attention before we can close it:
https://commitfest.postgresql.org/action/commitfest_view?id=17
What is a reasonable timeframe to target for completion of these items?

TBH, once Andrew commits the JSON patch, I wouldn't have a problem with
moving all the rest to Returned With Feedback or the next CF.


I can do that pretty much right away.

cheers

andrew



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


Re: [HACKERS] Getting to 9.3 beta

2013-03-29 Thread Andres Freund
On 2013-03-29 10:15:42 -0400, Bruce Momjian wrote:
> Our final 9.3 commit-fest has has exceeded the two-month mark, so it is
> time to start targeting a date to close it and get to 9.3 beta.  I see
> 25 items will needing attention before we can close it:

Very much agreed!

>   https://commitfest.postgresql.org/action/commitfest_view?id=17
>
> What is a reasonable timeframe to target for completion of these items?

Here's my take on it:

- Extension templates:
  Not ready yet, probably takes a bit more work till committable, last
  version got in rather late.
  => move to next fest

- Performance Improvement by reducing WAL for Update Operation:
  Not ready yet, some redesigns are recently made, benefits aren't all
  that great
  => move to next fest


- Index based regexp search for pg_trgm:
  Seems like the patch is undergoing restructuring of the regex access API
  => move to next fest

- Truncate trailing nulls from heap rows to reduce the size of the null
  bitmap:
  Not sure, benefits don't seem to be all that big for realistic
  usecases?
  => move or reject?

- allowing privileges on untrusted languages:
  Direction forward seems unclear, discussion required
  => move

- replace plugins directory with GUC
  Peter has agreed to boot it to the next fest afaics
  (515357f4.6000...@gmx.net)
  => move (done)

- sepgsql: name qualified default security label
  has been committed by robert, updated CF

- sepgsql: db_schema:search permission and sepgsql: db_procedure:execute
  permission:
  Not much review afaics.
  => looks unfair, but unless some comitter (robert?) takes it
  on I don't see much alternative to booting it to the next fest?

- Patch to compute Max LSN of Data Pages:
  I *personally* don't really see a point in including it in postgres,
  there doesn't really seem to be any real demand met by the tool
  => reject?

- Make recovery.conf parameters into GUCs:
  Nice to see progress, but this seems to be too late for 9.3.
  => move to -next

- pg_retainxlog for contrib:
  5102f7c9.6050...@gmx.net sums up my opinion of the utility, just
  doesn't seem to be reliable enough to be distributed with postgres.
  => reject/redesign?

- REINDEX CONCURRENTLY:
  Imo pretty close to being comittable and pretty useful, but it got
  redesigned pretty late and it mostly had review from me and fujii and
  it could use a bit more input
  => unclear

- built-in/SQL Command to edit postgresql.conf ("SET PERSISTENT" patch):
  Different patches afloat, none is really ready. I personally think
  Zoltan's patch is more realistic, but ...
  => move

- pg_ctl idempotent option:
  looks ready to me
  => commit

- New statistics for WAL buffer dirty writes:
  waiting on other for months
  => returned with feedback

- pg_stat_statements: query, session, and eviction identification:
  Seems to need at least docs
  => wait for author, seems to  be easy enough?

- Json API and extraction functions:
  Seems to be committable
  => commit

- psql watch
  Waiting on author since 4 days
  => wait or boot?

- User control over psql's error stream
  Unclear, there doesn't seem to be much progress and review
  => return with feedback

- plpgsql_check_function:
  Tom says (27661.1364267...@sss.pgh.pa.us) that even if the approach
  can be aggreed uppon it needs quite a bit more work
  => move

- transforms:
  I am not sure what solution is proposed for the dependency issue
  between shared objects.
  There also hasn't been code level review for a while...
  => unclear

- Check file parameters to psql before prompt for password:
  No answer to Stephen's review in december
  => return with feedback

- pkg-config for libpq and ecpg:
  Issues seem to be fixable easy enough
  => wait

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] Getting to 9.3 beta

2013-03-29 Thread Magnus Hagander
On Fri, Mar 29, 2013 at 4:05 PM, Tom Lane  wrote:
> Bruce Momjian  writes:
>> Our final 9.3 commit-fest has has exceeded the two-month mark, so it is
>> time to start targeting a date to close it and get to 9.3 beta.  I see
>> 25 items will needing attention before we can close it:
>
>>   https://commitfest.postgresql.org/action/commitfest_view?id=17
>
>> What is a reasonable timeframe to target for completion of these items?
>
> TBH, once Andrew commits the JSON patch, I wouldn't have a problem with
> moving all the rest to Returned With Feedback or the next CF.  None of
> the others seem to me to be close-to-committable (with the possible
> exception of the Max LSN patch, which I've not looked at), and April is
> not the time to be doing development.

I haven't looked through too many of them. as for the one I have on
there myself, the pg_retainxlog one, i think the consensus was to turn
it into a documentation patch - and we can keep adding doc patches
into beta.

But, to the point. It might be a decent starting point to at least
move all patches that are "waiting on author" and doesn't get almost
daily status updates (or has a very clear statement from the author on
when an update can be expected). That'll cut the list down quite a bit
I think, and should make it easier to push harder on the others
perhaps.


> Next week is going to be tied up with the back-branch releases, but
> maybe we could target beta for the week after?  The main gating factor
> at this point really would be how quickly we could write some draft
> release notes, so people know what to test.

I think we need to give it at least one more week before we can target
a beta. Packagers will if anything have *more* work to deal with these
backbranch releases than usual, and asking them to push another one
just one (or even two) weeks after that would be rushing it
unnecessarily...

Of course, that doesn't prevent from starting work on the release
notes meanwhile :)

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


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


Re: [HACKERS] Getting to 9.3 beta

2013-03-29 Thread Tom Lane
Bruce Momjian  writes:
> Our final 9.3 commit-fest has has exceeded the two-month mark, so it is
> time to start targeting a date to close it and get to 9.3 beta.  I see
> 25 items will needing attention before we can close it:

>   https://commitfest.postgresql.org/action/commitfest_view?id=17

> What is a reasonable timeframe to target for completion of these items?

TBH, once Andrew commits the JSON patch, I wouldn't have a problem with
moving all the rest to Returned With Feedback or the next CF.  None of
the others seem to me to be close-to-committable (with the possible
exception of the Max LSN patch, which I've not looked at), and April is
not the time to be doing development.

Next week is going to be tied up with the back-branch releases, but
maybe we could target beta for the week after?  The main gating factor
at this point really would be how quickly we could write some draft
release notes, so people know what to test.

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] Changing recovery.conf parameters into GUCs

2013-03-29 Thread Simon Riggs
On 29 March 2013 01:17, Michael Paquier  wrote:

> I highly recommend that
> you use one of the latest updated version I sent. Fujii's version had some
> bugs, one of them being that as standbyModeRequested can be set to true if
> specified in postgresql.conf, a portion of the code using in xlog.c can be
> triggered even if ArchiveRecoveryRequested is not set to true. I also added
> fixes related to documentation.

Yes, that is what I meant by "Fujii's patch". He would still be
credited first, having done the main part of the work. I'll call it
Michael's updated version to avoid any further confusion.

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


[HACKERS] Getting to 9.3 beta

2013-03-29 Thread Bruce Momjian
Our final 9.3 commit-fest has has exceeded the two-month mark, so it is
time to start targeting a date to close it and get to 9.3 beta.  I see
25 items will needing attention before we can close it:

https://commitfest.postgresql.org/action/commitfest_view?id=17

What is a reasonable timeframe to target for completion of these items?

-- 
  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] Changing recovery.conf parameters into GUCs

2013-03-29 Thread Bruce Momjian
On Fri, Mar 29, 2013 at 01:56:50PM +, Simon Riggs wrote:
> On 29 March 2013 13:24, Michael Paquier  wrote:
> > On Fri, Mar 29, 2013 at 9:59 PM, Simon Riggs  wrote:
> >>
> >> On 29 March 2013 01:17, Michael Paquier  wrote:
> >> > On Fri, Mar 29, 2013 at 12:48 AM, Simon Riggs 
> >> > wrote:
> >> Early discussions had difficulties because of the lack of config
> >> directories, include_if_exists and this patch. We now have the
> >> technical capability to meet every request. Circumstances have changed
> >> and outcomes may change also.
> >
> > Thanks for the clarifications. The following questions are still unanswered:
> 
> Minor points only. We can implement this differently if you have
> alternate proposals.

Seems we are doing design on this long after the final 9.3 commit fest
should have closed.  I think we need to punt this to 9.4.

-- 
  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] Changing recovery.conf parameters into GUCs

2013-03-29 Thread Simon Riggs
On 29 March 2013 13:24, Michael Paquier  wrote:
> On Fri, Mar 29, 2013 at 9:59 PM, Simon Riggs  wrote:
>>
>> On 29 March 2013 01:17, Michael Paquier  wrote:
>> > On Fri, Mar 29, 2013 at 12:48 AM, Simon Riggs 
>> > wrote:
>> Early discussions had difficulties because of the lack of config
>> directories, include_if_exists and this patch. We now have the
>> technical capability to meet every request. Circumstances have changed
>> and outcomes may change also.
>
> Thanks for the clarifications. The following questions are still unanswered:

Minor points only. We can implement this differently if you have
alternate proposals.

> 1) If recovery.trigger and recovery.conf are specified. To which one the
> priority is given?

Neither. No priority is required. If either is present we are triggered.

> 2) If both recovery.trigger and recovery.conf are used, let's imagine that
> the server removes recovery.trigger but fails in renaming recovery.conf but
> a reason or another. Isn't there a risk of inconsistency if both triggering
> methods are used at the same time?

No. If writes to the filesystem fail, you have much bigger problems.

> 3) Forcing a harcode of include_is_exists = 'recovery.conf' at the bottom of
> postgresql.conf doesn't look like a hack?

Well, that's just an emotive term to describe something you don't
like. There are no significant downsides, just a few lines of code,
like we have in many places for various purposes, such as the support
of multiple APIs for triggering standbys.

> 4) Based on your proposal, are all the parameters included in
> postgresql.conf.sample or not? Or only primary_conninfo, trigger_file and
> standby_mode?

Other values are specific to particular situations (e.g. PITR) and if
set in the wrong context could easily break replication. We could add
them if people wish it, but it would be commented out with a clear
"don't touch these" message, so it seems more sensible to avoid them
IMHO.

-- 
 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] Changing recovery.conf parameters into GUCs

2013-03-29 Thread Michael Paquier
On Fri, Mar 29, 2013 at 9:59 PM, Simon Riggs  wrote:

> On 29 March 2013 01:17, Michael Paquier  wrote:
> > On Fri, Mar 29, 2013 at 12:48 AM, Simon Riggs 
> wrote:
> Early discussions had difficulties because of the lack of config
> directories, include_if_exists and this patch. We now have the
> technical capability to meet every request. Circumstances have changed
> and outcomes may change also.
>
Thanks for the clarifications. The following questions are still unanswered:
1) If recovery.trigger and recovery.conf are specified. To which one the
priority is given?
2) If both recovery.trigger and recovery.conf are used, let's imagine that
the server removes recovery.trigger but fails in renaming recovery.conf but
a reason or another. Isn't there a risk of inconsistency if both triggering
methods are used at the same time?
3) Forcing a harcode of include_is_exists = 'recovery.conf' at the bottom
of postgresql.conf doesn't look like a hack?
4) Based on your proposal, are all the parameters included in
postgresql.conf.sample or not? Or only primary_conninfo, trigger_file and
standby_mode?
-- 
Michael


Re: [HACKERS] Changing recovery.conf parameters into GUCs

2013-03-29 Thread Simon Riggs
On 29 March 2013 01:17, Michael Paquier  wrote:

> The main argument on which this proposal is based on is to keep
> backward-compatibility.

The main objective is to get recovery parameters as GUCs, as I said

> On Fri, Mar 29, 2013 at 12:48 AM, Simon Riggs  wrote:
>> What we want to do is make recovery parameters into GUCs, allowing
>> them to be reset by SIGHUP and also to allow all users to see the
>> parameters in use, from any session.

>From the user's perspective, we are making no changes. All recovery
parameters will work exactly the same as they always did, just now we
get to see their values more easily and we can potentially place them
in a different file if we wish. The user will have no idea that we
plan to do some internal refactoring of how we process the parameters.
So IMHO simplicity means continuing to work the way it always did
work. We simply announce "PostgreSQL now supports configuration
directories. All parameters, including recovery parameters, can be
placed in any configuration file, or in $PGDATA/recovery.conf, as
before".

We introduced "pg_ctl promote" with a new API without removing
existing ones, and some people are still in favour of keeping both
APIs. Doing the same thing here makes sense and reduces conceptual
change.

Early discussions had difficulties because of the lack of config
directories, include_if_exists and this patch. We now have the
technical capability to meet every request. Circumstances have changed
and outcomes may change also.

-- 
 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] in-catalog Extension Scripts and Control parameters (templates?)

2013-03-29 Thread Dimitri Fontaine
Hi,

Thanks you for testing and reporting those strange bugs, I should be
able to fix them by Tuesday at the earliest.

Heikki Linnakangas  writes:
> create template for extension sslinfo version '1.0' with (schema public) as
> $$ DO EVIL STUFF $$;

What you're saying is that we should restrict the capability to
superusers only, where I didn't think about those security implications
before and though that it wouldn't be a necessary limitation.

I will add the usual superuser() checks in the next version of the
patch.

> Documentation doesn't build, multiple errors. In addition to the reference
> pages, there should be a section in the main docs about these templates.

I really would like for a simpler setup to build documentation on my OS
of choice, and realize that's no excuse. Will clean that up in next
version of the patch.

>> postgres=# create template for extension myextension version '1.0' with () 
>> as 'foobar';
>> CREATE TEMPLATE FOR EXTENSION
>> postgres=# create extension myextension;
>> ERROR:  syntax error at or near "foobar"
>> LINE 1: create extension myextension;
>> ^
>
> Confusing error message.

Do we need to compute a different error message when applying the script
from a template or from a file on-disk? Also please keep in mind that
those error messages are typically to be seen by the extension's author.

>> postgres=# create template for extension myextension version '1.0' with () 
>> as $$create table foobar(i int4) $$;
>> CREATE TEMPLATE FOR EXTENSION
>> postgres=# create extension myextension;
>> CREATE EXTENSION
>> postgres=# select * from foobar;
>> ERROR:  relation "foobar" does not exist
>> LINE 1: select * from foobar;
>>   ^
>
> Where did that table go?

Well that's not the answer I wanted to make, but:

select c.oid, relname, nspname
  from pg_class c join pg_namespace n on n.oid = c.relnamespace
 where relname ~ 'foobar';
  oid  | relname |   nspname   
---+-+-
 41412 | foobar  | 1.0
(1 row)

> Ah, here... Where did that "1.0" schema come from?

I need to sort that out. Didn't have that problem in my tests (included
in the regression tests), will add your test case and see about fixing
that bug in the next version of the patch.

>> postgres=> create template for extension myextension version '1.0' with
>> (schema public) as $$ create function evilfunc() returns int4 AS
>> evilfunc' language internal; $$;
>> CREATE TEMPLATE FOR EXTENSION
>> postgres=> create extension myextension version '1.0';ERROR:  permission 
>> denied for language internal
>> postgres=> drop template for extension myextension version '1.0';
>> ERROR:  extension with OID 16440 does not exist
>
> Something wrong with catalog caching.

Or something wrong with dependencies maybe… will have a look at that
too, and add some regression tests.

>> $ make -s  install
>> /usr/bin/install: cannot stat `./hstore--1.0.sql': No such file or directory
>> make: *** [install] Error 1
>
> Installing hstore fails.

Works for me. Anyway that part was to show up how we could have been
managing the hstore 1.1 update in the past, I don't intend for it to get
commited unless specifically asked to do so.

I guess I should now remove hstore changes from the patch now, and will
do so in the next version of the patch.

> If we check for an existing extension at CREATE, should also check for that
> in ALTER ... RENAME TO.

Indeed. Will fix that too.

> Also:
> pg_dump does not dump the owner of an extension template correctly.

Will look into that too.


Thanks for your reviewing and testing, sorry to have missed those bugs.
The new version of the patch, early next week, will include fixes for
all of those and some more testing.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


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


Re: [HACKERS] sql_drop Event Triggerg

2013-03-29 Thread Dimitri Fontaine
Alvaro Herrera  writes:
> Pushed, with some further minor changes.  One not-so-minor change I

Thanks a lot for all the work you did put into this patch!

> introduced was that pg_event_trigger_dropped_objects() now only works
> within a sql_drop event function.  The reason I decided to do this was
> that if we don't have that protection, then it is possible to have a
> ddl_command_end trigger calling pg_event_trigger_dropped_objects(); and
> if there is an sql_drop trigger, then it'd return the list of dropped
> objects, but if there's no sql_drop trigger, it'd raise an error.  That
> seemed surprising enough action-at-a-distance that some protection is
> warranted.

+1

I hope that we can add another such function for ddl_command_start and
ddl_command_end function to get at some object information from there,
now that the support code is in place.

That would allow making any use of the event trigger facility in 9.3
without having to write a C coded extension… which has been the goal for
the last two cycles…

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


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


Re: [HACKERS] Enabling Checksums

2013-03-29 Thread Andres Freund
On 2013-03-28 21:02:06 -0400, Robert Haas wrote:
> On Wed, Mar 27, 2013 at 10:15 AM, Andres Freund  
> wrote:
> > On 2013-03-27 10:06:19 -0400, Robert Haas wrote:
> >> On Mon, Mar 18, 2013 at 4:31 PM, Greg Smith  wrote:
> >> > to get them going again.  If the install had checksums, I could have 
> >> > figured
> >> > out which blocks were damaged and manually fixed them, basically go on a
> >> > hunt for torn pages and the last known good copy via full-page write.
> >>
> >> Wow.  How would you extract such a block image from WAL?
> >>
> >> That would be a great tool to have, but I didn't know there was any
> >> practical way of doing it today.
> >
> > Given pg_xlogdump that should be doable with 5min of hacking in 9.3. Just 
> > add
> > some hunk to write out the page to the if (config->bkp_details) hunk in
> > pg_xlogdump.c:XLogDumpDisplayRecord. I have done that for some debugging 
> > already.
> >
> > If somebody comes up with a sensible & simple UI for this I am willing to
> > propose a patch adding it to pg_xlogdump. One would have to specify the
> > rel/file/node, the offset, and the target file.
> 
> Hmm.  Cool.  But, wouldn't the hard part be to figure out where to
> start reading the WAL in search of the *latest* FPI?

I'd expect having to read the whole WAL and write out all the available
FPIs. You might be able to a guess a bit based on the LSN in the header.

 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