Re: [HACKERS] getting rid of SnapshotNow

2013-08-05 Thread Bruce Momjian 
On Thu, Jul 25, 2013 at 07:24:53PM -0400, Robert Haas wrote:
 - /* Used by pre-9.0 binary upgrades */
 - if (tuple-t_infomask  HEAP_MOVED_OFF)
 - {
 - TransactionId xvac = HeapTupleHeaderGetXvac(tuple);
 -
 - if (TransactionIdIsCurrentTransactionId(xvac))
 - return false;
 - if (!TransactionIdIsInProgress(xvac))
 - {
 - if (TransactionIdDidCommit(xvac))
 - {
 - SetHintBits(tuple, buffer, 
 HEAP_XMIN_INVALID,
 - 
 InvalidTransactionId);
 - return false;
 - }
 - SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
 - InvalidTransactionId);
 - }
 - }
 - /* Used by pre-9.0 binary upgrades */
 - else if (tuple-t_infomask  HEAP_MOVED_IN)
 - {
 - TransactionId xvac = HeapTupleHeaderGetXvac(tuple);
 -
 - if (!TransactionIdIsCurrentTransactionId(xvac))
 - {
 - if (TransactionIdIsInProgress(xvac))
 - return false;
 - if (TransactionIdDidCommit(xvac))
 - SetHintBits(tuple, buffer, 
 HEAP_XMIN_COMMITTED,
 - 
 InvalidTransactionId);
 - else
 - {
 - SetHintBits(tuple, buffer, 
 HEAP_XMIN_INVALID,
 - 
 InvalidTransactionId);
 - return false;
 - }
 - }
 - }

One interesting aspect of this patch is that the backend code is no
longer even checking HEAP_MOVED_OFF and HEAP_MOVED_IN.  However, we
can't reuse those bits because they could be set from pre-9.0 rows.

-- 
  Bruce Momjian  br...@momjian.ushttp://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 rid of SnapshotNow

2013-08-05 Thread Andres Freund 
On 2013-08-05 11:17:08 -0400, Bruce Momjian wrote:
 One interesting aspect of this patch is that the backend code is no
 longer even checking HEAP_MOVED_OFF and HEAP_MOVED_IN.  However, we
 can't reuse those bits because they could be set from pre-9.0 rows.

The other tqual.c .satisfies routines still check it - and have to do
so.

