getting rid of SnapshotNow

Robert Haas <robertmhaas@gmail.com> writes:

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.

FWIW, I think using InvalidSnapshot would be preferable to introducing
a new concept for what's pretty much the same thing.

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.

I had the idea they were used for a client-side implementation of WHERE
CURRENT OF. Perhaps that's dead code and could be removed entirely?

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.

Of those I'd go for (2); it's unlikely that an extra snapshot would be
visible compared to the query roundtrip overhead. But another attractive
possibility is to make these functions use GetActiveSnapshot(), which
would avoid taking any new snapshot.

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

On Thu, Jul 18, 2013 at 10:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

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.

FWIW, I think using InvalidSnapshot would be preferable to introducing
a new concept for what's pretty much the same thing.

Andres voted the other way on the previous thread. I'll wait and see
if there are any other opinions. The SnapshotError concept seemed
attractive to me initially, but I'm not as excited about it after
seeing how it turned out, so maybe it's best to do it as you suggest.

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.

I had the idea they were used for a client-side implementation of WHERE
CURRENT OF. Perhaps that's dead code and could be removed entirely?

It's been reported that ODBC still uses them.

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.

Of those I'd go for (2); it's unlikely that an extra snapshot would be
visible compared to the query roundtrip overhead. But another attractive
possibility is to make these functions use GetActiveSnapshot(), which
would avoid taking any new snapshot.

You could probably construct a case where the overhead is visible, if
you ran the functions many times in a single query, but arguably no
one does that. Also, Andres's test case that involves running BEGIN;
SELECT txid_current(); very short sleep; COMMIT; in several hundred
sessions at once is pretty brutal on PGXACT and makes the overhead of
taking extra snapshots a lot more visible.

I'm not too familiar with GetActiveSnapshot(), but wouldn't that
change the user-visible semantics? If, for example, someone's using
that function to test whether the row has been updated since their
snapshot was taken, that use case would get broken. SnapshotSelf
would be change from the current behavior in many fewer cases than
using an older snapshot.

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

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

Robert Haas escribi�:

On Thu, Jul 18, 2013 at 10:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

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.

FWIW, I think using InvalidSnapshot would be preferable to introducing
a new concept for what's pretty much the same thing.

Andres voted the other way on the previous thread. I'll wait and see
if there are any other opinions. The SnapshotError concept seemed
attractive to me initially, but I'm not as excited about it after
seeing how it turned out, so maybe it's best to do it as you suggest.

Yeah ... SnapshotError is a way to ensure the server doesn't crash if an
extension hasn't been fixed in order not to cause a crash if it doesn't
use the APIs correctly. However, there's many other ways for a
C-language extension to cause crashes, so I don't think this is buying
us much.

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.

I had the idea they were used for a client-side implementation of WHERE
CURRENT OF. Perhaps that's dead code and could be removed entirely?

It's been reported that ODBC still uses them.

They don't show up in a quick grep of psqlodbc's source code, FWIW.

--
�lvaro Herrera 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

On Thu, Jul 18, 2013 at 11:54 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

They don't show up in a quick grep of psqlodbc's source code, FWIW.

Hmm. Maybe we should just remove them and see if anyone complains.
We could always put them back (or make them available via contrib) if
it's functionality someone actually needs. The last discussion of
those functions was in 2007 and nobody seemed too sure back then
either, so maybe the rumor that anyone is actually using this is no
more than rumor.

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

On 2013-07-18 12:01:39 -0400, Robert Haas wrote:

On Thu, Jul 18, 2013 at 11:54 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

They don't show up in a quick grep of psqlodbc's source code, FWIW.

Hmm. Maybe we should just remove them and see if anyone complains.
We could always put them back (or make them available via contrib) if
it's functionality someone actually needs. The last discussion of
those functions was in 2007 and nobody seemed too sure back then
either, so maybe the rumor that anyone is actually using this is no
more than rumor.

I am pretty sure they are still used. A quick grep on a not too old
checkout prooves that... Note that the sql accessible functions are
named currtid and currtid2 (yes, really)...

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

Andres Freund escribi�:

On 2013-07-18 12:01:39 -0400, Robert Haas wrote:

On Thu, Jul 18, 2013 at 11:54 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

They don't show up in a quick grep of psqlodbc's source code, FWIW.

Hmm. Maybe we should just remove them and see if anyone complains.
We could always put them back (or make them available via contrib) if
it's functionality someone actually needs. The last discussion of
those functions was in 2007 and nobody seemed too sure back then
either, so maybe the rumor that anyone is actually using this is no
more than rumor.

I am pretty sure they are still used. A quick grep on a not too old
checkout prooves that... Note that the sql accessible functions are
named currtid and currtid2 (yes, really)...

Ah, yeah, that does show up. I had grepped for 'currtid_'. Sorry.
They're all in positioned_load() in results.c.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

On Thu, Jul 18, 2013 at 12:25 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Ah, yeah, that does show up. I had grepped for 'currtid_'. Sorry.
They're all in positioned_load() in results.c.

Well, in that case, we'll have to keep it around. I still wish we
could get a clear answer to the question of how it's being used.

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

(2013/07/18 23:54), Robert Haas wrote:

On Thu, Jul 18, 2013 at 10:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

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.

FWIW, I think using InvalidSnapshot would be preferable to introducing
a new concept for what's pretty much the same thing.

Andres voted the other way on the previous thread. I'll wait and see
if there are any other opinions. The SnapshotError concept seemed
attractive to me initially, but I'm not as excited about it after
seeing how it turned out, so maybe it's best to do it as you suggest.

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.

I had the idea they were used for a client-side implementation of WHERE
CURRENT OF. Perhaps that's dead code and could be removed entirely?

It's been reported that ODBC still uses them.

Though PostgreSQL's TID is similar to Orale's ROWID, it is transient
and changed after update operations unfortunately. I implemented
the currtid_xx functions to supplement the difference. For example

currtid(relname, original tid)

(hopefully) returns the current tid of the original row when it is
updated.

