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

On 12-06-13 01:27 PM, Andres Freund wrote: <blockquote cite="mid:201206131927.49923.andres@2ndquadrant.com"
type="cite"><prewrap="">The previous mail contained a patch with a mismerge caused by reording 
 
commits. Corrected version attached.

Thanks to Steve Singer for noticing this quickly.

</pre></blockquote><br /> Attached is a more complete review of this patch.<br /><br /> I agree that we will need to
identifythe node a change originated at.  We will not only want this for multi-master support but it might also be very
helpfulonce we introduce things like cascaded replicas. Using a 16 bit integer for this purpose makes sense to me.<br
/><br/> This patch (with the previous numbered patches already applied), still doesn't compile.<br /><br /> 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<br />
xact.c:In function ‘xact_redo_commit’:<br /> xact.c:4678: error: ‘xl_xact_commit’ has no member named ‘origin_lsn’<br
/>make[4]: *** [xact.o] Error 1<br /><br /> Your complete patch set did compile.  origin_lsn gets added as part of your
12'thpatch.  Managing so many related patches is going to be a pain. but it beats one big patch.  I don't think this
patchactually requires the origin_lsn change.<br /><br /><br /> Code Review<br /> -------------------------<br />
src/backend/utils/misc/guc.c<br/> @@ -1598,6 +1600,16 @@ static struct config_int ConfigureNamesInt[] =<br />     
},<br/>  <br />      {<br /> +        {"multimaster_node_id", PGC_POSTMASTER, REPLICATION_MASTER,<br /> +           
gettext_noop("nodeid for multimaster."),<br /> +            NULL<br /> +        },<br /> +       
&guc_replication_origin_id,<br/> +        InvalidMultimasterNodeId, InvalidMultimasterNodeId,
MaxMultimasterNodeId,<br/> +        NULL, assign_replication_node_id, NULL<br /> +    },<br /><br /> I'd rather see us
referto this as the 'node id for logical replication' over the multimaster node id.  I think that terminology will be
lesscontroversial.  Multi-master means different things to different people and it is still unclear what forms of
multi-masterwe will have in-core.  For example,  most people don't consider slony to be multi-master replication.  If a
futureversion of slony were to feed off logical replication (instead of triggers) then I think it would need this node
idto determine which node a particular change has come from.  <br /><br /> The description inside the gettext call
shouldprobably be "Sets the node id for ....." to be consistent with the description of the rest of the GUC's<br /><br
/>BootStrapXLOG in xlog.c<br /> creates a XLogRecord structure and shouldit  set xl_origin_id to the 
InvalidMultimasterNodeId?<br/><br /> WriteEmptyXLOG in pg_resetxlog.c might also should set xl_origin_id to a well
definedvalue.  I think InvalidMultimasterNodeId should be safe even for a no-op record in from a node that actually has
anode_id set on real records.<br /><br /> backend/replication/logical/logical.c:<br /> XLogRecPtr
current_replication_origin_lsn= {0, 0};<br /><br /> This variable isn't used/referenced by this patch it probably
belongsas part of the later patch.<br /><br /><br /> Steve<br /><br /><blockquote
cite="mid:201206131927.49923.andres@2ndquadrant.com"type="cite"><pre wrap="">Andres
 

</pre> <pre wrap="">
<fieldset class="mimeAttachmentHeader"></fieldset>

</pre></blockquote><br />