Re: pg_receivewal starting position

On Mon, Oct 25, 2021 at 4:19 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> > 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.

Then, please change if (*restart_lsn) and if (*restart_tli) to if
(restart_lsn) and if (restart_tli), just to be consistent with the
other parts of the patch and the existing code of RunIdentifySystem():
if (*restart_lsn)
*restart_lsn = lsn_loc;
if (restart_tli != NULL)
*restart_tli = tli_loc;

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

StreamLog() isn't reached for create and drop slot cases, see [1]. I
suggest to remove replication_slot != NULL and have Assert(slot_name)
in GetSlotInformation():
/*
* Try to get the starting point from the slot. This is supported in
* PostgreSQL 15 and up.
*/
if (PQserverVersion(conn) >= 150000)
{
if (!GetSlotInformation(conn, replication_slot, &stream.startpos,
&stream.timeline))
{
/* Error is logged by GetSlotInformation() */
return;
}
}

Here is another comment on the patch:
Remove the extra new line above the GetSlotInformation() definition:
return true;
}

+ -----> REMOVE THIS
+/*
+ * Run READ_REPLICATION_SLOT through a given connection and give back to

Apart from the above v12 patch LGTM.

[1]
/*
* Drop a replication slot.
*/
if (do_drop_slot)
{
if (verbose)
pg_log_info("dropping replication slot \"%s\"", replication_slot);

if (!DropReplicationSlot(conn, replication_slot))
exit(1);
exit(0);
}

/* Create a replication slot */
if (do_create_slot)
{
if (verbose)
pg_log_info("creating replication slot \"%s\"", replication_slot);

if (!CreateReplicationSlot(conn, replication_slot, NULL,
false, true, false,
slot_exists_ok, false))
exit(1);
exit(0);
}

Regards,
Bharath Rupireddy.