On Fri, Feb 19, 2016 at 4:33 PM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Thu, Feb 18, 2016 at 4:27 PM, Michael Paquier > <michael.paqu...@gmail.com> wrote: >> On Thu, Feb 4, 2016 at 3:24 PM, Heikki Linnakangas <hlinn...@iki.fi> wrote: >>> I dropped the ball on this one back in July, so here's an attempt to revive >>> this thread. >>> >>> I spent some time fixing the remaining issues with the prototype patch I >>> posted earlier, and rebased that on top of current git master. See attached. >>> >>> Some review of that would be nice. If there are no major issues with it, I'm >>> going to create backpatchable versions of this for 9.4 and below. >> >> I am going to look into that very soon. For now and to not forget >> about this bug, I have added an entry in the CF app: >> https://commitfest.postgresql.org/9/528/ > > Worth noting that this patch does not address the problem with index > relations when a TRUNCATE is used in the same transaction as its > CREATE TABLE, take that for example when wal_level = minimal: > 1) Run transaction > =# begin; > BEGIN > =# create table ab (a int primary key); > CREATE TABLE > =# truncate ab; > TRUNCATE TABLE > =# commit; > COMMIT > 2) Restart server with immediate mode. > 3) Failure: > =# table ab; > ERROR: XX001: could not read block 0 in file "base/16384/16388": read > only 0 of 8192 bytes > LOCATION: mdread, md.c:728 > > The case where a COPY is issued after TRUNCATE is fixed though, so > that's still an improvement. > > Here are other comments. > > + /* Flush updates to relations that we didn't WAL-logged */ > + smgrDoPendingSyncs(true); > "Flush updates to relations there were not WAL-logged"? > > +void > +FlushRelationBuffersWithoutRelCache(RelFileNode rnode, bool islocal) > +{ > + FlushRelationBuffers_common(smgropen(rnode, InvalidBackendId), islocal); > +} > islocal is always set as false, I'd rather remove it this argument > from FlushRelationBuffersWithoutRelCache. > > for (i = 0; i < nrels; i++) > + { > smgrclose(srels[i]); > + } > Looks like noise. > > + if (!found) > + { > + pending->truncated_to = InvalidBlockNumber; > + pending->sync_above = nblocks; > + > + elog(DEBUG2, "registering new pending sync for rel %u/%u/%u at > block %u", > + rnode.spcNode, rnode.dbNode, rnode.relNode, nblocks); > + > + } > + else if (pending->sync_above == InvalidBlockNumber) > + { > + elog(DEBUG2, "registering pending sync for rel %u/%u/%u at block %u", > + rnode.spcNode, rnode.dbNode, rnode.relNode, nblocks); > + pending->sync_above = nblocks; > + } > + else > Here couldn't it be possible that when (sync_above != > InvalidBlockNumber), nblocks can be higher than sync_above? In which > case we had better increase sync_above to nblocks, no? > > + if (!pendingSyncs) > + createPendingSyncsHash(); > + pending = (PendingRelSync *) hash_search(pendingSyncs, > + (void *) &rel->rd_node, > + HASH_ENTER, &found); > This is lacking comments. > > - if (XLogHintBitIsNeeded() && (bufHdr->flags & BM_PERMANENT)) > + BufferGetTag(buffer, &rnode, &forknum, &blknum); > + if (XLogHintBitIsNeeded() && (bufHdr->flags & BM_PERMANENT) && > + !smgrIsSyncPending(rnode, blknum)) > Here as well explaining in more details why the buffer does not need > to go through XLogSaveBufferForHint would be nice.
An additional one: - XLogRegisterBuffer(0, newbuf, bufflags); - if (oldbuf != newbuf) - XLogRegisterBuffer(1, oldbuf, REGBUF_STANDARD); In log_heap_update, the new buffer is now conditionally logged depending on if the heap needs WAL or not. Now during replay the following thing is done: - oldaction = XLogReadBufferForRedo(record, (oldblk == newblk) ? 0 : 1, - &obuffer); + if (oldblk == newblk) + oldaction = XLogReadBufferForRedo(record, 0, &obuffer); + else if (XLogRecHasBlockRef(record, 1)) + oldaction = XLogReadBufferForRedo(record, 1, &obuffer); + else + oldaction = BLK_DONE; Shouldn't we check for XLogRecHasBlockRef(record, 0) when the tuple is updated on the same page? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
