On Thu, Jul 11, 2024 at 09:09:17PM -0500, Nathan Bossart wrote: > On second thought, maybe we should just limit this improvement to the minor > releases with the fix so that we _can_ get rid of the workaround. Or we > could use the hacky workaround only for versions with the bug.
Here is a new version of the patch set. The main differences are 1) we no longer gather the sequence data for schema-only dumps and 2) 0003 uses a simplified query for dumps on v18 and newer. I considered also using a slightly simplified query for dumps on versions with the unlogged-sequences-on-standbys fix, but I felt that wasn't worth the extra code. Unfortunately, I've also discovered a problem with 0003. pg_sequence_last_value() returns NULL when is_called is false, in which case we assume last_value == seqstart, which is, sadly, bogus due to commands like ALTER SEQUENCE [RE]START WITH. AFAICT there isn't an easy way around this. We could either create a giant query that gathers the information from all sequences in the database, or we could introduce a new function in v18 that returns everything we need (which would only help for upgrades _from_ v18). Assuming I'm not missing a better option, I think the latter is the better choice, and I still think it's worth doing even though it probably won't help anyone for ~2.5 years. -- nathan
>From cc17b018e5f96df48e820a2d624ade7ceccfef18 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@postgresql.org> Date: Mon, 15 Jul 2024 13:13:05 -0500 Subject: [PATCH v3 1/3] parse sequence information --- src/bin/pg_dump/pg_dump.c | 64 ++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 38 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index b8b1888bd3..bbcbe581aa 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -17198,18 +17198,16 @@ dumpSequence(Archive *fout, const TableInfo *tbinfo) { DumpOptions *dopt = fout->dopt; PGresult *res; - char *startv, - *incby, - *maxv, - *minv, - *cache, - *seqtype; + char seqtype[10]; bool cycled; bool is_ascending; int64 default_minv, - default_maxv; - char bufm[32], - bufx[32]; + default_maxv, + minv, + maxv, + startv, + incby, + cache; PQExpBuffer query = createPQExpBuffer(); PQExpBuffer delqry = createPQExpBuffer(); char *qseqname; @@ -17251,16 +17249,21 @@ dumpSequence(Archive *fout, const TableInfo *tbinfo) PQntuples(res)), tbinfo->dobj.name, PQntuples(res)); - seqtype = PQgetvalue(res, 0, 0); - startv = PQgetvalue(res, 0, 1); - incby = PQgetvalue(res, 0, 2); - maxv = PQgetvalue(res, 0, 3); - minv = PQgetvalue(res, 0, 4); - cache = PQgetvalue(res, 0, 5); + Assert(strlen(PQgetvalue(res, 0, 0)) < sizeof(seqtype)); + strncpy(seqtype, PQgetvalue(res, 0, 0), sizeof(seqtype)); + seqtype[sizeof(seqtype) - 1] = '\0'; + + startv = strtoi64(PQgetvalue(res, 0, 1), NULL, 10); + incby = strtoi64(PQgetvalue(res, 0, 2), NULL, 10); + maxv = strtoi64(PQgetvalue(res, 0, 3), NULL, 10); + minv = strtoi64(PQgetvalue(res, 0, 4), NULL, 10); + cache = strtoi64(PQgetvalue(res, 0, 5), NULL, 10); cycled = (strcmp(PQgetvalue(res, 0, 6), "t") == 0); + PQclear(res); + /* Calculate default limits for a sequence of this type */ - is_ascending = (incby[0] != '-'); + is_ascending = (incby >= 0); if (strcmp(seqtype, "smallint") == 0) { default_minv = is_ascending ? 1 : PG_INT16_MIN; @@ -17282,19 +17285,6 @@ dumpSequence(Archive *fout, const TableInfo *tbinfo) default_minv = default_maxv = 0; /* keep compiler quiet */ } - /* - * 64-bit strtol() isn't very portable, so convert the limits to strings - * and compare that way. - */ - snprintf(bufm, sizeof(bufm), INT64_FORMAT, default_minv); - snprintf(bufx, sizeof(bufx), INT64_FORMAT, default_maxv); - - /* Don't print minv/maxv if they match the respective default limit */ - if (strcmp(minv, bufm) == 0) - minv = NULL; - if (strcmp(maxv, bufx) == 0) - maxv = NULL; - /* * Identity sequences are not to be dropped separately. */ @@ -17346,22 +17336,22 @@ dumpSequence(Archive *fout, const TableInfo *tbinfo) appendPQExpBuffer(query, " AS %s\n", seqtype); } - appendPQExpBuffer(query, " START WITH %s\n", startv); + appendPQExpBuffer(query, " START WITH " INT64_FORMAT "\n", startv); - appendPQExpBuffer(query, " INCREMENT BY %s\n", incby); + appendPQExpBuffer(query, " INCREMENT BY " INT64_FORMAT "\n", incby); - if (minv) - appendPQExpBuffer(query, " MINVALUE %s\n", minv); + if (minv != default_minv) + appendPQExpBuffer(query, " MINVALUE " INT64_FORMAT "\n", minv); else appendPQExpBufferStr(query, " NO MINVALUE\n"); - if (maxv) - appendPQExpBuffer(query, " MAXVALUE %s\n", maxv); + if (maxv != default_maxv) + appendPQExpBuffer(query, " MAXVALUE " INT64_FORMAT "\n", maxv); else appendPQExpBufferStr(query, " NO MAXVALUE\n"); appendPQExpBuffer(query, - " CACHE %s%s", + " CACHE " INT64_FORMAT "%s", cache, (cycled ? "\n CYCLE" : "")); if (tbinfo->is_identity_sequence) @@ -17448,8 +17438,6 @@ dumpSequence(Archive *fout, const TableInfo *tbinfo) tbinfo->dobj.namespace->dobj.name, tbinfo->rolname, tbinfo->dobj.catId, 0, tbinfo->dobj.dumpId); - PQclear(res); - destroyPQExpBuffer(query); destroyPQExpBuffer(delqry); free(qseqname); -- 2.39.3 (Apple Git-146)
>From cca9b72850bb375443ce1979634a6a65410c3ee3 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@postgresql.org> Date: Tue, 9 Jul 2024 14:06:23 -0500 Subject: [PATCH v3 2/3] cache sequence information --- src/bin/pg_dump/pg_dump.c | 155 +++++++++++++++++++++++++------ src/tools/pgindent/typedefs.list | 1 + 2 files changed, 128 insertions(+), 28 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index bbcbe581aa..a54e32c7be 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -104,6 +104,18 @@ typedef struct RelFileNumber toast_index_relfilenumber; /* toast table index filenode */ } BinaryUpgradeClassOidItem; +typedef struct +{ + Oid oid; /* sequence OID */ + char seqtype[10]; /* data type of sequence */ + bool cycled; /* whether sequence cycles */ + int64 minv; /* minimum value */ + int64 maxv; /* maximum value */ + int64 startv; /* start value */ + int64 incby; /* increment value */ + int64 cache; /* cache size */ +} SequenceItem; + typedef enum OidOptions { zeroIsError = 1, @@ -173,6 +185,10 @@ static int nseclabels = 0; static BinaryUpgradeClassOidItem *binaryUpgradeClassOids = NULL; static int nbinaryUpgradeClassOids = 0; +/* sorted table of sequences */ +static SequenceItem *sequences = NULL; +static int nsequences = 0; + /* * The default number of rows per INSERT when * --inserts is specified without --rows-per-insert @@ -270,6 +286,7 @@ static void dumpTable(Archive *fout, const TableInfo *tbinfo); static void dumpTableSchema(Archive *fout, const TableInfo *tbinfo); static void dumpTableAttach(Archive *fout, const TableAttachInfo *attachinfo); static void dumpAttrDef(Archive *fout, const AttrDefInfo *adinfo); +static void collectSequences(Archive *fout); static void dumpSequence(Archive *fout, const TableInfo *tbinfo); static void dumpSequenceData(Archive *fout, const TableDataInfo *tdinfo); static void dumpIndex(Archive *fout, const IndxInfo *indxinfo); @@ -992,6 +1009,9 @@ main(int argc, char **argv) if (dopt.binary_upgrade) collectBinaryUpgradeClassOids(fout); + /* Collect sequence information. */ + collectSequences(fout); + /* Lastly, create dummy objects to represent the section boundaries */ boundaryObjs = createBoundaryObjects(); @@ -17189,6 +17209,71 @@ dumpTableConstraintComment(Archive *fout, const ConstraintInfo *coninfo) free(qtabname); } +/* + * bsearch() comparator for SequenceItem + */ +static int +SequenceItemCmp(const void *p1, const void *p2) +{ + SequenceItem v1 = *((const SequenceItem *) p1); + SequenceItem v2 = *((const SequenceItem *) p2); + + return pg_cmp_u32(v1.oid, v2.oid); +} + +/* + * collectSequences + * + * Construct a table of sequence information. This table is sorted by OID for + * speed in lookup. + */ +static void +collectSequences(Archive *fout) +{ + PGresult *res; + const char *query; + + /* + * Before Postgres 10, sequence metadata is in the sequence itself. We + * could likely make use of the sorted table with some extra effort, but + * for now it seems unlikely to be worth it. + */ + if (fout->remoteVersion < 100000) + return; + + query = "SELECT seqrelid, format_type(seqtypid, NULL), " + "seqstart, seqincrement, " + "seqmax, seqmin, " + "seqcache, seqcycle " + "FROM pg_catalog.pg_sequence " + "ORDER BY seqrelid"; + + res = ExecuteSqlQuery(fout, query, PGRES_TUPLES_OK); + + nsequences = PQntuples(res); + sequences = (SequenceItem *) pg_malloc(nsequences * sizeof(SequenceItem)); + + for (int i = 0; i < nsequences; i++) + { + size_t seqtype_sz = sizeof(((SequenceItem *) 0)->seqtype); + + sequences[i].oid = atooid(PQgetvalue(res, i, 0)); + + Assert(strlen(PQgetvalue(res, i, 1)) < seqtype_sz); + strncpy(sequences[i].seqtype, PQgetvalue(res, i, 1), seqtype_sz); + sequences[i].seqtype[seqtype_sz - 1] = '\0'; + + sequences[i].startv = strtoi64(PQgetvalue(res, i, 2), NULL, 10); + sequences[i].incby = strtoi64(PQgetvalue(res, i, 3), NULL, 10); + sequences[i].maxv = strtoi64(PQgetvalue(res, i, 4), NULL, 10); + sequences[i].minv = strtoi64(PQgetvalue(res, i, 5), NULL, 10); + sequences[i].cache = strtoi64(PQgetvalue(res, i, 6), NULL, 10); + sequences[i].cycled = (strcmp(PQgetvalue(res, i, 7), "t") == 0); + } + + PQclear(res); +} + /* * dumpSequence * write the declaration (not data) of one user-defined sequence @@ -17197,7 +17282,6 @@ static void dumpSequence(Archive *fout, const TableInfo *tbinfo) { DumpOptions *dopt = fout->dopt; - PGresult *res; char seqtype[10]; bool cycled; bool is_ascending; @@ -17215,19 +17299,34 @@ dumpSequence(Archive *fout, const TableInfo *tbinfo) qseqname = pg_strdup(fmtId(tbinfo->dobj.name)); + /* + * For versions >= 10, the sequence information is gathered in a sorted + * table before any calls to dumpSequence(). See collectSequences() for + * more information. + */ if (fout->remoteVersion >= 100000) { - appendPQExpBuffer(query, - "SELECT format_type(seqtypid, NULL), " - "seqstart, seqincrement, " - "seqmax, seqmin, " - "seqcache, seqcycle " - "FROM pg_catalog.pg_sequence " - "WHERE seqrelid = '%u'::oid", - tbinfo->dobj.catId.oid); + SequenceItem key = {0}; + SequenceItem *entry; + + Assert(sequences); + + key.oid = tbinfo->dobj.catId.oid; + entry = bsearch(&key, sequences, nsequences, + sizeof(SequenceItem), SequenceItemCmp); + + strncpy(seqtype, entry->seqtype, sizeof(seqtype)); + startv = entry->startv; + incby = entry->incby; + maxv = entry->maxv; + minv = entry->minv; + cache = entry->cache; + cycled = entry->cycled; } else { + PGresult *res; + /* * Before PostgreSQL 10, sequence metadata is in the sequence itself. * @@ -17239,28 +17338,28 @@ dumpSequence(Archive *fout, const TableInfo *tbinfo) "start_value, increment_by, max_value, min_value, " "cache_value, is_cycled FROM %s", fmtQualifiedDumpable(tbinfo)); - } - - res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); - if (PQntuples(res) != 1) - pg_fatal(ngettext("query to get data of sequence \"%s\" returned %d row (expected 1)", - "query to get data of sequence \"%s\" returned %d rows (expected 1)", - PQntuples(res)), - tbinfo->dobj.name, PQntuples(res)); - - Assert(strlen(PQgetvalue(res, 0, 0)) < sizeof(seqtype)); - strncpy(seqtype, PQgetvalue(res, 0, 0), sizeof(seqtype)); - seqtype[sizeof(seqtype) - 1] = '\0'; + res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); - startv = strtoi64(PQgetvalue(res, 0, 1), NULL, 10); - incby = strtoi64(PQgetvalue(res, 0, 2), NULL, 10); - maxv = strtoi64(PQgetvalue(res, 0, 3), NULL, 10); - minv = strtoi64(PQgetvalue(res, 0, 4), NULL, 10); - cache = strtoi64(PQgetvalue(res, 0, 5), NULL, 10); - cycled = (strcmp(PQgetvalue(res, 0, 6), "t") == 0); + if (PQntuples(res) != 1) + pg_fatal(ngettext("query to get data of sequence \"%s\" returned %d row (expected 1)", + "query to get data of sequence \"%s\" returned %d rows (expected 1)", + PQntuples(res)), + tbinfo->dobj.name, PQntuples(res)); + + Assert(strlen(PQgetvalue(res, 0, 0)) < sizeof(seqtype)); + strncpy(seqtype, PQgetvalue(res, 0, 0), sizeof(seqtype)); + seqtype[sizeof(seqtype) - 1] = '\0'; + + startv = strtoi64(PQgetvalue(res, 0, 1), NULL, 10); + incby = strtoi64(PQgetvalue(res, 0, 2), NULL, 10); + maxv = strtoi64(PQgetvalue(res, 0, 3), NULL, 10); + minv = strtoi64(PQgetvalue(res, 0, 4), NULL, 10); + cache = strtoi64(PQgetvalue(res, 0, 5), NULL, 10); + cycled = (strcmp(PQgetvalue(res, 0, 6), "t") == 0); - PQclear(res); + PQclear(res); + } /* Calculate default limits for a sequence of this type */ is_ascending = (incby >= 0); diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index b4d7f9217c..a09adcfb90 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2572,6 +2572,7 @@ SeqScan SeqScanState SeqTable SeqTableData +SequenceItem SerCommitSeqNo SerialControl SerialIOData -- 2.39.3 (Apple Git-146)
>From 457ab1ac24d5ee438f89b71e2829c593c032bb91 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@postgresql.org> Date: Tue, 30 Apr 2024 19:33:24 -0500 Subject: [PATCH v3 3/3] cache more sequence data --- src/bin/pg_dump/pg_dump.c | 126 ++++++++++++++++++++++++++++++-------- 1 file changed, 102 insertions(+), 24 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index a54e32c7be..fd738f7087 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -114,6 +114,8 @@ typedef struct int64 startv; /* start value */ int64 incby; /* increment value */ int64 cache; /* cache size */ + int64 last_value; /* last value of sequence */ + bool is_called; /* whether nextval advances before returning */ } SequenceItem; typedef enum OidOptions @@ -17231,7 +17233,7 @@ static void collectSequences(Archive *fout) { PGresult *res; - const char *query; + PQExpBuffer query; /* * Before Postgres 10, sequence metadata is in the sequence itself. We @@ -17241,14 +17243,63 @@ collectSequences(Archive *fout) if (fout->remoteVersion < 100000) return; - query = "SELECT seqrelid, format_type(seqtypid, NULL), " - "seqstart, seqincrement, " - "seqmax, seqmin, " - "seqcache, seqcycle " - "FROM pg_catalog.pg_sequence " - "ORDER BY seqrelid"; + query = createPQExpBuffer(); - res = ExecuteSqlQuery(fout, query, PGRES_TUPLES_OK); + appendPQExpBuffer(query, "SELECT seqrelid, format_type(seqtypid, NULL), " + "seqstart, seqincrement, " + "seqmax, seqmin, " + "seqcache, seqcycle, "); + + if (fout->dopt->schemaOnly && !fout->dopt->sequence_data) + { + /* + * If we aren't dumping the sequence data, just return garbage for + * is_called and last_value. + */ + appendPQExpBuffer(query, "'f', NULL FROM pg_catalog.pg_sequence "); + } + else if (fout->remoteVersion >= 180000) + { + /* + * Since version 18, pg_sequence_last_value() returns NULL if the + * caller doesn't have privileges on the sequence (instead of + * ERROR-ing), so we don't need to make any extra calls to + * has_sequence_privilege() in the query. + */ + appendPQExpBuffer(query, + "pg_sequence_last_value(seqrelid) IS NOT NULL, " + "CASE WHEN pg_sequence_last_value(seqrelid) IS NOT NULL " + "THEN pg_sequence_last_value(seqrelid) " + "ELSE seqstart END " + "FROM pg_catalog.pg_sequence "); + } + else + { + /* + * Before v18, pg_sequence_last_value() ERRORs for sequences for which + * the caller lacks privileges, so we must check privileges before + * calling it. Furthermore, some older versions have a buggy + * pg_sequence_last_value() that ERRORs for unlogged sequences on + * standbys (see commit 3cb2f13ac5), so we need some additional checks + * to avoid that. + */ + appendPQExpBuffer(query, + "CASE WHEN has_sequence_privilege(seqrelid, 'SELECT,USAGE'::text) " + "AND (pg_is_in_recovery() = 'f' OR c.relpersistence = 'p') " + "THEN pg_sequence_last_value(seqrelid) IS NOT NULL " + "ELSE 'f' END, " + "CASE WHEN has_sequence_privilege(seqrelid, 'SELECT,USAGE'::text) " + "AND (pg_is_in_recovery() = 'f' OR c.relpersistence = 'p') " + "AND pg_sequence_last_value(seqrelid) IS NOT NULL " + "THEN pg_sequence_last_value(seqrelid) " + "ELSE seqstart END " + "FROM pg_catalog.pg_sequence s " + "JOIN pg_class c ON s.seqrelid = c.oid "); + } + + appendPQExpBuffer(query, "ORDER BY seqrelid"); + + res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); nsequences = PQntuples(res); sequences = (SequenceItem *) pg_malloc(nsequences * sizeof(SequenceItem)); @@ -17269,9 +17320,12 @@ collectSequences(Archive *fout) sequences[i].minv = strtoi64(PQgetvalue(res, i, 5), NULL, 10); sequences[i].cache = strtoi64(PQgetvalue(res, i, 6), NULL, 10); sequences[i].cycled = (strcmp(PQgetvalue(res, i, 7), "t") == 0); + sequences[i].is_called = (strcmp(PQgetvalue(res, i, 8), "t") == 0); + sequences[i].last_value = strtoi64(PQgetvalue(res, i, 9), NULL, 10); } PQclear(res); + destroyPQExpBuffer(query); } /* @@ -17550,30 +17604,56 @@ static void dumpSequenceData(Archive *fout, const TableDataInfo *tdinfo) { TableInfo *tbinfo = tdinfo->tdtable; - PGresult *res; - char *last; + int64 last; bool called; PQExpBuffer query = createPQExpBuffer(); - appendPQExpBuffer(query, - "SELECT last_value, is_called FROM %s", - fmtQualifiedDumpable(tbinfo)); + /* + * For versions >= 10, the sequence information is gathered in a sorted + * table before any calls to dumpSequenceData(). See collectSequences() + * for more information. + */ + if (fout->remoteVersion < 100000) + { + PGresult *res; - res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); + appendPQExpBuffer(query, + "SELECT last_value, is_called FROM %s", + fmtQualifiedDumpable(tbinfo)); - if (PQntuples(res) != 1) - pg_fatal(ngettext("query to get data of sequence \"%s\" returned %d row (expected 1)", - "query to get data of sequence \"%s\" returned %d rows (expected 1)", - PQntuples(res)), - tbinfo->dobj.name, PQntuples(res)); + res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); + + if (PQntuples(res) != 1) + pg_fatal(ngettext("query to get data of sequence \"%s\" returned %d row (expected 1)", + "query to get data of sequence \"%s\" returned %d rows (expected 1)", + PQntuples(res)), + tbinfo->dobj.name, PQntuples(res)); + + last = strtoi64(PQgetvalue(res, 0, 0), NULL, 10); + called = (strcmp(PQgetvalue(res, 0, 1), "t") == 0); + + PQclear(res); + } + else + { + SequenceItem key = {0}; + SequenceItem *entry; - last = PQgetvalue(res, 0, 0); - called = (strcmp(PQgetvalue(res, 0, 1), "t") == 0); + Assert(sequences); + Assert(tbinfo->dobj.catId.oid); + + key.oid = tbinfo->dobj.catId.oid; + entry = bsearch(&key, sequences, nsequences, + sizeof(SequenceItem), SequenceItemCmp); + + last = entry->last_value; + called = entry->is_called; + } resetPQExpBuffer(query); appendPQExpBufferStr(query, "SELECT pg_catalog.setval("); appendStringLiteralAH(query, fmtQualifiedDumpable(tbinfo), fout); - appendPQExpBuffer(query, ", %s, %s);\n", + appendPQExpBuffer(query, ", " INT64_FORMAT ", %s);\n", last, (called ? "true" : "false")); if (tdinfo->dobj.dump & DUMP_COMPONENT_DATA) @@ -17587,8 +17667,6 @@ dumpSequenceData(Archive *fout, const TableDataInfo *tdinfo) .deps = &(tbinfo->dobj.dumpId), .nDeps = 1)); - PQclear(res); - destroyPQExpBuffer(query); } -- 2.39.3 (Apple Git-146)