Re: [HACKERS] [PATCH 11/16] Add infrastructure for manipulating multiple streams of wal on a segment handling level

[Boszormenyi Zoltan]

2012-06-29 15:01 keltezéssel, Andres Freund írta:

Hi,

On Friday, June 29, 2012 02:43:52 PM Boszormenyi Zoltan wrote:

trying to review this one according to
http://wiki.postgresql.org/wiki/Reviewing_a_Patch

# Is the patch in context diff format
?
No. (Does this requirement still apply after PostgreSQL switched to GIT?)

Many people seem to send patches in unified format and just some days ago Tom
said it doesn't matter to him. I still can't properly read context diffs, so I
am using unified...


Unified diffs are usually more readable for me after following
the Linux kernel development for years and are shorter than
context diffs.


# Does it apply cleanly to the current git master?
No. The patches 01...09 in this series taken from the mailing list apply
cleanly, 10 and 11 fail with rejects.

Yea, and even the patches before that need to be rebased, at least partially
they won't compile even though they apply cleanly.

I will produce a rebased version soon, but then we haven't fully aggreed on
preliminary patches to this one, so there doesn't seem to be too much point in
reviewing this one before the other stuff is clear.

Marking the patch as "Returned with Feedback" for now. I have done the same
with 13, 15. Those seem to be too much in limbo for CF-style reviews.

Thanks!


You're welcome.


Andres



--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/


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

Re: [HACKERS] [PATCH 11/16] Add infrastructure for manipulating multiple streams of wal on a segment handling level

[Andres Freund]

Hi,

On Friday, June 29, 2012 02:43:52 PM Boszormenyi Zoltan wrote:
> trying to review this one according to
> http://wiki.postgresql.org/wiki/Reviewing_a_Patch
> 
> # Is the patch in context diff format
> ?
> No. (Does this requirement still apply after PostgreSQL switched to GIT?)
Many people seem to send patches in unified format and just some days ago Tom 
said it doesn't matter to him. I still can't properly read context diffs, so I 
am using unified...

> # Does it apply cleanly to the current git master?
> No. The patches 01...09 in this series taken from the mailing list apply
> cleanly, 10 and 11 fail with rejects.
Yea, and even the patches before that need to be rebased, at least partially 
they won't compile even though they apply cleanly.

I will produce a rebased version soon, but then we haven't fully aggreed on 
preliminary patches to this one, so there doesn't seem to be too much point in 
reviewing this one before the other stuff is clear.

Marking the patch as "Returned with Feedback" for now. I have done the same 
with 13, 15. Those seem to be too much in limbo for CF-style reviews.

Thanks!

Andres

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

Re: [HACKERS] [PATCH 11/16] Add infrastructure for manipulating multiple streams of wal on a segment handling level

[Boszormenyi Zoltan]

Hi,

trying to review this one according to 
http://wiki.postgresql.org/wiki/Reviewing_a_Patch

# Is the patch in context diff format 
?

No. (Does this requirement still apply after PostgreSQL switched to GIT?)

# Does it apply cleanly to the current git master?

No. The patches 01...09 in this series taken from the mailing list apply 
cleanly,
10 and 11 fail with rejects.

Best regards,
Zoltán Böszörményi

2012-06-13 13:28 keltezéssel, Andres Freund írta:

From: Andres Freund 

For that add a 'node_id' parameter to most commands dealing with wal
segments. A node_id thats 'InvalidMultimasterNodeId' references local wal,
every other node_id referes to wal in a new pg_lcr directory.

Using duplicated code would reduce the impact of that change but the long-term
code-maintenance burden outweighs that by a far bit.

Besides the decision to add a 'node_id' parameter to several functions the
changes in this patch are fairly mechanical.
---
  src/backend/access/transam/xlog.c   |   54 ---
  src/backend/replication/basebackup.c|4 +-
  src/backend/replication/walreceiver.c   |2 +-
  src/backend/replication/walsender.c |9 +++--
  src/bin/initdb/initdb.c |1 +
  src/bin/pg_resetxlog/pg_resetxlog.c |2 +-
  src/include/access/xlog.h   |2 +-
  src/include/access/xlog_internal.h  |   13 +--
  src/include/replication/logical.h   |2 +
  src/include/replication/walsender_private.h |2 +-
  10 files changed, 56 insertions(+), 35 deletions(-)

diff --git a/src/backend/access/transam/xlog.c 
b/src/backend/access/transam/xlog.c
index 504b4d0..0622726 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -635,8 +635,8 @@ static bool XLogCheckBuffer(XLogRecData *rdata, bool 
doPageWrites,
  static bool AdvanceXLInsertBuffer(bool new_segment);
  static bool XLogCheckpointNeeded(uint32 logid, uint32 logseg);
  static void XLogWrite(XLogwrtRqst WriteRqst, bool flexible, bool xlog_switch);
-static bool InstallXLogFileSegment(uint32 *log, uint32 *seg, char *tmppath,
-  bool find_free, int *max_advance,
+static bool InstallXLogFileSegment(RepNodeId node_id, uint32 *log, uint32 *seg,
+  char *tmppath, bool find_free, int 
*max_advance,
   bool use_lock);
  static int XLogFileRead(uint32 log, uint32 seg, int emode, TimeLineID tli,
 int source, bool notexistOk);
@@ -1736,8 +1736,8 @@ XLogWrite(XLogwrtRqst WriteRqst, bool flexible, bool 
xlog_switch)
  
  			/* create/use new log file */

use_existent = true;
-   openLogFile = XLogFileInit(openLogId, openLogSeg,
-  
&use_existent, true);
+   openLogFile = XLogFileInit(InvalidMultimasterNodeId, 
openLogId,
+  openLogSeg, &use_existent, 
true);
openLogOff = 0;
}
  
@@ -2376,6 +2376,9 @@ XLogNeedsFlush(XLogRecPtr record)

   * place.  This should be TRUE except during bootstrap log creation.  The
   * caller must *not* hold the lock at call.
   *
+ * node_id: if != InvalidMultimasterNodeId this xlog file is actually a LCR
+ * file
+ *
   * Returns FD of opened file.
   *
   * Note: errors here are ERROR not PANIC because we might or might not be
@@ -2384,8 +2387,8 @@ XLogNeedsFlush(XLogRecPtr record)
   * in a critical section.
   */
  int
-XLogFileInit(uint32 log, uint32 seg,
-bool *use_existent, bool use_lock)
+XLogFileInit(RepNodeId node_id, uint32 log, uint32 seg,
+ bool *use_existent, bool use_lock)
  {
charpath[MAXPGPATH];
chartmppath[MAXPGPATH];
@@ -2396,7 +2399,7 @@ XLogFileInit(uint32 log, uint32 seg,
int fd;
int nbytes;
  
-	XLogFilePath(path, ThisTimeLineID, log, seg);

+   XLogFilePath(path, ThisTimeLineID, node_id, log, seg);
  
  	/*

 * Try to use existent file (checkpoint maker may have created it 
already)
@@ -2425,6 +2428,11 @@ XLogFileInit(uint32 log, uint32 seg,
 */
elog(DEBUG2, "creating and filling new WAL file");
  
+	/*

+* FIXME: to be safe we need to create tempfile in the pg_lcr directory 
if
+* its actually an lcr file because pg_lcr might be in a different
+* partition.
+*/
snprintf(tmppath, MAXPGPATH, XLOGDIR "/xlogtemp.%d", (int) getpid());
  
  	unlink(tmppath);

@@ -2493,7 +2501,7 @@ XLogFileInit(uint32 log, uint32 seg,
installed_log = log;
installed_seg = seg;
max_advance = XLOGfileslop;
-   if (!InstallXLogFileSegment(&installed_log, &installed