Simplify some logic in CreateReplicationSlot()

This refactoring reduces the code in charge of creating replication
slots from two "if" block to a single one, making it slightly cleaner.

This change is possible since 1d04a59be31b, that has removed the
intermediate code that existed between the two "if" blocks in charge of
initializing the output message buffer.

Author: Peter Smith
Discussion: https://postgr.es/m/CAHut+PtnJzqKT41Zt8pChRzba=QgCqjtfYvcf84NMj3VFJoKfw@mail.gmail.com

doc/src/sgml/Makefile

@@ -15,6 +15,8 @@
 # Make "html" the default target, since that is what most people tend
 # to want to use.
 html:
+# Note that all is *not* the default target in this directory
+all: html man
 
 # We don't need the tree-wide headers or install support here.
 NO_GENERATED_HEADERS=yes
@@ -25,8 +27,6 @@ top_builddir = ../../..
 include $(top_builddir)/src/Makefile.global
 
 
-all: html man
-
 
 ifndef DBTOEPUB
 DBTOEPUB = $(missing) dbtoepub
@@ -55,7 +55,7 @@ override XSLTPROCFLAGS += --stringparam pg.version '$(VERSION)'
 
 GENERATED_SGML = version.sgml \
 	features-supported.sgml features-unsupported.sgml errcodes-table.sgml \
