Re: [Xen-devel] [HACKERS] [OSSTEST PATCH 0/1] PostgreSQL db: Retry on constraint violation [and 2 more messages]

[Kevin Grittner <kgrittn@xxxxxxxxx>]

On Tue, Dec 13, 2016 at 9:50 AM, Ian Jackson <ian.jackson@xxxxxxxxxxxxx> wrote:
> Kevin Grittner writes ("Re: [HACKERS] [OSSTEST PATCH 0/1] PostgreSQL db: 
> Retry on constraint violation [and 2 more messages]"):
>> On Tue, Dec 13, 2016 at 5:30 AM, Ian Jackson <ian.jackson@xxxxxxxxxxxxx> 
>> wrote:
>>> Are all of these cases fixed by fcff8a57519847 "Detect SSI conflicts
>>> before reporting constraint violations" ?
>>
>> No.  It was specifically meant to address duplicate keys, and there
>> was one particular set of steps which it was not able to address.
>> See post by Thomas Munro.  Hopefully, he, I, or someone else will
>> have a chance to work on the one known remaining issue and look for
>> others.  Your efforts have been helpful; it would be great if you
>> can find and document any other test cases which show a
>> less-than-ideal SQLSTATE or other outright serialization anomalies.
>
> Well, my own usage of postgresql is not really that advanced and we do
> not have very many complicated constraints.  So for projects I'm
> responsible for, what has been done in 9.6 is going to be good enough
> in practice (when it finally bubbles through via my distro etc.); and
> I have a workaround for now.

I worked in a shop with over 30,000 types of transactions, and
*none* of them involved using a subtransaction to catch and ignore
an attempted constraint violation.  I know of several larger shops
which have not experienced such problems, either.  I agree you
found a bug and it needs to be fixed, but it doesn't seem to be a
bug that occurs in common usage patterns; otherwise, it would
probably have taken less than five years for it to be found.

> But I am trying to save others (who may have more complicated
> situations) from being misled.

A motive I certainly appreciate.

>>> All statements in such transactions, even aborted transactions, need
>>> to see results, and have behaviour, which are completely consistent
>>> with some serialisaton of all involved transactions.  This must apply
>>> up to (but not including) any serialisation failure error.
>>
>> If I understand what you are saying, I disagree.
>
> But I have just demonstrated a completely general technique which can
> be used to convert any "spurious" constraint failure into a nonsense
> transaction actually committed to the database.

Really?  I see a lot of daylight between "you must discard any
results from a transaction which later throws a serialization
failure" and "if you catch the exception from a constraint
violation within a subtransaction it is possible to create a
serialization anomaly".  I don't actually see how one relates to
the other.  What am I missing?

> I think there are only two possible coherent positions:
>
> 1. Declare that all spurious failures, in SERIALIZABLE transactions,
> are bugs.

It's not clear to me what you mean here.

> 2. State that the SERIALIZABLE guarantee in Postgresql only applies to
> transactions which (a) complete successsfully and (b) contain only
> very simple pgsql constructs.

About 24 hours ago you reported a bug which was hard enough to hit
that it took 5 years for anyone to find it.  I think you might
consider allowing a bit more time to analyze the situation before
declaring that serializable transactions only work "in very simple
constructs".  Some of the transactions with which I have seen it
reliably work involved in excess of 20,000 lines of procedural
code.  What it seems to me we should do is try to identify the
conditions under which a bug can occur (which seems likely to be
limited to a subset of cases where errors thrown by declarative
constraints are caught within a subtransaction, and the
subtransaction work is discarded without throwing another error),
and see whether the bugs can be fixed.

> I think (2) would be rather a bad concession!  (It is probably also
> contrary to some standards document somewhere, if that matters.)
> Furthermore if you adopt (2) you would have to make a list of the
> "safe" pgsql constructs.

I think the unsafe constructs are likely to be a *much* shorter list.

> That would definitely exclude the exception trapping facility; but
> what other facilities or statements might be have similar effects ?
> ISTM that very likely INSERT ... ON CONFLICT can be used the same way.
> Surely you do not want to say "PostgreSQL does not give a
> transactional guarantee when INSERT ... ON CONFLICT is used".

I think we've chased down the bugs in this area.  Counterexamples
welcome.

>>  To prevent incorrect results from being returned even when a
>> transaction later fails with a serialization failure would require
>> blocking
>
> I'm afraid I don't understand enough about database implementation to
> answer this with confidence.  But this does not seem likely to be
> true.  Blocking can surely always be avoided, by simply declaring a
> serialisation failure instead of blocking.  I have no idea whether
> that is a practical approach in the general case, or whether it brings
> its own problems.

The "false positive" rate (throwing a serialization failure error
when there would not have been a serialization anomaly without the
error) would be horrible.  I don't think performance would be
acceptable to very many.

As a side-note, PostgreSQL originally used S2PL, and therefore
provided exactly the guarantees you're looking for (modulus any
bugs which might have been there); but that was ripped out 18 years
ago due to the horrible performance, in favor of providing snapshot
isolation.

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=3f7fbf85dc5b42dfd33c803efe6c90533773576a

Personally, I don't think the community properly understood what
they were giving up in doing so, and the documentation glossed over
the differences until I made an effort to give people a clearer
idea of the situation.  During discussions, the paper on the
Serializable Snapshot Isolation technique was pointed out (it won
the "Best Paper" award at ACM SIGMOD 2008, a very prestigious
conference in the database world) and I jumped on using that to try
to improve the behavior, rather than just the docs.

> But whether or not it is true, I think that "it's hard" is not really
> a good answer to "PostgreSQL should implement behaviour for
> SERIALIZABLE which can be coherently described and which covers
> practically useful use cases".

The behavior we have been providing, modulus the bug you just found
and any related bugs of the same ilk, is "we don't allow
serialization anomalies into the database or in results from
serializable transactions which commit."  That is enough to be very
useful to many.

>>> I am concerned that there are other possible bugs of this form.
>>> In earlier messages on this topic, it has been suggested that the
>>> "impossible" unique constraint violation is only one example of a
>>> possible "leakage".
>>
>> As I see it, the main point of serializable transactions is to
>> prevent serialization anomalies from being persisted in the
>> database or seen by a serializable transaction which successfully
>> commits.
>
> As I have demonstrated, "spurious error" conditions can result in
> "serialisation anomaly persisted in the database".  So there is not a
> real distinction here.

You found a bug.  Thanks.  Let's not exaggerate the issue; let's fix it.

> There is another very important use case for serialisable transactions
> which is read-only report transactions.  An application doing such a
> transaction needs to see a consistent snapshot.  Such a transaction is
> readonly so will never commit.

Yes, it will.  It can also get a serialization failure before it
does unless you use the DEFERRABLE option.

https://www.postgresql.org/docs/current/static/sql-set-transaction.html

| The DEFERRABLE transaction property has no effect unless the
| transaction is also SERIALIZABLE and READ ONLY. When all three of
| these properties are selected for a transaction, the transaction
| may block when first acquiring its snapshot, after which it is
| able to run without the normal overhead of a SERIALIZABLE
| transaction and without any risk of contributing to or being
| canceled by a serialization failure. This mode is well suited for
| long-running reports or backups.

> Any reasonable definition of what SERIALIZABLE means needs to give a
> sensible semantics for readonly transactions.

I certainly agree.  I don't see where it doesn't.

>> Well, the test includes commits and teardown, but this gets you to
>> the problem.  Connection2 gets this:
>>
>> ERROR:  duplicate key value violates unique constraint "invoice_pkey"
>> DETAIL:  Key (year, invoice_number)=(2016, 3) already exists.
>
> I think my error trapping technique can be used to convert this into
> a scenario which commits "impossible" information to the database.
>
> Do you agree ?

It seems likely, which converts this from a "convenience feature
request" to a bug, as of yesterday.  You are certainly very
fortunate if you have worked for a software company which never had
a bug that took longer than a day to fix.

>> If connection1 had explicitly read the "gap" into which it inserted
>> its row (i.e., with a SELECT statement) there would be a
>> serialization failure instead.  Getting the RI index maintenance to
>> register as a read for this purpose is a bit tricky, and we don't
>> yet have a working patch for that.
>
> Of course I understand that these problems may be hard to solve.  But,
> I think it needs to be recognised that these problems are all bugs.

Yes, when they are shown to allow serialization anomalies, they
certainly are bugs.  That has now happened.

> And, then, we should aim to have an understanding of what the scope of
> these bugs are.  That way something can be written in the
> documentation.

You sure don't aim very high.  Given a higher priority now that
they have been shown to *be* bugs, I expect that they will be fixed
fairly quickly.

> "If you do not use features X or Y, these bugs will not affect you" is
> a useful thing to perhaps say, even if X or Y is as general as "INSERT
> ... ON CONFLICT" or "... EXCEPTION WHEN ...".

Well, if they are not fixed soon, it might be worth adding
something to the docs; but as far as I can see, the specifics would
be more like "If you catch errors generated by declarative
constraints or unique index insertion within a subtransaction, and
then discard the work of the subtransaction without throwing any
error, you might create a serialization anomaly, depending on what
else you do within the transaction."  Have you seen anything that
suggests that this is not a complete description of the scope of
the issue?  I don't see any point in a statement that there might
be undiscovered bugs in the software, either in general or relative
to any particular feature.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel