Re: Libpq support to connect to standby server as priority - Mailing list pgsql-hackers

I wrote:
> BTW, I think it would be worth splitting this into separate server-side
> and libpq patches.  It looked to me like the server side is pretty
> nearly committable, modulo bikeshedding about the new GUC name.

Actually ... I looked that over again and got a bit more queasy about
all the new signaling logic it is adding.  Signals are inherently
bug-prone stuff, plus it's not very clear what sort of guarantees
we'd have about either the reliability or the timeliness of client
notifications about exiting hot-standby mode.

I also wonder what consideration has been given to the performance
implications of marking transaction_read_only as GUC_REPORT, thus
causing client traffic to occur every time it's changed.  Most of
the current GUC_REPORT variables don't change too often in typical
sessions, but I'm less convinced about that for transaction_read_only.

So I thought about alternative ways to implement this, and realized
that it would not be hard to make guc.c handle it all by itself, if
we use a custom show-hook for the in_hot_standby GUC that calls
RecoveryInProgress() instead of examining purely static state.
Now, by itself that idea only takes care of the session-start-time
report, because there'd never be any GUC action causing a new
report to occur.  But we can improve the situation if we get rid
of the current design whereby ReportGUCOption() is called immediately
when any GUC value changes, and instead batch up the reports to
occur when we're about to go idle waiting for a new client query.

Not incidentally, this responds to a concern Robert mentioned awhile
back about the performance of GUC reporting [1].  You can already get
the server to spam the client excessively if any GUC_REPORT variables
are changed by, for example, functions' SET clauses, because that could
lead to the active value changing many times within a query.  We've
gotten away with that so far, but it'd be a problem if any more-often-
changed variables get marked GUC_REPORT.  (I actually have a vague
memory of other complaints about that, but I couldn't find any in a
desultory search of the archives.)

So I present 0001 attached which changes the GUC_REPORT code to work
that way, and then 0002 is a pretty small hack to add a reportable
in_hot_standby GUC by having the end-of-query function check (when
needed) to see if the active value changed.

As it stands, 0001 reduces the ParameterStatus message traffic to
at most one per GUC per query, but it doesn't attempt to eliminate
duplicate ParameterStatus messages altogether.  We could do that
as a pretty simple adjustment if we're willing to expend the storage
to remember the last value sent to the client.  It might be worth
doing, since for example the function-SET-clause case would typically
lead to no net change in the GUC's value by the end of the query.

An objection that could be raised to this approach for in_hot_standby
is that it will only report in_hot_standby becoming false at the end
of a query, whereas the v19 patch at least attempts to deliver an
async ParameterStatus message when the client is idle (and, I think,
indeed may fail to provide *any* message if the transition occurs
when it isn't idle).  I don't find that too compelling though;
libpq-based clients, at least, don't have any very practical way to
deal with async ParameterStatus messages anyway.

(Note that I did not touch the docs here, so that while 0001 might
be committable as-is, 0002 is certainly just WIP.)

BTW, as far as the transaction_read_only side of things goes, IMO
it would make a lot more sense to mark default_transaction_read_only
as GUC_REPORT, since that changes a lot less frequently.  We'd then
have to expend some work to report that value honestly, since right
now the hot-standby code cheats by ignoring the GUC's value during
hot standby.  But I think a technique much like 0002's would work
for that.

Thoughts?

            regards, tom lane

[1] https://www.postgresql.org/message-id/CA%2BTgmoaDoVtMnfKNFm-iyyCSp%3DFPiHkfU1AXuEHJqmcLTAX6kQ%40mail.gmail.com

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 411cfadbff..b67cc2f375 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4233,6 +4233,9 @@ PostgresMain(int argc, char *argv[],
                 pgstat_report_activity(STATE_IDLE, NULL);
             }

+            /* Report any recently-changed GUC options */
+            ReportChangedGUCOptions();
+
             ReadyForQuery(whereToSendOutput);
             send_ready_for_query = false;
         }
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 596bcb7b84..ddfc7ea05d 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -4822,6 +4822,8 @@ static bool guc_dirty;            /* true if need to do commit/abort work */

 static bool reporting_enabled;    /* true to enable GUC_REPORT */

+static bool report_needed;        /* true if any GUC_REPORT reports are needed */
+
 static int    GUCNestLevel = 0;    /* 1 when in main transaction */


@@ -5828,7 +5830,10 @@ ResetAllOptions(void)
         gconf->scontext = gconf->reset_scontext;

         if (gconf->flags & GUC_REPORT)
-            ReportGUCOption(gconf);
+        {
+            gconf->status |= GUC_NEEDS_REPORT;
+            report_needed = true;
+        }
     }
 }

@@ -6215,7 +6220,10 @@ AtEOXact_GUC(bool isCommit, int nestLevel)

             /* Report new value if we changed it */
             if (changed && (gconf->flags & GUC_REPORT))
-                ReportGUCOption(gconf);
+            {
+                gconf->status |= GUC_NEEDS_REPORT;
+                report_needed = true;
+            }
         }                        /* end of stack-popping loop */

         if (stack != NULL)
@@ -6257,26 +6265,64 @@ BeginReportingGUCOptions(void)
         if (conf->flags & GUC_REPORT)
             ReportGUCOption(conf);
     }
+
+    report_needed = false;
 }

 /*
- * ReportGUCOption: if appropriate, transmit option value to frontend
+ * Report recently-changed GUC_REPORT variables.
+ * This is called just before we wait for a new client query.
+ *
+ * By handling things this way, we ensure that a ParameterStatus message
+ * is sent at most once per variable per query, even if the variable
+ * changed multiple times within the query.  That's quite possible when
+ * using features such as function SET clauses.  We do not, however, go to
+ * the length of trying to suppress sending anything when the variable was
+ * changed and then reverted to its original value.
+ */
+void
+ReportChangedGUCOptions(void)
+{
+    /* Quick exit if not (yet) enabled */
+    if (!reporting_enabled)
+        return;
+
+    /* Quick exit if no values have been changed */
+    if (!report_needed)
+        return;
+
+    /* Transmit new values of interesting variables */
+    for (int i = 0; i < num_guc_variables; i++)
+    {
+        struct config_generic *conf = guc_variables[i];
+
+        if ((conf->flags & GUC_REPORT) && (conf->status & GUC_NEEDS_REPORT))
+            ReportGUCOption(conf);
+    }
+
+    report_needed = false;
+}
+
+/*
+ * ReportGUCOption: transmit option value to frontend
+ *
+ * Caller is now fully responsible for deciding whether this should be done.
+ * However, we do clear the NEEDS_REPORT flag here.
  */
 static void
 ReportGUCOption(struct config_generic *record)
 {
-    if (reporting_enabled && (record->flags & GUC_REPORT))
-    {
-        char       *val = _ShowOption(record, false);
-        StringInfoData msgbuf;
+    char       *val = _ShowOption(record, false);
+    StringInfoData msgbuf;

-        pq_beginmessage(&msgbuf, 'S');
-        pq_sendstring(&msgbuf, record->name);
-        pq_sendstring(&msgbuf, val);
-        pq_endmessage(&msgbuf);
+    pq_beginmessage(&msgbuf, 'S');
+    pq_sendstring(&msgbuf, record->name);
+    pq_sendstring(&msgbuf, val);
+    pq_endmessage(&msgbuf);

-        pfree(val);
-    }
+    pfree(val);
+
+    record->status &= ~GUC_NEEDS_REPORT;
 }

 /*
@@ -7667,7 +7713,10 @@ set_config_option(const char *name, const char *value,
     }

     if (changeVal && (record->flags & GUC_REPORT))
-        ReportGUCOption(record);
+    {
+        record->status |= GUC_NEEDS_REPORT;
+        report_needed = true;
+    }

     return changeVal ? 1 : -1;
 }
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 2819282181..76236fb0c0 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -362,6 +362,7 @@ extern void AtStart_GUC(void);
 extern int    NewGUCNestLevel(void);
 extern void AtEOXact_GUC(bool isCommit, int nestLevel);
 extern void BeginReportingGUCOptions(void);
+extern void ReportChangedGUCOptions(void);
 extern void ParseLongOption(const char *string, char **name, char **value);
 extern bool parse_int(const char *value, int *result, int flags,
                       const char **hintmsg);
diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h
index 04431d0eb2..a29c2b01b4 100644
--- a/src/include/utils/guc_tables.h
+++ b/src/include/utils/guc_tables.h
@@ -172,7 +172,8 @@ struct config_generic
  * Caution: the GUC_IS_IN_FILE bit is transient state for ProcessConfigFile.
  * Do not assume that its value represents useful information elsewhere.
  */
-#define GUC_PENDING_RESTART 0x0002
+#define GUC_PENDING_RESTART 0x0002    /* changed value cannot be applied yet */
+#define GUC_NEEDS_REPORT    0x0004    /* new value must be reported to client */


 /* GUC records for specific variable types */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index ddfc7ea05d..207dc9bf4d 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -209,6 +209,7 @@ static bool check_cluster_name(char **newval, void **extra, GucSource source);
 static const char *show_unix_socket_permissions(void);
 static const char *show_log_file_mode(void);
 static const char *show_data_directory_mode(void);
+static const char *show_in_hot_standby(void);
 static bool check_backtrace_functions(char **newval, void **extra, GucSource source);
 static void assign_backtrace_functions(const char *newval, void *extra);
 static bool check_recovery_target_timeline(char **newval, void **extra, GucSource source);
@@ -607,6 +608,8 @@ static int    max_identifier_length;
 static int    block_size;
 static int    segment_size;
 static int    wal_block_size;
+static bool in_hot_standby;
+static bool last_reported_in_hot_standby;
 static bool data_checksums;
 static bool integer_datetimes;
 static bool assert_enabled;
@@ -1844,6 +1847,17 @@ static struct config_bool ConfigureNamesBool[] =
         NULL, NULL, NULL
     },

+    {
+        {"in_hot_standby", PGC_INTERNAL, PRESET_OPTIONS,
+            gettext_noop("Shows whether hot standby is currently active."),
+            NULL,
+            GUC_REPORT | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+        },
+        &in_hot_standby,
+        false,
+        NULL, NULL, show_in_hot_standby
+    },
+
     {
         {"allow_system_table_mods", PGC_SUSET, DEVELOPER_OPTIONS,
             gettext_noop("Allows modifications of the structure of system tables."),
@@ -6266,6 +6280,9 @@ BeginReportingGUCOptions(void)
             ReportGUCOption(conf);
     }

+    /* Hack for in_hot_standby: remember the value we just sent */
+    last_reported_in_hot_standby = in_hot_standby;
+
     report_needed = false;
 }

@@ -6287,6 +6304,23 @@ ReportChangedGUCOptions(void)
     if (!reporting_enabled)
         return;

+    /*
+     * Since in_hot_standby isn't actually changed by normal GUC actions, we
+     * need a hack to check whether a new value needs to be reported to the
+     * client.  For speed, we rely on the assumption that it can never
+     * transition from false to true.
+     */
+    if (last_reported_in_hot_standby && !RecoveryInProgress())
+    {
+        struct config_generic *record;
+
+        record = find_option("in_hot_standby", false, ERROR);
+        Assert(record != NULL);
+        record->status |= GUC_NEEDS_REPORT;
+        report_needed = true;
+        last_reported_in_hot_standby = false;
+    }
+
     /* Quick exit if no values have been changed */
     if (!report_needed)
         return;
@@ -11738,6 +11772,18 @@ show_data_directory_mode(void)
     return buf;
 }

+static const char *
+show_in_hot_standby(void)
+{
+    /*
+     * Unlike most show hooks, this has a side-effect of updating the dummy
+     * GUC variable to contain the value last shown.  See confederate code in
+     * BeginReportingGUCOptions and ReportChangedGUCOptions.
+     */
+    in_hot_standby = RecoveryInProgress();
+    return in_hot_standby ? "on" : "off";
+}
+
 /*
  * We split the input string, where commas separate function names
  * and certain whitespace chars are ignored, into a \0-separated (and