-	keywords-table.sgml wait_event_types.sgml
+	keywords-table.sgml targets-meson.sgml wait_event_types.sgml
 
 ALLSGML := $(wildcard $(srcdir)/*.sgml $(srcdir)/ref/*.sgml) $(GENERATED_SGML)
 
@@ -110,6 +110,9 @@ keywords-table.sgml: $(top_srcdir)/src/include/parser/kwlist.h $(wildcard $(srcd
 wait_event_types.sgml: $(top_srcdir)/src/backend/utils/activity/wait_event_names.txt $(top_srcdir)/src/backend/utils/activity/generate-wait_event_types.pl
 	$(PERL) $(top_srcdir)/src/backend/utils/activity/generate-wait_event_types.pl --docs $<
 
+targets-meson.sgml: targets-meson.txt $(srcdir)/generate-targets-meson.pl
+	$(PERL) $(srcdir)/generate-targets-meson.pl $^ > $@
+
 ##
 ## Generation of some text files.
 ##

doc/src/sgml/config.sgml

@@ -7510,11 +7510,12 @@ log_line_prefix = '%m [%p] %q%u@%d/%a '
       </term>
       <listitem>
        <para>
-        Causes each replication command to be logged in the server log.
-        See <xref linkend="protocol-replication"/> for more information about
-        replication command. The default value is <literal>off</literal>.
-        Only superusers and users with the appropriate <literal>SET</literal>
-        privilege can change this setting.
+        Causes each replication command and <literal>walsender</literal>
+        process's replication slot acquisition/release to be logged in the
+        server log. See <xref linkend="protocol-replication"/> for more
+        information about replication command. The default value is
+        <literal>off</literal>. Only superusers and users with the appropriate
+        <literal>SET</literal> privilege can change this setting.
        </para>
       </listitem>
      </varlistentry>

doc/src/sgml/docguide.sgml

@@ -434,20 +434,19 @@ LOGLEVEL=-Dorg.apache.commons.logging.simplelog.defaultlog=WARN
   <title>Building the Documentation with Meson</title>
 
    <para>
-    Two options are provided for building the documentation using Meson.
-    Change to the <filename>build</filename> directory before running
-    one of these commands, or add <option>-C build</option> to the command.
+    To build the documentation using Meson, change to the
+    <filename>build</filename> directory before running one of these commands,
+    or add <option>-C build</option> to the command.
    </para>
 
    <para>
     To build just the <acronym>HTML</acronym> version of the documentation:
 <screen>
-<prompt>build$ </prompt><userinput>ninja docs</userinput>
-</screen>
-    To build all forms of the documentation:
-<screen>
-<prompt>build$ </prompt><userinput>ninja alldocs</userinput>
+<prompt>build$ </prompt><userinput>ninja html</userinput>
 </screen>
+    For a list of other documentation targets see
+    <xref linkend="targets-meson-documentation"/>.
+
     The output appears in the
     subdirectory <filename>build/doc/src/sgml</filename>.
    </para>

doc/src/sgml/filelist.sgml

@@ -38,6 +38,7 @@
 <!ENTITY high-availability      SYSTEM "high-availability.sgml">
 <!ENTITY installbin    SYSTEM "install-binaries.sgml">
 <!ENTITY installation  SYSTEM "installation.sgml">
+<!ENTITY targets-meson  SYSTEM "targets-meson.sgml">
 <!ENTITY installw      SYSTEM "install-windows.sgml">
 <!ENTITY maintenance   SYSTEM "maintenance.sgml">
 <!ENTITY manage-ag     SYSTEM "manage-ag.sgml">

doc/src/sgml/generate-targets-meson.pl

@@ -0,0 +1,63 @@
+#!/usr/bin/perl
+#
+# Generate the targets-meson.sgml file from targets-meson.txt
+# Copyright (c) 2000-2023, PostgreSQL Global Development Group
+
+use strict;
+use warnings;
+
+my $targets_meson_file = $ARGV[0];
+open my $targets_meson, '<', $targets_meson_file or die;
+
+print
+  "<!-- autogenerated from doc/src/sgml/targets-meson.txt, do not edit -->\n";
+
+# Find the start of each group of targets
+while (<$targets_meson>)
+{
+	next if /^#/;
+
+	if (/^(.*) Targets:$/)
+	{
+		my $targets = $1;
+		my $targets_id = lc $targets;
+
+		print qq(
+<sect3 id="targets-meson-$targets_id">
+ <title>$targets Targets</title>
+
+ <variablelist>
+);
+
+		# Each target in the group
+		while (<$targets_meson>)
+		{
+			next if /^#/;
+			last if !/^\s+([^ ]+)\s+(.+)/;
+
+			my $target = $1;
+			my $desc = $2;
+			my $target_id = $1;
+
+			$target_id =~ s/\//-/g;
+
+			print qq(
+  <varlistentry id="meson-target-${target_id}">
+    <term><option>${target}</option></term>
+    <listitem>
+     <para>
+      ${desc}
+     </para>
+    </listitem>
+  </varlistentry>
+);
+		}
+
+		print qq(
+ </variablelist>
+</sect3>
+);
+	}
+}
+
+close $targets_meson;

doc/src/sgml/installation.sgml

@@ -3200,6 +3200,21 @@ ninja install
     </variablelist>
    </sect3>
   </sect2>
+
+  <sect2 id="targets-meson">
+   <title><literal>meson</literal> Build Targets</title>
+
+   <para>
+    Individual build targets can be built using <command>ninja</command> <replaceable>target</replaceable>.
+
+    When no target is specified, everything except documentation is
+    built. Individual build products can be built using the path/filename as
+    <replaceable>target</replaceable>.
+   </para>
+
+   &targets-meson;
+  </sect2>
+
  </sect1>
 
  <sect1 id="install-post">

doc/src/sgml/meson.build

@@ -71,6 +71,15 @@ doc_generated += custom_target('keywords-table.sgml',
   capture: true,
 )
 
+doc_generated += custom_target('targets-meson.sgml',
+  input: files('targets-meson.txt'),
+  output: 'targets-meson.sgml',
+  command: [perl, files('generate-targets-meson.pl'), '@INPUT@'],
+  build_by_default: false,
+  install: false,
+  capture: true,
+)
+
 # For everything else we need at least xmllint
 if not xmllint_bin.found()
   subdir_done()
@@ -142,7 +151,8 @@ if docs_dep.found()
       '--install-dir-contents', dir_doc_html, html],
     build_always_stale: true, build_by_default: false,
   )
-  alias_target('install-doc-html', install_doc_html)
+  alias_target('html', html)
+  alias_target('install-html', install_doc_html)
 
   # build and install multi-page html docs as part of docs target
   docs += html
@@ -231,11 +241,12 @@ if docs_dep.found()
       '--install-dirs', dir_man, '@INPUT@'],
     build_always_stale: true, build_by_default: false,
   )
-  alias_target('install-doc-man', install_doc_man)
+  alias_target('man', man)
+  alias_target('install-man', install_doc_man)
 
-  # even though we don't want to build man pages as part of 'docs', we do want
-  # to install them as part of install-docs
+  # built and installed as part of the the docs target
   installdocs += install_doc_man
+  docs += man
 endif
 
 

doc/src/sgml/targets-meson.txt

@@ -0,0 +1,43 @@
+# Copyright (c) 2023, PostgreSQL Global Development Group
+#
+# Description of important meson targets, used for the 'help' target and
+# installation.sgml (via generate-targets-meson.pl). Right now the parsers are
+# extremely simple. Both parsers ignore comments. The help target prints
+# everything else. For xml everything without a leading newline is a group,
+# remaining lines are target separated by whitespace from their description
+#
+Code Targets:
+  all                           Build everything other than documentation
+  backend                       Build backend and related modules
+  bin                           Build frontend binaries
+  contrib                       Build contrib modules
+  pl                            Build procedual languages
+
+Developer Targets:
+  reformat-dat-files            Rewrite catalog data files into standard format
+  expand-dat-files              Expand all data files to include defaults
+  update-unicode                Update unicode data to new version
+
+Documentation Targets:
+  html                          Build documentation in multi-page HTML format
+  man                           Build documentation in man page format
+  docs                          Build documentation in multi-page HTML and man page format
+  doc/src/sgml/postgres-A4.pdf  Build documentation in PDF format, with A4 pages
+  doc/src/sgml/postgres-US.pdf  Build documentation in PDF format, with US letter pages
+  doc/src/sgml/postgres.html    Build documentation in single-page HTML format
+  alldocs                       Build documentation in all supported formats
+
+Installation Targets:
+  install                       Install postgres, excluding documentation
+  install-docs                  Install documentation in multi-page HTML and man page formats
+  install-html                  Install documentation in multi-page HTML format
+  install-man                   Install documentation in man page format
+  install-quiet                 Like "install", but installed files are not displayed
+  install-world                 Install postgres, including multi-page HTML and man page documentation
+  uninstall                     Remove installed files
+
+Other Targets:
+  clean                         Remove all build products
+  test                          Run all enabled tests (including contrib)
+  world                         Build everything, including documentation
+  help                          List important targets

meson.build

@@ -3331,8 +3331,17 @@ alias_target('bin', bin_targets + [libpq_st])
 alias_target('pl', pl_targets)
 alias_target('contrib', contrib_targets)
 alias_target('testprep', testprep_targets)
+
+alias_target('world', all_built, docs)
 alias_target('install-world', install_quiet, installdocs)
 
+run_target('help',
+  command: [
+    perl, '-ne', 'next if /^#/; print',
+    files('doc/src/sgml/targets-meson.txt'),
+  ]
+)
+
 
 
 ###############################################################

src/backend/catalog/namespace.c

@@ -235,6 +235,10 @@ static bool MatchNamedCall(HeapTuple proctup, int nargs, List *argnames,
  * when a function has search_path set in proconfig. Add a search path cache
  * that can be used by recomputeNamespacePath().
  *
+ * The cache is also used to remember already-validated strings in
+ * check_search_path() to avoid the need to call SplitIdentifierString()
+ * repeatedly.
+ *
  * The search path cache is based on a wrapper around a simplehash hash table
  * (nsphash, defined below). The spcache wrapper deals with OOM while trying
  * to initialize a key, and also offers a more convenient API.
@@ -279,40 +283,36 @@ spcachekey_equal(SearchPathCacheKey a, SearchPathCacheKey b)
 static nsphash_hash * SearchPathCache = NULL;
 
 /*
- * Create search path cache.
+ * Create or reset search_path cache as necessary.
  */
 static void
 spcache_init(void)
 {
 	Assert(SearchPathCacheContext);
 
-	if (SearchPathCache)
+	if (SearchPathCache && searchPathCacheValid &&
+		SearchPathCache->members < SPCACHE_RESET_THRESHOLD)
 		return;
 
+	MemoryContextReset(SearchPathCacheContext);
 	/* arbitrary initial starting size of 16 elements */
 	SearchPathCache = nsphash_create(SearchPathCacheContext, 16, NULL);
 	searchPathCacheValid = true;
 }
 
 /*
- * Reset and reinitialize search path cache.
+ * Look up entry in search path cache without inserting. Returns NULL if not
+ * present.
  */
-static void
-spcache_reset(void)
+static SearchPathCacheEntry *
+spcache_lookup(const char *searchPath, Oid roleid)
 {
-	Assert(SearchPathCacheContext);
-	Assert(SearchPathCache);
+	SearchPathCacheKey cachekey = {
+		.searchPath = searchPath,
+		.roleid = roleid
+	};
 
-	MemoryContextReset(SearchPathCacheContext);
-	SearchPathCache = NULL;
-
-	spcache_init();
-}
-
-static uint32
-spcache_members(void)
-{
-	return SearchPathCache->members;
+	return nsphash_lookup(SearchPathCache, cachekey);
 }
 
 /*
@@ -325,27 +325,25 @@ static SearchPathCacheEntry *
 spcache_insert(const char *searchPath, Oid roleid)
 {
 	SearchPathCacheEntry *entry;
-	bool		found;
 	SearchPathCacheKey cachekey = {
 		.searchPath = searchPath,
 		.roleid = roleid
 	};
 
 	/*
-	 * If a new entry is created, we must ensure that it's properly
-	 * initialized. Set the cache invalid temporarily, so that if the
-	 * MemoryContextStrdup() below raises an OOM, the cache will be reset on
-	 * the next use, clearing the uninitialized entry.
+	 * searchPath is not saved in SearchPathCacheContext. First perform a
+	 * lookup, and copy searchPath only if we need to create a new entry.
 	 */
-	searchPathCacheValid = false;
+	entry = nsphash_lookup(SearchPathCache, cachekey);
 
-	entry = nsphash_insert(SearchPathCache, cachekey, &found);
-
-	/* ensure that key is initialized and the rest is zeroed */
-	if (!found)
+	if (!entry)
 	{
-		entry->key.searchPath = MemoryContextStrdup(SearchPathCacheContext, searchPath);
-		entry->key.roleid = roleid;
+		bool		found;
+
+		cachekey.searchPath = MemoryContextStrdup(SearchPathCacheContext, searchPath);
+		entry = nsphash_insert(SearchPathCache, cachekey, &found);
+		Assert(!found);
+
 		entry->oidlist = NIL;
 		entry->finalPath = NIL;
 		entry->firstNS = InvalidOid;
@@ -354,7 +352,6 @@ spcache_insert(const char *searchPath, Oid roleid)
 		/* do not touch entry->status, used by simplehash */
 	}
 
-	searchPathCacheValid = true;
 	return entry;
 }
 
