Re: [HACKERS] snapshot too old, configured by time

2016-05-02 Thread Bruce Momjian
On Mon, May 2, 2016 at 03:50:36PM -0500, Kevin Grittner wrote: > >> Also, it seems we have similar behavior already in applying WAL on the > >> standby --- we delay WAL replay when there is a long-running > >> transaction. Once the time expires, we apply the WAL. Do we cancel the > >> long-runni

Re: [HACKERS] snapshot too old, configured by time

2016-05-02 Thread Kevin Grittner
On Sun, May 1, 2016 at 11:54 PM, Bruce Momjian wrote: > On Sat, Apr 23, 2016 at 10:20:19AM -0400, Bruce Momjian wrote: >> On Sat, Apr 23, 2016 at 12:48:08PM +0530, Amit Kapila wrote: >>> On Sat, Apr 23, 2016 at 8:34 AM, Bruce Momjian wrote: I kind of agreed with Tom about just aborting

Re: [HACKERS] snapshot too old, configured by time

2016-05-01 Thread Bruce Momjian
On Sat, Apr 23, 2016 at 10:20:19AM -0400, Bruce Momjian wrote: > On Sat, Apr 23, 2016 at 12:48:08PM +0530, Amit Kapila wrote: > > On Sat, Apr 23, 2016 at 8:34 AM, Bruce Momjian wrote: > > > > > > I kind of agreed with Tom about just aborting transactions that held > > > snapshots for too long, and

Re: [HACKERS] snapshot too old, configured by time

2016-04-25 Thread Alexander Korotkov
On Sat, Apr 23, 2016 at 5:20 PM, Bruce Momjian wrote: > On Sat, Apr 23, 2016 at 12:48:08PM +0530, Amit Kapila wrote: > > On Sat, Apr 23, 2016 at 8:34 AM, Bruce Momjian wrote: > > > > > > I kind of agreed with Tom about just aborting transactions that held > > > snapshots for too long, and liked

Re: [HACKERS] snapshot too old, configured by time

2016-04-23 Thread Bruce Momjian
On Sat, Apr 23, 2016 at 12:48:08PM +0530, Amit Kapila wrote: > On Sat, Apr 23, 2016 at 8:34 AM, Bruce Momjian wrote: > > > > I kind of agreed with Tom about just aborting transactions that held > > snapshots for too long, and liked the idea this could be set per > > session, but the idea that we a

Re: [HACKERS] snapshot too old, configured by time

2016-04-23 Thread Amit Kapila
On Sat, Apr 23, 2016 at 8:34 AM, Bruce Momjian wrote: > > On Tue, Apr 19, 2016 at 07:38:04AM -0400, Robert Haas wrote: > > 2. Without this feature, you can kill sessions or transactions to > > control bloat, but this feature is properly thought of as a way to > > avoid bloat *without* killing sess

Re: [HACKERS] snapshot too old, configured by time

2016-04-22 Thread Bruce Momjian
On Tue, Apr 19, 2016 at 07:38:04AM -0400, Robert Haas wrote: > 2. Without this feature, you can kill sessions or transactions to > control bloat, but this feature is properly thought of as a way to > avoid bloat *without* killing sessions or transactions. You can let > the session live, without ha

Re: [HACKERS] snapshot too old, configured by time

2016-04-21 Thread Kevin Grittner
On Wed, Apr 20, 2016 at 8:50 AM, Kevin Grittner wrote: > In case anyone notices some code left at the bottom of bufmgr.h > related to inline functions, that was left on purpose, because I am > pretty sure that the fix for the performance regression observed > when the "snapshot too old" feature i

Re: [HACKERS] snapshot too old, configured by time

2016-04-20 Thread Kevin Grittner
On Tue, Apr 19, 2016 at 12:49 PM, Alvaro Herrera wrote: > Well, it seems I'm outvoted. The no-op change to attempt to force an explicit choice of whether to test for snapshot age after calling BufferGetPage() has been reverted. This eliminates about 500 back-patching pain points in 65 files. I

Re: [HACKERS] snapshot too old, configured by time

2016-04-19 Thread Alvaro Herrera
Kevin Grittner wrote: > On Tue, Apr 19, 2016 at 11:02 AM, Alvaro Herrera > wrote: > > Robert Haas wrote: > > > >> That wouldn't have fixed my problem, which involved rebasing a patch. > > > > True. I note that it's possible to munge a patch mechanically to sort > > out this situation. > > I adm