It'd be nice to get rid of that, but this patch doesn't seem to get us
nearer towards it :(

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 rid of SnapshotNow

2013-08-05 Thread Bruce Momjian 
On Mon, Aug  5, 2013 at 05:22:28PM +0200, Andres Freund wrote:
 On 2013-08-05 11:17:08 -0400, Bruce Momjian wrote:
  One interesting aspect of this patch is that the backend code is no
  longer even checking HEAP_MOVED_OFF and HEAP_MOVED_IN.  However, we
  can't reuse those bits because they could be set from pre-9.0 rows.
 
 The other tqual.c .satisfies routines still check it - and have to do
 so.
 
 It'd be nice to get rid of that, but this patch doesn't seem to get us
 nearer towards it :(

Oh, sorry, thanks for pointing that out --- somehow I missed it.

-- 
  Bruce Momjian  br...@momjian.ushttp://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 rid of SnapshotNow

2013-07-26 Thread Tom Lane 
Robert Haas robertmh...@gmail.com writes:
 OK.  I've taken care of all remaining uses of SnapshotNow in the code
 base.  I think we can go ahead and remove it, now.  Patch attached.

 (And there was, hopefully, much rejoicing.)

What about SnapshotSelf?

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 rid of SnapshotNow

2013-07-26 Thread Andres Freund 
On 2013-07-26 08:49:38 -0400, Tom Lane wrote:
 Robert Haas robertmh...@gmail.com writes:
  OK.  I've taken care of all remaining uses of SnapshotNow in the code
  base.  I think we can go ahead and remove it, now.  Patch attached.
 
  (And there was, hopefully, much rejoicing.)
 
 What about SnapshotSelf?

I thought about that yesterday and I think we should replace the usages
which aren't easily replaceable (constraint stuff) with an mvcc
snapshot, just one treats our transaction's current CommandId as
visible. That should be doable?

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 rid of SnapshotNow

2013-07-26 Thread Andres Freund 
On 2013-07-25 19:24:53 -0400, Robert Haas wrote:
 (And there was, hopefully, much rejoicing.)

Definitely! Thanks.

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 rid of SnapshotNow

2013-07-26 Thread Robert Haas 
On Fri, Jul 26, 2013 at 8:49 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 OK.  I've taken care of all remaining uses of SnapshotNow in the code
 base.  I think we can go ahead and remove it, now.  Patch attached.

 (And there was, hopefully, much rejoicing.)

 What about SnapshotSelf?

Well, that's still used in _bt_check_unique, unique_key_recheck
(trigger function to do a deferred uniqueness check), RI_FKey_check,
and rather extensively by sepgsql.  I don't really have much desire to
do the work to get rid of it, though.

Getting rid of SnapshotNow is arguably important on the grounds that
third-party code may be using it, and doing this will force them to do
it the new way instead, and that's got some value.  I'm not sure if
anything's already been committed that relies on MVCC catalog access,
but several things have certainly been proposed and it's a good bet
that 9.4 will rely on the catalog access using MVCC-semantics, so
forcing third-party code to stop using SnapshotNow will prevent subtle
bugs.

But there's no similar joy for SnapshotSelf.  You can argue that all
of the things that we're doing with it are crufty, but nobody's
complaining about any of them, and some of them are in places where
the cost of an additional MVCC snapshot on every iteration might be
much more serious than anything we ever saw for catalog scans.  So I'm
personally content to leave it well enough alone.

-- 
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] getting rid of SnapshotNow

2013-07-26 Thread Tom Lane 
Robert Haas robertmh...@gmail.com writes:
 On Fri, Jul 26, 2013 at 8:49 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 What about SnapshotSelf?

 Well, that's still used in _bt_check_unique, unique_key_recheck
 (trigger function to do a deferred uniqueness check), RI_FKey_check,
 and rather extensively by sepgsql.  I don't really have much desire to
 do the work to get rid of it, though.

Hm.  I agree the first three may be all right, but I can't help
suspecting that sepgsql is doing the wrong thing here.

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 rid of SnapshotNow

2013-07-26 Thread Robert Haas 
On Fri, Jul 26, 2013 at 9:18 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Fri, Jul 26, 2013 at 8:49 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 What about SnapshotSelf?

 Well, that's still used in _bt_check_unique, unique_key_recheck
 (trigger function to do a deferred uniqueness check), RI_FKey_check,
 and rather extensively by sepgsql.  I don't really have much desire to
 do the work to get rid of it, though.

 Hm.  I agree the first three may be all right, but I can't help
 suspecting that sepgsql is doing the wrong thing here.

sepgsql is using SnapshotSelf to find the old version of a tuple that
was updated by the core code just before.  That should be safe in the
sense that there can't be a currently-committing transaction somewhere
else that's updated that tuple, if we know that our own uncommitted
transaction has done a transactional update.  There was a recent
thread discussing whether another API might be better, and I'd be
prepared to concede that it might be.  But I don't think it's
drop-dead broken.

Not that I really object if someone wants to have a go at getting rid
of SnapshotSelf, but I think it'd be worth articulating what we hope
to accomplish by so doing.  For example, the btree README says the
following about the deletion algorithm:

---
...  The reason we do it is to
provide an interlock between non-full VACUUM and indexscans.  Since VACUUM
deletes index entries before deleting tuples, the super-exclusive lock
guarantees that VACUUM can't delete any heap tuple that an indexscanning
process might be about to visit.  (This guarantee works only for simple
indexscans that visit the heap in sync with the index scan, not for bitmap
scans.  We only need the guarantee when using non-MVCC snapshot rules; in
an MVCC snapshot, it wouldn't matter if the heap tuple were replaced with
an unrelated tuple at the same TID, because the new tuple wouldn't be
visible to our scan anyway.)
---

Obviously, when we were using SnapshotNow for catalog access, changing
anything here was a non-starter.  But now that we're not, it might be
worth asking whether there are few enough users of non-MVCC rules that
we could apply some suitable treatment to those that remain and then
change the locking protocol here.  And if we did do that, would there
be enough performance benefit to justify the work?  I don't have
answers to those questions, and the answer may well be that we should
leave things as they are, but I think the questions are worth thinking
about.

Aside from any possible advantage in further trimming the list of
available snapshot types, I'd like to spend some time thinking about
what we can do that's safe and useful in terms of reducing lock
levels; or even adding completely new facilities that would have been
DOA in the old world.  I have high hopes in both areas, but I wouldn't
be surprised to find that there are problems we haven't thought about
yet.  I think our dependence on SnapshotNow has wormed itself into our
design choices in deep ways, and I suspect it's going to take a good
deal of thought to figure out exactly what we can improve and how.

-- 
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] getting rid of SnapshotNow