regards,
Hiroshi Inoue

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

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-odbc mailing list (pgsql-odbc@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-odbc

Noah Misch <noah@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-odbc mailing list (pgsql-odbc@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-odbc

On Fri, Jul 19, 2013 at 1:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Noah Misch <noah@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

On 2013-07-19 08:57:01 +0900, Inoue, Hiroshi wrote:

I had the idea they were used for a client-side implementation of WHERE
CURRENT OF. Perhaps that's dead code and could be removed entirely?

It's been reported that ODBC still uses them.

Though PostgreSQL's TID is similar to Orale's ROWID, it is transient
and changed after update operations unfortunately. I implemented
the currtid_xx functions to supplement the difference. For example

currtid(relname, original tid)

(hopefully) returns the current tid of the original row when it is
updated.

That is only guaranteed to work though when you're in a transaction old
enough to prevent removal of the old or intermediate row versions. E.g.
BEGIN;
INSERT INTO foo...; -- last tid (0, 1)
COMMIT;
BEGIN;
SELECT currtid(foo, '(0, 1'));
COMMIT;

can basically return no or even an arbitrarily different row. Same with
an update...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

On 2013-07-19 01:27:41 -0400, Tom Lane wrote:

Noah Misch <noah@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-odbc mailing list (pgsql-odbc@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-odbc

(2013/07/19 22:03), Andres Freund wrote:

On 2013-07-19 08:57:01 +0900, Inoue, Hiroshi wrote:

I had the idea they were used for a client-side implementation of WHERE
CURRENT OF. Perhaps that's dead code and could be removed entirely?

It's been reported that ODBC still uses them.

Though PostgreSQL's TID is similar to Orale's ROWID, it is transient
and changed after update operations unfortunately. I implemented
the currtid_xx functions to supplement the difference. For example

currtid(relname, original tid)

(hopefully) returns the current tid of the original row when it is
updated.

That is only guaranteed to work though when you're in a transaction old
enough to prevent removal of the old or intermediate row versions. E.g.

Yes it's what I meant by (hopefully).
At the time when I implemented currtid(), I was able to use TIDs in
combination with OIDs.

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

(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

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-odbc mailing list (pgsql-odbc@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-odbc

Andres Freund <andres@2ndquadrant.com> writes:

On 2013-07-20 00:49:11 +0900, Hiroshi Inoue wrote:

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

I'm pretty sure Hiroshi-san was only opining about whether it would
matter for ODBC's usage. IIUC, ODBC is using this function to re-fetch
rows that it inserted, updated, or at least selected-for-update in a
previous command of the current transaction, so actually any snapshot
would do fine.

In any case, since I moved the goalposts by suggesting that SnapshotSelf
is just as dangerous as SnapshotNow, what we need to know is whether
it'd be all right to change this code to use a fresh MVCC snapshot;
and if not, why not. It's pretty hard to see a reason why client-side
code would want to make use of the results of a non-MVCC snapshot.

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

Robert Haas escribi�:

On Fri, Jul 19, 2013 at 1:27 AM, Tom Lane <tgl@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 Herrera 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

On Fri, Jul 19, 2013 at 12:29 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Robert Haas escribió:

On Fri, Jul 19, 2013 at 1:27 AM, Tom Lane <tgl@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

Robert Haas escribi�:

On Fri, Jul 19, 2013 at 12:29 PM, Alvaro Herrera
<alvherre@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 Herrera 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

On Fri, Jul 19, 2013 at 12:51 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Robert Haas escribió:

On Fri, Jul 19, 2013 at 12:29 PM, Alvaro Herrera
<alvherre@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-odbc mailing list (pgsql-odbc@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-odbc

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 Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

(2013/07/20 1:11), Tom Lane wrote:

Andres Freund <andres@2ndquadrant.com> writes:

On 2013-07-20 00:49:11 +0900, Hiroshi Inoue wrote:

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

I'm pretty sure Hiroshi-san was only opining about whether it would
matter for ODBC's usage. IIUC, ODBC is using this function to re-fetch
rows that it inserted, updated, or at least selected-for-update in a
previous command of the current transaction, so actually any snapshot
would do fine.

In any case, since I moved the goalposts by suggesting that SnapshotSelf
is just as dangerous as SnapshotNow, what we need to know is whether
it'd be all right to change this code to use a fresh MVCC snapshot;
and if not, why not. It's pretty hard to see a reason why client-side
code would want to make use of the results of a non-MVCC snapshot.

OK I agree to replace SnapshotNow for currtid_xx() by a MVCC-snapshot.

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

On Fri, Jul 19, 2013 at 1:20 PM, Alvaro Herrera
<alvherre@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-odbc mailing list (pgsql-odbc@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-odbc

On Thu, Jul 18, 2013 at 8:46 AM, Robert Haas <robertmhaas@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

Robert Haas <robertmhaas@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

On Tue, Jul 23, 2013 at 12:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@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

Robert Haas <robertmhaas@gmail.com> writes:

On Tue, Jul 23, 2013 at 12:28 PM, Tom Lane <tgl@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

On Tue, Jul 23, 2013 at 2:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Tue, Jul 23, 2013 at 12:28 PM, Tom Lane <tgl@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

Robert Haas <robertmhaas@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

On Tue, Jul 23, 2013 at 2:53 PM, Tom Lane <tgl@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

Robert Haas <robertmhaas@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

On 2013-07-26 08:49:38 -0400, Tom Lane wrote:

Robert Haas <robertmhaas@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

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

On Fri, Jul 26, 2013 at 8:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@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

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Jul 26, 2013 at 8:49 AM, Tom Lane <tgl@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

On Fri, Jul 26, 2013 at 9:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Jul 26, 2013 at 8:49 AM, Tom Lane <tgl@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

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Jul 26, 2013 at 9:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@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

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

On Fri, Jul 26, 2013 at 10:16 AM, Andres Freund <andres@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

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

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

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