@@ -4183,31 +4180,15 @@ finalNamespacePath(List *oidlist, Oid *firstNS)
 /*
  * Retrieve search path information from the cache; or if not there, fill
  * it. The returned entry is valid only until the next call to this function.
- *
- * We also determine if the newly-computed finalPath is the same as the
- * prevPath passed by the caller (i.e. a no-op or a real change?). It's more
- * efficient to check for a change in this function than the caller, because
- * we can avoid unnecessary temporary copies of the previous path.
  */
 static const SearchPathCacheEntry *
-cachedNamespacePath(const char *searchPath, Oid roleid, List *prevPath,
-					bool *same)
+cachedNamespacePath(const char *searchPath, Oid roleid)
 {
 	MemoryContext oldcxt;
 	SearchPathCacheEntry *entry;
-	List	   *prevPathCopy = NIL;
 
 	spcache_init();
 
-	/* invalidate cache if necessary */
-	if (!searchPathCacheValid || spcache_members() >= SPCACHE_RESET_THRESHOLD)
-	{
-		/* prevPath will be destroyed; make temp copy for later comparison */
-		prevPathCopy = list_copy(prevPath);
-		prevPath = prevPathCopy;
-		spcache_reset();
-	}
-
 	entry = spcache_insert(searchPath, roleid);
 
 	/*
@@ -4232,38 +4213,22 @@ cachedNamespacePath(const char *searchPath, Oid roleid, List *prevPath,
 	if (entry->finalPath == NIL || object_access_hook ||
 		entry->forceRecompute)
 	{
-		/*
-		 * Do not free the stale value of entry->finalPath until we've
-		 * performed the comparison, in case it's aliased by prevPath (which
-		 * can only happen when recomputing due to an object_access_hook).
-		 */
-		List	   *finalPath;
+		list_free(entry->finalPath);
+		entry->finalPath = NIL;
 
 		oldcxt = MemoryContextSwitchTo(SearchPathCacheContext);
-		finalPath = finalNamespacePath(entry->oidlist,
-									   &entry->firstNS);
+		entry->finalPath = finalNamespacePath(entry->oidlist,
+											  &entry->firstNS);
 		MemoryContextSwitchTo(oldcxt);
 
-		*same = equal(prevPath, finalPath);
-
-		list_free(entry->finalPath);
-		entry->finalPath = finalPath;
-
 		/*
-		 * If an object_access_hook set when finalPath is calculated, the
+		 * If an object_access_hook is set when finalPath is calculated, the
 		 * result may be affected by the hook. Force recomputation of
 		 * finalPath the next time this cache entry is used, even if the
 		 * object_access_hook is not set at that time.
 		 */
 		entry->forceRecompute = object_access_hook ? true : false;
 	}
