Re: [HACKERS] bug of recovery?

2011-09-30 Thread Simon Riggs
On Fri, Sep 30, 2011 at 2:09 AM, Fujii Masao masao.fu...@gmail.com wrote:
 On Thu, Sep 29, 2011 at 11:12 PM, Florian Pflug f...@phlo.org wrote:
 On Sep29, 2011, at 13:49 , Simon Riggs wrote:
 This worries me slightly now though because the patch makes us PANIC
 in a place we didn't used to and once we do that we cannot restart the
 server at all. Are we sure we want that? It's certainly a great way to
 shake down errors in other code...

 The patch only introduces a new PANIC condition during archive recovery,
 though. Crash recovery is unaffected, except that we no longer create
 restart points before we reach consistency.

 Also, if we hit an invalid page reference after reaching consistency,
 the cause is probably either a bug in our recovery code, or (quite unlikely)
 a corrupted WAL that passed the CRC check. In both cases, the likelyhood
 of data-corruption seems high, so PANICing seems like the right thing to do.

 Fair enough.

 We might be able to use FATAL or ERROR instead of PANIC because they
 also cause all processes to exit when the startup process emits them.
 For example, we now use FATAL to stop the server in recovery mode
 when recovery is about to end before we've reached a consistent state.

I think we should issue PANIC if the source is a critical rmgr, or
just WARNING if from a non-critical rmgr, such as indexes.

Ideally, I think we should have a mechanism to allow indexes to be
marked corrupt. For example, a file that if present shows that the
index is corrupt and would be marked not valid. We can then create the
file and send a sinval message to force the index relcache to be
rebuilt showing valid set to false.

-- 
 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] [REVIEW] pg_last_xact_insert_timestamp

2011-09-30 Thread Simon Riggs
On Fri, Sep 30, 2011 at 3:24 AM, Kyotaro HORIGUCHI
horiguchi.kyot...@oss.ntt.co.jp wrote:

 Ok, I send this patch to comitters.

I repeat my objection to this patch. I'm very sorry I haven't been
around much in last few weeks to keep up a dialogue about this and to
make it clear how wrong I think this is.

Adding something onto the main path of transaction commit is not good,
especially when the only purpose of it is to run an occasional
monitoring query for those people that use replication. Not everybody
uses replication and even people that do use it don't need to run it
so frequently that every single commit needs this. This is just bloat
that we do not need and can also easily avoid.

The calculation itself would be problematic since it differs from the
way standby_delay is calculated on the standby, which will cause much
confusion. Some thought or comment should be made about that also.

If we want to measure times, we can easily send regular messages into
WAL to provide this function. Using checkpoint records would seem
frequent enough to me. We don't always send checkpoint records but we
can send an info message instead if we are streaming. If
archive_timeout is not set and max_wal_senders  0 then we can send an
info WAL message with timestamp on a regular cycle.

Alternatively, we can send regular special messages from WALSender to
WALReceiver that do not form part of the WAL stream, so we don't bulk
up WAL archives. (i.e. don't use w messages). That seems like the
most viable approach to this problem.

-- 
 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] fix for pg_upgrade

2011-09-30 Thread panam
Great, thanks!

--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/fix-for-pg-upgrade-tp3411128p4856336.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

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


Re: [HACKERS] [v9.2] Fix Leaky View Problem

