On Mon, Oct 25, 2021 at 02:40:05PM +0530, Bharath Rupireddy wrote:
> 2) I think we should tweak the below error message
> to
> pg_log_error("could not read replication slot \"%s\": %s",
> "IDENTIFY_SYSTEM", PQerrorMessage(conn));
> Having slot name in the error message helps to isolate the error
> message from tons of server logs that gets generated.
Yes, this suggestion makes sense.
> 3) Also for the same reasons stated as above, change the below error message
> pg_log_error("could not read replication slot: got %d rows and %d
> fields, expected %d rows and %d or more fields",
> to
> pg_log_error("could not read replication slot \"%s\": got %d rows and
> %d fields, expected %d rows and %d or more fields", slot_name,....
We can even get rid of "or more" to match the condition used.
> 4) Also for the same reasons, change below
> + pg_log_error("could not parse slot's restart_lsn \"%s\"",
> to
> pg_log_error("could not parse replicaton slot \"%s\" restart_lsn \"%s\"",
> slot_name, PQgetvalue(res, 0, 1));
Appending the slot name makes sense.
> 5) I think we should also have assertion for the timeline id:
> Assert(stream.startpos != InvalidXLogRecPtr);
> Assert(stream.timeline!= 0);
Okay.
> 6) Why do we need these two assignements?
> I think we can just get rid of lsn_loc and tli_loc, initlaize
> *restart_lsn = InvalidXLogRecPtr and *restart_tli = 0 at the start of
> the function and directly assign the requrested values to *restart_lsn
> and *restart_tli, also see comment (8).
FWIW, I find the style of the patch easier to follow.
> 9) 80char limit crossed:
> +GetSlotInformation(PGconn *conn, const char *slot_name, XLogRecPtr
> *restart_lsn, TimeLineID *restart_tli)
pgindent says nothing.
> 10) Missing word "command", and use "issued to the server", so change the below:
> + <command>READ_REPLICATION_SLOT</command> is issued to retrieve the
> to
> + <command>READ_REPLICATION_SLOT</command> command is issued to
> the server to retrieve the
Okay.
> 11) Will replication_slot ever be NULL? If it ever be null, then we
> don't reach this far right? We see the pg_log_error("%s needs a slot
> to be specified using --slot". Please revmove below if condition:
> + * server may not support this option.
Did you notice that this applies when creating or dropping a slot, for
code paths entirely different than what we are dealing with here?
--
Michael