-	else
-	{
-		/* use cached version of finalPath */
-		*same = equal(prevPath, entry->finalPath);
-	}
-
-	list_free(prevPathCopy);
 
 	return entry;
 }
@@ -4275,7 +4240,6 @@ static void
 recomputeNamespacePath(void)
 {
 	Oid			roleid = GetUserId();
-	bool		newPathEqual;
 	bool		pathChanged;
 	const		SearchPathCacheEntry *entry;
 
@@ -4283,24 +4247,32 @@ recomputeNamespacePath(void)
 	if (baseSearchPathValid && namespaceUser == roleid)
 		return;
 
-	entry = cachedNamespacePath(namespace_search_path, roleid, baseSearchPath,
-								&newPathEqual);
+	entry = cachedNamespacePath(namespace_search_path, roleid);
 
 	if (baseCreationNamespace == entry->firstNS &&
 		baseTempCreationPending == entry->temp_missing &&
-		newPathEqual)
+		equal(entry->finalPath, baseSearchPath))
 	{
 		pathChanged = false;
 	}
 	else
 	{
-		pathChanged = true;
-	}
+		MemoryContext oldcxt;
+		List	   *newpath;
 
-	/* Now safe to assign to state variables. */
-	baseSearchPath = entry->finalPath;
-	baseCreationNamespace = entry->firstNS;
-	baseTempCreationPending = entry->temp_missing;
+		pathChanged = true;
+
+		/* Must save OID list in permanent storage. */
+		oldcxt = MemoryContextSwitchTo(TopMemoryContext);
+		newpath = list_copy(entry->finalPath);
+		MemoryContextSwitchTo(oldcxt);
+
+		/* Now safe to assign to state variables. */
+		list_free(baseSearchPath);
+		baseSearchPath = newpath;
+		baseCreationNamespace = entry->firstNS;
+		baseTempCreationPending = entry->temp_missing;
+	}
 
 	/* Mark the path valid. */
 	baseSearchPathValid = true;
