Re: [HACKERS] Reviewing freeze map code

On Thu, Aug 4, 2016 at 3:24 AM, Andres Freund wrote: > Hi, > > On 2016-08-02 10:55:18 -0400, Noah Misch wrote: >> [Action required within 72 hours. This is a generic notification.] >> >> The above-described topic is currently a PostgreSQL 9.6 open item. Andres, >> since you

Re: [HACKERS] Reviewing freeze map code

Hi, On 2016-08-02 10:55:18 -0400, Noah Misch wrote: > [Action required within 72 hours. This is a generic notification.] > > The above-described topic is currently a PostgreSQL 9.6 open item. Andres, > since you committed the patch believed to have created it, you own this open > item. Well

Re: [HACKERS] Reviewing freeze map code

On Tue, Aug 02, 2016 at 02:10:29PM +0530, Amit Kapila wrote: > On Tue, Aug 2, 2016 at 11:19 AM, Noah Misch wrote: > > On Sat, Jul 23, 2016 at 01:25:55PM +0530, Amit Kapila wrote: > >> On Mon, Jul 18, 2016 at 2:03 PM, Andres Freund wrote: > >> > On

Re: [HACKERS] Reviewing freeze map code

On Tue, Aug 2, 2016 at 11:19 AM, Noah Misch wrote: > On Sat, Jul 23, 2016 at 01:25:55PM +0530, Amit Kapila wrote: >> On Mon, Jul 18, 2016 at 2:03 PM, Andres Freund wrote: >> > On 2016-07-18 10:02:52 +0530, Amit Kapila wrote: >> >> Consider the below

Re: [HACKERS] Reviewing freeze map code

On Sat, Jul 23, 2016 at 01:25:55PM +0530, Amit Kapila wrote: > On Mon, Jul 18, 2016 at 2:03 PM, Andres Freund wrote: > > On 2016-07-18 10:02:52 +0530, Amit Kapila wrote: > >> Consider the below scenario. > >> > >> Vacuum > >> a. acquires a cleanup lock for page - 10 > >> b.

Re: [HACKERS] Reviewing freeze map code

On Wed, Jul 27, 2016 at 3:24 AM, Robert Haas wrote: > On Sat, Jul 23, 2016 at 3:55 AM, Amit Kapila wrote: >> Attached patch fixes the problem for me. Note, I have not tried to >> reproduce the problem for heap_lock_updated_tuple_rec(), but I think

Re: [HACKERS] Reviewing freeze map code

On Sat, Jul 23, 2016 at 3:55 AM, Amit Kapila wrote: > Attached patch fixes the problem for me. Note, I have not tried to > reproduce the problem for heap_lock_updated_tuple_rec(), but I think > if you are convinced with above cases, then we should have a similar > check

Re: [HACKERS] Reviewing freeze map code

On Mon, Jul 18, 2016 at 2:03 PM, Andres Freund wrote: > On 2016-07-18 10:02:52 +0530, Amit Kapila wrote: >> > >> >> Consider the below scenario. >> >> Vacuum >> a. acquires a cleanup lock for page - 10 >> b. busy in checking visibility of tuples >> --assume, here it takes some

Re: [HACKERS] Reviewing freeze map code

On Sat, Jul 16, 2016 at 10:08 AM, Andres Freund wrote: > On 2016-07-14 20:53:07 -0700, Andres Freund wrote: >> On 2016-07-13 23:06:07 -0700, Andres Freund wrote: >> > won't enter the branch, because HEAP_XMAX_LOCK_ONLY won't be set. Which >> > will leave t_ctid and

Re: [HACKERS] Reviewing freeze map code

On 2016-07-18 01:33:10 -0700, Andres Freund wrote: > On 2016-07-18 10:02:52 +0530, Amit Kapila wrote: > > On Mon, Jul 18, 2016 at 9:13 AM, Andres Freund wrote: > > > On 2016-07-18 09:07:19 +0530, Amit Kapila wrote: > > >> + /* > > >> + * Before locking the buffer, pin the

Re: [HACKERS] Reviewing freeze map code

On 2016-07-18 10:02:52 +0530, Amit Kapila wrote: > On Mon, Jul 18, 2016 at 9:13 AM, Andres Freund wrote: > > On 2016-07-18 09:07:19 +0530, Amit Kapila wrote: > >> + /* > >> + * Before locking the buffer, pin the visibility map page if it may be > >> + * necessary. > >> + */ >

Re: [HACKERS] Reviewing freeze map code

On Mon, Jul 18, 2016 at 9:13 AM, Andres Freund wrote: > On 2016-07-18 09:07:19 +0530, Amit Kapila wrote: >> + /* >> + * Before locking the buffer, pin the visibility map page if it may be >> + * necessary. >> + */ >> >> + if (PageIsAllVisible(BufferGetPage(*buffer))) >> +

Re: [HACKERS] Reviewing freeze map code

On 2016-07-18 09:07:19 +0530, Amit Kapila wrote: > + /* > + * Before locking the buffer, pin the visibility map page if it may be > + * necessary. > + */ > > + if (PageIsAllVisible(BufferGetPage(*buffer))) > + visibilitymap_pin(relation, block, ); > + > LockBuffer(*buffer,

Re: [HACKERS] Reviewing freeze map code

On 2016-07-17 23:34:01 -0400, Robert Haas wrote: > Thanks very much for working on this. Random suggestions after a quick look: > > + * Before locking the buffer, pin the visibility map page if it may be > + * necessary. > > s/necessary/needed/ > > More substantively, what happens if

Re: [HACKERS] Reviewing freeze map code

On Mon, Jul 18, 2016 at 8:18 AM, Andres Freund wrote: > On 2016-07-16 10:45:26 -0700, Andres Freund wrote: >> >> >> On July 16, 2016 8:49:06 AM PDT, Tom Lane wrote: >> >Amit Kapila writes: >> >> On Sat, Jul 16, 2016 at 7:02 AM,

Re: [HACKERS] Reviewing freeze map code

On Sun, Jul 17, 2016 at 10:48 PM, Andres Freund wrote: > Took till today. Attached is a rather heavily revised version of > Sawada-san's patch. Most notably the recovery routines take care to > reset the vm in all cases, we don't perform visibilitymap_get_status > from inside

Re: [HACKERS] Reviewing freeze map code

On 2016-07-16 10:45:26 -0700, Andres Freund wrote: > > > On July 16, 2016 8:49:06 AM PDT, Tom Lane wrote: > >Amit Kapila writes: > >> On Sat, Jul 16, 2016 at 7:02 AM, Andres Freund > >wrote: > >>> I think we have two choices how

Re: [HACKERS] Reviewing freeze map code

On July 16, 2016 8:49:06 AM PDT, Tom Lane wrote: >Amit Kapila writes: >> On Sat, Jul 16, 2016 at 7:02 AM, Andres Freund >wrote: >>> I think we have two choices how to deal with that: First, we can add >a >>> new flags variable

Re: [HACKERS] Reviewing freeze map code

Amit Kapila writes: > On Sat, Jul 16, 2016 at 7:02 AM, Andres Freund wrote: >> I think we have two choices how to deal with that: First, we can add a >> new flags variable to xl_heap_lock similar to >> xl_heap_insert/update/... and bump page magic, >

Re: [HACKERS] Reviewing freeze map code

On Sat, Jul 16, 2016 at 7:02 AM, Andres Freund wrote: > On 2016-07-13 23:06:07 -0700, Andres Freund wrote: >> > + /* Clear only the all-frozen bit on visibility map if needed */ >> > + if (PageIsAllVisible(BufferGetPage(buffer)) && >> > +

Re: [HACKERS] Reviewing freeze map code

On 2016-07-13 23:06:07 -0700, Andres Freund wrote: > > + /* Clear only the all-frozen bit on visibility map if needed */ > > + if (PageIsAllVisible(BufferGetPage(buffer)) && > > + VM_ALL_FROZEN(relation, block, )) > > + { > > +

Re: [HACKERS] Reviewing freeze map code

On 2016-07-14 20:53:07 -0700, Andres Freund wrote: > On 2016-07-13 23:06:07 -0700, Andres Freund wrote: > > won't enter the branch, because HEAP_XMAX_LOCK_ONLY won't be set. Which > > will leave t_ctid and HEAP_HOT_UPDATED set differently on the master and > > standby / after crash recovery.

Re: [HACKERS] Reviewing freeze map code

On 2016-07-13 23:06:07 -0700, Andres Freund wrote: > won't enter the branch, because HEAP_XMAX_LOCK_ONLY won't be set. Which > will leave t_ctid and HEAP_HOT_UPDATED set differently on the master and > standby / after crash recovery. I'm failing to see any harmful > consequences right now, but

Re: [HACKERS] Reviewing freeze map code

On 2016-07-14 18:12:42 +0530, Amit Kapila wrote: > Just thinking out loud. If we set HEAP_XMAX_LOCK_ONLY during update, > then won't it impact the return value of > HeapTupleHeaderIsOnlyLocked(). It will start returning true whereas > otherwise I think it would have returned false due to

Re: [HACKERS] Reviewing freeze map code

On Thu, Jul 14, 2016 at 11:36 AM, Andres Freund wrote: > Hi, > > Master does > /* temporarily make it look not-updated */ > oldtup.t_data->t_ctid = oldtup.t_self; > here, and as is the wal record won't reflect that, because: > static void >

Re: [HACKERS] Reviewing freeze map code

Hi, So I'm generally happy with 0001, baring some relatively minor adjustments. I am however wondering about one thing: On 2016-07-11 23:51:05 +0900, Masahiko Sawada wrote: > diff --git a/src/backend/access/heap/heapam.c > b/src/backend/access/heap/heapam.c > index 57da57a..e7cb8ca 100644 > ---

Re: [HACKERS] Reviewing freeze map code

On Fri, Jul 8, 2016 at 10:24 PM, Amit Kapila wrote: > On Thu, Jul 7, 2016 at 12:34 PM, Masahiko Sawada > wrote: >> Than you for reviewing! >> >> On Thu, Jul 7, 2016 at 7:06 AM, Andres Freund wrote: >>> On 2016-07-05 23:37:59

Re: [HACKERS] Reviewing freeze map code

On Thu, Jul 7, 2016 at 12:34 PM, Masahiko Sawada wrote: > Than you for reviewing! > > On Thu, Jul 7, 2016 at 7:06 AM, Andres Freund wrote: >> On 2016-07-05 23:37:59 +0900, Masahiko Sawada wrote: >>> diff --git a/src/backend/access/heap/heapam.c >>>

Re: [HACKERS] Reviewing freeze map code

On Thu, Jul 7, 2016 at 2:04 PM, Andres Freund wrote: >> Yeah, that's true, but I'm having a bit of trouble imagining exactly >> we end up with corruption that actually matters. I guess a torn page >> could do it. > > I think Noah pointed out a bad scenario: If we crash after

Re: [HACKERS] Reviewing freeze map code

On 2016-07-07 14:01:05 -0400, Robert Haas wrote: > On Thu, Jul 7, 2016 at 1:58 PM, Andres Freund wrote: > > On 2016-07-07 10:37:15 -0400, Robert Haas wrote: > >> On Wed, Jul 6, 2016 at 6:06 PM, Andres Freund wrote: > >> > Hm. We can't easily do that in the

Re: [HACKERS] Reviewing freeze map code

On Thu, Jul 7, 2016 at 1:58 PM, Andres Freund wrote: > On 2016-07-07 10:37:15 -0400, Robert Haas wrote: >> On Wed, Jul 6, 2016 at 6:06 PM, Andres Freund wrote: >> > Hm. We can't easily do that in the back-patched version; because a >> > standby won't know

Re: [HACKERS] Reviewing freeze map code

On 2016-07-07 10:37:15 -0400, Robert Haas wrote: > On Wed, Jul 6, 2016 at 6:06 PM, Andres Freund wrote: > > Hm. We can't easily do that in the back-patched version; because a > > standby won't know to check for the flag . That's kinda ok, since we > > don't yet need to clear

Re: [HACKERS] Reviewing freeze map code

On Thu, Jul 7, 2016 at 10:53 AM, Alvaro Herrera wrote: > Robert Haas wrote: >> On Wed, Jul 6, 2016 at 6:06 PM, Andres Freund wrote: >> > Hm. We can't easily do that in the back-patched version; because a >> > standby won't know to check for the flag

Re: [HACKERS] Reviewing freeze map code

Robert Haas wrote: > On Wed, Jul 6, 2016 at 6:06 PM, Andres Freund wrote: > > Hm. We can't easily do that in the back-patched version; because a > > standby won't know to check for the flag . That's kinda ok, since we > > don't yet need to clear all-visible yet at that point

Re: [HACKERS] Reviewing freeze map code

On Wed, Jul 6, 2016 at 6:06 PM, Andres Freund wrote: > Hm. We can't easily do that in the back-patched version; because a > standby won't know to check for the flag . That's kinda ok, since we > don't yet need to clear all-visible yet at that point of > heap_update. But that

Re: [HACKERS] Reviewing freeze map code

Than you for reviewing! On Thu, Jul 7, 2016 at 7:06 AM, Andres Freund wrote: > On 2016-07-05 23:37:59 +0900, Masahiko Sawada wrote: >> diff --git a/src/backend/access/heap/heapam.c >> b/src/backend/access/heap/heapam.c >> index 57da57a..fd66527 100644 >> ---

Re: [HACKERS] Reviewing freeze map code

On Thu, Jul 7, 2016 at 3:36 AM, Andres Freund wrote: > On 2016-07-05 23:37:59 +0900, Masahiko Sawada wrote: > >> @@ -8694,6 +8761,23 @@ heap_xlog_lock(XLogReaderState *record) >> } >> HeapTupleHeaderSetXmax(htup, xlrec->locking_xid); >>

Re: [HACKERS] Reviewing freeze map code

On 2016-07-05 23:37:59 +0900, Masahiko Sawada wrote: > diff --git a/src/backend/access/heap/heapam.c > b/src/backend/access/heap/heapam.c > index 57da57a..fd66527 100644 > --- a/src/backend/access/heap/heapam.c > +++ b/src/backend/access/heap/heapam.c > @@ -3923,6 +3923,17 @@ l2: > > if

Re: [HACKERS] Reviewing freeze map code

On Mon, Jul 4, 2016 at 5:44 PM, Masahiko Sawada wrote: > On Sat, Jul 2, 2016 at 12:34 PM, Amit Kapila wrote: >> On Fri, Jul 1, 2016 at 7:52 PM, Masahiko Sawada >> wrote: >>> On Fri, Jul 1, 2016 at 11:12 AM, Amit Kapila

Re: [HACKERS] Reviewing freeze map code

On Mon, Jul 4, 2016 at 2:31 PM, Masahiko Sawada wrote: > On Sat, Jul 2, 2016 at 12:17 PM, Amit Kapila wrote: >> On Sat, Jul 2, 2016 at 12:53 AM, Andres Freund wrote: >>> On 2016-07-01 15:18:39 -0400, Robert Haas wrote: >>>

Re: [HACKERS] Reviewing freeze map code

On Sat, Jul 2, 2016 at 12:17 PM, Amit Kapila wrote: > On Sat, Jul 2, 2016 at 12:53 AM, Andres Freund wrote: >> On 2016-07-01 15:18:39 -0400, Robert Haas wrote: >>> On Fri, Jul 1, 2016 at 10:22 AM, Masahiko Sawada >>> wrote:

Re: [HACKERS] Reviewing freeze map code

On Sat, Jul 2, 2016 at 12:34 PM, Amit Kapila wrote: > On Fri, Jul 1, 2016 at 7:52 PM, Masahiko Sawada wrote: >> On Fri, Jul 1, 2016 at 11:12 AM, Amit Kapila wrote: >>> >>> Why do you think IndexOnlyScan will return wrong

Re: [HACKERS] Reviewing freeze map code

On Fri, Jul 1, 2016 at 7:52 PM, Masahiko Sawada wrote: > On Fri, Jul 1, 2016 at 11:12 AM, Amit Kapila wrote: >> >> Why do you think IndexOnlyScan will return wrong result? If the >> server crash in the way as you described, the transaction that

Re: [HACKERS] Reviewing freeze map code

On Sat, Jul 2, 2016 at 12:53 AM, Andres Freund wrote: > On 2016-07-01 15:18:39 -0400, Robert Haas wrote: >> On Fri, Jul 1, 2016 at 10:22 AM, Masahiko Sawada >> wrote: >> > Ah, you're right, I misunderstood. >> > >> > Attached updated patch

Re: [HACKERS] Reviewing freeze map code

On 7/1/16 3:43 PM, Andres Freund wrote: On 2016-07-01 15:42:22 -0500, Jim Nasby wrote: On 7/1/16 2:23 PM, Andres Freund wrote: The only cost of that is that vacuum will come along and mark the page all-visible again instead of skipping it, but that's probably not an enormous expense in most

Re: [HACKERS] Reviewing freeze map code

On 2016-07-01 15:42:22 -0500, Jim Nasby wrote: > On 7/1/16 2:23 PM, Andres Freund wrote: > > > > The only > > > > cost of that is that vacuum will come along and mark the page > > > > all-visible again instead of skipping it, but that's probably not an > > > > enormous expense in most cases. > > I

Re: [HACKERS] Reviewing freeze map code

On 7/1/16 2:23 PM, Andres Freund wrote: > The only > cost of that is that vacuum will come along and mark the page > all-visible again instead of skipping it, but that's probably not an > enormous expense in most cases. I think the main cost is not having the page marked as all-visible for

Re: [HACKERS] Reviewing freeze map code

On 2016-07-01 15:18:39 -0400, Robert Haas wrote: > On Fri, Jul 1, 2016 at 10:22 AM, Masahiko Sawada > wrote: > > Ah, you're right, I misunderstood. > > > > Attached updated patch incorporating your comments. > > I've changed it so that heap_xlog_lock clears vm flags if

Re: [HACKERS] Reviewing freeze map code

On Fri, Jul 1, 2016 at 10:22 AM, Masahiko Sawada wrote: > Ah, you're right, I misunderstood. > > Attached updated patch incorporating your comments. > I've changed it so that heap_xlog_lock clears vm flags if page is > marked all frozen. I believe that this should be

Re: [HACKERS] Reviewing freeze map code

On Fri, Jul 1, 2016 at 11:12 AM, Amit Kapila wrote: > On Thu, Jun 30, 2016 at 8:10 PM, Masahiko Sawada > wrote: >> On Thu, Jun 30, 2016 at 3:12 PM, Amit Kapila wrote: >>> On Thu, Jun 30, 2016 at 9:13 AM, Andres Freund

Re: [HACKERS] Reviewing freeze map code

On Thu, Jun 30, 2016 at 8:10 PM, Masahiko Sawada wrote: > On Thu, Jun 30, 2016 at 3:12 PM, Amit Kapila wrote: >> On Thu, Jun 30, 2016 at 9:13 AM, Andres Freund wrote: >>> On 2016-06-30 08:59:16 +0530, Amit Kapila wrote: On

Re: [HACKERS] Reviewing freeze map code

On Thu, Jun 30, 2016 at 3:12 PM, Amit Kapila wrote: > On Thu, Jun 30, 2016 at 9:13 AM, Andres Freund wrote: >> On 2016-06-30 08:59:16 +0530, Amit Kapila wrote: >>> On Wed, Jun 29, 2016 at 10:30 PM, Andres Freund wrote: >>> > On

Re: [HACKERS] Reviewing freeze map code

On Thu, Jun 30, 2016 at 9:13 AM, Andres Freund wrote: > On 2016-06-30 08:59:16 +0530, Amit Kapila wrote: >> On Wed, Jun 29, 2016 at 10:30 PM, Andres Freund wrote: >> > On 2016-06-29 19:04:31 +0530, Amit Kapila wrote: >> >> There is nothing in this record

Re: [HACKERS] Reviewing freeze map code

On 2016-06-30 08:59:16 +0530, Amit Kapila wrote: > On Wed, Jun 29, 2016 at 10:30 PM, Andres Freund wrote: > > On 2016-06-29 19:04:31 +0530, Amit Kapila wrote: > >> There is nothing in this record which recorded the information about > >> visibility clear flag. > > > > I think

Re: [HACKERS] Reviewing freeze map code

On Wed, Jun 29, 2016 at 10:30 PM, Andres Freund wrote: > On 2016-06-29 19:04:31 +0530, Amit Kapila wrote: >> There is nothing in this record which recorded the information about >> visibility clear flag. > > I think we can actually defer the clearing to the lock release? How

Re: [HACKERS] Reviewing freeze map code

On 2016-06-29 19:04:31 +0530, Amit Kapila wrote: > There is nothing in this record which recorded the information about > visibility clear flag. I think we can actually defer the clearing to the lock release? A tuple being locked doesn't require the vm being cleared. > I think in this approach,

Re: [HACKERS] Reviewing freeze map code

On Wed, Jun 29, 2016 at 11:14 AM, Masahiko Sawada wrote: > On Fri, Jun 24, 2016 at 11:04 AM, Amit Kapila wrote: >> On Fri, Jun 24, 2016 at 4:33 AM, Andres Freund wrote: >>> On 2016-06-23 18:59:57 -0400, Alvaro Herrera wrote:

Re: [HACKERS] Reviewing freeze map code

On Fri, Jun 24, 2016 at 11:04 AM, Amit Kapila wrote: > On Fri, Jun 24, 2016 at 4:33 AM, Andres Freund wrote: >> On 2016-06-23 18:59:57 -0400, Alvaro Herrera wrote: >>> Andres Freund wrote: >>> >>> > I'm looking into three approaches right now: >>> >

Re: [HACKERS] Reviewing freeze map code

On Tue, Jun 28, 2016 at 8:06 PM, Masahiko Sawada wrote: > On Tue, Jun 21, 2016 at 6:59 AM, Andres Freund wrote: >> On 2016-06-20 17:55:19 -0400, Robert Haas wrote: >>> On Mon, Jun 20, 2016 at 4:24 PM, Andres Freund wrote: >>> > On

Re: [HACKERS] Reviewing freeze map code

On Tue, Jun 21, 2016 at 6:59 AM, Andres Freund wrote: > On 2016-06-20 17:55:19 -0400, Robert Haas wrote: >> On Mon, Jun 20, 2016 at 4:24 PM, Andres Freund wrote: >> > On 2016-06-20 16:10:23 -0400, Robert Haas wrote: >> >> What exactly is the point of all

Re: [HACKERS] Reviewing freeze map code

On Tue, Jun 21, 2016 at 10:59:25AM +1200, Thomas Munro wrote: > On Fri, Jun 17, 2016 at 3:36 PM, Noah Misch wrote: > > I agree the non-atomic, unlogged change is a problem. A related threat > > doesn't require a torn page: > > > > AssignTransactionId() xid=123 > >

Re: [HACKERS] Reviewing freeze map code

On Fri, Jun 24, 2016 at 4:33 AM, Andres Freund wrote: > On 2016-06-23 18:59:57 -0400, Alvaro Herrera wrote: >> Andres Freund wrote: >> >> > I'm looking into three approaches right now: >> > >> > 3) Use WAL logging for the already_marked = true case. >> >> >> > 3) This approach

Re: [HACKERS] Reviewing freeze map code

On 2016-06-23 18:59:57 -0400, Alvaro Herrera wrote: > Andres Freund wrote: > > > I'm looking into three approaches right now: > > > > 3) Use WAL logging for the already_marked = true case. > > > > 3) This approach so far seems the best. It's possible to reuse the > > xl_heap_lock record (in an

Re: [HACKERS] Reviewing freeze map code

Andres Freund wrote: > I'm looking into three approaches right now: > > 3) Use WAL logging for the already_marked = true case. > 3) This approach so far seems the best. It's possible to reuse the > xl_heap_lock record (in an afaics backwards compatible manner), and in > most cases the overhead

Re: [HACKERS] Reviewing freeze map code

On 2016-06-21 16:32:03 -0400, Robert Haas wrote: > On Tue, Jun 21, 2016 at 3:46 PM, Andres Freund wrote: > > On 2016-06-21 15:38:25 -0400, Robert Haas wrote: > >> On Tue, Jun 21, 2016 at 1:49 PM, Andres Freund wrote: > >> >> I'm also a bit dubious that

Re: [HACKERS] Reviewing freeze map code

On Tue, Jun 21, 2016 at 3:46 PM, Andres Freund wrote: > On 2016-06-21 15:38:25 -0400, Robert Haas wrote: >> On Tue, Jun 21, 2016 at 1:49 PM, Andres Freund wrote: >> >> I'm also a bit dubious that LockAcquire is safe to call in general >> >> with interrupts

Re: [HACKERS] Reviewing freeze map code

On 2016-06-21 15:38:25 -0400, Robert Haas wrote: > On Tue, Jun 21, 2016 at 1:49 PM, Andres Freund wrote: > >> I'm also a bit dubious that LockAcquire is safe to call in general > >> with interrupts held. > > > > Looks like we could just acquire the tuple-lock *before* doing

Re: [HACKERS] Reviewing freeze map code

On Tue, Jun 21, 2016 at 1:49 PM, Andres Freund wrote: >> I'm also a bit dubious that LockAcquire is safe to call in general >> with interrupts held. > > Looks like we could just acquire the tuple-lock *before* doing the > toast_insert_or_update/RelationGetBufferForTuple, but

Re: [HACKERS] Reviewing freeze map code

On 2016-06-21 13:03:24 -0400, Robert Haas wrote: > On Tue, Jun 21, 2016 at 12:54 PM, Andres Freund wrote: > >> I don't quite understand the intended semantics of this proposed flag. > > > > Whenever the flag is set, we have to acquire the heavyweight tuple lock > > before

Re: [HACKERS] Reviewing freeze map code

On Tue, Jun 21, 2016 at 12:54 PM, Andres Freund wrote: >> I don't quite understand the intended semantics of this proposed flag. > > Whenever the flag is set, we have to acquire the heavyweight tuple lock > before continuing. That guarantees nobody else can modify the tuple, >

Re: [HACKERS] Reviewing freeze map code

On 2016-06-21 10:50:36 -0400, Robert Haas wrote: > On Mon, Jun 20, 2016 at 11:51 PM, Andres Freund wrote: > >> > So far the best idea I have - and it's really not a good one - is to > >> > invent a new hint-bit that tells concurrent updates to acquire a > >> > heavyweight

Re: [HACKERS] Reviewing freeze map code

On Tue, Jun 21, 2016 at 10:47 AM, Tom Lane wrote: > Robert Haas writes: >> On Mon, Jun 20, 2016 at 5:59 PM, Andres Freund wrote: >>> Consider what happens if we, after restarting at l2, notice that we >>> can't actually insert, but

Re: [HACKERS] Reviewing freeze map code

On Mon, Jun 20, 2016 at 11:51 PM, Andres Freund wrote: >> > So far the best idea I have - and it's really not a good one - is to >> > invent a new hint-bit that tells concurrent updates to acquire a >> > heavyweight tuple lock, while releasing the page-level lock. If that >> >

Re: [HACKERS] Reviewing freeze map code

Robert Haas writes: > On Mon, Jun 20, 2016 at 5:59 PM, Andres Freund wrote: >> Consider what happens if we, after restarting at l2, notice that we >> can't actually insert, but return in the !HeapTupleMayBeUpdated >> branch. > OK, I see what you mean.

Re: [HACKERS] Reviewing freeze map code

On Mon, Jun 20, 2016 at 5:59 PM, Andres Freund wrote: > On 2016-06-20 17:55:19 -0400, Robert Haas wrote: >> On Mon, Jun 20, 2016 at 4:24 PM, Andres Freund wrote: >> > On 2016-06-20 16:10:23 -0400, Robert Haas wrote: >> >> I >> >> mean, suppose we just

Re: [HACKERS] Reviewing freeze map code

On Tue, Jun 21, 2016 at 9:21 AM, Andres Freund wrote: > > On 2016-06-21 08:59:13 +0530, Amit Kapila wrote: > > Can we consider to use some strategy to avoid deadlocks without releasing > > the lock on old page? Consider if we could have a mechanism such that > >

Re: [HACKERS] Reviewing freeze map code

On Tue, Jun 21, 2016 at 9:16 AM, Thomas Munro wrote: > > On Tue, Jun 21, 2016 at 3:29 PM, Amit Kapila wrote: > > Some others ways could be: > > > > Before releasing the lock on buffer containing old tuple, clear the VM and > > visibility

Re: [HACKERS] Reviewing freeze map code

On Tue, Jun 21, 2016 at 9:08 AM, Thomas Munro wrote: > > On Tue, Jun 21, 2016 at 3:29 PM, Amit Kapila wrote: > > On Tue, Jun 21, 2016 at 1:03 AM, Andres Freund wrote: > >> Well, I think generally nobody seriously looked

Re: [HACKERS] Reviewing freeze map code

On 2016-06-21 08:59:13 +0530, Amit Kapila wrote: > Can we consider to use some strategy to avoid deadlocks without releasing > the lock on old page? Consider if we could have a mechanism such that > RelationGetBufferForTuple() will ensure that it always returns a new buffer > which has

Re: [HACKERS] Reviewing freeze map code

On Tue, Jun 21, 2016 at 3:29 PM, Amit Kapila wrote: > Some others ways could be: > > Before releasing the lock on buffer containing old tuple, clear the VM and > visibility info from page and WAL log it. I think this could impact > performance depending on how frequently

Re: [HACKERS] Reviewing freeze map code

On Tue, Jun 21, 2016 at 3:29 PM, Amit Kapila wrote: > On Tue, Jun 21, 2016 at 1:03 AM, Andres Freund wrote: >> Well, I think generally nobody seriously looked at actually refactoring >> heap_update(), even though that'd be a good idea. But in this

Re: [HACKERS] Reviewing freeze map code

On Tue, Jun 21, 2016 at 1:03 AM, Andres Freund wrote: > > On 2016-06-15 08:56:52 -0400, Robert Haas wrote: > > Yikes: heap_update() sets the tuple's XMAX, CMAX, infomask, infomask2, > > and CTID without logging anything or clearing the all-visible flag and > > then releases

Re: [HACKERS] Reviewing freeze map code

On Fri, Jun 17, 2016 at 3:36 PM, Noah Misch wrote: > I agree the non-atomic, unlogged change is a problem. A related threat > doesn't require a torn page: > > AssignTransactionId() xid=123 > heapam.c:3931 HeapTupleHeaderSetXmax(oldtup.t_data, 123); > some ERROR before

Re: [HACKERS] Reviewing freeze map code

On 2016-06-20 17:55:19 -0400, Robert Haas wrote: > On Mon, Jun 20, 2016 at 4:24 PM, Andres Freund wrote: > > On 2016-06-20 16:10:23 -0400, Robert Haas wrote: > >> What exactly is the point of all of that already_marked stuff? > > > > Preventing the old tuple from being

Re: [HACKERS] Reviewing freeze map code

On Mon, Jun 20, 2016 at 4:24 PM, Andres Freund wrote: > On 2016-06-20 16:10:23 -0400, Robert Haas wrote: >> What exactly is the point of all of that already_marked stuff? > > Preventing the old tuple from being locked/updated by another backend, > while unlocking the buffer. >

Re: [HACKERS] Reviewing freeze map code

On 2016-06-20 16:10:23 -0400, Robert Haas wrote: > What exactly is the point of all of that already_marked stuff? Preventing the old tuple from being locked/updated by another backend, while unlocking the buffer. > I > mean, suppose we just don't do any of that before we go off to do >

Re: [HACKERS] Reviewing freeze map code

On Mon, Jun 20, 2016 at 3:33 PM, Andres Freund wrote: >> I'm not sure what to do about this: this part of the heap_update() >> logic has been like this forever, and I assume that if it were easy to >> refactor this away, somebody would have done it by now. > > Well, I think

Re: [HACKERS] Reviewing freeze map code

On 2016-06-15 08:56:52 -0400, Robert Haas wrote: > Yikes: heap_update() sets the tuple's XMAX, CMAX, infomask, infomask2, > and CTID without logging anything or clearing the all-visible flag and > then releases the lock on the heap page to go do some more work that > might even ERROR out. Only if

Re: [HACKERS] Reviewing freeze map code

On Wed, Jun 15, 2016 at 08:56:52AM -0400, Robert Haas wrote: > On Wed, Jun 15, 2016 at 2:41 AM, Thomas Munro > wrote: > > I spent some time chasing down the exact circumstances. I suspect > > that there may be an interlocking problem in heap_update. Using the > >

Re: [HACKERS] Reviewing freeze map code

On Wed, Jun 15, 2016 at 9:59 AM, Amit Kapila wrote: >> That just kicks the can down the road. Then you have PD_ALL_VISIBLE >> clear but the VM bit is still set. > > I mean to say clear both as we are doing currently in code: > if (PageIsAllVisible(BufferGetPage(buffer)))

Re: [HACKERS] Reviewing freeze map code

On Wed, Jun 15, 2016 at 10:03 AM, Masahiko Sawada wrote: >> I'm not sure what to do about this: this part of the heap_update() >> logic has been like this forever, and I assume that if it were easy to >> refactor this away, somebody would have done it by now. > > How about

Re: [HACKERS] Reviewing freeze map code

On Wed, Jun 15, 2016 at 9:56 PM, Robert Haas wrote: > On Wed, Jun 15, 2016 at 2:41 AM, Thomas Munro > wrote: >> I spent some time chasing down the exact circumstances. I suspect >> that there may be an interlocking problem in heap_update.

Re: [HACKERS] Reviewing freeze map code

On Wed, Jun 15, 2016 at 7:13 PM, Robert Haas wrote: > > On Wed, Jun 15, 2016 at 9:43 AM, Amit Kapila wrote: > > On Wed, Jun 15, 2016 at 6:26 PM, Robert Haas wrote: > >> On Wed, Jun 15, 2016 at 2:41 AM, Thomas Munro > >>

Re: [HACKERS] Reviewing freeze map code

On Wed, Jun 15, 2016 at 9:43 AM, Amit Kapila wrote: > On Wed, Jun 15, 2016 at 6:26 PM, Robert Haas wrote: >> On Wed, Jun 15, 2016 at 2:41 AM, Thomas Munro >> wrote: >> > I spent some time chasing down the exact

Re: [HACKERS] Reviewing freeze map code

On Wed, Jun 15, 2016 at 6:26 PM, Robert Haas wrote: > > On Wed, Jun 15, 2016 at 2:41 AM, Thomas Munro > wrote: > > I spent some time chasing down the exact circumstances. I suspect > > that there may be an interlocking problem in

Re: [HACKERS] Reviewing freeze map code

On Wed, Jun 15, 2016 at 2:41 AM, Thomas Munro wrote: > I spent some time chasing down the exact circumstances. I suspect > that there may be an interlocking problem in heap_update. Using the > line numbers from cae1c788 [1], I see the following interaction >

Re: [HACKERS] Reviewing freeze map code

On Wed, Jun 15, 2016 at 12:44 AM, Robert Haas wrote: > On Tue, Jun 14, 2016 at 8:11 AM, Robert Haas wrote: I noticed that the tuples that it reported were always offset 1 in a page, and that the page always had a maxoff over a couple of

Re: [HACKERS] Reviewing freeze map code

On Tue, Jun 14, 2016 at 8:11 AM, Robert Haas wrote: >>> I noticed that the tuples that it reported were always offset 1 in a >>> page, and that the page always had a maxoff over a couple of hundred, >>> and that we called record_corrupt_item because VM_ALL_VISIBLE returned

Re: [HACKERS] Reviewing freeze map code

On Tue, Jun 14, 2016 at 8:08 AM, Robert Haas wrote: > On Tue, Jun 14, 2016 at 2:53 AM, Thomas Munro > wrote: >> On Tue, Jun 14, 2016 at 4:02 AM, Robert Haas wrote: >>> On Sat, Jun 11, 2016 at 5:00 PM, Robert Haas

Re: [HACKERS] Reviewing freeze map code

On Tue, Jun 14, 2016 at 2:53 AM, Thomas Munro wrote: > On Tue, Jun 14, 2016 at 4:02 AM, Robert Haas wrote: >> On Sat, Jun 11, 2016 at 5:00 PM, Robert Haas wrote: How about changing the return tuple of

  1   2   3   >