Re: Commits 8de72b and 5457a1 (COPY FREEZE) - Mailing list pgsql-hackers

From Simon Riggs
Subject Re: Commits 8de72b and 5457a1 (COPY FREEZE)
Date
Msg-id CA+U5nM+SwT2Y95ALw1m4OHQpCpePMnJiT3OH_rXhXNVBS1CtsA@mail.gmail.com
Whole thread Raw
In response to Re: Commits 8de72b and 5457a1 (COPY FREEZE)  (Jeff Davis <pgsql@j-davis.com>)
List pgsql-hackers
On 4 December 2012 20:38, Jeff Davis <pgsql@j-davis.com> wrote:

> The simple case of BEGIN; CREATE TABLE ...; COPY ... WITH (FREEZE);
> doesn't meet the pre-conditions. It only meets the conditions if
> preceded by a TRUNCATE, which all of the tests do. I looked into it, and
> I think the test:
>
>   ... &&
>   cstate->rel->rd_newRelfilenodeSubid == GetCurrentSubTransactionId()
>
> should be:
>
>     ... &&
>  (cstate->rel->rd_newRelfilenodeSubid == GetCurrentSubTransactionId() ||
>   cstate->rel->rd_createSubid == GetCurrentSubTransactionId())
>
> (haven't tested). Another option to consider is that
> rd_newRelfilenodeSubid could be set on newly-created tables as well as
> newly-truncated tables.

I'll expand the test above and add new regression. I focused on
correctness ahead of a wide use case and missed this.

> Also, I recommend a hint along with the NOTICE when the pre-conditions
> aren't met to tell the user what they need to do. Perhaps something
> like:
>
>    "Create or truncate the table in the same transaction as COPY
> FREEZE."

OK, will add hint.

> The documentation could expand on that, clarifying that a TRUNCATE in
> the same transaction (perhaps even ALTER?) allows a COPY FREEZE.
>
> The code comment could be improved a little, while we're at it:
>
>   "Note that the stronger test of exactly which subtransaction created
> it is crucial for correctness of this optimisation."
>
> is a little vague, can you explain using the reasoning from the thread?
> I would say something like:
>
>   "The transaction and subtransaction creating or truncating the table
> must match that of the COPY FREEZE. Otherwise, it could mix tuples from
> different transactions together, and it would be impossible to
> distinguish them for the purposes of atomicity."

OK, will try.

> After reading that thread, I still don't understand why it's unsafe to
> set HEAP_XMIN_COMMITTED in those conditions. Even if it is, I would
> think that a sufficiently narrow case -- such as CTAS outside of a
> transaction block -- would be safe, along with some slightly broader
> cases (like BEGIN; CREATE TABLE; INSERT/COPY).

It *could* be safe, but needs changes to visibility rules, which as
explained I had not been able to optimise sufficiently.

The code is easy enough to add back in at the time it is sufficiently
well optimised and we all agree.

-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services



pgsql-hackers by date:

Previous
From: Simon Riggs
Date:
Subject: Re: Commits 8de72b and 5457a1 (COPY FREEZE)
Next
From: Andres Freund
Date:
Subject: Re: Commits 8de72b and 5457a1 (COPY FREEZE)