Re: [HACKERS] [COMMITTERS] pgsql: Account for catalog snapshot in PGXACT->xmin updates.
Noah Mischwrites: > 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. (which reminds me, I'd better get on with making release notes.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Account for catalog snapshot in PGXACT->xmin updates.
On 19 August 2017 at 20:54, Noah Mischwrote: > 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. 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 ...but that was before we had MVCC scans on catalogs, which changed things in unforeseen ways. The right fix is surely to do a more principled approach and renaming of the function so that it reflects the current snapshot types. As Noah mentions there are potential anomalies in other transactions but this was understood and hence why we have a specific command keyword to acknowledge user acceptance of these behaviours. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Account for catalog snapshot in PGXACT->xmin updates.
On Tue, Dec 6, 2016 at 1:36 PM, Tom Lanewrote: > 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. > > 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 > 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 > 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 pretty clear to me. What are you confused about? It seems quite obvious to me that no matter how old the catalog snapshot is, it's nothing that we need to worry about here because it'll get invalidated and refreshed before use if anything changes meanwhile. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Account for catalog snapshot in PGXACT->xmin updates.
Robert Haaswrites: > 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. 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 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 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. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Account for catalog snapshot in PGXACT->xmin updates.
On Tue, Nov 15, 2016 at 3:55 PM, Tom Lanewrote: > Account for catalog snapshot in PGXACT->xmin updates. > > The CatalogSnapshot was not plugged into SnapshotResetXmin()'s accounting > for whether MyPgXact->xmin could be cleared or advanced. In normal > transactions this was masked by the fact that the transaction snapshot > would be older, but during backend startup and certain utility commands > it was possible to re-use the CatalogSnapshot after MyPgXact->xmin had > been cleared, meaning that recently-deleted rows could be pruned even > though this snapshot could still see them, causing unexpected catalog > lookup failures. This effect appears to be the explanation for a recent > failure on buildfarm member piculet. > > To fix, add the CatalogSnapshot to the RegisteredSnapshots heap whenever > it is valid. 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers