Re: [HACKERS] [WIP PATCH] for Performance Improvement in Buffer Management

2012-10-20 Thread Amit kapila
On Friday, October 19, 2012 9:15 PM Jeff Janes wrote:
On Tue, Sep 4, 2012 at 6:25 AM, Amit kapila amit.kap...@huawei.com wrote:
 On Tuesday, September 04, 2012 12:42 AM Jeff Janes wrote:
 On Mon, Sep 3, 2012 at 7:15 AM, Amit kapila amit.kap...@huawei.com wrote:
 This patch is based on below Todo Item:

 Consider adding buffers the background writer finds reusable to the free
 list



 I have tried implementing it and taken the readings for Select when all the
 data is in either OS buffers

 or Shared Buffers.



 As I understood and anlyzed based on above, that there is problem in 
 attached patch such that in function
 InvalidateBuffer(), after UnlockBufHdr() and before PartitionLock if some 
 backend uses that buffer and increase the usage count to 1, still
 InvalidateBuffer() will remove the buffer from hash table and put it in 
 Freelist.
 I have modified the code to address above by checking refcount  usage_count 
  inside Partition Lock
 , LockBufHdr and only after that move it to freelist which is similar to 
 InvalidateBuffer.
 In actual code we can optimize the current code by using extra parameter in 
 InvalidateBuffer.

 Please let me know if I understood you correctly or you want to say 
 something else by above comment?

 Yes, I think that this is part of the risk I was hinting at.  I
 haven't evaluated your fix to it.  But assuming it is now safe, I
 still think it is a bad idea to invalidate a perfectly good buffer.
 Now a process that wants that page will have to read it in again, even
 though it is still sitting there.  This is particularly bad because
 the background writer is coded to always circle the buffer pool every
 2 minutes, whether that many clean buffers are needed or not.  I think
 that that is a bad idea, but having it invalidate buffers as it goes
 is even worse.

That is true, but is it not the case of low activity, and in general BGwriter 
takes into account how many buffers alloced and clock swipe completed passes to 
make sure it cleans the buffers appropriately.
One more doubt I have is whether this behavior (circle the buffer pool every 2 
minutes) can't be controlled by 'bgwriter_lru_maxpages' as this number can 
dictate how much buffers to clean in each cycle.

 I think the code for the free-list linked list is written so that it
 performs correctly for a valid buffer to be on the freelist, even
 though that does not happen under current implementations. 

 If you
 find that a buffer on the freelist has become pinned, used, or dirty
 since it was added (which can only happen if it is still valid), you
 just remove it and try again.

Is it  actually possible in any usecase, that buffer mgmt algorithm can find 
any buffer on freelist which is pinned or is dirty?


 Also, do we want to actually invalidate the buffers?  If someone does
 happen to want one after it is put on the freelist, making it read it
 in again into a different buffer doesn't seem like a nice thing to do,
 rather than just letting it reclaim it.

 But even if bgwriter/checkpoint don't do, Backend needing new buffer will do 
 similar things (remove from hash table) for this buffer as this is nextvictim 
 buffer.

 Right, but only if it is the nextvictim, here we do it if it is
 nextvictim+N, for some largish values of N.  (And due to the 2 minutes
 rule, sometimes for very large values of N)

Can't we control this 2 minutes rule using new or existing GUC, is there any 
harm in that as you pointed out earlier also in mail chain that it is not good.
Because such a parameter can make the flushing by BGwriter more valuable.

I'm not sure how to devise a test case to prove that this can be important, 
though.

To start with, can't we do simple test where all (most) of the pages are in 
shared buffers and then run pg_bench select only test?
This test we can run with various configurations of shared buffers.

I have done the tests similar to above, and it shows good perf. improvement for 
shared buffers conf. as(25% of RAM).


 Robert wrote an accounting patch a while ago that tallied how often a
 buffer was cleaned but then reclaimed for the same page before being
 evicted.  But now I can't find it.  If you can find that thread, there
 might be some benchmarks posted to it that would be useful.

