Re: [HACKERS] [COMMITTERS] pgsql: Account for catalog snapshot in PGXACT->xmin updates.

On 19 August 2017 at 20:54, Noah Misch <noah@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 <tgl@sss.pgh.pa.us> wrote:
>> > Robert Haas <robertmhaas@gmail.com> writes:
>> >> On Tue, Nov 15, 2016 at 3:55 PM, Tom Lane <tgl@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.  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 Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services