Re: Add absolute value to dict_int - Mailing list pgsql-hackers

I wrote:
> There are at least three things we could do here:
> 1. Insist that defGetBoolean and its siblings should accept the
> string equivalent of any non-string value they accept.  This
> would change the behavior of a whole lot of utility commands,
> not only the text search commands, and I'm not exactly convinced
> it's a good idea.  Seems like it's losing error detection
> capability.
> 2. Improve the catalog representation of text search parameters
> so that the original Value node can be faithfully reconstructed.
> I'd be for this, except it seems like a lot of work for a rather
> minor benefit.
> 3. Rearrange text search parameter validation so we smash parameters
> to strings *before* we validate them, ensuring we won't take any
> settings that will be rejected later.

I still don't much like #1, but after looking closer, #2 is not as
impractical as I thought.  The catalog representation doesn't need
any redefinition really, because per the existing comments,

 * For the convenience of pg_dump, the output is formatted exactly as it
 * would need to appear in CREATE TEXT SEARCH DICTIONARY to reproduce the
 * same options.

So all we really need to do is upgrade [de]serialize_deflist to be smarter
about int and float nodes.  This is still a bit invasive because somebody
decided to make deserialize_deflist serve two masters, and I don't feel
like working through whether the prsheadline code would cope nicely with
non-string output nodes.  So the first patch attached adds a flag argument
to deserialize_deflist to tell it whether to force all the values to
strings.

Alternatively, we could do #3, as in the second patch below.  This
seems clearly Less Good, but it's a smaller/safer patch, and it's
at least potentially back-patchable, whereas changing the signature
of deserialize_deflist in stable branches would risk trouble.

(I didn't bother with regression test additions yet, but some would
be appropriate for either patch.)

Given the lack of field complaints, I'm not that excited about
back-patching anything for this.  So my inclination is to go with #2
(first patch) and fix it in HEAD only.

Thoughts?

            regards, tom lane

diff --git a/src/backend/commands/tsearchcmds.c b/src/backend/commands/tsearchcmds.c
index 9dca682..0731ca5 100644
--- a/src/backend/commands/tsearchcmds.c
+++ b/src/backend/commands/tsearchcmds.c
@@ -36,6 +36,7 @@
 #include "commands/alter.h"
 #include "commands/defrem.h"
 #include "commands/event_trigger.h"
+#include "common/string.h"
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
 #include "parser/parse_func.h"
@@ -52,6 +53,8 @@ static void MakeConfigurationMapping(AlterTSConfigurationStmt *stmt,
                                      HeapTuple tup, Relation relMap);
 static void DropConfigurationMapping(AlterTSConfigurationStmt *stmt,
                                      HeapTuple tup, Relation relMap);
+static DefElem *buildDefItem(const char *name, const char *val,
+                             bool force_strings);


 /* --------------------- TS Parser commands ------------------------ */
@@ -566,7 +569,7 @@ AlterTSDictionary(AlterTSDictionaryStmt *stmt)
     if (isnull)
         dictoptions = NIL;
     else
-        dictoptions = deserialize_deflist(opt);
+        dictoptions = deserialize_deflist(opt, false);

     /*
      * Modify the options list as per specified changes
@@ -1519,9 +1522,6 @@ DropConfigurationMapping(AlterTSConfigurationStmt *stmt,
  * For the convenience of pg_dump, the output is formatted exactly as it
  * would need to appear in CREATE TEXT SEARCH DICTIONARY to reproduce the
  * same options.
- *
- * Note that we assume that only the textual representation of an option's
- * value is interesting --- hence, non-string DefElems get forced to strings.
  */
 text *
 serialize_deflist(List *deflist)
@@ -1539,19 +1539,30 @@ serialize_deflist(List *deflist)

         appendStringInfo(&buf, "%s = ",
                          quote_identifier(defel->defname));
