Feedback-ID: ia2694551:Fastmail
Date: Thu, 28 Nov 2024 13:16:18 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Amit Kapila <amit.kapila16@gmail.com>,
	"Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>,
 Gabriele Bartolini <gabriele.bartolini@enterprisedb.com>
Subject: crash with synchronized_standby_slots
Message-ID: <202411281216.sutbxtr6idnn@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="oc4jx2uaztfzzqyp"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/202411281216.sutbxtr6idnn%40alvherre.pgsql>
Precedence: bulk


--oc4jx2uaztfzzqyp
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Hello,

Gabriele just reported a crash when changing synchronized_standby_slots
under SIGHUP and logging collector working.  The problem apparently is
that validate_sync_standby_slots is run for the GUC check routine, and
it requires acquiring an LWLock; but syslogger doesn't have a PGPROC so
that doesn't work.

Apparently we already have a hack in place against the same problem in
postmaster (and elsewhere?), by testing that ReplicationSlotCtl is not
null.  But that hack seems to be incomplete, as the crash here attests.

To reproduce, simply start with no synchronized_standby_slots setting
and logging_collector=on, then change the value in postgresql.conf and
reload.

One option to fix this might be as attached.  The point here is that
processes without a PGPROC don't need or care to have a correct setting,
so we just skip verifying it on those.  AFAICS this is more general than
the test for ReplicationSlotCtl, so I just removed that one.

Here's the backtrace Gabriele showed me.

