Thread: getting rid of SnapshotNow
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. 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. Barring objections, I'll commit both of these next week. 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
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
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
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
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
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
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
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
(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
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
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
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
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
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
(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
(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
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
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
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
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
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
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
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
(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
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