Re: WAL logging problem in 9.4.3?

On Fri, Feb 19, 2016 at 4:33 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Feb 18, 2016 at 4:27 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Thu, Feb 4, 2016 at 3:24 PM, Heikki Linnakangas <hlinnaka@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