2013-07-26 Thread Tom Lane 
Robert Haas robertmh...@gmail.com writes:
 On Fri, Jul 26, 2013 at 9:18 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 Well, that's still used in _bt_check_unique, unique_key_recheck
 (trigger function to do a deferred uniqueness check), RI_FKey_check,
 and rather extensively by sepgsql.  I don't really have much desire to
 do the work to get rid of it, though.

 Hm.  I agree the first three may be all right, but I can't help
 suspecting that sepgsql is doing the wrong thing here.

 sepgsql is using SnapshotSelf to find the old version of a tuple that
 was updated by the core code just before.

Oh.  OK, then it reduces to the same case as the other three, ie we're
looking at tuples we know to be update-locked.

 [ interesting ruminations snipped ]

Yeah, removing SnapshotNow catalog access certainly opens the doors
for a lot of new thinking.

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 rid of SnapshotNow

2013-07-26 Thread Andres Freund 
On 2013-07-26 09:50:32 -0400, Robert Haas wrote:
 sepgsql is using SnapshotSelf to find the old version of a tuple that
 was updated by the core code just before.  That should be safe in the
 sense that there can't be a currently-committing transaction somewhere
 else that's updated that tuple, if we know that our own uncommitted
 transaction has done a transactional update.  There was a recent
 thread discussing whether another API might be better, and I'd be
 prepared to concede that it might be.  But I don't think it's
 drop-dead broken.

It's safe for the tuples updated in that transaction, but it's not safe
to look at anything else if you expect results without the SnapshotNow
problems. E.g. looking at a newly created attribute is fine, but
iterating over all attributes not necessarily.

I am more concerned about the care needed when placing
CommandCounterIncrement()'s somewhere though. It seems more than likely
that this will get repeatedly broken, even if it's not atm (which I
doubt). E.g. inheritance handling seems to be rather wonky WRT this.

 Not that I really object if someone wants to have a go at getting rid
 of SnapshotSelf, but I think it'd be worth articulating what we hope
 to accomplish by so doing.

Agreed. From the internal usages there doesn't seem to be too much
pressure.

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 rid of SnapshotNow

2013-07-26 Thread Robert Haas 
On Fri, Jul 26, 2013 at 10:16 AM, Andres Freund and...@2ndquadrant.com wrote:
 I am more concerned about the care needed when placing
 CommandCounterIncrement()'s somewhere though. It seems more than likely
 that this will get repeatedly broken, even if it's not atm (which I
 doubt). E.g. inheritance handling seems to be rather wonky WRT this.

There may well be bugs.  I am fine with reviewing patches to improve
the code in this area, but I don't plan to take it upon myself to
rewrite that code.  Either it's working as expected, or nobody's using
it, because we're not getting any bug reports.

 Not that I really object if someone wants to have a go at getting rid
 of SnapshotSelf, but I think it'd be worth articulating what we hope
 to accomplish by so doing.

 Agreed. From the internal usages there doesn't seem to be too much
 pressure.

So unless there are objections to the patch as posted, I'm going to
apply that next week.  This in no way precludes more work in this area
later, but since we're likely to break third-party code with this
change, we might as well get it out of the way as early in the release
cycle as possible.

-- 
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] getting rid of SnapshotNow

2013-07-25 Thread Robert Haas 
On Tue, Jul 23, 2013 at 2:53 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Works for me.

OK.  I've taken care of all remaining uses of SnapshotNow in the code
base.  I think we can go ahead and remove it, now.  Patch attached.

(And there was, hopefully, much rejoicing.)

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


remove-snapshotnow-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] getting rid of SnapshotNow

2013-07-23 Thread Robert Haas 
On Thu, Jul 18, 2013 at 8:46 AM, Robert Haas robertmh...@gmail.com wrote:
 There seems to be a consensus that we should try to get rid of
 SnapshotNow entirely now that we have MVCC catalog scans, so I'm
 attaching two patches that together come close to achieving that goal:

 1. snapshot-error-v1.patch introduces a new special snapshot, called
 SnapshotError.  In the cases where we set SnapshotNow as a sort of
 default snapshot, this patch changes the code to use SnapshotError
 instead.  This affects scan-xs_snapshot in genam.c and
 estate-es_snapshot in execUtils.c.  This passes make check-world, so
 apparently there is no code in the core distribution that does this.
 However, this is safer for third-party code, which will ERROR instead
 of seg faulting.  The alternative approach would be to use
 InvalidSnapshot, which I think would be OK too if people dislike this
 approach.

