Horton

Thread

Commits

  1. Make COPY TO keep locks until the transaction end.

  2. COPY to a relation should keep write lock till transaction commit.

  3. Mega-commit to make heap_open/heap_openr/heap_close take an

  1. COPY, lock release and MVCC

    Laurenz Albe <laurenz.albe@cybertec.at> — 2020-05-08T08:58:24Z

    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

  2. Re: COPY, lock release and MVCC

    Robert Haas <robertmhaas@gmail.com> — 2020-05-11T19:43:17Z

    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

  3. Re: COPY, lock release and MVCC

    Laurenz Albe <laurenz.albe@cybertec.at> — 2020-05-12T15:37:46Z

    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

  4. Re: COPY, lock release and MVCC

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-12T15:50:10Z

    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

  5. Re: COPY, lock release and MVCC

    Laurenz Albe <laurenz.albe@cybertec.at> — 2020-05-12T19:50:40Z

    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

  6. Re: COPY, lock release and MVCC

    Amit Kapila <amit.kapila16@gmail.com> — 2020-05-13T13:59:14Z

    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

  7. Re: COPY, lock release and MVCC

    Laurenz Albe <laurenz.albe@cybertec.at> — 2020-05-13T20:36:40Z

    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

  8. Re: COPY, lock release and MVCC

    Amit Kapila <amit.kapila16@gmail.com> — 2020-05-14T09:41:34Z

    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

  9. Re: COPY, lock release and MVCC

    Laurenz Albe <laurenz.albe@cybertec.at> — 2020-05-14T13:40:44Z

    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

  10. Re: COPY, lock release and MVCC

    Amit Kapila <amit.kapila16@gmail.com> — 2020-05-15T04:41:12Z

    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

  11. Re: COPY, lock release and MVCC

    Laurenz Albe <laurenz.albe@cybertec.at> — 2020-05-15T05:04:55Z

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

Thank you!

Yours,
Laurenz Albe