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

On Tue, Dec 6, 2016 at 1:36 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Tue, Nov 15, 2016 at 3:55 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.
>
> 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