Обсуждение: RE: Patch for migration of the pg_commit_ts directory

Hi,

Thanks for updating the patch. Mostly looks good.

I ran pgindent locally and indents of my part were incorrect. PSA the
included version. 

I have no comments anymore. Thanks for the great work.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Hackers,

I found that v7 needs rebased. Copyright was also updated in the attached patch.
I'm not the author of the patch though.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Hi,

> I didn't quite understand what I needed to do. 
> My assumptions:
> 1. You need to download the postgresql master branch and create a patch file on it.

This is correct. I periodically checked the commitfest app [1], and it has
reported [Needs rebase!] for weeks. Now it could be applied cleanly and tests
could be passed.

> 2. Replace the 2025 with 2026 header comment in all сhangeable patch files.

Since I cannot follow the sentence, let me clarify my understanding.

Basically, the copyright notation is maintained by the PostgreSQL community;
nothing for us to do. They are updated at the beginning of the year [2].
If you are proposing to add new files, however, they must contain the copyright
and be updated in the new year. It's not yet included in the codebase and is out
of scope for the community's maintenance.

Please ask me anything if you have more questions :-).

[1]: https://commitfest.postgresql.org/patch/6119/ 
[2]: https://github.com/postgres/postgres/commit/451c43974f8e199097d97624a4952ad0973cea61

Best regards,
Hayato Kuroda
FUJITSU LIMITED
 

On Mon, Jan 5, 2026 at 1:54 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> I found that v7 needs rebased. Copyright was also updated in the attached patch.
> I'm not the author of the patch though.
>

Can we update the use case of this patch in the commit message? One of
the use case I recall is to detect conflicts with accuracy after
upgrade. See docs [1] ("Commit timestamps and origin data are not
preserved during the upgrade. ..) I think this note needs an update
after this patch.

res = executeQueryOrDie(conn, "SELECT setting FROM pg_settings "
+ "WHERE name = 'track_commit_timestamp'");
+ track_commit_timestamp_on = strcmp(PQgetvalue(res, 0, 0), "on") == 0;

As this is a boolean variable, what if the user sets the value of this
GUC as true, will the above work correctly? Also, _on in the variable
name appears bit odd.
--
With Regards,
Amit Kapila.

Dear Amit,

> One of
> the use case I recall is to detect conflicts with accuracy after
> upgrade. See docs [1] ("Commit timestamps and origin data are not
> preserved during the upgrade. ..) I think this note needs an update
> after this patch.

Right, and code comments [a] should be also updated.

BTW, is there a possibility that we can export and import xmin of the conflict
slot to retain dead tuples even after the upgrade? Of course it's out-of-scope
from this thread.

> res = executeQueryOrDie(conn, "SELECT setting FROM pg_settings "
> + "WHERE name = 'track_commit_timestamp'");
> + track_commit_timestamp_on = strcmp(PQgetvalue(res, 0, 0), "on") == 0;
> 
> As this is a boolean variable, what if the user sets the value of this
> GUC as true, will the above work correctly?

Per my understanding, it is OK to use "on"/"off" notation. Below contains the
analysis.

pg_settings uses an SQL function pg_show_all_settings, and it is implemneted by
the C function show_all_settings(). And GetConfigOptionValues()->ShowGUCOption()
actualy create the output for the setting column in the view. According to the
function, the boolean would be the either "on" or "off" unless the GUC has show_hook.

```
    switch (record->vartype)
    {
        case PGC_BOOL:
            {
                const struct config_bool *conf = &record->_bool;

                if (conf->show_hook)
                    val = conf->show_hook();
                else
                    val = *conf->variable ? "on" : "off";
            }
            break;
```

It mactches my experiment below.

```
$ cat data_pub/postgresql.conf | grep track_commit_timestamp
track_commit_timestamp = true   # collect timestamp of transaction commit
$ psql -U postgres -p 5432
postgres=# SELECT setting FROM pg_settings WHERE name = 'track_commit_timestamp';
 setting 
---------
 on
(1 row)
```

> Also, _on in the variable name appears bit odd.

I have two options, thought?
 - commit_ts_is_enabled,
 - track_commit_timestmap, same as GUC variable.

[a]: https://github.com/postgres/postgres/blob/master/src/bin/pg_upgrade/pg_upgrade.c#L215

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Fri, Feb 20, 2026 at 4:35 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > One of
> > the use case I recall is to detect conflicts with accuracy after
> > upgrade. See docs [1] ("Commit timestamps and origin data are not
> > preserved during the upgrade. ..) I think this note needs an update
> > after this patch.
>
> Right, and code comments [a] should be also updated.
>

