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
>
>
>
>