pgsql: Add psql option: -1 or --single-transaction Simon Riggs

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

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