2011-09-30 Thread Noah Misch
On Sun, Sep 25, 2011 at 11:22:56PM -0400, Robert Haas wrote:
 On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch n...@leadboat.com wrote:
  On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote:
  Robert Haas ?09/25/11 10:58 AM 
 
   I'm not sure we've been 100% consistent about that, since we
   previously made CREATE OR REPLACE LANGUAGE not replace the owner
   with the current user.
 
  I think we've been consistent in *not* changing security on an
  object when it is replaced.
 
  [CREATE OR REPLACE FUNCTION does not change proowner or proacl]
 
  Good point. ?C-O-R VIEW also preserves column default values. ?I believe we 
  are
  consistent to the extent that everything possible to specify in each C-O-R
  statement gets replaced outright. ?The preserved characteristics *require*
  commands like GRANT, COMMENT and ALTER VIEW to set in the first place.
 
  The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION 
  reverts to
  SECURITY INVOKER if it's not specified each time. ?That default is safe, 
  though,
  while the proposed default of security_barrier=false is unsafe.
 
 Even though I normally take the opposite position, I still like the
 idea of dedicated syntax for this feature.  Not knowing what view
 options we might end up with in the future, I hate having to decide on
 what the general behavior ought to be.  But it would be easy to decide
 that CREATE SECURITY VIEW followed by CREATE OR REPLACE VIEW leaves
 the security flag set; it would be consistent with what we're doing
 with owner and acl information and wouldn't back us into any
 unpleasant decisions down the road.

I prefer the previous UI (WITH (security_barrier=...)) to this proposal, albeit
for diffuse reasons.  Both kinds of views can have the consequence of granting
new access to data.  One kind leaks tuples to untrustworthy code whenever it's
convenient for performance, and the other does not.  A non-security view would
not mimic either of these objects; it would be a mere subquery macro.  Using
WITH (...) syntax attached to the CREATE VIEW command better evokes the
similarity between the alternatives we're actually offering.  I also find it
mildly odd letting CREATE OR REPLACE VIEW update an object originating with
CREATE SECURITY VIEW.

Unqualified CREATE VIEW will retain no redeeming value apart from backward
compatibility; new applications with any concern for database-level security
should use only security_barrier=true and mark functions LEAKPROOF as needed.

nm

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


Re: [HACKERS] Re: Optimizing pg_trgm makesign() (was Re: WIP: Fast GiST index build)

2011-09-30 Thread Tom Lane
I wrote:
 Alexander Korotkov aekorot...@gmail.com writes:
 Isn't it possible to cache signature of newitem in gtrgm_penalty
 like gtrgm_consistent do this for query?

 [ studies that code for awhile ... ]  Ick, what a kluge.

 The main problem with that code is that the cache data gets leaked at
 the conclusion of a scan.  Having just seen the consequences of leaking
 the giststate, I think this is something we need to fix not emulate.

 I wonder whether it's worth having the GIST code create a special
 scan-lifespan (or insert-lifespan) memory context that could be used
 for cached data such as this?  It's already creating a couple of
 contexts for its own purposes, so one more might not be a big problem.
 We'd have to figure out a way to make that context available to GIST
 support functions, though, as well as something cleaner than fn_extra
 for them to keep pointers in.

I've been chewing on this for awhile and am now thinking that maybe what
gtrgm_consistent is doing isn't that unreasonable.  It's certainly
legitimate for it to use fn_extra to maintain state between calls.
The problem fundamentally is that when a function uses fn_extra to
reference data it keeps in the fn_mcxt context, there's an implicit
contract that fn_extra will survive for the same length of time that the
fn_mcxt context does.  (Otherwise there's no way for the function to
avoid leaking that data after it's been called for the last time using
that FmgrInfo.)  And GIST is violating that assumption: it resets
fn_extra when it does initGISTstate, but fn_mcxt gets set to
CurrentMemoryContext, which in the problematic cases is a query-lifespan
context that will still be around after the GIST indexscan is concluded.

So what I'm thinking we ought to do is redefine things so that
initGISTstate sets fn_mcxt to a context that has the same lifespan as
the GISTSTATE itself does.  We could possibly eliminate a few retail
pfree's in the process, eg by keeping the GISTSTATE itself in that same
context.

Having done that, what gtrgm_consistent is doing would be an officially
supported (dare I suggest even documented?) thing instead of a kluge,
and we could then adopt the same methodology in gtrgm_penalty.

Comments?

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] Mismatch of relation names: pg_toast.pg_toast_nnn during pg_upgrade from 8.4 to 9.1

