Re: pg_receivexlog and replication slots

On Sat, Sep 20, 2014 at 7:09 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> I have looked into refactoring related patch and would like
> to share my observations with you:
Thanks! Useful input is always welcome.

> 1.
> + * Run IDENTIFY_SYSTEM through a given connection and give back to caller
> This API also gets plugin if requested, so I think it is better
> to mention the same in function header as well.
True.

> 2.
> + /* Get LSN start position if necessary */
> Isn't it better if we try to get the LSN position only incase
> startpos!=NULL (as BaseBackup don't need this)
OK. Let's do that for consistency with the other fields.

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

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

> 5.
> Shouldn't there be Assert instead of if (conn == NULL), as
> RunIdentifySystem() is not expected to be called without connection.
Fine for me. That's indeed more consistent with the rest this way.

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

> b.
> Above call is trying to get startpos, won't it overwrite the value
> passed by user (case 'I':) for startpos?
Yep, that's a bug. The value that user could give would have been
overwritten all the time. Current code does not even care about
checking startpos in IDENTIFY_SYSTEM so we're fine by just removing
this part.

Updated patches are attached.
--
Michael