Horton

Re: WAL logging problem in 9.4.3?

Michael Paquier <michael.paquier@gmail.com>

View thread View on postgr.es
From: Michael Paquier <michael.paquier@gmail.com>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>, Simon Riggs <simon@2ndquadrant.com>, Tom Lane <tgl@sss.pgh.pa.us>, Andres Freund <andres@anarazel.de>, Fujii Masao <masao.fujii@gmail.com>, Martijn van Oosterhout <kleptog@svana.org>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2016-02-19T07:33:21Z
Lists: pgsql-hackers 
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.
-- 
Michael

Commits

  1. Add perl2host call missing from a new test file.

  2. Skip WAL for new relfilenodes, under wal_level=minimal.

  3. Revert "Skip WAL for new relfilenodes, under wal_level=minimal."

  4. Back-patch log_newpage_range().

  5. During heap rebuild, lock any TOAST index until end of transaction.

  6. In log_newpage_range(), heed forkNum and page_std arguments.

  7. Back-patch src/test/recovery and PostgresNode from 9.6 to 9.5.

  8. Reduce pg_ctl's reaction time when waiting for postmaster start/stop.

  9. Accelerate end-of-transaction dropping of relations

  10. Redesign the planner's handling of index-descent cost estimation.

  11. Make TRUNCATE do truncate-in-place when processing a relation that was created