[New LWP 29]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `postgres: freddie: logger                  '.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  pg_atomic_read_u32_impl (ptr=0x7f0cf21a1f84) at ./build/../src/include/port/atomics/generic.h:48
Download failed: Invalid argument.  Continuing without source file ./build/src/backend/storage/lmgr/./build/../src/include/port/atomics/generic.h.
48	./build/../src/include/port/atomics/generic.h: No such file or directory.
#0  pg_atomic_read_u32_impl (ptr=0x7f0cf21a1f84) at ./build/../src/include/port/atomics/generic.h:48
No locals.
#1  pg_atomic_read_u32 (ptr=0x7f0cf21a1f84) at ./build/../src/include/port/atomics.h:237
No locals.
#2  LWLockAttemptLock (lock=lock@entry=0x7f0cf21a1f80, mode=mode@entry=LW_SHARED) at ./build/../src/backend/storage/lmgr/lwlock.c:796
        old_state = <optimized out>
#3  0x0000557cbe97ba60 in LWLockAcquire (lock=0x7f0cf21a1f80, mode=mode@entry=LW_SHARED) at ./build/../src/backend/storage/lmgr/lwlock.c:1235
        mustwait = <optimized out>
        proc = 0x0
        result = true
        extraWaits = 0
        __func__ = "LWLockAcquire"
#4  0x0000557cbe935919 in validate_sync_standby_slots (elemlist=0x7ffda5aa60d0, rawname=0x557cfd704c98 "_cnpg_freddie_2") at ./build/../src/backend/replication/slot.c:2443
        ok = true
        ok = <optimized out>
        name = <optimized out>
        name__outerloop = <optimized out>
        name__state = {l = <optimized out>, i = <optimized out>}
        slot = <optimized out>
#5  check_synchronized_standby_slots (newval=<optimized out>, extra=0x7ffda5aa6300, source=<optimized out>) at ./build/../src/backend/replication/slot.c:2494
        rawname = 0x557cfd704c98 "_cnpg_freddie_2"
        ptr = <optimized out>
        elemlist = 0x557cfd7041c8
        size = <optimized out>
        ok = <optimized out>
        config = <optimized out>
#6  0x0000557cbeae95de in call_string_check_hook (conf=conf@entry=0x557cbee2f8b0 <ConfigureNamesString+14000>, newval=newval@entry=0x7ffda5aa62f8, extra=extra@entry=0x7ffda5aa6300, source=source@entry=PGC_S_FILE, elevel=elevel@entry=12) at ./build/../src/backend/utils/misc/guc.c:6925
        _save_exception_stack = 0x0
        _save_context_stack = 0x0
        _local_sigjmp_buf = {{__jmpbuf = {93994266851504, -6990900416883569069, 140727382860536, 12, 93994266851504, 93995316414744, -6990900416982135213, -3748856269321122221}, __mask_was_saved = 0, __saved_mask = {__val = {140727382860536, 140727382860272, 93995316305072, 139693771467072, 93994263427300, 4662694561863172096, 93994263387537, 140727382860536, 93994263505961, 93994266851504, 140727382860536, 140727382860336, 93994263421816, 140727382860536, 12, 93994266851504}}}}
        _do_rethrow = false
        result = true
        __func__ = "call_string_check_hook"
#7  0x0000557cbeaec1ba in parse_and_validate_value (record=0x557cbee2f8b0 <ConfigureNamesString+14000>, name=0x557cfd765840 "synchronized_standby_slots", value=<optimized out>, source=PGC_S_FILE, elevel=12, newval=0x7ffda5aa62f8, newextra=0x7ffda5aa6300) at ./build/../src/backend/utils/misc/guc.c:3260
        conf = 0x557cbee2f8b0 <ConfigureNamesString+14000>
        __func__ = "parse_and_validate_value"
#8  0x0000557cbeaecd6f in set_config_with_handle (name=0x557cfd765840 "synchronized_standby_slots", handle=<optimized out>, value=0x557cfd765868 "_cnpg_freddie_2", context=PGC_SIGHUP, source=PGC_S_FILE, srole=10, action=GUC_ACTION_SET, changeVal=<optimized out>, elevel=12, is_reload=false) at ./build/../src/backend/utils/misc/guc.c:4013
        conf = 0x557cbee2f8b0 <ConfigureNamesString+14000>
        orig_context = PGC_SIGHUP
        orig_source = PGC_S_FILE
        orig_srole = 10
        record = 0x557cbee2f8b0 <ConfigureNamesString+14000>
        newval_union = {boolval = 24, intval = -42858216, realval = 4.6439856710502817e-310, stringval = 0x557cfd720918 "_cnpg_freddie_2", enumval = -42858216}
        newextra = 0x0
        prohibitValueChange = false
        makeDefault = <optimized out>
        __func__ = "set_config_with_handle"
#9  0x0000557cbeaef400 in set_config_option (is_reload=false, elevel=0, changeVal=true, action=GUC_ACTION_SET, source=PGC_S_FILE, context=PGC_SIGHUP, value=<optimized out>, name=<optimized out>) at ./build/../src/backend/utils/misc/guc.c:3363
        srole = 10
        srole = <optimized out>
#10 ProcessConfigFileInternal (context=context@entry=PGC_SIGHUP, applySettings=applySettings@entry=true, elevel=elevel@entry=13) at ./build/../src/backend/utils/misc/guc.c:557
        pre_value = 0x0
        scres = <optimized out>
        error = false
        applying = true
        ConfFileWithError = 0x557cfd720100 "/var/lib/postgresql/data/pgdata/postgresql.conf"
        item = 0x557cfd765880
        head = 0x557cfd704248
        tail = 0x557cfd7664c0
        status = {hashp = 0x557cfd7219c8, curBucket = 512, curEntry = 0x0}
        hentry = <optimized out>
        bail_out = <optimized out>
        __func__ = "ProcessConfigFileInternal"
#11 0x0000557cbeaf2e81 in ProcessConfigFile (context=context@entry=PGC_SIGHUP) at ./build/../src/backend/utils/misc/guc-file.l:152
        elevel = 13
        config_cxt = 0x557cfd7040d0
        caller_cxt = 0x557cfd6fd800
#12 0x0000557cbe9001cc in SysLoggerMain (startup_data=<optimized out>, startup_data_len=<optimized out>) at ./build/../src/backend/postmaster/syslogger.c:369
        time_based_rotation = false
        size_rotation_for = 0
        cur_timeout = <optimized out>
        event = {pos = 0, events = 1, fd = -1, user_data = 0x0}
        rc = <optimized out>
        logbuffer = "\000\000\300\000\034\000\000\000!2024-11-28 10:19:42.415 UTC,,,28,,6748439f.1c,8,,2024-11-28 10:19:11 UTC,,0,LOG,00000,\"parameter \"\"synchronous_standby_names\"\" changed to \"\"ANY 1 (\"\"freddie-2\"\")\"\"\",,,,,,,,,\"\",\"postmaster\",,0"...
        bytes_in_logbuffer = 0
        currentLogDir = 0x557cfd6fe6d8 "/controller/log"
        currentLogFilename = 0x557cfd6fe6f0 "postgres"
        currentLogRotationAge = 0
        now = 1732789151
        wes = 0x557cfd6fe708
        __func__ = "SysLoggerMain"
#13 0x0000557cbe8f908a in postmaster_child_launch (child_type=child_type@entry=B_LOGGER, startup_data=startup_data@entry=0x0, startup_data_len=startup_data_len@entry=0, client_sock=client_sock@entry=0x0) at ./build/../src/backend/postmaster/launch_backend.c:277
        pid = <optimized out>
#14 0x0000557cbe8ff9c2 in SysLogger_Start () at ./build/../src/backend/postmaster/syslogger.c:706
        sysloggerPid = <optimized out>
        filename = 0x557cfd75a160 "og output will go to log destination \"csvlog\".\n"
        __func__ = "SysLogger_Start"
#15 0x0000557cbe8fe42e in PostmasterMain (argc=argc@entry=3, argv=argv@entry=0x557cfd6fd7a0) at ./build/../src/backend/postmaster/postmaster.c:1054
        opt = <optimized out>
        status = <optimized out>
        userDoption = <optimized out>
        listen_addr_saved = false
        output_config_variable = 0x0
        __func__ = "PostmasterMain"
#16 0x0000557cbe6258ec in main (argc=3, argv=0x557cfd6fd7a0) at ./build/../src/backend/main/main.c:197
        do_check_root = <optimized out>

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Once again, thank you and all of the developers for your hard work on
PostgreSQL.  This is by far the most pleasant management experience of
any database I've worked on."                             (Dan Harris)
http://archives.postgresql.org/pgsql-performance/2006-04/msg00247.php

--oc4jx2uaztfzzqyp
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="0001-Fix-synchronized_standby_slots-GUC-check-hook.patch"

From 2d38870be86200deefb9979b9dd0a164d5986499 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@alvh.no-ip.org>
Date: Thu, 28 Nov 2024 13:11:20 +0100
Subject: [PATCH] Fix synchronized_standby_slots GUC check hook

It requires an LWLock, so it cannot run in processes without PGPROC.
Skip it there; it makes no difference.

Reported-by: Gabriele Bartolini <gabriele.bartolini@enterprisedb.com>
---
 src/backend/replication/slot.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 77bae536915..b6e43c1a89e 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -2428,18 +2428,16 @@ validate_sync_standby_slots(char *rawname, List **elemlist)
 	{
 		GUC_check_errdetail("List syntax is invalid.");
 	}
-	else if (!ReplicationSlotCtl)
+	else if (MyProc)
 	{
 		/*
-		 * We cannot validate the replication slot if the replication slots'
-		 * data has not been initialized. This is ok as we will anyway
-		 * validate the specified slot when waiting for them to catch up. See
-		 * StandbySlotsHaveCaughtup() for details.
+		 * Check that the specified slots exist and are logical slots.  Note
+		 * that because we need an LWLock, we cannot do this on processes
+		 * without a PGPROC (hello syslogger!) so we skip it there, but it
+		 * doesn't have an effect for those anyway.
+		 *
+		 * See StandbySlotsHaveCaughtup() for details.
 		 */
-	}
-	else
-	{
-		/* Check that the specified slots exist and are logical slots */
 		LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
 
 		foreach_ptr(char, name, *elemlist)
-- 
2.39.5


--oc4jx2uaztfzzqyp--