Horiguchi-san,
Thanks for taking a look.
On 2018/07/19 18:23, Kyotaro HORIGUCHI wrote:
> Hello.
>
> At Thu, 19 Jul 2018 13:17:14 +0900, Amit Langote wrote:
>> On 2018/07/18 18:30, Peter Eisentraut wrote:
>>> The reason this is mentioned is that CREATE COLLATION takes a SHARE ROW
>>> EXCLUSIVE lock on pg_collation whereas similar CREATE commands only take
>>> a ROW EXCLUSIVE lock on their catalogs. (So you can only have one
>>> CREATE COLLATION running at a time. The reasons for this are explained
>>> in pg_collation.c.) I think mentioning this was requested during patch
>>> review.
>>
>> I see. Although, which lock we take on the system catalog for
>> implementing a particular command seems to be an internal detail. What's
>> clearly user-visible in this case is that CREATE COLLATION command cannot
>> be used simultaneously by concurrent sessions, so it should be pointed out
>> in the CREATE COLLATION command's documentation. On a quick check, it
>> doesn't seem to be. So, I have updated my patch to also add a line about
>> that on CREATE COLLATION page. What do you think?
>
> I'm not Peter but I have a comment on this.
>
> + Note that only one of the concurrent sessions can run
> + <command>CREATE COLLATION</command> at a time.
>
> The description seems to me to be failing to give clear idea of
> what operation causes what behavior. I agree that the description
> in the explicit-locking section is out of the place since it is
> not a lock on *specified* tables. I'd like to have a description
> with the similar level here instead, like this:
>
> Note that CREATE COLLATION takes a SHARE ROW EXLUCSIVE lock on
> pg_collation system calatlog, which blocks other concurrent
> CREATE COLLATION.
We don't explicitly mention what locks we take on system catalogs
elsewhere, but CREATE COLLATION is different from other commands, so I'm
fine with adding more details as you suggest, so updated the text.
>> When playing with this, I observed that a less user-friendly error message
>> is emitted if multiple sessions race to create the same collation.
>>
>> Session 1:
>> begin;
>> create collation collname (...);
>>
>> Session 2:
>> create collation collname (...);
>> <blocks for lock on pg_collation>
>>
>> Session 1:
>> commit;
>>
>> Session 2:
>> ERROR: duplicate key value violates unique constraint
>> "pg_collation_name_enc_nsp_index"
>> DETAIL: Key (collname, collencoding, collnamespace)=(collname, 6, 2200)
>> already exists.
>>
>> I figured that's because the order in CollationCreate of locking the
>> catalog and checking in syscache whether a duplicate exists. I think we
>> should check the syscache for duplicate *after* we have locked the
>> catalog, as done in the other patch that's attached.
>
> Such cases in other commands usually have a very narrow window
> but the said lock widens the window very much in the case:p So +1
> from me to this change.
Attached updated patches.
Thanks,
Amit
diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
index 24613e3c75..4ec853b77a 100644
--- a/doc/src/sgml/mvcc.sgml
+++ b/doc/src/sgml/mvcc.sgml
@@ -970,8 +970,7 @@ ERROR: could not serialize access due to read/write
dependencies among transact
</para>
<para>
- Acquired by <command>CREATE COLLATION</command>,
- <command>CREATE TRIGGER</command>, and many forms of
+ Acquired by <command>CREATE TRIGGER</command>, and many forms of
<command>ALTER TABLE</command> (see <xref linkend="sql-altertable"/>).
</para>
</listitem>
diff --git a/doc/src/sgml/ref/create_collation.sgml
b/doc/src/sgml/ref/create_collation.sgml
index 5bc9af5499..195d960603 100644
--- a/doc/src/sgml/ref/create_collation.sgml
+++ b/doc/src/sgml/ref/create_collation.sgml
@@ -163,6 +163,13 @@ CREATE COLLATION [ IF NOT EXISTS ]
<replaceable>name</replaceable> FROM <replace
<title>Notes</title>
<para>
+ Note that <command>CREATE COLLATION</command> takes a
+ <literal>SHARE ROW EXCLUSIVE</literal> lock on
+ <structname>pg_collation</structname> system calatlog, which prevents
+ concurrent sessions from running the same command.
+ </para>
+
+ <para>
Use <command>DROP COLLATION</command> to remove user-defined collations.
</para>
diff --git a/src/backend/catalog/pg_collation.c
b/src/backend/catalog/pg_collation.c
index ce7e5fb5cc..33a5510bf9 100644
--- a/src/backend/catalog/pg_collation.c
+++ b/src/backend/catalog/pg_collation.c
@@ -70,6 +70,9 @@ CollationCreate(const char *collname, Oid collnamespace,
AssertArg(collcollate);
AssertArg(collctype);
+ /* open pg_collation; see below about the lock level */
+ rel = heap_open(CollationRelationId, ShareRowExclusiveLock);
+
/*
* Make sure there is no existing collation of same name & encoding.
*
@@ -83,9 +86,13 @@ CollationCreate(const char *collname, Oid collnamespace,
ObjectIdGetDatum(collnamespace)))
{
if (quiet)
+ {
+ heap_close(rel, NoLock);
return InvalidOid;
+ }
else if (if_not_exists)
{
+ heap_close(rel, NoLock);
ereport(NOTICE,
(errcode(ERRCODE_DUPLICATE_OBJECT),
collencoding == -1
@@ -105,9 +112,6 @@ CollationCreate(const char *collname, Oid collnamespace,
collname,
pg_encoding_to_char(collencoding))));
}
- /* open pg_collation; see below about the lock level */
- rel = heap_open(CollationRelationId, ShareRowExclusiveLock);
-
/*
* Also forbid a specific-encoding collation shadowing an any-encoding
* collation, or an any-encoding collation being shadowed (see