In my first level search, I am also not able to find it. But now I am planning 
to check all
mails of Robert Haas on PostgreSQL site (which are approximately 13,000).
If you can tell me how long ago approximately (last year, 2 yrs back, ..) or 
whether such a patch is submitted to any CF or was just discussed in mail 
chain, then it will be little easier for me.


Thank you for doing the initial review of work.

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] [PATCH] explain tup_fetched/returned in monitoring-stats

2012-10-20 Thread Abhijit Menon-Sen
At 2012-10-15 10:28:17 -0400, robertmh...@gmail.com wrote:

  Is there any concise description that applies? […]
 
 I don't think there is.  I think we need to replace those counters
 with something better.  The status quo is quite bizarre.

Fair enough. Do you have any ideas?

I see two possibilities: first, they could become the tuple analogue of
blks_read and blks_hit, i.e. tuples fetched from disk, and tuples found
in memory. (I don't know if there's a simple way to count that, and I'm
not sure it would be very useful; we have blks_{read,hit} after all.)

Second, it could do what I thought it did, which is count tuples fetched
by sequential and index scans respectively. I'm not sure how useful the
values would be, but at least it's information you can't get elsewhere.

Also, what are the compatibility implications of changing this? I don't
think anyone is using the current *values*, but I imagine that changing
the column names might break some people's queries.

(I don't feel strongly about any course of action here. I just think the
current situation is unhelpful, and if there's a consensus about what to
change—whether code or documentation—I'm willing to do the work.)

-- Abhijit


-- 
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] Always include encoding of database in pg_dumpall

2012-10-20 Thread Jeremy Evans
On 10/19 06:55, Tom Lane wrote:
 Jeremy Evans c...@jeremyevans.net writes:
  I've have a PostgreSQL database cluster that I've continually upgraded
  from 7.1 to 9.1 without problems using pg_dumpall and psql.  When
  migrating to 9.2, I decided to change the default encoding for the
  database cluster from SQL_ASCII to UTF8.  When I went to restore my
  database backup (created using 9.1's pg_dumpall), the tables
  containing data not in UTF8 format were empty.
 
 I assume there were some errors reported?
 
Yes.  Of course, using psql to restore the dump, the error messages are
interspersed with all of the notices output, making it harder to see the
errors.

  This appears to be caused by pg_dumpall omitting the ENCODING when
  dumping if it is the same as the current database cluster's default
  encoding.  I believe this is a bad idea, as there is no guarantee the
  backup will be restored on a cluster with the same default encoding.
 
 I'm not persuaded by this argument.  The choice to omit default encoding
 and locale settings was intentional; if you choose to load into an
 installation with different defaults, it's probably because you *want*
 to change the encoding/locale you're using.  What you propose would make
 it impossible to do that.  Indeed, I'd have thought that was exactly
 the use-case you were trying to accomplish here.
 
My use case was just to change the default encoding to UTF8 so I
wouldn't have to set the encoding manually when creating databases in
the future, I didn't want to change the encoding of any of my current
databases.

I think the current behavior gives preference to usability at the
expense of reliability, but I understand that that might be an
appropriate decision to make.

  I'm not sure if similar changes should be made for LC_COLLATE or
  LC_CTYPE, but that's something that should be considered.
 
 It would be a particularly bad idea to change the behavior for the
 locale names, because those aren't consistent across platforms.
 If we didn't suppress them when possible, cross-platform restores
 would likely become impossible.

That makes sense.

Thanks for considering my patch. :)

Jeremy


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


[HACKERS] Foreign key constraint on sub-column of composite-type column

2012-10-20 Thread David Lee
Hi,

I was trying to create foreign key constraints on a sub-column of a 
composite-type column, but couldn't find a way to do it. After asking around on 
IRC, it seems like this isn't supported in PostgreSQL.

I wanted to do something like:

create type profile as (account_id integer);

create table users (profile profile);

alter table users add constraint account_fk foreign key 
((profile).account_id) references accounts;

Would this be a viable feature request?

--David

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


[HACKERS] patch to add \watch to psql

2012-10-20 Thread Will Leinweber
This patch adds \watch to psql. It is much like the unix equivalent,
defaulting to every 2 seconds, and allowing you optionally specify a number
of seconds.

I will add this to the commit fest app.

Thanks,
Will Leinweber

Example:

psql (9.3devel, server 9.1.4)
Type help for help.

will=# \watch select now();
Watch every 2s Fri Oct 19 17:09:23 2012

  now
---
 2012-10-19 17:09:23.743176-07
(1 row)

Watch every 2s Fri Oct 19 17:09:25 2012

  now
---
 2012-10-19 17:09:25.745125-07
(1 row)

Watch every 2s Fri Oct 19 17:09:27 2012

  now
---
 2012-10-19 17:09:27.746732-07
(1 row)

^Cwill=# \watch 5 select now();
Watch every 5s Fri Oct 19 17:09:33 2012

  now
---
 2012-10-19 17:09:33.563695-07
(1 row)

Watch every 5s Fri Oct 19 17:09:38 2012

  now
---
 2012-10-19 17:09:38.564802-07
(1 row)

^Cwill=# \watch select pg_sleep(20);
^CCancel request sent
ERROR:  canceling statement due to user request
will=#


psql-watch-v1.patch
Description: Binary 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] patch to add \watch to psql

2012-10-20 Thread Abhijit Menon-Sen
At 2012-10-19 17:15:27 -0700, w...@heroku.com wrote:

 will=# \watch select now();
 Watch every 2s Fri Oct 19 17:09:23 2012
 
   now
 ---
  2012-10-19 17:09:23.743176-07
 (1 row)

The patch looks OK at first glance, and I can confirm that it works as
intended. I can imagine this functionality being useful, e.g. to watch
pg_stat_activity or similar tables.

But a big part of why watch(1) is nice is that the output is overwritten
rather than scrolling. When it's scrolling, it's (a) difficult to notice
that something has changed, and (b) harder to compare values, especially
when the interval is short.

For these reasons, I can imagine using watch -n2 psql -c …, but not
\watch in its present form. (Of course, I doubt anyone would be enthused
about a proposal to link ncurses into psql, but that's another matter.)

Maybe you should call it \repeat or something. I'm sure people would get
around to using \watch that way eventually. :-)

-- Abhijit


-- 
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] Move postgresql_fdw_validator into dblink

2012-10-20 Thread Kohei KaiGai
2012/10/19 Robert Haas robertmh...@gmail.com:
 On Fri, Oct 19, 2012 at 7:17 AM, Shigeru HANADA
 shigeru.han...@gmail.com wrote:
 However, I'm not sure where that leaves us with respect to the original
 goal of getting rid of use of that function name.  Thoughts?

 Sorry, I had misunderstood the problem :-(.  In my proposal, postgresql_fdw
 uses public schema, as other contrib modules do, so its validator can live
 with existing pg_catalog.postgresql_fdw_validator.  IMHO we should
 remove  postgresql_fdw_validator sooner or later, but we don't need to hurry
 to remove existing postgresql_fdw_validator from core.

 Of course we must ensure that postgresql_fdw never uses in-core validator,
 and dblink and other product never use postgresql_fdw's validator.  To
 achieve this, how about to use a schema, say postgresql_fdw, for
 postgresql_fdw by specifying schema option in extension control file?
 We need to qualify function names, so relocatable should be false.  This
 requires users of postgresql_fdw to set search_path or qualify
 postgresql_fdw's functions and views every time, but it seems acceptable.

 In addition, this approach would prevent pollution of public schema.

 It seems to me that this is a case of the tail wagging the dog.  The
 original reason we ran into this issue is because there were some
 people (I forget who, sorry) who insisted that this had to be renamed
 from pgsql_fdw to postgresql_fdw.  That change then caused this naming
 conflict.  Now, normally what we do if we have a naming conflict is we
 rename one of the two things so that we don't have a naming conflict.
 If we've determined that we can't rename postgresql_fdw_validator for
 reasons of backward compatibility, then we should rename this new
 thing instead.  We of course do not have to use the original
 pgsql_fdw_validator name; it can be postgres_fdw_validator or
 postgree_fdw_validator or prostgreskewell_fdw_validator or whatever
 the consensus bikeshed position is.

IIRC, the reason why postgresql_fdw instead of pgsql_fdw was
no other fdw module has shorten naming such as ora_fdw for
Oracle.
However, I doubt whether it is enough strong reason to force to
solve the technical difficulty; naming conflicts with existing user
visible features.
Isn't it worth to consider to back to the pgsql_fdw_validator
naming again?

Thanks,
-- 
KaiGai Kohei kai...@kaigai.gr.jp


-- 
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] Foreign key constraint on sub-column of composite-type column

2012-10-20 Thread Simon Riggs
On 20 October 2012 06:57, David Lee davidomu...@gmail.com wrote:

 I was trying to create foreign key constraints on a sub-column of a 
 composite-type column, but couldn't find a way to do it. After asking around 
 on IRC, it seems like this isn't supported in PostgreSQL.

 I wanted to do something like:

 create type profile as (account_id integer);

 create table users (profile profile);

 alter table users add constraint account_fk foreign key 
 ((profile).account_id) references accounts;

 Would this be a viable feature request?

We're currently trying to add FKs for arrays. Other more complex
constraints make a lot of sense, just as exclusion constraints make
sense for single table constraints.

I think its worth logging so we know somebody actually wants this,
thanks for your input.

-- 
 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] [PATCH] explain tup_fetched/returned in monitoring-stats

2012-10-20 Thread Simon Riggs
On 20 October 2012 07:43, Abhijit Menon-Sen a...@2ndquadrant.com wrote:
 At 2012-10-15 10:28:17 -0400, robertmh...@gmail.com wrote:

  Is there any concise description that applies? […]

 I don't think there is.  I think we need to replace those counters
 with something better.  The status quo is quite bizarre.

 Fair enough. Do you have any ideas?

 I see two possibilities: first, they could become the tuple analogue of
 blks_read and blks_hit, i.e. tuples fetched from disk, and tuples found
 in memory. (I don't know if there's a simple way to count that, and I'm
 not sure it would be very useful; we have blks_{read,hit} after all.)

 Second, it could do what I thought it did, which is count tuples fetched
 by sequential and index scans respectively. I'm not sure how useful the
 values would be, but at least it's information you can't get elsewhere.

We already have the second one on pg_stat_all_tables.

A third possibility exists, which is the one Tom described above.

Collecting information at pg_stat_database level isn't interesting
anyway (to me) for information that can be collected at table level.

-- 
 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] Bugs in CREATE/DROP INDEX CONCURRENTLY

2012-10-20 Thread Kevin Grittner
Simon Riggs wrote:
 Kevin, you're good to go on the SSI patch, or I'll apply next week
 if you don't. Thanks for that.

There were some hunks failing because of minor improvements to the
comments you applied, so attached is a version with trivial
adjustments for that.  Will apply tomorrow if there are no further
objections.

Nice feature, BTW!

-Kevin
*** a/src/backend/catalog/index.c
--- b/src/backend/catalog/index.c
***
*** 1320,1325  index_drop(Oid indexId, bool concurrent)
--- 1320,1337 
  	 * In the concurrent case we make sure that nobody can be looking at the
  	 * indexes by dropping the index in multiple steps, so we don't need a full
  	 * AccessExclusiveLock yet.
+ 	 *
+ 	 * All predicate locks on the index are about to be made invalid. Promote
+ 	 * them to relation locks on the heap. For correctness the index must not
+ 	 * be seen with indisvalid = true during query planning after the move
+ 	 * starts, so that the index will not be used for a scan after the
+ 	 * predicate lock move, as this could create new predicate locks on the
+ 	 * index which would not ensure a heap relation lock. Also, the index must
+ 	 * not be seen during execution of a heap tuple insert with indisready =
+ 	 * false before the move is complete, since the conflict with the
+ 	 * predicate lock on the index gap could be missed before the lock on the
+ 	 * heap relation is in place to detect a conflict based on the heap tuple
+ 	 * insert.
  	 */
  	heapId = IndexGetRelation(indexId, false);
  	if (concurrent)
***
*** 1445,1450  index_drop(Oid indexId, bool concurrent)
--- 1457,1470 
  		}
  
  		/*
+ 		 * No more predicate locks will be acquired on this index, and we're
+ 		 * about to stop doing inserts into the index which could show
+ 		 * conflicts with existing predicate locks, so now is the time to move
+ 		 * them to the heap relation.
+ 		 */
+ 		TransferPredicateLocksToHeapRelation(userIndexRelation);
+ 
+ 		/*
  		 * Now we are sure that nobody uses the index for queries, they just
  		 * might have it opened for updating it. So now we can unset
  		 * indisready and wait till nobody could update the index anymore.
***
*** 1514,1525  index_drop(Oid indexId, bool concurrent)
  		userHeapRelation = heap_open(heapId, ShareUpdateExclusiveLock);
  		userIndexRelation = index_open(indexId, AccessExclusiveLock);
  	}
! 
! 	/*
! 	 * All predicate locks on the index are about to be made invalid. Promote
! 	 * them to relation locks on the heap.
! 	 */
! 	TransferPredicateLocksToHeapRelation(userIndexRelation);
  
  	/*
  	 * Schedule physical removal of the files
--- 1534,1541 
  		userHeapRelation = heap_open(heapId, ShareUpdateExclusiveLock);
  		userIndexRelation = index_open(indexId, AccessExclusiveLock);
  	}
! 	else
! 		TransferPredicateLocksToHeapRelation(userIndexRelation);
  
  	/*
  	 * Schedule physical removal of the files

-- 
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] [WIP PATCH] for Performance Improvement in Buffer Management

2012-10-20 Thread Jeff Janes
On Fri, Sep 7, 2012 at 6:14 AM, Amit kapila amit.kap...@huawei.com wrote:
 On Thursday, September 06, 2012 2:38 PM Amit kapila wrote:
 On Tuesday, September 04, 2012 6:55 PM Amit kapila wrote:
 On Tuesday, September 04, 2012 12:42 AM Jeff Janes wrote:
 On Mon, Sep 3, 2012 at 7:15 AM, Amit kapila amit.kap...@huawei.com wrote:
 This patch is based on below Todo Item:

 Consider adding buffers the background writer finds reusable to the free
 list

 The results for the updated code is attached with this mail.
 The scenario is same as in original mail.
1. Load all the files in to OS buffers (using pg_prewarm with 'read' 
 operation) of all tables and indexes.
2. Try to load all buffers with pgbench_accounts table and 
 pgbench_accounts_pkey pages (using pg_prewarm with 'buffers' operation).
3. Run the pgbench with select only for 20 minutes.

 Platform details:
Operating System: Suse-Linux 10.2 x86_64
Hardware : 4 core (Intel(R) Xeon(R) CPU L5408 @ 2.13GHz)
RAM : 24GB

 Server Configuration:
shared_buffers = 5GB (1/4 th of RAM size)
Total data size = 16GB
 Pgbench configuration:
transaction type: SELECT only
scaling factor: 1200
query mode: simple
number of clients: varying from 8 to 64 
number of threads: varying from 8 to 64 
duration: 1200 s

 I shall take further readings for following configurations and post the same:
 1. The intention for taking with below configuration is that, with the 
 defined testcase, there will be some cases where I/O can happen. So I wanted 
 to check the
 impact of it.

 Shared_buffers - 7 GB
 number of clients: varying from 8 to 64 
 number of threads: varying from 8 to 64 
 transaction type: SELECT only

 The data for shared_buffers = 7GB is attached with this mail. I have also 
 attached scripts used to take this data.

Is this result reproducible?  Did you monitor IO (with something like
vmstat) to make sure there was no IO going on during the runs?  Run
the modes in reciprocating order?

If you have 7GB of shared_buffers and 16GB of database, that comes out
to 23GB of data to be held in 24GB of RAM.  In my experience it is
hard to get that much data cached by simple prewarm. the newer data
will drive out the older data even if technically there is room.  So
then when you start running the benchmark, you still have to read in
some of the data which dramatically slows down the benchmark.

I haven't been able to detect any reliable difference in performance
with this patch.  I've been testing with 150 scale factor with 4GB of
ram and 4 cores, over a variety of shared_buffers and concurrencies.

Cheers,

Jeff


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


Re: [HACKERS] [WIP PATCH] for Performance Improvement in Buffer Management

2012-10-20 Thread Jeff Janes
On Fri, Oct 19, 2012 at 11:00 PM, Amit kapila amit.kap...@huawei.com wrote:

 Robert wrote an accounting patch a while ago that tallied how often a
 buffer was cleaned but then reclaimed for the same page before being
 evicted.  But now I can't find it.  If you can find that thread, there
 might be some benchmarks posted to it that would be useful.

 In my first level search, I am also not able to find it. But now I am 
 planning to check all
 mails of Robert Haas on PostgreSQL site (which are approximately 13,000).
 If you can tell me how long ago approximately (last year, 2 yrs back, ..) or 
 whether such a patch is submitted
 to any CF or was just discussed in mail chain, then it will be little easier 
 for me.

It was just an instrumentation patch for doing experiments, not
intended for commit.

I've tracked it down to the thread Initial 9.2 pgbench write
results.  But I don't think it applies to the -S benchmark, because
it records when the background writer cleaned a buffer by finding it
dirty and writing it out to make it clean, while in this situation we
would need something more like either made the buffer clean and
reusable, observed the buffer to already be clean and reusable


Cheers,

Jeff


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


Re: [HACKERS] Bug in -c CLI option of pg_dump/pg_restore

2012-10-20 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Thu, Oct 18, 2012 at 11:19 AM, Alvaro Herrera
 alvhe...@2ndquadrant.com wrote:
 Hm, but the bug is said to happen only in 9.2, so if we don't backpatch
 we would leave 9.2 alone exhibiting this behavior.

 Oh, yeah.  I missed that.  But then shouldn't we start by identifying
 which commit broke it before we decide on a fix?

It looks like I broke this in commit
4317e0246c645f60c39e6572644cff1cb03b4c65, because I removed this from
_tocEntryRequired():

-   /* Ignore DATABASE entry unless we should create it */
-   if (!ropt-createDB  strcmp(te-desc, DATABASE) == 0)
-   return 0;

