Re: [PATCH 10/16] Introduce the concept that wal has a 'origin' node

On 12-06-13 01:27 PM, Andres Freund wrote:
> The previous mail contained a patch with a mismerge caused by reording
> commits. Corrected version attached.
>
> Thanks to Steve Singer for noticing this quickly.
>

Attached is a more complete review of this patch.

I agree that we will need to identify the node a change originated at.  
We will not only want this for multi-master support but it might also be 
very helpful once we introduce things like cascaded replicas. Using a 16 
bit integer for this purpose makes sense to me.

This patch (with the previous numbered patches already applied), still 
doesn't compile.

gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith 
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute 
-Wformat-security -fno-strict-aliasing -fwrapv -I../../../../src/include 
-D_GNU_SOURCE   -c -o xact.o xact.c
xact.c: In function 'xact_redo_commit':
xact.c:4678: error: 'xl_xact_commit' has no member named 'origin_lsn'
make[4]: *** [xact.o] Error 1

Your complete patch set did compile.  origin_lsn gets added as part of 
your 12'th patch.  Managing so many related patches is going to be a 
pain. but it beats one big patch.  I don't think this patch actually 
requires the origin_lsn change.


Code Review
-------------------------
src/backend/utils/misc/guc.c
@@ -1598,6 +1600,16 @@ static struct config_int ConfigureNamesInt[] =
      },

      {
+        {"multimaster_node_id", PGC_POSTMASTER, REPLICATION_MASTER,
+            gettext_noop("node id for multimaster."),
+            NULL
+        },
+ &guc_replication_origin_id,
+        InvalidMultimasterNodeId, InvalidMultimasterNodeId, 
MaxMultimasterNodeId,
+        NULL, assign_replication_node_id, NULL
+    },

I'd rather see us refer to this as the 'node id for logical replication' 
over the multimaster node id.  I think that terminology will be less 
controversial.  Multi-master means different things to different people 
and it is still unclear what forms of multi-master we will have 
in-core.  For example,  most people don't consider slony to be 
multi-master replication.  If a future version of slony were to feed off 
logical replication (instead of triggers) then I think it would need 
this node id to determine which node a particular change has come from.

The description inside the gettext call should probably be "Sets the 
node id for ....." to be consistent with the description of the rest of 
the GUC's

BootStrapXLOG in xlog.c
creates a XLogRecord structure and shouldit  set xl_origin_id to the  
InvalidMultimasterNodeId?

WriteEmptyXLOG in pg_resetxlog.c might also should set xl_origin_id to a 
well defined value.  I think InvalidMultimasterNodeId should be safe 
even for a no-op record in from a node that actually has a node_id set 
on real records.

backend/replication/logical/logical.c:
XLogRecPtr current_replication_origin_lsn = {0, 0};

This variable isn't used/referenced by this patch it probably belongs as 
part of the later patch.


Steve

> Andres
>
>
>
>