public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 1/2] Rework examine_opclause_expression to use varonleft
14+ messages / 3 participants
[nested] [flat]
* [PATCH 1/2] Rework examine_opclause_expression to use varonleft
@ 2019-07-19 14:28 Tomas Vondra <[email protected]>
0 siblings, 0 replies; 14+ messages in thread
From: Tomas Vondra @ 2019-07-19 14:28 UTC (permalink / raw)
The examine_opclause_expression function needs to return information on
which side of the operator we found the Var, but the variable was called
"isgt" which is rather misleading (it assumes the operator is either
less-than or greater-than, but it may be equality or something else).
Other places in the planner use a variable called "varonleft" for this
purpose, so just adopt the same convention here.
The code also assumed we don't care about this flag for equality, as
(Var = Const) and (Const = Var) should be the same thing. But that does
not work for cross-type operators, in which case we need to pass the
parameters to the procedure in the right order. So just use the same
code for all types of expressions.
This means we don't need to care about the selectivity estimation
function anymore, at least not in this code. We should only get the
supported cases here (thanks to statext_is_compatible_clause).
Discussion: https://postgr.es/m/8736jdhbhc.fsf%40ansel.ydns.eu
Backpatch-to: 12
---
src/backend/statistics/extended_stats.c | 16 ++---
src/backend/statistics/mcv.c | 62 +++++--------------
.../statistics/extended_stats_internal.h | 2 +-
3 files changed, 26 insertions(+), 54 deletions(-)
diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index d2346cbe02..23c74f7d90 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -1196,15 +1196,15 @@ statext_clauselist_selectivity(PlannerInfo *root, List *clauses, int varRelid,
* returns true, otherwise returns false.
*
* Optionally returns pointers to the extracted Var/Const nodes, when passed
- * non-null pointers (varp, cstp and isgtp). The isgt flag specifies whether
- * the Var node is on the left (false) or right (true) side of the operator.
+ * non-null pointers (varp, cstp and varonleftp). The varonleftp flag specifies
+ * on which side of the operator we found the Var node.
*/
bool
-examine_opclause_expression(OpExpr *expr, Var **varp, Const **cstp, bool *isgtp)
+examine_opclause_expression(OpExpr *expr, Var **varp, Const **cstp, bool *varonleftp)
{
Var *var;
Const *cst;
- bool isgt;
+ bool varonleft;
Node *leftop,
*rightop;
@@ -1225,13 +1225,13 @@ examine_opclause_expression(OpExpr *expr, Var **varp, Const **cstp, bool *isgtp)
{
var = (Var *) leftop;
cst = (Const *) rightop;
- isgt = false;
+ varonleft = true;
}
else if (IsA(leftop, Const) && IsA(rightop, Var))
{
var = (Var *) rightop;
cst = (Const *) leftop;
- isgt = true;
+ varonleft = false;
}
else
return false;
@@ -1243,8 +1243,8 @@ examine_opclause_expression(OpExpr *expr, Var **varp, Const **cstp, bool *isgtp)
if (cstp)
*cstp = cst;
- if (isgtp)
- *isgtp = isgt;
+ if (varonleftp)
+ *varonleftp = varonleft;
return true;
}
diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c
index e5a4e86c5d..2b685ec67a 100644
--- a/src/backend/statistics/mcv.c
+++ b/src/backend/statistics/mcv.c
@@ -1581,18 +1581,15 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
OpExpr *expr = (OpExpr *) clause;
FmgrInfo opproc;
- /* get procedure computing operator selectivity */
- RegProcedure oprrest = get_oprrest(expr->opno);
-
/* valid only after examine_opclause_expression returns true */
Var *var;
Const *cst;
- bool isgt;
+ bool varonleft;
fmgr_info(get_opcode(expr->opno), &opproc);
/* extract the var and const from the expression */
- if (examine_opclause_expression(expr, &var, &cst, &isgt))
+ if (examine_opclause_expression(expr, &var, &cst, &varonleft))
{
int idx;
@@ -1629,46 +1626,21 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
if (RESULT_IS_FINAL(matches[i], is_or))
continue;
- switch (oprrest)
- {
- case F_EQSEL:
- case F_NEQSEL:
-
- /*
- * We don't care about isgt in equality, because
- * it does not matter whether it's (var op const)
- * or (const op var).
- */
- match = DatumGetBool(FunctionCall2Coll(&opproc,
- DEFAULT_COLLATION_OID,
- cst->constvalue,
- item->values[idx]));
-
- break;
-
- case F_SCALARLTSEL: /* column < constant */
- case F_SCALARLESEL: /* column <= constant */
- case F_SCALARGTSEL: /* column > constant */
- case F_SCALARGESEL: /* column >= constant */
-
- /*
- * First check whether the constant is below the
- * lower boundary (in that case we can skip the
- * bucket, because there's no overlap).
- */
- if (isgt)
- match = DatumGetBool(FunctionCall2Coll(&opproc,
- DEFAULT_COLLATION_OID,
- cst->constvalue,
- item->values[idx]));
- else
- match = DatumGetBool(FunctionCall2Coll(&opproc,
- DEFAULT_COLLATION_OID,
- item->values[idx],
- cst->constvalue));
-
- break;
- }
+ /*
+ * First check whether the constant is below the lower
+ * boundary (in that case we can skip the bucket, because
+ * there's no overlap).
+ */
+ if (varonleft)
+ match = DatumGetBool(FunctionCall2Coll(&opproc,
+ DEFAULT_COLLATION_OID,
+ item->values[idx],
+ cst->constvalue));
+ else
+ match = DatumGetBool(FunctionCall2Coll(&opproc,
+ DEFAULT_COLLATION_OID,
+ cst->constvalue,
+ item->values[idx]));
/* update the match bitmap with the result */
matches[i] = RESULT_MERGE(matches[i], is_or, match);
diff --git a/src/include/statistics/extended_stats_internal.h b/src/include/statistics/extended_stats_internal.h
index c7f01d4edc..8433c34f6d 100644
--- a/src/include/statistics/extended_stats_internal.h
+++ b/src/include/statistics/extended_stats_internal.h
@@ -98,7 +98,7 @@ extern SortItem *build_sorted_items(int numrows, int *nitems, HeapTuple *rows,
int numattrs, AttrNumber *attnums);
extern bool examine_opclause_expression(OpExpr *expr, Var **varp,
- Const **cstp, bool *isgtp);
+ Const **cstp, bool *varonleftp);
extern Selectivity mcv_clauselist_selectivity(PlannerInfo *root,
StatisticExtInfo *stat,
--
2.21.0
--s34fs4l326flsq3n
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
filename="0002-Use-column-collation-for-extended-statistics.patch"
^ permalink raw reply [nested|flat] 14+ messages in thread
* [PATCH v1 1/1] Avoid multiple hard links to same WAL file after a crash.
@ 2022-04-07 17:07 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 14+ messages in thread
From: Nathan Bossart @ 2022-04-07 17:07 UTC (permalink / raw)
Presently, WAL recycling uses durable_rename_excl(), which notes that a crash at
an unfortunate moment can result in two 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 prevents this problem by using durable_rename() instead of
durable_rename_excl() for WAL recycling. This removes the protection against
accidentally overwriting an existing WAL file, but there shouldn't be one.
Back-patch to all supported versions.
Author: Nathan Bossart
---
src/backend/access/transam/xlog.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 6770c3ddba..6ab5b2a622 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3324,13 +3324,15 @@ 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).
+ * Perform the rename. Ideally, we'd use link() and unlink() to avoid
+ * overwriting an existing file (there shouldn't be one). However, that
+ * approach opens up the possibility that pg_wal will contain multiple hard
+ * links to the same WAL file after a crash.
*/
- 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
--5vNYLRcllDrimb99--
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
@ 2022-04-18 07:48 Michael Paquier <[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; 14+ 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] 14+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-18 07:48 Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
@ 2022-04-27 07:09 ` Michael Paquier <[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; 14+ 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] 14+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-18 07:48 Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[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-28 05:56 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 14+ 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] 14+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
@ 2022-05-01 13:08 Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 17:36 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
0 siblings, 2 replies; 14+ messages in thread
From: Michael Paquier @ 2022-05-01 13:08 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; pgsql-hackers
On Tue, Apr 12, 2022 at 09:27:42AM -0700, Nathan Bossart wrote:
> On Tue, Apr 12, 2022 at 03:46:31PM +0900, Kyotaro Horiguchi wrote:
>> At Mon, 11 Apr 2022 09:52:57 -0700, Nathan Bossart <[email protected]> wrote in
>>> I traced this back a while ago. I believe the link() was first added in
>>> November 2000 as part of f0e37a8. This even predates WAL recycling, which
>>> was added in July 2001 as part of 7d4d5c0.
>>
>> f0e37a8 lacks discussion.. It introduced the CHECKPOINT command from
>> somwhere out of the ML.. This patch changed XLogFileInit to
>> supportusing existent files so that XLogWrite can use the new segment
>> provided by checkpoint and still allow XLogWrite to create a new
>> segment by itself.
Yes, I think that you are right here. I also suspect that the
checkpoint command was facing a concurrency issue while working on
the feature and that Vadim saw that this part of the implementation
would be safer in the long run if we use link() followed by unlink().
> Yeah, I've been unable to find any discussion besides a brief reference to
> adding checkpointing [0].
>
> [0] https://postgr.es/m/8F4C99C66D04D4118F580090272A7A23018D85%40sectorbase1.sectorbase.com
While looking at the history of this area, I have also noticed this
argument, telling also that this is a safety measure if this code were
to run in parallel, but that's without counting on the control file
lock hold while doing this operation anyway:
https://www.postgresql.org/message-id/[email protected]
As mentioned already upthread, f0e37a8 is the origin of the
link()/unlink() business in the WAL segment initialization logic, and
also note 1f159e5 that has added a rename() as extra code path for
systems where link() was not working.
At the end, switching directly from durable_rename_excl() to
durable_rename() should be fine for the WAL segment initialization,
but we could do things a bit more carefully by adding a check on the
file existence before calling durable_rename() and issue a elog(LOG)
if a file is found, giving a mean for the WAL recycling to give up
peacefully as it does now. Per my analysis, the TLI history file
created at the end of recovery ought to issue an elog(ERROR).
Now, I am surprised by the third code path of durable_rename_excl(),
as of the WAL receiver doing writeTimeLineHistoryFile(), to not cause
any issues, as link() should exit with EEXIST when the startup process
grabs the same history file concurrently. It seems to me that in this
last case using durable_rename() could be an improvement and prevent
extra WAL receiver restarts as a TLI history fetched from the primary
via streaming or from some archives should be the same, but we could
be more careful, like the WAL init logic, by skipping the
durable_rename() and issuing an elog(LOG). That would not be perfect,
still a bit better than the current state of HEAD.
As we are getting closer to the beta release, it looks safer to let
this change aside a bit longer and wait for v16 to be opened for
business on HEAD.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-05-01 13:08 Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
@ 2022-05-02 10:48 ` Michael Paquier <[email protected]>
2022-05-02 17:39 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
1 sibling, 1 reply; 14+ messages in thread
From: Michael Paquier @ 2022-05-02 10:48 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; pgsql-hackers
On Sun, May 01, 2022 at 10:08:53PM +0900, Michael Paquier wrote:
> Now, I am surprised by the third code path of durable_rename_excl(),
> as of the WAL receiver doing writeTimeLineHistoryFile(), to not cause
> any issues, as link() should exit with EEXIST when the startup process
> grabs the same history file concurrently. It seems to me that in this
> last case using durable_rename() could be an improvement and prevent
> extra WAL receiver restarts as a TLI history fetched from the primary
> via streaming or from some archives should be the same, but we could
> be more careful, like the WAL init logic, by skipping the
> durable_rename() and issuing an elog(LOG). That would not be perfect,
> still a bit better than the current state of HEAD.
Skimming through at the buildfarm logs, it happens that the tests are
able to see this race from time to time. Here is one such example on
rorqual:
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=rorqual&dt=2022-04-20%2004%3A47%3A...
And here are the relevant logs:
2022-04-20 05:04:19.028 UTC [3109048][startup][:0] LOG: restored log
file "00000002.history" from archive
2022-04-20 05:04:19.029 UTC [3109111][walreceiver][:0] LOG: fetching
timeline history file for timeline 2 from primary server
2022-04-20 05:04:19.048 UTC [3109111][walreceiver][:0] FATAL: could
not link file "pg_wal/xlogtemp.3109111" to "pg_wal/00000002.history":
File exists
[...]
2022-04-20 05:04:19.234 UTC [3109250][walreceiver][:0] LOG: started
streaming WAL from primary at 0/3000000 on timeline 2
The WAL receiver upgrades the ERROR to a FATAL, and restarts
streaming shortly after. Using durable_rename() would not be an issue
here.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-05-01 13:08 Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
@ 2022-05-02 17:39 ` Nathan Bossart <[email protected]>
2022-05-02 23:06 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
0 siblings, 1 reply; 14+ messages in thread
From: Nathan Bossart @ 2022-05-02 17:39 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; pgsql-hackers
On Mon, May 02, 2022 at 07:48:18PM +0900, Michael Paquier wrote:
> Skimming through at the buildfarm logs, it happens that the tests are
> able to see this race from time to time. Here is one such example on
> rorqual:
> https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=rorqual&dt=2022-04-20%2004%3A47%3A...
>
> And here are the relevant logs:
> 2022-04-20 05:04:19.028 UTC [3109048][startup][:0] LOG: restored log
> file "00000002.history" from archive
> 2022-04-20 05:04:19.029 UTC [3109111][walreceiver][:0] LOG: fetching
> timeline history file for timeline 2 from primary server
> 2022-04-20 05:04:19.048 UTC [3109111][walreceiver][:0] FATAL: could
> not link file "pg_wal/xlogtemp.3109111" to "pg_wal/00000002.history":
> File exists
> [...]
> 2022-04-20 05:04:19.234 UTC [3109250][walreceiver][:0] LOG: started
> streaming WAL from primary at 0/3000000 on timeline 2
>
> The WAL receiver upgrades the ERROR to a FATAL, and restarts
> streaming shortly after. Using durable_rename() would not be an issue
> here.
Thanks for investigating this one. I think I agree that we should simply
switch to durable_rename() (without a file existence check beforehand).
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-05-01 13:08 Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 17:39 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
@ 2022-05-02 23:06 ` Nathan Bossart <[email protected]>
2022-05-05 11:10 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
0 siblings, 1 reply; 14+ messages in thread
From: Nathan Bossart @ 2022-05-02 23:06 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; pgsql-hackers
On Mon, May 02, 2022 at 10:39:07AM -0700, Nathan Bossart wrote:
> On Mon, May 02, 2022 at 07:48:18PM +0900, Michael Paquier wrote:
>> The WAL receiver upgrades the ERROR to a FATAL, and restarts
>> streaming shortly after. Using durable_rename() would not be an issue
>> here.
>
> Thanks for investigating this one. I think I agree that we should simply
> switch to durable_rename() (without a file existence check beforehand).
Here is a new patch set. For now, I've only removed the file existence
check in writeTimeLineHistoryFile(). I don't know if I'm totally convinced
that there isn't a problem here (e.g., due to concurrent .ready file
creation), but since some platforms have been using rename() for some time,
I don't know how worried we should be. I thought about adding some kind of
locking between the WAL receiver and startup processes, but that seems
excessive. Alternatively, we could just fix xlog.c as proposed earlier
[0]. AFAICT that is the only caller that can experience problems due to
the multiple-hard-link issue. All other callers are simply renaming a
temporary file into place, and the temporary file can be discarded if left
behind after a crash.
[0] https://postgr.es/m/20220407182954.GA1231544%40nathanxps13
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-05-01 13:08 Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 17:39 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-02 23:06 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
@ 2022-05-05 11:10 ` Michael Paquier <[email protected]>
2022-07-05 01:19 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
0 siblings, 1 reply; 14+ messages in thread
From: Michael Paquier @ 2022-05-05 11:10 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; pgsql-hackers
On Mon, May 02, 2022 at 04:06:13PM -0700, Nathan Bossart wrote:
> Here is a new patch set. For now, I've only removed the file existence
> check in writeTimeLineHistoryFile(). I don't know if I'm totally convinced
> that there isn't a problem here (e.g., due to concurrent .ready file
> creation), but since some platforms have been using rename() for some time,
> I don't know how worried we should be.
That's only about Windows these days, meaning that there is much less
coverage in this code path.
> I thought about adding some kind of
> locking between the WAL receiver and startup processes, but that seems
> excessive.
Agreed.
> Alternatively, we could just fix xlog.c as proposed earlier
> [0]. AFAICT that is the only caller that can experience problems due to
> the multiple-hard-link issue. All other callers are simply renaming a
> temporary file into place, and the temporary file can be discarded if left
> behind after a crash.
I'd agree with removing all the callers at the end. pgrename() is
quite robust on Windows, but I'd keep the two checks in
writeTimeLineHistory(), as the logic around findNewestTimeLine() would
consider a past TLI history file as in-use even if we have a crash
just after the file got created in the same path by the same standby,
and the WAL segment init part. Your patch does that.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../YnOwioN%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-05-01 13:08 Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 17:39 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-02 23:06 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-05 11:10 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
@ 2022-07-05 01:19 ` Michael Paquier <[email protected]>
2022-07-05 16:58 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
0 siblings, 1 reply; 14+ messages in thread
From: Michael Paquier @ 2022-07-05 01:19 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; pgsql-hackers
On Thu, May 05, 2022 at 08:10:02PM +0900, Michael Paquier wrote:
> I'd agree with removing all the callers at the end. pgrename() is
> quite robust on Windows, but I'd keep the two checks in
> writeTimeLineHistory(), as the logic around findNewestTimeLine() would
> consider a past TLI history file as in-use even if we have a crash
> just after the file got created in the same path by the same standby,
> and the WAL segment init part. Your patch does that.
As v16 is now open for business, I have revisited this change and
applied 0001 to change all the callers (aka removal of the assertion
for the WAL receiver when it overwrites a TLI history file). The
commit log includes details about the reasoning of all the areas
changed, for clarity, as of the WAL recycling part, the TLI history
file part and basic_archive.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../YsORtfT4+fXj%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-05-01 13:08 Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 17:39 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-02 23:06 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-05 11:10 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-07-05 01:19 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
@ 2022-07-05 16:58 ` Nathan Bossart <[email protected]>
2022-07-05 23:57 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
0 siblings, 1 reply; 14+ messages in thread
From: Nathan Bossart @ 2022-07-05 16:58 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; pgsql-hackers
On Tue, Jul 05, 2022 at 10:19:49AM +0900, Michael Paquier wrote:
> On Thu, May 05, 2022 at 08:10:02PM +0900, Michael Paquier wrote:
>> I'd agree with removing all the callers at the end. pgrename() is
>> quite robust on Windows, but I'd keep the two checks in
>> writeTimeLineHistory(), as the logic around findNewestTimeLine() would
>> consider a past TLI history file as in-use even if we have a crash
>> just after the file got created in the same path by the same standby,
>> and the WAL segment init part. Your patch does that.
>
> As v16 is now open for business, I have revisited this change and
> applied 0001 to change all the callers (aka removal of the assertion
> for the WAL receiver when it overwrites a TLI history file). The
> commit log includes details about the reasoning of all the areas
> changed, for clarity, as of the WAL recycling part, the TLI history
> file part and basic_archive.
Thanks! I wonder if we should add a comment in writeTimeLineHistoryFile()
about possible concurrent use by a WAL receiver and the startup process and
why that is okay.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-05-01 13:08 Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 17:39 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-02 23:06 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-05 11:10 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-07-05 01:19 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-07-05 16:58 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
@ 2022-07-05 23:57 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 14+ messages in thread
From: Michael Paquier @ 2022-07-05 23:57 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; pgsql-hackers
On Tue, Jul 05, 2022 at 09:58:38AM -0700, Nathan Bossart wrote:
> Thanks! I wonder if we should add a comment in writeTimeLineHistoryFile()
> about possible concurrent use by a WAL receiver and the startup process and
> why that is okay.
Agreed. Adding an extra note at the top of the routine would help in
the future.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-05-01 13:08 Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
@ 2022-05-02 17:36 ` Nathan Bossart <[email protected]>
1 sibling, 0 replies; 14+ messages in thread
From: Nathan Bossart @ 2022-05-02 17:36 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; pgsql-hackers
On Sun, May 01, 2022 at 10:08:53PM +0900, Michael Paquier wrote:
> At the end, switching directly from durable_rename_excl() to
> durable_rename() should be fine for the WAL segment initialization,
> but we could do things a bit more carefully by adding a check on the
> file existence before calling durable_rename() and issue a elog(LOG)
> if a file is found, giving a mean for the WAL recycling to give up
> peacefully as it does now. Per my analysis, the TLI history file
> created at the end of recovery ought to issue an elog(ERROR).
My only concern with this approach is that it inevitably introduces a race
condition. In most cases, the file existence check will prevent
overwrites, but it might not always. Furthermore, we believe that such
overwrites either 1) should not happen (e.g., WAL recycling) or 2) won't
cause problems if they happen (e.g., when the WAL receiver writes the TLI
history file). Also, these races will be difficult to test, so we won't
know what breaks when they occur.
My instinct is to just let the overwrites happen. That way, we are more
likely to catch breakage in tests, and we'll have one less race condition
to worry about. I don't mind asserting that the file doesn't exist when we
don't expect it to, as that might help catch potential problems in
development without affecting behavior in production. If we do want to
add file existence checks, I think we'd better add a comment about the
potential for race conditions.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 14+ messages in thread
end of thread, other threads:[~2022-07-05 23:57 UTC | newest]
Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-07-19 14:28 [PATCH 1/2] Rework examine_opclause_expression to use varonleft Tomas Vondra <[email protected]>
2022-04-07 17:07 [PATCH v1 1/1] Avoid multiple hard links to same WAL file after a crash. Nathan Bossart <[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-27 07:09 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-04-28 05:56 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-01 13:08 Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 17:39 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-02 23:06 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-05 11:10 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-07-05 01:19 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-07-05 16:58 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-07-05 23:57 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 17:36 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[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