2011-09-30 Thread Jamie Fox
I regret that as a part-timer recently brought back on here I didn't
get an opportunity to test this earlier.  The upgrade with the patch
worked fine on my first attempt.

Thanks again,

Jamie

On Wed, Sep 28, 2011 at 7:32 PM, Bruce Momjian br...@momjian.us wrote:
 Jamie Fox wrote:
 Thanks, I'm following the thread pg_upgrade automatic testing and
 will try the patch just detailed there.

 I have applied the patch to head and 9.1.X.  We still have a win32 bug
 to fix.  It is a shame I was not able to fix these before 9.1.1 was
 released.  :-(

 ---



 On Wed, Sep 28, 2011 at 12:50 AM, Peter Eisentraut pete...@gmx.net wrote:
  On tis, 2011-09-27 at 16:19 -0700, Jamie Fox wrote:
 
  It fails at this stage:
 
  ? ? Restoring user relation files
  ? ? linking /data/pgsql/prod-84/base/11564/2613 to
  /data/pgsql/prod-91/base/12698/12570
  ? ? linking /data/pgsql/prod-84/base/11564/2683 to
  /data/pgsql/prod-91/base/12698/12572
  ? ? Mismatch of relation names: database prod1, old rel
  pg_toast.pg_toast_54542379, new rel pg_toast.pg_toast_16735
  ? ? Failure, exiting
 
  This issue is known and a fix is currently being discussed.
 
 

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

 --
  Bruce Momjian  br...@momjian.us        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] [REVIEW] pg_last_xact_insert_timestamp

2011-09-30 Thread Robert Haas
On Fri, Sep 30, 2011 at 3:18 AM, Simon Riggs si...@2ndquadrant.com wrote:
 On Fri, Sep 30, 2011 at 3:24 AM, Kyotaro HORIGUCHI
 horiguchi.kyot...@oss.ntt.co.jp wrote:

 Ok, I send this patch to comitters.

 I repeat my objection to this patch. I'm very sorry I haven't been
 around much in last few weeks to keep up a dialogue about this and to
 make it clear how wrong I think this is.

 Adding something onto the main path of transaction commit is not good,
 especially when the only purpose of it is to run an occasional
 monitoring query for those people that use replication. Not everybody
 uses replication and even people that do use it don't need to run it
 so frequently that every single commit needs this. This is just bloat
 that we do not need and can also easily avoid.

I think the overhead of this is so completely trivial that we
shouldn't be concerned about it.  It's writing 12 bytes to shared
memory for each commit, without taking a lock, in a cache line that
should be minimally contended.  We write plenty of other data to
shared memory on each commit, and that's nowhere close to being the
expensive part of committing a transaction.  What's expensive is
fighting over WALInsertLock and ProcArrayLock and CLogControlLock, and
possibly waiting for the commit to be durably flushed to disk, and
maybe (at the margin) wrenching the cache lines in our PGPROC away
from whatever processor last stole them to do GetSnapshotData().  I
don't think that a couple of stores to uncontended shared memory are
going to make any difference.

 The calculation itself would be problematic since it differs from the
 way standby_delay is calculated on the standby, which will cause much
 confusion. Some thought or comment should be made about that also.

The string standby_delay doesn't appear in our source tree anywhere,
so I'm not sure what this is referring to.  In any case, I'm in favor
of this feature.  Currently, the only way to measure the lag on the
standby is to measure it in WAL bytes - and you get to write your own
script to parse the WAL positions.  This will allow people to measure
it in minutes.  That seems like a significant usability improvement.

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

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


Re: [HACKERS] [REVIEW] pg_last_xact_insert_timestamp

2011-09-30 Thread Simon Riggs
On Fri, Sep 30, 2011 at 8:11 PM, Robert Haas robertmh...@gmail.com wrote:
 On Fri, Sep 30, 2011 at 3:18 AM, Simon Riggs si...@2ndquadrant.com wrote:
 On Fri, Sep 30, 2011 at 3:24 AM, Kyotaro HORIGUCHI
 horiguchi.kyot...@oss.ntt.co.jp wrote:

 Ok, I send this patch to comitters.

 I repeat my objection to this patch. I'm very sorry I haven't been
 around much in last few weeks to keep up a dialogue about this and to
 make it clear how wrong I think this is.

 Adding something onto the main path of transaction commit is not good,
 especially when the only purpose of it is to run an occasional
 monitoring query for those people that use replication. Not everybody
 uses replication and even people that do use it don't need to run it
 so frequently that every single commit needs this. This is just bloat
 that we do not need and can also easily avoid.

 I think the overhead of this is so completely trivial that we
 shouldn't be concerned about it.  It's writing 12 bytes to shared
 memory for each commit, without taking a lock, in a cache line that
 should be minimally contended.  We write plenty of other data to
 shared memory on each commit, and that's nowhere close to being the
 expensive part of committing a transaction.  What's expensive is
 fighting over WALInsertLock and ProcArrayLock and CLogControlLock, and
 possibly waiting for the commit to be durably flushed to disk, and
 maybe (at the margin) wrenching the cache lines in our PGPROC away
 from whatever processor last stole them to do GetSnapshotData().  I
 don't think that a couple of stores to uncontended shared memory are
 going to make any difference.

If the feature could not be done another way, easily, I might agree.

The point is it isn't necessary, useful or elegant to do it this way
and *any* cycles added to mainline transactions need to have careful
justification. And there really isn't any justification for doing
things this way other than its the first way somebody thought of.

-- 
 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] [REVIEW] pg_last_xact_insert_timestamp

2011-09-30 Thread Robert Haas
On Fri, Sep 30, 2011 at 3:22 PM, Simon Riggs si...@2ndquadrant.com wrote:
 If the feature could not be done another way, easily, I might agree.

I don't see that you've offered a reasonable alternative.  The
alternative proposals that you proposed don't appear to me to be
solving the same problem.  AIUI, the requested feature is to be able
to get, on the master, the timestamp of the last commit or abort, just
as we can already get the timestamp of the last commit or abort
replayed on the standby.  Nothing you WAL log and no messages you send
to the standby are going to accomplish that goal.

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

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


Re: [HACKERS] [REVIEW] pg_last_xact_insert_timestamp

2011-09-30 Thread Simon Riggs
On Fri, Sep 30, 2011 at 8:57 PM, Robert Haas robertmh...@gmail.com wrote:
 On Fri, Sep 30, 2011 at 3:22 PM, Simon Riggs si...@2ndquadrant.com wrote:
 If the feature could not be done another way, easily, I might agree.

 I don't see that you've offered a reasonable alternative.  The
 alternative proposals that you proposed don't appear to me to be
 solving the same problem.  AIUI, the requested feature is to be able
 to get, on the master, the timestamp of the last commit or abort, just
 as we can already get the timestamp of the last commit or abort
 replayed on the standby.  Nothing you WAL log and no messages you send
 to the standby are going to accomplish that goal.

The goal of the OP was to find out the replication delay. This
function was suggested, but its not the only way.

My alternative proposals solve the original problem in a better way.

-- 
 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] pg_cancel_backend by non-superuser

2011-09-30 Thread Daniel Farina
This patch would appear(?) to have languished:

https://commitfest.postgresql.org/action/patch_view?id=541

I'd really like to see it included.  In the last comments of the
review, there seem to be problems in *terminate* backend, but even
just pg_cancel_backend as non-superuser would be just a huge
improvement.  What are the things blocking non-superuser
pg_cancel_backend from being accepted?

-- 
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] Re: Optimizing pg_trgm makesign() (was Re: WIP: Fast GiST index build)

