Re: pg_receivewal starting position - Mailing list pgsql-hackers

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