I've been casting about for low-hanging fruit for making the regression tests run faster. One thing I noticed is that this query in sysviews.sql is one of the slowest queries in the whole suite:
select count(distinct utc_offset) >= 24 as ok from pg_timezone_names; Reading pg_timezone_names takes upwards of 300 msec on my workstation, and north of 3 seconds on some of my slower buildfarm critters. I'd always figured that, since it's reading the whole of the timezone data directory tree, it's just naturally got to be slow. However, I chanced to try strace'ing a scan of pg_timezone_names, and what I saw was just horrid. We do something like this for *each* timezone data file: stat("/usr/share/zoneinfo/posix/America/Iqaluit", {st_mode=S_IFREG|0644, st_size=2000, ...}) = 0 open("/usr/share/zoneinfo", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 20 getdents(20, /* 68 entries */, 32768) = 1968 close(20) = 0 open("/usr/share/zoneinfo/posix", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 20 getdents(20, /* 62 entries */, 32768) = 1776 close(20) = 0 open("/usr/share/zoneinfo/posix/America", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 20 getdents(20, /* 146 entries */, 32768) = 4696 close(20) = 0 open("/usr/share/zoneinfo/posix/America/Iqaluit", O_RDONLY) = 20 read(20, "TZif2\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\10\0\0\0\10\0\0\0\0"..., 54968) = 2000 close(20) = 0 open("/usr/share/zoneinfo", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 20 getdents(20, /* 68 entries */, 32768) = 1968 close(20) = 0 open("/usr/share/zoneinfo/posixrules", O_RDONLY) = 20 read(20, "TZif2\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\4\0\0\0\4\0\0\0\0"..., 54968) = 3519 close(20) = 0 That is, having found a data file by dint of searching the directory tree, we *repeat the search* from the root of the timezone tree. And then we read the posixrules file, in addition to the target zoneinfo file itself. And just to add insult to injury, we search the directory path down to posixrules, though fortunately that's just one level down. There are a couple of things going on here. One is that pg_open_tzfile() performs the repeat directory searches we're seeing above, because it is tasked with accepting a case-insensitive spelling of a timezone name and returning the correctly-cased zone name as seen in the file system. That's necessary, and not too expensive, when performing something like a "SET timezone" command, because we don't insist that the user spell the zone name canonically in SET. But it's pretty silly in the context of a directory scan, because we already know the canonical spelling of the file path: we just read it from the filesystem. The second problem is that we don't cache the result of reading posixrules, even though we need it for practically every zone load. This seems like a performance bug in the upstream IANA library, though I'm not sure that they care about use-cases like this one. (They might also object that they don't want to cache data that could conceivably change on-disk; but we already cache timezone data at other levels, so there's no reason not to do it here.) The attached patch adjusts both of these things. On my workstation it reduces the runtime of the pg_timezone_names view by better than a factor of 3. If I were to commit this, I'd want to back-patch, because experience has shown that we don't want any cross-branch variation in the timezone code files; absorbing upstream fixes is painful enough without that. But it seems like a pretty safe and helpful thing to back-patch. Thoughts, objections? regards, tom lane
diff --git a/src/timezone/localtime.c b/src/timezone/localtime.c index 154124e..a2faa82 100644 *** a/src/timezone/localtime.c --- b/src/timezone/localtime.c *************** static const pg_time_t time_t_min = MINV *** 55,60 **** --- 55,67 ---- static const pg_time_t time_t_max = MAXVAL(pg_time_t, TYPE_BIT(pg_time_t)); /* + * We cache the result of trying to load the TZDEFRULES zone here. + * tzdefrules_loaded is 0 if not tried yet, +1 if good, -1 if failed. + */ + static struct state tzdefrules_s; + static int tzdefrules_loaded = 0; + + /* * The DST rules to use if TZ has no rules and we can't load TZDEFRULES. * We default to US rules as of 1999-08-17. * POSIX 1003.1 section 8.1.1 says that the default DST rules are *************** tzparse(const char *name, struct state * *** 942,948 **** charcnt = stdlen + 1; if (sizeof sp->chars < charcnt) return false; ! load_ok = tzload(TZDEFRULES, NULL, sp, false) == 0; } if (!load_ok) sp->leapcnt = 0; /* so, we're off a little */ --- 949,969 ---- charcnt = stdlen + 1; if (sizeof sp->chars < charcnt) return false; ! ! /* ! * This bit also differs from the IANA code, which doesn't make any ! * attempt to avoid repetitive loadings of the TZDEFRULES zone. ! */ ! if (tzdefrules_loaded == 0) ! { ! if (tzload(TZDEFRULES, NULL, &tzdefrules_s, false) == 0) ! tzdefrules_loaded = 1; ! else ! tzdefrules_loaded = -1; ! } ! load_ok = (tzdefrules_loaded > 0); ! if (load_ok) ! memcpy(sp, &tzdefrules_s, sizeof(struct state)); } if (!load_ok) sp->leapcnt = 0; /* so, we're off a little */ diff --git a/src/timezone/pgtz.c b/src/timezone/pgtz.c index 13ce399..31dbf89 100644 *** a/src/timezone/pgtz.c --- b/src/timezone/pgtz.c *************** pg_open_tzfile(const char *name, char *c *** 80,91 **** int fullnamelen; int orignamelen; /* * Loop to split the given name into directory levels; for each level, * search using scan_directory_ci(). */ - strlcpy(fullname, pg_TZDIR(), sizeof(fullname)); - orignamelen = fullnamelen = strlen(fullname); fname = name; for (;;) { --- 80,116 ---- int fullnamelen; int orignamelen; + /* Initialize fullname with base name of tzdata directory */ + strlcpy(fullname, pg_TZDIR(), sizeof(fullname)); + orignamelen = fullnamelen = strlen(fullname); + + if (fullnamelen + 1 + strlen(name) >= MAXPGPATH) + return -1; /* not gonna fit */ + + /* + * If the caller doesn't need the canonical spelling, first just try to + * open the name as-is. This can be expected to succeed if the given name + * is already case-correct, or if the filesystem is case-insensitive; and + * we don't need to distinguish those situations if we aren't tasked with + * reporting the canonical spelling. + */ + if (canonname == NULL) + { + int result; + + fullname[fullnamelen] = '/'; + /* test above ensured this will fit: */ + strcpy(fullname + fullnamelen + 1, name); + result = open(fullname, O_RDONLY | PG_BINARY, 0); + if (result >= 0) + return result; + /* If that didn't work, fall through to do it the hard way */ + } + /* * Loop to split the given name into directory levels; for each level, * search using scan_directory_ci(). */ fname = name; for (;;) { *************** pg_open_tzfile(const char *name, char *c *** 97,104 **** fnamelen = slashptr - fname; else fnamelen = strlen(fname); - if (fullnamelen + 1 + fnamelen >= MAXPGPATH) - return -1; /* not gonna fit */ if (!scan_directory_ci(fullname, fname, fnamelen, fullname + fullnamelen + 1, MAXPGPATH - fullnamelen - 1)) --- 122,127 ---- *************** pg_tzenumerate_next(pg_tzenum *dir) *** 458,467 **** /* * Load this timezone using tzload() not pg_tzset(), so we don't fill ! * the cache */ ! if (tzload(fullname + dir->baselen, dir->tz.TZname, &dir->tz.state, ! true) != 0) { /* Zone could not be loaded, ignore it */ continue; --- 481,491 ---- /* * Load this timezone using tzload() not pg_tzset(), so we don't fill ! * the cache. Also, don't ask for the canonical spelling: we already ! * know it, and pg_open_tzfile's way of finding it out is pretty ! * inefficient. */ ! if (tzload(fullname + dir->baselen, NULL, &dir->tz.state, true) != 0) { /* Zone could not be loaded, ignore it */ continue; *************** pg_tzenumerate_next(pg_tzenum *dir) *** 473,478 **** --- 497,506 ---- continue; } + /* OK, return the canonical zone name spelling. */ + strlcpy(dir->tz.TZname, fullname + dir->baselen, + sizeof(dir->tz.TZname)); + /* Timezone loaded OK. */ return &dir->tz; }
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers