Horton

Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager

Masahiko Sawada <sawada.mshk@gmail.com>

View thread
From: Masahiko Sawada <sawada.mshk@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Thomas Munro <thomas.munro@enterprisedb.com>, Amit Kapila <amit.kapila16@gmail.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2017-12-11T21:10:24Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Allow page lock to conflict among parallel group members.

  2. Allow relation extension lock to conflict among parallel group members.

  3. Add assert to ensure that page locks don't participate in deadlock cycle.

  4. Assert that we don't acquire a heavyweight lock on another object after

  5. Fix unsafe usage of strerror(errno) within ereport().

On Tue, Dec 12, 2017 at 5:15 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sun, Dec 10, 2017 at 11:51 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>> Attached updated version patch. Please review it.
>
> I went over this today; please find attached an updated version which
> I propose to commit.
>
> Changes:
>
> - Various formatting fixes, including running pgindent.
>
> - Various comment updates.
>
> - Make RELEXT_WAIT_COUNT_MASK equal RELEXT_LOCK_BIT - 1 rather than
> some unnecessarily smaller number.
>
> - In InitRelExtLocks, don't bother using mul_size; we already know it
> won't overflow, because we did the same thing in RelExtLockShmemSize.
>
> - When we run into an error trying to release a lock, log it as a
> WARNING and don't mark it as translatable.  Follows lock.c.  An ERROR
> here probably just recurses infinitely.
>
> - Don't bother passing OID to RelExtLockRelease.
>
> - Reorder functions a bit for (IMHO) better clarity.
>
> - Make UnlockRelationForExtension just use a single message for both
> failure modes.  They are closely-enough related that I think that's
> fine.
>
> - Make WaitForRelationExtensionLockToBeFree complain if we already
> hold an extension lock.
>
> - In RelExtLockCleanup, clear held_relextlock.waiting.  This would've
> made for a nasty bug.
>
> - Also in that function, assert that we don't hold both a lock and a wait count.
>

Thank you for updating the patch. Here is two minor comments.

+ * we acquire the same relation extension lock repeatedly.  nLocks is 0 is the
+ * number of times we've acquired that lock;

Should it be "nLocks is the number of times we've acquired that lock:"?

+    /* Remember lock held by this backend */
+    held_relextlock.relid = relid;
+    held_relextlock.lock = relextlock;
+    held_relextlock.nLocks = 1;

We set held_relextlock.relid and held_relextlock.lock again. Can we remove them?

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center