Thread: hot standby - further cleanup of recovery procs stuff

I've made a few further cleanups to the hot standby patch:

- UnobservedTransactionsRemoveXids() had an argument called
missing_in_error which was always set to false.  So I removed the
argument and the elog().
[It's an interesting question whether this should be considered an
error, but if we're always going to pass false there's no point in
having the check.]
- Fix compiler warnings in ProcArrayDisplay().  On my system this
generated too: one because index was used without initializing it, and
a second because there was no function prototype.  index was only used
once; I think it was intended to be the same as xid_index, so I merged
them.
- Reverted all the changes to ProcArrayAdd() and ProcArrayRemove() as
compared with CVS HEAD.  Now that RecoveryProcs are gone, none of this
looks to be necessary.
- Modified CreateSharedProcArray() to no longer add MaxBackends to
procArray->maxProcs twice.  This appears to be another RecoveryProcs
holdover.
- Adjusted a few comments that previously referred to recovery procs,
and reverted a few other semantically unimportant changes vs. CVS
HEAD.

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.

Revised patch updated and pushed to my git repo.

...Robert

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