On Mon, Aug 21, 2017 at 07:41:52AM +0100, Simon Riggs wrote:
> On 19 August 2017 at 20:54, Noah Misch <n...@leadboat.com> wrote:
> > On Tue, Dec 06, 2016 at 01:59:18PM -0500, Robert Haas wrote:
> >> On Tue, Dec 6, 2016 at 1:36 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> >> > Robert Haas <robertmh...@gmail.com> writes:
> >> >> On Tue, Nov 15, 2016 at 3:55 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> >> >>> Account for catalog snapshot in PGXACT->xmin updates.
> >> >
> >> >> It seems to me that this makes it possible for
> >> >> ThereAreNoPriorRegisteredSnapshots() to fail spuriously. The only
> >> >> core code that calls that function is in copy.c, and apparently we
> >> >> never reach that point with the catalog snapshot set. But that seems
> >> >> fragile.
> >
> > I recently noticed this by way of the copy2 regression test failing, in 9.4
> > and later, under REPEATABLE READ and SERIALIZABLE. That failure started
> > with
> > $SUBJECT.
> > Fair summary. ThereAreNoPriorRegisteredSnapshots() has functioned that way
> > since an early submission[1], and I don't see that we ever discussed it
> > explicitly. Adding Simon for his recollection.
>
> The intention, IIRC, was to allow the current snapshot (i.e. 1) but no
> earlier (i.e. prior) snapshots (so not >=2)
>
> The name was chosen so it was (hopefully) clear what it did --
> ThereAreNoPriorRegisteredSnapshots
I now understand the function name, so I added a comment but didn't rename it.
I plan to use the attached patch after the minor release tags land. If
there's significant support, I could instead push before the wrap.
commit d68eed7 (HEAD, ThereAreNoPriorRegisteredSnapshots-CatalogSnapshot)
Author: Noah Misch <n...@leadboat.com>
AuthorDate: Sat Nov 4 00:08:04 2017 -0700
Commit: Noah Misch <n...@leadboat.com>
CommitDate: Sat Nov 4 00:33:37 2017 -0700
Ignore CatalogSnapshot when checking COPY FREEZE prerequisites.
This restores the ability, essentially lost in commit
ffaa44cb559db332baeee7d25dedd74a61974203, to use COPY FREEZE under
REPEATABLE READ isolation. Back-patch to 9.4, like that commit.
Discussion:
https://postgr.es/m/ca+tgmoahwdm-7fperbxzu9uz99lpmumepsxltw9tmrogzwn...@mail.gmail.com
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 8006df3..1bdd492 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2394,13 +2394,25 @@ CopyFrom(CopyState cstate)
/*
* Optimize if new relfilenode was created in this subxact or one of its
* committed children and we won't see those rows later as part of an
- * earlier scan or command. This ensures that if this subtransaction
- * aborts then the frozen rows won't be visible after xact cleanup. Note
+ * earlier scan or command. The subxact test ensures that if this
subxact
+ * aborts then the frozen rows won't be visible after xact cleanup.
Note
* that the stronger test of exactly which subtransaction created it is
- * crucial for correctness of this optimization.
+ * crucial for correctness of this optimization. The test for an earlier
+ * scan or command tolerates false negatives. FREEZE causes other
sessions
+ * to see rows they would not see under MVCC, and a false negative
merely
+ * spreads that anomaly to the current session.
*/
if (cstate->freeze)
{
+ /*
+ * Tolerate one registration for the benefit of
FirstXactSnapshot.
+ * Scan-bearing queries generally create at least two
registrations,
+ * though relying on that is fragile, as is ignoring
ActiveSnapshot.
+ * Clear CatalogSnapshot to avoid counting its registration.
We'll
+ * still detect ongoing catalog scans, each of which separately
+ * registers the snapshot it uses.
+ */
+ InvalidateCatalogSnapshot();
if (!ThereAreNoPriorRegisteredSnapshots() ||
!ThereAreNoReadyPortals())
ereport(ERROR,
(errcode(ERRCODE_INVALID_TRANSACTION_STATE),
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 294ab70..addf87d 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -1645,6 +1645,14 @@ DeleteAllExportedSnapshotFiles(void)
FreeDir(s_dir);
}
+/*
+ * ThereAreNoPriorRegisteredSnapshots
+ * Is the registered snapshot count less than or equal to one?
+ *
+ * Don't use this to settle important decisions. While zero registrations and
+ * no ActiveSnapshot would confirm a certain idleness, the system makes no
+ * guarantees about the significance of one registered snapshot.
+ */
bool
ThereAreNoPriorRegisteredSnapshots(void)
{
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