It seems the consensus was mildly for InvalidSnapshot, so I did it that way.

 2. snapshot-self-not-now-v1.patch changes several uses of SnapshotNow
 to use SnapshotSelf instead.  These include pgrowlocks(),
 pgstat_heap(), and get_actual_variable_range().  In all of those
 cases, only an approximately-correct answer is needed, so the change
 should be fine.  I'd also generally expect that it's very unlikely for
 any of these things to get called in a context where the table being
 scanned has been updated by the current transaction after the most
 recent command-counter increment, so in practice the change in
 semantics will probably not be noticeable at all.

Tom proposed that we use SnapshotDirty for this case; let me just ask
whether there are any security concerns around that.  pgstattuple only
displays aggregate information so I think that's OK, but I wonder if
the value found in get_actual_variable_range() can leak out in EXPLAIN
output or whatever.  I can't particularly think of any reason why that
would actually matter, but I've generally shied away from exposing
data written by uncommitted transactions, and this would be a step in
the other direction.  Does this worry anyone else or am I being
paranoid?

But thinking about it a little more, I wonder why
get_actual_variable_range() is using a snapshot at all.  Presumably
what we want there is to find the last index key, regardless of the
visibility of the heap tuple to which it points.  We don't really need
to consult the heap at all, one would think; the value we need ought
to be present in the index tuple.  If we're going to use a snapshot
for simplicity of coding, maybe the right thing is SnapshotAny.  After
all, even if the index tuples are all dead, we still have to scan
them, so it's still relevant for costing purposes.

Thoughts?

 With that done, the only remaining uses of SnapshotNow in our code
 base will be in currtid_byreloid() and currtid_byrelname().  So far no
 one on this list has been able to understand clearly what the purpose
 of those functions is, so I'm copying this email to pgsql-odbc in case
 someone there can provide more insight.  If I were a betting man, I'd
 bet that they are used in contexts where the difference between
 SnapshotNow and SnapshotSelf wouldn't matter there, either.  For
 example, if those functions are always invoked in a query that does
 nothing but call those functions, the difference wouldn't be visible.
 If we don't want to risk any change to the semantics, we can (1) grit
 our teeth and keep SnapshotNow around or (2) use an instantaneous MVCC
 snapshot there, and accept that people who have very large connection
 counts and extremely heavy use of those functions may see a
 performance regression.

It seems like we're leaning toward a fresh MVCC snapshot for this case.

-- 
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] getting rid of SnapshotNow

2013-07-23 Thread Tom Lane 
Robert Haas robertmh...@gmail.com writes:
 2. snapshot-self-not-now-v1.patch changes several uses of SnapshotNow
 to use SnapshotSelf instead.  These include pgrowlocks(),
 pgstat_heap(), and get_actual_variable_range().

 Tom proposed that we use SnapshotDirty for this case; let me just ask
 whether there are any security concerns around that.  pgstattuple only
 displays aggregate information so I think that's OK, but I wonder if
 the value found in get_actual_variable_range() can leak out in EXPLAIN
 output or whatever.  I can't particularly think of any reason why that
 would actually matter, but I've generally shied away from exposing
 data written by uncommitted transactions, and this would be a step in
 the other direction.  Does this worry anyone else or am I being
 paranoid?

As far as get_actual_variable_range() is concerned, an MVCC snapshot
would probably be the thing to use anyway; I see no need for the planner
to be using estimates that are more up to date than that.  pgrowlocks
and pgstat_heap() might be in a different category.

 But thinking about it a little more, I wonder why
 get_actual_variable_range() is using a snapshot at all.  Presumably
 what we want there is to find the last index key, regardless of the
 visibility of the heap tuple to which it points.

No, what we ideally want is to know the current variable range that
would be seen by the query being planned.

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 rid of SnapshotNow

