Thread: pgsql: Add psql option: -1 or --single-transaction Simon Riggs
pgsql: Add psql option: -1 or --single-transaction Simon Riggs
From
momjian@postgresql.org (Bruce Momjian)
Date:
Log Message:
-----------
Add psql option:
-1 or --single-transaction
Simon Riggs
Modified Files:
--------------
pgsql/doc/src/sgml/ref:
pg_restore.sgml (r1.56 -> r1.57)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/ref/pg_restore.sgml.diff?r1=1.56&r2=1.57)
psql-ref.sgml (r1.158 -> r1.159)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/ref/psql-ref.sgml.diff?r1=1.158&r2=1.159)
pgsql/src/bin/pg_dump:
pg_backup.h (r1.37 -> r1.38)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/bin/pg_dump/pg_backup.h.diff?r1=1.37&r2=1.38)
pg_backup_archiver.c (r1.121 -> r1.122)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/bin/pg_dump/pg_backup_archiver.c.diff?r1=1.121&r2=1.122)
pg_restore.c (r1.73 -> r1.74)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/bin/pg_dump/pg_restore.c.diff?r1=1.73&r2=1.74)
pgsql/src/bin/psql:
command.c (r1.160 -> r1.161)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/bin/psql/command.c.diff?r1=1.160&r2=1.161)
command.h (r1.23 -> r1.24)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/bin/psql/command.h.diff?r1=1.23&r2=1.24)
help.c (r1.108 -> r1.109)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/bin/psql/help.c.diff?r1=1.108&r2=1.109)
startup.c (r1.129 -> r1.130)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/bin/psql/startup.c.diff?r1=1.129&r2=1.130)
Isn't this patch emitting the BEGIN at the wrong place?
--
-- PostgreSQL database dump
--
SET client_encoding = 'SQL_ASCII';
SET check_function_bodies = false;
SET client_min_messages = warning;
BEGIN;
--
-- Name: SCHEMA public; Type: COMMENT; Schema: -; Owner: postgres
--
COMMENT ON SCHEMA public IS 'Standard public schema';
That's not what I would call guaranteeing that all the commands execute
successfully. What if the client encoding is unrecognized, for
instance?
regards, tom lane
> BEGIN; > > -- > -- Name: SCHEMA public; Type: COMMENT; Schema: -; Owner: postgres > -- > > COMMENT ON SCHEMA public IS 'Standard public schema'; > > That's not what I would call guaranteeing that all the commands execute > successfully. What if the client encoding is unrecognized, for > instance? How does this interact with the begin/commit placed around LOB dumps? Chris
Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes:
> How does this interact with the begin/commit placed around LOB dumps?
Good question ... right offhand it looks like there are some broken
things there (try grepping bin/pg_dump/ for BEGIN).
regards, tom lane
> Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes: >> How does this interact with the begin/commit placed around LOB dumps? > > Good question ... right offhand it looks like there are some broken > things there (try grepping bin/pg_dump/ for BEGIN). I suggest you just suppress the begin/commit for each LOB if --single-transaction is specified, as it'd be in a single transaction anyway... Chris
>> Good question ... right offhand it looks like there are some broken >> things there (try grepping bin/pg_dump/ for BEGIN). > > I suggest you just suppress the begin/commit for each LOB if > --single-transaction is specified, as it'd be in a single transaction > anyway... Or...it could create and release a particular savepoint I suppose...I don't know if there's much point in that though. Chris
On Tue, 2006-02-14 at 12:21 +0800, Christopher Kings-Lynne wrote: > >> Good question ... right offhand it looks like there are some broken > >> things there (try grepping bin/pg_dump/ for BEGIN). > > > > I suggest you just suppress the begin/commit for each LOB if > > --single-transaction is specified, as it'd be in a single transaction > > anyway... > > > Or...it could create and release a particular savepoint I suppose...I > don't know if there's much point in that though. Didn't seem much point trying to remove it, Best Regards, Simon Riggs
On Mon, 2006-02-13 at 16:02 -0500, Tom Lane wrote: > Isn't this patch emitting the BEGIN at the wrong place? > > -- > -- PostgreSQL database dump > -- > > SET client_encoding = 'SQL_ASCII'; > SET check_function_bodies = false; > SET client_min_messages = warning; > > BEGIN; > > -- > -- Name: SCHEMA public; Type: COMMENT; Schema: -; Owner: postgres > -- > > COMMENT ON SCHEMA public IS 'Standard public schema'; > > That's not what I would call guaranteeing that all the commands execute > successfully. What if the client encoding is unrecognized, for > instance? I'll move it then. Best Regards, Simon Riggs
>> Or...it could create and release a particular savepoint I suppose...I >> don't know if there's much point in that though. > > Didn't seem much point trying to remove it, Huh? But it'll cause a total failure of dump restore? Chris
On Tue, 2006-02-14 at 16:55 +0800, Christopher Kings-Lynne wrote: > >> Or...it could create and release a particular savepoint I suppose...I > >> don't know if there's much point in that though. > > > > Didn't seem much point trying to remove it, > > Huh? But it'll cause a total failure of dump restore? Perhaps you can explain further? My understanding was that the desired functionality was that any failure would cause all aspects of the load to fail also, so I don't see any problem with that; clearly I need to listen to your thoughts. Best Regards, Simon Riggs
On Mon, 2006-02-13 at 16:02 -0500, Tom Lane wrote: > Isn't this patch emitting the BEGIN at the wrong place? > That's not what I would call guaranteeing that all the commands execute > successfully. What if the client encoding is unrecognized, for > instance? Please let me know if you are going to make the changes yourself. I agreed with your objection but was not immediately available because of the time difference between us. I have just recoded, but now see you did this already before I even read my email. Thanks for reviewing the patch and taking time to make changes. Best Regards, Simon Riggs
Simon Riggs <simon@2ndquadrant.com> writes:
> On Tue, 2006-02-14 at 16:55 +0800, Christopher Kings-Lynne wrote:
>> Huh? But it'll cause a total failure of dump restore?
> Perhaps you can explain further?
As the code stands, a restore involving blobs plus --single-transaction
produces
BEGIN;
...
BEGIN;
...
COMMIT;
...
COMMIT;
which does *not* have the intended behavior because BEGIN does not nest.
This is a must-fix, else we may as well revert the patch entirely,
because it does not work.
regards, tom lane
On Tue, 2006-02-14 at 09:35 -0500, Tom Lane wrote: > Simon Riggs <simon@2ndquadrant.com> writes: > > On Tue, 2006-02-14 at 16:55 +0800, Christopher Kings-Lynne wrote: > >> Huh? But it'll cause a total failure of dump restore? > > > Perhaps you can explain further? > > As the code stands, a restore involving blobs plus --single-transaction > produces > BEGIN; > ... > BEGIN; > ... > COMMIT; > ... > COMMIT; > which does *not* have the intended behavior because BEGIN does not nest. > This is a must-fix, else we may as well revert the patch entirely, > because it does not work. How should it work? 1. Remove the BEGIN and COMMIT around blobs? 2. Use SAVEPOINT ? Presumably (1). Best Regards, Simon Riggs
Simon Riggs <simon@2ndquadrant.com> writes:
> How should it work?
> 1. Remove the BEGIN and COMMIT around blobs?
> 2. Use SAVEPOINT ?
> Presumably (1).
Yeah, there is no need for the per-blob begin/commit if we've got one
around the whole restore.
One thing to be careful of is not to suppress BEGINs that are sent on
the dumping side --- it's sometimes hard to tell which parts of pg_dump
execute when.
regards, tom lane
> Perhaps you can explain further? My understanding was that the desired > functionality was that any failure would cause all aspects of the load > to fail also, so I don't see any problem with that; clearly I need to > listen to your thoughts. Maybe we're not talking about the same thing? I was talking about the begin/commit statements that are already in the dump - doesn't wrapping the entire lot guarantee failure when you have: begin; begin; commit; commit; ? Chris
On Tue, 2006-02-14 at 23:01 +0800, Christopher Kings-Lynne wrote: > > Perhaps you can explain further? My understanding was that the desired > > functionality was that any failure would cause all aspects of the load > > to fail also, so I don't see any problem with that; clearly I need to > > listen to your thoughts. > > Maybe we're not talking about the same thing? I was talking about the > begin/commit statements that are already in the dump - doesn't wrapping > the entire lot guarantee failure when you have: begin; begin; commit; > commit; ? Tom 'splained. I thought you meant the behaviour, rather than the invocation. Best Regards, Simon Riggs
On Tue, 2006-02-14 at 09:57 -0500, Tom Lane wrote: > Simon Riggs <simon@2ndquadrant.com> writes: > > How should it work? > > > 1. Remove the BEGIN and COMMIT around blobs? > > 2. Use SAVEPOINT ? > > > Presumably (1). > > Yeah, there is no need for the per-blob begin/commit if we've got one > around the whole restore. Just put an if test around that blob-handling behaviour. The code looks very simple, so patch enclosed to augment the already-applied patch. Chris, do you have a set-up to test out the blob behaviour? If your using them in production you'll spot any further slip-ups; they weren't intentionally ignored in the original patch. > One thing to be careful of is not to suppress BEGINs that are sent on > the dumping side --- it's sometimes hard to tell which parts of pg_dump > execute when. Not touched, nothing fancy going on. [It's a shame we don't support nested BEGINs, for use in nested function calls. I guess we took that out infavour of SAVEPOINTs? I seem to remember some idiot (me wasn't it?) suggesting we should do that.] Best Regards, Simon Riggs
Attachment
Simon Riggs <simon@2ndquadrant.com> writes:
> [It's a shame we don't support nested BEGINs, for use in nested function
> calls. I guess we took that out infavour of SAVEPOINTs? I seem to
> remember some idiot (me wasn't it?) suggesting we should do that.]
I still think that was probably the right move, though. With nested
BEGIN/COMMIT it'd be way too easy to get confused about how many levels
deep you are. The savepoint syntax at least provides names for the
levels. (Not that we have any mechanism to expose the state to you :-()
regards, tom lane
Simon Riggs <simon@2ndquadrant.com> writes:
> Just put an if test around that blob-handling behaviour. The code looks
> very simple, so patch enclosed to augment the already-applied patch.
Isn't the end-restore test backwards?
> Chris, do you have a set-up to test out the blob behaviour?
Just do a couple of lo_imports into a test database, dump, look at
pg_restore's output to see if it's sane ...
regards, tom lane
On Tue, 2006-02-14 at 12:32 -0500, Tom Lane wrote: > Simon Riggs <simon@2ndquadrant.com> writes: > > Just put an if test around that blob-handling behaviour. The code looks > > very simple, so patch enclosed to augment the already-applied patch. > > Isn't the end-restore test backwards? <sigh> Yup </sigh> > > Chris, do you have a set-up to test out the blob behaviour? > > Just do a couple of lo_imports into a test database, dump, look at > pg_restore's output to see if it's sane ... OK Best Regards, Simon Riggs
On Tue, 2006-02-14 at 12:32 -0500, Tom Lane wrote: > Just do a couple of lo_imports into a test database, dump, look at > pg_restore's output to see if it's sane ... Thats interesting. My testing shows there is a problem with HEAD that goes back a few versions, but works with 8.1.1. Tracing it now. Best Regards, Simon Riggs
On Tue, 2006-02-14 at 12:32 -0500, Tom Lane wrote: > Simon Riggs <simon@2ndquadrant.com> writes: > > Just put an if test around that blob-handling behaviour. The code looks > > very simple, so patch enclosed to augment the already-applied patch. > > Isn't the end-restore test backwards? Corrected in enclosed patch. Another problem found and fixed also. > > Chris, do you have a set-up to test out the blob behaviour? > > Just do a couple of lo_imports into a test database, dump, look at > pg_restore's output to see if it's sane ... Tests pass. BEGIN/COMMIT in correct places in log files (included). Tests enclosed: without single transaction ./test.sh > 0.log with single transaction ./test.sh -1 > 1.log Also tested with direct database connection, which works both in error and success. [My earlier problem was a library version mismatch from the recent update of the COPY protocol, thankfully.] Best Regards, Simon Riggs
Attachment
Simon Riggs <simon@2ndquadrant.com> writes:
>> Just do a couple of lo_imports into a test database, dump, look at
>> pg_restore's output to see if it's sane ...
> Tests pass. BEGIN/COMMIT in correct places in log files (included).
Applied, thanks.
regards, tom lane
> Chris, do you have a set-up to test out the blob behaviour? If your > using them in production you'll spot any further slip-ups; they weren't > intentionally ignored in the original patch. No sorry - my PostgreSQL contribution these days primary consists of "whining" :D I usually just do this: create database test; \c test \lo_import /etc/rc 'This is a comment' Then pg_dump that database. Chris