Re: pg_receivewal starting position - Mailing list pgsql-hackers

From Michael Paquier
Subject Re: pg_receivewal starting position
Date
Msg-id YXaLpHukgwD1wIf+@paquier.xyz
Whole thread Raw
In response to Re: pg_receivewal starting position  (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>)
Responses Re: pg_receivewal starting position
List pgsql-hackers
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

Attachment

pgsql-hackers by date:

Previous
From: Bharath Rupireddy
Date:
Subject: Re: should we allow users with a predefined role to access pg_backend_memory_contexts view and pg_log_backend_memory_contexts function?
Next
From: Amit Kapila
Date:
Subject: Re: pgsql: Document XLOG_INCLUDE_XID a little better