documentation about explicit locking

Started by Amit Langotealmost 8 years ago11 messageshackers
Jump to latest
#1Amit Langote
Langote_Amit_f8@lab.ntt.co.jp

Hi.

I wonder why we mention on the following page that CREATE COLLATION
requires SHARE ROW EXCLUSIVE lock

https://www.postgresql.org/docs/devel/static/explicit-locking.html

I know that's the lock taken on the pg_collation catalog, but do we need
to mention locks taken by a DDL command on the catalogs it affects? All
other commands mentioned on the page require to specify the table name
that the lock will be taken on.

Thanks,
Amit

#2Robert Haas
robertmhaas@gmail.com
In reply to: Amit Langote (#1)
Re: documentation about explicit locking

On Wed, Jul 4, 2018 at 3:09 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

I wonder why we mention on the following page that CREATE COLLATION
requires SHARE ROW EXCLUSIVE lock

https://www.postgresql.org/docs/devel/static/explicit-locking.html

I know that's the lock taken on the pg_collation catalog, but do we need
to mention locks taken by a DDL command on the catalogs it affects? All
other commands mentioned on the page require to specify the table name
that the lock will be taken on.

Yes, that looks odd.

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

#3Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Robert Haas (#2)
Re: documentation about explicit locking

On 2018/07/05 23:02, Robert Haas wrote:

On Wed, Jul 4, 2018 at 3:09 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

I wonder why we mention on the following page that CREATE COLLATION
requires SHARE ROW EXCLUSIVE lock

https://www.postgresql.org/docs/devel/static/explicit-locking.html

I know that's the lock taken on the pg_collation catalog, but do we need
to mention locks taken by a DDL command on the catalogs it affects? All
other commands mentioned on the page require to specify the table name
that the lock will be taken on.

Yes, that looks odd.

OK, here is a patch.

I see that it was one of Peter E's commits that added that, so cc'd him.

Thanks,
Amit

Attachments:

doc-locking.patchtext/plain; charset=UTF-8; name=doc-locking.patchDownload+1-2
#4Peter Eisentraut
peter_e@gmx.net
In reply to: Amit Langote (#3)
Re: documentation about explicit locking

On 06.07.18 04:00, Amit Langote wrote:

On 2018/07/05 23:02, Robert Haas wrote:

On Wed, Jul 4, 2018 at 3:09 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

I wonder why we mention on the following page that CREATE COLLATION
requires SHARE ROW EXCLUSIVE lock

https://www.postgresql.org/docs/devel/static/explicit-locking.html

I know that's the lock taken on the pg_collation catalog, but do we need
to mention locks taken by a DDL command on the catalogs it affects? All
other commands mentioned on the page require to specify the table name
that the lock will be taken on.

Yes, that looks odd.

OK, here is a patch.

I see that it was one of Peter E's commits that added that, so cc'd him.

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.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#5Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Peter Eisentraut (#4)
Re: documentation about explicit locking

On 2018/07/18 18:30, Peter Eisentraut wrote:

On 06.07.18 04:00, Amit Langote wrote:

On 2018/07/05 23:02, Robert Haas wrote:

On Wed, Jul 4, 2018 at 3:09 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

I wonder why we mention on the following page that CREATE COLLATION
requires SHARE ROW EXCLUSIVE lock

https://www.postgresql.org/docs/devel/static/explicit-locking.html

I know that's the lock taken on the pg_collation catalog, but do we need
to mention locks taken by a DDL command on the catalogs it affects? All
other commands mentioned on the page require to specify the table name
that the lock will be taken on.

Yes, that looks odd.

OK, here is a patch.

I see that it was one of Peter E's commits that added that, so cc'd him.

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?

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.

Thanks,
Amit

Attachments:

create-collation-locking-doc.patchtext/plain; charset=UTF-8; name=create-collation-locking-doc.patchDownload+6-2
create-collation-race-fix.patchtext/plain; charset=UTF-8; name=create-collation-race-fix.patchDownload+7-3
#6Kyotaro Horiguchi
horikyota.ntt@gmail.com
In reply to: Amit Langote (#5)
Re: documentation about explicit locking

Hello.

At Thu, 19 Jul 2018 13:17:14 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <85dc6464-eb59-ba59-75d3-09b292fa853d@lab.ntt.co.jp>

On 2018/07/18 18:30, Peter Eisentraut wrote:

On 06.07.18 04:00, Amit Langote wrote:

On 2018/07/05 23:02, Robert Haas wrote:

On Wed, Jul 4, 2018 at 3:09 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

I wonder why we mention on the following page that CREATE COLLATION
requires SHARE ROW EXCLUSIVE lock

https://www.postgresql.org/docs/devel/static/explicit-locking.html

I know that's the lock taken on the pg_collation catalog, but do we need
to mention locks taken by a DDL command on the catalogs it affects? All
other commands mentioned on the page require to specify the table name
that the lock will be taken on.

Yes, that looks odd.

OK, here is a patch.

I see that it was one of Peter E's commits that added that, so cc'd him.

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.

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.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

#7Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Kyotaro Horiguchi (#6)
Re: documentation about explicit locking

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

Attachments:

create-collation-locking-doc-v2.patchtext/plain; charset=UTF-8; name=create-collation-locking-doc-v2.patchDownload+8-2
create-collation-race-fix.patchtext/plain; charset=UTF-8; name=create-collation-race-fix.patchDownload+7-3
#8Peter Eisentraut
peter_e@gmx.net
In reply to: Amit Langote (#7)
Re: documentation about explicit locking

On 20/07/2018 02:30, Amit Langote wrote:

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.

committed the documentation change

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#9Peter Eisentraut
peter_e@gmx.net
In reply to: Amit Langote (#5)
Re: documentation about explicit locking

On 19/07/2018 06:17, Amit Langote wrote:

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.

You get the same behavior with for example CREATE FUNCTION or CREATE
TYPE. I don't think we need to fix this specifically for CREATE COLLATION.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#10Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Peter Eisentraut (#8)
Re: documentation about explicit locking

On 2018/07/31 5:25, Peter Eisentraut wrote:

On 20/07/2018 02:30, Amit Langote wrote:

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.

committed the documentation change

Thank you.

Regards,
Amit

#11Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Peter Eisentraut (#9)
Re: documentation about explicit locking

On 2018/07/31 5:27, Peter Eisentraut wrote:

On 19/07/2018 06:17, Amit Langote wrote:

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.

You get the same behavior with for example CREATE FUNCTION or CREATE
TYPE. I don't think we need to fix this specifically for CREATE COLLATION.

Hmm, yeah. Although fixing the race for CREATE COLLATION seems easier
than other cases due to the self-exclusive lock on the catalog, that
doesn't necessarily mean we have to.

Thanks,
Amit