Re: [HACKERS] snapshot too old, configured by time

2016-04-19 Thread Kevin Grittner
On Tue, Apr 19, 2016 at 11:02 AM, Alvaro Herrera wrote: > Andres Freund wrote: >> On 2016-04-19 12:03:22 -0300, Alvaro Herrera wrote: > Since this change to BufferGetPage() has caused severe back-patch pain for at least two committers so far, I will revert that (very recent) change

Re: [HACKERS] snapshot too old, configured by time

2016-04-19 Thread Alvaro Herrera
Andres Freund wrote: > On 2016-04-19 12:03:22 -0300, Alvaro Herrera wrote: > > > Since this change to BufferGetPage() has caused severe back-patch > > > pain for at least two committers so far, I will revert that (very > > > recent) change to this patch later today unless I hear an > > > objection

Re: [HACKERS] snapshot too old, configured by time

2016-04-19 Thread Robert Haas
On Tue, Apr 19, 2016 at 11:03 AM, Alvaro Herrera wrote: > Kevin Grittner wrote: >> On Tue, Apr 19, 2016 at 6:38 AM, Robert Haas wrote: >> >> > The right thing to do about that is just change it back to the >> > way Kevin had it originally. >> >> Since this change to BufferGetPage() has caused sev

Re: [HACKERS] snapshot too old, configured by time

2016-04-19 Thread Andres Freund
On 2016-04-19 12:03:22 -0300, Alvaro Herrera wrote: > Kevin Grittner wrote: > > On Tue, Apr 19, 2016 at 6:38 AM, Robert Haas wrote: > > > > > The right thing to do about that is just change it back to the > > > way Kevin had it originally. > > > > Since this change to BufferGetPage() has caused

Re: [HACKERS] snapshot too old, configured by time

2016-04-19 Thread Alvaro Herrera
Kevin Grittner wrote: > On Tue, Apr 19, 2016 at 6:38 AM, Robert Haas wrote: > > > The right thing to do about that is just change it back to the > > way Kevin had it originally. > > Since this change to BufferGetPage() has caused severe back-patch > pain for at least two committers so far, I wil

Re: [HACKERS] snapshot too old, configured by time

2016-04-19 Thread Kevin Grittner
On Tue, Apr 19, 2016 at 6:38 AM, Robert Haas wrote: > The right thing to do about that is just change it back to the > way Kevin had it originally. Since this change to BufferGetPage() has caused severe back-patch pain for at least two committers so far, I will revert that (very recent) change t

Re: [HACKERS] snapshot too old, configured by time

2016-04-19 Thread Robert Haas
On Tue, Apr 19, 2016 at 1:23 AM, Michael Paquier wrote: > On Tue, Apr 19, 2016 at 3:14 AM, Tom Lane wrote: >> Or in short: this is a whole lot further than I'm prepared to go to >> satisfy one customer with a badly-designed application. And from what >> I can tell from the Feb 2015 discussion, t

Re: [HACKERS] snapshot too old, configured by time

2016-04-18 Thread Michael Paquier
On Tue, Apr 19, 2016 at 3:14 AM, Tom Lane wrote: > Or in short: this is a whole lot further than I'm prepared to go to > satisfy one customer with a badly-designed application. And from what > I can tell from the Feb 2015 discussion, that's what this has been > written for. This holds true. I im

Re: [HACKERS] snapshot too old, configured by time

2016-04-18 Thread Tom Lane
Kevin Grittner writes: > On Mon, Apr 18, 2016 at 8:50 AM, Tom Lane wrote: >> Surely there was another way to get a similar end result without >> mucking with things at the level of BufferGetPage. > To get the feature that some customers have been demanding, a check > has to be made somewhere nea

Re: [HACKERS] snapshot too old, configured by time

2016-04-18 Thread Kevin Grittner
On Mon, Apr 18, 2016 at 8:50 AM, Tom Lane wrote: > Surely there was another way to get a similar end result without > mucking with things at the level of BufferGetPage. To get the feature that some customers have been demanding, a check has to be made somewhere near where any page is read in a s

Re: [HACKERS] snapshot too old, configured by time

2016-04-18 Thread Alvaro Herrera
Tom Lane wrote: > Alvaro Herrera writes: > > I disagree. A developer that sees an unadorned BufferGetPage() call > > doesn't stop to think twice about whether they need to add a snapshot > > test. Many reviewers will miss the necessary addition also. A > > developer that sees BufferGetPage(NO_S