-        /* If backslashes appear, force E syntax to determine their handling */
-        if (strchr(val, '\\'))
-            appendStringInfoChar(&buf, ESCAPE_STRING_SYNTAX);
-        appendStringInfoChar(&buf, '\'');
-        while (*val)
+
+        /*
+         * If the value is a T_Integer or T_Float, emit it without quotes,
+         * otherwise with quotes.  This is essential to allow correct
+         * reconstruction of the node type as well as the value.
+         */
+        if (IsA(defel->arg, Integer) || IsA(defel->arg, Float))
+            appendStringInfoString(&buf, val);
+        else
         {
-            char        ch = *val++;
+            /* If backslashes appear, force E syntax to quote them safely */
+            if (strchr(val, '\\'))
+                appendStringInfoChar(&buf, ESCAPE_STRING_SYNTAX);
+            appendStringInfoChar(&buf, '\'');
+            while (*val)
+            {
+                char        ch = *val++;

-            if (SQL_STR_DOUBLE(ch, true))
+                if (SQL_STR_DOUBLE(ch, true))
+                    appendStringInfoChar(&buf, ch);
                 appendStringInfoChar(&buf, ch);
-            appendStringInfoChar(&buf, ch);
+            }
+            appendStringInfoChar(&buf, '\'');
         }
-        appendStringInfoChar(&buf, '\'');
         if (lnext(deflist, l) != NULL)
             appendStringInfoString(&buf, ", ");
     }
@@ -1566,10 +1577,12 @@ serialize_deflist(List *deflist)
  *
  * This is also used for prsheadline options, so for backward compatibility
  * we need to accept a few things serialize_deflist() will never emit:
- * in particular, unquoted and double-quoted values.
+ * in particular, unquoted and double-quoted strings.  Also, for prsheadline
+ * we want to force all the output nodes to be plain strings, whereas for
+ * true deserialization we want to reconstruct the node type.
  */
 List *
-deserialize_deflist(Datum txt)
+deserialize_deflist(Datum txt, bool force_strings)
 {
     text       *in = DatumGetTextPP(txt);    /* in case it's toasted */
     List       *result = NIL;
@@ -1694,8 +1707,9 @@ deserialize_deflist(Datum txt)
                     {
                         *wsptr++ = '\0';
                         result = lappend(result,
-                                         makeDefElem(pstrdup(workspace),
-                                                     (Node *) makeString(pstrdup(startvalue)), -1));
+                                         buildDefItem(workspace,
+                                                      startvalue,
+                                                      true));
                         state = CS_WAITKEY;
                     }
                 }
@@ -1726,8 +1740,9 @@ deserialize_deflist(Datum txt)
                     {
                         *wsptr++ = '\0';
                         result = lappend(result,
-                                         makeDefElem(pstrdup(workspace),
-                                                     (Node *) makeString(pstrdup(startvalue)), -1));
+                                         buildDefItem(workspace,
+                                                      startvalue,
+                                                      true));
                         state = CS_WAITKEY;
                     }
                 }
@@ -1741,8 +1756,9 @@ deserialize_deflist(Datum txt)
                 {
                     *wsptr++ = '\0';
                     result = lappend(result,
-                                     makeDefElem(pstrdup(workspace),
-                                                 (Node *) makeString(pstrdup(startvalue)), -1));
+                                     buildDefItem(workspace,
+                                                  startvalue,
+                                                  force_strings));
                     state = CS_WAITKEY;
                 }
                 else
@@ -1760,8 +1776,9 @@ deserialize_deflist(Datum txt)
     {
         *wsptr++ = '\0';
         result = lappend(result,
-                         makeDefElem(pstrdup(workspace),
-                                     (Node *) makeString(pstrdup(startvalue)), -1));
+                         buildDefItem(workspace,
+                                      startvalue,
+                                      force_strings));
     }
     else if (state != CS_WAITKEY)
         ereport(ERROR,
@@ -1773,3 +1790,35 @@ deserialize_deflist(Datum txt)

     return result;
 }