@@ -4625,11 +4597,40 @@ ResetTempTableNamespace(void)
 bool
 check_search_path(char **newval, void **extra, GucSource source)
 {
+	Oid			roleid = InvalidOid;
+	const char *searchPath = *newval;
 	char	   *rawname;
 	List	   *namelist;
+	bool		use_cache = (SearchPathCacheContext != NULL);
 
-	/* Need a modifiable copy of string */
-	rawname = pstrdup(*newval);
+	/*
+	 * We used to try to check that the named schemas exist, but there are
+	 * many valid use-cases for having search_path settings that include
+	 * schemas that don't exist; and often, we are not inside a transaction
+	 * here and so can't consult the system catalogs anyway.  So now, the only
+	 * requirement is syntactic validity of the identifier list.
+	 */
+
+	/*
+	 * Checking only the syntactic validity also allows us to use the search
+	 * path cache (if available) to avoid calling SplitIdentifierString() on
+	 * the same string repeatedly.
+	 */
+	if (use_cache)
+	{
+		spcache_init();
+
+		roleid = GetUserId();
+
+		if (spcache_lookup(searchPath, roleid) != NULL)
+			return true;
+	}
+
+	/*
+	 * Ensure validity check succeeds before creating cache entry.
+	 */
+
+	rawname = pstrdup(searchPath);	/* need a modifiable copy */
 
 	/* Parse string into list of identifiers */
 	if (!SplitIdentifierString(rawname, ',', &namelist))
@@ -4652,6 +4653,10 @@ check_search_path(char **newval, void **extra, GucSource source)
 	pfree(rawname);
 	list_free(namelist);
 
+	/* create empty cache entry */
+	if (use_cache)
+		(void) spcache_insert(searchPath, roleid);
+
 	return true;
 }
 

