Tom, Geoff, Thanks for your feedback! Here is version 2 of this patch.
> You could just change it to > if (str[strspn(str, " \t\n\r\f")] == '\0') > to mitigate calling strlen. It's safe to do so because strspn will > only return values from 0 to strlen(str). > [...] I have serious doubts that the "optimized" implementation > you propose is actually faster than a naive one; it may be slower, and > it's certainly longer and harder to understand/test. I would like to point out that I never said it's optimized. However I like the code Geoff proposed. It definitely doesn't make anything worse. I decided to keep pg_str_contansonly procedure (i.e. not to inline this code) for now. Code with strspn looks OK in a simple example. However in a concrete context it looks like a bad Perl code in ROT13 to me: ``` /* try to figure out what's exactly going on */ if(somelongname[strspn(somelongname /* yes, again */, "longlistofchars")] != '\0') ``` > Function name seems weirdly spelled. I named it the same way pg_str_endswith is named. However I'm open for better suggestions here. > Whether it's worth worrying about, I dunno. This is hardly the only > C idiom you need to be familiar with to read the PG code. Well, at least this v2 version of the patch removes second string scanning. And I still believe that this inlined strspn is not readable or obvious at all. -- Best regards, Aleksander Alekseev
diff --git a/src/backend/parser/parse_type.c b/src/backend/parser/parse_type.c
index a8bb472..a1c5853 100644
--- a/src/backend/parser/parse_type.c
+++ b/src/backend/parser/parse_type.c
@@ -17,6 +17,7 @@
#include "access/htup_details.h"
#include "catalog/namespace.h"
#include "catalog/pg_type.h"
+#include "common/string.h"
#include "lib/stringinfo.h"
#include "nodes/makefuncs.h"
#include "parser/parser.h"
@@ -696,7 +697,7 @@ typeStringToTypeName(const char *str)
ErrorContextCallback ptserrcontext;
/* make sure we give useful error for empty input */
- if (strspn(str, " \t\n\r\f") == strlen(str))
+ if (pg_str_containsonly(str, " \t\n\r\f"))
goto fail;
initStringInfo(&buf);
diff --git a/src/backend/tsearch/ts_utils.c b/src/backend/tsearch/ts_utils.c
index 9c3e15c..81d6132 100644
--- a/src/backend/tsearch/ts_utils.c
+++ b/src/backend/tsearch/ts_utils.c
@@ -17,6 +17,7 @@
#include <ctype.h>
#include "miscadmin.h"
+#include "common/string.h"
#include "tsearch/ts_locale.h"
#include "tsearch/ts_utils.h"
@@ -45,7 +46,7 @@ get_tsearch_config_filename(const char *basename,
* and case-insensitive filesystems, and non-ASCII characters create other
* interesting risks, so on the whole a tight policy seems best.
*/
- if (strspn(basename, "abcdefghijklmnopqrstuvwxyz0123456789_") != strlen(basename))
+ if (!pg_str_containsonly(basename, "abcdefghijklmnopqrstuvwxyz0123456789_"))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid text search configuration file name \"%s\"",
diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c
index 394042c..7e41359 100644
--- a/src/backend/utils/adt/regproc.c
+++ b/src/backend/utils/adt/regproc.c
@@ -32,6 +32,7 @@
#include "catalog/pg_ts_config.h"
#include "catalog/pg_ts_dict.h"
#include "catalog/pg_type.h"
+#include "common/string.h"
#include "lib/stringinfo.h"
#include "miscadmin.h"
#include "parser/parse_type.h"
@@ -75,7 +76,7 @@ regprocin(PG_FUNCTION_ARGS)
/* Numeric OID? */
if (pro_name_or_oid[0] >= '0' &&
pro_name_or_oid[0] <= '9' &&
- strspn(pro_name_or_oid, "0123456789") == strlen(pro_name_or_oid))
+ pg_str_containsonly(pro_name_or_oid, "0123456789"))
{
result = DatumGetObjectId(DirectFunctionCall1(oidin,
CStringGetDatum(pro_name_or_oid)));
@@ -286,7 +287,7 @@ regprocedurein(PG_FUNCTION_ARGS)
/* Numeric OID? */
if (pro_name_or_oid[0] >= '0' &&
pro_name_or_oid[0] <= '9' &&
- strspn(pro_name_or_oid, "0123456789") == strlen(pro_name_or_oid))
+ pg_str_containsonly(pro_name_or_oid, "0123456789"))
{
result = DatumGetObjectId(DirectFunctionCall1(oidin,
CStringGetDatum(pro_name_or_oid)));
@@ -535,7 +536,7 @@ regoperin(PG_FUNCTION_ARGS)
/* Numeric OID? */
if (opr_name_or_oid[0] >= '0' &&
opr_name_or_oid[0] <= '9' &&
- strspn(opr_name_or_oid, "0123456789") == strlen(opr_name_or_oid))
+ pg_str_containsonly(opr_name_or_oid, "0123456789"))
{
result = DatumGetObjectId(DirectFunctionCall1(oidin,
CStringGetDatum(opr_name_or_oid)));
@@ -750,7 +751,7 @@ regoperatorin(PG_FUNCTION_ARGS)
/* Numeric OID? */
if (opr_name_or_oid[0] >= '0' &&
opr_name_or_oid[0] <= '9' &&
- strspn(opr_name_or_oid, "0123456789") == strlen(opr_name_or_oid))
+ pg_str_containsonly(opr_name_or_oid, "0123456789"))
{
result = DatumGetObjectId(DirectFunctionCall1(oidin,
CStringGetDatum(opr_name_or_oid)));
@@ -995,7 +996,7 @@ regclassin(PG_FUNCTION_ARGS)
/* Numeric OID? */
if (class_name_or_oid[0] >= '0' &&
class_name_or_oid[0] <= '9' &&
- strspn(class_name_or_oid, "0123456789") == strlen(class_name_or_oid))
+ pg_str_containsonly(class_name_or_oid, "0123456789"))
{
result = DatumGetObjectId(DirectFunctionCall1(oidin,
CStringGetDatum(class_name_or_oid)));
@@ -1186,7 +1187,7 @@ regtypein(PG_FUNCTION_ARGS)
/* Numeric OID? */
if (typ_name_or_oid[0] >= '0' &&
typ_name_or_oid[0] <= '9' &&
- strspn(typ_name_or_oid, "0123456789") == strlen(typ_name_or_oid))
+ pg_str_containsonly(typ_name_or_oid, "0123456789"))
{
result = DatumGetObjectId(DirectFunctionCall1(oidin,
CStringGetDatum(typ_name_or_oid)));
@@ -1358,7 +1359,7 @@ regconfigin(PG_FUNCTION_ARGS)
/* Numeric OID? */
if (cfg_name_or_oid[0] >= '0' &&
cfg_name_or_oid[0] <= '9' &&
- strspn(cfg_name_or_oid, "0123456789") == strlen(cfg_name_or_oid))
+ pg_str_containsonly(cfg_name_or_oid, "0123456789"))
{
result = DatumGetObjectId(DirectFunctionCall1(oidin,
CStringGetDatum(cfg_name_or_oid)));
@@ -1468,7 +1469,7 @@ regdictionaryin(PG_FUNCTION_ARGS)
/* Numeric OID? */
if (dict_name_or_oid[0] >= '0' &&
dict_name_or_oid[0] <= '9' &&
- strspn(dict_name_or_oid, "0123456789") == strlen(dict_name_or_oid))
+ pg_str_containsonly(dict_name_or_oid, "0123456789"))
{
result = DatumGetObjectId(DirectFunctionCall1(oidin,
CStringGetDatum(dict_name_or_oid)));
@@ -1578,7 +1579,7 @@ regrolein(PG_FUNCTION_ARGS)
/* Numeric OID? */
if (role_name_or_oid[0] >= '0' &&
role_name_or_oid[0] <= '9' &&
- strspn(role_name_or_oid, "0123456789") == strlen(role_name_or_oid))
+ pg_str_containsonly(role_name_or_oid, "0123456789"))
{
result = DatumGetObjectId(DirectFunctionCall1(oidin,
CStringGetDatum(role_name_or_oid)));
@@ -1699,7 +1700,7 @@ regnamespacein(PG_FUNCTION_ARGS)
/* Numeric OID? */
if (nsp_name_or_oid[0] >= '0' &&
nsp_name_or_oid[0] <= '9' &&
- strspn(nsp_name_or_oid, "0123456789") == strlen(nsp_name_or_oid))
+ pg_str_containsonly(nsp_name_or_oid, "0123456789"))
{
result = DatumGetObjectId(DirectFunctionCall1(oidin,
CStringGetDatum(nsp_name_or_oid)));
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 2a68359..5330154 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -62,6 +62,7 @@
#include "catalog/storage.h"
#include "commands/policy.h"
#include "commands/trigger.h"
+#include "common/string.h"
#include "miscadmin.h"
#include "nodes/nodeFuncs.h"
#include "optimizer/clauses.h"
@@ -5874,7 +5875,7 @@ RelationCacheInitFileRemove(void)
while ((de = ReadDir(dir, tblspcdir)) != NULL)
{
- if (strspn(de->d_name, "0123456789") == strlen(de->d_name))
+ if (pg_str_containsonly(de->d_name, "0123456789"))
{
/* Scan the tablespace dir for per-database dirs */
snprintf(path, sizeof(path), "%s/%s/%s",
@@ -5905,7 +5906,7 @@ RelationCacheInitFileRemoveInDir(const char *tblspcpath)
while ((de = ReadDir(dir, tblspcpath)) != NULL)
{
- if (strspn(de->d_name, "0123456789") == strlen(de->d_name))
+ if (pg_str_containsonly(de->d_name, "0123456789"))
{
/* Try to remove the init file in each database */
snprintf(initfilename, sizeof(initfilename), "%s/%s/%s",
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 6cf3829..db29c38 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -52,6 +52,7 @@
#include "access/xact.h"
#include "access/xlog.h"
#include "catalog/catalog.h"
+#include "common/string.h"
#include "lib/pairingheap.h"
#include "miscadmin.h"
#include "storage/predicate.h"
@@ -1423,7 +1424,7 @@ ImportSnapshot(const char *idstr)
* Verify the identifier: only 0-9, A-F and hyphens are allowed. We do
* this mainly to prevent reading arbitrary files.
*/
- if (strspn(idstr, "0123456789ABCDEF-") != strlen(idstr))
+ if (!pg_str_containsonly(idstr, "0123456789ABCDEF-"))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid snapshot identifier: \"%s\"", idstr)));
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index b89bd99..acfb760 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -27,6 +27,7 @@
#include "pg_backup_utils.h"
#include "dumputils.h"
#include "fe_utils/string_utils.h"
+#include "common/string.h"
#include <ctype.h>
#include <fcntl.h>
@@ -1364,7 +1365,7 @@ SortTocFromFile(Archive *AHX)
cmnt[0] = '\0';
/* Ignore if all blank */
- if (strspn(buf, " \t\r\n") == strlen(buf))
+ if (pg_str_containsonly(buf, " \t\r\n"))
continue;
/* Get an ID, check it's valid and not already seen */
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 7949aad..d2f9555 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -54,6 +54,7 @@
#include "catalog/pg_proc.h"
#include "catalog/pg_trigger.h"
#include "catalog/pg_type.h"
+#include "common/string.h"
#include "libpq/libpq-fs.h"
#include "dumputils.h"
@@ -1908,7 +1909,7 @@ dumpTableData_insert(Archive *fout, void *dcontext)
*/
const char *s = PQgetvalue(res, tuple, field);
- if (strspn(s, "0123456789 +-eE.") == strlen(s))
+ if (pg_str_containsonly(s, "0123456789 +-eE."))
archputs(s, fout);
else
archprintf(fout, "'%s'", s);
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index a9a2fdb..eed8865 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -38,6 +38,7 @@
#include "fe_utils/string_utils.h"
#include "common.h"
+#include "common/string.h"
#include "copy.h"
#include "crosstabview.h"
#include "describe.h"
@@ -591,7 +592,7 @@ exec_command(const char *cmd,
{
/* only one arg; maybe it is lineno not fname */
if (fname[0] &&
- strspn(fname, "0123456789") == strlen(fname))
+ pg_str_containsonly(fname, "0123456789"))
{
/* all digits, so assume it is lineno */
ln = fname;
diff --git a/src/bin/psql/crosstabview.c b/src/bin/psql/crosstabview.c
index b283c24..f8f1942 100644
--- a/src/bin/psql/crosstabview.c
+++ b/src/bin/psql/crosstabview.c
@@ -10,6 +10,7 @@
#include <string.h>
#include "common.h"
+#include "common/string.h"
#include "crosstabview.h"
#include "pqexpbuffer.h"
#include "psqlscanslash.h"
@@ -599,8 +600,8 @@ rankSort(int num_columns, pivot_field *piv_columns)
/* ranking information is valid if non null and matches /^-?\d+$/ */
if (val &&
((*val == '-' &&
- strspn(val + 1, "0123456789") == strlen(val + 1)) ||
- strspn(val, "0123456789") == strlen(val)))
+ pg_str_containsonly(val + 1, "0123456789")) ||
+ pg_str_containsonly(val, "0123456789")))
{
hmap[i * 2] = atoi(val);
hmap[i * 2 + 1] = i;
@@ -637,7 +638,7 @@ indexOfColumn(char *arg, const PGresult *res)
{
int idx;
- if (arg[0] && strspn(arg, "0123456789") == strlen(arg))
+ if (arg[0] && pg_str_containsonly(arg, "0123456789"))
{
/* if arg contains only digits, it's a column number */
idx = atoi(arg) - 1;
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index f0d955b..47856e4 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -18,6 +18,7 @@
#include "fe_utils/string_utils.h"
#include "common.h"
+#include "common/string.h"
#include "describe.h"
#include "fe_utils/mbprint.h"
#include "fe_utils/print.h"
@@ -309,7 +310,7 @@ describeFunctions(const char *functypes, const char *pattern, bool verbose, bool
/* No "Parallel" column before 9.6 */
static const bool translate_columns_pre_96[] = {false, false, false, false, true, true, false, true, false, false, false, false};
- if (strlen(functypes) != strspn(functypes, "antwS+"))
+ if (!pg_str_containsonly(functypes, "antwS+"))
{
psql_error("\\df only takes [antwS+] as options\n");
return true;
diff --git a/src/common/string.c b/src/common/string.c
index 69b26e7..231d5eb 100644
--- a/src/common/string.c
+++ b/src/common/string.c
@@ -41,3 +41,13 @@ pg_str_endswith(const char *str, const char *end)
str += slen - elen;
return strcmp(str, end) == 0;
}
+
+/*
+ * Returns whether the string `str' contains only letters from `allowed'.
+ * If `str' is empty procedure always returns `true'.
+ */
+bool
+pg_str_containsonly(const char *str, const char *allowed)
+{
+ return str[strspn(str, allowed)] == '\0';
+}
diff --git a/src/include/common/string.h b/src/include/common/string.h
index bb54d28..77f1ae0 100644
--- a/src/include/common/string.h
+++ b/src/include/common/string.h
@@ -11,5 +11,6 @@
#define COMMON_STRING_H
extern bool pg_str_endswith(const char *str, const char *end);
+extern bool pg_str_containsonly(const char *str, const char *allowed);
#endif /* COMMON_STRING_H */
signature.asc
Description: PGP signature