2013-07-23 Thread Robert Haas 
On Tue, Jul 23, 2013 at 12:28 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 2. snapshot-self-not-now-v1.patch changes several uses of SnapshotNow
 to use SnapshotSelf instead.  These include pgrowlocks(),
 pgstat_heap(), and get_actual_variable_range().

 Tom proposed that we use SnapshotDirty for this case; let me just ask
 whether there are any security concerns around that.  pgstattuple only
 displays aggregate information so I think that's OK, but I wonder if
 the value found in get_actual_variable_range() can leak out in EXPLAIN
 output or whatever.  I can't particularly think of any reason why that
 would actually matter, but I've generally shied away from exposing
 data written by uncommitted transactions, and this would be a step in
 the other direction.  Does this worry anyone else or am I being
 paranoid?

 As far as get_actual_variable_range() is concerned, an MVCC snapshot
 would probably be the thing to use anyway; I see no need for the planner
 to be using estimates that are more up to date than that.  pgrowlocks
 and pgstat_heap() might be in a different category.

 But thinking about it a little more, I wonder why
 get_actual_variable_range() is using a snapshot at all.  Presumably
 what we want there is to find the last index key, regardless of the
 visibility of the heap tuple to which it points.

 No, what we ideally want is to know the current variable range that
 would be seen by the query being planned.

Oh, really?  Well, should we use GetActiveSnapshot() then?

That surprises me, though.  I really thought the point was to cost the
index scan, and surely that will be slowed down even by entries we
can't see.

-- 
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] getting rid of SnapshotNow

2013-07-23 Thread Tom Lane 
Robert Haas robertmh...@gmail.com writes:
 On Tue, Jul 23, 2013 at 12:28 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 As far as get_actual_variable_range() is concerned, an MVCC snapshot
 would probably be the thing to use anyway;

 That surprises me, though.  I really thought the point was to cost the
 index scan, and surely that will be slowed down even by entries we
 can't see.

No, the usage (or the main usage anyway) is for selectivity estimation,
ie how many rows will the query fetch.

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 rid of SnapshotNow

2013-07-23 Thread Robert Haas 
On Tue, Jul 23, 2013 at 2:24 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Tue, Jul 23, 2013 at 12:28 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 As far as get_actual_variable_range() is concerned, an MVCC snapshot
 would probably be the thing to use anyway;

 That surprises me, though.  I really thought the point was to cost the
 index scan, and surely that will be slowed down even by entries we
 can't see.

 No, the usage (or the main usage anyway) is for selectivity estimation,
 ie how many rows will the query fetch.

OK, so GetActiveSnapshot()?

-- 
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] getting rid of SnapshotNow

2013-07-23 Thread Tom Lane 
Robert Haas robertmh...@gmail.com writes:
 OK, so GetActiveSnapshot()?

Works for me.

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 rid of SnapshotNow

2013-07-22 Thread Robert Haas 
On Fri, Jul 19, 2013 at 1:20 PM, Alvaro Herrera
alvhe...@2ndquadrant.com wrote:
 4. If we use GetActiveSnapshot, all the comments about about a fresh
 MVCC snapshot still apply.  However, the snapshot in question could be
 even more stale, especially in repeatable read or serializable mode.
 However, this might be thought a more consistent behavior than what we
 have now.  And I'm guessing that this function is typically run as its
 own transaction, so in practice this doesn't seem much different from
 an MVCC snapshot, only cheaper.

 At the moment, I dislike #2 and slightly prefer #4 to #3.

 +1 for #4, and if we ever need more then we can provide a non-default
 way to get at #2.

OK, done.

-- 
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] getting rid of SnapshotNow

2013-07-19 Thread Robert Haas 
On Fri, Jul 19, 2013 at 1:27 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Noah Misch n...@leadboat.com writes:
 To me, the major advantage of removing SnapshotNow is to force all
 third-party code to reevaluate.  But that could be just as well
 achieved by renaming it to, say, SnapshotImmediate.  If there are
 borderline-legitimate SnapshotNow uses in our code base, I'd lean
 toward a rename instead.  Even if we decide to remove every core use,
 third-party code might legitimately reach a different conclusion on
 similar borderline cases.

 Meh.  If there is third-party code with a legitimate need for
 SnapshotNow, all we'll have done is to create an annoying version
 dependency for them.  So if we think that's actually a likely scenario,
 we shouldn't rename it.  But the entire point of this change IMO is that
 we *don't* think there is a legitimate use-case for SnapshotNow.

 Indeed, I'm thinking I don't believe in SnapshotSelf anymore either.
 It's got all the same consistency issues as SnapshotNow.  In fact, it
 has *more* issues, because it's also vulnerable to weirdnesses caused by
 inconsistent ordering of tuple updates among multiple tuples updated by
 the same command.

 Why not tell people to use SnapshotDirty if they need a
 not-guaranteed-consistent result?  At least then it's pretty obvious
 that you're getting some randomness in with your news.

You know, I didn't really consider that before, but I kind of like it.
 I think that would be entirely suitable (and perhaps better) for
pgstattuple and get_actual_variable_range().

On further reflection, I think perhaps pgrowlocks should just register
a fresh MVCC snapshot and use that.  Using SnapshotDirty would return
TIDs of unseen tuples, which does not seem to be what is wanted there.

-- 
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] getting rid of SnapshotNow

2013-07-19 Thread Andres Freund 
On 2013-07-19 01:27:41 -0400, Tom Lane wrote:
 Noah Misch n...@leadboat.com writes:
  To me, the major advantage of removing SnapshotNow is to force all
  third-party code to reevaluate.  But that could be just as well
  achieved by renaming it to, say, SnapshotImmediate.  If there are
  borderline-legitimate SnapshotNow uses in our code base, I'd lean
  toward a rename instead.  Even if we decide to remove every core use,
  third-party code might legitimately reach a different conclusion on
  similar borderline cases.

I don't think there are many people that aren't active on -hackers that
can actually understand the implications of using SnapshotNow. Given
-hackers hasn't fully grasped them in several cases... And even if those
borderline cases are safe, that's really only valid for a specific
postgres version. Catering to that doesn't seem like a good idea to me.

 Indeed, I'm thinking I don't believe in SnapshotSelf anymore either.
 It's got all the same consistency issues as SnapshotNow.  In fact, it
 has *more* issues, because it's also vulnerable to weirdnesses caused by
 inconsistent ordering of tuple updates among multiple tuples updated by
 the same command.

Hm. I kind of can see the point of it in constraint code where it
probably would be rather hard to remove usage of it, but e.g. the
sepgsql usage looks pretty dubious to me.
At least in the cases where the constraint code uses them I don't think
the SnapshotNow dangers apply since those specific rows should be locked
et al.

The selinux usage looks like a design flaw to me, but I don't really
understand that code, so I very well may be wrong.

 Why not tell people to use SnapshotDirty if they need a
 not-guaranteed-consistent result?  At least then it's pretty obvious
 that you're getting some randomness in with your news.

Especially if we're going to lower the lock level of some commands, but
even now, that opens us to more issues due to nonmatching table
definitions et al. That doesn't sound like a good idea to me.

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 rid of SnapshotNow

2013-07-19 Thread Hiroshi Inoue 

(2013/07/18 21:46), Robert Haas wrote:

There seems to be a consensus that we should try to get rid of
SnapshotNow entirely now that we have MVCC catalog scans, so I'm
attaching two patches that together come close to achieving that goal:


...


With that done, the only remaining uses of SnapshotNow in our code
base will be in currtid_byreloid() and currtid_byrelname().  So far no
one on this list has been able to understand clearly what the purpose
of those functions is, so I'm copying this email to pgsql-odbc in case
someone there can provide more insight.  If I were a betting man, I'd
bet that they are used in contexts where the difference between
SnapshotNow and SnapshotSelf wouldn't matter there, either.


Using SnapshotSelf instead of SnapshotNow for currtid_ () wouldn't
 matter.

regards,
Hiroshi Inoue




--
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 rid of SnapshotNow

2013-07-19 Thread Andres Freund 
On 2013-07-20 00:49:11 +0900, Hiroshi Inoue wrote:
 (2013/07/18 21:46), Robert Haas wrote:
 There seems to be a consensus that we should try to get rid of
 SnapshotNow entirely now that we have MVCC catalog scans, so I'm
 attaching two patches that together come close to achieving that goal:
 
 ...
 
 With that done, the only remaining uses of SnapshotNow in our code
 base will be in currtid_byreloid() and currtid_byrelname().  So far no
 one on this list has been able to understand clearly what the purpose
 of those functions is, so I'm copying this email to pgsql-odbc in case
 someone there can provide more insight.  If I were a betting man, I'd
 bet that they are used in contexts where the difference between
 SnapshotNow and SnapshotSelf wouldn't matter there, either.
 
 Using SnapshotSelf instead of SnapshotNow for currtid_ () wouldn't
  matter.

I think it actually might. You could get into dicey situations if you
use currtid_ in a query performing updates or inserts because it would
see the to-be-inserted tuple...

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 rid of SnapshotNow

2013-07-19 Thread Alvaro Herrera 
Robert Haas escribió:
 On Fri, Jul 19, 2013 at 1:27 AM, Tom Lane t...@sss.pgh.pa.us wrote:

  Why not tell people to use SnapshotDirty if they need a
  not-guaranteed-consistent result?  At least then it's pretty obvious
  that you're getting some randomness in with your news.

 On further reflection, I think perhaps pgrowlocks should just register
 a fresh MVCC snapshot and use that.  Using SnapshotDirty would return
 TIDs of unseen tuples, which does not seem to be what is wanted there.

I think seeing otherwise invisible rows is useful in pgrowlocks.  It
helps observe the effects on tuples written by concurrent transactions
during experimentation.  But then, maybe this functionality belongs in
pageinspect instead.

-- 
Álvaro Herrerahttp://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 rid of SnapshotNow

2013-07-19 Thread Alvaro Herrera 
Robert Haas escribió:
 On Fri, Jul 19, 2013 at 12:29 PM, Alvaro Herrera
 alvhe...@2ndquadrant.com wrote:

  I think seeing otherwise invisible rows is useful in pgrowlocks.  It
  helps observe the effects on tuples written by concurrent transactions
  during experimentation.  But then, maybe this functionality belongs in
  pageinspect instead.
 
 It does seem like it should be useful, at least as an option.  But I
 feel like changing that ought to be separate from getting rid of
 SnapshotNow.  It seems like too big of a behavior change to pass off
 as a harmless tweak.

Agreed.

-- 
Álvaro Herrerahttp://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 rid of SnapshotNow

2013-07-19 Thread Robert Haas 
On Fri, Jul 19, 2013 at 12:29 PM, Alvaro Herrera
alvhe...@2ndquadrant.com wrote:
 Robert Haas escribió:
 On Fri, Jul 19, 2013 at 1:27 AM, Tom Lane t...@sss.pgh.pa.us wrote:

  Why not tell people to use SnapshotDirty if they need a
  not-guaranteed-consistent result?  At least then it's pretty obvious
  that you're getting some randomness in with your news.

 On further reflection, I think perhaps pgrowlocks should just register
 a fresh MVCC snapshot and use that.  Using SnapshotDirty would return
 TIDs of unseen tuples, which does not seem to be what is wanted there.

 I think seeing otherwise invisible rows is useful in pgrowlocks.  It
 helps observe the effects on tuples written by concurrent transactions
 during experimentation.  But then, maybe this functionality belongs in
 pageinspect instead.

It does seem like it should be useful, at least as an option.  But I
feel like changing that ought to be separate from getting rid of
SnapshotNow.  It seems like too big of a behavior change to pass off
as a harmless tweak.

-- 
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] getting rid of SnapshotNow

2013-07-19 Thread Robert Haas 
On Fri, Jul 19, 2013 at 12:51 PM, Alvaro Herrera
alvhe...@2ndquadrant.com wrote:
 Robert Haas escribió:
 On Fri, Jul 19, 2013 at 12:29 PM, Alvaro Herrera
 alvhe...@2ndquadrant.com wrote:

  I think seeing otherwise invisible rows is useful in pgrowlocks.  It
  helps observe the effects on tuples written by concurrent transactions
  during experimentation.  But then, maybe this functionality belongs in
  pageinspect instead.

 It does seem like it should be useful, at least as an option.  But I
 feel like changing that ought to be separate from getting rid of
 SnapshotNow.  It seems like too big of a behavior change to pass off
 as a harmless tweak.

 Agreed.

So any change we make to pgrowlocks is going to have some behavior consequences.

1. If we use SnapshotSelf, then nobody will notice the difference
unless this is used as part of a query that locks or modifies tuples
in the table being examined.  But in that case you might see the
results of the current query.  Thus, I think this is the smallest
possible behavior change, but Tom doesn't like SnapshotSelf any more
than he likes SnapshotNow.

2. If we use SnapshotDirty, then the difference is probably
noticeable, because you'll see the results of concurrent, uncommitted
transactions.  Maybe useful, but probably shouldn't be the new
default.

3. If we use a fresh MVCC snapshot, then when you scan a table you'll
see the state of play as of the beginning of your scan rather than the
state of play as of when your scan reaches the target page.  This
might be noticeable on a large table.  However, it might also be
thought an improvement.

4. If we use GetActiveSnapshot, all the comments about about a fresh
MVCC snapshot still apply.  However, the snapshot in question could be
even more stale, especially in repeatable read or serializable mode.
However, this might be thought a more consistent behavior than what we
have now.  And I'm guessing that this function is typically run as its
own transaction, so in practice this doesn't seem much different from
an MVCC snapshot, only cheaper.

At the moment, I dislike #2 and slightly prefer #4 to #3.

-- 
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] getting rid of SnapshotNow