So, leaving aside update_delete, copying commit_ts could be helpful to
detect some other conflicts. You may want to once test the same and
show it here as part of use case establishment.

> BTW, is there a possibility that we can export and import xmin of the conflict
> slot to retain dead tuples even after the upgrade? Of course it's out-of-scope
> from this thread.
>

Yeah, this is a good point to explore separately. The key thing we
need to evaluate is whether the rows corresponding to old_xids are
retained. We probably need to evaluate the epoch part as well in old
cluster's slot. We do call set_frozenxids() for new cluster that might
have some impact on the functionality you are looking at.

> > Also, _on in the variable name appears bit odd.
>
> I have two options, thought?
>  - commit_ts_is_enabled,

This looks reasonable to me.

--
With Regards,
Amit Kapila.

Dear Amit,

> > Right, and code comments [a] should be also updated.
> >
> 
> So, leaving aside update_delete, copying commit_ts could be helpful to
> detect some other conflicts. You may want to once test the same and
> show it here as part of use case establishment.

I confirmed that {update|delete}_origin_differs could be detected with the
following steps.

0.
Constructed pub-sub replication system. track_commit_timestamp=on was set on the
subscriber, and the table below was defined on both clusters

```
              Table "public.employee"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 id     | integer |           | not null |
 salary | integer |           |          |
Indexes:
    "employee_pkey" PRIMARY KEY, btree (id)
```


1. 
Inserted a tuple on the publisher

```
pub=# INSERT INTO employee VALUES (1, 100);
INSERT 0 1
```

2.
UPDATEd the replicated tuple on the subscriber. Confirmed that commit timestamps
were stored.

```
sub=# SELECT * FROM pg_last_committed_xact();
 xid |           timestamp           | roident
-----+-------------------------------+---------
 738 | 2026-02-23 13:17:19.263146+09 |       1
(1 row)
sub=# UPDATE employee SET salary = 10 WHERE id = 1;
UPDATE 1
sub=# SELECT * FROM pg_last_committed_xact();
 xid |           timestamp           | roident
-----+-------------------------------+---------
 739 | 2026-02-23 13:17:33.230773+09 |       0
(1 row)
```

3.
Ran pg_upgrade to upgrade the subscriber. The new cluster must also set
track_commit_timestamp to on.

4.
Confirmed commit timestamps could be migrated.

```
new=# SELECT * FROM pg_xact_commit_timestamp_origin('739');
           timestamp           | roident
-------------------------------+---------
 2026-02-23 13:17:33.230773+09 |       0
(1 row)
```

5.
UPDATEd the tuple on the publisher.

```
pub=# UPDATE employee SET salary = 200 WHERE id = 1;
UPDATE 1
```

6.
update_origin_differs conflict was detected on the new subscriber.

```
LOG:  conflict detected on relation "public.employee": conflict=update_origin_differs
DETAIL:  Updating the row that was modified locally in transaction 739 at 2026-02-23 13:17:33.230773+09: local row (1,
10),remote row (1, 200), replica identity (id)=(1).
 
CONTEXT:  processing remote data for replication origin "pg_16402" during message type "UPDATE" for replication target
relation"public.employee" in transaction 745, finished at 0/018A4C6
 
```

One debatable point is whether we should include this in the TAP test. Personally
It's not necessary to simplify the test; either one is OK.


Not sure it is OK, but I created updated patches and considered a commit message.
0001 was not changed from the original, and 0002 addressed comments from you and
contained the commit message. For now I added my name as one of the author, but
OK to be listed as reviewer. Most of parts were written by Sergey.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Sergey,

You seemed to merge the patch correctly, but missing update the commit message.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Sergey,

> I don't understand what I need to do.
>Subject: [PATCH v10] Migration of the pg_commit_ts directory
> I have specified a new version in the patch. Do I need to add anything else?

When we see the commit message in postgres repo, you can see that each commit
contain the background, what we did in the commit, considerations, and etc.
Compared with them, the description you added seems insufficient.
Of course the committer will take care, but Amit suggested [1] to write a draft.
The basic style guide is determined in [2].

Actually I wrote a draft for the patch set in v9-0002, but you missed. You may
able to refer it, and re-create new version or use the same one.

[1]: https://www.postgresql.org/message-id/CAA4eK1KMuhaH%3DQgzJCwQgawuybosq-JMWWT9qSqyMZNV1MWjtg%40mail.gmail.com
[2]: https://wiki.postgresql.org/wiki/Commit_Message_Guidance

Best regards,
Hayato Kuroda
FUJITSU LIMITED