agora inbox for [email protected]help / color / mirror / Atom feed
[PATCH v45 3/3] Replace assuming a composite object on a scalar 12+ messages / 4 participants [nested] [flat]
* [PATCH v45 3/3] Replace assuming a composite object on a scalar @ 2021-01-07 08:04 Dmitrii Dolgov <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Dmitrii Dolgov @ 2021-01-07 08:04 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 | 22 ++++++++++++++++++++-- src/backend/utils/adt/jsonfuncs.c | 11 +++++++++++ src/test/regress/expected/jsonb.out | 12 ++++++++++++ src/test/regress/sql/jsonb.sql | 8 ++++++++ 4 files changed, 51 insertions(+), 2 deletions(-) diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml index 9af015d222..924762e128 100644 --- a/doc/src/sgml/json.sgml +++ b/doc/src/sgml/json.sgml @@ -612,8 +612,23 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @> '{"tags": ["qu <literal>path</literal> argument in <literal>jsonb_set</literal> function, e.g. in case of arrays it is a 0-based operation or that negative integers that appear in <literal>path</literal> count from the end of JSON arrays. - The result of subscripting expressions is always jsonb data type. An - example of subscripting syntax: + The result of subscripting expressions is always jsonb data type. + + <para> + <command>UPDATE</command> statements may use subscripting in the + <literal>SET</literal> clause to modify <type>jsonb</type> values. Every + affected value must conform to the path defined by the subscript(s). If the + path contradicts structure of modified <type>jsonb</type> for any individual + value (e.g. path <literal>val['a']['b']['c']</literal> assumes keys + <literal>'a'</literal> and <literal>'b'</literal> have object values + assigned to them, but if <literal>val['a']</literal> or + <literal>val['b']</literal> is null, a string, or a number, then the path + contradicts with the existing structure), an error is raised even if other + values do conform. + </para> + <para> + + An example of subscripting syntax: <programlisting> -- Extract value by key SELECT ('{"a": 1}'::jsonb)['a']; @@ -628,6 +643,9 @@ SELECT ('[1, "2", null]'::jsonb)[1]; -- needs to be of jsonb type as well UPDATE table_name SET jsonb_field['key'] = '1'; +-- This will raise an error if jsonb_field is {"a": 1} +UPDATE table_name SET jsonb_field['a']['b']['c'] = '1'; + -- Select records using where clause with subscripting. Since the result of -- subscripting is jsonb and we basically want to compare two jsonb objects, we -- need to put the value in double quotes to be able to convert it to jsonb. diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index f14f6c3191..ebc0b06f5b 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -4943,6 +4943,17 @@ setPath(JsonbIterator **it, Datum *path_elems, break; case WJB_ELEM: case WJB_VALUE: + /* + * If instructed complain about attempts to replace whithin a + * scalar 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 b7c268b53f..0df808c36d 100644 --- a/src/test/regress/expected/jsonb.out +++ b/src/test/regress/expected/jsonb.out @@ -5134,6 +5134,18 @@ select * from test_jsonb_subscript; 1 | {"a": [null, {"c": [null, null, 1]}]} (1 row) +-- trying replace assuming a composite object, but it's a scalar +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{"a": 1}'); +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]['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. -- 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..c62a2f9aec 100644 --- a/src/test/regress/sql/jsonb.sql +++ b/src/test/regress/sql/jsonb.sql @@ -1371,6 +1371,14 @@ 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 a scalar + +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{"a": 1}'); +update test_jsonb_subscript set test_json['a']['b']['c'] = '1'; +update test_jsonb_subscript set test_json['a'][0]['c'] = '1'; +update test_jsonb_subscript set test_json['a'][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 --2v7pubbjx3p2om6u-- ^ permalink raw reply [nested|flat] 12+ messages in thread
* Standby recovers records from wrong timeline @ 2022-10-19 15:50 Ants Aasma <[email protected]> 2022-10-20 08:29 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 0 siblings, 1 reply; 12+ messages in thread From: Ants Aasma @ 2022-10-19 15:50 UTC (permalink / raw) To: pgsql-hackers When standby is recovering to a timeline that doesn't have any segments archived yet it will just blindly blow past the timeline switch point and keeps on recovering on the old timeline. Typically that will eventually result in an error about incorrect prev-link, but under unhappy circumstances can result in standby silently having different contents. Attached is a shell script that reproduces the issue. Goes back to at least v12, probably longer. I think we should be keeping track of where the current replay timeline is going to end and not read any records past it on the old timeline. Maybe while at it, we should also track that the next record should be a checkpoint record for the timeline switch and error out if not. Thoughts? -- Ants Aasma Senior Database Engineerwww.cybertec-postgresql.com Attachments: [application/x-shellscript] recoverytest.sh (1.9K, ../../CANwKhkMN3QwAcvuDZHb6wsvLRtkweBiYso-KLFykkQVWuQLcOw@mail.gmail.com/3-recoverytest.sh) download ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Standby recovers records from wrong timeline 2022-10-19 15:50 Standby recovers records from wrong timeline Ants Aasma <[email protected]> @ 2022-10-20 08:29 ` Kyotaro Horiguchi <[email protected]> 2022-10-20 08:34 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 2022-10-20 11:44 ` Re: Standby recovers records from wrong timeline Ants Aasma <[email protected]> 0 siblings, 2 replies; 12+ messages in thread From: Kyotaro Horiguchi @ 2022-10-20 08:29 UTC (permalink / raw) To: [email protected]; +Cc: pgsql-hackers At Wed, 19 Oct 2022 18:50:09 +0300, Ants Aasma <[email protected]> wrote in > When standby is recovering to a timeline that doesn't have any segments > archived yet it will just blindly blow past the timeline switch point and > keeps on recovering on the old timeline. Typically that will eventually > result in an error about incorrect prev-link, but under unhappy > circumstances can result in standby silently having different contents. > > Attached is a shell script that reproduces the issue. Goes back to at least > v12, probably longer. > > I think we should be keeping track of where the current replay timeline is > going to end and not read any records past it on the old timeline. Maybe > while at it, we should also track that the next record should be a > checkpoint record for the timeline switch and error out if not. Thoughts? primary_restored did a time-travel to past a bit because of the recovery_target=immediate. In other words, the primary_restored and the replica diverge. I don't think it is legit to connect a diverged standby to a primary. So, about the behavior in doubt, it is the correct behavior to seemingly ignore the history file in the archive. Recovery assumes that the first half of the first segment of the new timeline is the same with the same segment of the old timeline (.partial) so it is legit to read the <tli=1,seg=2> file til the end and that causes the replica goes beyond the divergence point. As you know, when new primary starts a diverged history, the recommended way is to blow (or stash) away the archive, then take a new backup from the running primary. If you don't want to trash all the past backups, remove the archived files equals to or after the divergence point before starting the standby. They're <tli=2,seg=2,3> in this case. Also you must remove replica/pg_wal/<tli=2,seg=2> before starting the replica. That file causes recovery run beyond the divergence point before fetching from archive or stream. regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Standby recovers records from wrong timeline 2022-10-19 15:50 Standby recovers records from wrong timeline Ants Aasma <[email protected]> 2022-10-20 08:29 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> @ 2022-10-20 08:34 ` Kyotaro Horiguchi <[email protected]> 2022-10-20 08:47 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 1 sibling, 1 reply; 12+ messages in thread From: Kyotaro Horiguchi @ 2022-10-20 08:34 UTC (permalink / raw) To: [email protected]; +Cc: pgsql-hackers Sorry, a correction needed.. At Thu, 20 Oct 2022 17:29:57 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in > At Wed, 19 Oct 2022 18:50:09 +0300, Ants Aasma <[email protected]> wrote in > > When standby is recovering to a timeline that doesn't have any segments > > archived yet it will just blindly blow past the timeline switch point and > > keeps on recovering on the old timeline. Typically that will eventually > > result in an error about incorrect prev-link, but under unhappy > > circumstances can result in standby silently having different contents. > > > > Attached is a shell script that reproduces the issue. Goes back to at least > > v12, probably longer. > > > > I think we should be keeping track of where the current replay timeline is > > going to end and not read any records past it on the old timeline. Maybe > > while at it, we should also track that the next record should be a > > checkpoint record for the timeline switch and error out if not. Thoughts? > > primary_restored did a time-travel to past a bit because of the > recovery_target=immediate. In other words, the primary_restored and > the replica diverge. I don't think it is legit to connect a diverged > standby to a primary. > > So, about the behavior in doubt, it is the correct behavior to > seemingly ignore the history file in the archive. Recovery assumes > that the first half of the first segment of the new timeline is the > same with the same segment of the old timeline (.partial) so it is > legit to read the <tli=1,seg=2> file til the end and that causes the > replica goes beyond the divergence point. > > As you know, when new primary starts a diverged history, the > recommended way is to blow (or stash) away the archive, then take a > new backup from the running primary. > > If you don't want to trash all the past backups, remove the archived > files equals to or after the divergence point before starting the > standby. They're <tli=2,seg=2,3> in this case. Also you must remove <tli=2,seg=2,3> => <tli=1,seg=2,3> > replica/pg_wal/<tli=2,seg=2> before starting the replica. That file > causes recovery run beyond the divergence point before fetching from > archive or stream. <tli=2,seg=2> => <tli=1,seg=2> regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Standby recovers records from wrong timeline 2022-10-19 15:50 Standby recovers records from wrong timeline Ants Aasma <[email protected]> 2022-10-20 08:29 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 2022-10-20 08:34 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> @ 2022-10-20 08:47 ` Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Kyotaro Horiguchi @ 2022-10-20 08:47 UTC (permalink / raw) To: [email protected]; +Cc: pgsql-hackers Forgot a caveat. At Thu, 20 Oct 2022 17:34:13 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in > At Wed, 19 Oct 2022 18:50:09 +0300, Ants Aasma <[email protected]> wrote in > > When standby is recovering to a timeline that doesn't have any segments > > archived yet it will just blindly blow past the timeline switch point and > > keeps on recovering on the old timeline. Typically that will eventually > > result in an error about incorrect prev-link, but under unhappy > > circumstances can result in standby silently having different contents. > > > > Attached is a shell script that reproduces the issue. Goes back to at least > > v12, probably longer. > > > > I think we should be keeping track of where the current replay timeline is > > going to end and not read any records past it on the old timeline. Maybe > > while at it, we should also track that the next record should be a > > checkpoint record for the timeline switch and error out if not. Thoughts? > > primary_restored did a time-travel to past a bit because of the > recovery_target=immediate. In other words, the primary_restored and > the replica diverge. I don't think it is legit to connect a diverged > standby to a primary. > > So, about the behavior in doubt, it is the correct behavior to > seemingly ignore the history file in the archive. Recovery assumes > that the first half of the first segment of the new timeline is the > same with the same segment of the old timeline (.partial) so it is > legit to read the <tli=1,seg=2> file til the end and that causes the > replica goes beyond the divergence point. > > As you know, when new primary starts a diverged history, the > recommended way is to blow (or stash) away the archive, then take a > new backup from the running primary. > > If you don't want to trash all the past backups, remove the archived > files equals to or after the divergence point before starting the > standby. They're <tli=1,seg=2,3> in this case. Also you must remove > replica/pg_wal/<tli=1,seg=2> before starting the replica. That file > causes recovery run beyond the divergence point before fetching from > archive or stream. The reason this is workable is (as far as I can see) using recovery_target=immediate to stop replication and the two clusters share the completely identical disk image. Otherwise this steps results in a broken standby. regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Standby recovers records from wrong timeline 2022-10-19 15:50 Standby recovers records from wrong timeline Ants Aasma <[email protected]> 2022-10-20 08:29 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> @ 2022-10-20 11:44 ` Ants Aasma <[email protected]> 2022-10-21 07:45 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 1 sibling, 1 reply; 12+ messages in thread From: Ants Aasma @ 2022-10-20 11:44 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: pgsql-hackers On Thu, 20 Oct 2022 at 11:30, Kyotaro Horiguchi <[email protected]> wrote: > > primary_restored did a time-travel to past a bit because of the > recovery_target=immediate. In other words, the primary_restored and > the replica diverge. I don't think it is legit to connect a diverged > standby to a primary. primary_restored did timetravel to the past, as we're doing PITR on the primary that's the expected behavior. However replica is not diverged, it's a copy of the exact same basebackup. The usecase is restoring a cluster from backup using PITR and using the same backup to create a standby. Currently this breaks when primary has not yet archived any segments. > So, about the behavior in doubt, it is the correct behavior to > seemingly ignore the history file in the archive. Recovery assumes > that the first half of the first segment of the new timeline is the > same with the same segment of the old timeline (.partial) so it is > legit to read the <tli=1,seg=2> file til the end and that causes the > replica goes beyond the divergence point. What is happening is that primary_restored has a timeline switch at tli 2, lsn 0/2000100, and the next insert record starts in the same segment. Replica is starting on the same backup on timeline 1, tries to find tli 2 seg 2, which is not archived yet, so falls back to tli 1 seg 2 and replays tli 1 seg 2 continuing to tli seg 3, then connects to primary and starts applying wal starting from tli 2 seg 4. To me that seems completely broken. > As you know, when new primary starts a diverged history, the > recommended way is to blow (or stash) away the archive, then take a > new backup from the running primary. My understanding is that backup archives are supposed to remain valid even after PITR or equivalently a lagging standby promoting. -- Ants Aasma Senior Database Engineer www.cybertec-postgresql.com ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Standby recovers records from wrong timeline 2022-10-19 15:50 Standby recovers records from wrong timeline Ants Aasma <[email protected]> 2022-10-20 08:29 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 2022-10-20 11:44 ` Re: Standby recovers records from wrong timeline Ants Aasma <[email protected]> @ 2022-10-21 07:45 ` Kyotaro Horiguchi <[email protected]> 2022-10-21 08:12 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 0 siblings, 1 reply; 12+ messages in thread From: Kyotaro Horiguchi @ 2022-10-21 07:45 UTC (permalink / raw) To: [email protected]; +Cc: pgsql-hackers At Thu, 20 Oct 2022 14:44:40 +0300, Ants Aasma <[email protected]> wrote in > My understanding is that backup archives are supposed to remain valid > even after PITR or equivalently a lagging standby promoting. Sorry, I was dim because of maybe catching a cold:p On second thought. everything works fine if the first segment of the new timeline is archived in this case. So the problem here is whether recovery should wait for a known new timline when no segment on the new timeline is available yet. As you say, I think it is sensible that recovery waits at the divergence LSN for the first segment on the new timeline before proceeding on the same timeline. regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Standby recovers records from wrong timeline 2022-10-19 15:50 Standby recovers records from wrong timeline Ants Aasma <[email protected]> 2022-10-20 08:29 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 2022-10-20 11:44 ` Re: Standby recovers records from wrong timeline Ants Aasma <[email protected]> 2022-10-21 07:45 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> @ 2022-10-21 08:12 ` Kyotaro Horiguchi <[email protected]> 2022-10-21 08:44 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 0 siblings, 1 reply; 12+ messages in thread From: Kyotaro Horiguchi @ 2022-10-21 08:12 UTC (permalink / raw) To: [email protected]; +Cc: pgsql-hackers At Fri, 21 Oct 2022 16:45:59 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in > At Thu, 20 Oct 2022 14:44:40 +0300, Ants Aasma <[email protected]> wrote in > > My understanding is that backup archives are supposed to remain valid > > even after PITR or equivalently a lagging standby promoting. > > Sorry, I was dim because of maybe catching a cold:p > > On second thought. everything works fine if the first segment of the > new timeline is archived in this case. So the problem here is whether > recovery should wait for a known new timline when no segment on the > new timeline is available yet. As you say, I think it is sensible > that recovery waits at the divergence LSN for the first segment on the > new timeline before proceeding on the same timeline. It is simpler than anticipated. Just not descending timelines when latest works. It dones't consider the case of explict target timlines so it's just a PoC. (So this doesn't work if recovery_target_timeline is set to 2 for the "standby" in the repro.) regards. -- Kyotaro Horiguchi NTT Open Source Software Center Attachments: [text/x-patch] PoC_dont_go_beyond_divergence_when_target_is_latest.patch (638B, ../../[email protected]/2-PoC_dont_go_beyond_divergence_when_target_is_latest.patch) download | inline diff: diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index cb07694aea..18c14d1fbf 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -4223,6 +4223,13 @@ XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source) return fd; } } + + /* + * If recovery_target_timeline is "latest", we don't want to read this + * segment belongs to older timelines. + */ + if (recoveryTargetTimeLineGoal == RECOVERY_TARGET_TIMELINE_LATEST) + break; } /* Couldn't find it. For simplicity, complain about front timeline */ ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Standby recovers records from wrong timeline 2022-10-19 15:50 Standby recovers records from wrong timeline Ants Aasma <[email protected]> 2022-10-20 08:29 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 2022-10-20 11:44 ` Re: Standby recovers records from wrong timeline Ants Aasma <[email protected]> 2022-10-21 07:45 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 2022-10-21 08:12 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> @ 2022-10-21 08:44 ` Kyotaro Horiguchi <[email protected]> 2022-10-21 09:38 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 2022-10-21 09:48 ` Re: Standby recovers records from wrong timeline Ants Aasma <[email protected]> 0 siblings, 2 replies; 12+ messages in thread From: Kyotaro Horiguchi @ 2022-10-21 08:44 UTC (permalink / raw) To: [email protected]; +Cc: pgsql-hackers At Fri, 21 Oct 2022 17:12:45 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in > latest works. It dones't consider the case of explict target timlines > so it's just a PoC. (So this doesn't work if recovery_target_timeline > is set to 2 for the "standby" in the repro.) So, finally I noticed that the function XLogFileReadAnyTLI is not needed at all if we are going this direction. Regardless of recvoery_target_timeline is latest or any explicit imeline id or checkpoint timeline, what we can do to reach the target timline is just to follow the history file's direction. If segments are partly gone while reading on a timeline, a segment on the older timelines is just a crap since it should be incompatible. So.. I'm at a loss about what the function is for. Please anyone tell me why do we need the behavior of XLogFileReadAnyTLI() at all? regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Standby recovers records from wrong timeline 2022-10-19 15:50 Standby recovers records from wrong timeline Ants Aasma <[email protected]> 2022-10-20 08:29 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 2022-10-20 11:44 ` Re: Standby recovers records from wrong timeline Ants Aasma <[email protected]> 2022-10-21 07:45 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 2022-10-21 08:12 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 2022-10-21 08:44 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> @ 2022-10-21 09:38 ` Kyotaro Horiguchi <[email protected]> 1 sibling, 0 replies; 12+ messages in thread From: Kyotaro Horiguchi @ 2022-10-21 09:38 UTC (permalink / raw) To: [email protected]; +Cc: pgsql-hackers At Fri, 21 Oct 2022 17:44:40 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in > At Fri, 21 Oct 2022 17:12:45 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in > > latest works. It dones't consider the case of explict target timlines > > so it's just a PoC. (So this doesn't work if recovery_target_timeline > > is set to 2 for the "standby" in the repro.) > > So, finally I noticed that the function XLogFileReadAnyTLI is not > needed at all if we are going this direction. > > Regardless of recvoery_target_timeline is latest or any explicit > imeline id or checkpoint timeline, what we can do to reach the target > timline is just to follow the history file's direction. > > If segments are partly gone while reading on a timeline, a segment on > the older timelines is just a crap since it should be incompatible. > > So.. I'm at a loss about what the function is for. > > Please anyone tell me why do we need the behavior of > XLogFileReadAnyTLI() at all? It is introduced by 1bb2558046. And the behavior dates back to 2042b3428d. Hmmm.. XLogFileRead() at the time did essentially the same thing to the current XLogFileReadAnyTLI. At that time the expectedTL*I*s contained only timeline IDs. Thus it seems to me, at that time, recovery assumed that it is fine with reading the segment on the greatest available timeline in the TLI list at every mement. (Mmm. I cannot describe this precise enough....) In other words it did not intend to use the segments on the older timelines than expected as the replacement of the segment on the correct timelnie. If this is correct (I hople the description above makes sense), now that we can determine the exact TLI to read for the specified segno, we don't need to descend to older timelines. In other words, the current XLogFileReadAnyTLI() should be just XLogFileReadOnHistory(), which reads a segment of the exact timeline calculated from the expectedTLEs and the segno. I'm going to work in this direction. regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Standby recovers records from wrong timeline 2022-10-19 15:50 Standby recovers records from wrong timeline Ants Aasma <[email protected]> 2022-10-20 08:29 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 2022-10-20 11:44 ` Re: Standby recovers records from wrong timeline Ants Aasma <[email protected]> 2022-10-21 07:45 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 2022-10-21 08:12 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 2022-10-21 08:44 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> @ 2022-10-21 09:48 ` Ants Aasma <[email protected]> 1 sibling, 0 replies; 12+ messages in thread From: Ants Aasma @ 2022-10-21 09:48 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: pgsql-hackers On Fri, 21 Oct 2022 at 11:44, Kyotaro Horiguchi <[email protected]> wrote: > > At Fri, 21 Oct 2022 17:12:45 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in > > latest works. It dones't consider the case of explict target timlines > > so it's just a PoC. (So this doesn't work if recovery_target_timeline > > is set to 2 for the "standby" in the repro.) > > So, finally I noticed that the function XLogFileReadAnyTLI is not > needed at all if we are going this direction. > > Regardless of recvoery_target_timeline is latest or any explicit > imeline id or checkpoint timeline, what we can do to reach the target > timline is just to follow the history file's direction. > > If segments are partly gone while reading on a timeline, a segment on > the older timelines is just a crap since it should be incompatible. I came to the same conclusion. I adjusted XLogFileReadAnyTLI to not use any timeline that ends within the segment (attached patch). At this point the name of the function becomes really wrong, XLogFileReadCorrectTLI or something to that effect would be much more descriptive and the code could be simplified. However I'm not particularly happy with this approach as it will not use valid WAL if that is not available. Consider scenario of a cascading failure. Node A has a hard failure, then node B promotes, archives history file, but doesn't see enough traffic to archive a full segment before failing itself. While this is happening we restore node A from backup and start it up as a standby. If node b fails before node A has a chance to connect then either we are continuing recovery on the wrong timeline (current behavior) or we will not try to recover the first portion of the archived WAL file (with patch). So I think the correct approach would still be to have ReadRecord() or ApplyWalRecord() determine that switching timelines is needed. -- Ants Aasma www.cybertec-postgresql.com Attachments: [text/x-patch] Only_consider_latest_valid_TLI_for_archive_fetching.patch (1.2K, ../../CANwKhkOUK74crShXSMJ_deTm_NDg4QwLivDFWJN0-Qq8hHbq-Q@mail.gmail.com/2-Only_consider_latest_valid_TLI_for_archive_fetching.patch) download | inline diff: diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index cb07694aea6..73bde98b920 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -4171,6 +4171,7 @@ XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source) { TimeLineHistoryEntry *hent = (TimeLineHistoryEntry *) lfirst(cell); TimeLineID tli = hent->tli; + XLogSegNo beginseg = 0; if (tli < curFileTLI) break; /* don't bother looking at too-old TLIs */ @@ -4181,7 +4182,6 @@ XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source) */ if (hent->begin != InvalidXLogRecPtr) { - XLogSegNo beginseg = 0; XLByteToSeg(hent->begin, beginseg, wal_segment_size); @@ -4223,6 +4223,14 @@ XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source) return fd; } } + + /* + * For segments containing known timeline switches only consider the + * last timeline as redo otherwise doesn't know when to switch + * timelines. + */ + if (segno == beginseg && beginseg > 0) + break; } /* Couldn't find it. For simplicity, complain about front timeline */ ^ permalink raw reply [nested|flat] 12+ messages in thread
* [PATCH v15 4/8] Row pattern recognition patch (planner). @ 2024-03-28 10:30 Tatsuo Ishii <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Tatsuo Ishii @ 2024-03-28 10:30 UTC (permalink / raw) --- src/backend/optimizer/plan/createplan.c | 24 +++++++++++++++----- src/backend/optimizer/plan/planner.c | 3 +++ src/backend/optimizer/plan/setrefs.c | 27 ++++++++++++++++++++++- src/backend/optimizer/prep/prepjointree.c | 4 ++++ src/include/nodes/plannodes.h | 19 ++++++++++++++++ 5 files changed, 71 insertions(+), 6 deletions(-) diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index 5f479fc56c..fe51ad351e 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -287,9 +287,11 @@ static WindowAgg *make_windowagg(List *tlist, Index winref, int ordNumCols, AttrNumber *ordColIdx, Oid *ordOperators, Oid *ordCollations, int frameOptions, Node *startOffset, Node *endOffset, Oid startInRangeFunc, Oid endInRangeFunc, - Oid inRangeColl, bool inRangeAsc, bool inRangeNullsFirst, - List *runCondition, List *qual, bool topWindow, - Plan *lefttree); + Oid inRangeColl, bool inRangeAsc, bool inRangeNullsFirst, List *runCondition, + RPSkipTo rpSkipTo, List *patternVariable, List *patternRegexp, + List *defineClause, + List *defineInitial, + List *qual, bool topWindow, Plan *lefttree); static Group *make_group(List *tlist, List *qual, int numGroupCols, AttrNumber *grpColIdx, Oid *grpOperators, Oid *grpCollations, Plan *lefttree); @@ -2699,6 +2701,11 @@ create_windowagg_plan(PlannerInfo *root, WindowAggPath *best_path) wc->inRangeAsc, wc->inRangeNullsFirst, wc->runCondition, + wc->rpSkipTo, + wc->patternVariable, + wc->patternRegexp, + wc->defineClause, + wc->defineInitial, best_path->qual, best_path->topwindow, subplan); @@ -6627,8 +6634,10 @@ make_windowagg(List *tlist, Index winref, int ordNumCols, AttrNumber *ordColIdx, Oid *ordOperators, Oid *ordCollations, int frameOptions, Node *startOffset, Node *endOffset, Oid startInRangeFunc, Oid endInRangeFunc, - Oid inRangeColl, bool inRangeAsc, bool inRangeNullsFirst, - List *runCondition, List *qual, bool topWindow, Plan *lefttree) + Oid inRangeColl, bool inRangeAsc, bool inRangeNullsFirst, List *runCondition, + RPSkipTo rpSkipTo, List *patternVariable, List *patternRegexp, List *defineClause, + List *defineInitial, + List *qual, bool topWindow, Plan *lefttree) { WindowAgg *node = makeNode(WindowAgg); Plan *plan = &node->plan; @@ -6654,6 +6663,11 @@ make_windowagg(List *tlist, Index winref, node->inRangeAsc = inRangeAsc; node->inRangeNullsFirst = inRangeNullsFirst; node->topWindow = topWindow; + node->rpSkipTo = rpSkipTo, + node->patternVariable = patternVariable; + node->patternRegexp = patternRegexp; + node->defineClause = defineClause; + node->defineInitial = defineInitial; plan->targetlist = tlist; plan->lefttree = lefttree; diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 38d070fa00..82c370c4b4 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -869,6 +869,9 @@ subquery_planner(PlannerGlobal *glob, Query *parse, wc->runCondition = (List *) preprocess_expression(root, (Node *) wc->runCondition, EXPRKIND_TARGET); + wc->defineClause = (List *) preprocess_expression(root, + (Node *) wc->defineClause, + EXPRKIND_TARGET); } parse->limitOffset = preprocess_expression(root, parse->limitOffset, diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index 42603dbc7c..f7abb2be96 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -210,7 +210,6 @@ static List *set_windowagg_runcondition_references(PlannerInfo *root, List *runcondition, Plan *plan); - /***************************************************************************** * * SUBPLAN REFERENCES @@ -2455,6 +2454,32 @@ set_upper_references(PlannerInfo *root, Plan *plan, int rtoffset) NRM_EQUAL, NUM_EXEC_QUAL(plan)); + /* + * Modifies an expression tree in each DEFINE clause so that all Var + * nodes's varno refers to OUTER_VAR. + */ + if (IsA(plan, WindowAgg)) + { + WindowAgg *wplan = (WindowAgg *) plan; + + if (wplan->defineClause != NIL) + { + foreach(l, wplan->defineClause) + { + TargetEntry *tle = (TargetEntry *) lfirst(l); + + tle->expr = (Expr *) + fix_upper_expr(root, + (Node *) tle->expr, + subplan_itlist, + OUTER_VAR, + rtoffset, + NRM_EQUAL, + NUM_EXEC_QUAL(plan)); + } + } + } + pfree(subplan_itlist); } diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c index 300691cc4d..3220072a51 100644 --- a/src/backend/optimizer/prep/prepjointree.c +++ b/src/backend/optimizer/prep/prepjointree.c @@ -2147,6 +2147,10 @@ perform_pullup_replace_vars(PlannerInfo *root, if (wc->runCondition != NIL) wc->runCondition = (List *) pullup_replace_vars((Node *) wc->runCondition, rvcontext); + + if (wc->defineClause != NIL) + wc->defineClause = (List *) + pullup_replace_vars((Node *) wc->defineClause, rvcontext); } if (parse->onConflict) { diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index 7f3db5105d..b0e50ae886 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -20,6 +20,7 @@ #include "lib/stringinfo.h" #include "nodes/bitmapset.h" #include "nodes/lockoptions.h" +#include "nodes/parsenodes.h" #include "nodes/primnodes.h" @@ -1096,6 +1097,24 @@ typedef struct WindowAgg /* nulls sort first for in_range tests? */ bool inRangeNullsFirst; + /* Row Pattern Recognition AFTER MACH SKIP clause */ + RPSkipTo rpSkipTo; /* Row Pattern Skip To type */ + + /* Row Pattern PATTERN variable name (list of String) */ + List *patternVariable; + + /* + * Row Pattern RPATTERN regular expression quantifier ('+' or ''. list of + * String) + */ + List *patternRegexp; + + /* Row Pattern DEFINE clause (list of TargetEntry) */ + List *defineClause; + + /* Row Pattern DEFINE variable initial names (list of String) */ + List *defineInitial; + /* * false for all apart from the WindowAgg that's closest to the root of * the plan -- 2.25.1 ----Next_Part(Thu_Mar_28_19_59_25_2024_076)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v15-0005-Row-pattern-recognition-patch-executor.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
end of thread, other threads:[~2024-03-28 10:30 UTC | newest] Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-01-07 08:04 [PATCH v45 3/3] Replace assuming a composite object on a scalar Dmitrii Dolgov <[email protected]> 2022-10-19 15:50 Standby recovers records from wrong timeline Ants Aasma <[email protected]> 2022-10-20 08:29 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 2022-10-20 08:34 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 2022-10-20 08:47 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 2022-10-20 11:44 ` Re: Standby recovers records from wrong timeline Ants Aasma <[email protected]> 2022-10-21 07:45 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 2022-10-21 08:12 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 2022-10-21 08:44 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 2022-10-21 09:38 ` Re: Standby recovers records from wrong timeline Kyotaro Horiguchi <[email protected]> 2022-10-21 09:48 ` Re: Standby recovers records from wrong timeline Ants Aasma <[email protected]> 2024-03-28 10:30 [PATCH v15 4/8] Row pattern recognition patch (planner). Tatsuo Ishii <[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