public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v2 1/3] Allow CREATE INDEX CONCURRENTLY on partitioned table
18+ messages / 7 participants
[nested] [flat]
* [PATCH v2 1/3] Allow CREATE INDEX CONCURRENTLY on partitioned table
@ 2020-06-06 22:42 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 18+ messages in thread
From: Justin Pryzby @ 2020-06-06 22:42 UTC (permalink / raw)
---
doc/src/sgml/ref/create_index.sgml | 9 ----
src/backend/commands/indexcmds.c | 68 ++++++++++++++------------
src/test/regress/expected/indexing.out | 14 +++++-
src/test/regress/sql/indexing.sql | 3 +-
4 files changed, 52 insertions(+), 42 deletions(-)
diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index ff87b2d28f..e1298b8523 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -657,15 +657,6 @@ Indexes:
cannot.
</para>
- <para>
- Concurrent builds for indexes on partitioned tables are currently not
- supported. However, you may concurrently build the index on each
- partition individually and then finally create the partitioned index
- non-concurrently in order to reduce the time where writes to the
- partitioned table will be locked out. In this case, building the
- partitioned index is a metadata only operation.
- </para>
-
</refsect2>
</refsect1>
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 2baca12c5f..344cabaf52 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -652,17 +652,6 @@ DefineIndex(Oid relationId,
partitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
if (partitioned)
{
- /*
- * Note: we check 'stmt->concurrent' rather than 'concurrent', so that
- * the error is thrown also for temporary tables. Seems better to be
- * consistent, even though we could do it on temporary table because
- * we're not actually doing it concurrently.
- */
- if (stmt->concurrent)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot create index on partitioned table \"%s\" concurrently",
- RelationGetRelationName(rel))));
if (stmt->excludeOpNames)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -1139,8 +1128,26 @@ DefineIndex(Oid relationId,
CreateComments(indexRelationId, RelationRelationId, 0,
stmt->idxcomment);
+ /* save lockrelid and locktag for below */
+ heaprelid = rel->rd_lockInfo.lockRelId;
+ SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId);
+
if (partitioned)
{
+ /*
+ * Need to close the relation before recursing into children, so copy
+ * needed data.
+ */
+ PartitionDesc partdesc = RelationGetPartitionDesc(rel);
+ int nparts = partdesc->nparts;
+ TupleDesc parentDesc = CreateTupleDescCopy(RelationGetDescr(rel));
+ char *relname = pstrdup(RelationGetRelationName(rel));
+ Oid *part_oids;
+
+ part_oids = palloc(sizeof(Oid) * nparts);
+ memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
+ table_close(rel, NoLock);
+
/*
* Unless caller specified to skip this step (via ONLY), process each
* partition to make sure they all contain a corresponding index.
@@ -1149,19 +1156,11 @@ DefineIndex(Oid relationId,
*/
if (!stmt->relation || stmt->relation->inh)
{
- PartitionDesc partdesc = RelationGetPartitionDesc(rel);
- int nparts = partdesc->nparts;
- Oid *part_oids = palloc(sizeof(Oid) * nparts);
bool invalidate_parent = false;
- TupleDesc parentDesc;
Oid *opfamOids;
pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
nparts);
-
- memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
-
- parentDesc = RelationGetDescr(rel);
opfamOids = palloc(sizeof(Oid) * numberOfKeyAttributes);
for (i = 0; i < numberOfKeyAttributes; i++)
opfamOids[i] = get_opclass_family(classObjectId[i]);
@@ -1196,9 +1195,9 @@ DefineIndex(Oid relationId,
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot create unique index on partitioned table \"%s\"",
- RelationGetRelationName(rel)),
+ relname),
errdetail("Table \"%s\" contains partitions that are foreign tables.",
- RelationGetRelationName(rel))));
+ relname)));
table_close(childrel, lockmode);
continue;
@@ -1365,21 +1364,35 @@ DefineIndex(Oid relationId,
}
}
+ /*
+ * CIC needs to mark a partitioned table as VALID, which itself
+ * requires setting READY, which is unset for CIC (even though
+ * it's meaningless for an index without storage).
+ */
+ if (concurrent)
+ {
+ if (ActiveSnapshotSet())
+ PopActiveSnapshot();
+ CommitTransactionCommand();
+ StartTransactionCommand();
+ index_set_state_flags(indexRelationId, INDEX_CREATE_SET_READY);
+
+ CommandCounterIncrement();
+ index_set_state_flags(indexRelationId, INDEX_CREATE_SET_VALID);
+ }
+
/*
* Indexes on partitioned tables are not themselves built, so we're
* done here.
*/
- table_close(rel, NoLock);
if (!OidIsValid(parentIndexId))
pgstat_progress_end_command();
return address;
}
+ table_close(rel, NoLock);
if (!concurrent)
{
- /* Close the heap and we're done, in the non-concurrent case */
- table_close(rel, NoLock);
-
/* If this is the top-level index, we're done. */
if (!OidIsValid(parentIndexId))
pgstat_progress_end_command();
@@ -1387,11 +1400,6 @@ DefineIndex(Oid relationId,
return address;
}
- /* save lockrelid and locktag for below, then close rel */
- heaprelid = rel->rd_lockInfo.lockRelId;
- SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId);
- table_close(rel, NoLock);
-
/*
* For a concurrent build, it's important to make the catalog entries
* visible to other transactions before we start to build the index. That
diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out
index f78865ef81..17fa77e317 100644
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -50,11 +50,21 @@ select relname, relkind, relhassubclass, inhparent::regclass
(8 rows)
drop table idxpart;
--- Some unsupported features
+-- CIC on partitioned table
create table idxpart (a int, b int, c text) partition by range (a);
create table idxpart1 partition of idxpart for values from (0) to (10);
create index concurrently on idxpart (a);
-ERROR: cannot create index on partitioned table "idxpart" concurrently
+\d idxpart1
+ Table "public.idxpart1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+ b | integer | | |
+ c | text | | |
+Partition of: idxpart FOR VALUES FROM (0) TO (10)
+Indexes:
+ "idxpart1_a_idx" btree (a)
+
drop table idxpart;
-- Verify bugfix with query on indexed partitioned table with no partitions
-- https://postgr.es/m/[email protected]
diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql
index 35d159f41b..19a21eba9d 100644
--- a/src/test/regress/sql/indexing.sql
+++ b/src/test/regress/sql/indexing.sql
@@ -29,10 +29,11 @@ select relname, relkind, relhassubclass, inhparent::regclass
where relname like 'idxpart%' order by relname;
drop table idxpart;
--- Some unsupported features
+-- CIC on partitioned table
create table idxpart (a int, b int, c text) partition by range (a);
create table idxpart1 partition of idxpart for values from (0) to (10);
create index concurrently on idxpart (a);
+\d idxpart1
drop table idxpart;
-- Verify bugfix with query on indexed partitioned table with no partitions
--
2.17.0
--JwB53PgKC5A7+0Ej
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v2-0002-Implement-CLUSTER-of-partitioned-table.patch"
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: The danger of deleting backup_label
@ 2023-10-10 21:06 David Steele <[email protected]>
0 siblings, 2 replies; 18+ messages in thread
From: David Steele @ 2023-10-10 21:06 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: pgsql-hackers
On 9/28/23 22:30, Michael Paquier wrote:
> On Thu, Sep 28, 2023 at 05:14:22PM -0400, David Steele wrote:
>
>> Recovery worked perfectly as long as backup_label was present and failed
>> hard when it was not:
>>
>> LOG: invalid primary checkpoint record
>> PANIC: could not locate a valid checkpoint record
>>
>> It's not a very good message, but at least the foot gun has been removed. We
>> could use this as a special value to give a better message, and maybe use
>> something a bit more unique like 0xFFFFFFFFFADEFADE (or whatever) as the
>> value.
>
> Why not just InvalidXLogRecPtr?
That fails because there is a check to make sure the checkpoint is valid
when pg_control is loaded. Another possibility is to use a special LSN
like we use for unlogged tables. Anything >= 24 and < WAL segment size
will work fine.
>> This is all easy enough for pg_basebackup to do, but will certainly be
>> non-trivial for most backup software to implement. In [2] we have discussed
>> perhaps returning pg_control from pg_backup_stop() for the backup software
>> to save, or it could become part of the backup_label (encoded as hex or
>> base64, presumably). I prefer the latter as this means less work for the
>> backup software (except for the need to exclude pg_control from the backup).
>>
>> I don't have a patch for this yet because I did not test this idea using
>> pg_basebackup, but I'll be happy to work up a patch if there is interest.
>
> If the contents of the control file are tweaked before sending it
> through a BASE_BACKUP, it would cover more than just pg_basebackup.
> Switching the way the control file is sent with new contents in
> sendFileWithContent() rather than sendFile() would be one way, for
> instance..
Good point, and that makes this even more compelling. If we include
pg_control into backup_label then there is no need to modify pg_control
(as above) -- we can just exclude it from the backup entirely. That will
certainly require some rejigging in recovery but seems worth it for
backup solutions that can't easily modify pg_control. The C-based
solutions can do this pretty easily but it is a pretty high bar for
anyone else.
Regards,
-David
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: The danger of deleting backup_label
@ 2023-10-11 22:10 Thomas Munro <[email protected]>
parent: David Steele <[email protected]>
1 sibling, 1 reply; 18+ messages in thread
From: Thomas Munro @ 2023-10-11 22:10 UTC (permalink / raw)
To: David Steele <[email protected]>; +Cc: Michael Paquier <[email protected]>; pgsql-hackers; Stephen Frost <[email protected]>
Hi David,
Even though I spent a whole bunch of time trying to figure out how to
make concurrent reads of the control file sufficiently atomic for
backups (pg_basebackup and low level filesystem tools), and we
explored multiple avenues with varying results, and finally came up
with something that basically works pretty well... actually I just
hate all of that stuff, and I'm hoping to be able to just withdraw
https://commitfest.postgresql.org/45/4025/ and chalk it all up to
discovery/education and call *this* thread the real outcome of that
preliminary work.
So I'm +1 on the idea of putting a control file image into the backup
label and I'm happy that you're looking into it.
We could just leave the control file out of the base backup
completely, as you said, removing a whole foot-gun. People following
the 'low level' instructions will still get a copy of the control file
from the filesystem, and I don't see any reliable way to poison that
file without also making it so that a crash wouldn't also be prevented
from recovering. I have wondered about putting extra "fingerprint"
information into the control file such as the file's path and inode
number etc, so that you can try to distinguish between a control file
written by PostgreSQL, and a control file copied somewhere else, but
that all feels too fragile, and at the end of the day, people
following the low level backup instructions had better follow the low
level backup instructions (hopefully via the intermediary of an
excellent external backup tool).
As Stephen mentioned[1], we could perhaps also complain if both backup
label and control file exist, and then hint that the user should
remove the *control file* (not the backup label!). I had originally
suggested we would just overwrite the control file, but by explicitly
complaining about it we would also bring the matter to tool/script
authors' attention, ie that they shouldn't be backing that file up, or
should be removing it in a later step if they copy everything. He
also mentions that there doesn't seem to be anything stopping us from
back-patching changes to the backup label contents if we go this way.
I don't have a strong opinion on that and we could leave the question
for later.
[1] https://www.postgresql.org/message-id/ZL69NXjCNG%2BWHCqG%40tamriel.snowman.net
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: The danger of deleting backup_label
@ 2023-10-11 22:22 Michael Paquier <[email protected]>
parent: David Steele <[email protected]>
1 sibling, 1 reply; 18+ messages in thread
From: Michael Paquier @ 2023-10-11 22:22 UTC (permalink / raw)
To: David Steele <[email protected]>; +Cc: pgsql-hackers
On Tue, Oct 10, 2023 at 05:06:45PM -0400, David Steele wrote:
> That fails because there is a check to make sure the checkpoint is valid
> when pg_control is loaded. Another possibility is to use a special LSN like
> we use for unlogged tables. Anything >= 24 and < WAL segment size will work
> fine.
Do we have any reason to do that in the presence of a backup_label
file anyway? We'll know the LSN of the checkpoint based on what the
base backup wants us to use. Using a fake-still-rather-valid value
for the LSN in the control file to bypass this check does not address
the issue you are pointing at: it is just avoiding this check. A
reasonable answer would be, IMO, to just not do this check at all
based on the control file in this case.
>> If the contents of the control file are tweaked before sending it
>> through a BASE_BACKUP, it would cover more than just pg_basebackup.
>> Switching the way the control file is sent with new contents in
>> sendFileWithContent() rather than sendFile() would be one way, for
>> instance..
>
> Good point, and that makes this even more compelling. If we include
> pg_control into backup_label then there is no need to modify pg_control (as
> above) -- we can just exclude it from the backup entirely. That will
> certainly require some rejigging in recovery but seems worth it for backup
> solutions that can't easily modify pg_control. The C-based solutions can do
> this pretty easily but it is a pretty high bar for anyone else.
I have little idea about that, but I guess that you are referring to
backrest here.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: The danger of deleting backup_label
@ 2023-10-12 14:19 David Steele <[email protected]>
parent: Thomas Munro <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: David Steele @ 2023-10-12 14:19 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Michael Paquier <[email protected]>; pgsql-hackers; Stephen Frost <[email protected]>
Hi Thomas,
On 10/11/23 18:10, Thomas Munro wrote:
>
> Even though I spent a whole bunch of time trying to figure out how to
> make concurrent reads of the control file sufficiently atomic for
> backups (pg_basebackup and low level filesystem tools), and we
> explored multiple avenues with varying results, and finally came up
> with something that basically works pretty well... actually I just
> hate all of that stuff, and I'm hoping to be able to just withdraw
> https://commitfest.postgresql.org/45/4025/ and chalk it all up to
> discovery/education and call *this* thread the real outcome of that
> preliminary work.
>
> So I'm +1 on the idea of putting a control file image into the backup
> label and I'm happy that you're looking into it.
Well, hopefully this thread will *at least* be the solution going
forward. Not sure about a back patch yet, see below...
> We could just leave the control file out of the base backup
> completely, as you said, removing a whole foot-gun.
That's the plan.
> People following
> the 'low level' instructions will still get a copy of the control file
> from the filesystem, and I don't see any reliable way to poison that
> file without also making it so that a crash wouldn't also be prevented
> from recovering. I have wondered about putting extra "fingerprint"
> information into the control file such as the file's path and inode
> number etc, so that you can try to distinguish between a control file
> written by PostgreSQL, and a control file copied somewhere else, but
> that all feels too fragile, and at the end of the day, people
> following the low level backup instructions had better follow the low
> level backup instructions (hopefully via the intermediary of an
> excellent external backup tool).
Not sure about the inode idea, because it seems OK for people to move a
cluster elsewhere under a variety of circumstances. I do have an idea
about how to mark a cluster in "recovery to consistency" mode, but not
quite sure how to atomically turn that off at the end of recovery to
consistency. I have some ideas I'll work on though.
> As Stephen mentioned[1], we could perhaps also complain if both backup
> label and control file exist, and then hint that the user should
> remove the *control file* (not the backup label!). I had originally
> suggested we would just overwrite the control file, but by explicitly
> complaining about it we would also bring the matter to tool/script
> authors' attention, ie that they shouldn't be backing that file up, or
> should be removing it in a later step if they copy everything. He
> also mentions that there doesn't seem to be anything stopping us from
> back-patching changes to the backup label contents if we go this way.
> I don't have a strong opinion on that and we could leave the question
> for later.
I'm worried about the possibility of back patching this unless the
solution comes out to be simpler than I think and that rarely comes to
pass. Surely throwing errors on something that is currently valid (i.e.
backup_label and pg_control both present).
But perhaps there is a simpler, acceptable solution we could back patch
(transparent to all parties except Postgres) and then a more advanced
solution we could go forward with.
I guess I had better get busy on this.
Regards,
-David
[1]
https://www.postgresql.org/message-id/ZL69NXjCNG%2BWHCqG%40tamriel.snowman.net
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: The danger of deleting backup_label
@ 2023-10-12 14:25 David Steele <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 0 replies; 18+ messages in thread
From: David Steele @ 2023-10-12 14:25 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: pgsql-hackers
On 10/11/23 18:22, Michael Paquier wrote:
> On Tue, Oct 10, 2023 at 05:06:45PM -0400, David Steele wrote:
>> That fails because there is a check to make sure the checkpoint is valid
>> when pg_control is loaded. Another possibility is to use a special LSN like
>> we use for unlogged tables. Anything >= 24 and < WAL segment size will work
>> fine.
>
> Do we have any reason to do that in the presence of a backup_label
> file anyway? We'll know the LSN of the checkpoint based on what the
> base backup wants us to use. Using a fake-still-rather-valid value
> for the LSN in the control file to bypass this check does not address
> the issue you are pointing at: it is just avoiding this check. A
> reasonable answer would be, IMO, to just not do this check at all
> based on the control file in this case.
Yeah, that's fair. And it looks like we are leaning towards excluding
pg_control from the backup entirely, so the point is probably moot.
>>> If the contents of the control file are tweaked before sending it
>>> through a BASE_BACKUP, it would cover more than just pg_basebackup.
>>> Switching the way the control file is sent with new contents in
>>> sendFileWithContent() rather than sendFile() would be one way, for
>>> instance..
>>
>> Good point, and that makes this even more compelling. If we include
>> pg_control into backup_label then there is no need to modify pg_control (as
>> above) -- we can just exclude it from the backup entirely. That will
>> certainly require some rejigging in recovery but seems worth it for backup
>> solutions that can't easily modify pg_control. The C-based solutions can do
>> this pretty easily but it is a pretty high bar for anyone else.
>
> I have little idea about that, but I guess that you are referring to
> backrest here.
Sure, pgBackRest, but there are other backup solutions written in C. My
point is really that we should not depend on backup solutions being able
to manipulate C structs. It looks the the solution we are working
towards would not require that.
Regards,
-David
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: The danger of deleting backup_label
@ 2023-10-14 15:30 David Steele <[email protected]>
parent: David Steele <[email protected]>
0 siblings, 2 replies; 18+ messages in thread
From: David Steele @ 2023-10-14 15:30 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Michael Paquier <[email protected]>; pgsql-hackers; Stephen Frost <[email protected]>
On 10/12/23 10:19, David Steele wrote:
> On 10/11/23 18:10, Thomas Munro wrote:
>
>> As Stephen mentioned[1], we could perhaps also complain if both backup
>> label and control file exist, and then hint that the user should
>> remove the *control file* (not the backup label!). I had originally
>> suggested we would just overwrite the control file, but by explicitly
>> complaining about it we would also bring the matter to tool/script
>> authors' attention, ie that they shouldn't be backing that file up, or
>> should be removing it in a later step if they copy everything. He
>> also mentions that there doesn't seem to be anything stopping us from
>> back-patching changes to the backup label contents if we go this way.
>> I don't have a strong opinion on that and we could leave the question
>> for later.
>
> I'm worried about the possibility of back patching this unless the
> solution comes out to be simpler than I think and that rarely comes to
> pass. Surely throwing errors on something that is currently valid (i.e.
> backup_label and pg_control both present).
>
> But perhaps there is a simpler, acceptable solution we could back patch
> (transparent to all parties except Postgres) and then a more advanced
> solution we could go forward with.
>
> I guess I had better get busy on this.
Attached is a very POC attempt at something along these lines that could
be back patched. I stopped when I realized excluding pg_control from the
backup is a necessity to make this work (else we still could end up with
a torn copy of pg_control even if there is a good copy elsewhere). To
enumerate the back patch issues as I see them:
1) We still need a copy of pg_control in order to get Postgres to start
and that copy might be torn (pretty much where we are now). We can
handle this easily in pg_basebackup but most backup software will not be
able to do so. In my opinion teaching Postgres to start without
pg_control is too big a change to possibly back patch.
2) We need to deal with backups made with a prior *minor* version that
did not include pg_control in the backup_label. Doable, but...
3) We need to move backup_label to the end of the main pg_basebackup
tar, which could cause unforeseen breakage for tools.
4) This patch is less efficient for backups taken from standby because
it will overwrite pg_control on restart and force replay back to the
original MinRecoveryPoint.
5) We still need a solution for exclusive backup (still valid in PG <=
14). Doable but it would still have the weakness of 1.
All of this is fixable in HEAD, but seems incredibly dangerous to back
patch. Even so, I have attached the patch in case somebody sees an
opportunity that I do not.
Regards,
-David
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c0e4ca50899..e4a4478af75 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -74,6 +74,7 @@
#include "pg_trace.h"
#include "pgstat.h"
#include "port/atomics.h"
+#include "port/pg_crc32c.h"
#include "port/pg_iovec.h"
#include "postmaster/bgwriter.h"
#include "postmaster/startup.h"
@@ -8691,6 +8692,21 @@ do_pg_backup_stop(BackupState *state, bool waitforarchive)
"and should not be used. "
"Try taking another online backup.")));
+ /*
+ * Create a copy of control data to be stored in the backup label.
+ * Recalculate the CRC so recovery can validate the contents but do not
+ * bother with the timestamp since that will be applied before it is
+ * written by recovery.
+ */
+ LWLockAcquire(ControlFileLock, LW_SHARED);
+ state->controlFile = *ControlFile;
+ LWLockRelease(ControlFileLock);
+
+ INIT_CRC32C(state->controlFile.crc);
+ COMP_CRC32C(state->controlFile.crc, (char *)&state->controlFile,
+ offsetof(ControlFileData, crc));
+ FIN_CRC32C(state->controlFile.crc);
+
/*
* During recovery, we don't write an end-of-backup record. We assume that
* pg_control was backed up last and its minimum recovery point can be
@@ -8741,11 +8757,8 @@ do_pg_backup_stop(BackupState *state, bool waitforarchive)
"Enable full_page_writes and run CHECKPOINT on the primary, "
"and then try an online backup again.")));
-
- LWLockAcquire(ControlFileLock, LW_SHARED);
- state->stoppoint = ControlFile->minRecoveryPoint;
- state->stoptli = ControlFile->minRecoveryPointTLI;
- LWLockRelease(ControlFileLock);
+ state->stoppoint = state->controlFile.minRecoveryPoint;
+ state->stoptli = state->controlFile.minRecoveryPointTLI;
}
else
{
diff --git a/src/backend/access/transam/xlogbackup.c b/src/backend/access/transam/xlogbackup.c
index 21d68133ae1..79559d78acb 100644
--- a/src/backend/access/transam/xlogbackup.c
+++ b/src/backend/access/transam/xlogbackup.c
@@ -16,6 +16,7 @@
#include "access/xlog.h"
#include "access/xlog_internal.h"
#include "access/xlogbackup.h"
+#include "common/base64.h"
/*
* Build contents for backup_label or backup history file.
@@ -76,6 +77,17 @@ build_backup_content(BackupState *state, bool ishistoryfile)
appendStringInfo(result, "STOP TIME: %s\n", stopstrfbuf);
appendStringInfo(result, "STOP TIMELINE: %u\n", state->stoptli);
}
+ /* Include a copy of control data */
+ else
+ {
+ char controlbuf[((sizeof(ControlFileData) + 2) / 3 * 4) + 1];
+
+ pg_b64_encode((char *)&state->controlFile, sizeof(ControlFileData),
+ controlbuf, sizeof(controlbuf) - 1);
+ controlbuf[sizeof(controlbuf) - 1] = '\0';
+
+ appendStringInfo(result, "CONTROL DATA: %s\n", controlbuf);
+ }
data = result->data;
pfree(result);
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index becc2bda62e..1abd9494bf0 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -43,6 +43,7 @@
#include "backup/basebackup.h"
#include "catalog/pg_control.h"
#include "commands/tablespace.h"
+#include "common/base64.h"
#include "common/file_utils.h"
#include "miscadmin.h"
#include "pgstat.h"
@@ -390,7 +391,8 @@ static void readRecoverySignalFile(void);
static void validateRecoveryParameters(void);
static bool read_backup_label(XLogRecPtr *checkPointLoc,
TimeLineID *backupLabelTLI,
- bool *backupEndRequired, bool *backupFromStandby);
+ bool *backupEndRequired, bool *backupFromStandby,
+ ControlFileData *ControlFile);
static bool read_tablespace_map(List **tablespaces);
static void xlogrecovery_redo(XLogReaderState *record, TimeLineID replayTLI);
@@ -610,7 +612,7 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
primary_image_masked = (char *) palloc(BLCKSZ);
if (read_backup_label(&CheckPointLoc, &CheckPointTLI, &backupEndRequired,
- &backupFromStandby))
+ &backupFromStandby, ControlFile))
{
List *tablespaces = NIL;
@@ -1167,7 +1169,8 @@ validateRecoveryParameters(void)
*/
static bool
read_backup_label(XLogRecPtr *checkPointLoc, TimeLineID *backupLabelTLI,
- bool *backupEndRequired, bool *backupFromStandby)
+ bool *backupEndRequired, bool *backupFromStandby,
+ ControlFileData *ControlFile)
{
char startxlogfilename[MAXFNAMELEN];
TimeLineID tli_from_walseg,
@@ -1180,6 +1183,7 @@ read_backup_label(XLogRecPtr *checkPointLoc, TimeLineID *backupLabelTLI,
char backuptime[128];
uint32 hi,
lo;
+ char controlB64Buf[684 + 1];
/* suppress possible uninitialized-variable warnings */
*checkPointLoc = InvalidXLogRecPtr;
@@ -1285,6 +1289,51 @@ read_backup_label(XLogRecPtr *checkPointLoc, TimeLineID *backupLabelTLI,
tli_from_file, BACKUP_LABEL_FILE)));
}
+ /*
+ * Read control data to be used to create pg_control. Control data may not
+ * exist if the backup was made with an older version, in which case the
+ * control file read from disk will be used.
+ */
+ if (fscanf(lfp, "CONTROL DATA: %684s\n", controlB64Buf) == 1)
+ {
+ ControlFileData controlBuf;
+ pg_crc32c crc;
+
+ /* Check that the base64 data is the correct length */
+ if (strlen(controlB64Buf) != (sizeof(ControlFileData) + 2) / 3 * 4)
+ ereport(FATAL,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("invalid data in file \"%s\"", BACKUP_LABEL_FILE),
+ errdetail("Control data expected %zu base64 characters.",
+ (sizeof(ControlFileData) + 2) / 3 * 4)));
+
+ /* Decode control file */
+ if (pg_b64_decode(controlB64Buf, strlen(controlB64Buf), (char *)&controlBuf,
+ sizeof(ControlFileData)) == -1)
+ ereport(FATAL,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("invalid data in file \"%s\"", BACKUP_LABEL_FILE),
+ errdetail("Control data contains invalid base64 encoding.")));
+
+ /* CRC check on stored control file */
+ INIT_CRC32C(crc);
+ COMP_CRC32C(crc, (char *)&controlBuf, offsetof(ControlFileData, crc));
+ FIN_CRC32C(crc);
+
+ if (!EQ_CRC32C(crc, controlBuf.crc))
+ ereport(FATAL,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("invalid data in file \"%s\"", BACKUP_LABEL_FILE),
+ errdetail("Control data contains invalid CRC.")));
+
+ ereport(DEBUG1,
+ (errmsg_internal("backup control data in file \"%s\"",
+ BACKUP_LABEL_FILE)));
+
+ /* Copy control data */
+ memcpy(ControlFile, &controlBuf, sizeof(ControlFileData));
+ }
+
if (ferror(lfp) || FreeFile(lfp))
ereport(FATAL,
(errcode_for_file_access(),
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index 7d025bcf382..f49dabf63ef 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -330,13 +330,7 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
bbsink_begin_archive(sink, "base.tar");
- /* In the main tar, include the backup_label first... */
- backup_label = build_backup_content(backup_state, false);
- sendFileWithContent(sink, BACKUP_LABEL_FILE,
- backup_label, &manifest);
- pfree(backup_label);
-
- /* Then the tablespace_map file, if required... */
+ /* Send the tablespace_map file, if required... */
if (opt->sendtblspcmapfile)
{
sendFileWithContent(sink, TABLESPACE_MAP,
@@ -348,7 +342,7 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
sendDir(sink, ".", 1, false, state.tablespaces,
sendtblspclinks, &manifest, NULL);
- /* ... and pg_control after everything else. */
+ /* ... and pg_control after everything but backup_label */
if (lstat(XLOG_CONTROL_FILE, &statbuf) != 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -356,6 +350,16 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
XLOG_CONTROL_FILE)));
sendFile(sink, XLOG_CONTROL_FILE, XLOG_CONTROL_FILE, &statbuf,
false, InvalidOid, &manifest, NULL);
+
+ /* End the backup before sending backup_label */
+ basebackup_progress_wait_wal_archive(&state);
+ do_pg_backup_stop(backup_state, !opt->nowait);
+
+ /* Last in the main tar, include backup_label */
+ backup_label = build_backup_content(backup_state, false);
+ sendFileWithContent(sink, BACKUP_LABEL_FILE,
+ backup_label, &manifest);
+ pfree(backup_label);
}
else
{
@@ -389,9 +393,6 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
}
}
- basebackup_progress_wait_wal_archive(&state);
- do_pg_backup_stop(backup_state, !opt->nowait);
-
endptr = backup_state->stoppoint;
endtli = backup_state->stoptli;
diff --git a/src/include/access/xlogbackup.h b/src/include/access/xlogbackup.h
index 1611358137b..bd53405670e 100644
--- a/src/include/access/xlogbackup.h
+++ b/src/include/access/xlogbackup.h
@@ -15,6 +15,7 @@
#define XLOG_BACKUP_H
#include "access/xlogdefs.h"
+#include "catalog/pg_control.h"
#include "pgtime.h"
/* Structure to hold backup state. */
@@ -33,6 +34,7 @@ typedef struct BackupState
XLogRecPtr stoppoint; /* backup stop WAL location */
TimeLineID stoptli; /* backup stop TLI */
pg_time_t stoptime; /* backup stop time */
+ ControlFileData controlFile; /* Copy of control data */
} BackupState;
extern char *build_backup_content(BackupState *state,
Attachments:
[text/plain] v01-pgcontrol-in-backup-label.patch (9.3K, ../../[email protected]/2-v01-pgcontrol-in-backup-label.patch)
download | inline diff:
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c0e4ca50899..e4a4478af75 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -74,6 +74,7 @@
#include "pg_trace.h"
#include "pgstat.h"
#include "port/atomics.h"
+#include "port/pg_crc32c.h"
#include "port/pg_iovec.h"
#include "postmaster/bgwriter.h"
#include "postmaster/startup.h"
@@ -8691,6 +8692,21 @@ do_pg_backup_stop(BackupState *state, bool waitforarchive)
"and should not be used. "
"Try taking another online backup.")));
+ /*
+ * Create a copy of control data to be stored in the backup label.
+ * Recalculate the CRC so recovery can validate the contents but do not
+ * bother with the timestamp since that will be applied before it is
+ * written by recovery.
+ */
+ LWLockAcquire(ControlFileLock, LW_SHARED);
+ state->controlFile = *ControlFile;
+ LWLockRelease(ControlFileLock);
+
+ INIT_CRC32C(state->controlFile.crc);
+ COMP_CRC32C(state->controlFile.crc, (char *)&state->controlFile,
+ offsetof(ControlFileData, crc));
+ FIN_CRC32C(state->controlFile.crc);
+
/*
* During recovery, we don't write an end-of-backup record. We assume that
* pg_control was backed up last and its minimum recovery point can be
@@ -8741,11 +8757,8 @@ do_pg_backup_stop(BackupState *state, bool waitforarchive)
"Enable full_page_writes and run CHECKPOINT on the primary, "
"and then try an online backup again.")));
-
- LWLockAcquire(ControlFileLock, LW_SHARED);
- state->stoppoint = ControlFile->minRecoveryPoint;
- state->stoptli = ControlFile->minRecoveryPointTLI;
- LWLockRelease(ControlFileLock);
+ state->stoppoint = state->controlFile.minRecoveryPoint;
+ state->stoptli = state->controlFile.minRecoveryPointTLI;
}
else
{
diff --git a/src/backend/access/transam/xlogbackup.c b/src/backend/access/transam/xlogbackup.c
index 21d68133ae1..79559d78acb 100644
--- a/src/backend/access/transam/xlogbackup.c
+++ b/src/backend/access/transam/xlogbackup.c
@@ -16,6 +16,7 @@
#include "access/xlog.h"
#include "access/xlog_internal.h"
#include "access/xlogbackup.h"
+#include "common/base64.h"
/*
* Build contents for backup_label or backup history file.
@@ -76,6 +77,17 @@ build_backup_content(BackupState *state, bool ishistoryfile)
appendStringInfo(result, "STOP TIME: %s\n", stopstrfbuf);
appendStringInfo(result, "STOP TIMELINE: %u\n", state->stoptli);
}
+ /* Include a copy of control data */
+ else
+ {
+ char controlbuf[((sizeof(ControlFileData) + 2) / 3 * 4) + 1];
+
+ pg_b64_encode((char *)&state->controlFile, sizeof(ControlFileData),
+ controlbuf, sizeof(controlbuf) - 1);
+ controlbuf[sizeof(controlbuf) - 1] = '\0';
+
+ appendStringInfo(result, "CONTROL DATA: %s\n", controlbuf);
+ }
data = result->data;
pfree(result);
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index becc2bda62e..1abd9494bf0 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -43,6 +43,7 @@
#include "backup/basebackup.h"
#include "catalog/pg_control.h"
#include "commands/tablespace.h"
+#include "common/base64.h"
#include "common/file_utils.h"
#include "miscadmin.h"
#include "pgstat.h"
@@ -390,7 +391,8 @@ static void readRecoverySignalFile(void);
static void validateRecoveryParameters(void);
static bool read_backup_label(XLogRecPtr *checkPointLoc,
TimeLineID *backupLabelTLI,
- bool *backupEndRequired, bool *backupFromStandby);
+ bool *backupEndRequired, bool *backupFromStandby,
+ ControlFileData *ControlFile);
static bool read_tablespace_map(List **tablespaces);
static void xlogrecovery_redo(XLogReaderState *record, TimeLineID replayTLI);
@@ -610,7 +612,7 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
primary_image_masked = (char *) palloc(BLCKSZ);
if (read_backup_label(&CheckPointLoc, &CheckPointTLI, &backupEndRequired,
- &backupFromStandby))
+ &backupFromStandby, ControlFile))
{
List *tablespaces = NIL;
@@ -1167,7 +1169,8 @@ validateRecoveryParameters(void)
*/
static bool
read_backup_label(XLogRecPtr *checkPointLoc, TimeLineID *backupLabelTLI,
- bool *backupEndRequired, bool *backupFromStandby)
+ bool *backupEndRequired, bool *backupFromStandby,
+ ControlFileData *ControlFile)
{
char startxlogfilename[MAXFNAMELEN];
TimeLineID tli_from_walseg,
@@ -1180,6 +1183,7 @@ read_backup_label(XLogRecPtr *checkPointLoc, TimeLineID *backupLabelTLI,
char backuptime[128];
uint32 hi,
lo;
+ char controlB64Buf[684 + 1];
/* suppress possible uninitialized-variable warnings */
*checkPointLoc = InvalidXLogRecPtr;
@@ -1285,6 +1289,51 @@ read_backup_label(XLogRecPtr *checkPointLoc, TimeLineID *backupLabelTLI,
tli_from_file, BACKUP_LABEL_FILE)));
}
+ /*
+ * Read control data to be used to create pg_control. Control data may not
+ * exist if the backup was made with an older version, in which case the
+ * control file read from disk will be used.
+ */
+ if (fscanf(lfp, "CONTROL DATA: %684s\n", controlB64Buf) == 1)
+ {
+ ControlFileData controlBuf;
+ pg_crc32c crc;
+
+ /* Check that the base64 data is the correct length */
+ if (strlen(controlB64Buf) != (sizeof(ControlFileData) + 2) / 3 * 4)
+ ereport(FATAL,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("invalid data in file \"%s\"", BACKUP_LABEL_FILE),
+ errdetail("Control data expected %zu base64 characters.",
+ (sizeof(ControlFileData) + 2) / 3 * 4)));
+
+ /* Decode control file */
+ if (pg_b64_decode(controlB64Buf, strlen(controlB64Buf), (char *)&controlBuf,
+ sizeof(ControlFileData)) == -1)
+ ereport(FATAL,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("invalid data in file \"%s\"", BACKUP_LABEL_FILE),
+ errdetail("Control data contains invalid base64 encoding.")));
+
+ /* CRC check on stored control file */
+ INIT_CRC32C(crc);
+ COMP_CRC32C(crc, (char *)&controlBuf, offsetof(ControlFileData, crc));
+ FIN_CRC32C(crc);
+
+ if (!EQ_CRC32C(crc, controlBuf.crc))
+ ereport(FATAL,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("invalid data in file \"%s\"", BACKUP_LABEL_FILE),
+ errdetail("Control data contains invalid CRC.")));
+
+ ereport(DEBUG1,
+ (errmsg_internal("backup control data in file \"%s\"",
+ BACKUP_LABEL_FILE)));
+
+ /* Copy control data */
+ memcpy(ControlFile, &controlBuf, sizeof(ControlFileData));
+ }
+
if (ferror(lfp) || FreeFile(lfp))
ereport(FATAL,
(errcode_for_file_access(),
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index 7d025bcf382..f49dabf63ef 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -330,13 +330,7 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
bbsink_begin_archive(sink, "base.tar");
- /* In the main tar, include the backup_label first... */
- backup_label = build_backup_content(backup_state, false);
- sendFileWithContent(sink, BACKUP_LABEL_FILE,
- backup_label, &manifest);
- pfree(backup_label);
-
- /* Then the tablespace_map file, if required... */
+ /* Send the tablespace_map file, if required... */
if (opt->sendtblspcmapfile)
{
sendFileWithContent(sink, TABLESPACE_MAP,
@@ -348,7 +342,7 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
sendDir(sink, ".", 1, false, state.tablespaces,
sendtblspclinks, &manifest, NULL);
- /* ... and pg_control after everything else. */
+ /* ... and pg_control after everything but backup_label */
if (lstat(XLOG_CONTROL_FILE, &statbuf) != 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -356,6 +350,16 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
XLOG_CONTROL_FILE)));
sendFile(sink, XLOG_CONTROL_FILE, XLOG_CONTROL_FILE, &statbuf,
false, InvalidOid, &manifest, NULL);
+
+ /* End the backup before sending backup_label */
+ basebackup_progress_wait_wal_archive(&state);
+ do_pg_backup_stop(backup_state, !opt->nowait);
+
+ /* Last in the main tar, include backup_label */
+ backup_label = build_backup_content(backup_state, false);
+ sendFileWithContent(sink, BACKUP_LABEL_FILE,
+ backup_label, &manifest);
+ pfree(backup_label);
}
else
{
@@ -389,9 +393,6 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
}
}
- basebackup_progress_wait_wal_archive(&state);
- do_pg_backup_stop(backup_state, !opt->nowait);
-
endptr = backup_state->stoppoint;
endtli = backup_state->stoptli;
diff --git a/src/include/access/xlogbackup.h b/src/include/access/xlogbackup.h
index 1611358137b..bd53405670e 100644
--- a/src/include/access/xlogbackup.h
+++ b/src/include/access/xlogbackup.h
@@ -15,6 +15,7 @@
#define XLOG_BACKUP_H
#include "access/xlogdefs.h"
+#include "catalog/pg_control.h"
#include "pgtime.h"
/* Structure to hold backup state. */
@@ -33,6 +34,7 @@ typedef struct BackupState
XLogRecPtr stoppoint; /* backup stop WAL location */
TimeLineID stoptli; /* backup stop TLI */
pg_time_t stoptime; /* backup stop time */
+ ControlFileData controlFile; /* Copy of control data */
} BackupState;
extern char *build_backup_content(BackupState *state,
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: The danger of deleting backup_label
@ 2023-10-16 14:55 Robert Haas <[email protected]>
parent: David Steele <[email protected]>
1 sibling, 1 reply; 18+ messages in thread
From: Robert Haas @ 2023-10-16 14:55 UTC (permalink / raw)
To: David Steele <[email protected]>; +Cc: Thomas Munro <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers; Stephen Frost <[email protected]>
On Sat, Oct 14, 2023 at 11:33 AM David Steele <[email protected]> wrote:
> All of this is fixable in HEAD, but seems incredibly dangerous to back
> patch. Even so, I have attached the patch in case somebody sees an
> opportunity that I do not.
I really do not think we should be even thinking about back-patching
something like this. It's clearly not a bug fix, although I'm sure
that someone can try to characterize it that way, if they want to make
the well-worn argument that any behavior they don't like is a bug. But
that's a pretty lame argument. Usage errors on the part of users are
not bugs, even if we've coded the software in such a way as to make
those errors more likely.
I think what we ought to be talking about is whether a change like
this is a good idea even in master. I don't think it's a terrible
idea, but I'm also not sure that it's a good idea. The problem is that
if you're doing the right thing with your backup_label, then this is
unnecessary, and if you're doing the wrong thing, then why should you
do the right thing about this? I mean, admittedly you can't just
ignore a fatal error, but I think people will just run pg_resetwal,
which is even worse than starting from the wrong checkpoint. I feel
like in cases where a customer I'm working with has a bad backup,
their entire focus is on doing something to that backup to get a
running system back, whatever it takes. It's already too late at that
point to fix the backup procedure - they only have the backups they
have. You could hope people would do test restores before disaster
strikes, but people who are that prepared are probably running a real
backup tool and will never have this problem in the first place.
Perhaps that's all too pessimistic. I don't know. Certainly, other
people can have experiences that are different than mine. But I feel
like I struggle to think of a case where this would have prevented a
bad outcome, and that makes me wonder whether it's really a good idea
to complicate the system.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: The danger of deleting backup_label
@ 2023-10-16 15:45 David Steele <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: David Steele @ 2023-10-16 15:45 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Thomas Munro <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers; Stephen Frost <[email protected]>
On 10/16/23 10:55, Robert Haas wrote:
> On Sat, Oct 14, 2023 at 11:33 AM David Steele <[email protected]> wrote:
>> All of this is fixable in HEAD, but seems incredibly dangerous to back
>> patch. Even so, I have attached the patch in case somebody sees an
>> opportunity that I do not.
>
> I really do not think we should be even thinking about back-patching
> something like this. It's clearly not a bug fix, although I'm sure
> that someone can try to characterize it that way, if they want to make
> the well-worn argument that any behavior they don't like is a bug. But
> that's a pretty lame argument. Usage errors on the part of users are
> not bugs, even if we've coded the software in such a way as to make
> those errors more likely.
Hmmm, the reason to back patch this is that it would fix [1], which sure
looks like a problem to me even if it is not a "bug". We can certainly
require backup software to retry pg_control until the checksum is valid
but that seems like a pretty big ask, even considering how complicated
backup is.
I investigated this as a solution to [1] because it seemed like a better
solution that what we have in [1]. But once I got into the weeds it was
obvious that this wasn't going to be something we could back patch.
> I think what we ought to be talking about is whether a change like
> this is a good idea even in master. I don't think it's a terrible
> idea, but I'm also not sure that it's a good idea. The problem is that
> if you're doing the right thing with your backup_label, then this is
> unnecessary, and if you're doing the wrong thing, then why should you
> do the right thing about this?
First and foremost, this solves the problem of pg_control being torn
when read by the backup software. It can't be torn if it is not there.
There are also other advantages -- we can massage pg_control before
writing it back out. This already happens, but before that happens there
is a copy of the (prior) running pg_control there that can mess up the
process.
> I mean, admittedly you can't just
> ignore a fatal error, but I think people will just run pg_resetwal,
> which is even worse than starting from the wrong checkpoint.
If you start from the last checkpoint (which is what will generally be
stored in pg_control) then the effect is pretty similar.
> I feel
> like in cases where a customer I'm working with has a bad backup,
> their entire focus is on doing something to that backup to get a
> running system back, whatever it takes. It's already too late at that
> point to fix the backup procedure - they only have the backups they
> have. You could hope people would do test restores before disaster
> strikes, but people who are that prepared are probably running a real
> backup tool and will never have this problem in the first place.
Right now the user can remove backup_label and get a "successful"
restore and not realize that they have just corrupted their cluster.
This is independent of the backup/restore tool doing all the right things.
My goal here is to narrow the options to try and make it so there is
*one* valid procedure that will work. For this patch the idea is that
they *must* start Postgres to get a valid pg_control from the
backup_label. Any other action leads to a fatal error.
Note that the current patch is very WIP and does not actually do
everything I'm talking about here. I was just trying to see if it could
be used to solve the problem in [1]. It can't.
> Perhaps that's all too pessimistic. I don't know. Certainly, other
> people can have experiences that are different than mine. But I feel
> like I struggle to think of a case where this would have prevented a
> bad outcome, and that makes me wonder whether it's really a good idea
> to complicate the system.
I'm specifically addressing cases like those that came up (twice!) in
[2]. This is the main place I see people stumbling these days. If even
hackers can make this mistake then we should do better.
Regards,
-David
[1]
https://www.postgresql.org/message-id/20221123014224.xisi44byq3cf5psi%40awork3.anarazel.de
[2] [1]
https://www.postgresql.org/message-id/flat/CAM_vCudkSjr7NsNKSdjwtfAm9dbzepY6beZ5DP177POKy8%3D2aw%40m...
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: The danger of deleting backup_label
@ 2023-10-16 16:25 Robert Haas <[email protected]>
parent: David Steele <[email protected]>
0 siblings, 2 replies; 18+ messages in thread
From: Robert Haas @ 2023-10-16 16:25 UTC (permalink / raw)
To: David Steele <[email protected]>; +Cc: Thomas Munro <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers; Stephen Frost <[email protected]>
On Mon, Oct 16, 2023 at 11:45 AM David Steele <[email protected]> wrote:
> Hmmm, the reason to back patch this is that it would fix [1], which sure
> looks like a problem to me even if it is not a "bug". We can certainly
> require backup software to retry pg_control until the checksum is valid
> but that seems like a pretty big ask, even considering how complicated
> backup is.
That seems like a problem with pg_control not being written atomically
when the standby server is updating it during recovery, rather than a
problem with backup_label not being used at the start of recovery.
Unless I'm confused.
> If you start from the last checkpoint (which is what will generally be
> stored in pg_control) then the effect is pretty similar.
If the backup didn't span a checkpoint, then restoring from the one in
pg_control actually works fine. Not that I'm encouraging that. But if
you replay WAL from the control file, you at least get the last
checkpoint's worth of WAL; if you use pg_resetwal, you get nothing.
I don't really want to get hung up on this though. My main point here
is that I have trouble believing that an error after you've already
screwed up your backup helps much. I think what we need is to make it
less likely that you will screw up your backup in the first place.
> Right now the user can remove backup_label and get a "successful"
> restore and not realize that they have just corrupted their cluster.
> This is independent of the backup/restore tool doing all the right things.
I don't think it's independent of that at all.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: The danger of deleting backup_label
@ 2023-10-16 17:00 David Steele <[email protected]>
parent: Robert Haas <[email protected]>
1 sibling, 1 reply; 18+ messages in thread
From: David Steele @ 2023-10-16 17:00 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Thomas Munro <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers; Stephen Frost <[email protected]>
On 10/16/23 12:25, Robert Haas wrote:
> On Mon, Oct 16, 2023 at 11:45 AM David Steele <[email protected]> wrote:
>> Hmmm, the reason to back patch this is that it would fix [1], which sure
>> looks like a problem to me even if it is not a "bug". We can certainly
>> require backup software to retry pg_control until the checksum is valid
>> but that seems like a pretty big ask, even considering how complicated
>> backup is.
>
> That seems like a problem with pg_control not being written atomically
> when the standby server is updating it during recovery, rather than a
> problem with backup_label not being used at the start of recovery.
> Unless I'm confused.
You are not confused. But the fact that it practically can be read as
torn at all on the standby is a function of how rapidly it is being
written to update min recovery point. Writing it atomically via a temp
file may affect performance in this area, but only during the backup,
which may cause recovery to run more slowly during a backup.
I don't have proof of this because I don't have any hosts large enough
to test the theory.
>> Right now the user can remove backup_label and get a "successful"
>> restore and not realize that they have just corrupted their cluster.
>> This is independent of the backup/restore tool doing all the right things.
>
> I don't think it's independent of that at all.
I think it is. Imagine the user does backup/recovery using their
favorite tool and everything is fine. But due to some misconfiguration
or a problem in the WAL archive, they get this error:
FATAL: could not locate required checkpoint record
2023-10-16 16:42:50.132 UTC HINT: If you are restoring from a backup,
touch "/home/dev/test/data/recovery.signal" or
"/home/dev/test/data/standby.signal" and add required recovery options.
If you are not restoring from a backup, try removing the file
"/home/dev/test/data/backup_label".
Be careful: removing "/home/dev/test/data/backup_label" will
result in a corrupt cluster if restoring from a backup.
I did this by setting restore_command=false, but it could just as easily
be the actual restore command that returns false due to a variety of
reasons. The user has no idea what "could not locate required checkpoint
record" means and if there is enough automation they may not even
realize they just restored from a backup.
After some agonizing (we hope) they decide to delete backup_label and,
wow, it just works! So now they merrily go on their way with a corrupted
cluster. They also remember for the next time that deleting backup_label
is definitely a good procedure.
The idea behind this patch is that deleting backup_label would produce a
hard error because pg_control would be missing as well (if the backup
software did its job). If both pg_control and backup_label are present
(but pg_control has not been loaded with the contents of backup_label,
i.e. it is the running copy from the backup cluster) we can also error.
It's not perfect, because they could backup (or restore) pg_control but
not backup_label, but we are narrowing the cases where something can go
wrong and they have silent corruption, especially if their
backup/restore software follows the directions.
Regards,
-David
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: The danger of deleting backup_label
@ 2023-10-16 19:06 Robert Haas <[email protected]>
parent: David Steele <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Robert Haas @ 2023-10-16 19:06 UTC (permalink / raw)
To: David Steele <[email protected]>; +Cc: Thomas Munro <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers; Stephen Frost <[email protected]>
On Mon, Oct 16, 2023 at 1:00 PM David Steele <[email protected]> wrote:
> After some agonizing (we hope) they decide to delete backup_label and,
> wow, it just works! So now they merrily go on their way with a corrupted
> cluster. They also remember for the next time that deleting backup_label
> is definitely a good procedure.
>
> The idea behind this patch is that deleting backup_label would produce a
> hard error because pg_control would be missing as well (if the backup
> software did its job). If both pg_control and backup_label are present
> (but pg_control has not been loaded with the contents of backup_label,
> i.e. it is the running copy from the backup cluster) we can also error.
I mean, I think we're just going in circles, here. I did and do
understand, but I didn't and don't agree. You're hypothesizing a user
who is willing to do ONE thing that they shouldn't do during backup
restoration (namely, remove backup_label) but who won't be willing to
do a SECOND thing that they shouldn't do during backup restoration
(namely, run pg_resetwal). In my experience, users who are willing to
corrupt their database don't typically limit themselves to one bad
decision, and therefore I doubt that this proposal delivers enough
value to justify the complexity.
I understand that you feel differently, and that's fine, but I don't
think our disagreement here stems from me being confused. I just ...
don't agree.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: The danger of deleting backup_label
@ 2023-10-16 23:38 Michael Paquier <[email protected]>
parent: Robert Haas <[email protected]>
1 sibling, 0 replies; 18+ messages in thread
From: Michael Paquier @ 2023-10-16 23:38 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: David Steele <[email protected]>; Thomas Munro <[email protected]>; pgsql-hackers; Stephen Frost <[email protected]>
On Mon, Oct 16, 2023 at 12:25:59PM -0400, Robert Haas wrote:
> On Mon, Oct 16, 2023 at 11:45 AM David Steele <[email protected]> wrote:
> > If you start from the last checkpoint (which is what will generally be
> > stored in pg_control) then the effect is pretty similar.
>
> If the backup didn't span a checkpoint, then restoring from the one in
> pg_control actually works fine. Not that I'm encouraging that. But if
> you replay WAL from the control file, you at least get the last
> checkpoint's worth of WAL; if you use pg_resetwal, you get nothing.
There's no guarantee that the backend didn't spawn an extra checkpoint
while a base backup was taken, either, because we don't fail hard at
the end of the BASE_BACKUP code paths if the redo LSN has been updated
since the beginning of a BASE_BACKUP. So that's really *never* do it
except if you like silent corruptions.
> I don't really want to get hung up on this though. My main point here
> is that I have trouble believing that an error after you've already
> screwed up your backup helps much. I think what we need is to make it
> less likely that you will screw up your backup in the first place.
Yeah.. Now what's the best user experience? Is it better for a base
backup to fail and have a user retry? Or is it better to have the
backend-side backup logic do what we think is safer? The former
(likely with a REDO check or similar), will likely never work on large
instances, while users will likely always find ways to screw up base
backups taken by latter methods. A third approach is to put more
careful checks at restore time, and the latter helps a lot here.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: The danger of deleting backup_label
@ 2023-10-16 23:45 David Steele <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: David Steele @ 2023-10-16 23:45 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Thomas Munro <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers; Stephen Frost <[email protected]>
On 10/16/23 15:06, Robert Haas wrote:
> On Mon, Oct 16, 2023 at 1:00 PM David Steele <[email protected]> wrote:
>> After some agonizing (we hope) they decide to delete backup_label and,
>> wow, it just works! So now they merrily go on their way with a corrupted
>> cluster. They also remember for the next time that deleting backup_label
>> is definitely a good procedure.
>>
>> The idea behind this patch is that deleting backup_label would produce a
>> hard error because pg_control would be missing as well (if the backup
>> software did its job). If both pg_control and backup_label are present
>> (but pg_control has not been loaded with the contents of backup_label,
>> i.e. it is the running copy from the backup cluster) we can also error.
>
> I mean, I think we're just going in circles, here. I did and do
> understand, but I didn't and don't agree. You're hypothesizing a user
> who is willing to do ONE thing that they shouldn't do during backup
> restoration (namely, remove backup_label) but who won't be willing to
> do a SECOND thing that they shouldn't do during backup restoration
> (namely, run pg_resetwal).
In my experience the first case is much more likely than the second.
Your experience may vary.
Anyway, I think they are pretty different. Deleting backup label appears
to give a perfectly valid restore. Running pg_resetwal is more clearly
(I think) the nuclear solution.
> I understand that you feel differently, and that's fine, but I don't
> think our disagreement here stems from me being confused. I just ...
> don't agree.
Fair enough, we don't agree.
Regards,
-David
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: The danger of deleting backup_label
@ 2023-10-17 19:17 Stephen Frost <[email protected]>
parent: David Steele <[email protected]>
0 siblings, 0 replies; 18+ messages in thread
From: Stephen Frost @ 2023-10-17 19:17 UTC (permalink / raw)
To: David Steele <[email protected]>; +Cc: Robert Haas <[email protected]>; Thomas Munro <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers
Greetings,
* David Steele ([email protected]) wrote:
> On 10/16/23 15:06, Robert Haas wrote:
> > On Mon, Oct 16, 2023 at 1:00 PM David Steele <[email protected]> wrote:
> > > After some agonizing (we hope) they decide to delete backup_label and,
> > > wow, it just works! So now they merrily go on their way with a corrupted
> > > cluster. They also remember for the next time that deleting backup_label
> > > is definitely a good procedure.
> > >
> > > The idea behind this patch is that deleting backup_label would produce a
> > > hard error because pg_control would be missing as well (if the backup
> > > software did its job). If both pg_control and backup_label are present
> > > (but pg_control has not been loaded with the contents of backup_label,
> > > i.e. it is the running copy from the backup cluster) we can also error.
> >
> > I mean, I think we're just going in circles, here. I did and do
> > understand, but I didn't and don't agree. You're hypothesizing a user
> > who is willing to do ONE thing that they shouldn't do during backup
> > restoration (namely, remove backup_label) but who won't be willing to
> > do a SECOND thing that they shouldn't do during backup restoration
> > (namely, run pg_resetwal).
>
> In my experience the first case is much more likely than the second. Your
> experience may vary.
My experience (though perhaps not a surprise) mirrors David's.
> Anyway, I think they are pretty different. Deleting backup label appears to
> give a perfectly valid restore. Running pg_resetwal is more clearly (I
> think) the nuclear solution.
Right, and a delete of backup_label is just an 'rm' that folks may think
"oh, this is just some leftover thing that isn't actually needed".
OTOH, pg_resetwal has an online documentation page and a man page that's
very clear that it's only to be used as a last resort (perhaps we should
pull that into the --help output too..?). It's also pretty clear that
pg_resetwal is actually changing things about the cluster while nuking
backup_label doesn't *seem* to be in that same category, even though we
all know it is because it's needed once recovery begins.
I'd also put out there that while people don't do restore testing
nearly as much as they should, they tend to at _least_ try to do it once
after taking their first backup and if that fails then they try to figure
out why and what they're not doing right.
Thanks,
Stephen
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: The danger of deleting backup_label
@ 2023-10-17 20:16 David Steele <[email protected]>
parent: David Steele <[email protected]>
1 sibling, 1 reply; 18+ messages in thread
From: David Steele @ 2023-10-17 20:16 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Michael Paquier <[email protected]>; pgsql-hackers; Stephen Frost <[email protected]>
On 10/14/23 11:30, David Steele wrote:
> On 10/12/23 10:19, David Steele wrote:
>> On 10/11/23 18:10, Thomas Munro wrote:
>>
>>> As Stephen mentioned[1], we could perhaps also complain if both backup
>>> label and control file exist, and then hint that the user should
>>> remove the *control file* (not the backup label!). I had originally
>>> suggested we would just overwrite the control file, but by explicitly
>>> complaining about it we would also bring the matter to tool/script
>>> authors' attention, ie that they shouldn't be backing that file up, or
>>> should be removing it in a later step if they copy everything. He
>>> also mentions that there doesn't seem to be anything stopping us from
>>> back-patching changes to the backup label contents if we go this way.
>>> I don't have a strong opinion on that and we could leave the question
>>> for later.
>>
>> I'm worried about the possibility of back patching this unless the
>> solution comes out to be simpler than I think and that rarely comes to
>> pass. Surely throwing errors on something that is currently valid
>> (i.e. backup_label and pg_control both present).
>>
>> But perhaps there is a simpler, acceptable solution we could back
>> patch (transparent to all parties except Postgres) and then a more
>> advanced solution we could go forward with.
>>
>> I guess I had better get busy on this.
>
> Attached is a very POC attempt at something along these lines that could
> be back patched. I stopped when I realized excluding pg_control from the
> backup is a necessity to make this work (else we still could end up with
> a torn copy of pg_control even if there is a good copy elsewhere). To
> enumerate the back patch issues as I see them:
Given that the above can't be back patched, I'm thinking we don't need
backup_label at all going forward. We just write the values we need for
recovery into pg_control and return *that* from pg_backup_stop() and
tell the user to store it with their backup. We already have "These
files are vital to the backup working and must be written byte for byte
without modification, which may require opening the file in binary
mode." in the documentation so dealing with pg_control should not be a
problem. pg_control also has a CRC so we will know if it gets munged.
It doesn't really matter where/how they store pg_control as long as it
is written back into PGDATA before the cluster starts. If
backupEndRequired, etc., are set appropriately then recovery will do the
right thing when it starts, just as now if PG crashes after it has
renamed backup_label but before recovery to consistency has completed.
We can still enforce the presence of recovery.signal by checking
backupEndRequired if that's something we want but it seems like
backupEndRequired would be enough. I'm fine either way.
Another thing we can do here is make backup from standby easier. The
user won't need to worry about *when* pg_control is copied. We can just
write the ideal min recovery point into pg_control.
Any informational data currently in backup_label can be returned as
columns (like the start/end lsn is now).
This makes the patch much less invasive and while it definitely,
absolutely cannot be back patched, it seems like a good way forward.
This is the direction I'm planning to work on patch-wise but I'd like to
hear people's thoughts.
Regards,
-David
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: The danger of deleting backup_label
@ 2023-10-18 02:13 Kyotaro Horiguchi <[email protected]>
parent: David Steele <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Kyotaro Horiguchi @ 2023-10-18 02:13 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; pgsql-hackers; [email protected]
At Tue, 17 Oct 2023 16:16:42 -0400, David Steele <[email protected]> wrote in
> Given that the above can't be back patched, I'm thinking we don't need
> backup_label at all going forward. We just write the values we need
> for recovery into pg_control and return *that* from pg_backup_stop()
> and tell the user to store it with their backup. We already have
> "These files are vital to the backup working and must be written byte
> for byte without modification, which may require opening the file in
> binary mode." in the documentation so dealing with pg_control should
> not be a problem. pg_control also has a CRC so we will know if it gets
> munged.
I'm somewhat perplexed regarding the objective of this thread.
This thread began with the intent of preventing users from removing
the backup_label from a backup. At the beginning, the proposal aimed
to achieve this by injecting an invalid value to pg_control file
located in the generated backup. However, this (and previous) proposal
seems to deviate from that initial objective. It now eliminates the
need to be concerned about the pg_control version that is coped into
the generated backup. However, if someone removes the backup_label
from a backup, the initial concerns could still surface.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: The danger of deleting backup_label
@ 2023-10-18 14:38 David Steele <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 18+ messages in thread
From: David Steele @ 2023-10-18 14:38 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; pgsql-hackers; [email protected]
On 10/17/23 22:13, Kyotaro Horiguchi wrote:
> At Tue, 17 Oct 2023 16:16:42 -0400, David Steele <[email protected]> wrote in
>> Given that the above can't be back patched, I'm thinking we don't need
>> backup_label at all going forward. We just write the values we need
>> for recovery into pg_control and return *that* from pg_backup_stop()
>> and tell the user to store it with their backup. We already have
>> "These files are vital to the backup working and must be written byte
>> for byte without modification, which may require opening the file in
>> binary mode." in the documentation so dealing with pg_control should
>> not be a problem. pg_control also has a CRC so we will know if it gets
>> munged.
>
> I'm somewhat perplexed regarding the objective of this thread.
>
> This thread began with the intent of preventing users from removing
> the backup_label from a backup. At the beginning, the proposal aimed
> to achieve this by injecting an invalid value to pg_control file
> located in the generated backup. However, this (and previous) proposal
> seems to deviate from that initial objective. It now eliminates the
> need to be concerned about the pg_control version that is coped into
> the generated backup. However, if someone removes the backup_label
> from a backup, the initial concerns could still surface.
Yeah, the discussion has moved around quite a bit, but the goal remains
the same, to make Postgres error when it does not have the information
it needs to proceed with recovery. Right now if you delete backup_label
recovery appears to complete successfully, silently corrupting the database.
In the proposal as it stands now there would be no backup_label at all,
so no danger of removing it.
We have also gotten a bit sidetracked by our hope to use this proposal
to address torn reads of pg_control during the backup, at least in HEAD.
Regards,
-David
^ permalink raw reply [nested|flat] 18+ messages in thread
end of thread, other threads:[~2023-10-18 14:38 UTC | newest]
Thread overview: 18+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-06-06 22:42 [PATCH v2 1/3] Allow CREATE INDEX CONCURRENTLY on partitioned table Justin Pryzby <[email protected]>
2023-10-10 21:06 Re: The danger of deleting backup_label David Steele <[email protected]>
2023-10-11 22:10 ` Re: The danger of deleting backup_label Thomas Munro <[email protected]>
2023-10-12 14:19 ` Re: The danger of deleting backup_label David Steele <[email protected]>
2023-10-14 15:30 ` Re: The danger of deleting backup_label David Steele <[email protected]>
2023-10-16 14:55 ` Re: The danger of deleting backup_label Robert Haas <[email protected]>
2023-10-16 15:45 ` Re: The danger of deleting backup_label David Steele <[email protected]>
2023-10-16 16:25 ` Re: The danger of deleting backup_label Robert Haas <[email protected]>
2023-10-16 17:00 ` Re: The danger of deleting backup_label David Steele <[email protected]>
2023-10-16 19:06 ` Re: The danger of deleting backup_label Robert Haas <[email protected]>
2023-10-16 23:45 ` Re: The danger of deleting backup_label David Steele <[email protected]>
2023-10-17 19:17 ` Re: The danger of deleting backup_label Stephen Frost <[email protected]>
2023-10-16 23:38 ` Re: The danger of deleting backup_label Michael Paquier <[email protected]>
2023-10-17 20:16 ` Re: The danger of deleting backup_label David Steele <[email protected]>
2023-10-18 02:13 ` Re: The danger of deleting backup_label Kyotaro Horiguchi <[email protected]>
2023-10-18 14:38 ` Re: The danger of deleting backup_label David Steele <[email protected]>
2023-10-11 22:22 ` Re: The danger of deleting backup_label Michael Paquier <[email protected]>
2023-10-12 14:25 ` Re: The danger of deleting backup_label David Steele <[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