From 801792e528d6cb5eeb3a70f5ceb10b15bf92f0c7 Mon Sep 17 00:00:00 2001 From: Amit Kapila Date: Thu, 22 Feb 2024 11:17:00 +0530 Subject: [PATCH] Improve ERROR/LOG messages added by commits ddd5f4f54a and 7a424ece48. Additionally, in slotsync.c, replace one StringInfoData variable usage with a constant string to avoid palloc/pfree. Also, replace the inclusion of "logical.h" with "slot.h" to prevent the exposure of unnecessary implementation details. Reported-by: Kyotaro Horiguchi, Masahiko Sawada Author: Shveta Malik based on suggestions by Robert Haas and Amit Kapila Reviewed-by: Kyotaro Horiguchi, Amit Kapila Discussion: https://postgr.es/m/20240214.162652.773291409747353211.horikyota.ntt@gmail.com Discussion: https://postgr.es/m/20240219.134015.1888940527023074780.horikyota.ntt@gmail.com Discussion: https://postgr.es/m/CAD21AoCYXhDYOQDAS-rhGasC2T+tYbV=8Y18o94sB=5AxcW+yA@mail.gmail.com --- src/backend/replication/logical/slotsync.c | 65 ++++++++----------- .../t/040_standby_failover_slots_sync.pl | 2 +- 2 files changed, 28 insertions(+), 39 deletions(-) diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c index 4cab7b7101..4cc9148c57 100644 --- a/src/backend/replication/logical/slotsync.c +++ b/src/backend/replication/logical/slotsync.c @@ -35,7 +35,7 @@ #include "access/xlogrecovery.h" #include "catalog/pg_database.h" #include "commands/dbcommands.h" -#include "replication/logical.h" +#include "replication/slot.h" #include "replication/slotsync.h" #include "storage/ipc.h" #include "storage/lmgr.h" @@ -368,14 +368,13 @@ update_and_persist_local_synced_slot(RemoteSlot *remote_slot, Oid remote_dbid) * XXX should this be changed to elog(DEBUG1) perhaps? */ ereport(LOG, - errmsg("could not sync slot information as remote slot precedes local slot:" - " remote slot \"%s\": LSN (%X/%X), catalog xmin (%u) local slot: LSN (%X/%X), catalog xmin (%u)", - remote_slot->name, - LSN_FORMAT_ARGS(remote_slot->restart_lsn), - remote_slot->catalog_xmin, - LSN_FORMAT_ARGS(slot->data.restart_lsn), - slot->data.catalog_xmin)); - + errmsg("could not sync slot \"%s\" as remote slot precedes local slot", + remote_slot->name), + errdetail("Remote slot has LSN %X/%X and catalog xmin %u, but local slot has LSN %X/%X and catalog xmin %u.", + LSN_FORMAT_ARGS(remote_slot->restart_lsn), + remote_slot->catalog_xmin, + LSN_FORMAT_ARGS(slot->data.restart_lsn), + slot->data.catalog_xmin)); return; } @@ -569,8 +568,12 @@ synchronize_slots(WalReceiverConn *wrconn) WalRcvExecResult *res; TupleTableSlot *tupslot; - StringInfoData s; List *remote_slot_list = NIL; + const char *query = "SELECT slot_name, plugin, confirmed_flush_lsn," + " restart_lsn, catalog_xmin, two_phase, failover," + " database, conflict_reason" + " FROM pg_catalog.pg_replication_slots" + " WHERE failover and NOT temporary"; SpinLockAcquire(&SlotSyncCtx->mutex); if (SlotSyncCtx->syncing) @@ -586,19 +589,8 @@ synchronize_slots(WalReceiverConn *wrconn) syncing_slots = true; - initStringInfo(&s); - - /* Construct query to fetch slots with failover enabled. */ - appendStringInfo(&s, - "SELECT slot_name, plugin, confirmed_flush_lsn," - " restart_lsn, catalog_xmin, two_phase, failover," - " database, conflict_reason" - " FROM pg_catalog.pg_replication_slots" - " WHERE failover and NOT temporary"); - /* Execute the query */ - res = walrcv_exec(wrconn, s.data, SLOTSYNC_COLUMN_COUNT, slotRow); - pfree(s.data); + res = walrcv_exec(wrconn, query, SLOTSYNC_COLUMN_COUNT, slotRow); if (res->status != WALRCV_OK_TUPLES) ereport(ERROR, @@ -743,12 +735,12 @@ validate_remote_info(WalReceiverConn *wrconn) ereport(ERROR, errmsg("could not fetch primary_slot_name \"%s\" info from the primary server: %s", PrimarySlotName, res->err), - errhint("Check if \"primary_slot_name\" is configured correctly.")); + errhint("Check if primary_slot_name is configured correctly.")); tupslot = MakeSingleTupleTableSlot(res->tupledesc, &TTSOpsMinimalTuple); if (!tuplestore_gettupleslot(res->tuplestore, true, false, tupslot)) elog(ERROR, - "failed to fetch tuple for the primary server slot specified by \"primary_slot_name\""); + "failed to fetch tuple for the primary server slot specified by primary_slot_name"); remote_in_recovery = DatumGetBool(slot_getattr(tupslot, 1, &isnull)); Assert(!isnull); @@ -764,9 +756,9 @@ validate_remote_info(WalReceiverConn *wrconn) if (!primary_slot_valid) ereport(ERROR, errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("bad configuration for slot synchronization"), + errmsg("slot synchronization requires valid primary_slot_name"), /* translator: second %s is a GUC variable name */ - errdetail("The replication slot \"%s\" specified by \"%s\" does not exist on the primary server.", + errdetail("The replication slot \"%s\" specified by %s does not exist on the primary server.", PrimarySlotName, "primary_slot_name")); ExecClearTuple(tupslot); @@ -792,8 +784,7 @@ ValidateSlotSyncParams(void) ereport(ERROR, /* translator: %s is a GUC variable name */ errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("bad configuration for slot synchronization"), - errhint("\"%s\" must be defined.", "primary_slot_name")); + errmsg("slot synchronization requires %s to be defined", "primary_slot_name")); /* * hot_standby_feedback must be enabled to cooperate with the physical @@ -804,15 +795,14 @@ ValidateSlotSyncParams(void) ereport(ERROR, /* translator: %s is a GUC variable name */ errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("bad configuration for slot synchronization"), - errhint("\"%s\" must be enabled.", "hot_standby_feedback")); + errmsg("slot synchronization requires %s to be enabled", + "hot_standby_feedback")); /* Logical slot sync/creation requires wal_level >= logical. */ if (wal_level < WAL_LEVEL_LOGICAL) ereport(ERROR, errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("bad configuration for slot synchronization"), - errhint("\"wal_level\" must be >= logical.")); + errmsg("slot synchronization requires wal_level >= \"logical\"")); /* * The primary_conninfo is required to make connection to primary for @@ -822,8 +812,8 @@ ValidateSlotSyncParams(void) ereport(ERROR, /* translator: %s is a GUC variable name */ errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("bad configuration for slot synchronization"), - errhint("\"%s\" must be defined.", "primary_conninfo")); + errmsg("slot synchronization requires %s to be defined", + "primary_conninfo")); /* * The slot synchronization needs a database connection for walrcv_exec to @@ -834,12 +824,11 @@ ValidateSlotSyncParams(void) ereport(ERROR, /* - * translator: 'dbname' is a specific option; %s is a GUC variable - * name + * translator: dbname is a specific option; %s is a GUC variable name */ errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("bad configuration for slot synchronization"), - errhint("'dbname' must be specified in \"%s\".", "primary_conninfo")); + errmsg("slot synchronization requires dbname to be specified in %s", + "primary_conninfo")); } /* diff --git a/src/test/recovery/t/040_standby_failover_slots_sync.pl b/src/test/recovery/t/040_standby_failover_slots_sync.pl index 2755c3fc84..0f2f819f53 100644 --- a/src/test/recovery/t/040_standby_failover_slots_sync.pl +++ b/src/test/recovery/t/040_standby_failover_slots_sync.pl @@ -319,7 +319,7 @@ $standby1->reload; ($result, $stdout, $stderr) = $standby1->psql('postgres', "SELECT pg_sync_replication_slots();"); ok( $stderr =~ - /HINT: 'dbname' must be specified in "primary_conninfo"/, + /ERROR: slot synchronization requires dbname to be specified in primary_conninfo/, "cannot sync slots if dbname is not specified in primary_conninfo"); ##################################################