Thread: Statement-level rollback
I would like to bring up again the topic of statement-level rollback. This was discussed in some depth at [1]. This patch is not based on Tsunakawa-san's patch submitted in that thread; although I started from it, I eventually removed almost everything and replaced with a completely different implementation. Patch 0001 here attached introduces the new setting and behavior with a few applicability restrictions I outlined in [2], on safety grounds. However, that wasn't exactly met with the standing ovation that I was expecting[3], so patch 0002 removes those and changes the behavior to that of any other GUC -- so much so, in fact, that after that patch you can change the rollback scope in the middle of a transaction, and it affects from that point onwards. There is a definite user demand for this feature; almost anyone porting nontrivial apps from other DBMS systems will want this to be an option. Of course, the default behavior doesn't change. Some commercial DBMSs offer only the behavior that this patch introduces. While they aren't universally loved, they have lots of users, and many of those users have migrated or are migrating or will migrate to Postgres in some form or another. Adding this feature lowers the barrier to such migrations, which cannot be but a good thing. I think we should add this, and if that means some tools will need to add a SET line to ensure they get the behavior they need in case careless DBAs enable this behavior server-wide, that seems not a terribly onerous cost anyway. [1] https://postgr.es/m/0A3221C70F24FB45833433255569204D1F6A9286@G01JPEXMBYT05 [2] https://postgr.es/m/20180615202328.7m46qo46v5a5wkd2@alvherre.pgsql [3] https://postgr.es/m/CA+TgmoavJybY0C8LXHZNcw4p=Eh48yoZwbjq8HVa10qhuP=gpg@mail.gmail.com -- Álvaro Herrera PostgreSQL Expert, https://www.2ndQuadrant.com/
Attachment
Hi,
On 2018-12-07 16:20:06 -0300, Alvaro Herrera wrote:
> case TBLOCK_BEGIN:
> + s->rollbackScope = XactRollbackScope;
> s->blockState = TBLOCK_INPROGRESS;
> + if (s->rollbackScope == XACT_ROLLBACK_SCOPE_STMT)
> + {
> + PushTransaction();
> + s = CurrentTransactionState; /* changed by push */
> + s->name = MemoryContextStrdup(TopTransactionContext, "pg internal");
> + StartSubTransaction();
> + s->blockState = TBLOCK_SUBINPROGRESS;
> + }
Isn't this going to be performing ridiculously bad, to the point of
being not much but a trap for users?
I can see the feature being useful, but I don't think we should accept a
feature that'll turn bulkloading with insert into a server shutdown
scenario.
Greetings,
Andres Freund
On 2018-Dec-07, Andres Freund wrote:
> Hi,
>
> On 2018-12-07 16:20:06 -0300, Alvaro Herrera wrote:
> > case TBLOCK_BEGIN:
> > + s->rollbackScope = XactRollbackScope;
> > s->blockState = TBLOCK_INPROGRESS;
> > + if (s->rollbackScope == XACT_ROLLBACK_SCOPE_STMT)
> > + {
> > + PushTransaction();
> > + s = CurrentTransactionState; /* changed by push */
> > + s->name = MemoryContextStrdup(TopTransactionContext, "pg internal");
> > + StartSubTransaction();
> > + s->blockState = TBLOCK_SUBINPROGRESS;
> > + }
>
> Isn't this going to be performing ridiculously bad, to the point of
> being not much but a trap for users?
If you bulk-load with INSERT under this behavior, yeah it'll create lots
of subtransactions, with the obvious downsides -- eating lots of xids
for one. I think the answer to that is "don't do that". However, if
you want to do 100k inserts and have the 10 bogus of those fail cleanly
(and not abort the other 99990), that's clearly this patch's intention.
> I can see the feature being useful, but I don't think we should accept a
> feature that'll turn bulkloading with insert into a server shutdown
> scenario.
Hm.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi,
On December 7, 2018 11:44:17 AM PST, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>On 2018-Dec-07, Andres Freund wrote:
>
>> Hi,
>>
>> On 2018-12-07 16:20:06 -0300, Alvaro Herrera wrote:
>> > case TBLOCK_BEGIN:
>> > + s->rollbackScope = XactRollbackScope;
>> > s->blockState = TBLOCK_INPROGRESS;
>> > + if (s->rollbackScope == XACT_ROLLBACK_SCOPE_STMT)
>> > + {
>> > + PushTransaction();
>> > + s = CurrentTransactionState; /* changed by push */
>> > + s->name = MemoryContextStrdup(TopTransactionContext, "pg
>internal");
>> > + StartSubTransaction();
>> > + s->blockState = TBLOCK_SUBINPROGRESS;
>> > + }
>>
>> Isn't this going to be performing ridiculously bad, to the point of
>> being not much but a trap for users?
>
>If you bulk-load with INSERT under this behavior, yeah it'll create
>lots
>of subtransactions, with the obvious downsides -- eating lots of xids
>for one. I think the answer to that is "don't do that". However, if
>you want to do 100k inserts and have the 10 bogus of those fail cleanly
>(and not abort the other 99990), that's clearly this patch's intention
>> I can see the feature being useful, but I don't think we should
>accept a
>> feature that'll turn bulkloading with insert into a server shutdown
>> scenario.
>
>Hm.
Isn't the realistic scenario for migrations that people will turn this feature on on a more global basis? If they need
todo per transaction choices, that makes this much less useful for easy migrations.
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Hi On 2018-Dec-07, Andres Freund wrote: > Isn't the realistic scenario for migrations that people will turn this > feature on on a more global basis? If they need to do per transaction > choices, that makes this much less useful for easy migrations. The way I envision this to be used in practice is that it's enabled globally when the migration effort starts, then gradually disabled as parts of applications affected by these downsides are taught how to deal with the other behavior. BTW, a couple of months ago I measured the performance implications for a single update under pgbench and it represented a decrease of about 3%-5%. Side-effects such as xid consumption have worse implications, but as far as performance is concerned, it's not as bad as all that. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On December 7, 2018 11:56:55 AM PST, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >BTW, a couple of months ago I measured the performance implications for >a single update under pgbench and it represented a decrease of about >3%-5%. Side-effects such as xid consumption have worse implications, >but as far as performance is concerned, it's not as bad as all that. I don't think that's a fair test for the performance downsides. For pgbench with modifications the full commit is such alarge portion of the time that you'd need to make things a lot slower to show a large slowdown. And for ro pgbench the subxactsdon't matter that much. It'd probably be more meaningful to have a mixed workload of 15 ro statements per xact inone type of session, and 5rw /10ro in another. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
On 2018-Dec-07, Andres Freund wrote:
> On December 7, 2018 11:56:55 AM PST, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> >BTW, a couple of months ago I measured the performance implications for
> >a single update under pgbench and it represented a decrease of about
> >3%-5%. Side-effects such as xid consumption have worse implications,
> >but as far as performance is concerned, it's not as bad as all that.
>
> I don't think that's a fair test for the performance downsides. For
> pgbench with modifications the full commit is such a large portion of
> the time that you'd need to make things a lot slower to show a large
> slowdown. And for ro pgbench the subxacts don't matter that much. It'd
> probably be more meaningful to have a mixed workload of 15 ro
> statements per xact in one type of session, and 5rw /10ro in another.
I just reviewed the old emails where I reported some performance
figures. Turns out I tried pgbench with varying number of inserts per
transaction:
nr_inserts │ transaction │ statement │ decrease
────────────┼─────────────┼───────────┼────────────────────────
10 │ 20674.12 │ 19983.33 │ 3.34132722456868780900
63 │ 3704.95 │ 3613.39 │ 2.47128841144954722700
64 │ 3646.99 │ 3553.79 │ 2.55553209633149528800
65 │ 3582.17 │ 3503.47 │ 2.19699232588068126300
66 │ 3513.27 │ 3423.34 │ 2.55972356237920797400
80 │ 2906.50 │ 2833.80 │ 2.50129021159470153100
100 │ 2337.58 │ 2272.01 │ 2.80503768854969669500
200 │ 1171.06 │ 1143.22 │ 2.37733335610472563300
500 │ 464.55 │ 451.04 │ 2.90819072220428371500
1000 │ 229.79 │ 223.66 │ 2.66765307454632490500
10000 │ 22.02 │ 21.43 │ 2.67938237965485921900
(The numbers around 64 were tested to make sure we weren't particularly
in trouble when we hit PGPROC subxid cache size.)
In a different test, I used HOT updates, and I said "This works out to
be between 4.3% and 5.6% in the four cases, which is not growing with
the number of ops per transaction."
nr_updates │ transaction │ statement
────────────┼──────────────────┼─────────────────
10 │ 6714.67 +-114.37 │ 6362.89 +-99.96
100 │ 699.19 +-7.20 │ 671.64 +-7.75
200 │ 359.92 +-0.86 │ 342.07 +-8.34
500 │ 143.11 +-2.47 │ 135.14 +-3.21
1000 │ 70.07 +-2.61 │ 67.04 +-1.52
I didn't measure read-only statements. I definitely expect that case to
have a larger performance decrease; however, that's the kind of code
site that, if it becomes a hot path, you'd change from the slow mode to
the fast mode.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Dec 7, 2018 at 2:57 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > The way I envision this to be used in practice is that it's enabled > globally when the migration effort starts, then gradually disabled as > parts of applications affected by these downsides are taught how to deal > with the other behavior. I'm not sure how realistic that is, honestly. I think if you add this feature, people are going to turn it on and leave it on. But even if you're right and people only use it temporarily, you'd really only need to do one large bulk load to break the whole system. A five-minute bulk load could burn through tens of millions of XIDs, and some EDB customers, at least, do loads that last many hours. Full disclosure: EDB has a feature like this and has for years, but it uses a subtransaction per statement, not a subtransaction per row. It is indeed useful to customers, but it also does cause its share of problems. It is *very* easy to burn through a *lot* of XIDs this way, even with a subtransaction per statement. For example, PL code in function A can call PL code in function B which can call PL code in function C, and you throw in a loop here and an EXCEPTION block there and all kinds of fun ensues. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2018-Dec-07, Robert Haas wrote: > Full disclosure: EDB has a feature like this and has for years, but it > uses a subtransaction per statement, not a subtransaction per row. Well, this implementation only uses one subtransaction per statement; Andres says per-row referring to the case of one INSERT per row, so it's still one statement. > It is indeed useful to customers, but it also does cause its share of > problems. It is *very* easy to burn through a *lot* of XIDs this way, > even with a subtransaction per statement. For example, PL code in > function A can call PL code in function B which can call PL code in > function C, and you throw in a loop here and an EXCEPTION block there > and all kinds of fun ensues. Yeah, I agree that this downside is real. I think our only protection against that is to say "don't do that". Like any other tool, it has upsides and downsides; we shouldn't keep it away from users only because they might misuse it. I would be interested to know if the EDB implementation does something in a better way than this one; then we can improve ours. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Dec 7, 2018 at 3:34 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Yeah, I agree that this downside is real. I think our only protection > against that is to say "don't do that". Like any other tool, it has > upsides and downsides; we shouldn't keep it away from users only because > they might misuse it. I have a hard time arguing against that given that EDB has this thing in our bag of tricks, but if it weren't for that I'd be fighting against this tooth and nail. Behavior-changing GUCs suuuuck. More generally, whether or not we should "keep something away from our users" really depends on how likely the upsides are to occur relative to the downsides. We don't try to keep users from running DELETE because they might delete data they want; that would be nanny-ism. But we do try to keep them from reading dirty data from an uncommitted transaction because we can't implement that without a risk of server crashes, and that's too big a downside to justify the upside. If we could do it safely, we might. From that point of view, this is doubtless not the worst feature PostgreSQL will ever have, but it sure ain't the best. > I would be interested to know if the EDB implementation does something > in a better way than this one; then we can improve ours. I haven't studied yours (and don't have time for that right now) so can't comment, at least not now. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2018-Dec-07, Robert Haas wrote: > More generally, whether or not we should "keep something away from our > users" really depends on how likely the upsides are to occur relative > to the downsides. We don't try to keep users from running DELETE > because they might delete data they want; that would be nanny-ism. > But we do try to keep them from reading dirty data from an uncommitted > transaction because we can't implement that without a risk of server > crashes, and that's too big a downside to justify the upside. If we > could do it safely, we might. > > From that point of view, this is doubtless not the worst feature > PostgreSQL will ever have, but it sure ain't the best. Well, look at this from this point of view: EnterpriseDB implemented this because of customer demand (presumably). Fujitsu also implemented this for customers. The pgjdbc driver implemented this for its users. Now 2ndQuadrant also implemented this, and not out of the goodness of our hearts. Is there any room to say that there is no customer demand for this feature? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 12/7/18 12:50 PM, Alvaro Herrera wrote: > On 2018-Dec-07, Robert Haas wrote: > >> More generally, whether or not we should "keep something away from our >> users" really depends on how likely the upsides are to occur relative >> to the downsides. We don't try to keep users from running DELETE >> because they might delete data they want; that would be nanny-ism. >> But we do try to keep them from reading dirty data from an uncommitted >> transaction because we can't implement that without a risk of server >> crashes, and that's too big a downside to justify the upside. If we >> could do it safely, we might. >> >> From that point of view, this is doubtless not the worst feature >> PostgreSQL will ever have, but it sure ain't the best. > Well, look at this from this point of view: EnterpriseDB implemented > this because of customer demand (presumably). Fujitsu also implemented > this for customers. The pgjdbc driver implemented this for its users. > Now 2ndQuadrant also implemented this, and not out of the goodness of > our hearts. Is there any room to say that there is no customer demand > for this feature? Amazon also implemented something similar for the database migration tool. I am unsure if they do it similarly with the DMS. With the DMT it definitely had the XID issue that some are concerned with but I would argue that is the cost of doing business. JD > -- Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc *** A fault and talent of mine is to tell it exactly how it is. *** PostgreSQL centered full stack support, consulting and development. Advocate: @amplifypostgres || Learn: https://postgresconf.org ***** Unless otherwise stated, opinions are my own. *****
Robert Haas <robertmhaas@gmail.com> writes:
> I have a hard time arguing against that given that EDB has this thing
> in our bag of tricks, but if it weren't for that I'd be fighting
> against this tooth and nail. Behavior-changing GUCs suuuuck.
Uh, we're not seriously considering a GUC that changes transactional
behavior are we? I thought we learned our lesson about that from the
autocommit fiasco. I'm not quite going to say "that'll go in over my
dead body", but I *urgently* recommend finding a less fragile way
to do it.
In a quick look at the patch, it seems that it has a BEGIN/START
TRANSACTION option, which perhaps would do for the "less fragile"
way; the problem is that it's also adding a GUC. Maybe we could
make the GUC read-only, so that it's only a reporting mechanism
not a twiddlable knob? (BTW, if it's not GUC_REPORT, you are
missing a large bet; that would at least make it *possible* for
clients to not be broken by this, even if it would still be an
unreasonable amount of work for them to cope with it.)
regards, tom lane
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Well, look at this from this point of view: EnterpriseDB implemented
> this because of customer demand (presumably). Fujitsu also implemented
> this for customers. The pgjdbc driver implemented this for its users.
> Now 2ndQuadrant also implemented this, and not out of the goodness of
> our hearts. Is there any room to say that there is no customer demand
> for this feature?
Yeah, but there is also lots of demand for stability in basic
transactional semantics. I refer you again to the AUTOCOMMIT business;
there were a lot of claims that that wouldn't break too much, and they
were all wrong.
regards, tom lane
Hi, On 2018-12-07 16:02:53 -0500, Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > Well, look at this from this point of view: EnterpriseDB implemented > > this because of customer demand (presumably). Fujitsu also implemented > > this for customers. The pgjdbc driver implemented this for its users. > > Now 2ndQuadrant also implemented this, and not out of the goodness of > > our hearts. Is there any room to say that there is no customer demand > > for this feature? > > Yeah, but there is also lots of demand for stability in basic > transactional semantics. I refer you again to the AUTOCOMMIT business; > there were a lot of claims that that wouldn't break too much, and they > were all wrong. I think it could partially be addressed by not allowing to set it on the commandline, server config, etc. So the user would have to set it on a per-connection basis, potentially via the connection string. I'm quite concerned how this'll make it much harder to write UDFs that work correctly. If suddenly the error handling expected doesn't work anymore - because they an error just skips over a statements - a lot of things will break in hard to understand ways. Greetings, Andres Freund
On Fri, Dec 7, 2018 at 11:34 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > On 2018-Dec-07, Robert Haas wrote: > > Full disclosure: EDB has a feature like this and has for years, but it > > uses a subtransaction per statement, not a subtransaction per row. > > Well, this implementation only uses one subtransaction per statement; > Andres says per-row referring to the case of one INSERT per row, so it's > still one statement. I'd like to add my 2 cents. I think usage of subtransaction per statement for statement level rollback is fair. It's unlikely we're going to invent something better in this part. The issue here is that our subtransaction mechanism is not effective enough to use it for every statement. Especially when there are many statements and each of them touches the only row. But we can improve that. The first thing, which comes to the mind is undo log. When you have undo log, then on new subtransaction you basically memorize the current undo log position. If subtransaction rollbacks, you have to just play back undo log until reach memorized position. This might be an option for zheap. But actually, I didn't inspect zheap code to understand how far we're from this. But nevetheless, we also have to do something with our heap, which is not undo-based storage. I think it is possible to implement a compromise solution, which allows to accelerate small subtransactions on heap. Instead of allocating xid for subtransaction immediately on write, we can start with using top transaction xid. But also allocate small memory buffer to memorize locations of heap tuples added or modified by this subtransaction. If we wouldn't overflow this small buffer, we can use it to rollback state of heap on subtransaction abort. Otherwise, we can decide to nevertheless allocate xid for this subtransaction. But then xid allocation would happen only for large-enough subtransactions. If there is an interest in this, I can write a POC. ------ Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
On Fri, Dec 7, 2018 at 7:25 PM Alexander Korotkov <a.korotkov@postgrespro.ru> wrote: > The first thing, which comes to the mind is undo log. When you have > undo log, then on new subtransaction you basically memorize the > current undo log position. If subtransaction rollbacks, you have to > just play back undo log until reach memorized position. This might be > an option for zheap. But actually, I didn't inspect zheap code to > understand how far we're from this. Yeah, zheap works this way. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Dec 7, 2018 at 3:50 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > On 2018-Dec-07, Robert Haas wrote: > > More generally, whether or not we should "keep something away from our > > users" really depends on how likely the upsides are to occur relative > > to the downsides. We don't try to keep users from running DELETE > > because they might delete data they want; that would be nanny-ism. > > But we do try to keep them from reading dirty data from an uncommitted > > transaction because we can't implement that without a risk of server > > crashes, and that's too big a downside to justify the upside. If we > > could do it safely, we might. > > > > From that point of view, this is doubtless not the worst feature > > PostgreSQL will ever have, but it sure ain't the best. > > Well, look at this from this point of view: EnterpriseDB implemented > this because of customer demand (presumably). Fujitsu also implemented > this for customers. The pgjdbc driver implemented this for its users. > Now 2ndQuadrant also implemented this, and not out of the goodness of > our hearts. Is there any room to say that there is no customer demand > for this feature? There is no room at all for doubt on that point. The issue, at least IMV, is collateral damage -- customers surely want it. Indeed, if we'd been smarter, maybe we would've invented a way to make it work like that from the beginning. But changing it now, even as an optional behavior, will (as Tom and Andres are rightly saying) mean that every tool and extension author potentially has a problem with things not working as expected. One can make an argument that such pain is worth enduring, or that it isn't. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2018-Dec-07, Andres Freund wrote: > I think it could partially be addressed by not allowing to set it on the > commandline, server config, etc. So the user would have to set it on a > per-connection basis, potentially via the connection string. This is what patch 0001 does -- it's only allowed in the connection string, or on ALTER USER / ALTER DATABASE. Setting it in postgresql.conf is forbidden, as well as changing from transaction to statement in SET (the opposite is allowed, though.) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, On 2018-12-08 17:10:27 -0300, Alvaro Herrera wrote: > On 2018-Dec-07, Andres Freund wrote: > > > I think it could partially be addressed by not allowing to set it on the > > commandline, server config, etc. So the user would have to set it on a > > per-connection basis, potentially via the connection string. > > This is what patch 0001 does -- it's only allowed in the connection > string, or on ALTER USER / ALTER DATABASE. Setting it in > postgresql.conf is forbidden, as well as changing from transaction to > statement in SET (the opposite is allowed, though.) I don't think allowing to set it on a per-user basis is acceptable either, it still leaves the client in a state where they'll potentially be confused about it. Do you have a proposal to address the issue that this makes it just about impossible to write UDFs in a safe way? Greetings, Andres Freund
On 2018-Dec-08, Andres Freund wrote: > On 2018-12-08 17:10:27 -0300, Alvaro Herrera wrote: > > This is what patch 0001 does -- it's only allowed in the connection > > string, or on ALTER USER / ALTER DATABASE. Setting it in > > postgresql.conf is forbidden, as well as changing from transaction to > > statement in SET (the opposite is allowed, though.) > > I don't think allowing to set it on a per-user basis is acceptable > either, it still leaves the client in a state where they'll potentially > be confused about it. Hmm, true. > Do you have a proposal to address the issue that this makes it just > about impossible to write UDFs in a safe way? Not yet, but I'll give it a think next week. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From: Andres Freund [mailto:andres@anarazel.de] > Isn't the realistic scenario for migrations that people will turn this > feature on on a more global basis? If they need to do per transaction choices, > that makes this much less useful for easy migrations. Agreed. My approach of per transaction choice may be overkill. Actually, I didn't think per-transaction change was reallynecessary in practice. But I didn't think of any reason to prohibit per transaction change, so I just wanted to allowflexibility. I think per database/user change (ALTER DATABASE/USER) can be useful, in cases where applications are migrated from otherDBMSs to a PostgreSQL instance. That is, database consolidation for easier data analytics and database management. One database/schema holds data for a PostgreSQL application, and another one stores data for a migrated application. Or, should we recommend a separate PostgreSQL instance on a different VM or container, and just introduce the parameter onlyin postgresql.conf? Regards Takayuki Tsunakawa
On 2018-12-08 17:55:03 -0300, Alvaro Herrera wrote: > On 2018-Dec-08, Andres Freund wrote: > > > On 2018-12-08 17:10:27 -0300, Alvaro Herrera wrote: > > > > This is what patch 0001 does -- it's only allowed in the connection > > > string, or on ALTER USER / ALTER DATABASE. Setting it in > > > postgresql.conf is forbidden, as well as changing from transaction to > > > statement in SET (the opposite is allowed, though.) > > > > I don't think allowing to set it on a per-user basis is acceptable > > either, it still leaves the client in a state where they'll potentially > > be confused about it. > > Hmm, true. > > > Do you have a proposal to address the issue that this makes it just > > about impossible to write UDFs in a safe way? > > Not yet, but I'll give it a think next week. Is this still in development? Or should we mark this as returned with feedback? Greetings, Andres Freund
On Fri 7 Dec 2018, 21:43 Robert Haas <robertmhaas@gmail.com wrote:
On Fri, Dec 7, 2018 at 3:34 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Yeah, I agree that this downside is real. I think our only protection
> against that is to say "don't do that". Like any other tool, it has
> upsides and downsides; we shouldn't keep it away from users only because
> they might misuse it.
I have a hard time arguing against that given that EDB has this thing
in our bag of tricks, but if it weren't for that I'd be fighting
against this tooth and nail. Behavior-changing GUCs suuuuck.
This looks like repeating the autocommit mistakes of the past.
And based on that experience wouldn't the replacement approach be to do this client side? If libpq had a library option to wrap every statement in a subtransaction by adding begin/end then this problem would be completely sidestepped.
On 2019-Jan-31, Greg Stark wrote: > This looks like repeating the autocommit mistakes of the past. Yeah, this argument has been made before. Peter E argued against that: https://postgr.es/m/ea395aa8-5ac4-6bcd-366d-aab2ff2b05ef@2ndquadrant.com > And based on that experience wouldn't the replacement approach be to do > this client side? JDBC has a feature for this already (see "autosave" in [1]), but apparently you get a good performance improvement by doing it server-side, which is why this was written in the first place. psql also has ON_ERROR_ROLLBACK [2]. > If libpq had a library option to wrap every statement in a > subtransaction by adding begin/end then this problem would be > completely sidestepped. I don't think anyone's talked about doing it in libpq before specifically, and if you did that, it would obviously have to be implemented separately on JDBC. [1] https://jdbc.postgresql.org/documentation/head/connect.html#connection-parameters [2] https://www.postgresql.org/docs/current/app-psql.html -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Jan 31, 2019 at 04:38:27AM -0800, Andres Freund wrote: > Is this still in development? Or should we mark this as returned with > feedback? Marked as returned with feedback, as it has already been two months. If you have an updated patch set, please feel free to resubmit. -- Michael
Attachment
On 2019-Feb-04, Michael Paquier wrote: > On Thu, Jan 31, 2019 at 04:38:27AM -0800, Andres Freund wrote: > > Is this still in development? Or should we mark this as returned with > > feedback? > > Marked as returned with feedback, as it has already been two months. > If you have an updated patch set, please feel free to resubmit. It was reasonable to close this patch as returned-with-feedback in January commitfest, but what you did here was first move it to the March commitfest and then close it as returned-with-feedback in the March commitfest, which has not even started. That makes no sense. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Feb 12, 2019 at 07:13:45PM -0300, Alvaro Herrera wrote: > It was reasonable to close this patch as returned-with-feedback in > January commitfest, but what you did here was first move it to the March > commitfest and then close it as returned-with-feedback in the March > commitfest, which has not even started. That makes no sense. Except if the CF app does not leave any place for error, particularly as it is not possible to move back a patch to a previous commit fest once it has been moved to the next one. In this case, it looks that I did a mistake in moving the patch first, my apologies for that. There were many patches to classify last week, so it may be possible that I did similar mistakes for some other patches. -- Michael