Re: logical copy_replication_slot issues

On Fri, 6 Mar 2020 at 20:02, Arseny Sher <a.sher@postgrespro.ru> wrote:
>
> I wrote:
>
> > It looks good to me now.
>
> After lying for some time in my head it reminded me that
> CreateInitDecodingContext not only pegs the LSN, but also xmin, so
> attached makes a minor comment correction.
>
> While taking a look at the nearby code it seemed weird to me that
> GetOldestSafeDecodingTransactionId checks PGXACT->xid, not xmin. Don't
> want to investigate this at the moment though, and not for this thread.
>
> Also not for this thread, but I've noticed
> pg_copy_logical_replication_slot doesn't allow to change plugin name
> which is an omission in my view. It would be useful and trivial to do.
>

Thank you for updating the patch. The patch looks basically good to me
but I have a few questions:

    /*
-    * Create logical decoding context, to build the initial snapshot.
+    * Create logical decoding context to find start point or, if we don't
+    * need it, to 1) bump slot's restart_lsn and xmin 2) check plugin sanity.
     */

Do we need to numbering that despite not referring them?

    ctx = CreateInitDecodingContext(plugin, NIL,
-                                   false,  /* do not build snapshot */
+                                   false,  /* do not build data snapshot */
                                    restart_lsn,
                                    logical_read_local_xlog_page, NULL, NULL,
                                    NULL);

I'm not sure this change makes the comment better. Could you elaborate
on the motivation of this change?

Regards,

--
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services