2013-07-19 Thread Alvaro Herrera 
Robert Haas escribió:

 4. If we use GetActiveSnapshot, all the comments about about a fresh
 MVCC snapshot still apply.  However, the snapshot in question could be
 even more stale, especially in repeatable read or serializable mode.
 However, this might be thought a more consistent behavior than what we
 have now.  And I'm guessing that this function is typically run as its
 own transaction, so in practice this doesn't seem much different from
 an MVCC snapshot, only cheaper.
 
 At the moment, I dislike #2 and slightly prefer #4 to #3.

+1 for #4, and if we ever need more then we can provide a non-default
way to get at #2.

-- 
Álvaro Herrerahttp://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 rid of SnapshotNow

2013-07-18 Thread Noah Misch 
On Thu, Jul 18, 2013 at 08:46:48AM -0400, Robert Haas wrote:
 1. snapshot-error-v1.patch introduces a new special snapshot, called
 SnapshotError.  In the cases where we set SnapshotNow as a sort of
 default snapshot, this patch changes the code to use SnapshotError
 instead.  This affects scan-xs_snapshot in genam.c and
 estate-es_snapshot in execUtils.c.  This passes make check-world, so
 apparently there is no code in the core distribution that does this.
 However, this is safer for third-party code, which will ERROR instead
 of seg faulting.  The alternative approach would be to use
 InvalidSnapshot, which I think would be OK too if people dislike this
 approach.

I don't have a strong opinion.  Anything it diagnoses is a code bug, probably
one that makes the affected extension useless until it's fixed.  But the patch
is small and self-contained.  I think the benefit, more than making things
safer in production, would be reducing the amount of time the developer needs
to zero in on the problem.  It wouldn't be the first time we've done that;
compare AtEOXact_Buffers().  Does this particular class of bug deserve that
aid?  I don't know.

 2. snapshot-self-not-now-v1.patch changes several uses of SnapshotNow
 to use SnapshotSelf instead.  These include pgrowlocks(),
 pgstat_heap(), and get_actual_variable_range().  In all of those
 cases, only an approximately-correct answer is needed, so the change
 should be fine.  I'd also generally expect that it's very unlikely for
 any of these things to get called in a context where the table being
 scanned has been updated by the current transaction after the most
 recent command-counter increment, so in practice the change in
 semantics will probably not be noticeable at all.

SnapshotSelf is awfully special; currently, you can grep for all uses of it
and find a collection of callers with highly-technical needs.  Diluting that
with a handful of callers that legitimately preferred SnapshotNow but don't
care enough to mind SnapshotSelf in its place brings a minor loss of clarity.

From an accuracy perspective, GetActiveSnapshot() does seem ideal for
get_actual_variable_range().  That's independent of any hurry to remove
SnapshotNow.  A possible disadvantage is that older snapshots could waste time
scanning back through newer index entries, when a more-accessible value would
be good enough for estimation purposes.

To me, the major advantage of removing SnapshotNow is to force all third-party
code to reevaluate.  But that could be just as well achieved by renaming it
to, say, SnapshotImmediate.  If there are borderline-legitimate SnapshotNow
uses in our code base, I'd lean toward a rename instead.  Even if we decide to
remove every core use, third-party code might legitimately reach a different
conclusion on similar borderline cases.

Thanks,
nm

-- 
Noah Misch
EnterpriseDB http://www.enterprisedb.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 rid of SnapshotNow

2013-07-18 Thread Tom Lane 
Noah Misch n...@leadboat.com writes:
 To me, the major advantage of removing SnapshotNow is to force all
 third-party code to reevaluate.  But that could be just as well
 achieved by renaming it to, say, SnapshotImmediate.  If there are
 borderline-legitimate SnapshotNow uses in our code base, I'd lean
 toward a rename instead.  Even if we decide to remove every core use,
 third-party code might legitimately reach a different conclusion on
 similar borderline cases.

Meh.  If there is third-party code with a legitimate need for
SnapshotNow, all we'll have done is to create an annoying version
dependency for them.  So if we think that's actually a likely scenario,
we shouldn't rename it.  But the entire point of this change IMO is that
we *don't* think there is a legitimate use-case for SnapshotNow.

Indeed, I'm thinking I don't believe in SnapshotSelf anymore either.
It's got all the same consistency issues as SnapshotNow.  In fact, it
has *more* issues, because it's also vulnerable to weirdnesses caused by
inconsistent ordering of tuple updates among multiple tuples updated by
the same command.

Why not tell people to use SnapshotDirty if they need a
not-guaranteed-consistent result?  At least then it's pretty obvious
that you're getting some randomness in with your news.

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