COPY, lock release and MVCC
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
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
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
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
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
Attachments:
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
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
Attachments:
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
Attachments:
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
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