Обсуждение: 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.