public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v48 3/3] Replace assuming a composite object on a scalar 5+ messages / 4 participants [nested] [flat]
* [PATCH v48 3/3] Replace assuming a composite object on a scalar @ 2021-01-20 15:53 Dmitrii Dolgov <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Dmitrii Dolgov @ 2021-01-20 15:53 UTC (permalink / raw) For jsonb subscripting assignment it could happen that the provided path assumes an object or an array at some level, but the source jsonb has a scalar value there. Originally the update operation will be skipped and no message will be shown that nothing happened. Return an error to indicate such situations. --- doc/src/sgml/json.sgml | 19 +++++++++++++++++++ src/backend/utils/adt/jsonfuncs.c | 27 +++++++++++++++++++++++++++ src/test/regress/expected/jsonb.out | 27 +++++++++++++++++++++++++++ src/test/regress/sql/jsonb.sql | 17 +++++++++++++++++ 4 files changed, 90 insertions(+) diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml index 07bd19f974..deeb9e66e0 100644 --- a/doc/src/sgml/json.sgml +++ b/doc/src/sgml/json.sgml @@ -614,8 +614,23 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @> '{"tags": ["qu The result of a subscripting expression is always of the jsonb data type. </para> + <para> + <command>UPDATE</command> statements may use subscripting in the + <literal>SET</literal> clause to modify <type>jsonb</type> values. Object + values being traversed must exist as specified by the subscript path. For + instance, the path <literal>val['a']['b']['c']</literal> assumes that + <literal>val</literal>, <literal>val['a']</literal>, and <literal>val['a']['b']</literal> + are all objects in every record being updated (<literal>val['a']['b']</literal> + may or may not contain a field named <literal>c</literal>, as long as it's an + object). If any individual <literal>val</literal>, <literal>val['a']</literal>, + or <literal>val['a']['b']</literal> is a non-object such as a string, a number, + or <literal>NULL</literal>, an error is raised even if other values do conform. + Array values are not subject to this restriction, as detailed below. + </para> + <para> An example of subscripting syntax: + <programlisting> -- Extract object value by key @@ -631,6 +646,10 @@ SELECT ('[1, "2", null]'::jsonb)[1]; -- value must be of the jsonb type as well UPDATE table_name SET jsonb_field['key'] = '1'; +-- This will raise an error if any record's jsonb_field['a']['b'] is something +-- other than an object. For example, the value {"a": 1} has no 'b' key. +UPDATE table_name SET jsonb_field['a']['b']['c'] = '1'; + -- Filter records using a WHERE clause with subscripting. Since the result of -- subscripting is jsonb, the value we compare it against must also be jsonb. -- The double quotes make "value" also a valid jsonb string. diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index f14f6c3191..9138b7950e 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -4926,6 +4926,20 @@ setPath(JsonbIterator **it, Datum *path_elems, switch (r) { case WJB_BEGIN_ARRAY: + /* + * If instructed complain about attempts to replace whithin a raw + * scalar value. This happens even when current level is equal to + * path_len, because the last path key should also correspond to an + * object or an array, not raw scalar. + */ + if ((op_type & JB_PATH_FILL_GAPS) && (level <= path_len - 1) && + v.val.array.rawScalar) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("cannot replace existing key"), + errdetail("The path assumes key is a composite object, " + "but it is a scalar value."))); + (void) pushJsonbValue(st, r, NULL); setPathArray(it, path_elems, path_nulls, path_len, st, level, newval, v.val.array.nElems, op_type); @@ -4943,6 +4957,19 @@ setPath(JsonbIterator **it, Datum *path_elems, break; case WJB_ELEM: case WJB_VALUE: + /* + * If instructed complain about attempts to replace whithin a + * scalar value. This happens even when current level is equal to + * path_len, because the last path key should also correspond to an + * object or an array, not an element or value. + */ + if ((op_type & JB_PATH_FILL_GAPS) && (level <= path_len - 1)) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("cannot replace existing key"), + errdetail("The path assumes key is a composite object, " + "but it is a scalar value."))); + res = pushJsonbValue(st, r, &v); break; default: diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expected/jsonb.out index 5b5510c4fd..cf0ce0e44c 100644 --- a/src/test/regress/expected/jsonb.out +++ b/src/test/regress/expected/jsonb.out @@ -5134,6 +5134,33 @@ select * from test_jsonb_subscript; 1 | {"a": [null, {"c": [null, null, 1]}]} (1 row) +-- trying replace assuming a composite object, but it's an element or a value +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{"a": 1}'); +update test_jsonb_subscript set test_json['a']['b'] = '1'; +ERROR: cannot replace existing key +DETAIL: The path assumes key is a composite object, but it is a scalar value. +update test_jsonb_subscript set test_json['a']['b']['c'] = '1'; +ERROR: cannot replace existing key +DETAIL: The path assumes key is a composite object, but it is a scalar value. +update test_jsonb_subscript set test_json['a'][0] = '1'; +ERROR: cannot replace existing key +DETAIL: The path assumes key is a composite object, but it is a scalar value. +update test_jsonb_subscript set test_json['a'][0]['c'] = '1'; +ERROR: cannot replace existing key +DETAIL: The path assumes key is a composite object, but it is a scalar value. +update test_jsonb_subscript set test_json['a'][0][0] = '1'; +ERROR: cannot replace existing key +DETAIL: The path assumes key is a composite object, but it is a scalar value. +-- trying replace assuming a composite object, but it's a raw scalar +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, 'null'); +update test_jsonb_subscript set test_json[0] = '1'; +ERROR: cannot replace existing key +DETAIL: The path assumes key is a composite object, but it is a scalar value. +update test_jsonb_subscript set test_json[0][0] = '1'; +ERROR: cannot replace existing key +DETAIL: The path assumes key is a composite object, but it is a scalar value. -- jsonb to tsvector select to_tsvector('{"a": "aaa bbb ddd ccc", "b": ["eee fff ggg"], "c": {"d": "hhh iii"}}'::jsonb); to_tsvector diff --git a/src/test/regress/sql/jsonb.sql b/src/test/regress/sql/jsonb.sql index 0320db0ea4..1a9d21741f 100644 --- a/src/test/regress/sql/jsonb.sql +++ b/src/test/regress/sql/jsonb.sql @@ -1371,6 +1371,23 @@ insert into test_jsonb_subscript values (1, '{"a": []}'); update test_jsonb_subscript set test_json['a'][1]['c'][2] = '1'; select * from test_jsonb_subscript; +-- trying replace assuming a composite object, but it's an element or a value + +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{"a": 1}'); +update test_jsonb_subscript set test_json['a']['b'] = '1'; +update test_jsonb_subscript set test_json['a']['b']['c'] = '1'; +update test_jsonb_subscript set test_json['a'][0] = '1'; +update test_jsonb_subscript set test_json['a'][0]['c'] = '1'; +update test_jsonb_subscript set test_json['a'][0][0] = '1'; + +-- trying replace assuming a composite object, but it's a raw scalar + +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, 'null'); +update test_jsonb_subscript set test_json[0] = '1'; +update test_jsonb_subscript set test_json[0][0] = '1'; + -- jsonb to tsvector select to_tsvector('{"a": "aaa bbb ddd ccc", "b": ["eee fff ggg"], "c": {"d": "hhh iii"}}'::jsonb); -- 2.21.0 --m4t3wpskod5t4yye-- ^ permalink raw reply [nested|flat] 5+ messages in thread
* Requiring recovery.signal or standby.signal when recovering with a backup_label @ 2023-03-10 06:59 Michael Paquier <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Michael Paquier @ 2023-03-10 06:59 UTC (permalink / raw) To: Postgres hackers <[email protected]> Hi all, This is a follow-up of the point I have made a few weeks ago on this thread of pgsql-bugs about $subject: https://www.postgresql.org/message-id/Y/Q/[email protected] https://www.postgresql.org/message-id/Y/v0c+3W89NBT/[email protected] Here is a short summary of what I think is incorrect, and what I'd like to do to improve things moving forward, this pointing to a simple solution.. While looking at the so-said thread, I have dug into the recovery code to see what looks like an incorrect assumption behind the two boolean flags named ArchiveRecoveryRequested and InArchiveRecovery that we have in xlogrecovery.c to control the behavior of archive recovery in the startup process. For information, as of HEAD, these two are described as follows: /* * When ArchiveRecoveryRequested is set, archive recovery was requested, * ie. signal files were present. When InArchiveRecovery is set, we are * currently recovering using offline XLOG archives. These variables are only * valid in the startup process. * * When ArchiveRecoveryRequested is true, but InArchiveRecovery is false, we're * currently performing crash recovery using only XLOG files in pg_wal, but * will switch to using offline XLOG archives as soon as we reach the end of * WAL in pg_wal. */ bool ArchiveRecoveryRequested = false; bool InArchiveRecovery = false; When you read this text alone, its assumptions are simple. When the startup process finds a recovery.signal or a standby.signal, we switch ArchiveRecoveryRequested to true. If there is a standby.signal, InArchiveRecovery would be immediately set to true before beginning the redo loop. If we begin redo with a recovery.signal, ArchiveRecoveryRequested = true and InArchiveRecovery = false, crash recovery happens first, consuming all the WAL in pg_wal/, then we'd move on with archive recovery. Now comes the problem of the other thread, which is what happens when you use a backup_label *without* a recovery.signal or a standby.signal. In this case, as currently coded, it is possible to enforce ArchiveRecoveryRequested = false and later InArchiveRecovery = true. Not setting ArchiveRecoveryRequested has a couple of undesired effect. First, this skips some initialization steps that may be needed at a later point in recovery. The thread quoted above has reported one aspect of that: we miss some hot-standby related intialization that can reflect if replaying enough WAL that a restart point could happen. Depending on the amount of data copied into pg_wal/ before starting a node with only a backup_label it may also be possible that a consistent point has been reached, where restart points would be legit. A second Kiss Cool effect (understands who can), is that we miss the initialization of the recoveryWakeupLatch. A third effect is that some code paths can use GUC values related to recovery without ArchiveRecoveryRequested being around, one example seems to be hot_standby whose default is true. It is worth noting the end of FinishWalRecovery(), that includes this part: if (ArchiveRecoveryRequested) { /* * We are no longer in archive recovery state. * * We are now done reading the old WAL. Turn off archive fetching if * it was active. */ Assert(InArchiveRecovery); InArchiveRecovery = false; I have been pondering for a few weeks now about what kind of definition would suit to a cluster having a backup_label file without a signal file added, which is documented as required by the docs in the HA section as well as pg_rewind. It is true that there could be a point to allow such a configuration so as a node recovers without a TLI jump, but I cannot find appealing this case, as well, as a node could just happily overwrite WAL segments in the archives on an existing timeline, potentially corruption other nodes writing on the same TLI. There are a few other recovery scenarios where one copies directly WAL segments into pg_wal/ that can lead to a lot of weird inconsistencies as well, one being the report of the thread of pgsql-hackers. At the end, I'd like to think that we should just require a recovery.signal or a standby.signal if we have a backup_label file, and even enforce this rule at the end of recovery for some sanity checks. I don't think that we can just enforce ArchiveRecoveryRequested in this path, either, as a backup_label would be renamed to .old once the control file knows up to which LSN it needs to replay to reach consistency and if an end-of-backup record is required. That's not something that can be reasonably backpatched, as it could disturb some recovery workflows, even if these are kind of in a dangerous spot, IMO, so I would like to propose that only on HEAD for 16~ because the recovery code has never really considered this combination of ArchiveRecoveryRequested and InArchiveRecovery. While digging into that, I have found one TAP test of pg_basebackup that was doing recovery with just a backup_label file, with a restore_command already set. A second case was in pg_rewind, were we have a node without standby.signal, still it uses a primary_conninfo. Attached is a patch on the lines of what I am thinking about. This reworks a bit some of the actions at the beginning of the startup process: - Report the set of LOGs showing the state of the node after reading the backup_label. - Enforce a rule in ShutdownWalRecovery() and document the restriction. - Add a check with the signal files after finding a backup_label file. - The validation checks on the recovery parameters are applied (aka restore_command required with recovery.signal, or a primary_conninfo required on standby for streaming, etc.). My apologies for the long message, but this deserves some attention, IMHO. So, any thoughts? -- Michael Attachments: [text/x-diff] v1-0001-Strengthen-use-of-ArchiveRecoveryRequested-and-In.patch (7.8K, ../../[email protected]/2-v1-0001-Strengthen-use-of-ArchiveRecoveryRequested-and-In.patch) download | inline diff: From 0a19d16c185473b7cb3cfab0b6129b714c827d28 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Fri, 10 Mar 2023 15:51:32 +0900 Subject: [PATCH v1] Strengthen use of ArchiveRecoveryRequested and InArchiveRecovery --- src/backend/access/transam/xlogrecovery.c | 112 +++++++++++------- src/bin/pg_basebackup/t/010_pg_basebackup.pl | 3 +- src/bin/pg_rewind/t/008_min_recovery_point.pl | 1 + 3 files changed, 75 insertions(+), 41 deletions(-) diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index dbe9394762..001dcf2077 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -125,15 +125,19 @@ static TimeLineID curFileTLI; /* * When ArchiveRecoveryRequested is set, archive recovery was requested, - * ie. signal files were present. When InArchiveRecovery is set, we are - * currently recovering using offline XLOG archives. These variables are only - * valid in the startup process. + * ie. signal files or backup_label were present. When InArchiveRecovery is + * set, we are currently recovering using offline XLOG archives. These + + variables are only valid in the startup process. Note that the presence of + * a backup_label file forces archive recovery even if there is no signal + * file. * * When ArchiveRecoveryRequested is true, but InArchiveRecovery is false, we're * currently performing crash recovery using only XLOG files in pg_wal, but * will switch to using offline XLOG archives as soon as we reach the end of * WAL in pg_wal. -*/ + * + * InArchiveRecovery should never be set without ArchiveRecoveryRequested. + */ bool ArchiveRecoveryRequested = false; bool InArchiveRecovery = false; @@ -540,42 +544,7 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr, readRecoverySignalFile(); validateRecoveryParameters(); - if (ArchiveRecoveryRequested) - { - if (StandbyModeRequested) - ereport(LOG, - (errmsg("entering standby mode"))); - else if (recoveryTarget == RECOVERY_TARGET_XID) - ereport(LOG, - (errmsg("starting point-in-time recovery to XID %u", - recoveryTargetXid))); - else if (recoveryTarget == RECOVERY_TARGET_TIME) - ereport(LOG, - (errmsg("starting point-in-time recovery to %s", - timestamptz_to_str(recoveryTargetTime)))); - else if (recoveryTarget == RECOVERY_TARGET_NAME) - ereport(LOG, - (errmsg("starting point-in-time recovery to \"%s\"", - recoveryTargetName))); - else if (recoveryTarget == RECOVERY_TARGET_LSN) - ereport(LOG, - (errmsg("starting point-in-time recovery to WAL location (LSN) \"%X/%X\"", - LSN_FORMAT_ARGS(recoveryTargetLSN)))); - else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE) - ereport(LOG, - (errmsg("starting point-in-time recovery to earliest consistent point"))); - else - ereport(LOG, - (errmsg("starting archive recovery"))); - } - - /* - * Take ownership of the wakeup latch if we're going to sleep during - * recovery. - */ - if (ArchiveRecoveryRequested) - OwnLatch(&XLogRecoveryCtl->recoveryWakeupLatch); - + /* Set the WAL reading processor, as backup_label may need it */ private = palloc0(sizeof(XLogPageReadPrivate)); xlogreader = XLogReaderAllocate(wal_segment_size, NULL, @@ -609,11 +578,30 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr, replay_image_masked = (char *) palloc(BLCKSZ); primary_image_masked = (char *) palloc(BLCKSZ); + /* + * Read the backup_label file. We want to run this part of the recovery + * process after checking for signal files and perform validation of the + * recovery parameters, as it may be possible that a backup needs to be + * run, but no signal files have been set. + */ if (read_backup_label(&CheckPointLoc, &CheckPointTLI, &backupEndRequired, &backupFromStandby)) { List *tablespaces = NIL; + if (!ArchiveRecoveryRequested) + ereport(FATAL, + (errmsg("could not find recovery.signal or standby.signal when recovering with backup_label"), + errhint("If you are restoring from a backup, touch \"%s/recovery.signal\" or \"%s/standby.signal\" and add required recovery options.", + DataDir, DataDir))); + + /* + * Take ownership of the wakeup latch if we're going to sleep during + * recovery if needed. + */ + if (ArchiveRecoveryRequested) + OwnLatch(&XLogRecoveryCtl->recoveryWakeupLatch); + /* * Archive recovery was requested, and thanks to the backup label * file, we know how far we need to replay to reach consistency. Enter @@ -706,6 +694,15 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr, } else { + /* No backup_label file has been found if we are here. */ + + /* + * Take ownership of the wakeup latch if we're going to sleep during + * recovery if needed. + */ + if (ArchiveRecoveryRequested) + OwnLatch(&XLogRecoveryCtl->recoveryWakeupLatch); + /* * If tablespace_map file is present without backup_label file, there * is no use of such file. There is no harm in retaining it, but it @@ -789,6 +786,35 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr, wasShutdown = ((record->xl_info & ~XLR_INFO_MASK) == XLOG_CHECKPOINT_SHUTDOWN); } + if (ArchiveRecoveryRequested) + { + if (StandbyModeRequested) + ereport(LOG, + (errmsg("entering standby mode"))); + else if (recoveryTarget == RECOVERY_TARGET_XID) + ereport(LOG, + (errmsg("starting point-in-time recovery to XID %u", + recoveryTargetXid))); + else if (recoveryTarget == RECOVERY_TARGET_TIME) + ereport(LOG, + (errmsg("starting point-in-time recovery to %s", + timestamptz_to_str(recoveryTargetTime)))); + else if (recoveryTarget == RECOVERY_TARGET_NAME) + ereport(LOG, + (errmsg("starting point-in-time recovery to \"%s\"", + recoveryTargetName))); + else if (recoveryTarget == RECOVERY_TARGET_LSN) + ereport(LOG, + (errmsg("starting point-in-time recovery to WAL location (LSN) \"%X/%X\"", + LSN_FORMAT_ARGS(recoveryTargetLSN)))); + else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE) + ereport(LOG, + (errmsg("starting point-in-time recovery to earliest consistent point"))); + else + ereport(LOG, + (errmsg("starting archive recovery"))); + } + /* * If the location of the checkpoint record is not on the expected * timeline in the history of the requested timeline, we cannot proceed: @@ -1574,6 +1600,12 @@ ShutdownWalRecovery(void) */ if (ArchiveRecoveryRequested) DisownLatch(&XLogRecoveryCtl->recoveryWakeupLatch); + + /* + * InArchiveRecovery should never have been set without + * ArchiveRecoveryRequested. + */ + Assert(ArchiveRecoveryRequested || !InArchiveRecovery); } /* diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl index b60cb78a0d..26d2b11fd5 100644 --- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl +++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl @@ -375,7 +375,8 @@ SKIP: my $node2 = PostgreSQL::Test::Cluster->new('replica'); # Recover main data directory - $node2->init_from_backup($node, 'tarbackup2', tar_program => $tar); + $node2->init_from_backup($node, 'tarbackup2', tar_program => $tar, + has_restoring => 1); # Recover tablespace into a new directory (not where it was!) my $repTsDir = "$tempdir/tblspc1replica"; diff --git a/src/bin/pg_rewind/t/008_min_recovery_point.pl b/src/bin/pg_rewind/t/008_min_recovery_point.pl index c753a64fdb..0026e6491e 100644 --- a/src/bin/pg_rewind/t/008_min_recovery_point.pl +++ b/src/bin/pg_rewind/t/008_min_recovery_point.pl @@ -152,6 +152,7 @@ move( "$tmp_folder/node_2-postgresql.conf.tmp", "$node_2_pgdata/postgresql.conf"); +$node_2->append_conf('standby.signal', ''); $node_2->start; # Check contents of the test tables after rewind. The rows inserted in node 3 -- 2.39.2 [application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc) download ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Requiring recovery.signal or standby.signal when recovering with a backup_label @ 2023-09-28 03:58 Kyotaro Horiguchi <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 2 replies; 5+ messages in thread From: Kyotaro Horiguchi @ 2023-09-28 03:58 UTC (permalink / raw) To: [email protected]; +Cc: [email protected] At Fri, 10 Mar 2023 15:59:04 +0900, Michael Paquier <[email protected]> wrote in > My apologies for the long message, but this deserves some attention, > IMHO. > > So, any thoughts? Sorry for being late. However, I agree with David's concern regarding the unnecessary inconvenience it introduces. I'd like to maintain the functionality. While I agree that InArchiveRecovery should be activated only if ArchiveReArchiveRecoveryRequested is true, I oppose to the notion that the mere presence of backup_label should be interpreted as a request for archive recovery (even if it is implied in a comment in InitWalRecovery()). Instead, I propose that we separate backup_label and archive recovery, in other words, we should not turn on InArchiveRecovery if !ArchiveRecoveryRequested, regardless of the presence of backup_label. We can know the minimum required recovery LSN by the XLOG_BACKUP_END record. The attached is a quick mock-up, but providing an approximation of my thoughts. (For example, end_of_backup_reached could potentially be extended to the ArchiveRecoveryRequested case and we could simplify the condition..) regards. -- Kyotaro Horiguchi NTT Open Source Software Center diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index fcbde10529..e4af945319 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -5489,17 +5489,15 @@ StartupXLOG(void) set_ps_display(""); /* - * When recovering from a backup (we are in recovery, and archive recovery - * was requested), complain if we did not roll forward far enough to reach - * the point where the database is consistent. For regular online - * backup-from-primary, that means reaching the end-of-backup WAL record - * (at which point we reset backupStartPoint to be Invalid), for - * backup-from-replica (which can't inject records into the WAL stream), - * that point is when we reach the minRecoveryPoint in pg_control (which - * we purposefully copy last when backing up from a replica). For - * pg_rewind (which creates a backup_label with a method of "pg_rewind") - * or snapshot-style backups (which don't), backupEndRequired will be set - * to false. + * When recovering from a backup, complain if we did not roll forward far + * enough to reach the point where the database is consistent. For regular + * online backup-from-primary, that means reaching the end-of-backup WAL + * record, for backup-from-replica (which can't inject records into the WAL + * stream), that point is when we reach the minRecoveryPoint in pg_control + * (which we purposefully copy last when backing up from a replica). For + * pg_rewind (which creates a backup_label with a method of "pg_rewind") or + * snapshot-style backups (which don't), backupEndRequired will be set to + * false. * * Note: it is indeed okay to look at the local variable * LocalMinRecoveryPoint here, even though ControlFile->minRecoveryPoint @@ -5508,7 +5506,7 @@ StartupXLOG(void) */ if (InRecovery && (EndOfLog < LocalMinRecoveryPoint || - !XLogRecPtrIsInvalid(ControlFile->backupStartPoint))) + (haveBackupLabel && !endOfRecoveryInfo->end_of_backup_reached))) { /* * Ran off end of WAL before reaching end-of-backup WAL record, or @@ -5516,16 +5514,13 @@ StartupXLOG(void) * recover from an online backup but never called pg_backup_stop(), or * you didn't archive all the WAL needed. */ - if (ArchiveRecoveryRequested || ControlFile->backupEndRequired) - { - if (!XLogRecPtrIsInvalid(ControlFile->backupStartPoint) || ControlFile->backupEndRequired) - ereport(FATAL, - (errmsg("WAL ends before end of online backup"), - errhint("All WAL generated while online backup was taken must be available at recovery."))); - else - ereport(FATAL, - (errmsg("WAL ends before consistent recovery point"))); - } + if (haveBackupLabel && !endOfRecoveryInfo->end_of_backup_reached) + ereport(FATAL, + (errmsg("WAL ends before end of online backup"), + errhint("All WAL generated while online backup was taken must be available at recovery."))); + else + ereport(FATAL, + (errmsg("WAL ends before consistent recovery point"))); } /* diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index becc2bda62..d3cbf0703b 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -281,6 +281,7 @@ static TimeLineID minRecoveryPointTLI; static XLogRecPtr backupStartPoint; static XLogRecPtr backupEndPoint; static bool backupEndRequired = false; +static bool backupEndReached = false; /* * Have we reached a consistent database state? In crash recovery, we have @@ -615,11 +616,12 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr, List *tablespaces = NIL; /* - * Archive recovery was requested, and thanks to the backup label + * When archive recovery was requested, thanks to the backup label * file, we know how far we need to replay to reach consistency. Enter * archive recovery directly. */ - InArchiveRecovery = true; + if (ArchiveRecoveryRequested) + InArchiveRecovery = true; if (StandbyModeRequested) EnableStandbyMode(); @@ -1531,6 +1533,19 @@ FinishWalRecovery(void) result->standby_signal_file_found = standby_signal_file_found; result->recovery_signal_file_found = recovery_signal_file_found; + /* + * If archive recovery was performed, backupEndReached indicates passing + * the end of backup. If not, a valid backupEndPoint value suggests the + * recovery began from a base backup; verify if recovery surpasses that + * point. + */ + if (backupEndReached || + (!XLogRecPtrIsInvalid(backupEndPoint) && + backupEndPoint <= XLogRecoveryCtl->lastReplayedEndRecPtr)) + result->end_of_backup_reached = true; + else + result->end_of_backup_reached = false; + return result; } @@ -2133,6 +2148,7 @@ CheckRecoveryConsistency(void) backupStartPoint = InvalidXLogRecPtr; backupEndPoint = InvalidXLogRecPtr; backupEndRequired = false; + backupEndReached = true; } /* diff --git a/src/include/access/xlogrecovery.h b/src/include/access/xlogrecovery.h index 47c29350f5..b8c1a97224 100644 --- a/src/include/access/xlogrecovery.h +++ b/src/include/access/xlogrecovery.h @@ -129,6 +129,8 @@ typedef struct */ bool standby_signal_file_found; bool recovery_signal_file_found; + + bool end_of_backup_reached; } EndOfWalRecoveryInfo; extern EndOfWalRecoveryInfo *FinishWalRecovery(void); Attachments: [text/plain] xlogrecov_not_enter_arch_if_not_requested.txt (5.3K, ../../[email protected]/2-xlogrecov_not_enter_arch_if_not_requested.txt) download | inline diff: diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index fcbde10529..e4af945319 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -5489,17 +5489,15 @@ StartupXLOG(void) set_ps_display(""); /* - * When recovering from a backup (we are in recovery, and archive recovery - * was requested), complain if we did not roll forward far enough to reach - * the point where the database is consistent. For regular online - * backup-from-primary, that means reaching the end-of-backup WAL record - * (at which point we reset backupStartPoint to be Invalid), for - * backup-from-replica (which can't inject records into the WAL stream), - * that point is when we reach the minRecoveryPoint in pg_control (which - * we purposefully copy last when backing up from a replica). For - * pg_rewind (which creates a backup_label with a method of "pg_rewind") - * or snapshot-style backups (which don't), backupEndRequired will be set - * to false. + * When recovering from a backup, complain if we did not roll forward far + * enough to reach the point where the database is consistent. For regular + * online backup-from-primary, that means reaching the end-of-backup WAL + * record, for backup-from-replica (which can't inject records into the WAL + * stream), that point is when we reach the minRecoveryPoint in pg_control + * (which we purposefully copy last when backing up from a replica). For + * pg_rewind (which creates a backup_label with a method of "pg_rewind") or + * snapshot-style backups (which don't), backupEndRequired will be set to + * false. * * Note: it is indeed okay to look at the local variable * LocalMinRecoveryPoint here, even though ControlFile->minRecoveryPoint @@ -5508,7 +5506,7 @@ StartupXLOG(void) */ if (InRecovery && (EndOfLog < LocalMinRecoveryPoint || - !XLogRecPtrIsInvalid(ControlFile->backupStartPoint))) + (haveBackupLabel && !endOfRecoveryInfo->end_of_backup_reached))) { /* * Ran off end of WAL before reaching end-of-backup WAL record, or @@ -5516,16 +5514,13 @@ StartupXLOG(void) * recover from an online backup but never called pg_backup_stop(), or * you didn't archive all the WAL needed. */ - if (ArchiveRecoveryRequested || ControlFile->backupEndRequired) - { - if (!XLogRecPtrIsInvalid(ControlFile->backupStartPoint) || ControlFile->backupEndRequired) - ereport(FATAL, - (errmsg("WAL ends before end of online backup"), - errhint("All WAL generated while online backup was taken must be available at recovery."))); - else - ereport(FATAL, - (errmsg("WAL ends before consistent recovery point"))); - } + if (haveBackupLabel && !endOfRecoveryInfo->end_of_backup_reached) + ereport(FATAL, + (errmsg("WAL ends before end of online backup"), + errhint("All WAL generated while online backup was taken must be available at recovery."))); + else + ereport(FATAL, + (errmsg("WAL ends before consistent recovery point"))); } /* diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index becc2bda62..d3cbf0703b 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -281,6 +281,7 @@ static TimeLineID minRecoveryPointTLI; static XLogRecPtr backupStartPoint; static XLogRecPtr backupEndPoint; static bool backupEndRequired = false; +static bool backupEndReached = false; /* * Have we reached a consistent database state? In crash recovery, we have @@ -615,11 +616,12 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr, List *tablespaces = NIL; /* - * Archive recovery was requested, and thanks to the backup label + * When archive recovery was requested, thanks to the backup label * file, we know how far we need to replay to reach consistency. Enter * archive recovery directly. */ - InArchiveRecovery = true; + if (ArchiveRecoveryRequested) + InArchiveRecovery = true; if (StandbyModeRequested) EnableStandbyMode(); @@ -1531,6 +1533,19 @@ FinishWalRecovery(void) result->standby_signal_file_found = standby_signal_file_found; result->recovery_signal_file_found = recovery_signal_file_found; + /* + * If archive recovery was performed, backupEndReached indicates passing + * the end of backup. If not, a valid backupEndPoint value suggests the + * recovery began from a base backup; verify if recovery surpasses that + * point. + */ + if (backupEndReached || + (!XLogRecPtrIsInvalid(backupEndPoint) && + backupEndPoint <= XLogRecoveryCtl->lastReplayedEndRecPtr)) + result->end_of_backup_reached = true; + else + result->end_of_backup_reached = false; + return result; } @@ -2133,6 +2148,7 @@ CheckRecoveryConsistency(void) backupStartPoint = InvalidXLogRecPtr; backupEndPoint = InvalidXLogRecPtr; backupEndRequired = false; + backupEndReached = true; } /* diff --git a/src/include/access/xlogrecovery.h b/src/include/access/xlogrecovery.h index 47c29350f5..b8c1a97224 100644 --- a/src/include/access/xlogrecovery.h +++ b/src/include/access/xlogrecovery.h @@ -129,6 +129,8 @@ typedef struct */ bool standby_signal_file_found; bool recovery_signal_file_found; + + bool end_of_backup_reached; } EndOfWalRecoveryInfo; extern EndOfWalRecoveryInfo *FinishWalRecovery(void); ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Requiring recovery.signal or standby.signal when recovering with a backup_label @ 2023-09-28 04:26 Michael Paquier <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Michael Paquier @ 2023-09-28 04:26 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected] On Thu, Sep 28, 2023 at 12:58:51PM +0900, Kyotaro Horiguchi wrote: > The attached is a quick mock-up, but providing an approximation of my > thoughts. (For example, end_of_backup_reached could potentially be > extended to the ArchiveRecoveryRequested case and we could simplify > the condition..) I am not sure why this is related to this thread.. static XLogRecPtr backupStartPoint; static XLogRecPtr backupEndPoint; static bool backupEndRequired = false; +static bool backupEndReached = false; Anyway, sneaking at your suggestion, this is actually outlining the main issue I have with this code currently. We have so many static booleans to control one behavior over the other that we always try to make this code more complicated, while we should try to make it simpler instead. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Requiring recovery.signal or standby.signal when recovering with a backup_label @ 2023-09-28 20:23 David Steele <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: David Steele @ 2023-09-28 20:23 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; [email protected]; +Cc: [email protected]; [email protected]; [email protected] On 9/27/23 23:58, Kyotaro Horiguchi wrote: > At Fri, 10 Mar 2023 15:59:04 +0900, Michael Paquier <[email protected]> wrote in >> My apologies for the long message, but this deserves some attention, >> IMHO. >> >> So, any thoughts? > > Sorry for being late. However, I agree with David's concern regarding > the unnecessary inconvenience it introduces. I'd like to maintain the > functionality. After some playing around, I find I agree with Michael on this, i.e. require at least standby.signal when a backup_label is present. According to my testing, you can preserve the "independent server" functionality by setting archive_command = /bin/false. In this case the timeline is not advanced and recovery proceeds from whatever is available in pg_wal. I think this type of recovery from a backup label without a timeline change should absolutely be the exception, not the default as it seems to be now. If the server is truly independent, then the timeline change is not important. If the server is not independent, then the timeline change is critical. So overall, +1 for Michael's patch, though I have only read through it and not tested it yet. One comment, though, if we are going to require recovery.signal when backup_label is present, should it just be implied? Why error and force the user to create it? Regards, -David ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-09-28 20:23 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-01-20 15:53 [PATCH v48 3/3] Replace assuming a composite object on a scalar Dmitrii Dolgov <[email protected]> 2023-03-10 06:59 Requiring recovery.signal or standby.signal when recovering with a backup_label Michael Paquier <[email protected]> 2023-09-28 03:58 ` Re: Requiring recovery.signal or standby.signal when recovering with a backup_label Kyotaro Horiguchi <[email protected]> 2023-09-28 04:26 ` Re: Requiring recovery.signal or standby.signal when recovering with a backup_label Michael Paquier <[email protected]> 2023-09-28 20:23 ` Re: Requiring recovery.signal or standby.signal when recovering with a 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