Robert Haas wrote:
> I've made a few further cleanups to the hot standby patch:
Thanks!
> I am not sure why we have a single GUC to size both the number of
> PGPROC structures we allow and the size of UnobservedXids. A
> read-only slave might only need to allow a few connections for
> reporting purposes, while the master needs to allow many.
Yeah, it is true that the two don't necessarily have much in common. We
could well make it a separate GUC. We'd still need to find a reasonable
default, though.
It should be noted that UnobservedXids array is allocated in shared
memory, but goes completely unused after the recovery, becoming a waste
of memory. It's only a few hundred kB at most, so I think that's
acceptable, but it would be nice to be able to release that memory
somehow. Perhaps it should be backed with a file, which would also have
the benefit that it could grow as needed, eliminating the need for the
GUC. Storing it in a new SLRU might be a good idea.
I started to look at the subtrans.c changes. The patch changes the role
of pg_subtrans substantially. It is no longer simply cleared at startup,
but we require it to contain valid data when we start recovery for
transactions that have overflowed the in-memory cache. That makes me a
bit uncomfortable, although I don't see anything obviously wrong with
it. The comment in CheckpointSUBTRANS() claiming that flushing
pg_subtrans is just a debugging aid is now wrong, however.
I think there's also a bug in ExtendSUBTRANS(): it will zap the first
page it touches in recovery, but right after we start recovery, and
replay the first RunningXacts WAL record, we need to have pg_subtrans
correctly set for the transactions in that RunningXacts record (that
have overflowed the in memory subxid cache). Zapping the pg_subtrans
page can destroy that information.
-- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com