Horton

Re: Speed up Clog Access by increasing CLOG buffers

Robert Haas <robertmhaas@gmail.com>

View thread View on postgr.es
From: Robert Haas <robertmhaas@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Thom Brown <thom@linux.com>, Simon Riggs <simon@2ndquadrant.com>, Jesper Pedersen <jesper.pedersen@redhat.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2016-02-11T14:32:52Z
Lists: pgsql-hackers 
On Thu, Feb 11, 2016 at 9:04 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> Is there anything, I can do to move this forward?
>
> Well, looking at this again, I think I'm OK to go with your names.
> That doesn't seem like the thing to hold up the patch for.  So I'll go
> ahead and push the renaming patch now.

On the substantive part of the patch, this doesn't look safe:

+    /*
+     * Add ourselves to the list of processes needing a group XID status
+     * update.
+     */
+    proc->clogGroupMember = true;
+    proc->clogGroupMemberXid = xid;
+    proc->clogGroupMemberXidStatus = status;
+    proc->clogGroupMemberPage = pageno;
+    proc->clogGroupMemberLsn = lsn;
+    while (true)
+    {
+        nextidx = pg_atomic_read_u32(&procglobal->clogGroupFirst);
+
+        /*
+         * Add the proc to list if the clog page where we need to update the
+         * current transaction status is same as group leader's clog page.
+         */
+        if (nextidx != INVALID_PGPROCNO &&
+            ProcGlobal->allProcs[nextidx].clogGroupMemberPage !=
proc->clogGroupMemberPage)
+            return false;

DANGER HERE!

+        pg_atomic_write_u32(&proc->clogGroupNext, nextidx);
+
+        if (pg_atomic_compare_exchange_u32(&procglobal->clogGroupFirst,
+                                           &nextidx,
+                                           (uint32) proc->pgprocno))
+            break;
+    }

There is a potential ABA problem here.  Suppose that this code
executes in one process as far as the line that says DANGER HERE.
Then, the group leader wakes up, performs all of the CLOG
modifications, performs another write transaction, and again becomes
the group leader, but for a different member page.  Then, the original
process that went to sleep at DANGER HERE wakes up.  At this point,
the pg_atomic_compare_exchange_u32 will succeed and we'll have
processes with different pages in the list, contrary to the intention
of the code.

This kind of thing can be really subtle and difficult to fix.  The
problem might not happen even with a very large amount of testing, and
then might happen in the real world on some other hardware or on
really rare occasions.  In general, compare-and-swap loops need to be
really really simple with minimal dependencies on other data, ideally
none.  It's very hard to make anything else work.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Commits

  1. Use group updates when setting transaction status in clog.

  2. Improve 64bit atomics support.

  3. Add ProcArrayGroupUpdate wait event.

  4. Make the different Unix-y semaphore implementations ABI-compatible.

  5. Fix broken ALTER INDEX documentation

  6. Code and docs review for commit 3187d6de0e5a9e805b27c48437897e8c39071d45.

  7. Partition the freelist for shared dynahash tables.

  8. Correct StartupSUBTRANS for page wraparound

  9. Make idle backends exit if the postmaster dies.

  10. contrib/sslinfo: add ssl_extension_info SRF

  11. Reduce ProcArrayLock contention by removing backends in batches.

  12. Fix `make installcheck` for serializable transactions.

  13. Lockless StrategyGetBuffer clock sweep hot path.

  14. Reduce sinval synchronization overhead.