src/backend/replication/slot.c

@@ -537,6 +537,16 @@ retry:
 	 */
 	if (SlotIsLogical(s))
 		pgstat_acquire_replslot(s);
+
+	if (am_walsender)
+	{
+		ereport(log_replication_commands ? LOG : DEBUG1,
+				SlotIsLogical(s)
+				? errmsg("acquired logical replication slot \"%s\"",
+						 NameStr(s->data.name))
+				: errmsg("acquired physical replication slot \"%s\"",
+						 NameStr(s->data.name)));
+	}
 }
 
 /*
@@ -549,9 +559,17 @@ void
 ReplicationSlotRelease(void)
 {
 	ReplicationSlot *slot = MyReplicationSlot;
+	char	   *slotname = NULL;	/* keep compiler quiet */
+	bool		is_logical = false; /* keep compiler quiet */
 
 	Assert(slot != NULL && slot->active_pid != 0);
 
+	if (am_walsender)
+	{
+		slotname = pstrdup(NameStr(slot->data.name));
+		is_logical = SlotIsLogical(slot);
+	}
+
 	if (slot->data.persistency == RS_EPHEMERAL)
 	{
 		/*
@@ -596,6 +614,18 @@ ReplicationSlotRelease(void)
 	MyProc->statusFlags &= ~PROC_IN_LOGICAL_DECODING;
 	ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
 	LWLockRelease(ProcArrayLock);
+
+	if (am_walsender)
+	{
+		ereport(log_replication_commands ? LOG : DEBUG1,
+				is_logical
+				? errmsg("released logical replication slot \"%s\"",
+						 slotname)
+				: errmsg("released physical replication slot \"%s\"",
+						 slotname));
+
+		pfree(slotname);
+	}
 }
 
 /*

src/backend/replication/walsender.c

@@ -1061,9 +1061,25 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd)
 		ReplicationSlotCreate(cmd->slotname, false,
 							  cmd->temporary ? RS_TEMPORARY : RS_PERSISTENT,
 							  false);
+
+		if (reserve_wal)
+		{
+			ReplicationSlotReserveWal();
+
+			ReplicationSlotMarkDirty();
+
+			/* Write this slot to disk if it's a permanent one. */
+			if (!cmd->temporary)
+				ReplicationSlotSave();
+		}
 	}
 	else
 	{
+		LogicalDecodingContext *ctx;
+		bool		need_full_snapshot = false;
+
+		Assert(cmd->kind == REPLICATION_KIND_LOGICAL);
+
 		CheckLogicalDecodingRequirements();
 
 		/*
@@ -1076,12 +1092,6 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd)
 		ReplicationSlotCreate(cmd->slotname, true,
 							  cmd->temporary ? RS_TEMPORARY : RS_EPHEMERAL,
 							  two_phase);
-	}
-
-	if (cmd->kind == REPLICATION_KIND_LOGICAL)
-	{
-		LogicalDecodingContext *ctx;
-		bool		need_full_snapshot = false;
 
 		/*
 		 * Do options check early so that we can bail before calling the
@@ -1175,16 +1185,6 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd)
 		if (!cmd->temporary)
 			ReplicationSlotPersist();
 	}
-	else if (cmd->kind == REPLICATION_KIND_PHYSICAL && reserve_wal)
-	{
-		ReplicationSlotReserveWal();
-
-		ReplicationSlotMarkDirty();
-
-		/* Write this slot to disk if it's a permanent one. */
-		if (!cmd->temporary)
-			ReplicationSlotSave();
-	}
 
 	snprintf(xloc, sizeof(xloc), "%X/%X",
 			 LSN_FORMAT_ARGS(MyReplicationSlot->data.confirmed_flush));