public inbox for [email protected]
help / color / mirror / Atom feedRe: Postgres restart in the middle of exclusive backup and the presence of backup_label file
9+ messages / 5 participants
[nested] [flat]
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file
@ 2022-03-09 01:24 Chapman Flack <[email protected]>
2022-03-09 15:42 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Stephen Frost <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Chapman Flack @ 2022-03-09 01:24 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; David Steele <[email protected]>; +Cc: Stephen Frost <[email protected]>; [email protected]
On 03/08/22 17:12, Nathan Bossart wrote:
> I spent some time trying to come up with a workable script to replace the
> existing one. I think the main problem is that you need to write out both
> the backup label file and the tablespace map file, but I didn't find an
> easy way to write the different output columns of pg_backup_stop() to
> separate files via psql.
Something like this might work:
SELECT * FROM pg_backup_stop(true) \gset
\out /tmp/backup_label \qecho :labelfile
\out /tmp/tablespace_map \qecho :spcmapfile
\out
\! ... tar command adding /tmp/{backup_label,tablespace_map} to the tarball
I notice the \qecho adds a final newline (and so if :spcmapfile is empty,
a file containing a single newline is made). In a quick test with a bogus
restore_command, I did not see any error messages specific to the format
of the backup_label or tablespace_map files, so maybe the final newline
isn't a problem.
Assuming the newline isn't a problem, that might be simple enough to
use in an example, and maybe it's not a bad thing that it highlights a few
psql capabilities the reader might not have stumbled on before. Or, maybe
it is just too confusing to bother.
While agreeing that pg_basebackup is the production-ready thing that
does it all for you (with tests for likely errors and so on), I think
there is also some value in a dead-simple example that concretely
shows you what "it" is, what the basic steps are that happen beneath
pg_basebackup's chrome.
If the added newline is a problem, I haven't thought of a way to exclude
it that doesn't take the example out of the realm of dead-simple.
Regards,
-Chap
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file
2022-03-09 01:24 Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Chapman Flack <[email protected]>
@ 2022-03-09 15:42 ` Stephen Frost <[email protected]>
2022-03-09 16:22 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Magnus Hagander <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Stephen Frost @ 2022-03-09 15:42 UTC (permalink / raw)
To: Chapman Flack <[email protected]>; +Cc: Nathan Bossart <[email protected]>; David Steele <[email protected]>; [email protected]
Greetings,
* Chapman Flack ([email protected]) wrote:
> On 03/08/22 17:12, Nathan Bossart wrote:
> > I spent some time trying to come up with a workable script to replace the
> > existing one. I think the main problem is that you need to write out both
> > the backup label file and the tablespace map file, but I didn't find an
> > easy way to write the different output columns of pg_backup_stop() to
> > separate files via psql.
Let's not confuse ourselves here- the existing script *doesn't* work in
any reasonable way when we're talking about everything that needs to be
done to perform a backup. That a lot of people are using it because
it's in the documentation is an actively bad thing.
The same goes for the archive command example.
> Something like this might work:
>
> SELECT * FROM pg_backup_stop(true) \gset
>
> \out /tmp/backup_label \qecho :labelfile
> \out /tmp/tablespace_map \qecho :spcmapfile
> \out
> \! ... tar command adding /tmp/{backup_label,tablespace_map} to the tarball
... this doesn't do what's needed either. We could try to write down
some minimum set of things that are needed for a backup tool to do but
it's not something that a 3 line script is going to cover. Indeed, it's
a lot more like pg_basebackup and if we want to give folks a script to
use, it should be "run pg_basebackup".
> I notice the \qecho adds a final newline (and so if :spcmapfile is empty,
> a file containing a single newline is made). In a quick test with a bogus
> restore_command, I did not see any error messages specific to the format
> of the backup_label or tablespace_map files, so maybe the final newline
> isn't a problem.
>
> Assuming the newline isn't a problem, that might be simple enough to
> use in an example, and maybe it's not a bad thing that it highlights a few
> psql capabilities the reader might not have stumbled on before. Or, maybe
> it is just too confusing to bother.
It's more than just too confusing, it's actively bad because people will
actually use it and then end up with backups that don't work.
> While agreeing that pg_basebackup is the production-ready thing that
> does it all for you (with tests for likely errors and so on), I think
> there is also some value in a dead-simple example that concretely
> shows you what "it" is, what the basic steps are that happen beneath
> pg_basebackup's chrome.
Documenting everything that pg_basebackup does to make sure that the
backup is viable might be something to work on if someone is really
excited about this, but it's not 'dead-simple' and it's darn close to
the bare minimum, something that none of these simple scripts will come
anywhere close to being and instead they'll be far less than the
minimum.
> If the added newline is a problem, I haven't thought of a way to exclude
> it that doesn't take the example out of the realm of dead-simple.
I disagree that there's really a way to provide 'dead-simple' backups
with what's built into core without using pg_basebackup. If we want a
'dead-simple' solution in core then we'd need to write an appropriate
backup tool that does all the basic things and include and maintain
that. Writing a shell script isn't enough and we shouldn't encourage
our users to do exactly that by having it in our documentation because
then they'll think it's enough.
Thanks,
Stephen
Attachments:
[application/pgp-signature] signature.asc (819B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file
2022-03-09 01:24 Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Chapman Flack <[email protected]>
2022-03-09 15:42 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Stephen Frost <[email protected]>
@ 2022-03-09 16:22 ` Magnus Hagander <[email protected]>
2022-03-09 17:12 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Chapman Flack <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Magnus Hagander @ 2022-03-09 16:22 UTC (permalink / raw)
To: Stephen Frost <[email protected]>; +Cc: Chapman Flack <[email protected]>; Nathan Bossart <[email protected]>; David Steele <[email protected]>; [email protected]
On Wed, Mar 9, 2022 at 4:42 PM Stephen Frost <[email protected]> wrote:
>
> Greetings,
>
> * Chapman Flack ([email protected]) wrote:
> > On 03/08/22 17:12, Nathan Bossart wrote:
> > > I spent some time trying to come up with a workable script to replace the
> > > existing one. I think the main problem is that you need to write out both
> > > the backup label file and the tablespace map file, but I didn't find an
> > > easy way to write the different output columns of pg_backup_stop() to
> > > separate files via psql.
>
> Let's not confuse ourselves here- the existing script *doesn't* work in
> any reasonable way when we're talking about everything that needs to be
> done to perform a backup. That a lot of people are using it because
> it's in the documentation is an actively bad thing.
>
> The same goes for the archive command example.
>
> > Something like this might work:
> >
> > SELECT * FROM pg_backup_stop(true) \gset
> >
> > \out /tmp/backup_label \qecho :labelfile
> > \out /tmp/tablespace_map \qecho :spcmapfile
> > \out
> > \! ... tar command adding /tmp/{backup_label,tablespace_map} to the tarball
>
> ... this doesn't do what's needed either. We could try to write down
> some minimum set of things that are needed for a backup tool to do but
> it's not something that a 3 line script is going to cover. Indeed, it's
> a lot more like pg_basebackup and if we want to give folks a script to
> use, it should be "run pg_basebackup".
>
> > I notice the \qecho adds a final newline (and so if :spcmapfile is empty,
> > a file containing a single newline is made). In a quick test with a bogus
> > restore_command, I did not see any error messages specific to the format
> > of the backup_label or tablespace_map files, so maybe the final newline
> > isn't a problem.
> >
> > Assuming the newline isn't a problem, that might be simple enough to
> > use in an example, and maybe it's not a bad thing that it highlights a few
> > psql capabilities the reader might not have stumbled on before. Or, maybe
> > it is just too confusing to bother.
>
> It's more than just too confusing, it's actively bad because people will
> actually use it and then end up with backups that don't work.
+1.
Or even worse, backups that sometimes work, but not reliably and not every time.
> > While agreeing that pg_basebackup is the production-ready thing that
> > does it all for you (with tests for likely errors and so on), I think
> > there is also some value in a dead-simple example that concretely
> > shows you what "it" is, what the basic steps are that happen beneath
> > pg_basebackup's chrome.
I agree that having a dead simple script would be good.
The *only* dead simple script that's going to be possible is one that
calls pg_basebackup.
The current APIs don't make it *possible* to drive them directly with
a dead simple script.
Pretending something is simple when it's not, is not doing anybody a favor.
> Documenting everything that pg_basebackup does to make sure that the
> backup is viable might be something to work on if someone is really
> excited about this, but it's not 'dead-simple' and it's darn close to
> the bare minimum, something that none of these simple scripts will come
> anywhere close to being and instead they'll be far less than the
> minimum.
Yeah, having the full set of steps required documented certainly
wouldn't be a bad thing. But it's a very *different* thing.
> > If the added newline is a problem, I haven't thought of a way to exclude
> > it that doesn't take the example out of the realm of dead-simple.
>
> I disagree that there's really a way to provide 'dead-simple' backups
> with what's built into core without using pg_basebackup. If we want a
> 'dead-simple' solution in core then we'd need to write an appropriate
> backup tool that does all the basic things and include and maintain
> that. Writing a shell script isn't enough and we shouldn't encourage
> our users to do exactly that by having it in our documentation because
> then they'll think it's enough.
+1.
We need to accept that the current APIs are far too low level to be
driven by a shellscript. No matter how much documentation we write is
not going to change that fact.
For the people who want to drive their backups from a shellscript and
for some reason *don't* want to use pg_basebackup, we need to come up
with a different API or a different set of tools. That is not a
documentation task. That is a "start from a list of which things
pg_basebackup cannot do that are still simple, or that tools like
pgbackrest cannot do if they're complicated". And then design an API
that's actually safe and easy to use *for that usecase*.
For example, if the use case is "i want to use filesystemor SAN
snapshots for my backups", we shouldn't try to write workarounds using
bash coprocs or whatever. Instead, we could write a tool that
interacts with the current api to start the backup, then launches a
shellscript that interacts with the snapshot system, and then stops
the backup after. With a well defined set of rules for how that shell
script should work and interact.
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file
2022-03-09 01:24 Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Chapman Flack <[email protected]>
2022-03-09 15:42 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Stephen Frost <[email protected]>
2022-03-09 16:22 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Magnus Hagander <[email protected]>
@ 2022-03-09 17:12 ` Chapman Flack <[email protected]>
2022-03-09 17:19 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Stephen Frost <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Chapman Flack @ 2022-03-09 17:12 UTC (permalink / raw)
To: Magnus Hagander <[email protected]>; Stephen Frost <[email protected]>; +Cc: Nathan Bossart <[email protected]>; David Steele <[email protected]>; [email protected]
On 03/09/22 11:22, Magnus Hagander wrote:
>> It's more than just too confusing, it's actively bad because people will
>> actually use it and then end up with backups that don't work.
>
> +1.
>
> Or even worse, backups that sometimes work, but not reliably and not
> every time.
> ...
> Pretending something is simple when it's not, is not doing anybody a favor.
Okay, I bow to this reasoning, for the purpose of this patch. Let's
just lose the example.
>> Documenting everything that pg_basebackup does to make sure that the
>> backup is viable might be something to work on if someone is really
>> excited about this, but it's not 'dead-simple' and it's darn close to
>> the bare minimum, something that none of these simple scripts will come
>> anywhere close to being and instead they'll be far less than the
>> minimum.
>
> Yeah, having the full set of steps required documented certainly
> wouldn't be a bad thing.
I'd say that qualifies as an understatement. While it certainly doesn't
have to be part of this patch, if the claim is that an admin who relies
on pg_basebackup is relying on essential things pg_basebackup does that
have not been enumerated in our documentation yet, I would argue they
should be.
> with a different API or a different set of tools. That is not a
> documentation task. That is a "start from a list of which things
> pg_basebackup cannot do that are still simple, or that tools like
> pgbackrest cannot do if they're complicated". And then design an API
> that's actually safe and easy to use *for that usecase*.
That might also be a good thing, but I don't see it as a substitute
for documenting the present reality of what the irreducibly essential
behaviors of pg_basebackup (or of third-party tools like pgbackrest)
are, and why they are so.
Regards,
-Chap
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file
2022-03-09 01:24 Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Chapman Flack <[email protected]>
2022-03-09 15:42 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Stephen Frost <[email protected]>
2022-03-09 16:22 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Magnus Hagander <[email protected]>
2022-03-09 17:12 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Chapman Flack <[email protected]>
@ 2022-03-09 17:19 ` Stephen Frost <[email protected]>
2022-03-09 19:32 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Chapman Flack <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Stephen Frost @ 2022-03-09 17:19 UTC (permalink / raw)
To: Chapman Flack <[email protected]>; +Cc: Magnus Hagander <[email protected]>; Nathan Bossart <[email protected]>; David Steele <[email protected]>; [email protected]
Greetings,
* Chapman Flack ([email protected]) wrote:
> On 03/09/22 11:22, Magnus Hagander wrote:
> >> It's more than just too confusing, it's actively bad because people will
> >> actually use it and then end up with backups that don't work.
> >
> > +1.
> >
> > Or even worse, backups that sometimes work, but not reliably and not
> > every time.
> > ...
> > Pretending something is simple when it's not, is not doing anybody a favor.
>
> Okay, I bow to this reasoning, for the purpose of this patch. Let's
> just lose the example.
Great.
> >> Documenting everything that pg_basebackup does to make sure that the
> >> backup is viable might be something to work on if someone is really
> >> excited about this, but it's not 'dead-simple' and it's darn close to
> >> the bare minimum, something that none of these simple scripts will come
> >> anywhere close to being and instead they'll be far less than the
> >> minimum.
> >
> > Yeah, having the full set of steps required documented certainly
> > wouldn't be a bad thing.
>
> I'd say that qualifies as an understatement. While it certainly doesn't
> have to be part of this patch, if the claim is that an admin who relies
> on pg_basebackup is relying on essential things pg_basebackup does that
> have not been enumerated in our documentation yet, I would argue they
> should be.
It doesn't have to be part of this patch and we should move forward with
this patch. Let's avoid hijacking this thread, which is about this
patch, for an independent debate about what our documentation should or
shouldn't include.
> > with a different API or a different set of tools. That is not a
> > documentation task. That is a "start from a list of which things
> > pg_basebackup cannot do that are still simple, or that tools like
> > pgbackrest cannot do if they're complicated". And then design an API
> > that's actually safe and easy to use *for that usecase*.
>
> That might also be a good thing, but I don't see it as a substitute
> for documenting the present reality of what the irreducibly essential
> behaviors of pg_basebackup (or of third-party tools like pgbackrest)
> are, and why they are so.
I disagree. If we provided a tool then we'd document that tool and how
users can use it, not every single step that it does (see also:
pg_basebackup).
Thanks,
Stephen
Attachments:
[application/pgp-signature] signature.asc (819B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file
2022-03-09 01:24 Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Chapman Flack <[email protected]>
2022-03-09 15:42 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Stephen Frost <[email protected]>
2022-03-09 16:22 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Magnus Hagander <[email protected]>
2022-03-09 17:12 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Chapman Flack <[email protected]>
2022-03-09 17:19 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Stephen Frost <[email protected]>
@ 2022-03-09 19:32 ` Chapman Flack <[email protected]>
2022-03-09 22:21 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Nathan Bossart <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Chapman Flack @ 2022-03-09 19:32 UTC (permalink / raw)
To: Stephen Frost <[email protected]>; +Cc: Magnus Hagander <[email protected]>; Nathan Bossart <[email protected]>; David Steele <[email protected]>; [email protected]
On 03/09/22 12:19, Stephen Frost wrote:
> Let's avoid hijacking this thread, which is about this
> patch, for an independent debate about what our documentation should or
> shouldn't include.
Agreed. New thread here:
https://www.postgresql.org/message-id/6228FFE4.3050309%40anastigmatix.net
Regards,
-Chap
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file
2022-03-09 01:24 Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Chapman Flack <[email protected]>
2022-03-09 15:42 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Stephen Frost <[email protected]>
2022-03-09 16:22 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Magnus Hagander <[email protected]>
2022-03-09 17:12 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Chapman Flack <[email protected]>
2022-03-09 17:19 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Stephen Frost <[email protected]>
2022-03-09 19:32 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Chapman Flack <[email protected]>
@ 2022-03-09 22:21 ` Nathan Bossart <[email protected]>
2022-03-09 23:11 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Chapman Flack <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Nathan Bossart @ 2022-03-09 22:21 UTC (permalink / raw)
To: Chapman Flack <[email protected]>; +Cc: Stephen Frost <[email protected]>; Magnus Hagander <[email protected]>; David Steele <[email protected]>; [email protected]
On Wed, Mar 09, 2022 at 02:32:24PM -0500, Chapman Flack wrote:
> On 03/09/22 12:19, Stephen Frost wrote:
>> Let's avoid hijacking this thread, which is about this
>> patch, for an independent debate about what our documentation should or
>> shouldn't include.
>
> Agreed. New thread here:
>
> https://www.postgresql.org/message-id/6228FFE4.3050309%40anastigmatix.net
Great. Is there any additional feedback on this patch? Should we add an
example of using pg_basebackup in the "Standalone Hot Backups" section, or
should we leave all documentation additions like this for Chap's new
thread?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file
2022-03-09 01:24 Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Chapman Flack <[email protected]>
2022-03-09 15:42 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Stephen Frost <[email protected]>
2022-03-09 16:22 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Magnus Hagander <[email protected]>
2022-03-09 17:12 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Chapman Flack <[email protected]>
2022-03-09 17:19 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Stephen Frost <[email protected]>
2022-03-09 19:32 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Chapman Flack <[email protected]>
2022-03-09 22:21 ` Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Nathan Bossart <[email protected]>
@ 2022-03-09 23:11 ` Chapman Flack <[email protected]>
0 siblings, 0 replies; 9+ messages in thread
From: Chapman Flack @ 2022-03-09 23:11 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Stephen Frost <[email protected]>; Magnus Hagander <[email protected]>; David Steele <[email protected]>; [email protected]
On 03/09/22 17:21, Nathan Bossart wrote:
> Great. Is there any additional feedback on this patch? Should we add an
> example of using pg_basebackup in the "Standalone Hot Backups" section, or
> should we leave all documentation additions like this for Chap's new
> thread?
I'm composing something longer for the new thread, but on the way
I noticed something we might fit into this one.
I think the listitem
In the same connection as before, issue the command:
SELECT * FROM pg_backup_stop(true);
would be clearer if it used named-parameter form, wait_for_archive => true.
This is not strictly necessary, of course, for a function with a single
IN parameter, but it's good documentation (and also could save us headaches
like these if there is ever another future need to give it more parameters).
That listitem doesn't say anything about what the parameter means, which
is a little weird, but probably ok because the next listitem does go into
it in some detail. I don't think a larger reorg is needed to bring that
language one listitem earlier. Just naming the parameter is probably
enough to make it less puzzling (or adding in that listitem, at most,
"the effect of the wait_for_archive parameter is explained below").
For consistency (and the same futureproofing benefit), I'd go to
fast => false in the earlier pg_backup_start as well.
I'm more ambivalent about label => 'label'. It would be consistent,
but should we just agree for conciseness that there will always be
a label and it will always be first?
You can pretty much tell in a call what's a label; it's those anonymous
trues and falses that are easier to read with named notation.
Regards,
-Chap
^ permalink raw reply [nested|flat] 9+ messages in thread
* [PATCH] Test to demonstrate bug in commit 0d3dba38c7 and to verify a fix.
@ 2026-05-12 10:27 Antonin Houska <[email protected]>
0 siblings, 0 replies; 9+ messages in thread
From: Antonin Houska @ 2026-05-12 10:27 UTC (permalink / raw)
The fix: https://www.postgresql.org/message-id/77611.1778055944%40localhost
---
src/backend/access/transam/xact.c | 2 +
src/backend/commands/repack_worker.c | 2 +
src/test/isolation/isolationtester.c | 9 +-
.../expected/repack_running_xacts.out | 81 ++++++++++++
.../specs/repack_running_xacts.spec | 119 ++++++++++++++++++
5 files changed, 212 insertions(+), 1 deletion(-)
create mode 100644 src/test/modules/injection_points/expected/repack_running_xacts.out
create mode 100644 src/test/modules/injection_points/specs/repack_running_xacts.spec
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 5586fbe5b07..b63ee166028 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -65,6 +65,7 @@
#include "utils/builtins.h"
#include "utils/combocid.h"
#include "utils/guc.h"
+#include "utils/injection_point.h"
#include "utils/inval.h"
#include "utils/memutils.h"
#include "utils/relmapper.h"
@@ -2428,6 +2429,7 @@ CommitTransaction(void)
* must be done _before_ releasing locks we hold and _after_
* RecordTransactionCommit.
*/
+ INJECTION_POINT("before-end-transaction", NULL);
ProcArrayEndTransaction(MyProc, latestXid);
/*
diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c
index c40f8c98e06..6835626d677 100644
--- a/src/backend/commands/repack_worker.c
+++ b/src/backend/commands/repack_worker.c
@@ -26,6 +26,7 @@
#include "storage/ipc.h"
#include "storage/proc.h"
#include "tcop/tcopprot.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#define REPL_PLUGIN_NAME "pgrepack"
@@ -233,6 +234,7 @@ repack_setup_logical_decoding(Oid relid)
* Neither prepare_write nor do_write callback nor update_progress is
* useful for us.
*/
+ INJECTION_POINT("before-create-decoding-context", NULL);
ctx = CreateInitDecodingContext(REPL_PLUGIN_NAME,
NIL,
true,
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index 440c875b8ac..8f17ee412c9 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -216,15 +216,22 @@ main(int argc, char **argv)
* exactly expect concurrent use of test tables. However, autovacuum will
* occasionally take AccessExclusiveLock to truncate a table, and we must
* ignore that transient wait.
+ *
+ * If the session's backend is blocked, and if its background worker is
+ * waiting on an injection point, we assume that the injection point is
+ * the reason for the backend to be blocked. That's what we check in the
+ * second query of the UNION. XXX Should we use a separate query for that?
*/
initPQExpBuffer(&wait_query);
appendPQExpBufferStr(&wait_query,
+ "WITH blocking(res) AS ("
"SELECT pg_catalog.pg_isolation_test_session_is_blocked($1, '{");
/* The spec syntax requires at least one session; assume that here. */
appendPQExpBufferStr(&wait_query, conns[1].backend_pid_str);
for (i = 2; i < nconns; i++)
appendPQExpBuffer(&wait_query, ",%s", conns[i].backend_pid_str);
- appendPQExpBufferStr(&wait_query, "}')");
+ appendPQExpBufferStr(&wait_query, "}') UNION "
+ "SELECT pg_catalog.pg_isolation_test_session_is_blocked(pid, '{}') FROM pg_stat_activity WHERE leader_pid=$1) SELECT bool_or(res) FROM blocking");
res = PQprepare(conns[0].conn, PREP_WAITING, wait_query.data, 0, NULL);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
diff --git a/src/test/modules/injection_points/expected/repack_running_xacts.out b/src/test/modules/injection_points/expected/repack_running_xacts.out
new file mode 100644
index 00000000000..271fe2b97cb
--- /dev/null
+++ b/src/test/modules/injection_points/expected/repack_running_xacts.out
@@ -0,0 +1,81 @@
+Parsed test spec with 5 sessions
+
+starting permutation: repack s3_assign_xid wakeup_bcdc s4_changes s4_attach s4_commit s3_commit s5_assign_xid wakeup_bet s5_commit check
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step repack:
+ REPACK (CONCURRENTLY) repack_test;
+ <waiting ...>
+step s3_assign_xid:
+ BEGIN;
+ INSERT INTO aux VALUES (1);
+
+step wakeup_bcdc:
+ SELECT injection_points_wakeup('before-create-decoding-context');
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s4_changes:
+ BEGIN;
+ INSERT INTO repack_test(i, j) VALUES (1, 1);
+
+step s4_attach:
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('before-end-transaction', 'wait');
+
+injection_points_set_local
+--------------------------
+
+(1 row)
+
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step s4_commit:
+ COMMIT;
+ <waiting ...>
+step s3_commit:
+ COMMIT;
+
+step s5_assign_xid:
+ BEGIN;
+ INSERT INTO aux VALUES (2);
+
+step wakeup_bet:
+ SELECT injection_points_wakeup('before-end-transaction');
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step repack: <... completed>
+step s4_commit: <... completed>
+step s5_commit:
+ COMMIT;
+
+step check:
+ TABLE repack_test;
+
+i|j
+-+-
+(0 rows)
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
diff --git a/src/test/modules/injection_points/specs/repack_running_xacts.spec b/src/test/modules/injection_points/specs/repack_running_xacts.spec
new file mode 100644
index 00000000000..1f878514046
--- /dev/null
+++ b/src/test/modules/injection_points/specs/repack_running_xacts.spec
@@ -0,0 +1,119 @@
+setup
+{
+ CREATE EXTENSION injection_points;
+ CREATE TABLE repack_test(i int PRIMARY KEY, j int);
+ CREATE TABLE aux(i int);
+}
+
+teardown
+{
+ DROP TABLE repack_test;
+ DROP TABLE aux;
+ DROP EXTENSION injection_points;
+}
+
+session s1
+setup
+{
+ SELECT injection_points_attach('before-create-decoding-context', 'wait');
+}
+step repack
+{
+ REPACK (CONCURRENTLY) repack_test;
+}
+step check
+{
+ TABLE repack_test;
+}
+teardown
+{
+ SELECT injection_points_detach('before-create-decoding-context');
+}
+
+session s2
+step wakeup_bcdc
+{
+ SELECT injection_points_wakeup('before-create-decoding-context');
+}
+step wakeup_bet
+{
+ SELECT injection_points_wakeup('before-end-transaction');
+}
+
+session s3
+step s3_assign_xid
+{
+ BEGIN;
+ INSERT INTO aux VALUES (1);
+}
+step s3_commit
+{
+ COMMIT;
+}
+
+session s4
+step s4_changes
+{
+ BEGIN;
+ INSERT INTO repack_test(i, j) VALUES (1, 1);
+}
+# Do not attach in the setup section, that would be too soon.
+step s4_attach
+{
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('before-end-transaction', 'wait');
+}
+step s4_commit
+{
+ COMMIT;
+}
+teardown
+{
+ SELECT injection_points_detach('before-end-transaction');
+}
+
+session s5
+step s5_assign_xid
+{
+ BEGIN;
+ INSERT INTO aux VALUES (2);
+}
+step s5_commit
+{
+ COMMIT;
+}
+
+permutation
+repack
+# Assign XID so that a running transaction prevents the snapshot builder from
+# reaching CONSISTENT state immediately. It will wait for this to complete
+# after having reached BUILDING_SNAPSHOT.
+s3_assign_xid
+# Let the decoding setup start.
+wakeup_bcdc
+# Likewise, the snapshot builder will wait for the s4's xact to complete after
+# having reached FULL_SNAPSHOT. This is the problematic transaction, so let it
+# do some changes.
+s4_changes
+# Attach to the 'before-end-transaction' injection point that s4 will need
+# during commit.
+s4_attach
+# Only write commit record for s4, but do not remove the xact from procarray
+# yet. Thus the snapshot builder still needs to wait.
+s4_commit
+# Let the snapshot builder proceed to FULL_SNAPSHOT.
+s3_commit
+# Start another transaction so that CONSISTENT is not reached "directly",
+# i.e. due to no running transaction. It's important here that builder->xmin
+# does not advance.
+s5_assign_xid
+# Remove s4 xact from procarray, and thus reach the CONSISTENT state. Since
+# the COMMIT appeared in WAL too early (i.e. when the snapshot builder state
+# did not allow decoding of COMMIT records yet), the snapshot builder will
+# consider s4 running. This is also due to returning from
+# SnapBuildProcessRunningXacts() too early, w/o advancing builder->xmin.
+wakeup_bet
+# s5 is not needed anymore
+s5_commit
+# Show that the data changes performed by s4 are lost.
+check
--
2.47.3
--=-=-=--
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2026-05-12 10:27 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-03-09 01:24 Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Chapman Flack <[email protected]>
2022-03-09 15:42 ` Stephen Frost <[email protected]>
2022-03-09 16:22 ` Magnus Hagander <[email protected]>
2022-03-09 17:12 ` Chapman Flack <[email protected]>
2022-03-09 17:19 ` Stephen Frost <[email protected]>
2022-03-09 19:32 ` Chapman Flack <[email protected]>
2022-03-09 22:21 ` Nathan Bossart <[email protected]>
2022-03-09 23:11 ` Chapman Flack <[email protected]>
2026-05-12 10:27 [PATCH] Test to demonstrate bug in commit 0d3dba38c7 and to verify a fix. Antonin Houska <[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