Re: Hot Standby 0.2.1

On Wed, 2009-09-23 at 11:13 +0300, Heikki Linnakangas wrote:

> Looking at the way cache invalidations are handled in two-phase
> transactions, it would be simpler if we store the shared cache
> invalidation messages in the twophase state file header, like we store
> deleted relations and subxids. This allows them to be copied to the
> COMMIT_PREPARED WAL record, so that we don't need treat twophase commits
> differently than regular ones in xact_redo_commit. As the patch stands,
> the new
> xactGetCommittedInvalidationMessages/MakeSharedInvalidMessagesArray
> mechanism is duplicated functionality with
> AtPrepare_Inval/-PersistInvalidationMessage - both materialize the
> pending shared invalidation messages so that they can be written to
> disk. I did that in my git branch.

We could, but the prepared transaction path already contains special
case code anyway, so we aren't reducing number of test cases required.
This looks like a possible area for refactoring, but I don't see the
need for pre-factoring. i.e. don't rework before commit, rework after.

> I wonder if we might have alignment issues with the
> SharedInvalidationMessages being stored in WAL records, following the
> subxids. All the data stored in that record have 4-byte alignment at the
> moment, but if SharedInvalidationMessage ever needs 8-byte alignment, we
> would have trouble. Probably not worth changing code, it's highly
> unlikely that SharedInvalidationMessage will ever need 8-byte alignment,
> but perhaps a comment somewhere would be in order.

It's a possible source of bugs, but there are no issues there AFAICS.
The technique of multiple arrays on a WAL record wasn't invented by this
patch.

> I note that we don't emit RunningXacts after a shutdown checkpoint. So
> if recovery starts at a shutdown checkpoint, we don't let read-only
> backends in until the first online checkpoint. Could we treat a shutdown
> checkpoint as a snapshot with no transactions running? Or do prepared
> transactions screw that up?

We could, but I see no requirement for starting HS from a backup taken
on a shutdown database. It's just another special case to test and since
we already have significant number of important test cases I'd say add
this later.

That seems to have reflected all of your points on this post, though
thanks for the comments. I'm keen to reduce complexity in areas that
have caused bugs, but for stuff that is working I am tempted to leave
alone on such a big patch. Anything we can do to avoid re-testing
sections of code and/or reduce the number of tests required is going to
increase stability.

--
Simon Riggs www.2ndQuadrant.com