Re: pg_receivexlog and replication slots

On Wed, Sep 10, 2014 at 10:09 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:
>
>
> New patches taking into account all those comments are attached.

I have looked into refactoring related patch and would like
to share my observations with you:

1.
+ * Run IDENTIFY_SYSTEM through a given connection and give back to caller
+ * some result information if
requested:
+ * - Start LSN position
+ * - Current timeline ID
+ * - system identifier

This API also gets plugin if requested, so I think it is better
to mention the same in function header as well.

2.
+ /* Get LSN start position if necessary */
+ if (sscanf(PQgetvalue(res, 0, 2), "%X/%X", &hi, &lo)
!= 2)
+ {
+ fprintf(stderr,
+ _("%s: could not parse transaction
log location \"%s\"\n"),
+ progname, PQgetvalue(res, 0, 2));
+
return false;
+ }
+ if (startpos != NULL)
+ *startpos = ((uint64) hi) << 32 | lo;

Isn't it better if we try to get the LSN position only incase
startpos!=NULL (as BaseBackup don't need this)

3.
/*
- * Run IDENTIFY_SYSTEM so we can get the timeline and current xlog
- * position.
+ * Identify
server, obtaining start LSN position and current timeline ID
+ * at the same time.
*/

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.

4.
/*
* Figure out where to start streaming.
@@ -492,6 +459,12 @@ main(int argc, char **argv)

pqsignal(SIGINT, sigint_handler);
#endif

+ /* Obtain a connection before doing anything */
+ conn
= GetConnection();
+ if (!conn)
+ /* Error message already written in GetConnection() */
+
exit(1);
+

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

5.
+bool
+RunIdentifySystem(PGconn *conn, char **sysid, TimeLineID *starttli,
+
XLogRecPtr *startpos, char **plugin)
+{
+ PGresult *res;
+ uint32 hi, lo;
+
+ /* Leave if
no connection present */
+ if (conn == NULL)
+ return false;

Shouldn't there be Assert instead of if (conn == NULL), as
RunIdentifySystem() is not expected to be called without connection.

6.
main()
{
..

+ /* Identify system, obtaining start LSN position at the same time */
+ if (!RunIdentifySystem(conn,
NULL, NULL, &startpos, &plugin_name))
+ disconnect_and_exit(1);
+
+ /*
+ * Check that there
is a plugin name, a logical slot needs to have
+ * one absolutely.
+ */
+ if (!plugin_name)
+ {
+
fprintf(stderr,
+ _("%s: no plugin found for replication slot \"%s
\"\n"),
+ progname, replication_slot);
+ disconnect_and_exit(1);
+
}
+
+ /* Stream loop */

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.
b.
Above call is trying to get startpos, won't it overwrite the value
passed by user (case 'I':) for startpos?

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