On Mon, Feb 5, 2024 at 4:36 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> I have pushed the first patch. Next, a few comments on 0002 are as follows:
Thanks for the feedback Amit. Some of these are addressed in v78. Rest
will be addressed in the next version.
> 1.
> +static bool
> +validate_parameters_and_get_dbname(char **dbname, int elevel)
>
> For 0002, we don't need dbname as out parameter. Also, we can rename
> the function to validate_slotsync_params() or something like that.
> Also, for 0003, we don't need to get the dbname from
> wait_for_valid_params_and_get_dbname(), instead there could be a
> common function that can be invoked from validate_slotsync_params()
> and caller of wait function that caches the value of dbname.
>
> The other parameter elevel is also not required for 0002.
>
> 2.
> + /*
> + * Make sure that concerned WAL is received and flushed before syncing
> + * slot to target lsn received from the primary server.
> + */
> + latestFlushPtr = GetStandbyFlushRecPtr(NULL);
> + if (remote_slot->confirmed_lsn > latestFlushPtr)
> + {
> + /*
> + * Can get here only if GUC 'standby_slot_names' on the primary server
> + * was not configured correctly.
> + */
> + ereport(LOG,
> + errmsg("skipping slot synchronization as the received slot sync"
> + " LSN %X/%X for slot \"%s\" is ahead of the standby position %X/%X",
> + LSN_FORMAT_ARGS(remote_slot->confirmed_lsn),
> + remote_slot->name,
> + LSN_FORMAT_ARGS(latestFlushPtr)));
> +
> + return false;
>
> In the case of a function invocation, this should be an ERROR. We can
> move the comment related to 'standby_slot_names' to a later patch
> where that GUC is introduced. See, if there are other LOGs in the
> patch that needs to be converted to ERROR.
>
> 3. The function pg_sync_replication_slots() should be in file
> slotfuncs.c and common functionality between this function and
> slotsync worker can be exposed via a function in slotsync.c.
>
> 4.
> /*
> + * Using the specified primary server connection, check whether we are
> + * cascading standby and validates primary_slot_name for
> + * non-cascading-standbys.
> + */
> + check_primary_info(wrconn, &am_cascading_standby,
> + &primary_slot_invalid, ERROR);
> +
> + if (am_cascading_standby)
> + ereport(ERROR,
> + errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("cannot synchronize replication slots to a cascading standby"));
>
> primary_slot_invalid is not used in this patch. I think we can allow
> the function can be executed on cascading_standby as well because this
> will be used for the planned switchover.
>
> 5. I don't see any problem with allowing concurrent processes trying
> to sync the same slot at the same time as each process will acquire
> the slot and only one process can acquire the slot at a time, the
> other will get an ERROR.
>
> --
> With Regards,
> Amit Kapila.
