Postgres Pro
Downloads
Home   >   mailing lists

Thread: COPY, lock release and MVCC

COPY, lock release and MVCC

From
Laurenz Albe
Date:
I happened to notice that COPY TO releases the ACCESS SHARE lock
on the table right when the command ends rather than holding it
until the end of the transaction:

From backend/commands/copy.c:

/*
 * Close the relation.  If reading, we can release the AccessShareLock
 * we got; if writing, we should hold the lock until end of transaction
 * to ensure that updates will be committed before lock is released.
 */
heap_close(rel, (from ? NoLock : AccessShareLock));

This violates the two-phase locking protocol and means that,
for example, the second COPY from the same table in a
REPEATABLE READ transaction might fail or return nothing if
a concurrent transaction dropped or truncated the table in the
mean time.

Is this a bug or intentional (for example, to make pg_dump release
its locks early)?  In the latter case, it warrants documentation.

I dug into the history:

The comment is from commit 4dded12faad, before which COPY TO also
released the lock immediately.

The early lock release was added in commit bd272cace63, but that
only reflected how the indexes were locked before.

So this behavior seems to go back all the way.

Yours,
Laurenz Albe

Re: COPY, lock release and MVCC

From
Robert Haas
Date:
On Fri, May 8, 2020 at 4:58 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> I happened to notice that COPY TO releases the ACCESS SHARE lock
> on the table right when the command ends rather than holding it
> until the end of the transaction:

That seems inconsistent with what an INSERT statement would do, and thus bad.

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

Re: COPY, lock release and MVCC

From
Laurenz Albe
Date:
On Mon, 2020-05-11 at 15:43 -0400, Robert Haas wrote:
> On Fri, May 8, 2020 at 4:58 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> > I happened to notice that COPY TO releases the ACCESS SHARE lock
> > on the table right when the command ends rather than holding it
> > until the end of the transaction:
> 
> That seems inconsistent with what an INSERT statement would do, and thus bad.

Well, should we fix the code or the documentation?

Yours,
Laurenz Albe

Re: COPY, lock release and MVCC

From
Tom Lane
Date:
Laurenz Albe <laurenz.albe@cybertec.at> writes:
> On Mon, 2020-05-11 at 15:43 -0400, Robert Haas wrote:
>> On Fri, May 8, 2020 at 4:58 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>>> I happened to notice that COPY TO releases the ACCESS SHARE lock
>>> on the table right when the command ends rather than holding it
>>> until the end of the transaction:

>> That seems inconsistent with what an INSERT statement would do, and thus bad.

> Well, should we fix the code or the documentation?

I'd agree with fixing the code.  Early lock release is something we do on
system catalog accesses, and while it hasn't bitten us yet, I've been
kind of expecting that someday it will.  We should not do it on SQL-driven
accesses to user tables.

Having said that, I'd vote for just changing it in HEAD, not
back-patching.  It's not clear that there are consequences bad enough
to merit a back-patched behavior change.

            regards, tom lane

Re: COPY, lock release and MVCC

From
Laurenz Albe
Date:
On Tue, 2020-05-12 at 11:50 -0400, Tom Lane wrote:
> Laurenz Albe <laurenz.albe@cybertec.at> writes:
> > On Mon, 2020-05-11 at 15:43 -0400, Robert Haas wrote:
> > > On Fri, May 8, 2020 at 4:58 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> > > > I happened to notice that COPY TO releases the ACCESS SHARE lock
> > > > on the table right when the command ends rather than holding it
> > > > until the end of the transaction:
> > > That seems inconsistent with what an INSERT statement would do, and thus bad.
> > Well, should we fix the code or the documentation?
> 
> I'd agree with fixing the code.  Early lock release is something we do on
> system catalog accesses, and while it hasn't bitten us yet, I've been
> kind of expecting that someday it will.  We should not do it on SQL-driven
> accesses to user tables.
> 
> Having said that, I'd vote for just changing it in HEAD, not
> back-patching.  It's not clear that there are consequences bad enough
> to merit a back-patched behavior change.

Agreed.

Here is a patch.

Yours,
Laurenz Albe
Attachment

Re: COPY, lock release and MVCC

From
Amit Kapila
Date:
On Wed, May 13, 2020 at 1:20 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> On Tue, 2020-05-12 at 11:50 -0400, Tom Lane wrote:
> > Laurenz Albe <laurenz.albe@cybertec.at> writes:
> > > On Mon, 2020-05-11 at 15:43 -0400, Robert Haas wrote:
> > > > On Fri, May 8, 2020 at 4:58 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> > > > > I happened to notice that COPY TO releases the ACCESS SHARE lock
> > > > > on the table right when the command ends rather than holding it
> > > > > until the end of the transaction:
> > > > That seems inconsistent with what an INSERT statement would do, and thus bad.
> > > Well, should we fix the code or the documentation?
> >
> > I'd agree with fixing the code.  Early lock release is something we do on
> > system catalog accesses, and while it hasn't bitten us yet, I've been
> > kind of expecting that someday it will.  We should not do it on SQL-driven
> > accesses to user tables.
> >
> > Having said that, I'd vote for just changing it in HEAD, not
> > back-patching.  It's not clear that there are consequences bad enough
> > to merit a back-patched behavior change.
>
> Agreed.
>
> Here is a patch.
>

- /*
- * Close the relation. If reading, we can release the AccessShareLock we
- * got; if writing, we should hold the lock until end of transaction to
- * ensure that updates will be committed before lock is released.
- */
- if (rel != NULL)
- table_close(rel, (is_from ? NoLock : AccessShareLock));
+ table_close(rel, NoLock);

I wonder why you have removed (rel != NULL) check?  It can be NULL
when we use a query instead of a relation.  Refer below code:
DoCopy()
{
..
..
{
Assert(stmt->query);

query = makeNode(RawStmt);
query->stmt = stmt->query;
query->stmt_location = stmt_location;
query->stmt_len = stmt_len;

relid = InvalidOid;
rel = NULL;
}
..
}


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: COPY, lock release and MVCC

From
Laurenz Albe
Date:
On Wed, 2020-05-13 at 19:29 +0530, Amit Kapila wrote:
> > > > > On Fri, May 8, 2020 at 4:58 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> > > > > > I happened to notice that COPY TO releases the ACCESS SHARE lock
> > > > > > on the table right when the command ends rather than holding it
> > > > > > until the end of the transaction:
> > 
> > Here is a patch.
> > 
> 
> - /*
> - * Close the relation. If reading, we can release the AccessShareLock we
> - * got; if writing, we should hold the lock until end of transaction to
> - * ensure that updates will be committed before lock is released.
> - */
> - if (rel != NULL)
> - table_close(rel, (is_from ? NoLock : AccessShareLock));
> + table_close(rel, NoLock);
> 
> I wonder why you have removed (rel != NULL) check?

That was just unexcusable sloppiness, nothing more.

Here is a fixed patch.

Yours,
Laurenz Albe
Attachment

Re: COPY, lock release and MVCC

From
Amit Kapila
Date:
On Thu, May 14, 2020 at 2:06 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> > I wonder why you have removed (rel != NULL) check?
>
> That was just unexcusable sloppiness, nothing more.
>

LGTM.  I have slightly modified the commit message, see if that looks
fine to you.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachment

Re: COPY, lock release and MVCC

From
Laurenz Albe
Date:
On Thu, 2020-05-14 at 15:11 +0530, Amit Kapila wrote:
> LGTM.  I have slightly modified the commit message, see if that looks
> fine to you.

Fine with me, thanks.

> This breaks the cases where a REPEATABLE READ transaction could see an
> empty table if it repeats a COPY statement and somebody truncated the
> table in the meantime.

I would use "case" rather than "cases" here.

Yours,
Laurenz Albe

Re: COPY, lock release and MVCC

From
Amit Kapila
Date:
On Thu, May 14, 2020 at 7:10 PM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> On Thu, 2020-05-14 at 15:11 +0530, Amit Kapila wrote:
> > LGTM.  I have slightly modified the commit message, see if that looks
> > fine to you.
>
> Fine with me, thanks.
>
> > This breaks the cases where a REPEATABLE READ transaction could see an
> > empty table if it repeats a COPY statement and somebody truncated the
> > table in the meantime.
>
> I would use "case" rather than "cases" here.
>

Okay, changed, and pushed.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: COPY, lock release and MVCC

From
Laurenz Albe
Date:
On Fri, 2020-05-15 at 10:11 +0530, Amit Kapila wrote:
> Okay, changed, and pushed.

Thank you!

Yours,
Laurenz Albe