pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedFrom: Nathan Bossart <nathandbossart@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: Fujii Masao <fujii@postgresql.org>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Weird failure with latches in curculio on v15
Date: Wed, 1 Feb 2023 09:58:06 -0800
Message-ID: <20230201175806.GA3199959@nathanxps13> (raw)
In-Reply-To: <20230201165801.33ydbxvjdbomjqa7@alap3.anarazel.de>
References: <Y9nGDSgIm83FHcad@paquier.xyz>
<20230201021206.wobi3dsnnuany3yq@alap3.anarazel.de>
<CA+hUKGKf9Bgik=g1yPQ-crSuuziyFsScnbauG50O5328TKV1rA@mail.gmail.com>
<20230201105514.rsjl4bnhb65giyvo@alap3.anarazel.de>
<1369666.1675264346@sss.pgh.pa.us>
<20230201165801.33ydbxvjdbomjqa7@alap3.anarazel.de>
On Wed, Feb 01, 2023 at 08:58:01AM -0800, Andres Freund wrote:
> On 2023-02-01 10:12:26 -0500, Tom Lane wrote:
>> The fundamental issue is that we have no good way to break out
>> of system(), and I think the original idea was that
>> in_restore_command would be set *only* for the duration of the
>> system() call. That's clearly been lost sight of completely,
>> but maybe as a stopgap we could try to get back to that.
>
> We could push the functions setting in_restore_command down into
> ExecuteRecoveryCommand(). But I don't think that'd end up necessarily
> being right either - we'd now use the mechanism in places we previously
> didn't (cleanup/end commands).
Right, we'd only want to set it for restore_command. I think that's
doable.
> And there's just plenty other stuff in the 14bdb3f13de 9a740f81eb0 that
> doesn't look right:
> - We now have two places open-coding what BuildRestoreCommand did
This was done because BuildRestoreCommand() had become a thin wrapper
around replace_percent_placeholders(). I can add it back if you don't
think this was the right decision.
> - I'm doubtful that the new shell_* functions are the base for a good
> API to abstract restoring files
Why?
> - the error message for a failed restore command seems to have gotten
> worse:
> could not restore file \"%s\" from archive: %s"
> ->
> "%s \"%s\": %s", commandName, command
Okay, I'll work on improving this message.
> - shell_* imo is not a good namespace for something called from xlog.c,
> xlogarchive.c. I realize the intention is that shell_archive.c is
> going to be its own "restore module", but for now it imo looks odd
What do you propose instead? FWIW this should go away with recovery
modules. This is just an intermediate state to simplify those patches.
> - The comment moved out of RestoreArchivedFile() doesn't seems less
> useful at its new location
Where do you think it should go?
> - explanation of why we use GetOldestRestartPoint() is halfway lost
Okay, I'll work on adding more context here.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
view thread (78+ messages) latest in thread
Message-ID: <20230201175806.GA3199959@nathanxps13>
Permalink: ../20230201175806.GA3199959@nathanxps13/
Also on: postgresql.org/message-id/20230201175806.GA3199959@nathanxps13
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, andres@anarazel.de, tgl@sss.pgh.pa.us, thomas.munro@gmail.com, fujii@postgresql.org, michael@paquier.xyz, pgsql-hackers@lists.postgresql.org
Subject: Re: Weird failure with latches in curculio on v15
In-Reply-To: <20230201175806.GA3199959@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox