public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v1] Remove useless savepoint level 7+ messages / 4 participants [nested] [flat]
* [PATCH v1] Remove useless savepoint level @ 2022-10-24 06:54 Japin Li <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Japin Li @ 2022-10-24 06:54 UTC (permalink / raw) --- src/backend/access/transam/xact.c | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index fd5103a78e..e8a90a3a30 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -190,7 +190,6 @@ typedef struct TransactionStateData FullTransactionId fullTransactionId; /* my FullTransactionId */ SubTransactionId subTransactionId; /* my subxact ID */ char *name; /* savepoint name, if any */ - int savepointLevel; /* savepoint level */ TransState state; /* low-level state */ TBlockState blockState; /* high-level state */ int nestingLevel; /* transaction nesting depth */ @@ -3234,12 +3233,10 @@ CommitTransactionCommand(void) case TBLOCK_SUBRESTART: { char *name; - int savepointLevel; /* save name and keep Cleanup from freeing it */ name = s->name; s->name = NULL; - savepointLevel = s->savepointLevel; AbortSubTransaction(); CleanupSubTransaction(); @@ -3247,7 +3244,6 @@ CommitTransactionCommand(void) DefineSavepoint(NULL); s = CurrentTransactionState; /* changed by push */ s->name = name; - s->savepointLevel = savepointLevel; /* This is the same as TBLOCK_SUBBEGIN case */ AssertState(s->blockState == TBLOCK_SUBBEGIN); @@ -3263,19 +3259,16 @@ CommitTransactionCommand(void) case TBLOCK_SUBABORT_RESTART: { char *name; - int savepointLevel; /* save name and keep Cleanup from freeing it */ name = s->name; s->name = NULL; - savepointLevel = s->savepointLevel; CleanupSubTransaction(); DefineSavepoint(NULL); s = CurrentTransactionState; /* changed by push */ s->name = name; - s->savepointLevel = savepointLevel; /* This is the same as TBLOCK_SUBBEGIN case */ AssertState(s->blockState == TBLOCK_SUBBEGIN); @@ -4352,11 +4345,6 @@ ReleaseSavepoint(const char *name) (errcode(ERRCODE_S_E_INVALID_SPECIFICATION), errmsg("savepoint \"%s\" does not exist", name))); - /* disallow crossing savepoint level boundaries */ - if (target->savepointLevel != s->savepointLevel) - ereport(ERROR, - (errcode(ERRCODE_S_E_INVALID_SPECIFICATION), - errmsg("savepoint \"%s\" does not exist within current savepoint level", name))); /* * Mark "commit pending" all subtransactions up to the target @@ -4461,11 +4449,6 @@ RollbackToSavepoint(const char *name) (errcode(ERRCODE_S_E_INVALID_SPECIFICATION), errmsg("savepoint \"%s\" does not exist", name))); - /* disallow crossing savepoint level boundaries */ - if (target->savepointLevel != s->savepointLevel) - ereport(ERROR, - (errcode(ERRCODE_S_E_INVALID_SPECIFICATION), - errmsg("savepoint \"%s\" does not exist within current savepoint level", name))); /* * Mark "abort pending" all subtransactions up to the target @@ -5253,7 +5236,6 @@ PushTransaction(void) s->parent = p; s->nestingLevel = p->nestingLevel + 1; s->gucNestLevel = NewGUCNestLevel(); - s->savepointLevel = p->savepointLevel; s->state = TRANS_DEFAULT; s->blockState = TBLOCK_SUBBEGIN; GetUserIdAndSecContext(&s->prevUser, &s->prevSecContext); -- 2.25.1 --=-=-=-- ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pg_dump versus hash partitioning @ 2023-03-11 03:10 Tom Lane <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Tom Lane @ 2023-03-11 03:10 UTC (permalink / raw) To: Julien Rouhaud <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Alvaro Herrera <[email protected]>; David Rowley <[email protected]>; Peter Geoghegan <[email protected]>; Robert Haas <[email protected]>; [email protected]; Andrew <[email protected]> Julien Rouhaud <[email protected]> writes: > Working on some side project that can cause dump of hash partitions to be > routed to a different partition, I realized that --load-via-partition-root can > indeed cause deadlock in such case without FK dependency or anything else. > The problem is that each worker will perform a TRUNCATE TABLE ONLY followed by > a copy of the original partition's data in a transaction, and that obviously > will lead to deadlock if the original and locked partition and the restored > partition are different. Oh, interesting. I wonder if we can rearrange things to avoid that. regards, tom lane ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pg_dump versus hash partitioning @ 2023-03-11 03:32 Julien Rouhaud <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Julien Rouhaud @ 2023-03-11 03:32 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Alvaro Herrera <[email protected]>; David Rowley <[email protected]>; Peter Geoghegan <[email protected]>; Robert Haas <[email protected]>; [email protected]; Andrew <[email protected]> On Fri, Mar 10, 2023 at 10:10:14PM -0500, Tom Lane wrote: > Julien Rouhaud <[email protected]> writes: > > Working on some side project that can cause dump of hash partitions to be > > routed to a different partition, I realized that --load-via-partition-root can > > indeed cause deadlock in such case without FK dependency or anything else. > > > The problem is that each worker will perform a TRUNCATE TABLE ONLY followed by > > a copy of the original partition's data in a transaction, and that obviously > > will lead to deadlock if the original and locked partition and the restored > > partition are different. > > Oh, interesting. I wonder if we can rearrange things to avoid that. The BEGIN + TRUNCATE is only there to avoid generating WAL records just in case the wal_level is minimal. I don't remember if that optimization still exists, but if yes we could avoid doing that if the server's wal_level is replica or higher? That's not perfect but it would help in many cases. ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pg_dump versus hash partitioning @ 2023-03-12 19:46 Tom Lane <[email protected]> parent: Julien Rouhaud <[email protected]> 0 siblings, 2 replies; 7+ messages in thread From: Tom Lane @ 2023-03-12 19:46 UTC (permalink / raw) To: Julien Rouhaud <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Alvaro Herrera <[email protected]>; David Rowley <[email protected]>; Peter Geoghegan <[email protected]>; Robert Haas <[email protected]>; [email protected]; Andrew <[email protected]> Julien Rouhaud <[email protected]> writes: > The BEGIN + TRUNCATE is only there to avoid generating WAL records just in case > the wal_level is minimal. I don't remember if that optimization still exists, > but if yes we could avoid doing that if the server's wal_level is replica or > higher? That's not perfect but it would help in many cases. After thinking about this, it seems like a better idea is to skip the TRUNCATE if we're doing load-via-partition-root. In that case it's clearly a dangerous thing to do regardless of deadlock worries, since it risks discarding previously-loaded data that came over from another partition. (IOW this seems like an independent, pre-existing bug in load-via-partition-root mode.) The trick is to detect in pg_restore whether pg_dump chose to do load-via-partition-root. If we have a COPY statement we can fairly easily examine it to see if the target table is what we expect or something else. However, if the table was dumped as INSERT statements it'd be far messier; the INSERTs are not readily accessible from the code that needs to make the decision. What I propose we do about that is further tweak things so that load-via-partition-root forces dumping via COPY. AFAIK the only compelling use-case for dump-as-INSERTs is in transferring data to a non-Postgres database, which is a context in which dumping partitioned tables as such is pretty hopeless anyway. (I wonder if we should have some way to dump all the contents of a partitioned table as if it were unpartitioned, to support such migration.) An alternative could be to extend the archive TOC format to record directly whether a given TABLE DATA object loads data via partition root or normally. Potentially we could do that without an archive format break by defining te->defn for TABLE DATA to be empty for normal dumps (as it is now) or something like "-- load via partition root" for the load-via-partition-root case. However, depending on examination of the COPY command would already work for the vast majority of existing archive files, so I feel like it might be the preferable choice. Thoughts? regards, tom lane ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pg_dump versus hash partitioning @ 2023-03-12 19:54 Justin Pryzby <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 1 reply; 7+ messages in thread From: Justin Pryzby @ 2023-03-12 19:54 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Alvaro Herrera <[email protected]>; David Rowley <[email protected]>; Peter Geoghegan <[email protected]>; Robert Haas <[email protected]>; [email protected]; Andrew <[email protected]> On Sun, Mar 12, 2023 at 03:46:52PM -0400, Tom Lane wrote: > What I propose we do about that is further tweak things so that > load-via-partition-root forces dumping via COPY. AFAIK the only > compelling use-case for dump-as-INSERTs is in transferring data > to a non-Postgres database, which is a context in which dumping > partitioned tables as such is pretty hopeless anyway. (I wonder if > we should have some way to dump all the contents of a partitioned > table as if it were unpartitioned, to support such migration.) I think that what this other thread is about. https://commitfest.postgresql.org/42/4130/ pg_dump all child tables with the root table ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pg_dump versus hash partitioning @ 2023-03-12 20:02 Tom Lane <[email protected]> parent: Justin Pryzby <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2023-03-12 20:02 UTC (permalink / raw) To: Justin Pryzby <[email protected]>; +Cc: Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Alvaro Herrera <[email protected]>; David Rowley <[email protected]>; Peter Geoghegan <[email protected]>; Robert Haas <[email protected]>; [email protected]; Andrew <[email protected]> Justin Pryzby <[email protected]> writes: > On Sun, Mar 12, 2023 at 03:46:52PM -0400, Tom Lane wrote: >> What I propose we do about that is further tweak things so that >> load-via-partition-root forces dumping via COPY. AFAIK the only >> compelling use-case for dump-as-INSERTs is in transferring data >> to a non-Postgres database, which is a context in which dumping >> partitioned tables as such is pretty hopeless anyway. (I wonder if >> we should have some way to dump all the contents of a partitioned >> table as if it were unpartitioned, to support such migration.) > I think that what this other thread is about. > https://commitfest.postgresql.org/42/4130/ > pg_dump all child tables with the root table As far as I understood (didn't actually read the latest patch) that one is just about easily selecting all the partitions of a partitioned table when doing a selective dump. It's not helping you produce a non-Postgres-specific dump. Although I guess by combining load-via-partition-root, data-only mode, and dump-as-inserts you could produce a clean collection of non-partition-dependent INSERT commands ... so maybe we'd better not force dump-as-inserts off. I'm starting to like the te->defn hack more. regards, tom lane ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pg_dump versus hash partitioning @ 2023-03-13 08:33 Julien Rouhaud <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 0 replies; 7+ messages in thread From: Julien Rouhaud @ 2023-03-13 08:33 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Alvaro Herrera <[email protected]>; David Rowley <[email protected]>; Peter Geoghegan <[email protected]>; Robert Haas <[email protected]>; [email protected]; Andrew <[email protected]> On Sun, Mar 12, 2023 at 03:46:52PM -0400, Tom Lane wrote: > Julien Rouhaud <[email protected]> writes: > > The BEGIN + TRUNCATE is only there to avoid generating WAL records just in case > > the wal_level is minimal. I don't remember if that optimization still exists, > > but if yes we could avoid doing that if the server's wal_level is replica or > > higher? That's not perfect but it would help in many cases. > > After thinking about this, it seems like a better idea is to skip the > TRUNCATE if we're doing load-via-partition-root. In that case it's > clearly a dangerous thing to do regardless of deadlock worries, since > it risks discarding previously-loaded data that came over from another > partition. (IOW this seems like an independent, pre-existing bug in > load-via-partition-root mode.) It's seems quite unlikely to be able to actually truncate already restored data without eventually going into a deadlock, but it's still possible so agreed. > The trick is to detect in pg_restore whether pg_dump chose to do > load-via-partition-root. If we have a COPY statement we can fairly > easily examine it to see if the target table is what we expect or > something else. However, if the table was dumped as INSERT statements > it'd be far messier; the INSERTs are not readily accessible from the > code that needs to make the decision. > > What I propose we do about that is further tweak things so that > load-via-partition-root forces dumping via COPY. AFAIK the only > compelling use-case for dump-as-INSERTs is in transferring data > to a non-Postgres database, which is a context in which dumping > partitioned tables as such is pretty hopeless anyway. It seems acceptable to me. > (I wonder if > we should have some way to dump all the contents of a partitioned > table as if it were unpartitioned, to support such migration.) (this would be nice to have) > An alternative could be to extend the archive TOC format to record > directly whether a given TABLE DATA object loads data via partition > root or normally. Potentially we could do that without an archive > format break by defining te->defn for TABLE DATA to be empty for > normal dumps (as it is now) or something like "-- load via partition root" > for the load-via-partition-root case. However, depending on examination > of the COPY command would already work for the vast majority of existing > archive files, so I feel like it might be the preferable choice. Given that this approach wouldn't help with existing dump files (at least if using COPY, in any case the one using INSERT are doomed), so I'm slightly in favor of the first approach, and later add an easy and non magic incantation way to produce dumps that don't depend on partitioning. It would mean that you would only be able to produce such dumps using pg16 client binaries, but such version would also work with older server versions so it doesn't seem like a huge problem in the long run. ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2023-03-13 08:33 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2022-10-24 06:54 [PATCH v1] Remove useless savepoint level Japin Li <[email protected]> 2023-03-11 03:10 Re: pg_dump versus hash partitioning Tom Lane <[email protected]> 2023-03-11 03:32 ` Re: pg_dump versus hash partitioning Julien Rouhaud <[email protected]> 2023-03-12 19:46 ` Re: pg_dump versus hash partitioning Tom Lane <[email protected]> 2023-03-12 19:54 ` Re: pg_dump versus hash partitioning Justin Pryzby <[email protected]> 2023-03-12 20:02 ` Re: pg_dump versus hash partitioning Tom Lane <[email protected]> 2023-03-13 08:33 ` Re: pg_dump versus hash partitioning Julien Rouhaud <[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