public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v5 2/2] Add a --outdated option to reindexdb 4+ messages / 4 participants [nested] [flat]
* [PATCH v5 2/2] Add a --outdated option to reindexdb @ 2021-02-24 17:33 Julien Rouhaud <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Julien Rouhaud @ 2021-02-24 17:33 UTC (permalink / raw) This uses the new OUTDATED option for REINDEX. If user asks for multiple job, the list of tables to process will be sorted by the total size of underlying indexes that have outdated dependency. Author: Julien Rouhaud <[email protected]> Reviewed-by: Discussion: https://postgr.es/m/20201203093143.GA64934%40nol --- src/bin/scripts/reindexdb.c | 143 ++++++++++++++++++++++++----- src/bin/scripts/t/090_reindexdb.pl | 34 ++++++- 2 files changed, 151 insertions(+), 26 deletions(-) diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c index cf28176243..369d164e65 100644 --- a/src/bin/scripts/reindexdb.c +++ b/src/bin/scripts/reindexdb.c @@ -35,20 +35,23 @@ typedef enum ReindexType static SimpleStringList *get_parallel_object_list(PGconn *conn, ReindexType type, SimpleStringList *user_list, + bool outdated, bool echo); static void reindex_one_database(const ConnParams *cparams, ReindexType type, SimpleStringList *user_list, const char *progname, bool echo, bool verbose, bool concurrently, - int concurrentCons, const char *tablespace); + int concurrentCons, const char *tablespace, + bool outdated); static void reindex_all_databases(ConnParams *cparams, const char *progname, bool echo, bool quiet, bool verbose, bool concurrently, - int concurrentCons, const char *tablespace); + int concurrentCons, const char *tablespace, + bool outdated); static void run_reindex_command(PGconn *conn, ReindexType type, const char *name, bool echo, bool verbose, bool concurrently, bool async, - const char *tablespace); + const char *tablespace, bool outdated); static void help(const char *progname); @@ -74,6 +77,7 @@ main(int argc, char *argv[]) {"concurrently", no_argument, NULL, 1}, {"maintenance-db", required_argument, NULL, 2}, {"tablespace", required_argument, NULL, 3}, + {"outdated", no_argument, NULL, 4}, {NULL, 0, NULL, 0} }; @@ -95,6 +99,7 @@ main(int argc, char *argv[]) bool quiet = false; bool verbose = false; bool concurrently = false; + bool outdated = false; SimpleStringList indexes = {NULL, NULL}; SimpleStringList tables = {NULL, NULL}; SimpleStringList schemas = {NULL, NULL}; @@ -170,6 +175,9 @@ main(int argc, char *argv[]) case 3: tablespace = pg_strdup(optarg); break; + case 4: + outdated = true; + break; default: fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname); exit(1); @@ -234,7 +242,8 @@ main(int argc, char *argv[]) cparams.dbname = maintenance_db; reindex_all_databases(&cparams, progname, echo, quiet, verbose, - concurrently, concurrentCons, tablespace); + concurrently, concurrentCons, tablespace, + outdated); } else if (syscatalog) { @@ -253,12 +262,17 @@ main(int argc, char *argv[]) pg_log_error("cannot reindex specific index(es) and system catalogs at the same time"); exit(1); } - if (concurrentCons > 1) { pg_log_error("cannot use multiple jobs to reindex system catalogs"); exit(1); } + if (outdated) + { + pg_log_error("cannot filter indexes having outdated dependencies " + "and reindex system catalogs at the same time"); + exit(1); + } if (dbname == NULL) { @@ -274,7 +288,7 @@ main(int argc, char *argv[]) reindex_one_database(&cparams, REINDEX_SYSTEM, NULL, progname, echo, verbose, - concurrently, 1, tablespace); + concurrently, 1, tablespace, outdated); } else { @@ -304,17 +318,20 @@ main(int argc, char *argv[]) if (schemas.head != NULL) reindex_one_database(&cparams, REINDEX_SCHEMA, &schemas, progname, echo, verbose, - concurrently, concurrentCons, tablespace); + concurrently, concurrentCons, tablespace, + outdated); if (indexes.head != NULL) reindex_one_database(&cparams, REINDEX_INDEX, &indexes, progname, echo, verbose, - concurrently, 1, tablespace); + concurrently, 1, tablespace, + outdated); if (tables.head != NULL) reindex_one_database(&cparams, REINDEX_TABLE, &tables, progname, echo, verbose, - concurrently, concurrentCons, tablespace); + concurrently, concurrentCons, tablespace, + outdated); /* * reindex database only if neither index nor table nor schema is @@ -323,7 +340,8 @@ main(int argc, char *argv[]) if (indexes.head == NULL && tables.head == NULL && schemas.head == NULL) reindex_one_database(&cparams, REINDEX_DATABASE, NULL, progname, echo, verbose, - concurrently, concurrentCons, tablespace); + concurrently, concurrentCons, tablespace, + outdated); } exit(0); @@ -334,7 +352,7 @@ reindex_one_database(const ConnParams *cparams, ReindexType type, SimpleStringList *user_list, const char *progname, bool echo, bool verbose, bool concurrently, int concurrentCons, - const char *tablespace) + const char *tablespace, bool outdated) { PGconn *conn; SimpleStringListCell *cell; @@ -363,6 +381,14 @@ reindex_one_database(const ConnParams *cparams, ReindexType type, exit(1); } + if (outdated && PQserverVersion(conn) < 140000) + { + PQfinish(conn); + pg_log_error("cannot use the \"%s\" option on server versions older than PostgreSQL %s", + "outdated", "14"); + exit(1); + } + if (!parallel) { switch (process_type) @@ -399,14 +425,24 @@ reindex_one_database(const ConnParams *cparams, ReindexType type, */ if (concurrently) pg_log_warning("cannot reindex system catalogs concurrently, skipping all"); + else if (outdated) + { + /* + * The only supported kind of object that can be outdated + * is collation. No system catalog has any index that can + * depend on an outdated collation, so skip system + * catalogs. + */ + } else run_reindex_command(conn, REINDEX_SYSTEM, PQdb(conn), echo, verbose, concurrently, false, - tablespace); + tablespace, outdated); /* Build a list of relations from the database */ process_list = get_parallel_object_list(conn, process_type, - user_list, echo); + user_list, outdated, + echo); process_type = REINDEX_TABLE; /* Bail out if nothing to process */ @@ -419,7 +455,8 @@ reindex_one_database(const ConnParams *cparams, ReindexType type, /* Build a list of relations from all the schemas */ process_list = get_parallel_object_list(conn, process_type, - user_list, echo); + user_list, outdated, + echo); process_type = REINDEX_TABLE; /* Bail out if nothing to process */ @@ -484,7 +521,8 @@ reindex_one_database(const ConnParams *cparams, ReindexType type, ParallelSlotSetHandler(free_slot, TableCommandResultHandler, NULL); run_reindex_command(free_slot->connection, process_type, objname, - echo, verbose, concurrently, true, tablespace); + echo, verbose, concurrently, true, tablespace, + outdated); cell = cell->next; } while (cell != NULL); @@ -509,7 +547,7 @@ finish: static void run_reindex_command(PGconn *conn, ReindexType type, const char *name, bool echo, bool verbose, bool concurrently, bool async, - const char *tablespace) + const char *tablespace, bool outdated) { const char *paren = "("; const char *comma = ", "; @@ -536,6 +574,12 @@ run_reindex_command(PGconn *conn, ReindexType type, const char *name, sep = comma; } + if (outdated) + { + appendPQExpBuffer(&sql, "%sOUTDATED", sep); + sep = comma; + } + if (sep != paren) appendPQExpBufferStr(&sql, ") "); @@ -641,7 +685,7 @@ run_reindex_command(PGconn *conn, ReindexType type, const char *name, */ static SimpleStringList * get_parallel_object_list(PGconn *conn, ReindexType type, - SimpleStringList *user_list, bool echo) + SimpleStringList *user_list, bool outdated, bool echo) { PQExpBufferData catalog_query; PQExpBufferData buf; @@ -660,16 +704,41 @@ get_parallel_object_list(PGconn *conn, ReindexType type, { case REINDEX_DATABASE: Assert(user_list == NULL); + appendPQExpBufferStr(&catalog_query, "SELECT c.relname, ns.nspname\n" " FROM pg_catalog.pg_class c\n" " JOIN pg_catalog.pg_namespace ns" - " ON c.relnamespace = ns.oid\n" + " ON c.relnamespace = ns.oid\n"); + + if (outdated) + { + appendPQExpBufferStr(&catalog_query, + " JOIN pg_catalog.pg_index i" + " ON c.oid = i.indrelid\n" + " JOIN pg_catalog.pg_class ci" + " ON i.indexrelid = ci.oid\n"); + } + + appendPQExpBufferStr(&catalog_query, " WHERE ns.nspname != 'pg_catalog'\n" " AND c.relkind IN (" CppAsString2(RELKIND_RELATION) ", " - CppAsString2(RELKIND_MATVIEW) ")\n" - " ORDER BY c.relpages DESC;"); + CppAsString2(RELKIND_MATVIEW) ")\n"); + + if (outdated) + { + appendPQExpBufferStr(&catalog_query, + " GROUP BY c.relname, ns.nspname\n" + " ORDER BY sum(ci.relpages)" + " FILTER (WHERE pg_catalog.pg_index_has_outdated_dependency(ci.oid)) DESC;"); + } + else + { + appendPQExpBufferStr(&catalog_query, + " ORDER BY c.relpages DESC;"); + } + break; case REINDEX_SCHEMA: @@ -687,7 +756,18 @@ get_parallel_object_list(PGconn *conn, ReindexType type, "SELECT c.relname, ns.nspname\n" " FROM pg_catalog.pg_class c\n" " JOIN pg_catalog.pg_namespace ns" - " ON c.relnamespace = ns.oid\n" + " ON c.relnamespace = ns.oid\n"); + + if (outdated) + { + appendPQExpBufferStr(&catalog_query, + " JOIN pg_catalog.pg_index i" + " ON c.oid = i.indrelid\n" + " JOIN pg_catalog.pg_class ci" + " ON i.indexrelid = ci.oid\n"); + } + + appendPQExpBufferStr(&catalog_query, " WHERE c.relkind IN (" CppAsString2(RELKIND_RELATION) ", " CppAsString2(RELKIND_MATVIEW) ")\n" @@ -705,8 +785,20 @@ get_parallel_object_list(PGconn *conn, ReindexType type, appendStringLiteralConn(&catalog_query, nspname, conn); } - appendPQExpBufferStr(&catalog_query, ")\n" - " ORDER BY c.relpages DESC;"); + appendPQExpBufferStr(&catalog_query, ")\n"); + + if (outdated) + { + appendPQExpBufferStr(&catalog_query, + " GROUP BY c.relname, ns.nspname\n" + " ORDER BY sum(ci.relpages)" + " FILTER (WHERE pg_catalog.pg_index_has_outdated_dependency(ci.oid)) DESC;"); + } + else + { + appendPQExpBufferStr(&catalog_query, + " ORDER BY c.relpages DESC;"); + } } break; @@ -754,7 +846,7 @@ static void reindex_all_databases(ConnParams *cparams, const char *progname, bool echo, bool quiet, bool verbose, bool concurrently, int concurrentCons, - const char *tablespace) + const char *tablespace, bool outdated) { PGconn *conn; PGresult *result; @@ -778,7 +870,7 @@ reindex_all_databases(ConnParams *cparams, reindex_one_database(cparams, REINDEX_DATABASE, NULL, progname, echo, verbose, concurrently, - concurrentCons, tablespace); + concurrentCons, tablespace, outdated); } PQclear(result); @@ -797,6 +889,7 @@ help(const char *progname) printf(_(" -e, --echo show the commands being sent to the server\n")); printf(_(" -i, --index=INDEX recreate specific index(es) only\n")); printf(_(" -j, --jobs=NUM use this many concurrent connections to reindex\n")); + printf(_(" --outdated only process indexes having outdated depencies\n")); printf(_(" -q, --quiet don't write any messages\n")); printf(_(" -s, --system reindex system catalogs\n")); printf(_(" -S, --schema=SCHEMA reindex specific schema(s) only\n")); diff --git a/src/bin/scripts/t/090_reindexdb.pl b/src/bin/scripts/t/090_reindexdb.pl index 159b637230..b60cac6081 100644 --- a/src/bin/scripts/t/090_reindexdb.pl +++ b/src/bin/scripts/t/090_reindexdb.pl @@ -3,7 +3,7 @@ use warnings; use PostgresNode; use TestLib; -use Test::More tests => 58; +use Test::More tests => 70; program_help_ok('reindexdb'); program_version_ok('reindexdb'); @@ -174,6 +174,9 @@ $node->command_fails( $node->command_fails( [ 'reindexdb', '-j', '2', '-i', 'i1', 'postgres' ], 'parallel reindexdb cannot process indexes'); +$node->command_fails( + [ 'reindexdb', '-s', '--outdated' ], + 'cannot reindex system catalog and filter indexes having outdated dependencies'); $node->issues_sql_like( [ 'reindexdb', '-j', '2', 'postgres' ], qr/statement:\ REINDEX SYSTEM postgres; @@ -196,3 +199,32 @@ $node->command_checks_all( qr/^reindexdb: warning: cannot reindex system catalogs concurrently, skipping all/s ], 'parallel reindexdb for system with --concurrently skips catalogs'); + +# Temporarily downgrade client-min-message to get the no-op report +$ENV{PGOPTIONS} = '--client-min-messages=NOTICE'; +$node->command_checks_all( + [ 'reindexdb', '--outdated', '-v', '-t', 's1.t1', 'postgres' ], + 0, + [qr/^$/], + [qr/table "t1" has no indexes to reindex/], + 'verbose reindexdb for outdated dependencies on a specific table reports no-op tables'); + +$node->command_checks_all( + [ 'reindexdb', '--outdated', '-v', '-d', 'postgres' ], + 0, + [qr/^$/], + [qr/^$/], + 'verbose reindexdb for outdated dependencies database wide silently ignore all tables'); +$node->command_checks_all( + [ 'reindexdb', '--outdated', '-v', '-j', '2', '-d', 'postgres' ], + 0, + [qr/^$/], + [qr/table "t1" has no indexes to reindex/], + 'parallel verbose reindexdb for outdated dependencies database wide reports no-op tables'); + +# Switch back to WARNING client-min-message +$ENV{PGOPTIONS} = '--client-min-messages=WARNING'; +$node->issues_sql_like( + [ 'reindexdb', '--outdated', '-t', 's1.t1', 'postgres' ], + qr/.*statement: REINDEX \(OUTDATED\) TABLE s1\.t1;/, + 'reindexdb for outdated dependencies specify the OUTDATED keyword'); -- 2.30.1 --oao5se3im4osqowp-- ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Mutable foreign key constraints @ 2024-09-13 02:41 Laurenz Albe <[email protected]> 2024-09-13 13:05 ` Re: Mutable foreign key constraints Andreas Karlsson <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Laurenz Albe @ 2024-09-13 02:41 UTC (permalink / raw) To: Tom Lane <[email protected]>; [email protected] On Thu, 2024-09-12 at 17:33 -0400, Tom Lane wrote: > I happened to notice that Postgres will let you do > > regression=# create table foo (id timestamp primary key); > CREATE TABLE > regression=# create table bar (ts timestamptz references foo); > CREATE TABLE > > This strikes me as a pretty bad idea, because whether a particular > timestamp is equal to a particular timestamptz depends on your > timezone setting. Thus the constraint could appear to be violated > after a timezone change. > > I'm inclined to propose rejecting FK constraints if the comparison > operator is not immutable. I think that is the only sane thing to do. Consider test=> SHOW timezone; TimeZone ═══════════════ Europe/Vienna (1 row) test=> INSERT INTO foo VALUES ('2024-09-13 12:00:00'); INSERT 0 1 test=> INSERT INTO bar VALUES ('2024-09-13 12:00:00+02'); INSERT 0 1 test=> SELECT * FROM foo JOIN bar ON foo.id = bar.ts; id │ ts ═════════════════════╪════════════════════════ 2024-09-13 12:00:00 │ 2024-09-13 12:00:00+02 (1 row) test=> SET timezone = 'Asia/Kolkata'; SET test=> SELECT * FROM foo JOIN bar ON foo.id = bar.ts; id │ ts ════╪════ (0 rows) test=> INSERT INTO foo VALUES ('2024-09-14 12:00:00'); INSERT 0 1 test=> INSERT INTO bar VALUES ('2024-09-14 12:00:00+02'); ERROR: insert or update on table "bar" violates foreign key constraint "bar_ts_fkey" DETAIL: Key (ts)=(2024-09-14 15:30:00+05:30) is not present in table "foo". That's very broken and should not be allowed. > A possible objection is that if anybody has such a setup and > hasn't noticed a problem because they never change their > timezone setting, they might not appreciate us breaking it. I hope that there are few cases of that in the field, and I think it is OK to break them. After all, it can be fixed with a simple ALTER TABLE foo ALTER id TYPE timestamptz; If the session time zone is UTC, that wouldn't even require a rewrite. I agree that it cannot be backpatched. Yours, Laurenz Albe ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Mutable foreign key constraints 2024-09-13 02:41 Re: Mutable foreign key constraints Laurenz Albe <[email protected]> @ 2024-09-13 13:05 ` Andreas Karlsson <[email protected]> 2024-09-13 13:38 ` Re: Mutable foreign key constraints Vik Fearing <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Andreas Karlsson @ 2024-09-13 13:05 UTC (permalink / raw) To: Laurenz Albe <[email protected]>; Tom Lane <[email protected]>; [email protected] On 9/13/24 4:41 AM, Laurenz Albe wrote: > That's very broken and should not be allowed. +1 >> A possible objection is that if anybody has such a setup and >> hasn't noticed a problem because they never change their >> timezone setting, they might not appreciate us breaking it. > > I hope that there are few cases of that in the field, and I think it > is OK to break them. After all, it can be fixed with a simple > > ALTER TABLE foo ALTER id TYPE timestamptz; > > If the session time zone is UTC, that wouldn't even require a rewrite. > > I agree that it cannot be backpatched. I unfortunately suspect there might be more cases than we think in the field due to many people not understanding the difference between timestamp and timestamptz but the good thing is that timestamp/timestamptz are rare in foreign keys, even in composite ones. Since this is quite broken and does not have any real world usefulness I think we should just go ahead and disallow it and have a few people complain. Andreas ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Mutable foreign key constraints 2024-09-13 02:41 Re: Mutable foreign key constraints Laurenz Albe <[email protected]> 2024-09-13 13:05 ` Re: Mutable foreign key constraints Andreas Karlsson <[email protected]> @ 2024-09-13 13:38 ` Vik Fearing <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Vik Fearing @ 2024-09-13 13:38 UTC (permalink / raw) To: Andreas Karlsson <[email protected]>; Laurenz Albe <[email protected]>; Tom Lane <[email protected]>; [email protected] On 9/13/24 15:05, Andreas Karlsson wrote: > On 9/13/24 4:41 AM, Laurenz Albe wrote: >> That's very broken and should not be allowed. > > +1 > >>> A possible objection is that if anybody has such a setup and >>> hasn't noticed a problem because they never change their >>> timezone setting, they might not appreciate us breaking it. >> >> I hope that there are few cases of that in the field, and I think it >> is OK to break them. After all, it can be fixed with a simple >> >> ALTER TABLE foo ALTER id TYPE timestamptz; >> >> If the session time zone is UTC, that wouldn't even require a rewrite. >> >> I agree that it cannot be backpatched. > > I unfortunately suspect there might be more cases than we think in the > field due to many people not understanding the difference between > timestamp and timestamptz but the good thing is that > timestamp/timestamptz are rare in foreign keys, even in composite ones. It will become a lot more common with WITHOUT OVERLAPS, so I think it is important to fix this at the same time or earlier as that feature. > Since this is quite broken and does not have any real world usefulness I > think we should just go ahead and disallow it and have a few people > complain. +1 -- Vik Fearing ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-09-13 13:38 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-02-24 17:33 [PATCH v5 2/2] Add a --outdated option to reindexdb Julien Rouhaud <[email protected]> 2024-09-13 02:41 Re: Mutable foreign key constraints Laurenz Albe <[email protected]> 2024-09-13 13:05 ` Re: Mutable foreign key constraints Andreas Karlsson <[email protected]> 2024-09-13 13:38 ` Re: Mutable foreign key constraints Vik Fearing <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox