public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v28 2/5] Add pg_depend.refobjversion. 11+ messages / 6 participants [nested] [flat]
* [PATCH v28 2/5] Add pg_depend.refobjversion. @ 2019-05-28 18:16 Thomas Munro <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Thomas Munro @ 2019-05-28 18:16 UTC (permalink / raw) Provide a place for the version of referenced database objects to be recorded. Later commits will be able to use this to record dependencies on collation versions for indexes and maybe more. Author: Thomas Munro <[email protected]> Reviewed-by: Julien Rouhaud <[email protected]> Reviewed-by: Peter Eisentraut <[email protected]> Reviewed-by: Michael Paquier <[email protected]> Discussion: https://postgr.es/m/CAEepm%3D0uEQCpfq_%2BLYFBdArCe4Ot98t1aR4eYiYTe%3DyavQygiQ%40mail.gmail.com --- doc/src/sgml/catalogs.sgml | 12 +++++++ src/backend/catalog/dependency.c | 14 +++++--- src/backend/catalog/pg_depend.c | 14 +++++--- src/bin/initdb/initdb.c | 44 +++++++++++------------ src/include/catalog/dependency.h | 1 + src/include/catalog/pg_depend.h | 4 +++ src/include/catalog/toasting.h | 1 + src/test/regress/expected/misc_sanity.out | 4 +-- 8 files changed, 62 insertions(+), 32 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index a28ae89e13..2567863dd9 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -3293,6 +3293,18 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l A code defining the specific semantics of this dependency relationship; see text </para></entry> </row> + + <row> + <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>refobjversion</structfield> <type>text</type> + </para> + <para> + An optional version for the referenced object. The only current use of + <structfield>refobjversion</structfield> is to record dependencies + between indexes and collation versions. + </para></entry> + </row> + </tbody> </tgroup> </table> diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c index f515e2c308..1a927377e7 100644 --- a/src/backend/catalog/dependency.c +++ b/src/backend/catalog/dependency.c @@ -1600,7 +1600,9 @@ recordDependencyOnExpr(const ObjectAddress *depender, /* And record 'em */ recordMultipleDependencies(depender, - context.addrs->refs, context.addrs->numrefs, + context.addrs->refs, + context.addrs->numrefs, + NULL, behavior); free_object_addresses(context.addrs); @@ -1687,7 +1689,9 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender, /* Record the self-dependencies with the appropriate direction */ if (!reverse_self) recordMultipleDependencies(depender, - self_addrs->refs, self_addrs->numrefs, + self_addrs->refs, + self_addrs->numrefs, + NULL, self_behavior); else { @@ -1707,7 +1711,9 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender, /* Record the external dependencies */ recordMultipleDependencies(depender, - context.addrs->refs, context.addrs->numrefs, + context.addrs->refs, + context.addrs->numrefs, + NULL, behavior); free_object_addresses(context.addrs); @@ -2679,7 +2685,7 @@ record_object_address_dependencies(const ObjectAddress *depender, { eliminate_duplicate_dependencies(referenced); recordMultipleDependencies(depender, - referenced->refs, referenced->numrefs, + referenced->refs, referenced->numrefs, NULL, behavior); } diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c index 70baf03178..d86f85d142 100644 --- a/src/backend/catalog/pg_depend.c +++ b/src/backend/catalog/pg_depend.c @@ -24,6 +24,7 @@ #include "catalog/pg_extension.h" #include "commands/extension.h" #include "miscadmin.h" +#include "utils/builtins.h" #include "utils/fmgroids.h" #include "utils/lsyscache.h" #include "utils/rel.h" @@ -44,7 +45,7 @@ recordDependencyOn(const ObjectAddress *depender, const ObjectAddress *referenced, DependencyType behavior) { - recordMultipleDependencies(depender, referenced, 1, behavior); + recordMultipleDependencies(depender, referenced, 1, NULL, behavior); } /* @@ -55,6 +56,7 @@ void recordMultipleDependencies(const ObjectAddress *depender, const ObjectAddress *referenced, int nreferenced, + const char *version, DependencyType behavior) { Relation dependDesc; @@ -79,8 +81,6 @@ recordMultipleDependencies(const ObjectAddress *depender, /* Don't open indexes unless we need to make an update */ indstate = NULL; - memset(nulls, false, sizeof(nulls)); - for (i = 0; i < nreferenced; i++, referenced++) { /* @@ -94,6 +94,9 @@ recordMultipleDependencies(const ObjectAddress *depender, * Record the Dependency. Note we don't bother to check for * duplicate dependencies; there's no harm in them. */ + memset(nulls, false, sizeof(nulls)); + memset(values, 0, sizeof(values)); + values[Anum_pg_depend_classid - 1] = ObjectIdGetDatum(depender->classId); values[Anum_pg_depend_objid - 1] = ObjectIdGetDatum(depender->objectId); values[Anum_pg_depend_objsubid - 1] = Int32GetDatum(depender->objectSubId); @@ -101,9 +104,12 @@ recordMultipleDependencies(const ObjectAddress *depender, values[Anum_pg_depend_refclassid - 1] = ObjectIdGetDatum(referenced->classId); values[Anum_pg_depend_refobjid - 1] = ObjectIdGetDatum(referenced->objectId); values[Anum_pg_depend_refobjsubid - 1] = Int32GetDatum(referenced->objectSubId); + if (version) + values[Anum_pg_depend_refobjversion - 1] = CStringGetTextDatum(version); + else + nulls[Anum_pg_depend_refobjversion - 1] = true; values[Anum_pg_depend_deptype - 1] = CharGetDatum((char) behavior); - tup = heap_form_tuple(dependDesc->rd_att, values, nulls); /* fetch index info only when we know we need it */ diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index 786672b1b6..4b2a58cf18 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -1572,55 +1572,55 @@ setup_depend(FILE *cmdfd) "DELETE FROM pg_shdepend;\n\n", "VACUUM pg_shdepend;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_class;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_proc;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_type;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_cast;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_constraint;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_conversion;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_attrdef;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_language;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_operator;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_opclass;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_opfamily;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_am;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_amop;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_amproc;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_rewrite;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_trigger;\n\n", /* * restriction here to avoid pinning the public namespace */ - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_namespace " " WHERE nspname LIKE 'pg%';\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_ts_parser;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_ts_dict;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_ts_template;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_ts_config;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',NULL" " FROM pg_collation;\n\n", "INSERT INTO pg_shdepend SELECT 0,0,0,0, tableoid,oid, 'p' " " FROM pg_authid;\n\n", diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h index a8f7e9965b..3baa5e498a 100644 --- a/src/include/catalog/dependency.h +++ b/src/include/catalog/dependency.h @@ -189,6 +189,7 @@ extern void recordDependencyOn(const ObjectAddress *depender, extern void recordMultipleDependencies(const ObjectAddress *depender, const ObjectAddress *referenced, int nreferenced, + const char *version, DependencyType behavior); extern void recordDependencyOnCurrentExtension(const ObjectAddress *object, diff --git a/src/include/catalog/pg_depend.h b/src/include/catalog/pg_depend.h index ccf0a98330..7489022795 100644 --- a/src/include/catalog/pg_depend.h +++ b/src/include/catalog/pg_depend.h @@ -61,6 +61,10 @@ CATALOG(pg_depend,2608,DependRelationId) * field. See DependencyType in catalog/dependency.h. */ char deptype; /* see codes in dependency.h */ +#ifdef CATALOG_VARLEN + text refobjversion; /* version tracking, NULL if not used or + * unknown */ +#endif } FormData_pg_depend; /* ---------------- diff --git a/src/include/catalog/toasting.h b/src/include/catalog/toasting.h index 8f131893dc..e320d82203 100644 --- a/src/include/catalog/toasting.h +++ b/src/include/catalog/toasting.h @@ -53,6 +53,7 @@ DECLARE_TOAST(pg_aggregate, 4159, 4160); DECLARE_TOAST(pg_attrdef, 2830, 2831); DECLARE_TOAST(pg_constraint, 2832, 2833); DECLARE_TOAST(pg_default_acl, 4143, 4144); +DECLARE_TOAST(pg_depend, 8888, 8889); DECLARE_TOAST(pg_description, 2834, 2835); DECLARE_TOAST(pg_event_trigger, 4145, 4146); DECLARE_TOAST(pg_extension, 4147, 4148); diff --git a/src/test/regress/expected/misc_sanity.out b/src/test/regress/expected/misc_sanity.out index 8538173ff8..d40afeef78 100644 --- a/src/test/regress/expected/misc_sanity.out +++ b/src/test/regress/expected/misc_sanity.out @@ -18,8 +18,8 @@ WHERE refclassid = 0 OR refobjid = 0 OR deptype NOT IN ('a', 'e', 'i', 'n', 'p') OR (deptype != 'p' AND (classid = 0 OR objid = 0)) OR (deptype = 'p' AND (classid != 0 OR objid != 0 OR objsubid != 0)); - classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype ----------+-------+----------+------------+----------+-------------+--------- + classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype | refobjversion +---------+-------+----------+------------+----------+-------------+---------+--------------- (0 rows) -- **************** pg_shdepend **************** -- 2.20.1 --gclmslubawp3yaq7 Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="v28-0003-Track-collation-versions-for-indexes.patch" ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: post-freeze damage control @ 2024-04-11 00:23 Tom Kincaid <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Tom Kincaid @ 2024-04-11 00:23 UTC (permalink / raw) To: David Steele <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andrey M. Borodin <[email protected]>; Alvaro Herrera <[email protected]>; Stefan Fercot <[email protected]>; Robert Haas <[email protected]>; Tomas Vondra <[email protected]>; pgsql-hackers > > > Yeah, that's an excellent practive, but is why I'm less worried for > > this feature. The docs at [1] caution about "not to remove earlier > > backups if they might be needed when restoring later incremental > > backups". Like Alvaro said, should we insist a bit more about the WAL > > retention part in this section of the docs, down to the last full > > backup? > > I think that would make sense in general. But if we are doing it because > we lack confidence in the incremental backup feature maybe that's a sign > that the feature should be released as experimental (or not released at > all). > > The extensive Beta process we have can be used to build confidence we need in a feature that has extensive review and currently has no known issues or outstanding objections. > Regards, > -David > > > -- Thomas John Kincaid ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: post-freeze damage control @ 2024-04-11 01:52 David Steele <[email protected]> parent: Tom Kincaid <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: David Steele @ 2024-04-11 01:52 UTC (permalink / raw) To: Tom Kincaid <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andrey M. Borodin <[email protected]>; Alvaro Herrera <[email protected]>; Stefan Fercot <[email protected]>; Robert Haas <[email protected]>; Tomas Vondra <[email protected]>; pgsql-hackers On 4/11/24 10:23, Tom Kincaid wrote: > > The extensive Beta process we have can be used to build confidence we > need in a feature that has extensive review and currently has no known > issues or outstanding objections. I did have objections, here [1] and here [2]. I think the complexity, space requirements, and likely performance issues involved in restores are going to be a real problem for users. Some of these can be addressed in future releases, but I can't escape the feeling that what we are releasing here is half-baked. Also, there are outstanding issues here [3] and now here [4]. Regards, -David [1] https://www.postgresql.org/message-id/590e3017-da1f-4af6-9bf0-1679511ca7e5%40pgmasters.net [2] https://www.postgresql.org/message-id/11b38a96-6ded-4668-b772-40f992132797%40pgmasters.net [3] https://www.postgresql.org/message-id/flat/05fb32c9-18d8-4f72-9af3-f41576c33119%40pgmasters.net#bb04... [4] https://www.postgresql.org/message-id/flat/9badd24d-5bd9-4c35-ba85-4c38a2feb73e%40pgmasters.net ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: post-freeze damage control @ 2024-04-11 10:26 Tomas Vondra <[email protected]> parent: David Steele <[email protected]> 0 siblings, 2 replies; 11+ messages in thread From: Tomas Vondra @ 2024-04-11 10:26 UTC (permalink / raw) To: David Steele <[email protected]>; Tom Kincaid <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andrey M. Borodin <[email protected]>; Alvaro Herrera <[email protected]>; Stefan Fercot <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers On 4/11/24 03:52, David Steele wrote: > On 4/11/24 10:23, Tom Kincaid wrote: >> >> The extensive Beta process we have can be used to build confidence we >> need in a feature that has extensive review and currently has no known >> issues or outstanding objections. > > I did have objections, here [1] and here [2]. I think the complexity, > space requirements, and likely performance issues involved in restores > are going to be a real problem for users. Some of these can be addressed > in future releases, but I can't escape the feeling that what we are > releasing here is half-baked. > I haven't been part of those discussions, and that part of the thread is a couple months old already, so I'll share my view here instead. I do not think it's half-baked. I certainly agree there are limitations, and there's all kinds of bells and whistles we could add, but I think the fundamental infrastructure is corrent and a meaningful step forward. Would I wish it to handle .tar for example? Sure I would. But I think it's something we can add in the future - if we require all of this to happen in a single release, it'll never happen. FWIW that discussion also mentions stuff that I think the feature should not do. In particular, I don't think the ambition was (or should be) to make pg_basebackup into a stand-alone tool. I always saw pg_basebackup more as an interface to "backup steps" correctly rather than a complete backup solution that'd manage backup registry, retention, etc. > Also, there are outstanding issues here [3] and now here [4]. > I agree with some of this, I'll respond in the threads. regards Tomas -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: post-freeze damage control @ 2024-04-11 21:48 David Steele <[email protected]> parent: Tomas Vondra <[email protected]> 1 sibling, 2 replies; 11+ messages in thread From: David Steele @ 2024-04-11 21:48 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; Tom Kincaid <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andrey M. Borodin <[email protected]>; Alvaro Herrera <[email protected]>; Stefan Fercot <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers On 4/11/24 20:26, Tomas Vondra wrote: > > On 4/11/24 03:52, David Steele wrote: >> On 4/11/24 10:23, Tom Kincaid wrote: >>> >>> The extensive Beta process we have can be used to build confidence we >>> need in a feature that has extensive review and currently has no known >>> issues or outstanding objections. >> >> I did have objections, here [1] and here [2]. I think the complexity, >> space requirements, and likely performance issues involved in restores >> are going to be a real problem for users. Some of these can be addressed >> in future releases, but I can't escape the feeling that what we are >> releasing here is half-baked. > > I haven't been part of those discussions, and that part of the thread is > a couple months old already, so I'll share my view here instead. > > I do not think it's half-baked. I certainly agree there are limitations, > and there's all kinds of bells and whistles we could add, but I think > the fundamental infrastructure is corrent and a meaningful step forward. > Would I wish it to handle .tar for example? Sure I would. But I think > it's something we can add in the future - if we require all of this to > happen in a single release, it'll never happen. Fair enough, but the current release is extremely limited and it would be best if that was well understood by users. > FWIW that discussion also mentions stuff that I think the feature should > not do. In particular, I don't think the ambition was (or should be) to > make pg_basebackup into a stand-alone tool. I always saw pg_basebackup > more as an interface to "backup steps" correctly rather than a complete > backup solution that'd manage backup registry, retention, etc. Right -- this is exactly my issue. pg_basebackup was never easy to use as a backup solution and this feature makes it significantly more complicated. Complicated enough that it would be extremely difficult for most users to utilize in a meaningful way. But they'll try because it is a new pg_basebackup feature and they'll assume it is there to be used. Maybe it would be a good idea to make it clear in the documentation that significant tooling will be required to make it work. Regards, -David ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: post-freeze damage control @ 2024-04-12 02:15 Robert Haas <[email protected]> parent: David Steele <[email protected]> 1 sibling, 0 replies; 11+ messages in thread From: Robert Haas @ 2024-04-12 02:15 UTC (permalink / raw) To: David Steele <[email protected]>; +Cc: Tomas Vondra <[email protected]>; Tom Kincaid <[email protected]>; Michael Paquier <[email protected]>; Andrey M. Borodin <[email protected]>; Alvaro Herrera <[email protected]>; Stefan Fercot <[email protected]>; pgsql-hackers On Thu, Apr 11, 2024 at 5:48 PM David Steele <[email protected]> wrote: > But they'll try because it is a new pg_basebackup feature and they'll > assume it is there to be used. Maybe it would be a good idea to make it > clear in the documentation that significant tooling will be required to > make it work. I don't agree with that idea. LOTS of what we ship takes a significant amount of effort to make it work. You may well need a connection pooler. You may well need a failover manager which may or may not be separate from your connection pooler. You need a backup tool. You need a replication management tool which may or may not be separate from your backup tool and may or may not be separate from your failover tool. You probably need various out-of-core connections for the programming languages you need. You may need a management tool, and you probably need a monitoring tool. Some of the tools you might choose to do all that stuff themselves have a whole bunch of complex dependencies. It's a mess. Now, if someone were to say that we ought to talk about these issues in our documentation and maybe give people some ideas about how to get started, I would likely be in favor of that, modulo the small political problem that various people would want their solution to be the canonical one to which everyone gets referred. But I think it's wrong to pretend like this feature is somehow special, that it's somehow more raw or unfinished than tons of other things. I actually think it's significantly *better* than a lot of other things. If we add a disclaimer to the documentation saying "hey, this new incremental backup feature is half-finished garbage!", and meanwhile the documentation still says "hey, you can use cp as your archive_command," then we have completely lost our minds. I also think that you're being more negative about this than the facts justify. As I said to several colleagues today, I *fully* acknowledge that you have a lot more practical experience in this area than I do, and a bunch of good ideas. I was really pleased to see you talking about how it would be good if these tools worked on tar files - and I completely agree, and I hope that will happen, and I hope to help in making that happen. I think there are a bunch of other problems too, only some of which I can guess at. However, I think saying that this feature is not realistically intended to be used by end-users or that they will not be able to do so is over the top, and is actually kind of insulting. There has been more enthusiasm for this feature on this mailing list and elsewhere than I've gotten for anything I've developed in years. And I don't think that's because all of the people who have expressed enthusiasm are silly geese who don't understand how terrible it is. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: post-freeze damage control @ 2024-04-12 06:42 David Steele <[email protected]> parent: Tomas Vondra <[email protected]> 1 sibling, 0 replies; 11+ messages in thread From: David Steele @ 2024-04-12 06:42 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; Tom Kincaid <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andrey M. Borodin <[email protected]>; Alvaro Herrera <[email protected]>; Stefan Fercot <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers On 4/11/24 20:26, Tomas Vondra wrote: > On 4/11/24 03:52, David Steele wrote: >> On 4/11/24 10:23, Tom Kincaid wrote: >>> >>> The extensive Beta process we have can be used to build confidence we >>> need in a feature that has extensive review and currently has no known >>> issues or outstanding objections. >> >> I did have objections, here [1] and here [2]. I think the complexity, >> space requirements, and likely performance issues involved in restores >> are going to be a real problem for users. Some of these can be addressed >> in future releases, but I can't escape the feeling that what we are >> releasing here is half-baked. >> > I do not think it's half-baked. I certainly agree there are limitations, > and there's all kinds of bells and whistles we could add, but I think > the fundamental infrastructure is corrent and a meaningful step forward. > Would I wish it to handle .tar for example? Sure I would. But I think > it's something we can add in the future - if we require all of this to > happen in a single release, it'll never happen. I'm not sure that I really buy this argument, anyway. It is not uncommon for significant features to spend years in development before they are committed. This feature went from first introduction to commit in just over six months. Obviously Robert had been working on it for a while, but for a feature this large six months is a sprint. Regards, -David ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: post-freeze damage control @ 2024-04-12 12:12 Tomas Vondra <[email protected]> parent: David Steele <[email protected]> 1 sibling, 1 reply; 11+ messages in thread From: Tomas Vondra @ 2024-04-12 12:12 UTC (permalink / raw) To: David Steele <[email protected]>; Tom Kincaid <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andrey M. Borodin <[email protected]>; Alvaro Herrera <[email protected]>; Stefan Fercot <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers On 4/11/24 23:48, David Steele wrote: > On 4/11/24 20:26, Tomas Vondra wrote: >> >> On 4/11/24 03:52, David Steele wrote: >>> On 4/11/24 10:23, Tom Kincaid wrote: >>>> >>>> The extensive Beta process we have can be used to build confidence we >>>> need in a feature that has extensive review and currently has no known >>>> issues or outstanding objections. >>> >>> I did have objections, here [1] and here [2]. I think the complexity, >>> space requirements, and likely performance issues involved in restores >>> are going to be a real problem for users. Some of these can be addressed >>> in future releases, but I can't escape the feeling that what we are >>> releasing here is half-baked. >> >> I haven't been part of those discussions, and that part of the thread is >> a couple months old already, so I'll share my view here instead. >> >> I do not think it's half-baked. I certainly agree there are limitations, >> and there's all kinds of bells and whistles we could add, but I think >> the fundamental infrastructure is corrent and a meaningful step forward. >> Would I wish it to handle .tar for example? Sure I would. But I think >> it's something we can add in the future - if we require all of this to >> happen in a single release, it'll never happen. > > Fair enough, but the current release is extremely limited and it would > be best if that was well understood by users. > >> FWIW that discussion also mentions stuff that I think the feature should >> not do. In particular, I don't think the ambition was (or should be) to >> make pg_basebackup into a stand-alone tool. I always saw pg_basebackup >> more as an interface to "backup steps" correctly rather than a complete >> backup solution that'd manage backup registry, retention, etc. > > Right -- this is exactly my issue. pg_basebackup was never easy to use > as a backup solution and this feature makes it significantly more > complicated. Complicated enough that it would be extremely difficult for > most users to utilize in a meaningful way. > Perhaps, I agree we could/should try to do better job to do backups, no argument there. But I still don't quite see why introducing such infrastructure to "manage backups" should be up to the patch adding incremental backups. I see it as something to build on top of pg_basebackup/pg_combinebackup, not into those tools. > But they'll try because it is a new pg_basebackup feature and they'll > assume it is there to be used. Maybe it would be a good idea to make it > clear in the documentation that significant tooling will be required to > make it work. > Sure, I'm not against making it clearer pg_combinebackup is not a complete backup solution, and documenting the existing restrictions. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: post-freeze damage control @ 2024-04-12 23:03 David Steele <[email protected]> parent: Tomas Vondra <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: David Steele @ 2024-04-12 23:03 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; Tom Kincaid <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andrey M. Borodin <[email protected]>; Alvaro Herrera <[email protected]>; Stefan Fercot <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers On 4/12/24 22:12, Tomas Vondra wrote: > On 4/11/24 23:48, David Steele wrote: >> On 4/11/24 20:26, Tomas Vondra wrote: >> >>> FWIW that discussion also mentions stuff that I think the feature should >>> not do. In particular, I don't think the ambition was (or should be) to >>> make pg_basebackup into a stand-alone tool. I always saw pg_basebackup >>> more as an interface to "backup steps" correctly rather than a complete >>> backup solution that'd manage backup registry, retention, etc. >> >> Right -- this is exactly my issue. pg_basebackup was never easy to use >> as a backup solution and this feature makes it significantly more >> complicated. Complicated enough that it would be extremely difficult for >> most users to utilize in a meaningful way. > > Perhaps, I agree we could/should try to do better job to do backups, no > argument there. But I still don't quite see why introducing such > infrastructure to "manage backups" should be up to the patch adding > incremental backups. I see it as something to build on top of > pg_basebackup/pg_combinebackup, not into those tools. I'm not saying that managing backups needs to be part of pg_basebackup, but I am saying without that it is not a complete backup solution. Therefore introducing advanced features that the user then has to figure out how to manage puts a large burden on them. Implementing pg_combinebackup inefficiently out of the gate just makes their life harder. >> But they'll try because it is a new pg_basebackup feature and they'll >> assume it is there to be used. Maybe it would be a good idea to make it >> clear in the documentation that significant tooling will be required to >> make it work. > > Sure, I'm not against making it clearer pg_combinebackup is not a > complete backup solution, and documenting the existing restrictions. Let's do that then. I think it would make sense to add caveats to the pg_combinebackup docs including space requirements, being explicit about the format required (e.g. plain), and also possible mitigation with COW filesystems. Regards, -David ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: post-freeze damage control @ 2024-04-13 10:18 Tomas Vondra <[email protected]> parent: David Steele <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Tomas Vondra @ 2024-04-13 10:18 UTC (permalink / raw) To: David Steele <[email protected]>; Tom Kincaid <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andrey M. Borodin <[email protected]>; Alvaro Herrera <[email protected]>; Stefan Fercot <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers On 4/13/24 01:03, David Steele wrote: > On 4/12/24 22:12, Tomas Vondra wrote: >> On 4/11/24 23:48, David Steele wrote: >>> On 4/11/24 20:26, Tomas Vondra wrote: >>> >>>> FWIW that discussion also mentions stuff that I think the feature >>>> should >>>> not do. In particular, I don't think the ambition was (or should be) to >>>> make pg_basebackup into a stand-alone tool. I always saw pg_basebackup >>>> more as an interface to "backup steps" correctly rather than a complete >>>> backup solution that'd manage backup registry, retention, etc. >>> >>> Right -- this is exactly my issue. pg_basebackup was never easy to use >>> as a backup solution and this feature makes it significantly more >>> complicated. Complicated enough that it would be extremely difficult for >>> most users to utilize in a meaningful way. >> >> Perhaps, I agree we could/should try to do better job to do backups, no >> argument there. But I still don't quite see why introducing such >> infrastructure to "manage backups" should be up to the patch adding >> incremental backups. I see it as something to build on top of >> pg_basebackup/pg_combinebackup, not into those tools. > > I'm not saying that managing backups needs to be part of pg_basebackup, > but I am saying without that it is not a complete backup solution. > Therefore introducing advanced features that the user then has to figure > out how to manage puts a large burden on them. Implementing > pg_combinebackup inefficiently out of the gate just makes their life > harder. > I agree with this in general, but I fail to see how it'd be the fault of this patch. It merely extends what pg_basebackup did before, so if it's not a complete solution now, it wasn't a complete solution before. Sure, I 100% agree it'd be great to have a more efficient pg_combinebackup with fewer restrictions. But if we make these limitations clear, I think it's better to have this than having nothing. >>> But they'll try because it is a new pg_basebackup feature and they'll >>> assume it is there to be used. Maybe it would be a good idea to make it >>> clear in the documentation that significant tooling will be required to >>> make it work. >> >> Sure, I'm not against making it clearer pg_combinebackup is not a >> complete backup solution, and documenting the existing restrictions. > > Let's do that then. I think it would make sense to add caveats to the > pg_combinebackup docs including space requirements, being explicit about > the format required (e.g. plain), and also possible mitigation with COW > filesystems. > OK. I'll add this as an open item, for me and Robert. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: post-freeze damage control @ 2024-04-16 08:47 Stefan Fercot <[email protected]> parent: Tomas Vondra <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Stefan Fercot @ 2024-04-16 08:47 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: David Steele <[email protected]>; Tom Kincaid <[email protected]>; Michael Paquier <[email protected]>; Andrey M. Borodin <[email protected]>; Alvaro Herrera <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers Hi, On Saturday, April 13th, 2024 at 12:18 PM, Tomas Vondra wrote: > On 4/13/24 01:03, David Steele wrote: > > On 4/12/24 22:12, Tomas Vondra wrote: > > > On 4/11/24 23:48, David Steele wrote: > > > > On 4/11/24 20:26, Tomas Vondra wrote: > > > > > > > > > FWIW that discussion also mentions stuff that I think the feature > > > > > should > > > > > not do. In particular, I don't think the ambition was (or should be) to > > > > > make pg_basebackup into a stand-alone tool. I always saw pg_basebackup > > > > > more as an interface to "backup steps" correctly rather than a complete > > > > > backup solution that'd manage backup registry, retention, etc. > > > > > > > > Right -- this is exactly my issue. pg_basebackup was never easy to use > > > > as a backup solution and this feature makes it significantly more > > > > complicated. Complicated enough that it would be extremely difficult for > > > > most users to utilize in a meaningful way. pg_basebackup has its own use-cases IMHO. It is very handy to simply take a copy of the running cluster, thanks to its ability to carry on the needed WAL segs without the user even needing to think about archive_command/pg_receivewal. And for this kind of tasks, the new incremental thing will not really be interesting and won't make things more complicated. I totally agree that we don't have any complete backup solution in core though. And adding more tools to the picture (pg_basebackup, pg_combinebackup, pg_receivewal, pg_verifybackup,...) will increase the need of on-top orchestration. But that's not new. And for people already having such orchestration, having the incremental feature will help. > > > Perhaps, I agree we could/should try to do better job to do backups, no > > > argument there. But I still don't quite see why introducing such > > > infrastructure to "manage backups" should be up to the patch adding > > > incremental backups. I see it as something to build on top of > > > pg_basebackup/pg_combinebackup, not into those tools. > > > > I'm not saying that managing backups needs to be part of pg_basebackup, > > but I am saying without that it is not a complete backup solution. > > Therefore introducing advanced features that the user then has to figure > > out how to manage puts a large burden on them. Implementing > > pg_combinebackup inefficiently out of the gate just makes their life > > harder. > > I agree with this in general, but I fail to see how it'd be the fault of > this patch. It merely extends what pg_basebackup did before, so if it's > not a complete solution now, it wasn't a complete solution before. +1. We can see it as a step to having a better backup solution in core for the future, but we definitely shouldn't rule out the fact that lots of people already developed such orchestration (home-made or relying to pgbarman, pgbackrest, wal-g,...). IMHO, if we're trying to extend the in core features, we should also aim at giving more lights and features for those tools (like adding more fields to the backup functions,...). > > > Sure, I'm not against making it clearer pg_combinebackup is not a > > > complete backup solution, and documenting the existing restrictions. > > > > Let's do that then. I think it would make sense to add caveats to the > > pg_combinebackup docs including space requirements, being explicit about > > the format required (e.g. plain), and also possible mitigation with COW > > filesystems. > > OK. I'll add this as an open item, for me and Robert. Thanks for this! It's probably not up to core docs to state all the steps that would be needed to develop a complete backup solution but documenting the links between the tools and mostly all the caveats (the "don't use INCREMENTAL.* filenames",...) will help users not be caught off guard. And as I mentioned in [1], IMO the earlier we can catch a potential issue (wrong filename, missing file,...), the better for the user. Thank you all for working on this. Kind Regards, -- Stefan FERCOT Data Egret (https://dataegret.com) [1] https://www.postgresql.org/message-id/vJnnuiaye5rNnCPN8h3xN1Y3lyUDESIgEQnR-Urat9_ld_fozShSJbEk8JDM_3... ^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2024-04-16 08:47 UTC | newest] Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2019-05-28 18:16 [PATCH v28 2/5] Add pg_depend.refobjversion. Thomas Munro <[email protected]> 2024-04-11 00:23 Re: post-freeze damage control Tom Kincaid <[email protected]> 2024-04-11 01:52 ` Re: post-freeze damage control David Steele <[email protected]> 2024-04-11 10:26 ` Re: post-freeze damage control Tomas Vondra <[email protected]> 2024-04-11 21:48 ` Re: post-freeze damage control David Steele <[email protected]> 2024-04-12 02:15 ` Re: post-freeze damage control Robert Haas <[email protected]> 2024-04-12 12:12 ` Re: post-freeze damage control Tomas Vondra <[email protected]> 2024-04-12 23:03 ` Re: post-freeze damage control David Steele <[email protected]> 2024-04-13 10:18 ` Re: post-freeze damage control Tomas Vondra <[email protected]> 2024-04-16 08:47 ` Re: post-freeze damage control Stefan Fercot <[email protected]> 2024-04-12 06:42 ` Re: post-freeze damage control David Steele <[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