Thread: Re: Set AUTOCOMMIT to on in script output by pg_dump
On Wed, 09 Oct 2024 11:10:37 +0900 Shinya Kato <Shinya11.Kato@oss.nttdata.com> 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 <nagata@sraoss.co.jp>
On Tuesday, October 8, 2024, Yugo Nagata <nagata@sraoss.co.jp> wrote:
On Wed, 09 Oct 2024 11:10:37 +0900
Shinya Kato <Shinya11.Kato@oss.nttdata.com> 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.
>> 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
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Tuesday, October 8, 2024, Yugo Nagata <nagata@sraoss.co.jp> wrote:
>> On Wed, 09 Oct 2024 11:10:37 +0900
>> Shinya Kato <Shinya11.Kato@oss.nttdata.com> 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
On Tuesday, October 8, 2024, Tom Lane <tgl@sss.pgh.pa.us> wrote:
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Tuesday, October 8, 2024, Yugo Nagata <nagata@sraoss.co.jp> wrote:
>> On Wed, 09 Oct 2024 11:10:37 +0900
>> Shinya Kato <Shinya11.Kato@oss.nttdata.com> 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.
On Tue, 8 Oct 2024 21:37:38 -0700 "David G. Johnston" <david.g.johnston@gmail.com> wrote: > On Tuesday, October 8, 2024, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > "David G. Johnston" <david.g.johnston@gmail.com> writes: > > > On Tuesday, October 8, 2024, Yugo Nagata <nagata@sraoss.co.jp> wrote: > > >> On Wed, 09 Oct 2024 11:10:37 +0900 > > >> Shinya Kato <Shinya11.Kato@oss.nttdata.com> 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 <nagata@sraoss.co.jp>
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