Re: AtEOXact_Snapshot timing - Mailing list pgsql-hackers

Hi,

On 2019-09-06 10:25:22 -0400, Robert Haas wrote:
> On Thu, Sep 5, 2019 at 5:32 PM Andres Freund <andres@anarazel.de> wrote:
> > On 2019-09-05 14:50:50 -0400, Robert Haas wrote:
> > > The best way that I've been able to come up with to enforce this rule
> > > after a little bit of thought is to add Assert(IsTransactionState())
> > > to a bunch of functions in snapmgr.c, most notably
> > > GetTransactionSnapshot and GetCatalogSnapshot.
> >
> > Wonder if there's a risk that callers might still have a snapshot
> > around, in independent memory?  It could make sense to add such an
> > assert to a visibility routine or two, maybe?
> >
> > I suspect that we should add non-assert condition to a central place
> > like GetSnapshotData(). It's not hard to imagine extensions just using
> > that directly, and that we'd never notice that with assert only
> > testing. It's also hard to imagine a single if
> > (unlikely(IsTransactionState())) to be expensive enough to matter
> > compared to GetSnapshotData()'s own cost.
>
> I guess we could do that, but I feel like it might be overkill.  It
> seems unlikely to me that an extension would call GetSnapshotData()
> directly, but if it does, it's probably some kind of advanced wizardry
> and I'm happy to trust that the extension author knows what she's
> doing (or she can keep both pieces if it breaks).

Hm. I feel like there's plenty reasons to get a snapshot in extensions -
there's plenty APIs one cannot really call without doing so? What I'm
worried about is not primarily that GetSnapshotData() is being called
directly, but that $author got a snapshot previously, and then tries to
use it in an xact callback or such.

I'd add asserts to at least PushActiveSnapshot(), and I don't see the
harm in adding one to GetSnapshotData().


> For my $0.02, calling GetTransactionSnapshot() in an aborted
> transaction is the kind of thing that's much more likely to be an
> unintentional goof.

Based on what I've seen people do in xact callback handlers... Didn't PG
itself e.g. have code doing syscache lookups in aborted transactions a
couple times?

The danger of using a previously acquired snapshot in that stage is why
I was pondering adding an assert to visibility functions
themselves. E.g. just adding one to HeapTupleSatisfiesVisibility() might
already add a good bit of coverage.


> I agree with you that it would be nicer if we could put the killing of
> the old snapshots directly adjacent to ProcArrayEndTransaction(), and
> that was the first thing I tried, but it doesn't work, because
> resource owner cleanup has to run first.

Hm. I'd even say that it actually belongs to *before* the
ProcArrayEndTransaction() call.


For a bit I wondered if the resowner snapshot cleanup ought to be at
least moved to RESOURCE_RELEASE_BEFORE_LOCKS. Not that it actually
addresses this issue, but it seems to belong there "thematically". But
then I honestly don't understand why most of the resowner managed
resources in the abort sequence are released where they are.  The only
really explanatory comment is:

     * The ordering of operations is not entirely random.  The idea is:
     * release resources visible to other backends (eg, files, buffer pins);
     * then release locks; then release backend-local resources. We want to
     * release locks at the point where any backend waiting for us will see
     * our transaction as being fully cleaned up.

but that doesn't explain why we e.g. process relcache references, jit
contexts (arguably it does for dsm segments), at that stage. And
definitely not why we do abort's relcache inval processing between
RESOURCE_RELEASE_BEFORE_LOCKS and RESOURCE_RELEASE_LOCKS - that can be
quite expensive whe needing to scan the whole relcache.

Anyway, this is grumbling about things far beyond the scope of this
patch.


> It's possible that we could
> split AtEOXact_Snapshot() into two pieces that run at different times,
> but I think we'd be guarding against a vulnerability that is mostly
> theoretical, and I don't really see the point.  The only kind of
> access that somebody's likely to attempt during AbortTransaction() is
> catalog access, and that's likely to trip either the assertion I added
> in GetCatalogSnapshot() or the existing one in SearchCatCache().

I'm not sure that's true - I've certainly seen extensions logging the
transaction state into a table, for example... Even in aborted xacts.


> Non-catalog access would probably fail the assertion in
> GetTransactionSnapshot() or GetLatestSnapshot().

Not this patch's fault, obviously, but none of this appears to catch
DML...


> I might be missing something here, so feel free to point out to me if
> there's a plausible coding pattern I'm missing where the things you
> are proposing would actually be a problem.  To me, it just feels like
> you're worried about a scenario that would require writing the code in
> a very unnatural way, like saving a snapshot in a global variable or
> calling GetSnapshotData() directly.

Well, if you actually want to look at state as it was at the beginning
of the database, it seems not unreasonable to get a snapshot at the
start of the transaction, push it onto the stack, and then use it at the
end of the transaction, too.


Just to be clear: While I think an assert or two more seem like a good
idea, that's musing around the edges, not a fundamental concern.


> diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
> index f594d33e7a..9993251607 100644
> --- a/src/backend/access/transam/xact.c
> +++ b/src/backend/access/transam/xact.c
> @@ -2732,6 +2732,18 @@ AbortTransaction(void)
>          pgstat_report_xact_timestamp(0);
>      }
>
> +    /*
> +     * Any snapshots taken by this transaction were unsafe to use even at the
> +     * time when we entered AbortTransaction(), since we might have, for
> +     * example, inserted a heap tuple and failed while inserting index tuples.
> +     * They were even more unsafe after ProcArrayEndTransaction(), since after
> +     * that point tuples we inserted could be pruned by other backends.
> +     * However, we postpone the cleanup until this point in the sequence
> +     * because it has to be done after ResourceOwnerRelease() has finishing
> +     * unregistering snapshots.
> +     */
> +    AtEOXact_Snapshot(false, true);

One thing that bothers me a bit here is that the other cleanup calls are
within

    /*
     * Post-abort cleanup.  See notes in CommitTransaction() concerning
     * ordering.  We can skip all of it if the transaction failed before
     * creating a resource owner.
     */
    if (TopTransactionResourceOwner != NULL)

and adding AtEOXact_Snapshot() below that block kind of makes that
comment wrong. I don't think we actually can make the call conditional
in the same way, however.


>      /*
>       * State remains TRANS_ABORT until CleanupTransaction().
>       */
> @@ -2757,7 +2769,6 @@ CleanupTransaction(void)
>       * do abort cleanup processing
>       */
>      AtCleanup_Portals();        /* now safe to release portal memory */
> -    AtEOXact_Snapshot(false, true); /* and release the transaction's snapshots */

Hm. I don't quite get why we tell AtEOXact_Snapshot() to clean up
->xmin, when we just called ProcArrayEndTransaction(). Again, not this
patch's fault...


Greetings,

Andres Freund