public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v27 5/9] Add Incremental View Maintenance support to psql
12+ messages / 6 participants
[nested] [flat]
* [PATCH v27 5/9] Add Incremental View Maintenance support to psql
@ 2019-12-20 01:21 Yugo Nagata <[email protected]>
0 siblings, 0 replies; 12+ messages in thread
From: Yugo Nagata @ 2019-12-20 01:21 UTC (permalink / raw)
Add tab completion and meta-command output for IVM.
---
src/bin/psql/describe.c | 32 +++++++++++++++++++++++++++++++-
src/bin/psql/tab-complete.c | 14 +++++++++-----
2 files changed, 40 insertions(+), 6 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 583817b0cc..8cdcb3f048 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1492,6 +1492,7 @@ describeOneTableDetails(const char *schemaname,
char relpersistence;
char relreplident;
char *relam;
+ bool isivm;
} tableinfo;
bool show_column_details = false;
@@ -1504,7 +1505,26 @@ describeOneTableDetails(const char *schemaname,
initPQExpBuffer(&tmpbuf);
/* Get general table info */
- if (pset.sversion >= 120000)
+ if (pset.sversion >= 150000)
+ {
+ printfPQExpBuffer(&buf,
+ "SELECT c.relchecks, c.relkind, c.relhasindex, c.relhasrules, "
+ "c.relhastriggers, c.relrowsecurity, c.relforcerowsecurity, "
+ "false AS relhasoids, c.relispartition, %s, c.reltablespace, "
+ "CASE WHEN c.reloftype = 0 THEN '' ELSE c.reloftype::pg_catalog.regtype::pg_catalog.text END, "
+ "c.relpersistence, c.relreplident, am.amname, "
+ "c.relisivm\n"
+ "FROM pg_catalog.pg_class c\n "
+ "LEFT JOIN pg_catalog.pg_class tc ON (c.reltoastrelid = tc.oid)\n"
+ "LEFT JOIN pg_catalog.pg_am am ON (c.relam = am.oid)\n"
+ "WHERE c.oid = '%s';",
+ (verbose ?
+ "pg_catalog.array_to_string(c.reloptions || "
+ "array(select 'toast.' || x from pg_catalog.unnest(tc.reloptions) x), ', ')\n"
+ : "''"),
+ oid);
+ }
+ else if (pset.sversion >= 120000)
{
printfPQExpBuffer(&buf,
"SELECT c.relchecks, c.relkind, c.relhasindex, c.relhasrules, "
@@ -1624,6 +1644,10 @@ describeOneTableDetails(const char *schemaname,
(char *) NULL : pg_strdup(PQgetvalue(res, 0, 14));
else
tableinfo.relam = NULL;
+ if (pset.sversion >= 150000)
+ tableinfo.isivm = strcmp(PQgetvalue(res, 0, 15), "t") == 0;
+ else
+ tableinfo.isivm = false;
PQclear(res);
res = NULL;
@@ -3424,6 +3448,12 @@ describeOneTableDetails(const char *schemaname,
printfPQExpBuffer(&buf, _("Access method: %s"), tableinfo.relam);
printTableAddFooter(&cont, buf.data);
}
+
+ /* Incremental view maintance info */
+ if (verbose && tableinfo.relkind == RELKIND_MATVIEW && tableinfo.isivm)
+ {
+ printTableAddFooter(&cont, _("Incremental view maintenance: yes"));
+ }
}
/* reloptions, if verbose */
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 588c0841fe..87b5d3e107 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1221,6 +1221,7 @@ static const pgsql_thing_t words_after_create[] = {
{"FOREIGN TABLE", NULL, NULL, NULL},
{"FUNCTION", NULL, NULL, Query_for_list_of_functions},
{"GROUP", Query_for_list_of_roles},
+ {"INCREMENTAL MATERIALIZED VIEW", NULL, NULL, &Query_for_list_of_matviews, NULL, THING_NO_DROP | THING_NO_ALTER},
{"INDEX", NULL, NULL, &Query_for_list_of_indexes},
{"LANGUAGE", Query_for_list_of_languages},
{"LARGE OBJECT", NULL, NULL, NULL, NULL, THING_NO_CREATE | THING_NO_DROP},
@@ -3074,7 +3075,7 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH("SEQUENCE", "TABLE", "VIEW");
/* Complete "CREATE UNLOGGED" with TABLE or MATVIEW */
else if (TailMatches("CREATE", "UNLOGGED"))
- COMPLETE_WITH("TABLE", "MATERIALIZED VIEW");
+ COMPLETE_WITH("TABLE", "MATERIALIZED VIEW", "INCREMENTAL MATERIALIZED VIEW");
/* Complete PARTITION BY with RANGE ( or LIST ( or ... */
else if (TailMatches("PARTITION", "BY"))
COMPLETE_WITH("RANGE (", "LIST (", "HASH (");
@@ -3390,13 +3391,16 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH("SELECT");
/* CREATE MATERIALIZED VIEW */
- else if (Matches("CREATE", "MATERIALIZED"))
+ else if (Matches("CREATE", "MATERIALIZED") ||
+ Matches("CREATE", "INCREMENTAL", "MATERIALIZED"))
COMPLETE_WITH("VIEW");
- /* Complete CREATE MATERIALIZED VIEW <name> with AS */
- else if (Matches("CREATE", "MATERIALIZED", "VIEW", MatchAny))
+ /* Complete CREATE MATERIALIZED VIEW <name> with AS */
+ else if (Matches("CREATE", "MATERIALIZED", "VIEW", MatchAny) ||
+ Matches("CREATE", "INCREMENTAL", "MATERIALIZED", "VIEW", MatchAny))
COMPLETE_WITH("AS");
/* Complete "CREATE MATERIALIZED VIEW <sth> AS with "SELECT" */
- else if (Matches("CREATE", "MATERIALIZED", "VIEW", MatchAny, "AS"))
+ else if (Matches("CREATE", "MATERIALIZED", "VIEW", MatchAny, "AS") ||
+ Matches("CREATE", "INCREMENTAL", "MATERIALIZED", "VIEW", MatchAny, "AS"))
COMPLETE_WITH("SELECT");
/* CREATE EVENT TRIGGER */
--
2.17.1
--Multipart=_Fri__22_Apr_2022_14_58_01_+0900_MN3L/o2YUVF2g4zw
Content-Type: text/x-diff;
name="v27-0006-Add-Incremental-View-Maintenance-support.patch"
Content-Disposition: attachment;
filename="v27-0006-Add-Incremental-View-Maintenance-support.patch"
Content-Transfer-Encoding: 7bit
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
@ 2022-04-08 16:53 Nathan Bossart <[email protected]>
2022-04-08 17:05 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-04-09 01:00 ` Re: avoid multiple hard links to same WAL file after a crash Robert Haas <[email protected]>
0 siblings, 2 replies; 12+ messages in thread
From: Nathan Bossart @ 2022-04-08 16:53 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: pgsql-hackers
On Fri, Apr 08, 2022 at 10:38:03AM -0400, Robert Haas wrote:
> I see that durable_rename_excl() has the following comment: "Similar
> to durable_rename(), except that this routine tries (but does not
> guarantee) not to overwrite the target file." If those are the desired
> semantics, we could achieve them more simply and more safely by just
> trying to stat() the target file and then, if it's not found, call
> durable_rename(). I think that would be a heck of a lot safer than
> what this function is doing right now.
IIUC it actually does guarantee that you won't overwrite the target file
when HAVE_WORKING_LINK is defined. If not, it provides no guarantees at
all. Using stat() before rename() would therefore weaken this check for
systems with working link(), but it'd probably strengthen it for systems
without a working link().
> I'd actually be in favor of nuking durable_rename_excl() from orbit
> and putting the file-exists tests in the callers. Otherwise, someone
> might assume that it actually has the semantics that its name
> suggests, which could be pretty disastrous. If we don't want to do
> that, then I'd changing to do the stat-then-durable-rename thing
> internally, so we don't leave hard links lying around in *any* code
> path. Perhaps that's the right answer for the back-branches in any
> case, since there could be third-party code calling this function.
I think there might be another problem. The man page for rename() seems to
indicate that overwriting an existing file also introduces a window where
the old and new path are hard links to the same file. This isn't a problem
for the WAL files because we should never be overwriting an existing one,
but I wonder if it's a problem for other code paths. My guess is that many
code paths that overwrite an existing file are first writing changes to a
temporary file before atomically replacing the original. Those paths are
likely okay, too, as you can usually just discard any existing temporary
files.
> Your proposed fix is OK if we don't want to do any of that stuff, but
> personally I'm much more inclined to blame durable_rename_excl() for
> being horrible than I am to blame the calling code for using it
> improvidently.
I do agree that it's worth examining this stuff a bit closer. I've
frequently found myself trying to reason about all the different states
that callers of these functions can produce, so any changes that help
simplify matters are a win in my book.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-08 16:53 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
@ 2022-04-08 17:05 ` Nathan Bossart <[email protected]>
1 sibling, 0 replies; 12+ messages in thread
From: Nathan Bossart @ 2022-04-08 17:05 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: pgsql-hackers
On Fri, Apr 08, 2022 at 09:53:12AM -0700, Nathan Bossart wrote:
> On Fri, Apr 08, 2022 at 10:38:03AM -0400, Robert Haas wrote:
>> I'd actually be in favor of nuking durable_rename_excl() from orbit
>> and putting the file-exists tests in the callers. Otherwise, someone
>> might assume that it actually has the semantics that its name
>> suggests, which could be pretty disastrous. If we don't want to do
>> that, then I'd changing to do the stat-then-durable-rename thing
>> internally, so we don't leave hard links lying around in *any* code
>> path. Perhaps that's the right answer for the back-branches in any
>> case, since there could be third-party code calling this function.
>
> I think there might be another problem. The man page for rename() seems to
> indicate that overwriting an existing file also introduces a window where
> the old and new path are hard links to the same file. This isn't a problem
> for the WAL files because we should never be overwriting an existing one,
> but I wonder if it's a problem for other code paths. My guess is that many
> code paths that overwrite an existing file are first writing changes to a
> temporary file before atomically replacing the original. Those paths are
> likely okay, too, as you can usually just discard any existing temporary
> files.
Ha, so there are only a few callers of durable_rename_excl() in the
PostgreSQL tree. One is basic_archive.c, which is already doing a stat()
check. IIRC I only used durable_rename_excl() here to handle the case
where multiple servers are writing archives to the same location. If that
happened, the archiver process would begin failing. If a crash left two
hard links to the same file around, we will silently succeed the next time
around thanks to the compare_files() check. Besides the WAL installation
code, the only other callers are in timeline.c, and both note that the use
of durable_rename_excl() is for "paranoidly trying to avoid overwriting an
existing file (there shouldn't be one)."
So AFAICT basic_archive.c is the only caller with a strong reason for using
durable_rename_excl(), and even that might not be worth keeping it around.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-08 16:53 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
@ 2022-04-09 01:00 ` Robert Haas <[email protected]>
2022-04-18 07:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
1 sibling, 1 reply; 12+ messages in thread
From: Robert Haas @ 2022-04-09 01:00 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: pgsql-hackers
On Fri, Apr 8, 2022 at 12:53 PM Nathan Bossart <[email protected]> wrote:
> On Fri, Apr 08, 2022 at 10:38:03AM -0400, Robert Haas wrote:
> > I see that durable_rename_excl() has the following comment: "Similar
> > to durable_rename(), except that this routine tries (but does not
> > guarantee) not to overwrite the target file." If those are the desired
> > semantics, we could achieve them more simply and more safely by just
> > trying to stat() the target file and then, if it's not found, call
> > durable_rename(). I think that would be a heck of a lot safer than
> > what this function is doing right now.
>
> IIUC it actually does guarantee that you won't overwrite the target file
> when HAVE_WORKING_LINK is defined. If not, it provides no guarantees at
> all. Using stat() before rename() would therefore weaken this check for
> systems with working link(), but it'd probably strengthen it for systems
> without a working link().
Sure, but a guarantee that happens on only some systems isn't worth
much. And, if it comes at the price of potentially having multiple
hard links to the same file in obscure situations, that seems like it
could easily cause more problems than this whole scheme can ever hope
to solve.
> I think there might be another problem. The man page for rename() seems to
> indicate that overwriting an existing file also introduces a window where
> the old and new path are hard links to the same file. This isn't a problem
> for the WAL files because we should never be overwriting an existing one,
> but I wonder if it's a problem for other code paths. My guess is that many
> code paths that overwrite an existing file are first writing changes to a
> temporary file before atomically replacing the original. Those paths are
> likely okay, too, as you can usually just discard any existing temporary
> files.
I wonder if this is really true. I thought rename() was supposed to be atomic.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-08 16:53 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-04-09 01:00 ` Re: avoid multiple hard links to same WAL file after a crash Robert Haas <[email protected]>
@ 2022-04-18 07:48 ` Michael Paquier <[email protected]>
2022-04-18 18:23 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-04-18 19:07 ` Re: avoid multiple hard links to same WAL file after a crash Tom Lane <[email protected]>
0 siblings, 2 replies; 12+ messages in thread
From: Michael Paquier @ 2022-04-18 07:48 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Nathan Bossart <[email protected]>; pgsql-hackers
On Fri, Apr 08, 2022 at 09:00:36PM -0400, Robert Haas wrote:
> On Fri, Apr 8, 2022 at 12:53 PM Nathan Bossart <[email protected]> wrote:
>> I think there might be another problem. The man page for rename() seems to
>> indicate that overwriting an existing file also introduces a window where
>> the old and new path are hard links to the same file. This isn't a problem
>> for the WAL files because we should never be overwriting an existing one,
>> but I wonder if it's a problem for other code paths. My guess is that many
>> code paths that overwrite an existing file are first writing changes to a
>> temporary file before atomically replacing the original. Those paths are
>> likely okay, too, as you can usually just discard any existing temporary
>> files.
>
> I wonder if this is really true. I thought rename() was supposed to be atomic.
Not always. For example, some old versions of MacOS have a non-atomic
implementation of rename(), like prairiedog with 10.4. Even 10.5 does
not handle atomicity as far as I call. In short, it looks like a bad
idea to me to rely on this idea at all. Some FSes have their own way
of handling things, as well, but I am not much into this world.
Saying that, it would be nice to see durable_rename_excl() gone as it
has created quite a bit of pain for us in the past years.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../Yl0X0+%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-08 16:53 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-04-09 01:00 ` Re: avoid multiple hard links to same WAL file after a crash Robert Haas <[email protected]>
2022-04-18 07:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
@ 2022-04-18 18:23 ` Nathan Bossart <[email protected]>
1 sibling, 0 replies; 12+ messages in thread
From: Nathan Bossart @ 2022-04-18 18:23 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers
On Mon, Apr 18, 2022 at 04:48:35PM +0900, Michael Paquier wrote:
> Saying that, it would be nice to see durable_rename_excl() gone as it
> has created quite a bit of pain for us in the past years.
Yeah, I think this is the right thing to do. Patch upthread [0].
For back-branches, I suspect we'll want to remove all uses of
durable_rename_excl() but leave the function around for any extensions that
are using it. Of course, we'd also need a big comment imploring folks not
to add any more callers. Another option would be to change the behavior of
durable_rename_excl() to something that we think is safer (e.g., stat then
rename), but that might just introduce a different set of problems.
[0] https://postgr.es/m/20220408194345.GA1541826%40nathanxps13
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-08 16:53 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-04-09 01:00 ` Re: avoid multiple hard links to same WAL file after a crash Robert Haas <[email protected]>
2022-04-18 07:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
@ 2022-04-18 19:07 ` Tom Lane <[email protected]>
2022-04-18 20:53 ` Re: avoid multiple hard links to same WAL file after a crash Greg Stark <[email protected]>
1 sibling, 1 reply; 12+ messages in thread
From: Tom Lane @ 2022-04-18 19:07 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Robert Haas <[email protected]>; Nathan Bossart <[email protected]>; pgsql-hackers
Michael Paquier <[email protected]> writes:
> On Fri, Apr 08, 2022 at 09:00:36PM -0400, Robert Haas wrote:
>> I wonder if this is really true. I thought rename() was supposed to be atomic.
> Not always. For example, some old versions of MacOS have a non-atomic
> implementation of rename(), like prairiedog with 10.4. Even 10.5 does
> not handle atomicity as far as I call.
I think that's not talking about the same thing. POSIX requires rename(2)
to replace an existing target link atomically:
If the link named by the new argument exists, it shall be removed and
old renamed to new. In this case, a link named new shall remain
visible to other threads throughout the renaming operation and refer
either to the file referred to by new or old before the operation
began.
(It's that requirement that ancient macOS fails to meet.)
However, I do not see any text that addresses the question of whether
the old link disappears atomically with the appearance of the new link,
and it seems like that'd be pretty impractical to ensure in cases like
moving a link from one directory to another. (What would it even mean
to say that, considering that a thread can't read the two directories
at the same instant?) From a crash-safety standpoint, it'd surely be
better to make the new link before removing the old, so I imagine
that's what most file systems do.
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-08 16:53 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-04-09 01:00 ` Re: avoid multiple hard links to same WAL file after a crash Robert Haas <[email protected]>
2022-04-18 07:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-04-18 19:07 ` Re: avoid multiple hard links to same WAL file after a crash Tom Lane <[email protected]>
@ 2022-04-18 20:53 ` Greg Stark <[email protected]>
2022-04-26 20:09 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
0 siblings, 1 reply; 12+ messages in thread
From: Greg Stark @ 2022-04-18 20:53 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Nathan Bossart <[email protected]>; pgsql-hackers
The readdir interface allows processes to be in the middle of reading
a directory and unless a kernel was happy to either materialize the
entire directory list when the readdir starts, or lock the entire
directory against modification for the entire time the a process has a
readdir fd open it's always going to be possible for the a process to
have previously read the old directory entry and later see the new
directory entry. Kernels don't do any MVCC or cmin type of games so
they're not going to be able to prevent it.
What's worse of course is that it may only happen in very large
directories. Most directories fit on a single block and readdir may
buffer up all the entries a block at a time for efficiency. So it may
only be visible on very large directories that span multiple blocks.
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-08 16:53 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-04-09 01:00 ` Re: avoid multiple hard links to same WAL file after a crash Robert Haas <[email protected]>
2022-04-18 07:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-04-18 19:07 ` Re: avoid multiple hard links to same WAL file after a crash Tom Lane <[email protected]>
2022-04-18 20:53 ` Re: avoid multiple hard links to same WAL file after a crash Greg Stark <[email protected]>
@ 2022-04-26 20:09 ` Nathan Bossart <[email protected]>
2022-04-27 07:09 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
0 siblings, 1 reply; 12+ messages in thread
From: Nathan Bossart @ 2022-04-26 20:09 UTC (permalink / raw)
To: Greg Stark <[email protected]>; +Cc: Tom Lane <[email protected]>; Michael Paquier <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers
Here is an attempt at creating something that can be back-patched. 0001
simply replaces calls to durable_rename_excl() with durable_rename() and is
intended to be back-patched. 0002 removes the definition of
durable_rename_excl() and is _not_ intended for back-patching. I imagine
0002 will need to be held back for v16devel.
I think back-patching 0001 will encounter a couple of small obstacles. For
example, the call in basic_archive won't exist on most of the
back-branches, and durable_rename_excl() was named durable_link_or_rename()
before v13. I don't mind producing a patch for each back-branch if needed.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v3-0001-Replace-calls-to-durable_rename_excl-with-durable.patch (4.6K, ../../20220426200935.GA3175655@nathanxps13/2-v3-0001-Replace-calls-to-durable_rename_excl-with-durable.patch)
download | inline diff:
From d489c2bff029db6e07e5028788faf869c35f886b Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Tue, 26 Apr 2022 11:56:50 -0700
Subject: [PATCH v3 1/2] Replace calls to durable_rename_excl() with
durable_rename().
durable_rename_excl() attempts to avoid overwriting any existing
files by using link() and unlink(), but it falls back to rename()
on some platforms (e.g., Windows), which offers no such overwrite
protection. Most callers use durable_rename_excl() just in case
there is an existing file, but in practice there shouldn't be one.
basic_archive used it to avoid overwriting an archive concurrently
created by another server, but as mentioned above, it will still
overwrite files on some platforms.
Furthermore, failures during durable_rename_excl() can result in
multiple hard links to the same file. My testing demonstrated that
it was possible to end up with two links to the same file in pg_wal
after a crash just before unlink() during WAL recycling.
Specifically, the test produced links to the same file for the
current WAL file and the next one because the half-recycled WAL
file was re-recycled upon restarting. This seems likely to lead to
WAL corruption.
This change replaces all calls to durable_rename_excl() with
durable_rename(). This removes the protection against
accidentally overwriting an existing file, but some platforms are
already living without it, and ordinarily there shouldn't be one.
The function itself is left around in case any extensions are using
it. It will be removed in v16 via a follow-up commit.
Back-patch to all supported versions. Before v13,
durable_rename_excl() was named durable_link_or_rename().
Author: Nathan Bossart
Reviewed-by: Robert Haas, Kyotaro Horiguchi, Michael Paquier
Discussion: https://postgr.es/m/20220418182336.GA2298576%40nathanxps13
---
contrib/basic_archive/basic_archive.c | 5 +++--
src/backend/access/transam/timeline.c | 14 ++------------
src/backend/access/transam/xlog.c | 8 ++------
3 files changed, 7 insertions(+), 20 deletions(-)
diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index e7efbfb9c3..ed33854c57 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -281,9 +281,10 @@ basic_archive_file_internal(const char *file, const char *path)
/*
* Sync the temporary file to disk and move it to its final destination.
- * This will fail if destination already exists.
+ * Note that this will overwrite any existing file, but this is only
+ * possible if someone else created the file since the stat() above.
*/
- (void) durable_rename_excl(temp, destination, ERROR);
+ (void) durable_rename(temp, destination, ERROR);
ereport(DEBUG1,
(errmsg("archived \"%s\" via basic_archive", file)));
diff --git a/src/backend/access/transam/timeline.c b/src/backend/access/transam/timeline.c
index be21968293..128f754e87 100644
--- a/src/backend/access/transam/timeline.c
+++ b/src/backend/access/transam/timeline.c
@@ -441,12 +441,7 @@ writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
* Now move the completed history file into place with its final name.
*/
TLHistoryFilePath(path, newTLI);
-
- /*
- * Perform the rename using link if available, paranoidly trying to avoid
- * overwriting an existing file (there shouldn't be one).
- */
- durable_rename_excl(tmppath, path, ERROR);
+ durable_rename(tmppath, path, ERROR);
/* The history file can be archived immediately. */
if (XLogArchivingActive())
@@ -519,12 +514,7 @@ writeTimeLineHistoryFile(TimeLineID tli, char *content, int size)
* Now move the completed history file into place with its final name.
*/
TLHistoryFilePath(path, tli);
-
- /*
- * Perform the rename using link if available, paranoidly trying to avoid
- * overwriting an existing file (there shouldn't be one).
- */
- durable_rename_excl(tmppath, path, ERROR);
+ durable_rename(tmppath, path, ERROR);
}
/*
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 61cda56c6f..f49194a8b5 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3323,14 +3323,10 @@ InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
}
}
- /*
- * Perform the rename using link if available, paranoidly trying to avoid
- * overwriting an existing file (there shouldn't be one).
- */
- if (durable_rename_excl(tmppath, path, LOG) != 0)
+ if (durable_rename(tmppath, path, LOG) != 0)
{
LWLockRelease(ControlFileLock);
- /* durable_rename_excl already emitted log message */
+ /* durable_rename already emitted log message */
return false;
}
--
2.25.1
[text/x-diff] v3-0002-Remove-durable_rename_excl.patch (4.0K, ../../20220426200935.GA3175655@nathanxps13/3-v3-0002-Remove-durable_rename_excl.patch)
download | inline diff:
From 398350968f35f0974f1668e06be1adad4a7f7e3c Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Tue, 26 Apr 2022 12:38:23 -0700
Subject: [PATCH v3 2/2] Remove durable_rename_excl().
A previous commit replaced all calls to this function with
durable_rename(), but the function itself was not removed in back-
branches since extensions may use it. This change removes the
function from v16devel.
Do not back-patch.
Author: Nathan Bossart
Reviewed-by: Robert Haas, Kyotaro Horiguchi, Michael Paquier
Discussion: https://postgr.es/m/20220418182336.GA2298576%40nathanxps13
---
src/backend/storage/file/fd.c | 63 ----------------------------------
src/include/pg_config_manual.h | 7 ----
src/include/storage/fd.h | 1 -
3 files changed, 71 deletions(-)
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 24704b6a02..f904f60c08 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -807,69 +807,6 @@ durable_unlink(const char *fname, int elevel)
return 0;
}
-/*
- * durable_rename_excl -- rename a file in a durable manner.
- *
- * Similar to durable_rename(), except that this routine tries (but does not
- * guarantee) not to overwrite the target file.
- *
- * Note that a crash in an unfortunate moment can leave you with two links to
- * the target file.
- *
- * Log errors with the caller specified severity.
- *
- * On Windows, using a hard link followed by unlink() causes concurrency
- * issues, while a simple rename() does not cause that, so be careful when
- * changing the logic of this routine.
- *
- * Returns 0 if the operation succeeded, -1 otherwise. Note that errno is not
- * valid upon return.
- */
-int
-durable_rename_excl(const char *oldfile, const char *newfile, int elevel)
-{
- /*
- * Ensure that, if we crash directly after the rename/link, a file with
- * valid contents is moved into place.
- */
- if (fsync_fname_ext(oldfile, false, false, elevel) != 0)
- return -1;
-
-#ifdef HAVE_WORKING_LINK
- if (link(oldfile, newfile) < 0)
- {
- ereport(elevel,
- (errcode_for_file_access(),
- errmsg("could not link file \"%s\" to \"%s\": %m",
- oldfile, newfile)));
- return -1;
- }
- unlink(oldfile);
-#else
- if (rename(oldfile, newfile) < 0)
- {
- ereport(elevel,
- (errcode_for_file_access(),
- errmsg("could not rename file \"%s\" to \"%s\": %m",
- oldfile, newfile)));
- return -1;
- }
-#endif
-
- /*
- * Make change persistent in case of an OS crash, both the new entry and
- * its parent directory need to be flushed.
- */
- if (fsync_fname_ext(newfile, false, false, elevel) != 0)
- return -1;
-
- /* Same for parent directory */
- if (fsync_parent_path(newfile, elevel) != 0)
- return -1;
-
- return 0;
-}
-
/*
* InitFileAccess --- initialize this module during backend startup
*
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index 84ce5a4a5d..830804fdfb 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -163,13 +163,6 @@
#define USE_BARRIER_SMGRRELEASE
#endif
-/*
- * Define this if your operating system supports link()
- */
-#if !defined(WIN32) && !defined(__CYGWIN__)
-#define HAVE_WORKING_LINK 1
-#endif
-
/*
* USE_POSIX_FADVISE controls whether Postgres will attempt to use the
* posix_fadvise() kernel call. Usually the automatic configure tests are
diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h
index 69549b000f..2b4a8e0ffe 100644
--- a/src/include/storage/fd.h
+++ b/src/include/storage/fd.h
@@ -187,7 +187,6 @@ extern void fsync_fname(const char *fname, bool isdir);
extern int fsync_fname_ext(const char *fname, bool isdir, bool ignore_perm, int elevel);
extern int durable_rename(const char *oldfile, const char *newfile, int loglevel);
extern int durable_unlink(const char *fname, int loglevel);
-extern int durable_rename_excl(const char *oldfile, const char *newfile, int loglevel);
extern void SyncDataDirectory(void);
extern int data_sync_elevel(int elevel);
--
2.25.1
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-08 16:53 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-04-09 01:00 ` Re: avoid multiple hard links to same WAL file after a crash Robert Haas <[email protected]>
2022-04-18 07:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-04-18 19:07 ` Re: avoid multiple hard links to same WAL file after a crash Tom Lane <[email protected]>
2022-04-18 20:53 ` Re: avoid multiple hard links to same WAL file after a crash Greg Stark <[email protected]>
2022-04-26 20:09 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
@ 2022-04-27 07:09 ` Michael Paquier <[email protected]>
2022-04-27 18:42 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
0 siblings, 1 reply; 12+ messages in thread
From: Michael Paquier @ 2022-04-27 07:09 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Greg Stark <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers
On Tue, Apr 26, 2022 at 01:09:35PM -0700, Nathan Bossart wrote:
> Here is an attempt at creating something that can be back-patched. 0001
> simply replaces calls to durable_rename_excl() with durable_rename() and is
> intended to be back-patched. 0002 removes the definition of
> durable_rename_excl() and is _not_ intended for back-patching. I imagine
> 0002 will need to be held back for v16devel.
I would not mind applying 0002 on HEAD now to avoid more uses of this
API, and I can get behind 0001 after thinking more about it.
> I think back-patching 0001 will encounter a couple of small obstacles. For
> example, the call in basic_archive won't exist on most of the
> back-branches, and durable_rename_excl() was named durable_link_or_rename()
> before v13. I don't mind producing a patch for each back-branch if needed.
I am not sure that have any need to backpatch this change based on the
unlikeliness of the problem, TBH. One thing that is itching me a bit,
like Robert upthread, is that we don't check anymore that the newfile
does not exist in the code paths because we never expect one. It is
possible to use stat() for that. But access() within a simple
assertion would be simpler? Say something like:
Assert(access(path, F_OK) != 0 && errno == ENOENT);
The case for basic_archive is limited as the comment of the patch
states, but that would be helpful for the two calls in timeline.c and
the one in xlog.c in the long-term. And this has no need to be part
of fd.c, this can be added before the durable_rename() calls. What do
you think?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-08 16:53 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-04-09 01:00 ` Re: avoid multiple hard links to same WAL file after a crash Robert Haas <[email protected]>
2022-04-18 07:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-04-18 19:07 ` Re: avoid multiple hard links to same WAL file after a crash Tom Lane <[email protected]>
2022-04-18 20:53 ` Re: avoid multiple hard links to same WAL file after a crash Greg Stark <[email protected]>
2022-04-26 20:09 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-04-27 07:09 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
@ 2022-04-27 18:42 ` Nathan Bossart <[email protected]>
2022-04-28 05:56 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
0 siblings, 1 reply; 12+ messages in thread
From: Nathan Bossart @ 2022-04-27 18:42 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Greg Stark <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers
On Wed, Apr 27, 2022 at 04:09:20PM +0900, Michael Paquier wrote:
> I am not sure that have any need to backpatch this change based on the
> unlikeliness of the problem, TBH. One thing that is itching me a bit,
> like Robert upthread, is that we don't check anymore that the newfile
> does not exist in the code paths because we never expect one. It is
> possible to use stat() for that. But access() within a simple
> assertion would be simpler? Say something like:
> Assert(access(path, F_OK) != 0 && errno == ENOENT);
>
> The case for basic_archive is limited as the comment of the patch
> states, but that would be helpful for the two calls in timeline.c and
> the one in xlog.c in the long-term. And this has no need to be part
> of fd.c, this can be added before the durable_rename() calls. What do
> you think?
Here is a new patch set with these assertions added. I think at least the
xlog.c change ought to be back-patched. The problem may be unlikely, but
AFAICT the possible consequences include WAL corruption.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v4-0001-Replace-calls-to-durable_rename_excl-with-durable.patch (4.8K, ../../20220427184204.GB3222843@nathanxps13/2-v4-0001-Replace-calls-to-durable_rename_excl-with-durable.patch)
download | inline diff:
From 8ffc337621f8a287350a7a55256b58b0585f7a1f Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Tue, 26 Apr 2022 11:56:50 -0700
Subject: [PATCH v4 1/2] Replace calls to durable_rename_excl() with
durable_rename().
durable_rename_excl() attempts to avoid overwriting any existing
files by using link() and unlink(), but it falls back to rename()
on some platforms (e.g., Windows), which offers no such overwrite
protection. Most callers use durable_rename_excl() just in case
there is an existing file, but in practice there shouldn't be one.
basic_archive used it to avoid overwriting an archive concurrently
created by another server, but as mentioned above, it will still
overwrite files on some platforms.
Furthermore, failures during durable_rename_excl() can result in
multiple hard links to the same file. My testing demonstrated that
it was possible to end up with two links to the same file in pg_wal
after a crash just before unlink() during WAL recycling.
Specifically, the test produced links to the same file for the
current WAL file and the next one because the half-recycled WAL
file was re-recycled upon restarting. This seems likely to lead to
WAL corruption.
This change replaces all calls to durable_rename_excl() with
durable_rename(). This removes the protection against
accidentally overwriting an existing file, but some platforms are
already living without it, and ordinarily there shouldn't be one.
The function itself is left around in case any extensions are using
it. It will be removed in v16 via a follow-up commit.
Back-patch to all supported versions. Before v13,
durable_rename_excl() was named durable_link_or_rename().
Author: Nathan Bossart
Reviewed-by: Robert Haas, Kyotaro Horiguchi, Michael Paquier
Discussion: https://postgr.es/m/20220418182336.GA2298576%40nathanxps13
---
contrib/basic_archive/basic_archive.c | 5 +++--
src/backend/access/transam/timeline.c | 16 ++++------------
src/backend/access/transam/xlog.c | 9 +++------
3 files changed, 10 insertions(+), 20 deletions(-)
diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index e7efbfb9c3..ed33854c57 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -281,9 +281,10 @@ basic_archive_file_internal(const char *file, const char *path)
/*
* Sync the temporary file to disk and move it to its final destination.
- * This will fail if destination already exists.
+ * Note that this will overwrite any existing file, but this is only
+ * possible if someone else created the file since the stat() above.
*/
- (void) durable_rename_excl(temp, destination, ERROR);
+ (void) durable_rename(temp, destination, ERROR);
ereport(DEBUG1,
(errmsg("archived \"%s\" via basic_archive", file)));
diff --git a/src/backend/access/transam/timeline.c b/src/backend/access/transam/timeline.c
index be21968293..f3a8e53aa4 100644
--- a/src/backend/access/transam/timeline.c
+++ b/src/backend/access/transam/timeline.c
@@ -441,12 +441,8 @@ writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
* Now move the completed history file into place with its final name.
*/
TLHistoryFilePath(path, newTLI);
-
- /*
- * Perform the rename using link if available, paranoidly trying to avoid
- * overwriting an existing file (there shouldn't be one).
- */
- durable_rename_excl(tmppath, path, ERROR);
+ Assert(access(path, F_OK) != 0 && errno == ENOENT);
+ durable_rename(tmppath, path, ERROR);
/* The history file can be archived immediately. */
if (XLogArchivingActive())
@@ -519,12 +515,8 @@ writeTimeLineHistoryFile(TimeLineID tli, char *content, int size)
* Now move the completed history file into place with its final name.
*/
TLHistoryFilePath(path, tli);
-
- /*
- * Perform the rename using link if available, paranoidly trying to avoid
- * overwriting an existing file (there shouldn't be one).
- */
- durable_rename_excl(tmppath, path, ERROR);
+ Assert(access(path, F_OK) != 0 && errno == ENOENT);
+ durable_rename(tmppath, path, ERROR);
}
/*
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 61cda56c6f..76ec80c950 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3323,14 +3323,11 @@ InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
}
}
- /*
- * Perform the rename using link if available, paranoidly trying to avoid
- * overwriting an existing file (there shouldn't be one).
- */
- if (durable_rename_excl(tmppath, path, LOG) != 0)
+ Assert(access(path, F_OK) != 0 && errno == ENOENT);
+ if (durable_rename(tmppath, path, LOG) != 0)
{
LWLockRelease(ControlFileLock);
- /* durable_rename_excl already emitted log message */
+ /* durable_rename already emitted log message */
return false;
}
--
2.25.1
[text/x-diff] v4-0002-Remove-durable_rename_excl.patch (4.0K, ../../20220427184204.GB3222843@nathanxps13/3-v4-0002-Remove-durable_rename_excl.patch)
download | inline diff:
From 1d8e3c2040d1acf04e8c14637debf0dc12d500d2 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Tue, 26 Apr 2022 12:38:23 -0700
Subject: [PATCH v4 2/2] Remove durable_rename_excl().
A previous commit replaced all calls to this function with
durable_rename(), but the function itself was not removed in back-
branches since extensions may use it. This change removes the
function from v16devel.
Do not back-patch.
Author: Nathan Bossart
Reviewed-by: Robert Haas, Kyotaro Horiguchi, Michael Paquier
Discussion: https://postgr.es/m/20220418182336.GA2298576%40nathanxps13
---
src/backend/storage/file/fd.c | 63 ----------------------------------
src/include/pg_config_manual.h | 7 ----
src/include/storage/fd.h | 1 -
3 files changed, 71 deletions(-)
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 24704b6a02..f904f60c08 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -807,69 +807,6 @@ durable_unlink(const char *fname, int elevel)
return 0;
}
-/*
- * durable_rename_excl -- rename a file in a durable manner.
- *
- * Similar to durable_rename(), except that this routine tries (but does not
- * guarantee) not to overwrite the target file.
- *
- * Note that a crash in an unfortunate moment can leave you with two links to
- * the target file.
- *
- * Log errors with the caller specified severity.
- *
- * On Windows, using a hard link followed by unlink() causes concurrency
- * issues, while a simple rename() does not cause that, so be careful when
- * changing the logic of this routine.
- *
- * Returns 0 if the operation succeeded, -1 otherwise. Note that errno is not
- * valid upon return.
- */
-int
-durable_rename_excl(const char *oldfile, const char *newfile, int elevel)
-{
- /*
- * Ensure that, if we crash directly after the rename/link, a file with
- * valid contents is moved into place.
- */
- if (fsync_fname_ext(oldfile, false, false, elevel) != 0)
- return -1;
-
-#ifdef HAVE_WORKING_LINK
- if (link(oldfile, newfile) < 0)
- {
- ereport(elevel,
- (errcode_for_file_access(),
- errmsg("could not link file \"%s\" to \"%s\": %m",
- oldfile, newfile)));
- return -1;
- }
- unlink(oldfile);
-#else
- if (rename(oldfile, newfile) < 0)
- {
- ereport(elevel,
- (errcode_for_file_access(),
- errmsg("could not rename file \"%s\" to \"%s\": %m",
- oldfile, newfile)));
- return -1;
- }
-#endif
-
- /*
- * Make change persistent in case of an OS crash, both the new entry and
- * its parent directory need to be flushed.
- */
- if (fsync_fname_ext(newfile, false, false, elevel) != 0)
- return -1;
-
- /* Same for parent directory */
- if (fsync_parent_path(newfile, elevel) != 0)
- return -1;
-
- return 0;
-}
-
/*
* InitFileAccess --- initialize this module during backend startup
*
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index 84ce5a4a5d..830804fdfb 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -163,13 +163,6 @@
#define USE_BARRIER_SMGRRELEASE
#endif
-/*
- * Define this if your operating system supports link()
- */
-#if !defined(WIN32) && !defined(__CYGWIN__)
-#define HAVE_WORKING_LINK 1
-#endif
-
/*
* USE_POSIX_FADVISE controls whether Postgres will attempt to use the
* posix_fadvise() kernel call. Usually the automatic configure tests are
diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h
index 69549b000f..2b4a8e0ffe 100644
--- a/src/include/storage/fd.h
+++ b/src/include/storage/fd.h
@@ -187,7 +187,6 @@ extern void fsync_fname(const char *fname, bool isdir);
extern int fsync_fname_ext(const char *fname, bool isdir, bool ignore_perm, int elevel);
extern int durable_rename(const char *oldfile, const char *newfile, int loglevel);
extern int durable_unlink(const char *fname, int loglevel);
-extern int durable_rename_excl(const char *oldfile, const char *newfile, int loglevel);
extern void SyncDataDirectory(void);
extern int data_sync_elevel(int elevel);
--
2.25.1
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-08 16:53 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-04-09 01:00 ` Re: avoid multiple hard links to same WAL file after a crash Robert Haas <[email protected]>
2022-04-18 07:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-04-18 19:07 ` Re: avoid multiple hard links to same WAL file after a crash Tom Lane <[email protected]>
2022-04-18 20:53 ` Re: avoid multiple hard links to same WAL file after a crash Greg Stark <[email protected]>
2022-04-26 20:09 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-04-27 07:09 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-04-27 18:42 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
@ 2022-04-28 05:56 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 12+ messages in thread
From: Michael Paquier @ 2022-04-28 05:56 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Greg Stark <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers
On Wed, Apr 27, 2022 at 11:42:04AM -0700, Nathan Bossart wrote:
> Here is a new patch set with these assertions added. I think at least the
> xlog.c change ought to be back-patched. The problem may be unlikely, but
> AFAICT the possible consequences include WAL corruption.
Okay, so I have applied this stuff this morning to see what the
buildfarm had to say, and we have finished with a set of failures in
various buildfarm members:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=kestrel&dt=2022-04-28%2002%3A13%3A27
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rorqual&dt=2022-04-28%2002%3A14%3A08
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=calliphoridae&dt=2022-04-28%2002%3A59%3A...
All of them did not like the part where we assume that a TLI history
file written by a WAL receiver should not exist beforehand, but as
025_stuck_on_old_timeline.pl is showing, a standby may attempt to
retrieve a TLI history file after getting it from the archives.
I was analyzing the whole thing, and it looks like a race condition.
Per the the buildfarm logs, we have less than 5ms between the moment
the startup process retrieves the history file of TLI 2 from the
archives and the moment the WAL receiver decides to check if this TLI
file exists. If it does not exist, it would then retrieve it from the
primary via streaming. So I guess that the sequence of events is
that:
- In WalRcvFetchTimeLineHistoryFiles(), the WAL receiver checks the
existence of the history file for TLI 2, does not find it.
- The startup process retrieves the file from the archives.
- The WAL receiver goes through the internal loop of
WalRcvFetchTimeLineHistoryFiles(), retrieves the history file from the
primary's stream.
Switching from durable_rename_excl() to durable_rename() would mean
that we'd overwrite the TLI file received from the primary stream over
what's been retrieved from the archives. That does not strike me as
an issue in itself and that should be safe, so the comment is
misleading, and we can live without the assertion in
writeTimeLineHistoryFile() called by the WAL receiver. Now, I think
that we'd better keep some belts in writeTimeLineHistory() called by
the startup process at the end-of-recovery as I should never ever have
a TLI file generated when selecting a new timeline. Perhaps this
should be a elog(ERROR) at least, with a check on the file existence
before calling durable_rename()?
Anyway, my time is constrained next week due to the upcoming Japanese
Golden Week and the buildfarm has to be stable, so I have reverted the
change for now.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 12+ messages in thread
end of thread, other threads:[~2022-04-28 05:56 UTC | newest]
Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-12-20 01:21 [PATCH v27 5/9] Add Incremental View Maintenance support to psql Yugo Nagata <[email protected]>
2022-04-08 16:53 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-04-08 17:05 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-04-09 01:00 ` Re: avoid multiple hard links to same WAL file after a crash Robert Haas <[email protected]>
2022-04-18 07:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-04-18 18:23 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-04-18 19:07 ` Re: avoid multiple hard links to same WAL file after a crash Tom Lane <[email protected]>
2022-04-18 20:53 ` Re: avoid multiple hard links to same WAL file after a crash Greg Stark <[email protected]>
2022-04-26 20:09 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-04-27 07:09 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-04-27 18:42 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-04-28 05:56 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[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