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

On Monday, June 18, 2012 02:43:26 AM Steve Singer wrote:
> 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.
Good.

> 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.
Hrmpf #666. I will go through through the series commit-by-commit again to 
make sure everything compiles again. Reordinging this late definitely wasn't a 
good idea...

I pushed a rebased version with all those fixups (and removal of the 
zeroRecPtr patch).
> 
> 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. 
Youre right. 'replication_node_id' or such should be ok?

> 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.
Good catches.


> 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.
Yea, just as the usage of origin_lsn in the above compile failure.


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