+
+/*
+ * Build one DefElem for deserialize_deflist
+ */
+static DefElem *
+buildDefItem(const char *name, const char *val, bool force_strings)
+{
+    if (!force_strings && val[0] != '\0')
+    {
+        int            v;
+        char       *endptr;
+
+        /* Try to parse as an integer */
+        errno = 0;
+        v = strtoint(val, &endptr, 10);
+        if (errno == 0 && *endptr == '\0')
+            return makeDefElem(pstrdup(name),
+                               (Node *) makeInteger(v),
+                               -1);
+        /* Nope, how about as a float? */
+        errno = 0;
+        (void) strtod(val, &endptr);
+        if (errno == 0 && *endptr == '\0')
+            return makeDefElem(pstrdup(name),
+                               (Node *) makeFloat(pstrdup(val)),
+                               -1);
+    }
+    /* Just make it a string */
+    return makeDefElem(pstrdup(name),
+                       (Node *) makeString(pstrdup(val)),
+                       -1);
+}
diff --git a/src/backend/tsearch/wparser.c b/src/backend/tsearch/wparser.c
index 88005c0..85851da 100644
--- a/src/backend/tsearch/wparser.c
+++ b/src/backend/tsearch/wparser.c
@@ -329,7 +329,7 @@ ts_headline_byid_opt(PG_FUNCTION_ARGS)
                 VARDATA_ANY(in), VARSIZE_ANY_EXHDR(in));

     if (opt)
-        prsoptions = deserialize_deflist(PointerGetDatum(opt));
+        prsoptions = deserialize_deflist(PointerGetDatum(opt), true);
     else
         prsoptions = NIL;

@@ -400,7 +400,7 @@ ts_headline_jsonb_byid_opt(PG_FUNCTION_ARGS)
     state->prsobj = lookup_ts_parser_cache(state->cfg->prsId);
     state->query = query;
     if (opt)
-        state->prsoptions = deserialize_deflist(PointerGetDatum(opt));
+        state->prsoptions = deserialize_deflist(PointerGetDatum(opt), true);
     else
         state->prsoptions = NIL;

@@ -477,7 +477,7 @@ ts_headline_json_byid_opt(PG_FUNCTION_ARGS)
     state->prsobj = lookup_ts_parser_cache(state->cfg->prsId);
     state->query = query;
     if (opt)
-        state->prsoptions = deserialize_deflist(PointerGetDatum(opt));
+        state->prsoptions = deserialize_deflist(PointerGetDatum(opt), true);
     else
         state->prsoptions = NIL;

diff --git a/src/backend/utils/cache/ts_cache.c b/src/backend/utils/cache/ts_cache.c
index 1641271..8900ac2 100644
--- a/src/backend/utils/cache/ts_cache.c
+++ b/src/backend/utils/cache/ts_cache.c
@@ -334,7 +334,7 @@ lookup_ts_dictionary_cache(Oid dictId)
             if (isnull)
                 dictoptions = NIL;
             else
-                dictoptions = deserialize_deflist(opt);
+                dictoptions = deserialize_deflist(opt, false);

             entry->dictData =
                 DatumGetPointer(OidFunctionCall1(template->tmplinit,
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index 5cd6975..1bd5de7 100644
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -127,7 +127,7 @@ extern void RemoveTSConfigurationById(Oid cfgId);
 extern ObjectAddress AlterTSConfiguration(AlterTSConfigurationStmt *stmt);

 extern text *serialize_deflist(List *deflist);
-extern List *deserialize_deflist(Datum txt);
+extern List *deserialize_deflist(Datum txt, bool force_strings);

 /* commands/foreigncmds.c */
 extern ObjectAddress AlterForeignServerOwner(const char *name, Oid newOwnerId);
diff --git a/src/backend/commands/tsearchcmds.c b/src/backend/commands/tsearchcmds.c
index 9dca682..182837b 100644
--- a/src/backend/commands/tsearchcmds.c
+++ b/src/backend/commands/tsearchcmds.c
@@ -419,6 +419,7 @@ DefineTSDictionary(List *names, List *parameters)
     NameData    dname;
     Oid            templId = InvalidOid;
     List       *dictoptions = NIL;
+    Datum        dictoptions_s;
     Oid            dictOid;
     Oid            namespaceoid;
     AclResult    aclresult;
@@ -453,6 +454,20 @@ DefineTSDictionary(List *names, List *parameters)
     }

     /*
+     * Immediately serialize the parameters, and then deserialize so that what
+     * we pass to validation is the same representation that will be seen at
+     * runtime.  This is a kluge to cope with the fact that the serialization
+     * logic fails to reproduce the node structure exactly.
+     */
+    if (dictoptions)
+    {
+        dictoptions_s = PointerGetDatum(serialize_deflist(dictoptions));
+        dictoptions = deserialize_deflist(dictoptions_s);
+    }
+    else
+        dictoptions_s = (Datum) 0;
+
+    /*
      * Validation
      */
     if (!OidIsValid(templId))
@@ -480,8 +495,7 @@ DefineTSDictionary(List *names, List *parameters)
     values[Anum_pg_ts_dict_dictowner - 1] = ObjectIdGetDatum(GetUserId());
     values[Anum_pg_ts_dict_dicttemplate - 1] = ObjectIdGetDatum(templId);
     if (dictoptions)
-        values[Anum_pg_ts_dict_dictinitoption - 1] =
-            PointerGetDatum(serialize_deflist(dictoptions));
+        values[Anum_pg_ts_dict_dictinitoption - 1] = dictoptions_s;
     else
         nulls[Anum_pg_ts_dict_dictinitoption - 1] = true;

@@ -537,6 +551,7 @@ AlterTSDictionary(AlterTSDictionaryStmt *stmt)
     Oid            dictId;
     ListCell   *pl;
     List       *dictoptions;
+    Datum        dictoptions_s;
     Datum        opt;
     bool        isnull;
     Datum        repl_val[Natts_pg_ts_dict];
@@ -595,6 +610,20 @@ AlterTSDictionary(AlterTSDictionaryStmt *stmt)
     }

     /*
+     * Immediately serialize the parameters, and then deserialize so that what
+     * we pass to validation is the same representation that will be seen at
+     * runtime.  This is a kluge to cope with the fact that the serialization
+     * logic fails to reproduce the node structure exactly.
+     */
+    if (dictoptions)
+    {
+        dictoptions_s = PointerGetDatum(serialize_deflist(dictoptions));
+        dictoptions = deserialize_deflist(dictoptions_s);
+    }
+    else
+        dictoptions_s = (Datum) 0;
+
+    /*
      * Validate
      */
     verify_dictoptions(((Form_pg_ts_dict) GETSTRUCT(tup))->dicttemplate,
@@ -608,8 +637,7 @@ AlterTSDictionary(AlterTSDictionaryStmt *stmt)
     memset(repl_repl, false, sizeof(repl_repl));

     if (dictoptions)
-        repl_val[Anum_pg_ts_dict_dictinitoption - 1] =
-            PointerGetDatum(serialize_deflist(dictoptions));
+        repl_val[Anum_pg_ts_dict_dictinitoption - 1] = dictoptions_s;
     else
         repl_null[Anum_pg_ts_dict_dictinitoption - 1] = true;
     repl_repl[Anum_pg_ts_dict_dictinitoption - 1] = true;
@@ -1522,6 +1550,9 @@ DropConfigurationMapping(AlterTSConfigurationStmt *stmt,
  *
  * Note that we assume that only the textual representation of an option's
  * value is interesting --- hence, non-string DefElems get forced to strings.
+ * This is, in fact, wrong.  For example, defGetBoolean accepts integers 0
+ * and 1 but not the string equivalents of those.  Pending making this smarter,
+ * callers must avoid assuming that serialize/deserialize is an identity.
  */
 text *
 serialize_deflist(List *deflist)