2011-09-30 Thread Tom Lane
I wrote:
 So what I'm thinking we ought to do is redefine things so that
 initGISTstate sets fn_mcxt to a context that has the same lifespan as
 the GISTSTATE itself does.  We could possibly eliminate a few retail
 pfree's in the process, eg by keeping the GISTSTATE itself in that same
 context.
 Having done that, what gtrgm_consistent is doing would be an officially
 supported (dare I suggest even documented?) thing instead of a kluge,
 and we could then adopt the same methodology in gtrgm_penalty.

I've committed patches along this line.  On my machine, the time needed
to do the test case Heikki proposed (build a gist_trgm_ops index on the
contents of /usr/share/dict/words) drops from around 29.9 seconds to
about 17.3.  makesign is now down in the noise according to oprofile,
so I see no further reason to pursue the patch that started this thread.

What's not in the noise is the memcmp() required to check if the cached
makesign result is still good --- as best I can tell, that's near 50%
of the remaining runtime.  I don't see any non-invasive way to avoid
that.  If we were willing to redo the GiST support function API, we
could fix it, but I'm not sure it's worth that.

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] pg_cancel_backend by non-superuser

2011-09-30 Thread Tom Lane
Daniel Farina dan...@heroku.com writes:
 This patch would appear(?) to have languished:
 https://commitfest.postgresql.org/action/patch_view?id=541

 I'd really like to see it included.  In the last comments of the
 review, there seem to be problems in *terminate* backend, but even
 just pg_cancel_backend as non-superuser would be just a huge
 improvement.  What are the things blocking non-superuser
 pg_cancel_backend from being accepted?

I think the reason the patch stalled is that we have not got consensus
on how far to extend the conditions under which these operations should
be allowed.  For instance, in the last comment attached to that
commitfest entry, Noah alleges that a non-superuser database owner
should be allowed to kill a superuser's session, if it's connected
to his database.  My reaction to that is somewhere between no and
hell no; IMO superusers can mess up non-superusers, never vice versa.
If I recall the discussion correctly, there were other points of
contention too.

I don't think we need more coding right now ... we need somebody to
write a spec that everyone can agree to.

ISTM it would be reasonably non-controversial to allow users to issue
pg_cancel_backend against other sessions logged in as the same userID.
The question is whether to go further than that, and if so how much.

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] pg_cancel_backend by non-superuser

2011-09-30 Thread Torello Querci
2011/10/1 Tom Lane t...@sss.pgh.pa.us:
 Daniel Farina dan...@heroku.com writes:
 This patch would appear(?) to have languished:
 https://commitfest.postgresql.org/action/patch_view?id=541

 I'd really like to see it included.  In the last comments of the
 review, there seem to be problems in *terminate* backend, but even
 just pg_cancel_backend as non-superuser would be just a huge
 improvement.  What are the things blocking non-superuser
 pg_cancel_backend from being accepted?

 I think the reason the patch stalled is that we have not got consensus
 on how far to extend the conditions under which these operations should
 be allowed.  For instance, in the last comment attached to that
 commitfest entry, Noah alleges that a non-superuser database owner
 should be allowed to kill a superuser's session, if it's connected
 to his database.  My reaction to that is somewhere between no and
 hell no; IMO superusers can mess up non-superusers, never vice versa.
 If I recall the discussion correctly, there were other points of
 contention too.


Hi,

the original patch allow only for the DB Owner to kill sessions owner
by other users.
This because in real world I have some production database where I'm
not the DBA, but only the DB owner.

I think that is not a good idea that a normal users is able to kill
session from the same user because, unfortunally,
in some real environment there are a lots of application that need to
access to the same database and the same user is used.
I know that is not a good practise but it is on the field 

For this reason I suppose that allow only to DB onwer to kill other
sessions it is a good compromize between functionality and security,
but is my personal opinion ...

 I don't think we need more coding right now ... we need somebody to
 write a spec that everyone can agree to.

 ISTM it would be reasonably non-controversial to allow users to issue
 pg_cancel_backend against other sessions logged in as the same userID.
 The question is whether to go further than that, and if so how much.

                        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


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