Re: pg_receivexlog and replication slots

On Sat, Sep 20, 2014 at 10:06 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:
> On Sat, Sep 20, 2014 at 7:09 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > 3.
> > I find existing comments okay, is there a need to change
> > in this case? Part of the new comment mentions
> > "obtaining start LSN position", actually the start position is
> > identified as part of next function call FindStreamingStart(),
> > only incase it return InvalidXLogRecPtr, the value returned
> > by IDENTIFY_SYSTEM will be used.
> Hopefully I am following you correctly here: comment has been updated
> to mention that the start LSN and TLI fetched from IDENTIFY_SYSTEM are
> always fetched but used only if no valid position is found in output
> folder of pg_receivexlog.

Updated comment is consistent with code, however my main point
was that in this case, I don't see much need to change existing
comment. I think this point is more related to each individual's
perspective, so if you think there is a need to update the existing
comment, then retain as it is in your patch and let Committer
take a call about it.

> > 4.
> > + /* Obtain a connection before doing anything */
> > + conn = GetConnection();
> > + if (!conn)
> > + /* Error message already written in GetConnection() */
> > Is there a reason for moving this function out of StreamLog(),
> > there is no harm in moving it, but there doesn't seem to be any
> > problem even if it is called inside StreamLog().
> For pg_recvlogical, this move is done to reduce code redundancy, and
> actually it makes sense to just grab one connection in the main() code
> path before performing any replication commands. For pg_receivexlog,
> the move is done because it makes the code more consistent with
> pg_recvlogical, and also it is a preparatory work for the second patch
> that introduces the create/drop slot. Even if 2nd patch is not
> accepted I figured out that it would not hurt to simply grab one
> connection in the main code path before doing any action.

In pg_receivexlog, StreamLog() calls PQfinish() to close a connection
to the backend and StreamLog() is getting called in while(true) loop,
so if you just grab the connection once in main loop, the current
logic won't work. For same reasons, the current coding related to
GetConnection() in pg_receivelogical seems to be right, so it is better
not to change that as well. For the second patch (that introduces the
create/drop slot), I think it is better to do in a way similar to what
current pg_receivelogical does.

>
> > 6.
> > + /* Identify system, obtaining start LSN position at the same time */
> > + if (!RunIdentifySystem(conn,
> > NULL, NULL, &startpos, &plugin_name))
> > + disconnect_and_exit(1);
> > a.
> > Generally IdentifySystem is called as first API, but I think you
> > have changed its location after CreateReplicationSlot as you want
> > to double check the value of plugin, not sure if that is necessary or
> > important enough that we start calling it later.
> Funny part here is that even the current code on master and
> REL9_4_STABLE of pg_recvlogical.c is fetching a start position when
> creating a replication slot that is not used as two different actions
> cannot be used at the same time. That's a matter of removing this line
> in code though:
> startpos = ((uint64) hi) << 32 | lo;

User is not allowed to give startpos with drop or create of replication
slot. It is prevented by check:
if (startpos != InvalidXLogRecPtr && (do_create_slot || do_drop_slot))
{
fprintf(stderr, _
("%s: cannot use --create or --drop together with --startpos\n"), progname);
fprintf(stderr, _
("Try \"%s --help\" for more information.\n"),
progname);
exit(1);
}
So it seems you cannot remove the startpos assignment in code.

> As that's only cosmetic for 9.4, and this patch makes things more
> correct I guess that's fine to do nothing and just try to get this
> patch in.

In what sense do you think that this part of patch is better than
current code?
I think calling Identify_System as a first command makes sense
(as it is in current code) because if that fails we should not
proceed with execution of other command's.

Another point about refactoring patch is that fourth column in
Identify_System is dbname and in patch, you are referring it as
plugin which seems to be inconsistent.
Refer below code in patch:

RunIdentifySystem()
{
..
+ /* Get decoder plugin name, only available in 9.4 and newer versions */
+ if (plugin != NULL)
+
*plugin = PQnfields(res) > 3 && !PQgetisnull(res, 0, 3) ?
+ pg_strdup(PQgetvalue
(res, 0, 3)) : (char *) NULL;
..
}

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com