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 bullet and 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 backup block, 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 and
  XLogCancelInsert() 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 rather hard 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 it really 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?


Andres Freund

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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to