and transferred the responsibility to restore_toc_entry():

+   /*
+* Ignore DATABASE entry unless we should create it.  We must check this
+* here, not in _tocEntryRequired, because the createDB option should
+* not affect emitting a DATABASE entry to an archive file.
+*/
+   if (!ropt-createDB  strcmp(te-desc, DATABASE) == 0)
+   reqs = 0;

That was a good change for the reason stated in the comment ... but
I missed the fact that the old coding was also suppressing emission of a
DROP DATABASE command in -c mode.  I concur with Guillaume that this is
a bug --- in fact, I got burnt by there being a DROP DATABASE command in
the tar-format special script a few weeks ago, but I supposed that that
was a tar-specific issue of long standing, and didn't pursue it further
at the time.

I think Guillaume's fix is basically OK except it would be better if it
looked more like the code added to restore_toc_entry().  Will adjust and
commit.

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] FDW for PostgreSQL

2012-10-20 Thread Kohei KaiGai
2012/10/11 Etsuro Fujita fujita.ets...@lab.ntt.co.jp:
 Hi Hanada-san,

 Please examine attached v2 patch (note that is should be applied onto
 latest dblink_fdw_validator patch).

 I've reviewed your patch quickly.  I noticed that the patch has been created 
 in
 a slightly different way from the guidelines:
 http://www.postgresql.org/docs/devel/static/fdw-planning.html  The guidelines
 say the following, but the patch identifies the clauses in
 baserel-baserestrictinfo in GetForeignRelSize, not GetForeignPaths.  Also, it
 has been implemented so that most sub_expressions are evaluated at the remote
 end, not the local end, though I'm missing something.  For postgresql_fdw to 
 be
 a good reference for FDW developers, ISTM it would be better that it be
 consistent with the guidelines.  I think it would be needed to update the
 following document or redesign the function to be consistent with the 
 following
 document.

Hmm. It seems to me Fujita-san's comment is right.

Even though the latest implementation gets an estimated number of rows
using EXPLAIN with qualified SELECT statement on remote side, then, it is
adjusted with selectivity of local qualifiers, we shall be able to obtain same
cost estimation because postgresql_fdw assumes all the pushed-down
qualifiers are built-in only.

So, is it available to move classifyConditions() to pgsqlGetForeignPlan(),
then, separate remote qualifiers and local ones here?
If we call get_remote_estimate() without WHERE clause, remote side
will give just whole number of rows of remote table. Then, it can be adjusted
with selectivity of all the RestrictInfo (including both remote and local).

Sorry, I should suggest it at the beginning.

This change may affects the optimization that obtains remote columns
being in-use at local side. Let me suggest an expression walker that
sets member of BitmapSet for columns in-use at local side or target list.
Then, we can list up them on the target list of the remote query.

Thanks,
-- 
KaiGai Kohei kai...@kaigai.gr.jp


-- 
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 -c CLI option of pg_dump/pg_restore

2012-10-20 Thread Tom Lane
I wrote:
 It looks like I broke this in commit
 4317e0246c645f60c39e6572644cff1cb03b4c65, because I removed this from
 _tocEntryRequired():

 - /* Ignore DATABASE entry unless we should create it */
 - if (!ropt-createDB  strcmp(te-desc, DATABASE) == 0)
 - return 0;

Actually, on closer look, this change provides the foundation needed to
do more than just fix this bug.  We can also make the combination
pg_dump -C -c work sanely, which it never has before.  I propose that
we fix this with the attached patch (plus probably some documentation
changes, though I've not looked yet to see what the docs say about it).
With this fix, the output for -C -c looks like

DROP DATABASE regression;
CREATE DATABASE regression WITH ...
ALTER DATABASE regression OWNER ...
\connect regression
... etc ...

which seems to me to be just about exactly what one would expect.

The patch also gets rid of a kluge in PrintTOCSummary, which was needed
because of the old coding in _tocEntryRequired(), but no longer is.

regards, tom lane

diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 7f47a7c..1fead28 100644
*** a/src/bin/pg_dump/pg_backup_archiver.c
--- b/src/bin/pg_dump/pg_backup_archiver.c
*** RestoreArchive(Archive *AHX)
*** 303,317 
  	/*
  	 * Check for nonsensical option combinations.
  	 *
- 	 * NB: createDB+dropSchema is useless because if you're creating the DB,
- 	 * there's no need to drop individual items in it.  Moreover, if we tried
- 	 * to do that then we'd issue the drops in the database initially
- 	 * connected to, not the one we will create, which is very bad...
- 	 */
- 	if (ropt-createDB  ropt-dropSchema)
- 		exit_horribly(modulename, -C and -c are incompatible options\n);
- 
- 	/*
  	 * -C is not compatible with -1, because we can't create a database inside
  	 * a transaction block.
  	 */
--- 303,308 
*** RestoreArchive(Archive *AHX)
*** 456,462 
  		{
  			AH-currentTE = te;
  
! 			/* We want anything that's selected and has a dropStmt */
  			if (((te-reqs  (REQ_SCHEMA | REQ_DATA)) != 0)  te-dropStmt)
  			{
  ahlog(AH, 1, dropping %s %s\n, te-desc, te-tag);
--- 447,471 
  		{
  			AH-currentTE = te;
  
! 			/*
! 			 * In createDB mode, issue a DROP *only* for the database as a
! 			 * whole.  Issuing drops against anything else would be wrong,
! 			 * because at this point we're connected to the wrong database.
! 			 * Conversely, if we're not in createDB mode, we'd better not
! 			 * issue a DROP against the database at all.
! 			 */
! 			if (ropt-createDB)
! 			{
! if (strcmp(te-desc, DATABASE) != 0)
! 	continue;
! 			}
! 			else
! 			{
! if (strcmp(te-desc, DATABASE) == 0)
! 	continue;
! 			}
! 
! 			/* Otherwise, drop anything that's selected and has a dropStmt */
  			if (((te-reqs  (REQ_SCHEMA | REQ_DATA)) != 0)  te-dropStmt)
  			{
  ahlog(AH, 1, dropping %s %s\n, te-desc, te-tag);
*** PrintTOCSummary(Archive *AHX, RestoreOpt
*** 938,946 
  
  	ahprintf(AH, ;\n;\n; Selected TOC Entries:\n;\n);
  
- 	/* We should print DATABASE entries whether or not -C was specified */
- 	ropt-createDB = 1;
- 
  	curSection = SECTION_PRE_DATA;
  	for (te = AH-toc-next; te != AH-toc; te = te-next)
  	{
--- 947,952 

-- 
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] enhanced error fields

2012-10-20 Thread Peter Geoghegan
I think that we're both going to be busy next week, since we're both
attending pgconf.eu. For that reason, I would like to spend some time
tomorrow to get something in shape, that I can mark ready for
committer. I'd like to get this patch committed during this
commitfest. You are welcome to do this work instead. I want to avoid a
redundant effort.

Let me know if you think that that's a good idea.

-- 
Peter Geoghegan   http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services


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


Re: [HACKERS] patch to add \watch to psql

2012-10-20 Thread Daniel Farina
On Sat, Oct 20, 2012 at 12:19 AM, Abhijit Menon-Sen a...@2ndquadrant.com 
wrote:
 For these reasons, I can imagine using watch -n2 psql -c …, but not
 \watch in its present form. (Of course, I doubt anyone would be enthused
 about a proposal to link ncurses into psql, but that's another matter.)

A good point.

Part of the patch is that it disables the pager as so things just keep
being appended to the buffer.

However, I wonder if clever utilization of a pager would give you the
effect you mention with much the same code, and while allowing the old
behavior to be retained, as it is also useful.  (appending to the
screen, and not overwriting)

An unrelated defect, although the patch tries to carefully clean up
the 'res' result from psqlexec in the error cases, it does forget to
do that, seemingly, in the 'positive' case, while it is looping.  I
think it needs another pqclear in there.

-- 
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] Bug in -c CLI option of pg_dump/pg_restore

2012-10-20 Thread Guillaume Lelarge
On Sat, 2012-10-20 at 14:28 -0400, Tom Lane wrote:
 I wrote:
  It looks like I broke this in commit
  4317e0246c645f60c39e6572644cff1cb03b4c65, because I removed this from
  _tocEntryRequired():
 
  -   /* Ignore DATABASE entry unless we should create it */
  -   if (!ropt-createDB  strcmp(te-desc, DATABASE) == 0)
  -   return 0;
 
 Actually, on closer look, this change provides the foundation needed to
 do more than just fix this bug.  We can also make the combination
 pg_dump -C -c work sanely, which it never has before.  I propose that
 we fix this with the attached patch (plus probably some documentation
 changes, though I've not looked yet to see what the docs say about it).
 With this fix, the output for -C -c looks like
 
 DROP DATABASE regression;
 CREATE DATABASE regression WITH ...
 ALTER DATABASE regression OWNER ...
 \connect regression
 ... etc ...
 
 which seems to me to be just about exactly what one would expect.
 
 The patch also gets rid of a kluge in PrintTOCSummary, which was needed
 because of the old coding in _tocEntryRequired(), but no longer is.
 

Thanks a lot.


-- 
Guillaume
http://blog.guillaume.lelarge.info
http://www.dalibo.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] [PATCH] Enforce that INSERT...RETURNING preserves the order of multi rows

2012-10-20 Thread Albert Cervera i Areny
A Dimecres, 17 d'octubre de 2012 19:13:47, Merlin Moncure va escriure:
 On Wed, Oct 17, 2012 at 9:29 AM, Peter Geoghegan pe...@2ndquadrant.com 
wrote:
  On 17 October 2012 14:53, Merlin Moncure mmonc...@gmail.com wrote:
  Is that defined in the standard?
  
  RETURNING isn't even defined in the standard.
 
 Right: Point being, assumptions based on implementation ordering are
 generally to be avoided unless they are explicitly defined in the
 standard or elsewhere.

I don't see how one could use RETURNING if result is not ensured to be in the 
same order as the tuples supplied. What's the use of RETURNING supplying data 
in random order?

-- 
Albert Cervera i Areny
http://www.NaN-tic.com
Tel: +34 93 553 18 03

http://twitter.com/albertnan 
http://www.nan-tic.com/blog