public inbox for [email protected]
help / color / mirror / Atom feedRe: Weird failure with latches in curculio on v15
15+ messages / 4 participants
[nested] [flat]
* Re: Weird failure with latches in curculio on v15
@ 2023-02-05 00:49 Michael Paquier <[email protected]>
2023-02-05 22:19 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-05 23:36 ` Re: Weird failure with latches in curculio on v15 Michael Paquier <[email protected]>
0 siblings, 2 replies; 15+ messages in thread
From: Michael Paquier @ 2023-02-05 00:49 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Andres Freund <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]>
On Sat, Feb 04, 2023 at 10:03:54AM -0800, Nathan Bossart wrote:
> Okay. Michael, why don't we revert the shell_restore stuff for now? Once
> the archive modules interface changes and the fix for this
> SIGTERM-during-system() problem are in, I will work through this feedback
> and give recovery modules another try. I'm still hoping to have recovery
> modules ready in time for the v16 feature freeze.
Yes, at this stage a revert of the refactoring with shell_restore.c is
the best path forward.
From the discussion, I got the following things on top of my mind, for
reference:
- Should we include archive_cleanup_command into the recovery modules
at all? We've discussed offloading that from the checkpointer, and it
makes the failure handling trickier when it comes to unexpected GUC
configurations, for one. The same may actually apply to
restore_end_command. Though it is done in the startup process now,
there may be an argument to offload that somewhere else based on the
timing of the end-of-recovery checkpoint. My opinion on this stuff is
that only including restore_command in the modules would make most
users I know of happy enough as it removes the overhead of the command
invocation from the startup process, if able to replay things fast
enough so as the restore command is the bottleneck.
restore_end_command would be simple enough, but if there is a wish to
redesign the startup process to offload it somewhere else, then the
recovery module makes backward-compatibility concerns harder to think
about in the long-term.
- Do we need to reconsider the assumptions of the startup
process where SIGTERM enforces an immediate shutdown while running
system() for the restore command? For example, the difference of
behavior when a restore_command uses a system sleep() that does not
react on signals from what I recall?
- Fixing the original issue of this thread may finish by impacting
what you are trying to do in this area, so fixing the original issue
first sounds like a pre-requirement to me at the end because it may
impact the final design of the modules and their callbacks. (I have
not looked at all the arguments raised about what to do with ~15,
still it does not look like we have a clear picture here yet.)
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Weird failure with latches in curculio on v15
2023-02-05 00:49 Re: Weird failure with latches in curculio on v15 Michael Paquier <[email protected]>
@ 2023-02-05 22:19 ` Nathan Bossart <[email protected]>
2023-02-05 23:01 ` Re: Weird failure with latches in curculio on v15 Andres Freund <[email protected]>
1 sibling, 1 reply; 15+ messages in thread
From: Nathan Bossart @ 2023-02-05 22:19 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]>
On Sun, Feb 05, 2023 at 09:49:57AM +0900, Michael Paquier wrote:
> - Should we include archive_cleanup_command into the recovery modules
> at all? We've discussed offloading that from the checkpointer, and it
> makes the failure handling trickier when it comes to unexpected GUC
> configurations, for one. The same may actually apply to
> restore_end_command. Though it is done in the startup process now,
> there may be an argument to offload that somewhere else based on the
> timing of the end-of-recovery checkpoint. My opinion on this stuff is
> that only including restore_command in the modules would make most
> users I know of happy enough as it removes the overhead of the command
> invocation from the startup process, if able to replay things fast
> enough so as the restore command is the bottleneck.
> restore_end_command would be simple enough, but if there is a wish to
> redesign the startup process to offload it somewhere else, then the
> recovery module makes backward-compatibility concerns harder to think
> about in the long-term.
I agree. I think we ought to first focus on getting the recovery modules
interface and restore_command functionality in place before we take on more
difficult things like archive_cleanup_command. But I still think the
archive_cleanup_command/recovery_end_command functionality should
eventually be added to recovery modules.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Weird failure with latches in curculio on v15
2023-02-05 00:49 Re: Weird failure with latches in curculio on v15 Michael Paquier <[email protected]>
2023-02-05 22:19 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
@ 2023-02-05 23:01 ` Andres Freund <[email protected]>
2023-02-05 23:57 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Andres Freund @ 2023-02-05 23:01 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Michael Paquier <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]>
Hi,
On 2023-02-05 14:19:38 -0800, Nathan Bossart wrote:
> On Sun, Feb 05, 2023 at 09:49:57AM +0900, Michael Paquier wrote:
> > - Should we include archive_cleanup_command into the recovery modules
> > at all? We've discussed offloading that from the checkpointer, and it
> > makes the failure handling trickier when it comes to unexpected GUC
> > configurations, for one. The same may actually apply to
> > restore_end_command. Though it is done in the startup process now,
> > there may be an argument to offload that somewhere else based on the
> > timing of the end-of-recovery checkpoint. My opinion on this stuff is
> > that only including restore_command in the modules would make most
> > users I know of happy enough as it removes the overhead of the command
> > invocation from the startup process, if able to replay things fast
> > enough so as the restore command is the bottleneck.
> > restore_end_command would be simple enough, but if there is a wish to
> > redesign the startup process to offload it somewhere else, then the
> > recovery module makes backward-compatibility concerns harder to think
> > about in the long-term.
>
> I agree. I think we ought to first focus on getting the recovery modules
> interface and restore_command functionality in place before we take on more
> difficult things like archive_cleanup_command. But I still think the
> archive_cleanup_command/recovery_end_command functionality should
> eventually be added to recovery modules.
I tend not to agree. If you make the API that small, you're IME likely
to end up with something that looks somewhat incoherent once extended.
The more I think about it, the less I am convinced that
one-callback-per-segment, invoked just before needing the file, is the
right approach to address the performance issues of restore_commmand.
The main performance issue isn't the shell invocation overhead, it's
synchronously needing to restore the archive, before replay can
continue. It's also gonna be slow if a restore module copies the segment
from a remote system - the latency is the problem.
The only way the restore module approach can do better, is to
asynchronously restore ahead of the current segment. But for that the
API really isn't suited well. The signature of the relevant callback is:
> +typedef bool (*RecoveryRestoreCB) (const char *file, const char *path,
> + const char *lastRestartPointFileName);
That's not very suited to restoring "ahead of time". You need to parse
file to figure out whether a segment or something else is restored, turn
"file" back into an LSN, figure out where to to store further segments,
somehow hand off to some background worker, etc.
That doesn't strike me as something we want to happen inside multiple
restore libraries.
I think at the very least you'd want to have a separate callback for
restoring segments than for restoring other files. But more likely a
separate callback for each type of file to be restored.
For the timeline history case an parameter indicating that we don't want
to restore the file, just to see if there's a conflict, would make
sense.
For the segment files, we'd likely need a parameter to indicate whether
the restore is random or not.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Weird failure with latches in curculio on v15
2023-02-05 00:49 Re: Weird failure with latches in curculio on v15 Michael Paquier <[email protected]>
2023-02-05 22:19 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-05 23:01 ` Re: Weird failure with latches in curculio on v15 Andres Freund <[email protected]>
@ 2023-02-05 23:57 ` Nathan Bossart <[email protected]>
2023-02-06 00:07 ` Re: Weird failure with latches in curculio on v15 Andres Freund <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Nathan Bossart @ 2023-02-05 23:57 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]>
On Sun, Feb 05, 2023 at 03:01:57PM -0800, Andres Freund wrote:
> I think at the very least you'd want to have a separate callback for
> restoring segments than for restoring other files. But more likely a
> separate callback for each type of file to be restored.
>
> For the timeline history case an parameter indicating that we don't want
> to restore the file, just to see if there's a conflict, would make
> sense.
That seems reasonable.
> For the segment files, we'd likely need a parameter to indicate whether
> the restore is random or not.
Wouldn't this approach still require each module to handle restoring ahead
of time? I agree that the shell overhead isn't the main performance issue,
but it's unclear to me how much of this should be baked into PostgreSQL. I
mean, we could introduce a GUC that tells us how far ahead to restore and
have a background worker (or multiple background workers) asynchronously
pull files into a staging directory via the callbacks. Is that the sort of
scope you are envisioning?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Weird failure with latches in curculio on v15
2023-02-05 00:49 Re: Weird failure with latches in curculio on v15 Michael Paquier <[email protected]>
2023-02-05 22:19 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-05 23:01 ` Re: Weird failure with latches in curculio on v15 Andres Freund <[email protected]>
2023-02-05 23:57 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
@ 2023-02-06 00:07 ` Andres Freund <[email protected]>
2023-02-06 00:46 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Andres Freund @ 2023-02-06 00:07 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Michael Paquier <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]>
Hi,
On 2023-02-05 15:57:47 -0800, Nathan Bossart wrote:
> > For the segment files, we'd likely need a parameter to indicate whether
> > the restore is random or not.
>
> Wouldn't this approach still require each module to handle restoring ahead
> of time?
Yes, to some degree at least. I was just describing a few pretty obvious
improvements.
The core code can make that a lot easier though. The problem of where to
store such files can be provided by core code (presumably a separate
directory). A GUC for aggressiveness can be provided. Etc.
> I agree that the shell overhead isn't the main performance issue,
> but it's unclear to me how much of this should be baked into
> PostgreSQL.
I don't know fully either. But just reimplementing all of it in
different modules doesn't seem like a sane approach either. A lot of it
is policy that we need to solve once, centrally.
> I mean, we could introduce a GUC that tells us how far ahead to
> restore and have a background worker (or multiple background workers)
> asynchronously pull files into a staging directory via the callbacks.
> Is that the sort of scope you are envisioning?
Closer, at least.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Weird failure with latches in curculio on v15
2023-02-05 00:49 Re: Weird failure with latches in curculio on v15 Michael Paquier <[email protected]>
2023-02-05 22:19 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-05 23:01 ` Re: Weird failure with latches in curculio on v15 Andres Freund <[email protected]>
2023-02-05 23:57 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-06 00:07 ` Re: Weird failure with latches in curculio on v15 Andres Freund <[email protected]>
@ 2023-02-06 00:46 ` Nathan Bossart <[email protected]>
2023-02-08 15:22 ` Re: Weird failure with latches in curculio on v15 Robert Haas <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Nathan Bossart @ 2023-02-06 00:46 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]>
On Sun, Feb 05, 2023 at 04:07:50PM -0800, Andres Freund wrote:
> On 2023-02-05 15:57:47 -0800, Nathan Bossart wrote:
>> I agree that the shell overhead isn't the main performance issue,
>> but it's unclear to me how much of this should be baked into
>> PostgreSQL.
>
> I don't know fully either. But just reimplementing all of it in
> different modules doesn't seem like a sane approach either. A lot of it
> is policy that we need to solve once, centrally.
>
>> I mean, we could introduce a GUC that tells us how far ahead to
>> restore and have a background worker (or multiple background workers)
>> asynchronously pull files into a staging directory via the callbacks.
>> Is that the sort of scope you are envisioning?
>
> Closer, at least.
Got it. I suspect we'll want to do something similar for archive modules
eventually, too.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Weird failure with latches in curculio on v15
2023-02-05 00:49 Re: Weird failure with latches in curculio on v15 Michael Paquier <[email protected]>
2023-02-05 22:19 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-05 23:01 ` Re: Weird failure with latches in curculio on v15 Andres Freund <[email protected]>
2023-02-05 23:57 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-06 00:07 ` Re: Weird failure with latches in curculio on v15 Andres Freund <[email protected]>
2023-02-06 00:46 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
@ 2023-02-08 15:22 ` Robert Haas <[email protected]>
2023-02-08 17:43 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Robert Haas @ 2023-02-08 15:22 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]>
On Sun, Feb 5, 2023 at 7:46 PM Nathan Bossart <[email protected]> wrote:
> Got it. I suspect we'll want to do something similar for archive modules
> eventually, too.
+1.
I felt like the archive modules work was a step forward when we did,
because basic_archive does some things that you're not likely to get
right if you do it on your own. And a similar approach to
restore_command might be also be valuable, at least in my opinion.
However, the gains that we can get out of the archive module facility
in its present form do seem to be somewhat limited, for exactly the
kinds of reasons being discussed here.
I kind of wonder whether we ought to try to flip the model around. At
present, the idea is that the archiver is doing its thing and it makes
callbacks into the archive module. But what if we got rid of the
archiver main loop altogether and put the main loop inside of the
archive module, and have it call back to some functions that we
provide? One function could be like char
*pgarch_next_file_to_be_archived_if_there_is_one_ready(void) and the
other could be like void
pgarch_some_file_that_you_gave_me_previously_is_now_fully_archived(char
*which_one). That way, we'd break the tight coupling where you have to
get a unit of work and perform it in full before you can get the next
unit of work. Some variant of this could work on the restore side,
too, I think, although we have less certainty about how much it makes
to prefetch for restore than we do about what needs to be archived.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Weird failure with latches in curculio on v15
2023-02-05 00:49 Re: Weird failure with latches in curculio on v15 Michael Paquier <[email protected]>
2023-02-05 22:19 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-05 23:01 ` Re: Weird failure with latches in curculio on v15 Andres Freund <[email protected]>
2023-02-05 23:57 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-06 00:07 ` Re: Weird failure with latches in curculio on v15 Andres Freund <[email protected]>
2023-02-06 00:46 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-08 15:22 ` Re: Weird failure with latches in curculio on v15 Robert Haas <[email protected]>
@ 2023-02-08 17:43 ` Nathan Bossart <[email protected]>
2023-02-08 21:24 ` Re: Weird failure with latches in curculio on v15 Robert Haas <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Nathan Bossart @ 2023-02-08 17:43 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]>
On Wed, Feb 08, 2023 at 10:22:24AM -0500, Robert Haas wrote:
> I felt like the archive modules work was a step forward when we did,
> because basic_archive does some things that you're not likely to get
> right if you do it on your own. And a similar approach to
> restore_command might be also be valuable, at least in my opinion.
> However, the gains that we can get out of the archive module facility
> in its present form do seem to be somewhat limited, for exactly the
> kinds of reasons being discussed here.
I'm glad to hear that there is interest in taking this stuff to the next
level. I'm currently planning to first get the basic API in place for
recovery modules like we have for archive modules, but I'm hoping to
position it so that it leads naturally to asynchronous, parallel, and/or
batching approaches down the road (v17?).
> I kind of wonder whether we ought to try to flip the model around. At
> present, the idea is that the archiver is doing its thing and it makes
> callbacks into the archive module. But what if we got rid of the
> archiver main loop altogether and put the main loop inside of the
> archive module, and have it call back to some functions that we
> provide? One function could be like char
> *pgarch_next_file_to_be_archived_if_there_is_one_ready(void) and the
> other could be like void
> pgarch_some_file_that_you_gave_me_previously_is_now_fully_archived(char
> *which_one). That way, we'd break the tight coupling where you have to
> get a unit of work and perform it in full before you can get the next
> unit of work. Some variant of this could work on the restore side,
> too, I think, although we have less certainty about how much it makes
> to prefetch for restore than we do about what needs to be archived.
I think this could be a good approach if we decide not to bake too much
into PostgreSQL itself (e.g., such as creating multiple archive workers
that each call out to the module). Archive module authors would
effectively need to write their own archiver processes. That sounds super
flexible, but it also sounds like it might be harder to get right.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Weird failure with latches in curculio on v15
2023-02-05 00:49 Re: Weird failure with latches in curculio on v15 Michael Paquier <[email protected]>
2023-02-05 22:19 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-05 23:01 ` Re: Weird failure with latches in curculio on v15 Andres Freund <[email protected]>
2023-02-05 23:57 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-06 00:07 ` Re: Weird failure with latches in curculio on v15 Andres Freund <[email protected]>
2023-02-06 00:46 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-08 15:22 ` Re: Weird failure with latches in curculio on v15 Robert Haas <[email protected]>
2023-02-08 17:43 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
@ 2023-02-08 21:24 ` Robert Haas <[email protected]>
2023-02-08 22:25 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Robert Haas @ 2023-02-08 21:24 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]>
On Wed, Feb 8, 2023 at 12:43 PM Nathan Bossart <[email protected]> wrote:
> I think this could be a good approach if we decide not to bake too much
> into PostgreSQL itself (e.g., such as creating multiple archive workers
> that each call out to the module). Archive module authors would
> effectively need to write their own archiver processes. That sounds super
> flexible, but it also sounds like it might be harder to get right.
Yep. That's a problem, and I'm certainly open to better ideas.
However, if we assume that the archive module is likely to be doing
something like juggling a bunch of file descriptors over which it is
speaking HTTP, what other model works, really? It might be juggling
those file descriptors indirectly, or it might be relying on an
intermediate library like curl or something from Amazon that talks to
S3 or whatever, but only it knows what resources it's juggling, or
what functions it needs to call to manage them. On the other hand, we
don't really need a lot from it. We need it to CHECK_FOR_INTERRUPTS()
and handle that without leaking resources or breaking the world in
some way, and we sort of need it to, you know, actually archive stuff,
but apart from that I guess it can do what it likes (unless I'm
missing some other important function of the archiver?).
It's probably a good idea if the archiver function returns when it's
fully caught up and there's no more work to do. Then we could handle
decisions about hibernation in the core code, rather than having every
archive module invent its own way of doing that. But when there's work
happening, as far as I can see, the archive module needs to have
control pretty nearly all the time, or it's not going to be able to do
anything clever.
Always happy to hear if you see it differently....
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Weird failure with latches in curculio on v15
2023-02-05 00:49 Re: Weird failure with latches in curculio on v15 Michael Paquier <[email protected]>
2023-02-05 22:19 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-05 23:01 ` Re: Weird failure with latches in curculio on v15 Andres Freund <[email protected]>
2023-02-05 23:57 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-06 00:07 ` Re: Weird failure with latches in curculio on v15 Andres Freund <[email protected]>
2023-02-06 00:46 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-08 15:22 ` Re: Weird failure with latches in curculio on v15 Robert Haas <[email protected]>
2023-02-08 17:43 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-08 21:24 ` Re: Weird failure with latches in curculio on v15 Robert Haas <[email protected]>
@ 2023-02-08 22:25 ` Nathan Bossart <[email protected]>
2023-02-08 23:56 ` Re: Weird failure with latches in curculio on v15 Michael Paquier <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Nathan Bossart @ 2023-02-08 22:25 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]>
On Wed, Feb 08, 2023 at 04:24:15PM -0500, Robert Haas wrote:
> On Wed, Feb 8, 2023 at 12:43 PM Nathan Bossart <[email protected]> wrote:
>> I think this could be a good approach if we decide not to bake too much
>> into PostgreSQL itself (e.g., such as creating multiple archive workers
>> that each call out to the module). Archive module authors would
>> effectively need to write their own archiver processes. That sounds super
>> flexible, but it also sounds like it might be harder to get right.
>
> Yep. That's a problem, and I'm certainly open to better ideas.
>
> However, if we assume that the archive module is likely to be doing
> something like juggling a bunch of file descriptors over which it is
> speaking HTTP, what other model works, really? It might be juggling
> those file descriptors indirectly, or it might be relying on an
> intermediate library like curl or something from Amazon that talks to
> S3 or whatever, but only it knows what resources it's juggling, or
> what functions it needs to call to manage them. On the other hand, we
> don't really need a lot from it. We need it to CHECK_FOR_INTERRUPTS()
> and handle that without leaking resources or breaking the world in
> some way, and we sort of need it to, you know, actually archive stuff,
> but apart from that I guess it can do what it likes (unless I'm
> missing some other important function of the archiver?).
>
> It's probably a good idea if the archiver function returns when it's
> fully caught up and there's no more work to do. Then we could handle
> decisions about hibernation in the core code, rather than having every
> archive module invent its own way of doing that. But when there's work
> happening, as far as I can see, the archive module needs to have
> control pretty nearly all the time, or it's not going to be able to do
> anything clever.
>
> Always happy to hear if you see it differently....
These are all good points. Perhaps there could be a base archiver
implementation that shell_archive uses (and that other modules could use if
desired, which might be important for backward compatibility with the
existing callbacks). But if you want to do something fancier than
archiving sequentially, you could write your own.
In any case, I'm not really wedded to any particular approach at the
moment, so I am likewise open to better ideas.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Weird failure with latches in curculio on v15
2023-02-05 00:49 Re: Weird failure with latches in curculio on v15 Michael Paquier <[email protected]>
2023-02-05 22:19 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-05 23:01 ` Re: Weird failure with latches in curculio on v15 Andres Freund <[email protected]>
2023-02-05 23:57 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-06 00:07 ` Re: Weird failure with latches in curculio on v15 Andres Freund <[email protected]>
2023-02-06 00:46 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-08 15:22 ` Re: Weird failure with latches in curculio on v15 Robert Haas <[email protected]>
2023-02-08 17:43 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-08 21:24 ` Re: Weird failure with latches in curculio on v15 Robert Haas <[email protected]>
2023-02-08 22:25 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
@ 2023-02-08 23:56 ` Michael Paquier <[email protected]>
2023-02-09 00:24 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Michael Paquier @ 2023-02-08 23:56 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]>
On Wed, Feb 08, 2023 at 02:25:54PM -0800, Nathan Bossart wrote:
> These are all good points. Perhaps there could be a base archiver
> implementation that shell_archive uses (and that other modules could use if
> desired, which might be important for backward compatibility with the
> existing callbacks). But if you want to do something fancier than
> archiving sequentially, you could write your own.
Which is basically the kind of things you can already achieve with a
background worker and a module of your own?
> In any case, I'm not really wedded to any particular approach at the
> moment, so I am likewise open to better ideas.
I am not sure how much we should try to move from core into the
modules when it comes to the current archiver process, with how much
control you'd like to give to users. It also looks like to me that
this is the kind of problem where we would not have the correct
callback design until someone comes in and develops a solution that
would shape around it. On top of that, this is the kind of things
achievable with just a bgworker, and perhaps simpler as the parallel
state can just be maintained in it, which is what the archiver process
is about at the end? Or were there some restrictions in the bgworker
APIs that would not fit with what an archiver process should do?
Perhaps these restrictions, if any, are what we'd better try to lift
first?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Weird failure with latches in curculio on v15
2023-02-05 00:49 Re: Weird failure with latches in curculio on v15 Michael Paquier <[email protected]>
2023-02-05 22:19 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-05 23:01 ` Re: Weird failure with latches in curculio on v15 Andres Freund <[email protected]>
2023-02-05 23:57 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-06 00:07 ` Re: Weird failure with latches in curculio on v15 Andres Freund <[email protected]>
2023-02-06 00:46 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-08 15:22 ` Re: Weird failure with latches in curculio on v15 Robert Haas <[email protected]>
2023-02-08 17:43 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-08 21:24 ` Re: Weird failure with latches in curculio on v15 Robert Haas <[email protected]>
2023-02-08 22:25 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-08 23:56 ` Re: Weird failure with latches in curculio on v15 Michael Paquier <[email protected]>
@ 2023-02-09 00:24 ` Nathan Bossart <[email protected]>
2023-02-09 14:22 ` Re: Weird failure with latches in curculio on v15 Robert Haas <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Nathan Bossart @ 2023-02-09 00:24 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]>
On Thu, Feb 09, 2023 at 08:56:24AM +0900, Michael Paquier wrote:
> On Wed, Feb 08, 2023 at 02:25:54PM -0800, Nathan Bossart wrote:
>> These are all good points. Perhaps there could be a base archiver
>> implementation that shell_archive uses (and that other modules could use if
>> desired, which might be important for backward compatibility with the
>> existing callbacks). But if you want to do something fancier than
>> archiving sequentially, you could write your own.
>
> Which is basically the kind of things you can already achieve with a
> background worker and a module of your own?
IMO one of the big pieces that's missing is a way to get the next N files
to archive. Right now, you'd have to trawl through archive_status on your
own if you wanted to batch/parallelize. I think one advantage of what
Robert is suggesting is that we could easily provide a supported way to get
the next set of files to archive, and we can asynchronously mark them
"done". Otherwise, each module has to implement this.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Weird failure with latches in curculio on v15
2023-02-05 00:49 Re: Weird failure with latches in curculio on v15 Michael Paquier <[email protected]>
2023-02-05 22:19 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-05 23:01 ` Re: Weird failure with latches in curculio on v15 Andres Freund <[email protected]>
2023-02-05 23:57 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-06 00:07 ` Re: Weird failure with latches in curculio on v15 Andres Freund <[email protected]>
2023-02-06 00:46 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-08 15:22 ` Re: Weird failure with latches in curculio on v15 Robert Haas <[email protected]>
2023-02-08 17:43 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-08 21:24 ` Re: Weird failure with latches in curculio on v15 Robert Haas <[email protected]>
2023-02-08 22:25 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
2023-02-08 23:56 ` Re: Weird failure with latches in curculio on v15 Michael Paquier <[email protected]>
2023-02-09 00:24 ` Re: Weird failure with latches in curculio on v15 Nathan Bossart <[email protected]>
@ 2023-02-09 14:22 ` Robert Haas <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Robert Haas @ 2023-02-09 14:22 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]>
On Wed, Feb 8, 2023 at 7:24 PM Nathan Bossart <[email protected]> wrote:
> On Thu, Feb 09, 2023 at 08:56:24AM +0900, Michael Paquier wrote:
> > On Wed, Feb 08, 2023 at 02:25:54PM -0800, Nathan Bossart wrote:
> >> These are all good points. Perhaps there could be a base archiver
> >> implementation that shell_archive uses (and that other modules could use if
> >> desired, which might be important for backward compatibility with the
> >> existing callbacks). But if you want to do something fancier than
> >> archiving sequentially, you could write your own.
> >
> > Which is basically the kind of things you can already achieve with a
> > background worker and a module of your own?
>
> IMO one of the big pieces that's missing is a way to get the next N files
> to archive. Right now, you'd have to trawl through archive_status on your
> own if you wanted to batch/parallelize. I think one advantage of what
> Robert is suggesting is that we could easily provide a supported way to get
> the next set of files to archive, and we can asynchronously mark them
> "done". Otherwise, each module has to implement this.
Right.
I think that we could certainly, as Michael suggests, have people
provide their own background worker rather than having the archiver
invoke the user-supplied code directly. As long as the functions that
you need in order to get the necessary information can be called from
some other process, that's fine. The only difficulty I see is that if
the archiving is happening from a separate background worker rather
than from the archiver, then what is the archiver doing? We could
somehow arrange to not run the archiver process at all, or I guess to
just sit there and have it do nothing. Or, we can decide not to have a
separate background worker and just have the archiver call the
user-supplied core directly. I kind of like that approach at the
moment; it seems more elegant to me.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Weird failure with latches in curculio on v15
2023-02-05 00:49 Re: Weird failure with latches in curculio on v15 Michael Paquier <[email protected]>
@ 2023-02-05 23:36 ` Michael Paquier <[email protected]>
1 sibling, 0 replies; 15+ messages in thread
From: Michael Paquier @ 2023-02-05 23:36 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Andres Freund <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]>
On Sun, Feb 05, 2023 at 09:49:57AM +0900, Michael Paquier wrote:
> Yes, at this stage a revert of the refactoring with shell_restore.c is
> the best path forward.
Done that now, as of 2f6e15a.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH v7 10/14] hio: Relax rules for calling GetVisibilityMapPins()
@ 2023-04-03 18:18 Andres Freund <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Andres Freund @ 2023-04-03 18:18 UTC (permalink / raw)
Author:
Reviewed-by:
Discussion: https://postgr.es/m/[email protected]
Backpatch:
---
src/backend/access/heap/hio.c | 37 ++++++++++++++++++++++++-----------
1 file changed, 26 insertions(+), 11 deletions(-)
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index 65886839e70..e77437fcac1 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -131,9 +131,9 @@ ReadBufferBI(Relation relation, BlockNumber targetBlock,
* For each heap page which is all-visible, acquire a pin on the appropriate
* visibility map page, if we haven't already got one.
*
- * buffer2 may be InvalidBuffer, if only one buffer is involved. buffer1
- * must not be InvalidBuffer. If both buffers are specified, block1 must
- * be less than block2.
+ * To avoid complexity in the callers, either buffer1 or buffer2 may be
+ * InvalidBuffer if only one buffer is involved. For the same reason, block2
+ * may be smaller than block1.
*/
static void
GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
@@ -143,6 +143,26 @@ GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
bool need_to_pin_buffer1;
bool need_to_pin_buffer2;
+ /*
+ * Swap buffers around to handle case of a single block/buffer, and to
+ * handle if lock ordering rules require to lock block2 first.
+ */
+ if (!BufferIsValid(buffer1) ||
+ (BufferIsValid(buffer2) && block1 > block2))
+ {
+ Buffer tmpbuf = buffer1;
+ Buffer *tmpvmbuf = vmbuffer1;
+ BlockNumber tmpblock = block1;
+
+ buffer1 = buffer2;
+ vmbuffer1 = vmbuffer2;
+ block1 = block2;
+
+ buffer2 = tmpbuf;
+ vmbuffer2 = tmpvmbuf;
+ block2 = tmpblock;
+ }
+
Assert(BufferIsValid(buffer1));
Assert(buffer2 == InvalidBuffer || block1 <= block2);
@@ -508,14 +528,9 @@ loop:
* done a bit of extra work for no gain, but there's no real harm
* done.
*/
- if (otherBuffer == InvalidBuffer || targetBlock <= otherBlock)
- GetVisibilityMapPins(relation, buffer, otherBuffer,
- targetBlock, otherBlock, vmbuffer,
- vmbuffer_other);
- else
- GetVisibilityMapPins(relation, otherBuffer, buffer,
- otherBlock, targetBlock, vmbuffer_other,
- vmbuffer);
+ GetVisibilityMapPins(relation, buffer, otherBuffer,
+ targetBlock, otherBlock, vmbuffer,
+ vmbuffer_other);
/*
* Now we can check to see if there's enough free space here. If so,
--
2.38.0
--ju7ntqqtbf66a3ug
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v7-0011-hio-Don-t-pin-the-VM-while-holding-buffer-lock-wh.patch"
^ permalink raw reply [nested|flat] 15+ messages in thread
end of thread, other threads:[~2023-04-03 18:18 UTC | newest]
Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-02-05 00:49 Re: Weird failure with latches in curculio on v15 Michael Paquier <[email protected]>
2023-02-05 22:19 ` Nathan Bossart <[email protected]>
2023-02-05 23:01 ` Andres Freund <[email protected]>
2023-02-05 23:57 ` Nathan Bossart <[email protected]>
2023-02-06 00:07 ` Andres Freund <[email protected]>
2023-02-06 00:46 ` Nathan Bossart <[email protected]>
2023-02-08 15:22 ` Robert Haas <[email protected]>
2023-02-08 17:43 ` Nathan Bossart <[email protected]>
2023-02-08 21:24 ` Robert Haas <[email protected]>
2023-02-08 22:25 ` Nathan Bossart <[email protected]>
2023-02-08 23:56 ` Michael Paquier <[email protected]>
2023-02-09 00:24 ` Nathan Bossart <[email protected]>
2023-02-09 14:22 ` Robert Haas <[email protected]>
2023-02-05 23:36 ` Michael Paquier <[email protected]>
2023-04-03 18:18 [PATCH v7 10/14] hio: Relax rules for calling GetVisibilityMapPins() Andres Freund <[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