Re: Fast COPY after TRUNCATE bug and fix

It's been pointed out to me that I introduced a bug as part of the
recent optimisation of COPY-after-truncate.

The attached patch fixes this for me on CVS HEAD. It does this by making
an explicit request for relcache hint cleanup at EOXact and takes a more
cautious approach during RelationCacheInvalidate().

Please can this be reviewed as soon as possible? Thanks.

TRUNCATE was setting a flag to show that it had created a new
relfilenode, but the flag was not cleared in all cases. This lead to a
COPY that followed a truncation, yet was in a *separate* transaction
from it and in a transaction on its own, to apparently lose data. The
data loss was caused because the COPY inadvertently avoided writing WAL,
which then led to skipping the recording of transaction commit, leaving
the inserted rows showing as aborted.

The failing test case was:

TRUNCATE foo;
COPY foo FROM ....;
SELECT count(*) FROM foo;

The returned count should be non-zero if the COPY succeeds, yet on CVS
HEAD this currently returns 0.

CLUSTER is not affected by this change, AFAICS, because its change of
relfilenode doesn't wait until EOXact, so COPY doesn't optimise after a
CLUSTER-in-same-trans.

Thanks to various EDB colleagues for bringing this to my attention.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---------------------------------------------------------------------------

Simon Riggs wrote:
> It's been pointed out to me that I introduced a bug as part of the
> recent optimisation of COPY-after-truncate.
>
> The attached patch fixes this for me on CVS HEAD. It does this by making
> an explicit request for relcache hint cleanup at EOXact and takes a more
> cautious approach during RelationCacheInvalidate().
>
> Please can this be reviewed as soon as possible? Thanks.
>
> TRUNCATE was setting a flag to show that it had created a new
> relfilenode, but the flag was not cleared in all cases. This lead to a
> COPY that followed a truncation, yet was in a *separate* transaction
> from it and in a transaction on its own, to apparently lose data. The
> data loss was caused because the COPY inadvertently avoided writing WAL,
> which then led to skipping the recording of transaction commit, leaving
> the inserted rows showing as aborted.
>
> The failing test case was:
>
> TRUNCATE foo;
> COPY foo FROM ....;
> SELECT count(*) FROM foo;
>
> The returned count should be non-zero if the COPY succeeds, yet on CVS
> HEAD this currently returns 0.
>
> CLUSTER is not affected by this change, AFAICS, because its change of
> relfilenode doesn't wait until EOXact, so COPY doesn't optimise after a
> CLUSTER-in-same-trans.
>
> Thanks to various EDB colleagues for bringing this to my attention.
>
> --
> Simon Riggs
> EnterpriseDB http://www.enterprisedb.com
>

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Wed, 2007-02-28 at 21:09 -0500, Bruce Momjian wrote:
> Your patch has been added to the PostgreSQL unapplied patches list at:

> Simon Riggs wrote:
> > It's been pointed out to me that I introduced a bug as part of the
> > recent optimisation of COPY-after-truncate.
> >
> > The attached patch fixes this for me on CVS HEAD. It does this by making
> > an explicit request for relcache hint cleanup at EOXact and takes a more
> > cautious approach during RelationCacheInvalidate().

You understand that this fixes a bug in CVS HEAD?

It isn't a new feature.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

Simon Riggs wrote:
> On Wed, 2007-02-28 at 21:09 -0500, Bruce Momjian wrote:
> > Your patch has been added to the PostgreSQL unapplied patches list at:
>
> > Simon Riggs wrote:
> > > It's been pointed out to me that I introduced a bug as part of the
> > > recent optimisation of COPY-after-truncate.
> > >
> > > The attached patch fixes this for me on CVS HEAD. It does this by making
> > > an explicit request for relcache hint cleanup at EOXact and takes a more
> > > cautious approach during RelationCacheInvalidate().
>
> You understand that this fixes a bug in CVS HEAD?
>
> It isn't a new feature.

This is for CVS HEAD only, right? Fixed still go into the queue, but
for a shorter amount of time, hopefully.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

what is the point of this?:

+ void
+ RelationCacheResetAtEOXact(void)
+ {
+ need_eoxact_work = true;
+ }

and why is it declared extern in relcache.h when it is only used in
relcache.c?

ISTM that there isn't much reason to un-inline the statement, and the
patch could be a lot smaller without it.

cheers

andrew

Bruce Momjian wrote:
> Your patch has been added to the PostgreSQL unapplied patches list at:
>
> http://momjian.postgresql.org/cgi-bin/pgpatches
>
> It will be applied as soon as one of the PostgreSQL committers reviews
> and approves it.
>
> ---------------------------------------------------------------------------
>
>
> Simon Riggs wrote:
>
>> It's been pointed out to me that I introduced a bug as part of the
>> recent optimisation of COPY-after-truncate.
>>
>> The attached patch fixes this for me on CVS HEAD. It does this by making
>> an explicit request for relcache hint cleanup at EOXact and takes a more
>> cautious approach during RelationCacheInvalidate().
>>
>> Please can this be reviewed as soon as possible? Thanks.
>>
>> TRUNCATE was setting a flag to show that it had created a new
>> relfilenode, but the flag was not cleared in all cases. This lead to a
>> COPY that followed a truncation, yet was in a *separate* transaction
>> from it and in a transaction on its own, to apparently lose data. The
>> data loss was caused because the COPY inadvertently avoided writing WAL,
>> which then led to skipping the recording of transaction commit, leaving
>> the inserted rows showing as aborted.
>>
>> The failing test case was:
>>
>> TRUNCATE foo;
>> COPY foo FROM ....;
>> SELECT count(*) FROM foo;
>>
>> The returned count should be non-zero if the COPY succeeds, yet on CVS
>> HEAD this currently returns 0.
>>
>> CLUSTER is not affected by this change, AFAICS, because its change of
>> relfilenode doesn't wait until EOXact, so COPY doesn't optimise after a
>> CLUSTER-in-same-trans.
>>
>> Thanks to various EDB colleagues for bringing this to my attention.
>>
>> --
>> Simon Riggs
>> EnterpriseDB http://www.enterprisedb.com
>>
>>
>
> [ Attachment, skipping... ]
>
>
>> ---------------------------(end of broadcast)---------------------------
>> TIP 1: if posting/reading through Usenet, please send an appropriate
>> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
>> message can get through to the mailing list cleanly
>>
>
>

I'm sorry. I misread the patch. I now see it used in index.c. return to
normal viewing ...

cheers

andrew

Andrew Dunstan wrote:
>
> what is the point of this?:
>
> + void
> + RelationCacheResetAtEOXact(void)
> + {
> + need_eoxact_work = true;
> + }
>
>
>
> and why is it declared extern in relcache.h when it is only used in
> relcache.c?
>
> ISTM that there isn't much reason to un-inline the statement, and the
> patch could be a lot smaller without it.
>
> cheers
>
> andrew
>
> Bruce Momjian wrote:
>> Your patch has been added to the PostgreSQL unapplied patches list at:
>>
>> http://momjian.postgresql.org/cgi-bin/pgpatches
>>
>> It will be applied as soon as one of the PostgreSQL committers reviews
>> and approves it.
>>
>> ---------------------------------------------------------------------------
>>
>>
>>
>> Simon Riggs wrote:
>>
>>> It's been pointed out to me that I introduced a bug as part of the
>>> recent optimisation of COPY-after-truncate.
>>> The attached patch fixes this for me on CVS HEAD. It does this by
>>> making
>>> an explicit request for relcache hint cleanup at EOXact and takes a
>>> more
>>> cautious approach during RelationCacheInvalidate().
>>>
>>> Please can this be reviewed as soon as possible? Thanks.
>>>
>>> TRUNCATE was setting a flag to show that it had created a new
>>> relfilenode, but the flag was not cleared in all cases. This lead to a
>>> COPY that followed a truncation, yet was in a *separate* transaction
>>> from it and in a transaction on its own, to apparently lose data. The
>>> data loss was caused because the COPY inadvertently avoided writing
>>> WAL,
>>> which then led to skipping the recording of transaction commit, leaving
>>> the inserted rows showing as aborted.
>>>
>>> The failing test case was:
>>>
>>> TRUNCATE foo;
>>> COPY foo FROM ....;
>>> SELECT count(*) FROM foo;
>>>
>>> The returned count should be non-zero if the COPY succeeds, yet on CVS
>>> HEAD this currently returns 0.
>>>
>>> CLUSTER is not affected by this change, AFAICS, because its change of
>>> relfilenode doesn't wait until EOXact, so COPY doesn't optimise after a
>>> CLUSTER-in-same-trans.
>>>
>>> Thanks to various EDB colleagues for bringing this to my attention.
>>>
>>> --
>>> Simon Riggs EnterpriseDB http://www.enterprisedb.com
>>>
>>>
>>
>> [ Attachment, skipping... ]
>>
>>
>>> ---------------------------(end of
>>> broadcast)---------------------------
>>> TIP 1: if posting/reading through Usenet, please send an appropriate
>>> subscribe-nomail command to majordomo(at)postgresql(dot)org so that
>>> your
>>> message can get through to the mailing list cleanly
>>>
>>
>>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: don't forget to increase your free space map settings
>

On Thu, 2007-03-01 at 09:12 -0500, Andrew Dunstan wrote:
> what is the point of this?:
>
> + void
> + RelationCacheResetAtEOXact(void)
> + {
> + need_eoxact_work = true;
> + }
>
>
>
> and why is it declared extern in relcache.h when it is only used in
> relcache.c?

It is called from index.c and relcache.c, hence it is extern.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

Simon Riggs wrote:
> On Thu, 2007-03-01 at 09:12 -0500, Andrew Dunstan wrote:
>
>> what is the point of this?:
>>
>> + void
>> + RelationCacheResetAtEOXact(void)
>> + {
>> + need_eoxact_work = true;
>> + }
>>
>>
>>
>> and why is it declared extern in relcache.h when it is only used in
>> relcache.c?
>>
>
> It is called from index.c and relcache.c, hence it is extern.
>
>
Yeah. I noticed just after I hit the send button ... sorry.

cheers

andrew

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Simon Riggs wrote:
>> You understand that this fixes a bug in CVS HEAD?

> This is for CVS HEAD only, right? Fixed still go into the queue, but
> for a shorter amount of time, hopefully.

It still needs review ... and the presence of the patch in the queue
reminds me that I never really reviewed the original patch. Which now
proves to have been a mistake.

regards, tom lane

Patch applied. Thanks.

---------------------------------------------------------------------------

Simon Riggs wrote:
> It's been pointed out to me that I introduced a bug as part of the
> recent optimisation of COPY-after-truncate.
>
> The attached patch fixes this for me on CVS HEAD. It does this by making
> an explicit request for relcache hint cleanup at EOXact and takes a more
> cautious approach during RelationCacheInvalidate().
>
> Please can this be reviewed as soon as possible? Thanks.
>
> TRUNCATE was setting a flag to show that it had created a new
> relfilenode, but the flag was not cleared in all cases. This lead to a
> COPY that followed a truncation, yet was in a *separate* transaction
> from it and in a transaction on its own, to apparently lose data. The
> data loss was caused because the COPY inadvertently avoided writing WAL,
> which then led to skipping the recording of transaction commit, leaving
> the inserted rows showing as aborted.
>
> The failing test case was:
>
> TRUNCATE foo;
> COPY foo FROM ....;
> SELECT count(*) FROM foo;
>
> The returned count should be non-zero if the COPY succeeds, yet on CVS
> HEAD this currently returns 0.
>
> CLUSTER is not affected by this change, AFAICS, because its change of
> relfilenode doesn't wait until EOXact, so COPY doesn't optimise after a
> CLUSTER-in-same-trans.
>
> Thanks to various EDB colleagues for bringing this to my attention.
>
> --
> Simon Riggs
> EnterpriseDB http://www.enterprisedb.com
>

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +