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