Re: [HACKERS] snapshot too old, configured by time

2016-04-18 Thread Kevin Grittner
On Mon, Apr 18, 2016 at 8:41 AM, Tom Lane wrote: > Kevin Grittner writes: >> On Sun, Apr 17, 2016 at 10:38 PM, Alvaro Herrera >> wrote: >>> I understand the backpatching pain argument, but my opinion was the >>> contrary of yours even so. > >> The other possibility would be to backpatch the no-o

Re: [HACKERS] snapshot too old, configured by time

2016-04-18 Thread Robert Haas
On Sun, Apr 17, 2016 at 6:15 PM, Tom Lane wrote: > After struggling with back-patching a GIN bug fix, I wish to offer up the > considered opinion that this was an impressively bad idea. It's inserted > 450 or so pain points for back-patching, which we will have to deal with > for the next five ye

Re: [HACKERS] snapshot too old, configured by time

2016-04-18 Thread Tom Lane
Alvaro Herrera writes: > I disagree. A developer that sees an unadorned BufferGetPage() call > doesn't stop to think twice about whether they need to add a snapshot > test. Many reviewers will miss the necessary addition also. A > developer that sees BufferGetPage(NO_SNAPSHOT_TEST) will at leas

Re: [HACKERS] snapshot too old, configured by time

2016-04-18 Thread Tom Lane
Kevin Grittner writes: > On Sun, Apr 17, 2016 at 10:38 PM, Alvaro Herrera > wrote: >> I understand the backpatching pain argument, but my opinion was the >> contrary of yours even so. > The other possibility would be to backpatch the no-op patch which > just uses the new syntax without any chang

Re: [HACKERS] snapshot too old, configured by time

2016-04-18 Thread Kevin Grittner
On Sun, Apr 17, 2016 at 10:38 PM, Alvaro Herrera wrote: > Tom Lane wrote: > >> After struggling with back-patching a GIN bug fix, I wish to offer up the >> considered opinion that this was an impressively bad idea. It's inserted >> 450 or so pain points for back-patching, which we will have to de

Re: [HACKERS] snapshot too old, configured by time

2016-04-17 Thread Alvaro Herrera
Tom Lane wrote: > After struggling with back-patching a GIN bug fix, I wish to offer up the > considered opinion that this was an impressively bad idea. It's inserted > 450 or so pain points for back-patching, which we will have to deal with > for the next five years. Moreover, I do not believe

Re: [HACKERS] snapshot too old, configured by time

2016-04-17 Thread Michael Paquier
On Mon, Apr 18, 2016 at 9:52 AM, Kevin Grittner wrote: > On Sun, Apr 17, 2016 at 5:15 PM, Tom Lane wrote: >> Kevin Grittner writes: >>> On Wed, Mar 30, 2016 at 3:26 PM, Alvaro Herrera> >>> wrote: Kevin Grittner wrote: >> I think we should revert BufferGetPage to be what it was before (wit

Re: [HACKERS] snapshot too old, configured by time

2016-04-17 Thread Kevin Grittner
On Sun, Apr 17, 2016 at 5:15 PM, Tom Lane wrote: > Kevin Grittner writes: >> On Wed, Mar 30, 2016 at 3:26 PM, Alvaro Herrera> >> wrote: >>> Kevin Grittner wrote: On Wed, Mar 30, 2016 at 2:22 PM, Alvaro Herrera wrote: I said that we should change BufferGetPage into having the sn

Re: [HACKERS] snapshot too old, configured by time

2016-04-17 Thread Tom Lane
Kevin Grittner writes: > On Wed, Mar 30, 2016 at 3:26 PM, Alvaro Herrera > wrote: >> Kevin Grittner wrote: >>> On Wed, Mar 30, 2016 at 2:22 PM, Alvaro Herrera >>> wrote: >>> I said that we should change BufferGetPage into having the snapshot >>> check built-in, except in the cases where a flag

Re: [HACKERS] snapshot too old, configured by time

2016-04-08 Thread David Steele
On 4/8/16 4:30 PM, Peter Geoghegan wrote: > On Fri, Apr 8, 2016 at 12:55 PM, Kevin Grittner wrote: >> Sadly, I forgot to include the reviewer information when writing the >> commit messages. :-( > > Oh well. I'm just glad we got the patch over the line. I think that > there are some types of use

Re: [HACKERS] snapshot too old, configured by time

2016-04-08 Thread Peter Geoghegan
On Fri, Apr 8, 2016 at 12:55 PM, Kevin Grittner wrote: > Sadly, I forgot to include the reviewer information when writing the > commit messages. :-( Oh well. I'm just glad we got the patch over the line. I think that there are some types of users that will very significantly benefit from this pa

Re: [HACKERS] snapshot too old, configured by time

2016-04-08 Thread Kevin Grittner
On Thu, Apr 7, 2016 at 6:12 PM, Peter Geoghegan wrote: > I think that there is a good argument in favor of this patch that you > may have failed to make yourself, which is: it limits bloat in a way > that's analogous to how RecentGlobalDataXmin can do so for logical > decoding (i.e. where wal_lev

Re: [HACKERS] snapshot too old, configured by time

2016-04-07 Thread Peter Geoghegan
On Thu, Apr 7, 2016 at 3:56 PM, Kevin Grittner wrote: >> Unfortunately, this does stop recycling from >> happening early for B-Tree pages, even though that's probably safe in >> principle. This is probably not so bad -- it just needs to be >> considered when reviewing this patch (the same is true

Re: [HACKERS] snapshot too old, configured by time

2016-04-07 Thread Kevin Grittner
On Mon, Apr 4, 2016 at 9:15 PM, Peter Geoghegan wrote: > The patch does this: > >> --- a/src/backend/access/heap/pruneheap.c >> +++ b/src/backend/access/heap/pruneheap.c >> @@ -92,12 +92,21 @@ heap_page_prune_opt(Relation relation, Buffer buffer) >> * need to use the horizon that includes sl

Re: [HACKERS] snapshot too old, configured by time

2016-04-07 Thread Kevin Grittner
On Sun, Apr 3, 2016 at 4:09 PM, Jeff Janes wrote: > On Wed, Mar 30, 2016 at 12:34 PM, Kevin Grittner wrote: >> On Sat, Mar 19, 2016 at 1:27 AM, Jeff Janes wrote: >>> I set the value to 1min. >>> >>> I set up a test like this: >>> >>> pgbench -i >>> >>> pgbench -c4 -j4 -T 3600 & >>> >>> ### watc

Re: [HACKERS] snapshot too old, configured by time

2016-04-07 Thread Jeff Janes
On Mon, Apr 4, 2016 at 8:38 PM, Peter Geoghegan wrote: > On Sun, Apr 3, 2016 at 2:09 PM, Jeff Janes wrote: >> Also, HOT-cleanup should stop the bloat increase once the snapshot >> crosses the old_snapshot_threshold without even needing to wait until >> the next autovac runs. >> >> Does the code i

Re: [HACKERS] snapshot too old, configured by time

2016-04-04 Thread Peter Geoghegan
On Sun, Apr 3, 2016 at 2:09 PM, Jeff Janes wrote: > Also, HOT-cleanup should stop the bloat increase once the snapshot > crosses the old_snapshot_threshold without even needing to wait until > the next autovac runs. > > Does the code intentionally only work for manual vacuums? If so, that > seems

Re: [HACKERS] snapshot too old, configured by time

2016-04-04 Thread Peter Geoghegan
On Wed, Mar 30, 2016 at 12:46 PM, Kevin Grittner wrote: > On Wed, Mar 30, 2016 at 2:29 PM, Peter Geoghegan wrote: > >> [Does the patch allow dangling page pointers?] > >> Again, I don't want to prejudice anyone against your patch, which I >> haven't read. > > I don't believe that the way the patc

Re: [HACKERS] snapshot too old, configured by time

2016-04-03 Thread Jeff Janes
On Wed, Mar 30, 2016 at 12:34 PM, Kevin Grittner wrote: > On Sat, Mar 19, 2016 at 1:27 AM, Jeff Janes wrote: > >> I'm not sure if this is operating as expected. >> >> I set the value to 1min. >> >> I set up a test like this: >> >> pgbench -i >> >> pgbench -c4 -j4 -T 3600 & >> >> ### watch the siz

Re: [HACKERS] snapshot too old, configured by time

2016-04-02 Thread Kevin Grittner
On Sat, Apr 2, 2016 at 7:12 AM, Michael Paquier wrote: > On Fri, Apr 1, 2016 at 11:45 PM, Alvaro Herrera > wrote: >> Kevin Grittner wrote: >> >>> Attached is what I think you're talking about for the first patch. >>> AFAICS this should generate identical executable code to unpatched. >>> Then th

Re: [HACKERS] snapshot too old, configured by time

2016-04-02 Thread Michael Paquier
On Fri, Apr 1, 2016 at 11:45 PM, Alvaro Herrera wrote: > Kevin Grittner wrote: > >> Attached is what I think you're talking about for the first patch. >> AFAICS this should generate identical executable code to unpatched. >> Then the patch to actually implement the feature would, instead >> of add

Re: [HACKERS] snapshot too old, configured by time

2016-04-01 Thread Alvaro Herrera
Kevin Grittner wrote: > Attached is what I think you're talking about for the first patch. > AFAICS this should generate identical executable code to unpatched. > Then the patch to actually implement the feature would, instead > of adding 30-some lines with TestForOldSnapshot() would implement > t

Re: [HACKERS] snapshot too old, configured by time

2016-03-31 Thread Kevin Grittner
On Wed, Mar 30, 2016 at 9:19 PM, Alvaro Herrera wrote: > Michael Paquier wrote: > >> Just a note: I began looking at the tests, but finished looking at the >> patch entirely at the end by curiosity. Regarding the integration of >> this patch for 9.6, I think that bumping that to 9.7 would be wiser

Re: [HACKERS] snapshot too old, configured by time

2016-03-30 Thread Alvaro Herrera
Michael Paquier wrote: > Just a note: I began looking at the tests, but finished looking at the > patch entirely at the end by curiosity. Regarding the integration of > this patch for 9.6, I think that bumping that to 9.7 would be wiser > because the patch needs to be re-written largely, and that'

Re: [HACKERS] snapshot too old, configured by time

2016-03-30 Thread Michael Paquier
On Thu, Mar 31, 2016 at 5:09 AM, Kevin Grittner wrote: > On Wed, Mar 30, 2016 at 2:22 PM, Alvaro Herrera > wrote: >> I understand the invasiveness argument, but to me the danger of >> introducing new bugs trumps that. The problem is not the current code, >> but future patches: it is just too eas

Re: [HACKERS] snapshot too old, configured by time

2016-03-30 Thread Kevin Grittner
On Thu, Mar 24, 2016 at 2:24 AM, Michael Paquier wrote: > I have been looking at 4a, the test module, and things are looking > good IMO. Something that I think would be adapted would be to define > the options for isolation tests in a variable, like ISOLATION_OPTS to > allow MSVC scripts to fetch

Re: [HACKERS] snapshot too old, configured by time

2016-03-30 Thread Alvaro Herrera
Kevin Grittner wrote: > On Wed, Mar 30, 2016 at 2:22 PM, Alvaro Herrera > wrote: > > I said that we should change BufferGetPage into having the snapshot > > check built-in, except in the cases where a flag is passed; and the flag > > would be passed in all cases except those 30-something you iden

Re: [HACKERS] snapshot too old, configured by time

2016-03-30 Thread Kevin Grittner
On Wed, Mar 30, 2016 at 2:22 PM, Alvaro Herrera wrote: > I understand the invasiveness argument, but to me the danger of > introducing new bugs trumps that. The problem is not the current code, > but future patches: it is just too easy to make the mistake of not > checking the snapshot in new ad

Re: [HACKERS] snapshot too old, configured by time

2016-03-30 Thread Kevin Grittner
On Wed, Mar 30, 2016 at 2:34 PM, Kevin Grittner wrote: > A connection should not get the > error just because it is using a snapshot that tries to look at > data that might be wrong, and the connection holding the long-lived > snapshot doesn't do that until it awakes from the sleep and runs > the

Re: [HACKERS] snapshot too old, configured by time

2016-03-30 Thread Kevin Grittner
On Wed, Mar 30, 2016 at 2:29 PM, Peter Geoghegan wrote: > [Does the patch allow dangling page pointers?] > Again, I don't want to prejudice anyone against your patch, which I > haven't read. I don't believe that the way the patch does its business opens any new vulnerabilities of this type. If

Re: [HACKERS] snapshot too old, configured by time

2016-03-30 Thread Kevin Grittner
On Sat, Mar 19, 2016 at 1:27 AM, Jeff Janes wrote: > I'm not sure if this is operating as expected. > > I set the value to 1min. > > I set up a test like this: > > pgbench -i > > pgbench -c4 -j4 -T 3600 & > > ### watch the size of branches table > while (true) ; do psql -c "\dt+" | fgrep _branche

Re: [HACKERS] snapshot too old, configured by time

2016-03-30 Thread Peter Geoghegan
On Wed, Mar 30, 2016 at 12:21 PM, Peter Geoghegan wrote: > Well, amcheck is a tool that in essence makes sure that B-Trees look > structurally sound, and respect invariants like having every item on > each page in logical order. That could catch a bug of the kind I just > described, because it's q

Re: [HACKERS] snapshot too old, configured by time

2016-03-30 Thread Alvaro Herrera
Kevin Grittner wrote: > On Wed, Mar 30, 2016 at 11:37 AM, Tom Lane wrote: > > Alvaro Herrera writes: > >> I think a safer proposition would be to replace all current > >> BufferGetPage() calls (there are about 500) by adding the necessary > >> arguments: buffer, snapshot, rel, and an integer "fla

Re: [HACKERS] snapshot too old, configured by time

2016-03-30 Thread Peter Geoghegan
On Wed, Mar 30, 2016 at 11:53 AM, Kevin Grittner wrote: > When the initial "proof of concept" patch was tested by the > customer, it was not effective due to issues related to what you > raise. Autovacuum workers were blocking due to the page pins for > scans using these old snapshots, causing th

Re: [HACKERS] snapshot too old, configured by time

2016-03-30 Thread Kevin Grittner
On Sun, Mar 20, 2016 at 6:25 PM, Peter Geoghegan wrote: > I haven't read the patch, but I wonder: What are the implications here > for B-Tree page recycling by VACUUM? > Offhand, I imagine that there'd be some special considerations. Why is > it okay that an index scan could land on a deleted pa

Re: [HACKERS] snapshot too old, configured by time

2016-03-30 Thread Kevin Grittner
On Wed, Mar 30, 2016 at 11:37 AM, Tom Lane wrote: > Alvaro Herrera writes: >> I think a safer proposition would be to replace all current >> BufferGetPage() calls (there are about 500) by adding the necessary >> arguments: buffer, snapshot, rel, and an integer "flags". All this >> without adding

Re: [HACKERS] snapshot too old, configured by time

2016-03-30 Thread Tom Lane
Alvaro Herrera writes: > I think a safer proposition would be to replace all current > BufferGetPage() calls (there are about 500) by adding the necessary > arguments: buffer, snapshot, rel, and an integer "flags". All this > without adding the feature. Then a subsequent commit would add the > T

Re: [HACKERS] snapshot too old, configured by time

2016-03-30 Thread Alvaro Herrera
Michael Paquier wrote: > page = BufferGetPage(buf); > + TestForOldSnapshot(scan->xs_snapshot, rel, page); > This is a sequence repeated many times in this patch, a new routine, > say BufferGetPageExtended with a uint8 flag, one flag being used to > test old snapshots would be more adapted. B

Re: [HACKERS] snapshot too old, configured by time

2016-03-30 Thread Kevin Grittner
On Tue, Mar 29, 2016 at 8:58 AM, David Steele wrote: > We're getting to the end of the CF now. Do you know when you'll have an > updated patch ready? I am working on it right now. Hopefully I can get it all sorted today. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise Postg

Re: [HACKERS] snapshot too old, configured by time

2016-03-29 Thread David Steele
Hi Kevin, On 3/21/16 4:05 PM, Kevin Grittner wrote: Thanks to all for the feedback; I will try to respond later this week. First I'm trying to get my reviews for other patches posted. We're getting to the end of the CF now. Do you know when you'll have an updated patch ready? Thanks, --

Re: [HACKERS] snapshot too old, configured by time

2016-03-24 Thread Michael Paquier
On Tue, Mar 22, 2016 at 5:05 AM, Kevin Grittner wrote: > Thanks to all for the feedback; I will try to respond later this > week. First I'm trying to get my reviews for other patches posted. I have been looking at 4a, the test module, and things are looking good IMO. Something that I think would

Re: [HACKERS] snapshot too old, configured by time

2016-03-21 Thread Kevin Grittner
Thanks to all for the feedback; I will try to respond later this week. First I'm trying to get my reviews for other patches posted. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make

Re: [HACKERS] snapshot too old, configured by time

2016-03-21 Thread Thom Brown
On 17 March 2016 at 21:15, Kevin Grittner wrote: > New patch just to merge in recent commits -- it was starting to > show some bit-rot. Tests folded in with main patch. In session 1, I've run: # begin transaction isolation level repeatable read ; BEGIN *# declare stuff scroll cursor for select

Re: [HACKERS] snapshot too old, configured by time

2016-03-20 Thread Peter Geoghegan
On Sun, Mar 20, 2016 at 4:25 PM, Peter Geoghegan wrote: > I worry that something weird could happen there. For example, perhaps > the page LSN on what is actually a newly recycled page could be set > such that the backend following a stale right spuriously raises a > "snapshot too old" error. I m

Re: [HACKERS] snapshot too old, configured by time

2016-03-20 Thread Peter Geoghegan
On Thu, Mar 17, 2016 at 2:15 PM, Kevin Grittner wrote: > New patch just to merge in recent commits -- it was starting to > show some bit-rot. Tests folded in with main patch. I haven't read the patch, but I wonder: What are the implications here for B-Tree page recycling by VACUUM? I know that y

Re: [HACKERS] snapshot too old, configured by time

2016-03-19 Thread Kevin Grittner
New patch just to merge in recent commits -- it was starting to show some bit-rot. Tests folded in with main patch. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company snapshot-too-old-v5.patch Description: invalid/octet-stream -- Sent via pgsql-hackers maili

Re: [HACKERS] snapshot too old, configured by time

2016-03-18 Thread Jeff Janes
On Thu, Mar 17, 2016 at 2:15 PM, Kevin Grittner wrote: > New patch just to merge in recent commits -- it was starting to > show some bit-rot. Tests folded in with main patch. I'm not sure if this is operating as expected. I set the value to 1min. I set up a test like this: pgbench -i pgbench

Re: [HACKERS] snapshot too old, configured by time

2016-03-11 Thread Michael Paquier
On Fri, Mar 11, 2016 at 2:35 PM, Robert Haas wrote: > On Thu, Mar 3, 2016 at 2:40 PM, Kevin Grittner wrote: >> Thanks for the tips. Attached is a minimal set of isolation tests. >> I can expand on it if needed, but wanted: >> >> (1) to confirm that this is the right way to do this, and >> >> (2)

Re: [HACKERS] snapshot too old, configured by time

2016-03-11 Thread Robert Haas
On Thu, Mar 3, 2016 at 2:40 PM, Kevin Grittner wrote: > Thanks for the tips. Attached is a minimal set of isolation tests. > I can expand on it if needed, but wanted: > > (1) to confirm that this is the right way to do this, and > > (2) how long people were willing to tolerate these tests running

Re: [HACKERS] snapshot too old, configured by time

2016-03-03 Thread Kevin Grittner
On Tue, Mar 1, 2016 at 12:58 AM, Michael Paquier wrote: > On Tue, Mar 1, 2016 at 9:35 AM, Andres Freund wrote: >> On 2016-02-29 18:30:27 -0600, Kevin Grittner wrote: >>> Basically, a connection needs to remain open and interleave >>> commands with other connections, which the isolation tester doe

Re: [HACKERS] snapshot too old, configured by time

2016-02-29 Thread Michael Paquier
On Tue, Mar 1, 2016 at 9:35 AM, Andres Freund wrote: > On 2016-02-29 18:30:27 -0600, Kevin Grittner wrote: >> Basically, a connection needs to remain open and interleave >> commands with other connections, which the isolation tester does >> just fine; but it needs to do that using a custom postgre

Re: [HACKERS] snapshot too old, configured by time

2016-02-29 Thread Andres Freund
On 2016-02-29 18:30:27 -0600, Kevin Grittner wrote: > Basically, a connection needs to remain open and interleave > commands with other connections, which the isolation tester does > just fine; but it needs to do that using a custom postgresql.conf > file, which TAP does just fine. I haven't been

Re: [HACKERS] snapshot too old, configured by time

2016-02-29 Thread Kevin Grittner
On Fri, Jan 8, 2016 at 5:22 PM, Alvaro Herrera wrote: >> People have said that issuing SQL commands directly from a TAP test >> via DBD::Pg is not acceptable for a core feature, and (despite >> assertions to the contrary) I see no way to test this feature with >> existing testing mechanisms. The

Re: [HACKERS] snapshot too old, configured by time

2016-02-01 Thread Alvaro Herrera
Kevin Grittner wrote: > > There has been a review but no replies for more than 1 month. Returned > > with feedback? > > I do intend to post another version of the patch to tweak the > calculations again, after I can get a patch in to expand the > testing capabilities to allow an acceptable way to

Re: [HACKERS] snapshot too old, configured by time

2016-01-08 Thread Alvaro Herrera
Kevin Grittner wrote: > People have said that issuing SQL commands directly from a TAP test > via DBD::Pg is not acceptable for a core feature, and (despite > assertions to the contrary) I see no way to test this feature with > existing testing mechanisms. The bigger set of work here, if we > don

Re: [HACKERS] snapshot too old, configured by time

2015-12-03 Thread Andres Freund
On 2015-12-02 14:48:24 -0600, Kevin Grittner wrote: > On Wed, Dec 2, 2015 at 12:39 AM, Michael Paquier > wrote: > > On Mon, Nov 9, 2015 at 8:07 AM, Steve Singer wrote: > > >> In snapmgr.c > >> > >> > >> + * XXX: If we can trust a read of an int64 value to be atomic, we can > >> skip the > >> +

Re: [HACKERS] snapshot too old, configured by time

2015-12-02 Thread Michael Paquier
On Thu, Dec 3, 2015 at 5:48 AM, Kevin Grittner wrote: >> There has been a review but no replies for more than 1 month. Returned >> with feedback? > > I do intend to post another version of the patch to tweak the > calculations again, after I can get a patch in to expand the > testing capabilities t

Re: [HACKERS] snapshot too old, configured by time

2015-12-02 Thread Kevin Grittner
On Wed, Dec 2, 2015 at 12:39 AM, Michael Paquier wrote: > On Mon, Nov 9, 2015 at 8:07 AM, Steve Singer wrote: >> In snapmgr.c >> >> >> + * XXX: If we can trust a read of an int64 value to be atomic, we can skip >> the >> + * spinlock here. >> + */ >> +int64 >> +GetOldSnapshotThresholdTimestamp(

Re: [HACKERS] snapshot too old, configured by time

2015-12-01 Thread Michael Paquier
On Mon, Nov 9, 2015 at 8:07 AM, Steve Singer wrote: > On 10/15/2015 05:47 PM, Kevin Grittner wrote: >> >> All other issues raised by Álvaro and Steve have been addressed, except >> for this one, which I will argue against: > > > I've been looking through the updated patch > > > In snapmgr.c > > >

Re: [HACKERS] snapshot too old, configured by time

2015-11-08 Thread Steve Singer
On 10/15/2015 05:47 PM, Kevin Grittner wrote: All other issues raised by Álvaro and Steve have been addressed, except for this one, which I will argue against: I've been looking through the updated patch In snapmgr.c + * XXX: If we can trust a read of an int64 value to be atomic, we can s

Re: [HACKERS] snapshot too old, configured by time

2015-10-15 Thread Kevin Grittner
On Tuesday, September 15, 2015 12:07 PM, Alvaro Herrera wrote: > Kevin Grittner wrote: >> Alvaro Herrera wrote: >>> I would place it inside src/test/modules, so that buildfarm >>> runs it automatically; I'm not sure it will pick up things in >>> src/test. >> >> As it stands now, the test is get

Re: [HACKERS] snapshot too old, configured by time

2015-09-15 Thread Alvaro Herrera
Kevin Grittner wrote: > Alvaro Herrera wrote: > > I would place it inside src/test/modules, so that buildfarm > > runs it automatically; I'm not sure it will pick up things in > > src/test. > > As it stands now, the test is getting run as part of `make > check-world`, and it seems like src/test

Re: [HACKERS] snapshot too old, configured by time

2015-09-13 Thread Kevin Grittner
Thanks to both Steve and Álvaro for review and comments. I won't be able to address all of those within the time frame of the current CF, so I have moved it to the next CF and flagged it as "Waiting on Author". I will post a patch to address all comments before that CF starts. I will discuss now

Re: [HACKERS] snapshot too old, configured by time

2015-09-11 Thread Alvaro Herrera
I'm starting to read through this and have a few questions. Kevin Grittner wrote: > 4. Tests have been added. They are currently somewhat minimal, > since this is using a whole new technique for testing from any > existing committed tests -- I wanted to make sure that this > approach to testing

Re: [HACKERS] snapshot too old, configured by time

2015-09-08 Thread Steve Singer
On 08/31/2015 10:07 AM, Kevin Grittner wrote: Kevin, I've started to do a review on this patch but I am a bit confused with some of what I am seeing. The attached testcase fails I replace the cursor in your test case with direct selects from the table. I would have expected this to generat