[HACKERS] Re: [COMMITTERS] pgsql: Account for catalog snapshot in PGXACT->xmin updates.
On Sat, Nov 04, 2017 at 12:23:36PM -0400, Tom Lane wrote: > Noah Misch writes: > > 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. > > This looks fine to me --- I think you should push now. Done. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [COMMITTERS] pgsql: Account for catalog snapshot in PGXACT->xmin updates.
On Mon, Aug 21, 2017 at 07:41:52AM +0100, Simon Riggs wrote:
> On 19 August 2017 at 20:54, Noah Misch 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 wrote:
> >> > Robert Haas writes:
> >> >> On Tue, Nov 15, 2016 at 3:55 PM, Tom Lane 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
AuthorDate: Sat Nov 4 00:08:04 2017 -0700
Commit: Noah Misch
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
[HACKERS] Re: [COMMITTERS] pgsql: Account for catalog snapshot in PGXACT->xmin updates.
On Tue, Dec 06, 2016 at 01:59:18PM -0500, Robert Haas wrote:
> On Tue, Dec 6, 2016 at 1:36 PM, Tom Lane wrote:
> > Robert Haas writes:
> >> On Tue, Nov 15, 2016 at 3:55 PM, Tom Lane 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. Minimal example:
CREATE TABLE vistest (a text);
BEGIN ISOLATION LEVEL REPEATABLE READ;
TRUNCATE vistest;
COPY vistest FROM stdin CSV FREEZE;
x
\.
Before $SUBJECT, the registered snapshot count was one. Since $SUBJECT, the
catalog snapshot raises the count to two.
> > Hm. If there were some documentation explaining what
> > ThereAreNoPriorRegisteredSnapshots is supposed to do, it would be possible
> > for us to have a considered argument as to whether this patch broke it or
> > not. As things stand, it is not obvious whether it ought to be ignoring
> > the catalog snapshot or not; one could construct plausible arguments
The rows COPY FREEZE writes will be visible (until deleted) to every possible
catalog snapshot, so whether CatalogSnapshot==NULL doesn't matter. It may be
useful to error out if a catalog scan is in-flight, but the registration for
CatalogSnapshot doesn't confirm or refute that.
> > either way. It's not even very obvious why both 0 and 1 registered
> > snapshots should be considered okay; that looks a lot more like a hack
> > than reasoned-out behavior. So I'm satisfied if COPY FREEZE does what
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. I think the goal was to raise
an error if any scan of the COPY-target table might use an older CommandId;
this prevents a way of seeing tuples that the scan would not see after COPY
without FREEZE. Allowing one registered snapshot was a satisfactory heuristic
at the time. It rejected running plain queries, which have been registering
two snapshots. It did not reject otherwise-idle REPEATABLE READ transactions,
which register FirstXactSnapshot once; that has been okay, because any use of
FirstXactSnapshot in a query pushes or registers a copy. I think the code was
wrong to consider registered snapshots and ignore ActiveSnapshot, though. (We
must ignore COPY's own ActiveSnapshot, but it should be the only one.) I
don't blame $SUBJECT for changing this landscape; COPY should not attribute
meaning to a particular nonzero registered snapshot count.
> > it's supposed to do, which it still appears to do.
> >
> > IOW, I'm not interested in touching this unless someone first provides
> > adequate documentation for the pre-existing kluge here. There is no
> > API spec at all on the function itself, and the comment in front of the
> > one call site is too muddle-headed to provide any insight.
>
> COPY FREEZE is designed to be usable only in situations where the new
> tuples won't be visible earlier than would otherwise be the case.
> Thus, copy.c has a check that the relfilenode was created by our
> (sub)transaction, which guards against the possibility that other
> transactions might see the data early, and also a check that there are
> no other registered snapshots or ready portals, which guarantees that,
> for example, a cursor belonging to the current subtransaction but with
> a lower command-ID doesn't see the rows early. I am fuzzy why we need
> to check for both snapshots and portals, but the overall intent seems
The snapshot check helps in this case:
CREATE TABLE vistest (a text);
BEGIN ISOLATION LEVEL READ COMMITTED;
CREATE FUNCTION copy_vistest() RETURNS void LANGUAGE plpgsql
AS $$BEGIN COPY vistest FROM '/dev/null' CSV FREEZE; END$$;
CREATE FUNCTION count_vistest() RETURNS int LANGUAGE plpgsql STABLE
AS $$BEGIN RETURN (SELECT count(*) FROM vistest); END$$;
TRUNCATE vistest;
SELECT copy_vistest(), count_vistest();
ROLLBACK;
Other sessions still see rows they ideally would not see[2], the same kind of
anomaly the portal and snapshot tests exist to prevent. It's odd that we
protect visibility solely within the COPY transaction, the transaction most
likely to know what it's doing. I'm therefore skeptical of having these tests
at all, but I hesitate to remove them in back branches. Even in master, we
may want them later if we add visibility protection for other transactions.
I'm tempted to add this hack, ...
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2445,2 +2445,3 @@ CopyFrom(CopyState cstate)
{
+ InvalidateCatalogSnapshot();
