Re: WAL format and API changes (9.5)

On 2014-08-12 12:44:22 +0300, Heikki Linnakangas wrote:
> Here's a new patch with those little things fixed.

I've so far ignored this thread... I'm now taking a look. Unfortunately
I have to admit I'm not unequivocally happy.

* XLogRegisterData/XLogRegisterRecData imo don't really form a consistent API namewise. What does 'Rec' mean here?

* I'm distinctly not a fan of passing around both the LSN and the struct XLogRecord to functions. We should bit the
bulletand use something encapsulating both.
 

* XLogReplayBuffer imo isn't a very good name - the buffer isn't replayed. If anything it's sometimes replaced by the
backupblock, but the real replay still happens outside of that function.
 

* Why do we need XLogBeginInsert(). Right now this leads to uglyness like duplicated if (RelationNeedsWAL()) wal checks
andXLogCancelInsert() of inserts that haven't been started. And if we have Begin, why do we also need Cancel?
 

* "XXX: do we need to do this for every page?" - yes, and it's every tuple, not every page... And It needs to be done
*after*the tuple has been put on the page, not before. Otherwise the location will be wrong.
 

* The patch mixes the API changes around WAL records with changes of how individual actions are logged. That makes it
ratherhard to review - and it's a 500kb patch already.
 
 I realize it's hard to avoid because the new API changes which information about blocks is available, but it does make
itreally hard to see whether the old/new code is doing something equivalent. It's hard to find more critical code than
this:/
 

Have you done any performance evaluation?

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services