Re: Something is broken about connection startup

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Something is broken about connection startup
Date: 2016-11-14 21:42:58
Message-ID: 12785.1479159778@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I assume you're going to back-patch this, and the consequences of
>> failing to reset it before going idle could easily be vastly worse
>> than the problem you're trying to fix. So I'd rather not make
>> assumptions like "the client will probably never sleep between Bind
>> and Execute". I bet that's actually false, and I certainly wouldn't
>> want to bet the farm on it being true.

> No, I'm not at all proposing to assume that. But I may be willing to
> assume that "we don't hold a CatalogSnapshot between Bind and Execute
> unless we're also holding a transaction snapshot". I need to do a bit
> more research to see if that's actually true, though.

Turns out it's not true.

I still don't much like having the main loop in PostgresMain know about
this hack, so I experimented with putting the InvalidateCatalogSnapshot()
calls into places in postgres.c that were already dealing with transaction
commit/abort or snapshot management. I ended up needing five such calls
(as in the attached patch), which is just about equally ugly. So at this
point I'm inclined to hold my nose and stick a call into step (1) of the
main loop instead.

Also, wherever we end up putting those calls, is it worth providing a
variant invalidation function that only kills the catalog snapshot when
it's the only one outstanding? (If it isn't, the transaction snapshot
should be older, so there's no chance of advancing our xmin by killing
it.) In principle this would save some catalog snapshot rebuilds for
inside-a-transaction-block cases, but I'm not sure it's worth sweating
that when we're doing client message exchange anyway.

Lastly, I find myself disliking the separate CatalogSnapshotStale flag
variable. The other special snapshots in snapmgr.c are managed by setting
the pointer to NULL when it's not valid, so I wonder why CatalogSnapshot
wasn't done that way. Since this patch is touching almost every use of
that flag already, it wouldn't take much to switch it over.

Comments?

regards, tom lane

Attachment Content-Type Size
register-catalog-snapshot-1.patch text/x-diff 5.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2016-11-14 21:43:07 Re: [PATCH] Allow TAP tests to be run individually
Previous Message Greg Stark 2016-11-14 20:43:12 Re: Physical append-only tables