| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | Paul A Jungwirth <pj(at)illuminatedcomputing(dot)com> |
| Cc: | Peter Eisentraut <peter(at)eisentraut(dot)org>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: SQL:2011 Application Time Update & Delete |
| Date: | 2025-11-13 03:31:57 |
| Message-ID: | 33505902-8524-430B-A39A-C71119937E91@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On Nov 12, 2025, at 17:31, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>
>
>
>> On Nov 5, 2025, at 03:12, Paul A Jungwirth <pj(at)illuminatedcomputing(dot)com> wrote:
>>
>> On Wed, Oct 29, 2025 at 11:02 PM Paul A Jungwirth
>> <pj(at)illuminatedcomputing(dot)com> wrote:
>>>
>>> On Tue, Oct 28, 2025 at 3:49 AM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>>>> On 24.10.25 19:08, Paul A Jungwirth wrote:
>>>>> The first 3 doc patches all apply to features that we released in v18,
>>>>> so it would be nice to get those reviewed/merged soon if possible.
>>>>
>>>> I have looked through the documentation patches 0001 through 0003.
>>>
>>> Thanks for taking a look! New patches attached; details below.
>>
>> Hi Hackers,
>>
>> Here is another set of patches. I added isolation tests for FOR
>> PORTION OF. In REPEATABLE READ and SERIALIZABLE you get
>> easy-to-predict results. In READ COMMITTED you get a lot of lost
>> updates/deletes, because the second operation doesn't see the
>> leftovers created by the first (and sometimes the first operation
>> changes the start/end times in a way that EvalPlanQual no longer sees
>> the being-changed row either). I think those results make sense, if
>> you think step-by-step what Postgres is doing, but they are not really
>> what a user wants.
>>
>> I tested the same sequences in MariaDB, and they also gave nonsense
>> results, although not always the same nonsense as Postgres. At
>> UNCOMMITTED READ it actually gave the results you'd want, but at that
>> level I assume you will have other problems.
>>
>> I also tested DB2. It doesn't have READ COMMITTED, but I think READ
>> STABILITY is the closest. At that level (as well as CURSOR STABILITY
>> and REPEATABLE READ), you get correct results.
>>
>> Back to Postgres, you can get "desired" results IN READ COMMITTED by
>> explicitly locking rows (with SELECT FOR UPDATE) just before
>> updating/deleting them. Since you acquire the lock before the
>> update/delete starts, there can be no new leftovers created within
>> that span of history, and the update/delete sees everything that is
>> there. The same approach also gives correct results in MariaDB. I
>> think it is just the way you have to do things with temporal tables in
>> READ COMMITTED whenever you expect concurrent updates to the same
>> history.
>>
>> I considered whether we should make EvalPlanQual (or something else)
>> automatically rescan for leftovers when it's a temporal operation.
>> Then you wouldn't have to explicitly lock anything. But it seems like
>> that is more than the isolation level "contract", and maybe even plain
>> violates it (but arguably not, if you say the update shouldn't *start*
>> until the other session commits). But since there is a workaround, and
>> since other RDBMSes also scramble temporal data in READ COMMITTED, and
>> since it is a lot of work and seems tricky, I didn't attempt it.
>>
>> Another idea (or maybe nearly the same thing) would be to
>> automatically do the same thing that SELECT FOR UPDATE is doing,
>> whenever we see a FOR PORTION OF DML command---i.e. scan for rows and
>> lock them first, then do the update. But that has similar issues. If
>> it adds locks the user doesn't expect, is it really the right thing?
>> And it means users pay the cost even when no concurrency is expected.
>> It offers strictly fewer options than requiring users to do SELECT FOR
>> UPDATE explicitly.
>>
>> The isolation tests are a separate patch for now, because they felt
>> like a significant chunk, and I wanted to emphasize them, but really
>> they should be part of the main FOR PORTION OF commit. Probably I'll
>> squash them in future submissions. That patch also makes some small
>> updates to a comment in ExecForPortionOf and the docs for
>> UPDATE/DELETE FOR PORTION OF, to raise awareness of the READ COMMITTED
>> issues.
>>
>> Rebased to 65f4976189.
>>
>> Yours,
>>
>> --
>> Paul ~{:-)
>> pj(at)illuminatedcomputing(dot)com
>> <v59-0003-Document-temporal-update-delete.patch><v59-0005-Add-UPDATE-DELETE-FOR-PORTION-OF.patch><v59-0001-Add-docs-section-for-temporal-tables-with-primar.patch><v59-0004-Add-range_minus_multi-and-multirange_minus_multi.patch><v59-0007-Add-tg_temporal-to-TriggerData.patch><v59-0002-Document-temporal-foreign-keys.patch><v59-0008-Look-up-more-temporal-foreign-key-helper-procs.patch><v59-0009-Add-CASCADE-SET-NULL-SET-DEFAULT-for-temporal-fo.patch><v59-0006-Add-isolation-tests-for-UPDATE-DELETE-FOR-PORTIO.patch><v59-0011-Add-PERIODs.patch><v59-0010-Expose-FOR-PORTION-OF-to-plpgsql-triggers.patch>
>
> I tried to review this patch. Though I “git reset” to commit 65f4976189, “git am” still failed at 0009.
>
> Today I only reviewed 0001, it was a happy reading. I found a small typo and got a suggestion:
>
> 1 - 0001
> ```
> + entity described by a table. In a typical non-temporal table, there is
> + single row for each entity. In a temporal table, an entity may have
> ```
>
> “There is single row” should be “there is a single row”.
>
>
> 2 - 0001 - The doc mentions rangetypes which is the key factor for defining a temporal table, can we add a hyper link on “rangetype” so that readers can easily jump to learn which rangetypes can be used.
>
> I will continue to review the rest of commits tomorrow.
>
I spent a hour reading through 0002-0004 and got my brain stuck. I’d stop here today, and maybe continue tomorrow.
A few more comments:
3 - 0002
```
+<programlisting>
+CREATE TABLE variants (
+ id integer NOT NULL,
+ product_id integer NOT NULL,
+ name text NOT NULL,
+ valid_at daterange NOT NULL,
+ CONSTRAINT variants_pkey
+ PRIMARY KEY (id, valid_at WITHOUT OVERLAPS),
+);
+</programlisting>
```
The common before ) is not needed.
4 - 0002
```
+ <para>
+
+ In a table, these records would be:
+<programlisting>
+ id | product_id | name | valid_at
+----+------------+--------+-------------------------
+ 8 | 5 | Medium | [2021-01-01,2023-06-01)
+ 9 | 5 | XXL | [2022-03-01,2024-06-01)
+</programlisting>
+ </para>
```
The blank line after “<para>” is not needed.
5 - 0003
```
+ zero, one, or two stretches of history that where not updated/deleted
```
Typo: where -> were
6 - 0004 - func-range.sgml
```
<row>
<entry role="func_table_entry"><para role="func_signature">
<indexterm>
<primary>multirange_minus_multi</primary>
</indexterm>
<function>multirange_minus_multi</function> ( <type>anymultirange</type>, <type>anymultirange</type> )
<returnvalue>setof anymultirange</returnvalue>
</para>
<para>
Returns the non-empty multirange(s) remaining after subtracting the second multirange from the first.
If the subtraction yields an empty multirange, no rows are returned.
Two rows are never returned, because a single multirange can always accommodate any result.
</para>
<para>
<literal>range_minus_multi('[0,10)'::int4range, '[3,4)'::int4range)</literal>
<returnvalue>{[0,3), [4,10)}</returnvalue>
</para></entry>
</row>
```
I believe in " <literal>range_minus_multi('[0,10)'::int4range, '[3,4)'::int4range)</literal>”, it should be “multirange_minus_multi”.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Amit Kapila | 2025-11-13 03:42:36 | Re: Rename sync_error_count to tbl_sync_error_count in subscription statistics |
| Previous Message | Mihail Nikalayeu | 2025-11-13 03:13:00 | Re: Logical replication: lost updates/deletes and invalid log messages caused by SnapshotDirty + concurrent updates |