public inbox for [email protected]
help / color / mirror / Atom feedRe: Set AUTOCOMMIT to on in script output by pg_dump
15+ messages / 7 participants
[nested] [flat]
* Re: Set AUTOCOMMIT to on in script output by pg_dump
@ 2024-10-09 03:03 Yugo Nagata <[email protected]>
2024-10-09 03:53 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Yugo Nagata @ 2024-10-09 03:03 UTC (permalink / raw)
To: Shinya Kato <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Wed, 09 Oct 2024 11:10:37 +0900
Shinya Kato <[email protected]> wrote:
> Hi hackers!
>
> When SQL scripts created with pg_dump/pg_dumpall/pg_restore are executed
> in psql with AUTOCOMMIT turned off, they will not succeed in many cases.
> This is because the script contains SQL statements that cannot be
> executed within a transaction block.
>
> If you simply add set AUTOCOMMIT on to the scripts created by
> pg_dump/pg_dumpall/pg_restore, they will work fine.
> A patch is attached
>
> No documentation has been added as we could not find any documentation
> on the details in the script.
>
> Do you think?
I am not sure if it is good to include psql's meta-command in pg_dump/pg_dumpall
results. Can we assume users will always use psql to restore the pg_dump results?
Regards,
Yugo Nagata
--
Yugo Nagata <[email protected]>
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Set AUTOCOMMIT to on in script output by pg_dump
2024-10-09 03:03 Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
@ 2024-10-09 03:53 ` David G. Johnston <[email protected]>
2024-10-09 03:58 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Tatsuo Ishii <[email protected]>
2024-10-09 04:10 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Tom Lane <[email protected]>
0 siblings, 2 replies; 15+ messages in thread
From: David G. Johnston @ 2024-10-09 03:53 UTC (permalink / raw)
To: Yugo Nagata <[email protected]>; +Cc: Shinya Kato <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tuesday, October 8, 2024, Yugo Nagata <[email protected]> wrote:
> On Wed, 09 Oct 2024 11:10:37 +0900
> Shinya Kato <[email protected]> wrote:
>
> > Hi hackers!
> >
> > When SQL scripts created with pg_dump/pg_dumpall/pg_restore are executed
> > in psql with AUTOCOMMIT turned off, they will not succeed in many cases.
> > This is because the script contains SQL statements that cannot be
> > executed within a transaction block.
> >
> > If you simply add set AUTOCOMMIT on to the scripts created by
> > pg_dump/pg_dumpall/pg_restore, they will work fine.
> > A patch is attached
> >
>
> I am not sure if it is good to include psql's meta-command in
> pg_dump/pg_dumpall
> results. Can we assume users will always use psql to restore the pg_dump
> results?
Agreed. If we aren’t already outputting psql-only stuff I am a strong -1
for making this the first such case.
It would be nice to describe exactly when there is a problem as well since
very few things require being outside of a transaction. There might be
documentation or code patches possible here to improve matters (like adding
a switch to output begin/commit in the places we’re a user might want
single-transaction behavior) but this approach breaks well-established
encapsulation and overrides user expectations in a bad way (since
autocommit=on is the default they choose to turn it off so turning it back
on silently - not even documented - is bad.)
David J.
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Set AUTOCOMMIT to on in script output by pg_dump
2024-10-09 03:03 Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
2024-10-09 03:53 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
@ 2024-10-09 03:58 ` Tatsuo Ishii <[email protected]>
1 sibling, 0 replies; 15+ messages in thread
From: Tatsuo Ishii @ 2024-10-09 03:58 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]
>> I am not sure if it is good to include psql's meta-command in
>> pg_dump/pg_dumpall
>> results. Can we assume users will always use psql to restore the pg_dump
>> results?
>
>
> Agreed. If we aren’t already outputting psql-only stuff I am a strong -1
> for making this the first such case.
I think the pg_dumpall output already includes "\connect".
> It would be nice to describe exactly when there is a problem as well since
> very few things require being outside of a transaction. There might be
> documentation or code patches possible here to improve matters (like adding
> a switch to output begin/commit in the places we’re a user might want
> single-transaction behavior) but this approach breaks well-established
> encapsulation and overrides user expectations in a bad way (since
> autocommit=on is the default they choose to turn it off so turning it back
> on silently - not even documented - is bad.)
+1.
Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Set AUTOCOMMIT to on in script output by pg_dump
2024-10-09 03:03 Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
2024-10-09 03:53 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
@ 2024-10-09 04:10 ` Tom Lane <[email protected]>
2024-10-09 04:37 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
1 sibling, 1 reply; 15+ messages in thread
From: Tom Lane @ 2024-10-09 04:10 UTC (permalink / raw)
To: David G. Johnston <[email protected]>; +Cc: Yugo Nagata <[email protected]>; Shinya Kato <[email protected]>; PostgreSQL Hackers <[email protected]>
"David G. Johnston" <[email protected]> writes:
> On Tuesday, October 8, 2024, Yugo Nagata <[email protected]> wrote:
>> On Wed, 09 Oct 2024 11:10:37 +0900
>> Shinya Kato <[email protected]> wrote:
>>> When SQL scripts created with pg_dump/pg_dumpall/pg_restore are executed
>>> in psql with AUTOCOMMIT turned off, they will not succeed in many cases.
> Agreed. If we aren’t already outputting psql-only stuff I am a strong -1
> for making this the first such case.
I really doubt that this is the only way in which you can break a
pg_dump script by executing it in a non-default psql environment.
We'd likely be better advised to spend some documentation effort
recommending that pg_dump scripts be executed under "psql --no-psqlrc".
If AUTOCOMMIT were a mainstream feature then maybe it'd be worth
doing something about this, but IMO it's a deprecated backwater,
so I'm not very excited about it.
If we do want to do something about it, the patch needs more thought
about where to put the additional output. As an example, it looks
like it breaks the expectation that pg_dump-to-text should generate
output identical to pg_dump-to-archive followed by pg_restore-to-text.
> ... but this approach breaks well-established
> encapsulation and overrides user expectations in a bad way (since
> autocommit=on is the default they choose to turn it off so turning it back
> on silently - not even documented - is bad.)
That particular angle doesn't bother me so much, because pg_dump
scripts already feel free to change search_path as well as a bunch
of other server parameters.
regards, tom lane
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Set AUTOCOMMIT to on in script output by pg_dump
2024-10-09 03:03 Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
2024-10-09 03:53 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
2024-10-09 04:10 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Tom Lane <[email protected]>
@ 2024-10-09 04:37 ` David G. Johnston <[email protected]>
2024-10-09 06:15 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: David G. Johnston @ 2024-10-09 04:37 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Yugo Nagata <[email protected]>; Shinya Kato <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tuesday, October 8, 2024, Tom Lane <[email protected]> wrote:
> "David G. Johnston" <[email protected]> writes:
> > On Tuesday, October 8, 2024, Yugo Nagata <[email protected]> wrote:
> >> On Wed, 09 Oct 2024 11:10:37 +0900
> >> Shinya Kato <[email protected]> wrote:
> >>> When SQL scripts created with pg_dump/pg_dumpall/pg_restore are
> executed
> >>> in psql with AUTOCOMMIT turned off, they will not succeed in many
> cases.
>
> > Agreed. If we aren’t already outputting psql-only stuff I am a strong -1
> > for making this the first such case.
>
> I really doubt that this is the only way in which you can break a
> pg_dump script by executing it in a non-default psql environment.
> We'd likely be better advised to spend some documentation effort
> recommending that pg_dump scripts be executed under "psql --no-psqlrc".
+1
Reinforcing that our output script basically assumes a default execution
environment seems worth mentioning even if it seems self-evident once it’s
said.
>
>
> ... but this approach breaks well-established
> > encapsulation and overrides user expectations in a bad way (since
> > autocommit=on is the default they choose to turn it off so turning it
> back
> > on silently - not even documented - is bad.)
>
> That particular angle doesn't bother me so much, because pg_dump
> scripts already feel free to change search_path as well as a bunch
> of other server parameters.
>
I wasn’t referring to the idea these should be restorable on non-PostgreSQL
systems though, only that if someone wanted to just open a connection in
their rust driver and send this text through that session it will (mostly?)
work.
pg_dumpall, though, is fundamentally tied to psql if databases are dumped,
if the resultant script has to be platform independently executable. I’m
open to a patch addressing this more narrowly but I’m still thinking that
we should be telling the user to use defaults instead of enforcing
ourselves.
David J.
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Set AUTOCOMMIT to on in script output by pg_dump
2024-10-09 03:03 Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
2024-10-09 03:53 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
2024-10-09 04:10 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Tom Lane <[email protected]>
2024-10-09 04:37 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
@ 2024-10-09 06:15 ` Yugo Nagata <[email protected]>
2024-10-10 05:56 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Shinya Kato <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Yugo Nagata @ 2024-10-09 06:15 UTC (permalink / raw)
To: David G. Johnston <[email protected]>; +Cc: Tom Lane <[email protected]>; Shinya Kato <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, 8 Oct 2024 21:37:38 -0700
"David G. Johnston" <[email protected]> wrote:
> On Tuesday, October 8, 2024, Tom Lane <[email protected]> wrote:
>
> > "David G. Johnston" <[email protected]> writes:
> > > On Tuesday, October 8, 2024, Yugo Nagata <[email protected]> wrote:
> > >> On Wed, 09 Oct 2024 11:10:37 +0900
> > >> Shinya Kato <[email protected]> wrote:
> > >>> When SQL scripts created with pg_dump/pg_dumpall/pg_restore are
> > executed
> > >>> in psql with AUTOCOMMIT turned off, they will not succeed in many
> > cases.
> >
> > > Agreed. If we aren’t already outputting psql-only stuff I am a strong -1
> > > for making this the first such case.
> >
> > I really doubt that this is the only way in which you can break a
> > pg_dump script by executing it in a non-default psql environment.
> > We'd likely be better advised to spend some documentation effort
> > recommending that pg_dump scripts be executed under "psql --no-psqlrc".
>
>
> +1
>
> Reinforcing that our output script basically assumes a default execution
> environment seems worth mentioning even if it seems self-evident once it’s
> said.
+1
Regards,
Yugo Nagata
--
Yugo Nagata <[email protected]>
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Set AUTOCOMMIT to on in script output by pg_dump
2024-10-09 03:03 Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
2024-10-09 03:53 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
2024-10-09 04:10 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Tom Lane <[email protected]>
2024-10-09 04:37 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
2024-10-09 06:15 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
@ 2024-10-10 05:56 ` Shinya Kato <[email protected]>
2024-10-17 00:32 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Shinya Kato <[email protected]>
2025-01-17 15:45 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Robert Treat <[email protected]>
0 siblings, 2 replies; 15+ messages in thread
From: Shinya Kato @ 2024-10-10 05:56 UTC (permalink / raw)
To: Yugo Nagata <[email protected]>; +Cc: David G. Johnston <[email protected]>; Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]>
Thank you all for the comments!
On 2024-10-09 15:15, Yugo Nagata wrote:
> On Tue, 8 Oct 2024 21:37:38 -0700
> "David G. Johnston" <[email protected]> wrote:
>
>> On Tuesday, October 8, 2024, Tom Lane <[email protected]> wrote:
>>
>> > "David G. Johnston" <[email protected]> writes:
>> > > On Tuesday, October 8, 2024, Yugo Nagata <[email protected]> wrote:
>> > >> On Wed, 09 Oct 2024 11:10:37 +0900
>> > >> Shinya Kato <[email protected]> wrote:
>> > >>> When SQL scripts created with pg_dump/pg_dumpall/pg_restore are
>> > executed
>> > >>> in psql with AUTOCOMMIT turned off, they will not succeed in many
>> > cases.
>> >
>> > > Agreed. If we aren’t already outputting psql-only stuff I am a strong -1
>> > > for making this the first such case.
>> >
>> > I really doubt that this is the only way in which you can break a
>> > pg_dump script by executing it in a non-default psql environment.
>> > We'd likely be better advised to spend some documentation effort
>> > recommending that pg_dump scripts be executed under "psql --no-psqlrc".
>>
>>
>> +1
>>
>> Reinforcing that our output script basically assumes a default
>> execution
>> environment seems worth mentioning even if it seems self-evident once
>> it’s
>> said.
>
> +1
While adding to the documentation is sufficient if users use it
correctly, users often behave unexpectedly. My intention was to
implement it in a way that works without issues even if misused.
However, since the prevailing opinion seems to favor simply updating the
documentation, I will proceed with that approach.
A new patch is attached.
I am not a native English, so corrections to the texts are welcome.
--
Regards,
Shinya Kato
NTT DATA GROUP CORPORATION
Attachments:
[text/x-diff] v2-0001-doc-Add-no-psqlrc-in-pg_dump-pg_dumpall-docs.patch (6.3K, ../../[email protected]/2-v2-0001-doc-Add-no-psqlrc-in-pg_dump-pg_dumpall-docs.patch)
download | inline diff:
From f2ac5156b8fc02b6b529d3aeef16d80f21bbe08c Mon Sep 17 00:00:00 2001
From: Shinya Kato <[email protected]>
Date: Thu, 10 Oct 2024 14:50:39 +0900
Subject: [PATCH v2] doc: Add --no-psqlrc in pg_dump/pg_dumpall docs
---
doc/src/sgml/backup.sgml | 16 +++++++++-------
doc/src/sgml/ref/pg_dump.sgml | 9 ++++++++-
doc/src/sgml/ref/pg_dumpall.sgml | 11 ++++++++++-
3 files changed, 27 insertions(+), 9 deletions(-)
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index e4e4c56cf1..21ad34f4e0 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -106,10 +106,10 @@ pg_dump <replaceable class="parameter">dbname</replaceable> > <replaceable cl
<para>
Text files created by <application>pg_dump</application> are intended to
- be read in by the <application>psql</application> program. The
- general command form to restore a dump is
+ be read in by the <application>psql</application> program with its default
+ settings. The general command form to restore a dump is
<synopsis>
-psql <replaceable class="parameter">dbname</replaceable> < <replaceable class="parameter">dumpfile</replaceable>
+psql --no-psqlrc <replaceable class="parameter">dbname</replaceable> < <replaceable class="parameter">dumpfile</replaceable>
</synopsis>
where <replaceable class="parameter">dumpfile</replaceable> is the
file output by the <application>pg_dump</application> command. The database <replaceable
@@ -117,7 +117,9 @@ psql <replaceable class="parameter">dbname</replaceable> < <replaceable class
command, so you must create it yourself from <literal>template0</literal>
before executing <application>psql</application> (e.g., with
<literal>createdb -T template0 <replaceable
- class="parameter">dbname</replaceable></literal>). <application>psql</application>
+ class="parameter">dbname</replaceable></literal>). To use
+ <application>psql</application> with its default settings, use the
+ <option>--no-psqlrc</option> option. <application>psql</application>
supports options similar to <application>pg_dump</application> for specifying
the database server to connect to and the user name to use. See
the <xref linkend="app-psql"/> reference page for more information.
@@ -141,7 +143,7 @@ psql <replaceable class="parameter">dbname</replaceable> < <replaceable class
behavior and have <application>psql</application> exit with an
exit status of 3 if an SQL error occurs:
<programlisting>
-psql --set ON_ERROR_STOP=on <replaceable>dbname</replaceable> < <replaceable>dumpfile</replaceable>
+psql --no-psqlrc --set ON_ERROR_STOP=on <replaceable>dbname</replaceable> < <replaceable>dumpfile</replaceable>
</programlisting>
Either way, you will only have a partially restored database.
Alternatively, you can specify that the whole dump should be
@@ -160,7 +162,7 @@ psql --set ON_ERROR_STOP=on <replaceable>dbname</replaceable> < <replaceable>
write to or read from pipes makes it possible to dump a database
directly from one server to another, for example:
<programlisting>
-pg_dump -h <replaceable>host1</replaceable> <replaceable>dbname</replaceable> | psql -h <replaceable>host2</replaceable> <replaceable>dbname</replaceable>
+pg_dump -h <replaceable>host1</replaceable> <replaceable>dbname</replaceable> | psql --no-psqlrc -h <replaceable>host2</replaceable> <replaceable>dbname</replaceable>
</programlisting>
</para>
@@ -205,7 +207,7 @@ pg_dumpall > <replaceable>dumpfile</replaceable>
</synopsis>
The resulting dump can be restored with <application>psql</application>:
<synopsis>
-psql -f <replaceable class="parameter">dumpfile</replaceable> postgres
+psql --no-psqlrc -f <replaceable class="parameter">dumpfile</replaceable> postgres
</synopsis>
(Actually, you can specify any existing database name to start from,
but if you are loading into an empty cluster then <literal>postgres</literal>
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index ffc29b04fb..b6c33c3c2f 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -1636,6 +1636,13 @@ CREATE DATABASE foo WITH TEMPLATE template0;
option will be automatically enabled by the subscriber if the subscription
had been originally created with <literal>two_phase = true</literal> option.
</para>
+
+ <para>
+ It is generally recommended to use the <option>--no-psqlrc</option> option
+ when restoring a database from a <application>pg_dump</application> script
+ to ensure a clean restore process and prevent potential conflicts with
+ existing <application>psql</application> configurations.
+ </para>
</refsect1>
<refsect1 id="pg-dump-examples" xreflabel="Examples">
@@ -1653,7 +1660,7 @@ CREATE DATABASE foo WITH TEMPLATE template0;
<literal>newdb</literal>:
<screen>
-<prompt>$</prompt> <userinput>psql -d newdb -f db.sql</userinput>
+<prompt>$</prompt> <userinput>psql --no-psqlrc -d newdb -f db.sql</userinput>
</screen>
</para>
diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
index 4d7c046468..f8db63326a 100644
--- a/doc/src/sgml/ref/pg_dumpall.sgml
+++ b/doc/src/sgml/ref/pg_dumpall.sgml
@@ -817,6 +817,15 @@ exclude database <replaceable class="parameter">PATTERN</replaceable>
database creation will fail for databases in non-default
locations.
</para>
+
+ <para>
+ It is generally recommended to use the <option>--no-psqlrc</option> option
+ when restoring a database from a <application>pg_dumpall</application> script
+ to ensure a clean restore process and prevent potential conflicts with
+ existing <application>psql</application> configurations. Additionally,
+ because the script includes <application>psql</application> meta-commands,
+ it is incompatible with clients other than <application>psql</application>.
+ </para>
</refsect1>
@@ -833,7 +842,7 @@ exclude database <replaceable class="parameter">PATTERN</replaceable>
<para>
To restore database(s) from this file, you can use:
<screen>
-<prompt>$</prompt> <userinput>psql -f db.out postgres</userinput>
+<prompt>$</prompt> <userinput>psql --no-psqlrc -f db.out postgres</userinput>
</screen>
It is not important to which database you connect here since the
script file created by <application>pg_dumpall</application> will
--
2.43.0
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Set AUTOCOMMIT to on in script output by pg_dump
2024-10-09 03:03 Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
2024-10-09 03:53 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
2024-10-09 04:10 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Tom Lane <[email protected]>
2024-10-09 04:37 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
2024-10-09 06:15 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
2024-10-10 05:56 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Shinya Kato <[email protected]>
@ 2024-10-17 00:32 ` Shinya Kato <[email protected]>
1 sibling, 0 replies; 15+ messages in thread
From: Shinya Kato @ 2024-10-17 00:32 UTC (permalink / raw)
To: Yugo Nagata <[email protected]>; +Cc: David G. Johnston <[email protected]>; Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]>
On 2024-10-10 14:56, Shinya Kato wrote:
> A new patch is attached.
> I am not a native English, so corrections to the texts are welcome.
I created a commit fest entry.
https://commitfest.postgresql.org/50/5306/
--
Regards,
Shinya Kato
NTT DATA GROUP CORPORATION
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Set AUTOCOMMIT to on in script output by pg_dump
2024-10-09 03:03 Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
2024-10-09 03:53 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
2024-10-09 04:10 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Tom Lane <[email protected]>
2024-10-09 04:37 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
2024-10-09 06:15 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
2024-10-10 05:56 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Shinya Kato <[email protected]>
@ 2025-01-17 15:45 ` Robert Treat <[email protected]>
2025-01-17 20:11 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Tom Lane <[email protected]>
1 sibling, 1 reply; 15+ messages in thread
From: Robert Treat @ 2025-01-17 15:45 UTC (permalink / raw)
To: Shinya Kato <[email protected]>; +Cc: Yugo Nagata <[email protected]>; David G. Johnston <[email protected]>; Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Oct 10, 2024 at 1:56 AM Shinya Kato
<[email protected]> wrote:
> Thank you all for the comments!
<snip>
> While adding to the documentation is sufficient if users use it
> correctly, users often behave unexpectedly. My intention was to
> implement it in a way that works without issues even if misused.
> However, since the prevailing opinion seems to favor simply updating the
> documentation, I will proceed with that approach.
>
> A new patch is attached.
> I am not a native English, so corrections to the texts are welcome.
>
This looks pretty good to me. I think there are a couple of minor
grammar changes that could be made, and I think the pg_dumpall section
could use a couple tweaks, specifically 1) not all incantations of
pg_dumpall emit psql meta commands (-g comes to mind quickly) and ISTR
some non-psql clients honor psql meta commands, so I would lessen the
language around incompatibility, and 2) I think adding an explicit -f
for the database name in the pg_dumpall is clearer, and mirrors the
pg_dump example.
suggested diffs attached, let me know if you would like a consolidated patch
Robert Treat
https://xzilla.net
Attachments:
[application/octet-stream] xzilla-pg_dump-no-psqlrc.diff (2.2K, ../../CABV9wwM4vzO2BYDgw0N9Yd6TTrmrvT+eLrSfWE2z7Ra3CQtdXA@mail.gmail.com/2-xzilla-pg_dump-no-psqlrc.diff)
download | inline diff:
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 21ad34f4e0..15c8cdb8aa 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -106,7 +106,7 @@ pg_dump <replaceable class="parameter">dbname</replaceable> > <replaceable cl
<para>
Text files created by <application>pg_dump</application> are intended to
- be read in by the <application>psql</application> program with its default
+ be read in by the <application>psql</application> program using its default
settings. The general command form to restore a dump is
<synopsis>
psql --no-psqlrc <replaceable class="parameter">dbname</replaceable> < <replaceable class="parameter">dumpfile</replaceable>
diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
index 9208387ec1..636c27fe17 100644
--- a/doc/src/sgml/ref/pg_dumpall.sgml
+++ b/doc/src/sgml/ref/pg_dumpall.sgml
@@ -823,8 +823,8 @@ exclude database <replaceable class="parameter">PATTERN</replaceable>
when restoring a database from a <application>pg_dumpall</application> script
to ensure a clean restore process and prevent potential conflicts with
existing <application>psql</application> configurations. Additionally,
- because the script includes <application>psql</application> meta-commands,
- it is incompatible with clients other than <application>psql</application>.
+ because the script can include <application>psql</application> meta-commands,
+ it may be incompatible with clients other than <application>psql</application>.
</para>
</refsect1>
@@ -842,9 +842,9 @@ exclude database <replaceable class="parameter">PATTERN</replaceable>
<para>
To restore database(s) from this file, you can use:
<screen>
-<prompt>$</prompt> <userinput>psql --no-psqlrc -f db.out postgres</userinput>
+<prompt>$</prompt> <userinput>psql --no-psqlrc -f db.out -d postgres</userinput>
</screen>
- It is not important to which database you connect here since the
+ It is not important which database you connect to here since the
script file created by <application>pg_dumpall</application> will
contain the appropriate commands to create and connect to the saved
databases. An exception is that if you specified <option>--clean</option>,
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Set AUTOCOMMIT to on in script output by pg_dump
2024-10-09 03:03 Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
2024-10-09 03:53 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
2024-10-09 04:10 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Tom Lane <[email protected]>
2024-10-09 04:37 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
2024-10-09 06:15 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
2024-10-10 05:56 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Shinya Kato <[email protected]>
2025-01-17 15:45 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Robert Treat <[email protected]>
@ 2025-01-17 20:11 ` Tom Lane <[email protected]>
2025-01-22 13:01 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Shinya Kato <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Tom Lane @ 2025-01-17 20:11 UTC (permalink / raw)
To: Robert Treat <[email protected]>; +Cc: Shinya Kato <[email protected]>; Yugo Nagata <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
Robert Treat <[email protected]> writes:
> suggested diffs attached, let me know if you would like a consolidated patch
Sadly, the cfbot is now confused since it doesn't understand the idea
of an incremental patch. Somebody please post a consolidated patch.
For myself, I'd suggest writing the examples with -X not --no-psqlrc.
-X is shorter, it's more likely to be what people would actually
type, and it's not jarringly inconsistent with the adjacent use
of -h and other short-form switches. We can write the added
paragraphs like
It is generally recommended to use the <option>-X</option>
(<option>--no-psqlrc</option>) option when restoring a database ...
to provide clarity about what the switch does.
regards, tom lane
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Set AUTOCOMMIT to on in script output by pg_dump
2024-10-09 03:03 Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
2024-10-09 03:53 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
2024-10-09 04:10 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Tom Lane <[email protected]>
2024-10-09 04:37 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
2024-10-09 06:15 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
2024-10-10 05:56 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Shinya Kato <[email protected]>
2025-01-17 15:45 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Robert Treat <[email protected]>
2025-01-17 20:11 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Tom Lane <[email protected]>
@ 2025-01-22 13:01 ` Shinya Kato <[email protected]>
2025-01-22 21:33 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Robert Treat <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Shinya Kato @ 2025-01-22 13:01 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Robert Treat <[email protected]>; Yugo Nagata <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi, thank you for the reviews.
On 2025-01-18 00:45, Robert Treat wrote:
> This looks pretty good to me. I think there are a couple of minor
> grammar changes that could be made, and I think the pg_dumpall section
> could use a couple tweaks, specifically 1) not all incantations of
> pg_dumpall emit psql meta commands (-g comes to mind quickly) and ISTR
> some non-psql clients honor psql meta commands, so I would lessen the
> language around incompatibility, and 2) I think adding an explicit -f
> for the database name in the pg_dumpall is clearer, and mirrors the
> pg_dump example.
Thanks, I had no reason to oppose it, so I reflected that in the patch.
On 2025-01-18 05:11, Tom Lane wrote:
> Robert Treat <[email protected]> writes:
>> suggested diffs attached, let me know if you would like a consolidated
>> patch
>
> Sadly, the cfbot is now confused since it doesn't understand the idea
> of an incremental patch. Somebody please post a consolidated patch.
I've attached new consolidated patch.
> For myself, I'd suggest writing the examples with -X not --no-psqlrc.
> -X is shorter, it's more likely to be what people would actually
> type, and it's not jarringly inconsistent with the adjacent use
> of -h and other short-form switches. We can write the added
> paragraphs like
>
> It is generally recommended to use the <option>-X</option>
> (<option>--no-psqlrc</option>) option when restoring a database ...
>
> to provide clarity about what the switch does.
I agree to it and fixed the patch.
--
Regards,
Shinya Kato
NTT DATA GROUP CORPORATION
Attachments:
[text/x-diff] v3-0001-doc-Add-no-psqlrc-in-pg_dump-pg_dumpall-docs.patch (6.6K, ../../[email protected]/2-v3-0001-doc-Add-no-psqlrc-in-pg_dump-pg_dumpall-docs.patch)
download | inline diff:
From 664f89f8467f356024fe6e4e4ac0369b65238d41 Mon Sep 17 00:00:00 2001
From: Shinya Kato <[email protected]>
Date: Wed, 22 Jan 2025 21:53:19 +0900
Subject: [PATCH v3] doc: Add --no-psqlrc in pg_dump pg_dumpall docs
---
doc/src/sgml/backup.sgml | 16 +++++++++-------
doc/src/sgml/ref/pg_dump.sgml | 10 +++++++++-
doc/src/sgml/ref/pg_dumpall.sgml | 14 ++++++++++++--
3 files changed, 30 insertions(+), 10 deletions(-)
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index e4e4c56cf1..dd38a8b220 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -106,10 +106,10 @@ pg_dump <replaceable class="parameter">dbname</replaceable> > <replaceable cl
<para>
Text files created by <application>pg_dump</application> are intended to
- be read in by the <application>psql</application> program. The
- general command form to restore a dump is
+ be read in by the <application>psql</application> program using its default
+ settings. The general command form to restore a dump is
<synopsis>
-psql <replaceable class="parameter">dbname</replaceable> < <replaceable class="parameter">dumpfile</replaceable>
+psql -X <replaceable class="parameter">dbname</replaceable> < <replaceable class="parameter">dumpfile</replaceable>
</synopsis>
where <replaceable class="parameter">dumpfile</replaceable> is the
file output by the <application>pg_dump</application> command. The database <replaceable
@@ -117,7 +117,9 @@ psql <replaceable class="parameter">dbname</replaceable> < <replaceable class
command, so you must create it yourself from <literal>template0</literal>
before executing <application>psql</application> (e.g., with
<literal>createdb -T template0 <replaceable
- class="parameter">dbname</replaceable></literal>). <application>psql</application>
+ class="parameter">dbname</replaceable></literal>). To use
+ <application>psql</application> with its default settings, use the
+ <option>-X</option> (<option>--no-psqlrc</option>) option. <application>psql</application>
supports options similar to <application>pg_dump</application> for specifying
the database server to connect to and the user name to use. See
the <xref linkend="app-psql"/> reference page for more information.
@@ -141,7 +143,7 @@ psql <replaceable class="parameter">dbname</replaceable> < <replaceable class
behavior and have <application>psql</application> exit with an
exit status of 3 if an SQL error occurs:
<programlisting>
-psql --set ON_ERROR_STOP=on <replaceable>dbname</replaceable> < <replaceable>dumpfile</replaceable>
+psql -X --set ON_ERROR_STOP=on <replaceable>dbname</replaceable> < <replaceable>dumpfile</replaceable>
</programlisting>
Either way, you will only have a partially restored database.
Alternatively, you can specify that the whole dump should be
@@ -160,7 +162,7 @@ psql --set ON_ERROR_STOP=on <replaceable>dbname</replaceable> < <replaceable>
write to or read from pipes makes it possible to dump a database
directly from one server to another, for example:
<programlisting>
-pg_dump -h <replaceable>host1</replaceable> <replaceable>dbname</replaceable> | psql -h <replaceable>host2</replaceable> <replaceable>dbname</replaceable>
+pg_dump -h <replaceable>host1</replaceable> <replaceable>dbname</replaceable> | psql -X -h <replaceable>host2</replaceable> <replaceable>dbname</replaceable>
</programlisting>
</para>
@@ -205,7 +207,7 @@ pg_dumpall > <replaceable>dumpfile</replaceable>
</synopsis>
The resulting dump can be restored with <application>psql</application>:
<synopsis>
-psql -f <replaceable class="parameter">dumpfile</replaceable> postgres
+psql -X -f <replaceable class="parameter">dumpfile</replaceable> postgres
</synopsis>
(Actually, you can specify any existing database name to start from,
but if you are loading into an empty cluster then <literal>postgres</literal>
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index d66e901f51..8b572cdec0 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -1636,6 +1636,14 @@ CREATE DATABASE foo WITH TEMPLATE template0;
option will be automatically enabled by the subscriber if the subscription
had been originally created with <literal>two_phase = true</literal> option.
</para>
+
+ <para>
+ It is generally recommended to use the <option>-X</option>
+ (<option>--no-psqlrc</option>) option when restoring a database from a
+ <application>pg_dump</application> script to ensure a clean restore process
+ and prevent potential conflicts with existing <application>psql</application>
+ configurations.
+ </para>
</refsect1>
<refsect1 id="pg-dump-examples" xreflabel="Examples">
@@ -1653,7 +1661,7 @@ CREATE DATABASE foo WITH TEMPLATE template0;
<literal>newdb</literal>:
<screen>
-<prompt>$</prompt> <userinput>psql -d newdb -f db.sql</userinput>
+<prompt>$</prompt> <userinput>psql -X -d newdb -f db.sql</userinput>
</screen>
</para>
diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
index 014f279258..bfbe305b08 100644
--- a/doc/src/sgml/ref/pg_dumpall.sgml
+++ b/doc/src/sgml/ref/pg_dumpall.sgml
@@ -817,6 +817,16 @@ exclude database <replaceable class="parameter">PATTERN</replaceable>
database creation will fail for databases in non-default
locations.
</para>
+
+ <para>
+ It is generally recommended to use the <option>-X</option>
+ (<option>--no-psqlrc</option>) option when restoring a database from a
+ <application>pg_dumpall</application> script to ensure a clean restore
+ process and prevent potential conflicts with existing
+ <application>psql</application> configurations. Additionally,
+ because the script can include <application>psql</application> meta-commands,
+ it may be incompatible with clients other than <application>psql</application>.
+ </para>
</refsect1>
@@ -833,9 +843,9 @@ exclude database <replaceable class="parameter">PATTERN</replaceable>
<para>
To restore database(s) from this file, you can use:
<screen>
-<prompt>$</prompt> <userinput>psql -f db.out postgres</userinput>
+<prompt>$</prompt> <userinput>psql -X -f db.out -d postgres</userinput>
</screen>
- It is not important to which database you connect here since the
+ It is not important which database you connect to here since the
script file created by <application>pg_dumpall</application> will
contain the appropriate commands to create and connect to the saved
databases. An exception is that if you specified <option>--clean</option>,
--
2.39.3
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Set AUTOCOMMIT to on in script output by pg_dump
2024-10-09 03:03 Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
2024-10-09 03:53 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
2024-10-09 04:10 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Tom Lane <[email protected]>
2024-10-09 04:37 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
2024-10-09 06:15 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
2024-10-10 05:56 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Shinya Kato <[email protected]>
2025-01-17 15:45 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Robert Treat <[email protected]>
2025-01-17 20:11 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Tom Lane <[email protected]>
2025-01-22 13:01 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Shinya Kato <[email protected]>
@ 2025-01-22 21:33 ` Robert Treat <[email protected]>
2025-01-25 17:45 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Tom Lane <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Robert Treat @ 2025-01-22 21:33 UTC (permalink / raw)
To: Shinya Kato <[email protected]>; +Cc: Tom Lane <[email protected]>; Yugo Nagata <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, Jan 22, 2025 at 8:02 AM Shinya Kato
<[email protected]> wrote:
>
> Hi, thank you for the reviews.
>
> On 2025-01-18 00:45, Robert Treat wrote:
> > This looks pretty good to me. I think there are a couple of minor
> > grammar changes that could be made, and I think the pg_dumpall section
> > could use a couple tweaks, specifically 1) not all incantations of
> > pg_dumpall emit psql meta commands (-g comes to mind quickly) and ISTR
> > some non-psql clients honor psql meta commands, so I would lessen the
> > language around incompatibility, and 2) I think adding an explicit -f
> > for the database name in the pg_dumpall is clearer, and mirrors the
> > pg_dump example.
>
> Thanks, I had no reason to oppose it, so I reflected that in the patch.
>
> On 2025-01-18 05:11, Tom Lane wrote:
> > Robert Treat <[email protected]> writes:
> >> suggested diffs attached, let me know if you would like a consolidated
> >> patch
> >
> > Sadly, the cfbot is now confused since it doesn't understand the idea
> > of an incremental patch. Somebody please post a consolidated patch.
>
> I've attached new consolidated patch.
>
> > For myself, I'd suggest writing the examples with -X not --no-psqlrc.
> > -X is shorter, it's more likely to be what people would actually
> > type, and it's not jarringly inconsistent with the adjacent use
> > of -h and other short-form switches. We can write the added
> > paragraphs like
> >
> > It is generally recommended to use the <option>-X</option>
> > (<option>--no-psqlrc</option>) option when restoring a database ...
> >
> > to provide clarity about what the switch does.
>
> I agree to it and fixed the patch.
>
LGTM
Robert Treat
https://xzilla.net
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Set AUTOCOMMIT to on in script output by pg_dump
2024-10-09 03:03 Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
2024-10-09 03:53 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
2024-10-09 04:10 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Tom Lane <[email protected]>
2024-10-09 04:37 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
2024-10-09 06:15 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
2024-10-10 05:56 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Shinya Kato <[email protected]>
2025-01-17 15:45 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Robert Treat <[email protected]>
2025-01-17 20:11 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Tom Lane <[email protected]>
2025-01-22 13:01 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Shinya Kato <[email protected]>
2025-01-22 21:33 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Robert Treat <[email protected]>
@ 2025-01-25 17:45 ` Tom Lane <[email protected]>
2025-01-27 00:23 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Shinya Kato <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Tom Lane @ 2025-01-25 17:45 UTC (permalink / raw)
To: Robert Treat <[email protected]>; +Cc: Shinya Kato <[email protected]>; Yugo Nagata <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
Robert Treat <[email protected]> writes:
> On Wed, Jan 22, 2025 at 8:02 AM Shinya Kato
> <[email protected]> wrote:
>> I agree to it and fixed the patch.
> LGTM
LGTM too. Pushed with a couple of very minor tweaks.
regards, tom lane
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Set AUTOCOMMIT to on in script output by pg_dump
2024-10-09 03:03 Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
2024-10-09 03:53 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
2024-10-09 04:10 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Tom Lane <[email protected]>
2024-10-09 04:37 ` Re: Set AUTOCOMMIT to on in script output by pg_dump David G. Johnston <[email protected]>
2024-10-09 06:15 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
2024-10-10 05:56 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Shinya Kato <[email protected]>
2025-01-17 15:45 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Robert Treat <[email protected]>
2025-01-17 20:11 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Tom Lane <[email protected]>
2025-01-22 13:01 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Shinya Kato <[email protected]>
2025-01-22 21:33 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Robert Treat <[email protected]>
2025-01-25 17:45 ` Re: Set AUTOCOMMIT to on in script output by pg_dump Tom Lane <[email protected]>
@ 2025-01-27 00:23 ` Shinya Kato <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Shinya Kato @ 2025-01-27 00:23 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Robert Treat <[email protected]>; Yugo Nagata <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On 2025-01-26 02:45, Tom Lane wrote:
> Robert Treat <[email protected]> writes:
>> On Wed, Jan 22, 2025 at 8:02 AM Shinya Kato
>> <[email protected]> wrote:
>>> I agree to it and fixed the patch.
>
>> LGTM
>
> LGTM too. Pushed with a couple of very minor tweaks.
>
> regards, tom lane
Thank you for pushing!
--
Regards,
Shinya Kato
NTT DATA GROUP CORPORATION
^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH v9 2/2] handle relation statistics correctly during rewrites
@ 2025-11-04 13:52 Bertrand Drouvot <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Bertrand Drouvot @ 2025-11-04 13:52 UTC (permalink / raw)
Now that PGSTAT_KIND_RELATION is keyed by refilenode, we need to handle rewrites.
To do so, this patch:
- Adds PgStat_PendingRewrite, a new struct to track rewrite operations within
a transaction, storing the old locator, new locator, and original locator (for
rewrite chains). This allows stats to be copied from the original location to
the final location at commit time.
- Adds a new function, pgstat_mark_rewrite(), called when a table rewrite begins.
It records the rewrite operation in a local list and detects rewrite chains by
checking if the old_locator matches any existing new_locator, preserving the
chain's original_locator.
- Modifies pgstat_copy_relation_stats(), to accept RelFileLocators instead of
Relations, with a new increment parameter to accumulate stats (needed for rewrite
chains with DML between rewrites).
- Ensures that AtEOXact_PgStat_Relations(), AtPrepare_PgStat_Relations(),
pgstat_twophase_postcommit()/postabort() pgstat_drop_relation() handle the
PgStat_PendingRewrite list correctly.
Note that due to the new flush call in pgstat_twophase_postcommit() we can not
call GetCurrentTransactionStopTimestamp() in pgstat_relation_flush_cb(). So,
adding a check to handle this special case and call GetCurrentTimestamp() instead.
Note that we'd call GetCurrentTimestamp() only if there is a rewrite, so that
the GetCurrentTimestamp() extra cost should be negligible. Another solution
could be to trigger the flush from FinishPreparedTransaction() but that's not
worth the extra complexity.
The new pending_rewrites list is traversed in multiple places. The overhead
should be negligible in comparison to a rewrite and the list should not contain
a lot of rewrites in practice.
The pending_rewrites list is traversed in multiple places. In typical usage,
the list will contain only a few entries so the traversal cost is negligible (
furthermore in comparison to a rewrite).
---
src/backend/catalog/index.c | 2 +-
src/backend/commands/cluster.c | 5 +
src/backend/commands/tablecmds.c | 6 +
src/backend/utils/activity/pgstat_relation.c | 391 ++++++++++++++++++-
src/backend/utils/activity/pgstat_xact.c | 25 +-
src/backend/utils/cache/relcache.c | 6 +
src/include/pgstat.h | 5 +-
src/tools/pgindent/typedefs.list | 1 +
8 files changed, 424 insertions(+), 17 deletions(-)
92.8% src/backend/utils/activity/
4.9% src/backend/
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 8dea58ad96b..b71925a22c3 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1795,7 +1795,7 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
changeDependenciesOn(RelationRelationId, oldIndexId, newIndexId);
/* copy over statistics from old to new index */
- pgstat_copy_relation_stats(newClassRel, oldClassRel);
+ pgstat_copy_relation_stats(newClassRel->rd_locator, oldClassRel->rd_locator, false);
/* Copy data of pg_statistic from the old index to the new one */
CopyStatistics(oldIndexId, newIndexId);
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 2120c85ccb4..6155b12afab 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1196,6 +1196,11 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
rel1 = relation_open(r1, NoLock);
rel2 = relation_open(r2, NoLock);
+
+ /* Mark that a rewrite happened */
+ if (RELKIND_HAS_STORAGE(rel1->rd_rel->relkind))
+ pgstat_mark_rewrite(rel1->rd_locator, rel2->rd_locator);
+
rel2->rd_createSubid = rel1->rd_createSubid;
rel2->rd_newRelfilelocatorSubid = rel1->rd_newRelfilelocatorSubid;
rel2->rd_firstRelfilelocatorSubid = rel1->rd_firstRelfilelocatorSubid;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6b1a00ed477..9de70f321ed 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16884,6 +16884,7 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
Oid reltoastrelid;
RelFileNumber newrelfilenumber;
RelFileLocator newrlocator;
+ RelFileLocator oldrlocator;
List *reltoastidxids = NIL;
ListCell *lc;
@@ -16922,6 +16923,7 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
newrlocator = rel->rd_locator;
newrlocator.relNumber = newrelfilenumber;
newrlocator.spcOid = newTableSpace;
+ oldrlocator = rel->rd_locator;
/* hand off to AM to actually create new rel storage and copy the data */
if (rel->rd_rel->relkind == RELKIND_INDEX)
@@ -16934,6 +16936,10 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
table_relation_copy_data(rel, &newrlocator);
}
+ /* mark that a rewrite happened */
+ if (RELKIND_HAS_STORAGE(rel->rd_rel->relkind))
+ pgstat_mark_rewrite(oldrlocator, newrlocator);
+
/*
* Update the pg_class row.
*
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index 48bf93cae6e..b6ba8fdf440 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -30,6 +30,19 @@
#include "utils/syscache.h"
#include "utils/timestamp.h"
+/* Pending rewrite operations for stats copying */
+typedef struct PgStat_PendingRewrite
+{
+ RelFileLocator old_locator;
+ RelFileLocator new_locator;
+ RelFileLocator original_locator;
+ int nest_level; /* Transaction nesting level where rewrite
+ * occurred */
+ struct PgStat_PendingRewrite *next;
+} PgStat_PendingRewrite;
+
+/* The pending rewrites list for current transaction */
+static PgStat_PendingRewrite *pending_rewrites = NULL;
/* Record that's written to 2PC state file when pgstat state is persisted */
typedef struct TwoPhasePgStatRecord
@@ -43,6 +56,8 @@ typedef struct TwoPhasePgStatRecord
PgStat_Counter deleted_pre_truncdrop;
RelFileLocator locator; /* table's rd_locator */
bool truncdropped; /* was the relation truncated/dropped? */
+ RelFileLocator rewrite_old_locator;
+ int rewrite_nest_level;
} TwoPhasePgStatRecord;
@@ -54,27 +69,70 @@ static void restore_truncdrop_counters(PgStat_TableXactStatus *trans);
/*
- * Copy stats between relations. This is used for things like REINDEX
+ * Copy stats between RelFileLocator. This is used for things like REINDEX
* CONCURRENTLY.
*/
void
-pgstat_copy_relation_stats(Relation dst, Relation src)
+pgstat_copy_relation_stats(RelFileLocator dst, RelFileLocator src, bool increment)
{
PgStat_StatTabEntry *srcstats;
PgStatShared_Relation *dstshstats;
PgStat_EntryRef *dst_ref;
- srcstats = pgstat_fetch_stat_tabentry_ext(RelationGetRelid(src));
+ srcstats = (PgStat_StatTabEntry *) pgstat_fetch_entry(PGSTAT_KIND_RELATION,
+ src.dbOid,
+ RelFileLocatorToPgStatObjid(src));
if (!srcstats)
return;
dst_ref = pgstat_get_entry_ref_locked(PGSTAT_KIND_RELATION,
- dst->rd_rel->relisshared ? InvalidOid : MyDatabaseId,
- RelationGetRelid(dst),
+ dst.dbOid,
+ RelFileLocatorToPgStatObjid(dst),
false);
dstshstats = (PgStatShared_Relation *) dst_ref->shared_stats;
- dstshstats->stats = *srcstats;
+
+ if (!increment)
+ dstshstats->stats = *srcstats;
+ else
+ {
+ /* Increment those statistics */
+#define RELFSTAT_ACC(fld, stats_to_add) \
+ (dstshstats->stats.fld += stats_to_add->fld)
+ RELFSTAT_ACC(numscans, srcstats);
+ RELFSTAT_ACC(tuples_returned, srcstats);
+ RELFSTAT_ACC(tuples_fetched, srcstats);
+ RELFSTAT_ACC(tuples_inserted, srcstats);
+ RELFSTAT_ACC(tuples_updated, srcstats);
+ RELFSTAT_ACC(tuples_deleted, srcstats);
+ RELFSTAT_ACC(tuples_hot_updated, srcstats);
+ RELFSTAT_ACC(tuples_newpage_updated, srcstats);
+ RELFSTAT_ACC(live_tuples, srcstats);
+ RELFSTAT_ACC(dead_tuples, srcstats);
+ RELFSTAT_ACC(mod_since_analyze, srcstats);
+ RELFSTAT_ACC(ins_since_vacuum, srcstats);
+ RELFSTAT_ACC(blocks_fetched, srcstats);
+ RELFSTAT_ACC(blocks_hit, srcstats);
+ RELFSTAT_ACC(vacuum_count, srcstats);
+ RELFSTAT_ACC(autovacuum_count, srcstats);
+ RELFSTAT_ACC(analyze_count, srcstats);
+ RELFSTAT_ACC(autoanalyze_count, srcstats);
+ RELFSTAT_ACC(total_vacuum_time, srcstats);
+ RELFSTAT_ACC(total_autovacuum_time, srcstats);
+ RELFSTAT_ACC(total_analyze_time, srcstats);
+ RELFSTAT_ACC(total_autoanalyze_time, srcstats);
+#undef RELFSTAT_ACC
+
+ /* Replace those statistics */
+#define RELFSTAT_REP(fld, stats_to_rep) \
+ (dstshstats->stats.fld = stats_to_rep->fld)
+ RELFSTAT_REP(lastscan, srcstats);
+ RELFSTAT_REP(last_vacuum_time, srcstats);
+ RELFSTAT_REP(last_autovacuum_time, srcstats);
+ RELFSTAT_REP(last_analyze_time, srcstats);
+ RELFSTAT_REP(last_autoanalyze_time, srcstats);
+#undef RELFSTAT_REP
+ }
pgstat_unlock_entry(dst_ref);
}
@@ -136,6 +194,7 @@ void
pgstat_assoc_relation(Relation rel)
{
RelFileLocator locator;
+ PgStat_TableStatus *pgstat_info;
Assert(rel->pgstat_enabled);
Assert(rel->pgstat_info == NULL);
@@ -164,14 +223,54 @@ pgstat_assoc_relation(Relation rel)
locator.relNumber = rel->rd_id;
}
+ /*
+ * If this relation was rewritten during the current transaction we may be
+ * reopening it with its new RelFileLocator. In that case, continue using
+ * the stats entry associated with the old locator rather than creating a
+ * new one. This ensures all stats from before and after the rewrite are
+ * tracked in a single entry which will be properly copied to the new
+ * locator at transaction commit.
+ */
+ if (pending_rewrites != NULL)
+ {
+ PgStat_PendingRewrite *rewrite;
+
+ for (rewrite = pending_rewrites; rewrite != NULL; rewrite = rewrite->next)
+ {
+ if (locator.dbOid == rewrite->new_locator.dbOid &&
+ locator.spcOid == rewrite->new_locator.spcOid &&
+ locator.relNumber == rewrite->new_locator.relNumber)
+ {
+ pgstat_info = pgstat_prep_relation_pending(rewrite->old_locator);
+ goto found_entry;
+ }
+ }
+ }
+
/* Else find or make the PgStat_TableStatus entry, and update link */
- rel->pgstat_info = pgstat_prep_relation_pending(locator);
+ pgstat_info = pgstat_prep_relation_pending(locator);
+
+found_entry:
+ rel->pgstat_info = pgstat_info;
+
+ /*
+ * For relations stats, we key by physical file location, not by relation
+ * OID. This means during operations like ALTER TYPE it's possible that
+ * the relation OID changes but the relfilenode stays the same (no actual
+ * rewrite needed). Unlink the old relation first.
+ */
+ if (pgstat_info->relation != NULL &&
+ pgstat_info->relation != rel)
+ {
+ pgstat_info->relation->pgstat_info = NULL;
+ pgstat_info->relation = NULL;
+ }
/* don't allow link a stats to multiple relcache entries */
- Assert(rel->pgstat_info->relation == NULL);
+ Assert(pgstat_info->relation == NULL);
/* mark this relation as the owner */
- rel->pgstat_info->relation = rel;
+ pgstat_info->relation = rel;
}
/*
@@ -214,14 +313,37 @@ pgstat_drop_relation(Relation rel)
{
int nest_level = GetCurrentTransactionNestLevel();
PgStat_TableStatus *pgstat_info;
+ bool skip_transactional_drop = false;
/* don't track stats for relations without storage */
if (!RELKIND_HAS_STORAGE(rel->rd_rel->relkind))
return;
- pgstat_drop_transactional(PGSTAT_KIND_RELATION,
- rel->rd_locator.dbOid,
- RelFileLocatorToPgStatObjid(rel->rd_locator));
+ /* Check if this drop is part of a pending rewrite */
+ if (pending_rewrites != NULL)
+ {
+ PgStat_PendingRewrite *rewrite;
+
+ for (rewrite = pending_rewrites; rewrite != NULL; rewrite = rewrite->next)
+ {
+ if (rel->rd_locator.dbOid == rewrite->old_locator.dbOid &&
+ rel->rd_locator.spcOid == rewrite->old_locator.spcOid &&
+ rel->rd_locator.relNumber == rewrite->old_locator.relNumber)
+ {
+ skip_transactional_drop = true;
+ break;
+ }
+ }
+ }
+
+ /*
+ * If it is part of a rewrite, drop its stats later, for example in
+ * AtEOXact_PgStat_Relations(), so skip it here.
+ */
+ if (!skip_transactional_drop)
+ pgstat_drop_transactional(PGSTAT_KIND_RELATION,
+ rel->rd_locator.dbOid,
+ RelFileLocatorToPgStatObjid(rel->rd_locator));
if (!pgstat_should_count_relation(rel))
return;
@@ -666,6 +788,48 @@ AtEOXact_PgStat_Relations(PgStat_SubXactStatus *xact_state, bool isCommit)
}
tabstat->trans = NULL;
}
+
+ /* preserve the stats in case of rewrite */
+ if (isCommit && pending_rewrites != NULL)
+ {
+ PgStat_PendingRewrite *rewrite;
+ PgStat_PendingRewrite *prev = NULL;
+ PgStat_PendingRewrite *current = pending_rewrites;
+ PgStat_PendingRewrite *next;
+
+ /* reverse the rewrites list to process in chronological order */
+ while (current != NULL)
+ {
+ next = current->next;
+ current->next = prev;
+ prev = current;
+ current = next;
+ }
+
+ /* now process rewrites in chronological order */
+ for (rewrite = prev; rewrite != NULL; rewrite = rewrite->next)
+ {
+ PgStat_EntryRef *old_entry_ref;
+
+ old_entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION,
+ rewrite->old_locator.dbOid,
+ RelFileLocatorToPgStatObjid(rewrite->old_locator));
+
+ if (old_entry_ref && old_entry_ref->pending)
+ pgstat_relation_flush_cb(old_entry_ref, false);
+
+ pgstat_copy_relation_stats(rewrite->new_locator,
+ rewrite->old_locator, true);
+
+ /* drop old locator's stats */
+ if (!pgstat_drop_entry(PGSTAT_KIND_RELATION,
+ rewrite->old_locator.dbOid,
+ RelFileLocatorToPgStatObjid(rewrite->old_locator)))
+ pgstat_request_entry_refs_gc();
+ }
+ }
+
+ pending_rewrites = NULL;
}
/*
@@ -681,6 +845,30 @@ AtEOSubXact_PgStat_Relations(PgStat_SubXactStatus *xact_state, bool isCommit, in
PgStat_TableXactStatus *trans;
PgStat_TableXactStatus *next_trans;
+ /*
+ * If we don't commit then remove the associated rewrites if any, to keep
+ * the rewrite chain in sync with what will be eventually committed.
+ */
+ if (!isCommit)
+ {
+ PgStat_PendingRewrite **rewrite_ptr = &pending_rewrites;
+
+ while (*rewrite_ptr != NULL)
+ {
+ if ((*rewrite_ptr)->nest_level >= nestDepth)
+ {
+ PgStat_PendingRewrite *to_remove = *rewrite_ptr;
+
+ *rewrite_ptr = (*rewrite_ptr)->next;
+ pfree(to_remove);
+ }
+ else
+ {
+ rewrite_ptr = &((*rewrite_ptr)->next);
+ }
+ }
+ }
+
for (trans = xact_state->first; trans != NULL; trans = next_trans)
{
PgStat_TableStatus *tabstat;
@@ -760,11 +948,19 @@ void
AtPrepare_PgStat_Relations(PgStat_SubXactStatus *xact_state)
{
PgStat_TableXactStatus *trans;
+ PgStat_PendingRewrite *rewrite;
+ /*
+ * For each tabstat, find its matching rewrite and remove it from the
+ * pending rewrites list. This way, after processing all tabstats, pending
+ * rewrites will only contain rewrite only transactions.
+ */
for (trans = xact_state->first; trans != NULL; trans = trans->next)
{
PgStat_TableStatus *tabstat PG_USED_FOR_ASSERTS_ONLY;
TwoPhasePgStatRecord record;
+ PgStat_PendingRewrite **rewrite_ptr;
+ bool found_rewrite = false;
Assert(trans->nest_level == 1);
Assert(trans->upper == NULL);
@@ -784,10 +980,83 @@ AtPrepare_PgStat_Relations(PgStat_SubXactStatus *xact_state)
record.locator = tabstat->locator;
record.truncdropped = trans->truncdropped;
+ record.rewrite_nest_level = 0;
+
+ /*
+ * Look for a matching rewrite and remove it from pending rewrites. We
+ * check three possible matches:
+ *
+ * The new_locator when stats have been added after the rewrite. The
+ * old_locator when stats have been added before the rewrite but not
+ * after. The original_locator when this tabstat is part of a rewrite
+ * chain.
+ */
+ rewrite_ptr = &pending_rewrites;
+ while (*rewrite_ptr != NULL)
+ {
+ rewrite = *rewrite_ptr;
+
+ if ((record.locator.dbOid == rewrite->new_locator.dbOid &&
+ record.locator.spcOid == rewrite->new_locator.spcOid &&
+ record.locator.relNumber == rewrite->new_locator.relNumber) ||
+ (tabstat->locator.dbOid == rewrite->old_locator.dbOid &&
+ tabstat->locator.spcOid == rewrite->old_locator.spcOid &&
+ tabstat->locator.relNumber == rewrite->old_locator.relNumber) ||
+ (tabstat->locator.dbOid == rewrite->original_locator.dbOid &&
+ tabstat->locator.spcOid == rewrite->original_locator.spcOid &&
+ tabstat->locator.relNumber == rewrite->original_locator.relNumber))
+ {
+ /*
+ * Found matching rewrite. Record the rewrite information and
+ * remove this rewrite from the list since it's now handled.
+ */
+ record.rewrite_old_locator = rewrite->original_locator;
+ record.rewrite_nest_level = rewrite->nest_level;
+ record.locator = rewrite->new_locator;
+ found_rewrite = true;
+
+ /* Remove from pending_rewrites list */
+ *rewrite_ptr = rewrite->next;
+ pfree(rewrite);
+ break;
+ }
+ else
+ {
+ /* Move to next rewrite in the list */
+ rewrite_ptr = &(rewrite->next);
+ }
+ }
+
+ /* If no rewrite found, clear the rewrite fields */
+ if (!found_rewrite)
+ {
+ memset(&record.rewrite_old_locator, 0, sizeof(RelFileLocator));
+ }
+
+ RegisterTwoPhaseRecord(TWOPHASE_RM_PGSTAT_ID, 0,
+ &record, sizeof(TwoPhasePgStatRecord));
+ }
+
+ /*
+ * Now process any rewrites still pending. These are rewrite only
+ * transactions. We need to preserve their stats even though there's no
+ * tabstat entry for them.
+ */
+ for (rewrite = pending_rewrites; rewrite != NULL; rewrite = rewrite->next)
+ {
+ TwoPhasePgStatRecord record;
+
+ memset(&record, 0, sizeof(TwoPhasePgStatRecord));
+ record.locator = rewrite->new_locator;
+ record.rewrite_old_locator = rewrite->original_locator;
+ record.rewrite_nest_level = rewrite->nest_level;
+ record.truncdropped = false;
RegisterTwoPhaseRecord(TWOPHASE_RM_PGSTAT_ID, 0,
&record, sizeof(TwoPhasePgStatRecord));
}
+
+ pending_rewrites = NULL;
}
/*
@@ -810,6 +1079,8 @@ PostPrepare_PgStat_Relations(PgStat_SubXactStatus *xact_state)
tabstat = trans->parent;
tabstat->trans = NULL;
}
+
+ pending_rewrites = NULL;
}
/*
@@ -845,6 +1116,29 @@ pgstat_twophase_postcommit(FullTransactionId fxid, uint16 info,
pgstat_info->counts.changed_tuples +=
rec->tuples_inserted + rec->tuples_updated +
rec->tuples_deleted;
+
+ if (rec->rewrite_nest_level > 0)
+ {
+ PgStat_EntryRef *old_entry_ref;
+
+ /* Flush any pending stats for old locator first */
+ old_entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION,
+ rec->rewrite_old_locator.dbOid,
+ RelFileLocatorToPgStatObjid(rec->rewrite_old_locator));
+
+ if (old_entry_ref && old_entry_ref->pending)
+ pgstat_relation_flush_cb(old_entry_ref, false);
+
+ /* Copy stats from old to new locator */
+ pgstat_copy_relation_stats(rec->locator, rec->rewrite_old_locator,
+ true);
+
+ /* Drop old locator's stats */
+ if (!pgstat_drop_entry(PGSTAT_KIND_RELATION,
+ rec->rewrite_old_locator.dbOid,
+ RelFileLocatorToPgStatObjid(rec->rewrite_old_locator)))
+ pgstat_request_entry_refs_gc();
+ }
}
/*
@@ -859,9 +1153,26 @@ pgstat_twophase_postabort(FullTransactionId fxid, uint16 info,
{
TwoPhasePgStatRecord *rec = (TwoPhasePgStatRecord *) recdata;
PgStat_TableStatus *pgstat_info;
+ RelFileLocator target_locator;
+
+ /*
+ * For aborted transactions with rewrites (like TRUNCATE), we need to
+ * restore stats to the old locator, not the new one. The new locator
+ * should be dropped since the rewrite is being rolled back.
+ */
+ if (rec->rewrite_nest_level > 0)
+ {
+ /* Use the old locator */
+ target_locator = rec->rewrite_old_locator;
+ }
+ else
+ {
+ /* No rewrite, use the original locator */
+ target_locator = rec->locator;
+ }
/* Find or create a tabstat entry for the target locator */
- pgstat_info = pgstat_prep_relation_pending(rec->locator);
+ pgstat_info = pgstat_prep_relation_pending(target_locator);
/* Same math as in AtEOXact_PgStat, abort case */
if (rec->truncdropped)
@@ -916,7 +1227,17 @@ pgstat_relation_flush_cb(PgStat_EntryRef *entry_ref, bool nowait)
tabentry->numscans += lstats->counts.numscans;
if (lstats->counts.numscans)
{
- TimestampTz t = GetCurrentTransactionStopTimestamp();
+ TimestampTz t;
+
+ /*
+ * Checking the transaction state due to the flush call in
+ * pgstat_twophase_postcommit() that would break the assertion on the
+ * state in GetCurrentTransactionStopTimestamp().
+ */
+ if (!IsTransactionState())
+ t = GetCurrentTransactionStopTimestamp();
+ else
+ t = GetCurrentTimestamp();
if (t > tabentry->lastscan)
tabentry->lastscan = t;
@@ -1167,3 +1488,45 @@ pgstat_reloid_to_relfilelocator(Oid reloid, RelFileLocator *locator)
ReleaseSysCache(tuple);
return result;
}
+
+/*
+ * Mark that a relation rewrite has occurred, preserving the original locator
+ * so stats can be copied at transaction commit.
+ */
+void
+pgstat_mark_rewrite(RelFileLocator old_locator, RelFileLocator new_locator)
+{
+ PgStat_PendingRewrite *rewrite;
+ PgStat_PendingRewrite *existing;
+ RelFileLocator original_locator = old_locator;
+
+ for (existing = pending_rewrites; existing != NULL; existing = existing->next)
+ {
+ if (old_locator.dbOid == existing->new_locator.dbOid &&
+ old_locator.spcOid == existing->new_locator.spcOid &&
+ old_locator.relNumber == existing->new_locator.relNumber)
+ {
+ original_locator = existing->original_locator;
+ break;
+ }
+ }
+
+ /* Allocate in TopTransactionContext memory context */
+ rewrite = MemoryContextAlloc(TopTransactionContext,
+ sizeof(PgStat_PendingRewrite));
+
+ rewrite->old_locator = old_locator;
+ rewrite->new_locator = new_locator;
+ rewrite->original_locator = original_locator;
+ rewrite->nest_level = GetCurrentTransactionNestLevel();
+
+ /* Add to the list */
+ rewrite->next = pending_rewrites;
+ pending_rewrites = rewrite;
+}
+
+void
+pgstat_clear_rewrite(void)
+{
+ pending_rewrites = NULL;
+}
diff --git a/src/backend/utils/activity/pgstat_xact.c b/src/backend/utils/activity/pgstat_xact.c
index bc9864bd8d9..f8cf3755ce2 100644
--- a/src/backend/utils/activity/pgstat_xact.c
+++ b/src/backend/utils/activity/pgstat_xact.c
@@ -55,6 +55,8 @@ AtEOXact_PgStat(bool isCommit, bool parallel)
}
pgStatXactStack = NULL;
+ pgstat_clear_rewrite();
+
/* Make sure any stats snapshot is thrown away */
pgstat_clear_snapshot();
}
@@ -360,8 +362,29 @@ create_drop_transactional_internal(PgStat_Kind kind, Oid dboid, uint64 objid, bo
void
pgstat_create_transactional(PgStat_Kind kind, Oid dboid, uint64 objid)
{
- if (pgstat_get_entry_ref(kind, dboid, objid, false, NULL))
+ PgStat_EntryRef *entry_ref;
+
+ entry_ref = pgstat_get_entry_ref(kind, dboid, objid, false, NULL);
+
+ if (entry_ref)
{
+ /*
+ * For relations stats, we key by physical file location, not by
+ * relation OID. This means during operations like ALTER TYPE where
+ * the relation OID changes but the relfilenode stays the same (no
+ * actual rewrite needed), we'll find an existing entry.
+ *
+ * This is expected behavior, we want to preserve stats across the
+ * catalog change. Simply reset and recreate the entry for the new
+ * relation OID without warning.
+ */
+ if (kind == PGSTAT_KIND_RELATION)
+ {
+ pgstat_reset(kind, dboid, objid);
+ create_drop_transactional_internal(kind, dboid, objid, true);
+ return;
+ }
+
ereport(WARNING,
errmsg("resetting existing statistics for kind %s, db=%u, oid=%" PRIu64,
(pgstat_get_kind_info(kind))->name, dboid,
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 2d0cb7bcfd4..c98e5c51d63 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -85,6 +85,7 @@
#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
+#include "utils/pgstat_internal.h"
#include "utils/relmapper.h"
#include "utils/resowner.h"
#include "utils/snapmgr.h"
@@ -3780,6 +3781,7 @@ RelationSetNewRelfilenumber(Relation relation, char persistence)
MultiXactId minmulti = InvalidMultiXactId;
TransactionId freezeXid = InvalidTransactionId;
RelFileLocator newrlocator;
+ RelFileLocator oldrlocator = relation->rd_locator;
if (!IsBinaryUpgrade)
{
@@ -3951,6 +3953,10 @@ RelationSetNewRelfilenumber(Relation relation, char persistence)
table_close(pg_class, RowExclusiveLock);
+ /* Mark that a rewrite happened */
+ if (RELKIND_HAS_STORAGE(relation->rd_rel->relkind))
+ pgstat_mark_rewrite(oldrlocator, newrlocator);
+
/*
* Make the pg_class row change or relation map change visible. This will
* cause the relcache entry to get updated, too.
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 3102f86aa24..8750c025bbe 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -670,7 +670,7 @@ extern PgStat_FunctionCounts *find_funcstat_entry(Oid func_id);
extern void pgstat_create_relation(Relation rel);
extern void pgstat_drop_relation(Relation rel);
-extern void pgstat_copy_relation_stats(Relation dst, Relation src);
+extern void pgstat_copy_relation_stats(RelFileLocator dst, RelFileLocator src, bool increment);
extern void pgstat_init_relation(Relation rel);
extern void pgstat_assoc_relation(Relation rel);
@@ -682,6 +682,9 @@ extern void pgstat_report_vacuum(Relation rel, PgStat_Counter livetuples,
extern void pgstat_report_analyze(Relation rel,
PgStat_Counter livetuples, PgStat_Counter deadtuples,
bool resetcounter, TimestampTz starttime);
+extern void pgstat_mark_rewrite(RelFileLocator old_locator,
+ RelFileLocator new_locator);
+extern void pgstat_clear_rewrite(void);
/*
* If stats are enabled, but pending data hasn't been prepared yet, call
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 3451538565e..0ca3eae8026 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2268,6 +2268,7 @@ PgStat_KindInfo
PgStat_LocalState
PgStat_PendingDroppedStatsItem
PgStat_PendingIO
+PgStat_PendingRewrite
PgStat_SLRUStats
PgStat_ShmemControl
PgStat_Snapshot
--
2.34.1
--tV2Nti+lXhoeT6I5--
^ permalink raw reply [nested|flat] 15+ messages in thread
end of thread, other threads:[~2025-11-04 13:52 UTC | newest]
Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-10-09 03:03 Re: Set AUTOCOMMIT to on in script output by pg_dump Yugo Nagata <[email protected]>
2024-10-09 03:53 ` David G. Johnston <[email protected]>
2024-10-09 03:58 ` Tatsuo Ishii <[email protected]>
2024-10-09 04:10 ` Tom Lane <[email protected]>
2024-10-09 04:37 ` David G. Johnston <[email protected]>
2024-10-09 06:15 ` Yugo Nagata <[email protected]>
2024-10-10 05:56 ` Shinya Kato <[email protected]>
2024-10-17 00:32 ` Shinya Kato <[email protected]>
2025-01-17 15:45 ` Robert Treat <[email protected]>
2025-01-17 20:11 ` Tom Lane <[email protected]>
2025-01-22 13:01 ` Shinya Kato <[email protected]>
2025-01-22 21:33 ` Robert Treat <[email protected]>
2025-01-25 17:45 ` Tom Lane <[email protected]>
2025-01-27 00:23 ` Shinya Kato <[email protected]>
2025-11-04 13:52 [PATCH v9 2/2] handle relation statistics correctly during rewrites Bertrand Drouvot <[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