Re: Something is broken about connection startup

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Nov 11, 2016 at 3:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The basic problem here, therefore, is that SnapshotResetXmin isn't aware
>> that GetCatalogSnapshot is keeping a possibly-unregistered snapshot in
>> its hip pocket.  That has to change.  We could either treat the saved
>> CatalogSnapshot as always being registered, or we could add some logic
>> to force invalidating the CatalogSnapshot whenever we clear MyPgXact->xmin
>> or advance it past that snapshot's xmin.
>> 
>> Neither of those is very nice from a performance standpoint.  With the
>> first option we risk delaying global cleanup by holding back
>> MyPgXact->xmin to protect a CatalogSnapshot we might not actually use
>> anytime soon.  With the second option we will likely end up doing many
>> more GetSnapshotData calls than we do now, because in a transaction
>> that hasn't yet set a regular snapshot, we will need to get a new
>> CatalogSnapshot for every catalog access (since we'll go back to
>> MyPgXact->xmin = 0 after every access).  And the parser, in particular,
>> tends to do a lot of catalog accesses before we ever get a regular
>> transaction snapshot.
>> 
>> Ideally, perhaps, we'd treat the saved CatalogSnapshot as registered
>> but automatically kill it "every so often".  I'm not sure how to drive
>> that though.

> I think the really important thing is that we don't leave xmin set
> when the backend is idle.

Agreed.  I did some stats-gathering on this over the weekend, seeing how
often the various situations occur during the core regression tests.
It's not as big a problem as I first thought, because we hold a snapshot
while doing planning, but the case does arise for various utility commands
(particularly LOCK TABLE) so it's not negligible.

What is looking like the best answer to me is to treat CatalogSnapshot
as registered, but forcibly unregister it before going idle.  We don't
need to unregister it more often than that, because we'd hold a
transaction snapshot of some description throughout any long-running
command, and the CatalogSnapshot would be no older than that anyway.

"Before going idle" could be implemented with an InvalidateCatalogSnapshot
call either in postgres.c's finish_xact_command(), or right in the main
message-receipt loop.  The latter seems like a wart, but it would cover
cases like a client going to sleep between Bind and Execute phases of an
extended query.  On the other hand, I think we might be holding a
transaction snapshot at that point anyway.  It's at least arguable that
we should be going through finish_xact_command() at any point where it's
likely that we have a snapshot to release.

Thoughts?
        regards, tom lane